Re: [RFC PATCH net-next 1/2] icssg: Add multi-flow RX support with per-flow NAPI and classification

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] net: ti: icssg: hardcoded flow 0 and queue 0 in multi-flow XSK and XDP
- [Critical] net: ti: icssg: missing rx_flow_num bounds check in XSK pool and wakeup
- [High] net: ti: icssg: undersized shared RX FDQ ring in prueth_init_rx_chns()
- [High] net: ti: icssg: shared RX FDQ page leaks in prueth_destroy_rxq() teardown
- [High] net: ti: icssg: build breakage in icssg_prueth_sr1.c from RX API changes
- [High] net: ti: icssg: hang in prueth_destroy_rxq() on prueth_create_rxq() error
- [Medium] net: ti: icssg: unsigned irq check misses errors in prueth_init_rx_chns()
- [Medium] net: ti: icssg: emac_set_coalesce() only sets RX coalescing on flow 0
- [Medium] net: ti: icssg: prueth_destroy_rxqs() skips NAPI wakeup for non-XSK flows
- [Medium] net: ti: icssg: RX teardown and IRQ pacing bugs in prueth_destroy_rxqs()
- [Medium] net: ti: icssg: PRUSS DRAM leak in prueth_netdev_init() on bitmap alloc error
- [Medium] net: ti: icssg: allocate PRUETH_MAX_RX_FLOWS rx queues and set real_num_rx_queues
--

commit a2ae6a161ac2d7097a317321767fd99ea3cc193c
Author: Meghana Malladi <m-malladi@ti.com>

icssg: Add multi-flow RX support with per-flow NAPI and classification

This commit extends the ICSSG driver to support up to 8 RX flows per EMAC,
introducing per-flow NAPI, hrtimer, page pool, XDP rxq, AF_XDP pool, and
IRQ management while sharing a single UDMA RX channel and descriptor pool.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index 4a7d1a6f470b1..99f4353b46966 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c

[ ... ]

> @@ -584,40 +622,49 @@ int prueth_init_rx_chns(struct prueth_emac *emac,
>  				K3_UDMA_GLUE_SRC_TAG_LO_USE_REMOTE_SRC_TAG,
>  		};
>  
> +		flow = &rx_chn->flows[i];
> +		flow->emac = emac;
> +		flow->id = i;
> +		flow->irq = -EINVAL;
> +
>  		rx_flow_cfg.ring_rxfdq0_id = fdqring_id;
> -		rx_flow_cfg.rx_cfg.size = max_desc_num;
> -		rx_flow_cfg.rxfdq_cfg.size = max_desc_num;
> +		rx_flow_cfg.rx_cfg.size = PRUETH_MAX_RX_DESC;
> +		rx_flow_cfg.rxfdq_cfg.size = PRUETH_MAX_RX_DESC;

[Severity: High]
Does rx_flow_cfg.rxfdq_cfg.size need to be scaled to rx_chn->descs_num
(PRUETH_MAX_RX_DESC * rx_cfg.flow_id_num) here?

In prueth_init_rx_chns(), flow 0 allocates a shared FDQ ring with
K3_RINGACC_RING_SHARED and subsequent flows pass rx_flow_cfg.ring_rxfdq0_id
= fdqring_id. On UDMAP platforms such as AM654, k3_udma_glue_cfg_rx_flow()
shares flow 0's ringrxfdq across all flows, and k3_ringacc_ring_cfg() only
configures the ring size on the first user.

When emac->rx_flow_num > 1, prueth_prepare_rx_flow(emac, 0) pushes
PRUETH_MAX_RX_DESC (512) descriptors into the 512-entry shared FDQ ring,
completely filling it, so prueth_prepare_rx_flow(emac, 1) immediately fails
with -ENOMEM in k3_ringacc_ring_push().

