Re: [PATCH net-next v1 11/11] eth: fbnic: Support AF_XDP page pool zero copy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] eth: fbnic: wrong Rx queue index used for idle polling in
  fbnic_queue_stop() [1]
--

commit 94641017ff155e4aaace7b6d7179917c8e4199a7
Author: Björn Töpel <bjorn@kernel.org>

eth: fbnic: Support AF_XDP page pool zero copy

This commit adds AF_XDP zero-copy support to fbnic using a page pool
memory provider shared between header and payload rings, and transmits
socket descriptors on the XDP TX ring during NAPI polling.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index d96c957d6fbad..c358003f053b6 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c

[ ... ]
> @@ -3216,5 +3479,6 @@ const struct netdev_queue_mgmt_ops fbnic_queue_mgmt_ops = {
>  	.ndo_queue_stop		= fbnic_queue_stop,
>  	.ndo_default_qcfg	= fbnic_default_qcfg,
>  	.ndo_validate_qcfg	= fbnic_validate_qcfg,
> -	.supported_params	= QCFG_RX_PAGE_SIZE,
> +	.supported_params	= QCFG_RX_PAGE_SIZE | QCFG_RX_HEADROOM,
> +	.xsk_page_pool		= true,
>  };

[Severity: Critical]
This problem wasn't introduced by this patch, but does fbnic_queue_stop()
poll the wrong RX hardware queue index for idle state [1] when
stopping a non-zero RX queue (such as when binding an AF_XDP socket)?

drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:fbnic_queue_stop() {
	...
	for (t = 0; t < nv->txt_count + nv->rxt_count; t++) {
		err = fbnic_wait_queue_idle(fbn, t >= nv->txt_count,
					    nv->qt[t].sub0.q_idx);
		if (err)
			goto err_restart;
	}
	...
}

In fbnic_alloc_napi_vector(), RX queue triads initialize qt->sub0.q_idx to
0 and store rxq_idx in qt->cmpl.q_idx. Passing sub0.q_idx to
fbnic_wait_queue_idle() checks RX queue 0 instead of the target RX queue:
if queue 0 is idle while the target RX queue is still active,
fbnic_queue_stop() frees the active queue's resources while DMA is still in
flight, whereas if queue 0 is busy, stopping another RX queue can time out.

[1] https://sashiko.dev/#/bug/linux-197dd65d-fb9f-419a-afba-7621f586ae26

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110102039.1002605-1-bjorn@kernel.org?part=11