Re: [PATCH net-next v4 04/10] NTB: ntb_transport: Keep local QP link requests separate

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] A cleared QP up-request can be re-asserted by the QP's own
  link_work, leaving a closed QP advertised as 'up' in the peer's…

--

NTB: ntb_transport: Keep local QP link requests separate

This moves the local QP up-request bitmap out of the local scratchpad into
a new nt->up_request field, serializing the read-modify-write and the peer
QP_LINKS write under a new nt->up_request_lock. The bitmap is cleared in
ntb_transport_link_cleanup() so each QP re-advertises after reconnecting.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index b69e8ac8047df..0b47285ef48b6 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -1113,6 +1119,21 @@ static void ntb_transport_link_work(struct work_struct *work)
>  				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
>  }
>  
> +static void ntb_qp_up_request(struct ntb_transport_qp *qp, bool up)
> +{
> +	struct ntb_transport_ctx *nt = qp->transport;
> +
> +	guard(spinlock)(&nt->up_request_lock);
> +
> +	if (up)
> +		nt->up_request |= BIT(qp->qp_num);
> +	else
> +		nt->up_request &= ~BIT(qp->qp_num);
> +
> +	/* Update the peer's view of our requests. */
> +	ntb_peer_spad_write(nt->ndev, PIDX, QP_LINKS, nt->up_request);
> +}
> +
>  static void ntb_qp_link_work(struct work_struct *work)
>  {
>  	struct ntb_transport_qp *qp = container_of(work,
> @@ -1126,7 +1147,7 @@ static void ntb_qp_link_work(struct work_struct *work)
>  
>  	val = ntb_spad_read(nt->ndev, QP_LINKS);
>  
> -	ntb_peer_spad_write(nt->ndev, PIDX, QP_LINKS, val | BIT(qp->qp_num));
> +	ntb_qp_up_request(qp, true);

[Severity: Medium]
The up/down decision is still made by the caller outside up_request_lock,
so the last writer wins.  Can a closed QP end up advertised as up again
here?

ntb_qp_link_work() calls ntb_qp_up_request(qp, true) unconditionally, with
no test of qp->client_ready, while ntb_transport_link_down() clears the bit
first and only cancels the worker in the qp->link_is_up == false branch:

drivers/ntb/ntb_transport.c:ntb_transport_link_down() {
	qp->client_ready = false;

	ntb_qp_up_request(qp, false);

	if (qp->link_is_up)
		ntb_send_link_down(qp);
	else
		cancel_delayed_work_sync(&qp->link_work);
}

So with qp->link_is_up true, a queued or in-flight qp->link_work can run
after the clear and re-set BIT(qp->qp_num) in nt->up_request, republishing
it to the peer's QP_LINKS.

There is also a repeating path through the cleanup worker:

drivers/ntb/ntb_transport.c:ntb_qp_link_cleanup_work() {
	ntb_qp_link_cleanup(qp);

	if (nt->link_is_up)
		schedule_delayed_work(&qp->link_work,
				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
}

nt->link_is_up is set false only in ntb_transport_probe(), so after the
peer's LINK_DOWN payload is consumed (ntb_process_rxc() -> ntb_qp_link_down()
-> schedule_work(&qp->link_cleanup)) the QP's link_work keeps re-arming every
NTB_LINK_DOWN_TIMEOUT and keeps re-advertising a QP whose client already went
down.  Once that happens, is there any remaining publisher that would clear
the bit again short of a transport-wide cleanup?

The peer would then declare its side of the QP up and transmit into a queue
whose client is closed, and those frames get dropped.

Would it be more robust to exclude qp->link_work unconditionally (cancel or
flush before clearing), or to evaluate qp->client_ready inside
ntb_qp_up_request() under up_request_lock, rather than only making the
read-modify-write atomic?

For reference, the later patch in this series ("NTB: ntb_transport: Exchange
client capabilities at link-up") adds

	if (!smp_load_acquire(&qp->client_ready))
		return;

before the ntb_qp_up_request(qp, true) call and moves the
cancel_delayed_work_sync() ahead of the clear, which removes the repeating
cleanup-worker path.  A worker that already passed that client_ready load
can still publish the bit after ntb_transport_link_down() cleared it when
qp->link_is_up is true, since that branch skips the cancel.

>  	/* query remote spad for qp ready bits */
>  	dev_dbg_ratelimited(&pdev->dev, "Remote QP link status = %x\n", val);

[ ... ]

> @@ -2412,16 +2434,12 @@ EXPORT_SYMBOL_GPL(ntb_transport_link_up);
>   */
>  void ntb_transport_link_down(struct ntb_transport_qp *qp)
>  {
> -	int val;
> -
>  	if (!qp)
>  		return;
>  
>  	qp->client_ready = false;
>  
> -	val = ntb_spad_read(qp->ndev, QP_LINKS);
> -
> -	ntb_peer_spad_write(qp->ndev, PIDX, QP_LINKS, val & ~BIT(qp->qp_num));
> +	ntb_qp_up_request(qp, false);
>  
>  	if (qp->link_is_up)
>  		ntb_send_link_down(qp);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914084838.2158249-1-den%40valinux.co.jp