>  		rx_flow_cfg.rxfdq_cfg.mode = emac->prueth->pdata.fdqring_mode;
>  
>  		ret = k3_udma_glue_rx_flow_init(rx_chn->rx_chn,
>  						i, &rx_flow_cfg);
>  		if (ret) {
>  			netdev_err(ndev, "Failed to init rx flow%d %d\n",
>  				   i, ret);
>  			goto fail;
>  		}
>  		if (!i)
>  			fdqring_id = k3_udma_glue_rx_flow_get_fdq_id(rx_chn->rx_chn,
>  								     i);
> -		ret = k3_udma_glue_rx_get_irq(rx_chn->rx_chn, i);
> -		if (ret < 0) {
> +		flow->irq = k3_udma_glue_rx_get_irq(rx_chn->rx_chn, i);
> +		if (flow->irq <= 0) {
> +			ret = flow->irq;
>  			netdev_err(ndev, "Failed to get rx dma irq");
>  			goto fail;
>  		}

[Severity: Medium]
In struct prueth_rx_flow, irq is declared as unsigned int. When
k3_udma_glue_rx_get_irq() returns a negative error code, assigning it
directly to flow->irq wraps it to a large positive value, causing
flow->irq <= 0 to evaluate to false and bypassing the error check.

Furthermore, if flow->irq is 0, ret = flow->irq sets ret to 0 and jumps to
fail:, which cleans up the RX channel and returns 0 (success) to
emac_ndo_open(). Should the return value of k3_udma_glue_rx_get_irq() be
checked in a signed int variable before assigning to flow->irq?

[ ... ]

> @@ -641,8 +688,9 @@ int prueth_dma_rx_push_mapped(struct prueth_emac *emac,
>  	swdata = cppi5_hdesc_get_swdata(desc_rx);
>  	swdata->type = PRUETH_SWDATA_PAGE;
>  	swdata->data.page = page;
> +	swdata->flow_id = flow_id;
>  
> -	return k3_udma_glue_push_rx_chn(rx_chn->rx_chn, PRUETH_RX_FLOW_DATA,
> +	return k3_udma_glue_push_rx_chn(rx_chn->rx_chn, flow_id,
>  					desc_rx, desc_dma);
>  }

[Severity: High]
If k3_udma_glue_push_rx_chn() fails in prueth_dma_rx_push_mapped() (or in
prueth_dma_rx_push_mapped_zc()), does this leak desc_rx from
rx_chn->desc_pool because k3_cppi_desc_pool_free() is not called before
returning?

[ ... ]

> @@ -863,31 +911,32 @@ static int prueth_dma_rx_push_mapped_zc(struct prueth_emac *emac,
>  					struct xdp_buff *xdp)
>  {
>  	struct net_device *ndev = emac->ndev;
> +	struct prueth_rx_flow *rx_flow = &emac->rx_chns.flows[PRUETH_RX_FLOW_DATA];
>  	struct cppi5_host_desc_t *desc_rx;
>  	struct prueth_swdata *swdata;
>  	dma_addr_t desc_dma;
>  	dma_addr_t buf_dma;
>  	int buf_len;
>  
>  	buf_dma = xsk_buff_xdp_get_dma(xdp);
>  	desc_rx = k3_cppi_desc_pool_alloc(rx_chn->desc_pool);
>  	if (!desc_rx) {
>  		netdev_err(ndev, "rx push: failed to allocate descriptor\n");
>  		return -ENOMEM;
>  	}
>  	desc_dma = k3_cppi_desc_pool_virt2dma(rx_chn->desc_pool, desc_rx);
>  
>  	cppi5_hdesc_init(desc_rx, CPPI5_INFO0_HDESC_EPIB_PRESENT,
>  			 PRUETH_NAV_PS_DATA_SIZE);
>  	k3_udma_glue_rx_dma_to_cppi5_addr(rx_chn->rx_chn, &buf_dma);
> -	buf_len = xsk_pool_get_rx_frame_size(rx_chn->xsk_pool);
> +	buf_len = xsk_pool_get_rx_frame_size(rx_flow->xsk_pool);
>  	cppi5_hdesc_attach_buf(desc_rx, buf_dma, buf_len, buf_dma, buf_len);
>  	swdata = cppi5_hdesc_get_swdata(desc_rx);
>  	swdata->type = PRUETH_SWDATA_XSK;
>  	swdata->data.xdp = xdp;
>  
>  	return k3_udma_glue_push_rx_chn(rx_chn->rx_chn, PRUETH_RX_FLOW_DATA,
>  					desc_rx, desc_dma);
>  }
>  
> -static int prueth_rx_alloc_zc(struct prueth_emac *emac, int budget)
> +static int prueth_rx_alloc_zc(struct prueth_emac *emac, int flow_id, int budget)
>  {
> -	struct prueth_rx_chn *rx_chn = &emac->rx_chns;
> +	struct prueth_rx_flow *rx_flow = &emac->rx_chns.flows[flow_id];
>  	struct xdp_buff *xdp;
>  	int i, ret;
>  
>  	for (i = 0; i < budget; i++) {
> -		xdp = xsk_buff_alloc(rx_chn->xsk_pool);
> +		xdp = xsk_buff_alloc(rx_flow->xsk_pool);
>  		if (!xdp)
>  			break;
>  
> -		ret = prueth_dma_rx_push_mapped_zc(emac, rx_chn, xdp);
> +		ret = prueth_dma_rx_push_mapped_zc(emac, &emac->rx_chns, xdp);

[Severity: Critical]
In prueth_rx_alloc_zc(), xdp buffers are allocated from
flows[flow_id].xsk_pool, but prueth_dma_rx_push_mapped_zc() does not take a
flow_id parameter and instead hardcodes PRUETH_RX_FLOW_DATA (0) for both
rx_flow->xsk_pool and k3_udma_glue_push_rx_chn().

If an XSK pool is active on flow_id > 0 while flows[0].xsk_pool is NULL,
won't xsk_pool_get_rx_frame_size(rx_flow->xsk_pool) dereference a NULL
pointer, or otherwise push flow_id's XSK buffers into flow 0's ring?

[ ... ]

> @@ -1155,19 +1206,22 @@ void prueth_rx_cleanup(void *data, dma_addr_t desc_dma)
>  	struct prueth_rx_chn *rx_chn = data;
>  	struct cppi5_host_desc_t *desc_rx;
>  	struct prueth_swdata *swdata;
> -	struct page_pool *pool;
> +	struct page_pool *pool = NULL;
>  	struct xdp_buff *xdp;
>  	struct page *page;
>  
> -	pool = rx_chn->pg_pool;
>  	desc_rx = k3_cppi_desc_pool_dma2virt(rx_chn->desc_pool, desc_dma);
>  	swdata = cppi5_hdesc_get_swdata(desc_rx);
> -	if (rx_chn->xsk_pool) {
> +
> +	if (swdata->type == PRUETH_SWDATA_XSK) {
>  		xdp = swdata->data.xdp;
>  		xsk_buff_free(xdp);
> -	} else {
> -		page = swdata->data.page;
> -		page_pool_recycle_direct(pool, page);
> +	} else if (swdata->type == PRUETH_SWDATA_PAGE) {
> +		pool = rx_chn->flows[swdata->flow_id].pg_pool;
> +		if (pool) {
> +			page = swdata->data.page;
> +			page_pool_recycle_direct(pool, page);
> +		}
>  	}
>  
>  	k3_cppi_desc_pool_free(rx_chn->desc_pool, desc_rx);

[ ... ]

> @@ -1488,59 +1531,77 @@ int icssg_napi_rx_poll(struct napi_struct *napi_rx, int budget)
>  }
>  EXPORT_SYMBOL_GPL(icssg_napi_rx_poll);
>  
> -int prueth_prepare_rx_chan(struct prueth_emac *emac,
> -			   struct prueth_rx_chn *chn,
> -			   int buf_size)
> +int prueth_prepare_rx_flow(struct prueth_emac *emac,
> +			   int flow)
>  {
> +	int buf_size = PRUETH_MAX_PKT_SIZE;
> +	struct prueth_rx_flow *rx_flow;
> +	struct prueth_rx_chn *rx_chn;
> +	struct page_pool *pool;
>  	struct page *page;
>  	int desc_avail;
>  	int i, ret;
>  
> -	desc_avail = k3_cppi_desc_pool_avail(chn->desc_pool);
> -	if (desc_avail < chn->descs_num)
> -		netdev_warn(emac->ndev,
> -			    "not enough RX descriptors available %d < %d\n",
> -			    desc_avail, chn->descs_num);
> +	rx_chn = &emac->rx_chns;
> +	rx_flow = &emac->rx_chns.flows[flow];
>  
> -	if (chn->xsk_pool) {
> -		/* get pages from xsk_pool and push to RX ring
> -		 * queue as much as possible
> +	if (rx_flow->xsk_pool) {
> +		/* XSK zero-copy: buffers come from the XSK fill ring, no page
> +		 * pool needed.  Push as many XSK buffers as descriptors allow;
> +		 * the fill ring will be replenished by user-space.
>  		 */
> -		ret = prueth_rx_alloc_zc(emac, desc_avail);
> -		if (!ret)
> -			goto recycle_alloc_pg;
> -	} else {
> -		for (i = 0; i < desc_avail; i++) {
> -			/* NOTE: we're not using memory efficiently here.
> -			 * 1 full page (4KB?) used here instead of
> -			 * PRUETH_MAX_PKT_SIZE (~1.5KB?)
> -			 */
> -			page = page_pool_dev_alloc_pages(chn->pg_pool);
> -			if (!page) {
> -				netdev_err(emac->ndev, "couldn't allocate rx page\n");
> -				ret = -ENOMEM;
> -				goto recycle_alloc_pg;
> -			}
> +		desc_avail = k3_cppi_desc_pool_avail(rx_chn->desc_pool);
> +		if (desc_avail < PRUETH_MAX_RX_DESC)
> +			netdev_warn(emac->ndev,
> +				    "not enough RX descriptors available %d < %d\n",
> +				    desc_avail, PRUETH_MAX_RX_DESC);
>  
> -			ret = prueth_dma_rx_push_mapped(emac, chn, page, buf_size);
> -			if (ret < 0) {
> -				netdev_err(emac->ndev,
> -					   "cannot submit page for rx chan %s ret %d\n",
> -					   chn->name, ret);
> -				page_pool_recycle_direct(chn->pg_pool, page);
> -				goto recycle_alloc_pg;
> -			}
> +		prueth_rx_alloc_zc(emac, flow, desc_avail);
> +		return 0;
> +	}

[Severity: High]
In the rx_flow->xsk_pool branch of prueth_prepare_rx_flow(), desc_avail is
the total number of available descriptors in the shared rx_chn->desc_pool
(up to PRUETH_MAX_RX_DESC * emac->rx_flow_num). Should the budget passed to
prueth_rx_alloc_zc() be capped at min(desc_avail, PRUETH_MAX_RX_DESC) so a
single XSK flow does not consume descriptors needed by subsequent flows?

> +
> +	pool = prueth_create_page_pool(emac, rx_chn->dma_dev, rx_chn->descs_num, rx_flow);
> +	if (IS_ERR(pool))
> +		return PTR_ERR(pool);
> +
> +	rx_flow->pg_pool = pool;
> +
> +	desc_avail = k3_cppi_desc_pool_avail(rx_chn->desc_pool);
> +	if (desc_avail < PRUETH_MAX_RX_DESC)
> +		netdev_warn(emac->ndev,
> +			    "not enough RX descriptors available %d < %d\n",
> +			    desc_avail, PRUETH_MAX_RX_DESC);
> +
> +	for (i = 0; i < PRUETH_MAX_RX_DESC; ++i) {
> +		page = page_pool_dev_alloc_pages(pool);
> +		if (!page) {
> +			netdev_err(emac->ndev, "couldn't allocate rx page\n");
> +			ret = -ENOMEM;
> +			goto err_free_pushed;
>  		}
> +
> +		ret = prueth_dma_rx_push_mapped(emac, rx_chn, page, buf_size, flow);
> +		if (ret < 0) {
> +			netdev_err(emac->ndev,
> +				   "cannot submit skb for rx chan %s ret %d\n",
> +				   rx_flow->name, ret);
> +			page_pool_recycle_direct(pool, page);
> +			goto err_free_pushed;
> +		}
>  	}
>  
>  	return 0;
>  
> -recycle_alloc_pg:
> -	prueth_reset_rx_chan(&emac->rx_chns, PRUETH_MAX_RX_FLOWS, false);
> -
> +err_free_pushed:
> +	/* i pages were successfully pushed to the ring.  Reset the channel
> +	 * to drain them back and recycle via prueth_rx_cleanup before the
> +	 * caller destroys the page pool.
> +	 */
> +	k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, flow, rx_chn,
> +				   prueth_rx_cleanup);
>  	return ret;
>  }

[Severity: High]
How does per-flow reset interact with the shared FDQ ring on UDMAP
platforms?

In k3_udma_glue_reset_rx_chn():

    if (rx_chn->single_fdq && flow_num)
            goto do_reset;

Because all flows share flow 0's FDQ (rx_chn->single_fdq is true on UDMAP),
k3_udma_glue_reset_rx_chn() only drains ringrxfdq when flow_num == 0 and
skips it completely when flow_num > 0:

1. Here at err_free_pushed:, if flow > 0 fails partway through pushing
   pages, k3_udma_glue_reset_rx_chn(..., flow, ...) skips ringrxfdq and
   does not drain the pushed pages before the caller destroys
   rx_flow->pg_pool.

2. In prueth_create_rxqs(), the error unwind loop runs backwards:

       for (--i; i >= 0; i--)
               prueth_destroy_rxq(emac, i);

   For i > 0, prueth_destroy_rxq(emac, i) skips draining ringrxfdq, calls
   page_pool_destroy(flow->pg_pool), and sets flow->pg_pool = NULL. When
   i == 0 finally runs k3_udma_glue_reset_rx_chn(..., 0, ...),
   prueth_rx_cleanup() sees rx_chn->flows[swdata->flow_id].pg_pool == NULL
   for all descriptors with swdata->flow_id > 0 and leaks those pages.

3. In prueth_xsk_pool_enable() and prueth_xsk_pool_disable(), calling
   prueth_destroy_rxq(emac, queue_id) on a single queue while the shared RX
   channel is enabled either drains and resets the shared FDQ for all flows
   (if queue_id == 0) or fails to drain queue_id's descriptors from the
   shared FDQ before destroying its page pool or unmapping its XSK pool
   (if queue_id > 0).

4. During normal multi-flow RX on a shared FDQ ring, UDMAP pops descriptors
   from the shared FDQ in FIFO order regardless of which flow's page_pool
   allocated the page, whereas emac_rx_packet() syncs and recycles pages
   using rx_flow->pg_pool rather than rx_chn->flows[swdata->flow_id].pg_pool,
   and concurrent per-flow NAPI instances call k3_ringacc_ring_push() on
   the same shared struct k3_ring without serialization.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> index b715af21d23ac..5c2936ecd595c 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c

[ ... ]

> @@ -245,6 +248,8 @@ static int emac_set_coalesce(struct net_device *ndev,
>  			     struct netlink_ext_ack *extack)
>  {
>  	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_rx_chn *rx_chn = &emac->rx_chns;
> +	struct prueth_rx_flow *flow = &rx_chn->flows[0];
>  	struct prueth *prueth = emac->prueth;
>  	struct prueth_tx_chn *tx_chn;
>  
> @@ -264,7 +269,7 @@ static int emac_set_coalesce(struct net_device *ndev,
>  		coal->tx_coalesce_usecs = ICSSG_MIN_COALESCE_USECS;
>  	}
>  
> -	emac->rx_pace_timeout_ns = coal->rx_coalesce_usecs * 1000;
> +	flow->rx_pace_timeout_ns = coal->rx_coalesce_usecs * 1000;
>  	tx_chn->tx_pace_timeout_ns = coal->tx_coalesce_usecs * 1000;
>  
>  	return 0;

[Severity: Medium]
Now that rx_pace_timeout_ns is stored per flow in struct prueth_rx_flow and
checked by icssg_napi_rx_poll() on each flow, emac_set_coalesce() only
updates rx_chn->flows[0].rx_pace_timeout_ns (and
emac_set_per_queue_coalesce() only handles TX queues). Does this leave RX
flows 1..7 with rx_pace_timeout_ns == 0 when RX coalescing is configured?

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 591be5c8056b4..8338402597b21 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c

[ ... ]

> @@ -883,14 +832,14 @@ static int icssg_update_vlan_mcast(struct net_device *vdev, int vid,
>  static void prueth_set_xsk_pool(struct prueth_emac *emac, u16 queue_id)
>  {
>  	struct prueth_tx_chn *tx_chn = &emac->tx_chns[queue_id];
> -	struct prueth_rx_chn *rx_chn = &emac->rx_chns;
> +	struct prueth_rx_flow *rx_flow = &emac->rx_chns.flows[PRUETH_RX_FLOW_DATA];
>  
> -	if (emac->xsk_qid != queue_id) {
> -		rx_chn->xsk_pool = NULL;
> -		tx_chn->xsk_pool = NULL;
> -	} else {
> -		rx_chn->xsk_pool = xsk_get_pool_from_qid(emac->ndev, queue_id);
> +	if (test_bit(queue_id, emac->xsk_queues)) {
> +		rx_flow->xsk_pool = xsk_get_pool_from_qid(emac->ndev, queue_id);
>  		tx_chn->xsk_pool = xsk_get_pool_from_qid(emac->ndev, queue_id);
> +	} else {
> +		rx_flow->xsk_pool = NULL;
> +		tx_chn->xsk_pool = NULL;
>  	}
>  }

[Severity: Critical]
In prueth_set_xsk_pool(), rx_flow is indexed with PRUETH_RX_FLOW_DATA (0)
instead of queue_id. When enabling or disabling an XSK pool on queue_id > 0,
won't this overwrite or clear flows[0].xsk_pool while flow 0 is running and
leave flows[queue_id].xsk_pool untouched?

Also, in prueth_init_rx_chns(), flow->xsk_pool is set to NULL for all flows
during emac_ndo_open() without calling prueth_set_xsk_pool() to restore
pools for queues set in emac->xsk_queues.

[ ... ]

> @@ -921,40 +870,58 @@ static void prueth_destroy_txq(struct prueth_emac *emac)
>  	}
>  }
>  
> -static void prueth_destroy_rxq(struct prueth_emac *emac)
> +static void prueth_destroy_rxq(struct prueth_emac *emac, int qid)
> +{
> +	struct prueth_rx_chn *rx_chn = &emac->rx_chns;
> +	struct prueth_rx_flow *flow = &rx_chn->flows[qid];
> +
> +	/* reset napi and hrtimer, and free all pending buffers */
> +	napi_disable(&flow->napi_rx);
> +	hrtimer_cancel(&flow->rx_hrtimer);

[Severity: Medium]
If flow->rx_hrtimer is pending when prueth_destroy_rxq() cancels it,
flow->irq_disabled is true and flow->irq is currently disabled at the IRQ
chip waiting for emac_rx_timer_callback() to re-enable it.

When prueth_destroy_rxq() is called from prueth_xsk_pool_enable() or
prueth_xsk_pool_disable() on a running interface, cancelling the timer
without re-enabling flow->irq and clearing flow->irq_disabled leaves the
flow's RX IRQ permanently disabled (and across emac_ndo_stop() /
emac_ndo_open(), flow->irq_disabled remains true because
prueth_init_rx_chns() does not reset it). Should prueth_destroy_rxq() check
flow->irq_disabled and call enable_irq(flow->irq) after hrtimer_cancel()?

> +	k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, flow->id, rx_chn,
> +				  prueth_rx_cleanup);
> +
> +	if (xdp_rxq_info_is_reg(&flow->xdp_rxq))
> +		xdp_rxq_info_unreg(&flow->xdp_rxq);
> +
> +	if (flow->pg_pool) {
> +		page_pool_destroy(flow->pg_pool);
> +		flow->pg_pool = NULL;
> +	}
> +}
> +
> +static void prueth_destroy_rxqs(struct prueth_emac *emac)
>  {
>  	int i, ret;
>  
>  	/* tear down and disable UDMA channels */
>  	reinit_completion(&emac->tdown_complete);
>  	k3_udma_glue_tdown_rx_chn(emac->rx_chns.rx_chn, true);
>  
>  	/* When RX DMA Channel Teardown is initiated, it will result in an
>  	 * interrupt and a Teardown Completion Marker (TDCM) is queued into
>  	 * the RX Completion queue. Acknowledging the interrupt involves
>  	 * popping the TDCM descriptor from the RX Completion queue via the
>  	 * RX NAPI Handler. To avoid timing out when waiting for the TDCM to
>  	 * be popped, schedule the RX NAPI handler to run immediately.
>  	 */
> -	if (!napi_if_scheduled_mark_missed(&emac->napi_rx)) {
> -		if (napi_schedule_prep(&emac->napi_rx))
> -			__napi_schedule(&emac->napi_rx);
> +	for (int i = 0; i < emac->rx_flow_num; i++) {
> +		if (test_bit(i, emac->xsk_queues)) {
> +			if (!napi_if_scheduled_mark_missed(&emac->rx_chns.flows[i].napi_rx)) {
> +				if (napi_schedule_prep(&emac->rx_chns.flows[i].napi_rx))
> +					__napi_schedule(&emac->rx_chns.flows[i].napi_rx);
> +			}
> +		}
>  	}

[Severity: Medium]
Why is NAPI scheduling during RX teardown restricted to flows with
test_bit(i, emac->xsk_queues) set (also shadowing the outer int i)?

If XSK is not enabled on a flow and its RX IRQ is currently disabled by
rx_hrtimer coalescing when prueth_destroy_rxqs() initiates teardown, NAPI
will not be scheduled immediately to pop the TDCM descriptor, causing
wait_for_completion_timeout(&emac->tdown_complete, msecs_to_jiffies(1000))
to time out.

[ ... ]

> @@ -978,27 +945,62 @@ static int prueth_create_txq(struct prueth_emac *emac)
>  	return ret;
>  }
>  
> -static int prueth_create_rxq(struct prueth_emac *emac)
> +static int prueth_create_rxq(struct prueth_emac *emac, int id)
>  {
> +	struct prueth_rx_chn *rx_chn = &emac->rx_chns;
> +	struct prueth_rx_flow *flow = &rx_chn->flows[id];
> +	struct xdp_rxq_info *rxq = &flow->xdp_rxq;
>  	int ret;
>  
> -	ret = prueth_prepare_rx_chan(emac, &emac->rx_chns, PRUETH_MAX_PKT_SIZE);
> +	ret = prueth_prepare_rx_flow(emac, id);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
> -	ret = k3_udma_glue_enable_rx_chn(emac->rx_chns.rx_chn);
> +	ret = xdp_rxq_info_reg(rxq, emac->ndev, 0, flow->napi_rx.napi_id);

[Severity: Critical]
Should the third argument to xdp_rxq_info_reg() be id instead of hardcoded
0 so each flow's xdp_rxq_info is registered with its own queue index? With
queue_index hardcoded to 0, XDP_REDIRECT to AF_XDP sockets bound to queues
1..7 will fail queue index validation in xsk_rcv_check().

>  	if (ret)
> -		goto reset_rx_chn;
> +		goto err;
>  
> -	ret = prueth_create_xdp_rxqs(emac);
> -	if (ret)
> -		goto reset_rx_chn;
> +	if (flow->xsk_pool) {
> +		ret = xdp_rxq_info_reg_mem_model(rxq, MEM_TYPE_XSK_BUFF_POOL, NULL);
> +		if (ret)
> +			goto err;
> +		xsk_pool_set_rxq_info(flow->xsk_pool, rxq);
> +	} else {
> +		ret = xdp_rxq_info_reg_mem_model(rxq, MEM_TYPE_PAGE_POOL, flow->pg_pool);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	napi_enable(&flow->napi_rx);
> +	return 0;
> +
> +err:
> +	prueth_destroy_rxq(emac, id);
> +	return ret;
> +}

[Severity: High]
If prueth_prepare_rx_flow(), xdp_rxq_info_reg(), or
xdp_rxq_info_reg_mem_model() fails in prueth_create_rxq(), execution jumps
to err: and calls prueth_destroy_rxq(emac, id) before
napi_enable(&flow->napi_rx) has been called.

Because netif_napi_add() (or a prior napi_disable()) leaves
NAPIF_STATE_SCHED set until napi_enable() is called, won't
napi_disable(&flow->napi_rx) in prueth_destroy_rxq() hang in an infinite
loop waiting for NAPIF_STATE_SCHED to clear?

[ ... ]

> @@ -1018,24 +1020,21 @@ static int emac_ndo_open(struct net_device *ndev)
>  	struct prueth *prueth = emac->prueth;
>  	int slice = prueth_emac_slice(emac);
>  	struct device *dev = prueth->dev;
> -	int max_rx_flows;
> -	int rx_flow;
>  
>  	/* set h/w MAC as user might have re-configured */
>  	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
>  
>  	icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>  	icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>  
>  	/* Notify the stack of the actual queue counts. */
>  	ret = netif_set_real_num_tx_queues(ndev, num_data_chn);
>  	if (ret) {
>  		dev_err(dev, "cannot set real number of tx queues\n");
>  		return ret;
>  	}

[Severity: Medium]
In prueth_netdev_init(), alloc_etherdev_mq(sizeof(*emac), num_tx_chn) is
called with num_tx_chn = PRUETH_MAX_TX_QUEUES (4), which only allocates 4
RX queues in ndev->_rx (ndev->num_rx_queues = 4, ndev->real_num_rx_queues =
4), and emac_ndo_open() never calls
netif_set_real_num_rx_queues(ndev, emac->rx_flow_num).

When emac->rx_flow_num is set to 5..8 via ethtool, queues 4..7 have no
struct netdev_rx_queue entries in ndev->_rx (breaking AF_XDP, RPS, sysfs,
and netlink queue operations for queues >= 4), and when emac->rx_flow_num
is 1..3, the stack still sees 4 active RX queues. Should
prueth_netdev_init() use alloc_etherdev_mqs(..., PRUETH_MAX_TX_QUEUES,
PRUETH_MAX_RX_FLOWS) and emac_ndo_open() call
netif_set_real_num_rx_queues(ndev, emac->rx_flow_num)?

[ ... ]

> @@ -1467,7 +1459,6 @@ static int emac_xdp_setup(struct prueth_emac *emac, struct netdev_bpf *bpf)
>  static int prueth_xsk_pool_enable(struct prueth_emac *emac,
>  				  struct xsk_buff_pool *pool, u16 queue_id)
>  {
> -	struct prueth_rx_chn *rx_chn = &emac->rx_chns;
>  	u32 frame_size;
>  	int ret;
>  
> @@ -1481,7 +1472,7 @@ static int prueth_xsk_pool_enable(struct prueth_emac *emac,
>  	if (frame_size < PRUETH_MAX_PKT_SIZE)
>  		return -EOPNOTSUPP;
>  
> -	ret = xsk_pool_dma_map(pool, rx_chn->dma_dev, PRUETH_RX_DMA_ATTR);
> +	ret = xsk_pool_dma_map(pool, emac->rx_chns.dma_dev, PRUETH_RX_DMA_ATTR);
>  	if (ret) {
>  		netdev_err(emac->ndev, "Failed to map XSK pool: %d\n", ret);
>  		return ret;
> @@ -1492,21 +1483,21 @@ static int prueth_xsk_pool_enable(struct prueth_emac *emac,
>  		ret = icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
>  		if (ret)
>  			return ret;
> -		prueth_destroy_rxq(emac);
> +		prueth_destroy_rxq(emac, queue_id);
>  	}
>  
> -	emac->xsk_qid = queue_id;
> +	set_bit(queue_id, emac->xsk_queues);
>  	prueth_set_xsk_pool(emac, queue_id);
>  
>  	if (netif_running(emac->ndev)) {
> -		ret = prueth_create_rxq(emac);
> +		ret = prueth_create_rxq(emac, queue_id);
>  		if (ret) {
>  			netdev_err(emac->ndev, "Failed to create RX queue: %d\n", ret);
>  			return ret;
>  		}
>  		ret = icssg_set_port_state(emac, ICSSG_EMAC_PORT_FORWARD);
>  		if (ret) {
> -			prueth_destroy_rxq(emac);
> +			prueth_destroy_rxq(emac, queue_id);
>  			return ret;
>  		}
>  		ret = prueth_xsk_wakeup(emac->ndev, queue_id, XDP_WAKEUP_RX);

[Severity: Critical]
In prueth_xsk_pool_enable() and prueth_xsk_pool_disable(), queue_id is
checked against PRUETH_MAX_RX_FLOWS and emac->tx_ch_num, but not against
emac->rx_flow_num:

    if (queue_id >= PRUETH_MAX_RX_FLOWS ||
        queue_id >= emac->tx_ch_num)

If emac->tx_ch_num > emac->rx_flow_num (e.g. tx_ch_num == 4 and
rx_flow_num == 1) and an AF_XDP pool is bound to queue_id == 1,
prueth_destroy_rxq(emac, 1) and prueth_create_rxq(emac, 1) will operate on
an uninitialized flows[1] (hanging in napi_disable() on an uninitialized
napi_struct and indexing past rx_chn->rx_chn->flows in
k3_udma_glue_reset_rx_chn()).

Also, if prueth_xsk_pool_enable() fails after xsk_pool_dma_map() or
set_bit(queue_id, emac->xsk_queues), does it need to unmap the pool and
clear the bit and xsk_pool pointers on the error path?

[ ... ]

> @@ -1594,9 +1586,9 @@ int prueth_xsk_wakeup(struct net_device *ndev, u32 qid, u32 flags)
>  {
>  	struct prueth_emac *emac = netdev_priv(ndev);
>  	struct prueth_tx_chn *tx_chn = &emac->tx_chns[qid];
> -	struct prueth_rx_chn *rx_chn = &emac->rx_chns;
> +	struct prueth_rx_flow *rx_flow = &emac->rx_chns.flows[PRUETH_RX_FLOW_DATA];
>  
> -	if (emac->xsk_qid != qid) {
> +	if (!test_bit(qid, emac->xsk_queues)) {
>  		netdev_err(ndev, "XSK queue %d not registered\n", qid);
>  		return -EINVAL;
>  	}
> @@ -1611,7 +1603,7 @@ int prueth_xsk_wakeup(struct net_device *ndev, u32 qid, u32 flags)
>  		return -EINVAL;
>  	}
>  
> -	if (!rx_chn->xsk_pool) {
> +	if (!rx_flow->xsk_pool) {
>  		netdev_err(ndev, "XSK pool not registered for RX queue %d\n", qid);
>  		return -EINVAL;
>  	}
> @@ -1624,9 +1616,9 @@ int prueth_xsk_wakeup(struct net_device *ndev, u32 qid, u32 flags)
>  	}
>  
>  	if (flags & XDP_WAKEUP_RX) {
> -		if (!napi_if_scheduled_mark_missed(&emac->napi_rx)) {
> -			if (likely(napi_schedule_prep(&emac->napi_rx)))
> -				__napi_schedule(&emac->napi_rx);
> +		if (!napi_if_scheduled_mark_missed(&rx_flow->napi_rx)) {
> +			if (likely(napi_schedule_prep(&rx_flow->napi_rx)))
> +				__napi_schedule(&rx_flow->napi_rx);
>  		}
>  	}

[Severity: Critical]
In prueth_xsk_wakeup():

1. test_bit(qid, emac->xsk_queues) is called before checking whether
   qid >= PRUETH_MAX_RX_FLOWS, which can read out of bounds of the
   emac->xsk_queues bitmap if qid is out of range (and the bounds check
   itself also misses qid >= emac->rx_flow_num).

2. rx_flow is hardcoded to &emac->rx_chns.flows[PRUETH_RX_FLOW_DATA]
   instead of &emac->rx_chns.flows[qid], so XDP_WAKEUP_RX always checks and
   schedules flow 0's NAPI instead of flow qid's NAPI.

> @@ -1697,6 +1689,14 @@ static int prueth_netdev_init(struct prueth *prueth,
>  	}
>  
>  	emac->tx_ch_num = 1;
> +	emac->rx_flow_num = 1;
> +	emac->xsk_queues = devm_bitmap_zalloc(prueth->dev,
> +					      PRUETH_MAX_RX_FLOWS, GFP_KERNEL);
> +	if (!emac->xsk_queues) {
> +		dev_err(prueth->dev, "failed to allocate xsk_queues bitmap\n");
> +		ret = -ENOMEM;
> +		goto free_ndev;
> +	}

[Severity: Medium]
If devm_bitmap_zalloc() fails here in prueth_netdev_init(), jumping to
free_ndev skips pruss_release_mem_region(prueth->pruss, &emac->dram) at the
free: label. Should this jump to free instead so emac->dram is not leaked?

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
> index b8115ca47082f..582d582cba89f 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
> @@ -570,11 +570,11 @@ static int emac_ndo_open(struct net_device *ndev)
>  	icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>  
>  	/* Prepare RX */
> -	ret = prueth_prepare_rx_chan(emac, &emac->rx_chns, PRUETH_MAX_PKT_SIZE);
> +	ret = prueth_prepare_rx_flow(emac, &emac->rx_chns, PRUETH_MAX_PKT_SIZE);
>  	if (ret)
>  		goto stop;
>  
> -	ret = prueth_prepare_rx_chan(emac, &emac->rx_mgm_chn, 64);
> +	ret = prueth_prepare_rx_flow(emac, &emac->rx_mgm_chn, 64);
>  	if (ret)
>  		goto reset_rx_chn;

[Severity: High]
Does drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c still build when
CONFIG_TI_ICSSG_PRUETH_SR1 is enabled?

Here in emac_ndo_open(), prueth_prepare_rx_flow() is called with 3
arguments (passing struct prueth_rx_chn * as the second argument), whereas
its prototype in icssg_prueth.h is now:

    int prueth_prepare_rx_flow(struct prueth_emac *emac, int flow);

Elsewhere in icssg_prueth_sr1.c:
- prueth_process_rx_mgm() still accesses the removed rx_chn->pg_pool member
  and calls the 5-argument prueth_dma_rx_push_mapped() with 4 arguments.
- emac_ndo_open(), emac_ndo_stop(), and prueth_netdev_init() still access
  the removed emac->napi_rx, emac->rx_chns.irq, and emac->rx_mgm_chn.irq
  fields, and call prueth_init_rx_chns(), prueth_reset_rx_chan(), and
  prueth_cleanup_rx_chns() with their old argument lists.
- emac_ndo_open() passes emac as dev_id to request_irq(..., prueth_rx_irq,
  ..., emac), whereas prueth_rx_irq() now expects &rx_flow->napi_rx as
  dev_id.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103134501.4123194-1-m-malladi@ti.com?part=1