Re: [PATCH net 4/4] net: ti: icssg: Fix XSK zero copy TX during application wakeup

[sashiko-bot@kernel.org]

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

New issues:
- [High] Removal of `xsk_tx_release()` prevents the AF_XDP TX ring consumer pointer from being updated, permanently stalling zero-copy transmissions.
- [High] The TX queue's `trans_start` timestamp is unconditionally updated even if no packets are transmitted, defeating the netdev watchdog.
- [Low] The variable `num_tx` is initialized and incremented but never read.

Pre-existing issues:
- [High] AF_XDP descriptors and Completion Queue (CQ) slots are permanently leaked on hardware allocation failure.
--

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

net: ti: icssg: Fix XSK zero copy TX during application wakeup
    
This patch modifies emac_xsk_xmit_zc() to handle zero-copy TX without returning
the number of packets transmitted. It removes the early return in
emac_tx_complete_packets() to allow application wakeups to initiate zero-copy
xmit traffic, and adds __netif_tx_lock() for queue protection.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index 82ddef9c17d54..c3f3cacdb28e7 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -93,8 +93,8 @@ void prueth_ndev_del_tx_napi(struct prueth_emac *emac, int num)
>  }
>  EXPORT_SYMBOL_GPL(prueth_ndev_del_tx_napi);
>  
> -static int emac_xsk_xmit_zc(struct prueth_emac *emac,
> -			    unsigned int q_idx)
> +static void emac_xsk_xmit_zc(struct prueth_emac *emac,
> +			     unsigned int q_idx)
>  {
>  	struct prueth_tx_chn *tx_chn = &emac->tx_chns[q_idx];
>  	struct xsk_buff_pool *pool = tx_chn->xsk_pool;
> @@ -115,7 +115,7 @@ static int emac_xsk_xmit_zc(struct prueth_emac *emac,
>  	 * necessary
>  	 */
>  	if (descs_avail <= MAX_SKB_FRAGS)
> -		return 0;
> +		return;
>  
>  	descs_avail -= MAX_SKB_FRAGS;
>  
> @@ -169,9 +169,6 @@ static int emac_xsk_xmit_zc(struct prueth_emac *emac,
>  
>  		num_tx++;
>  	}

[Severity: High]
This is a pre-existing issue, but does early exit on hardware push failure
permanently leak the peeked AF_XDP descriptor?

If k3_udma_glue_push_tx_chn() fails, the driver breaks out of the loop right
before the num_tx increment shown above:

		if (ret) {
			ndev->stats.tx_errors++;
			k3_cppi_desc_pool_free(tx_chn->desc_pool, host_desc);
			break;
		}

At this point, xsk_tx_peek_desc() has already consumed the descriptor from
the user space TX ring. Because the packet is never pushed to hardware, it
will never trigger a TX completion interrupt, and xsk_tx_completed() will
never be called.

Does the driver need to manually submit the completion for dropped packets
here to prevent the memory from being permanently lost to the application?

> -
> -	xsk_tx_release(tx_chn->xsk_pool);
> -	return num_tx;

[Severity: High]
Does removing xsk_tx_release() prevent the AF_XDP TX ring consumer pointer
from being updated? 

Since xsk_tx_peek_desc() only advances a local cached consumer pointer, it
seems xsk_tx_release() is required to publish this pointer to the shared
memory ring. 

Without this, will userspace eventually observe the TX ring as completely
full and stop producing new packets, leading to a permanent traffic stall?

[Severity: Low]
Since the return of num_tx was removed and the function changed to void,
is the num_tx variable now unused? 

It looks like it is initialized and incremented inside the loop but never
read.

>  }
>  
>  void prueth_xmit_free(struct prueth_tx_chn *tx_chn,
> @@ -279,9 +276,6 @@ int emac_tx_complete_packets(struct prueth_emac *emac, int chn,
>  		num_tx++;
>  	}
>  
> -	if (!num_tx)
> -		return 0;
> -

[Severity: High]
Because this early return was removed, does the code now unconditionally
update the TX queue's trans_start timestamp on every NAPI poll?

Later in this function, if tx_chn->xsk_pool is set, the driver calls
txq_trans_cond_update(), which updates txq->trans_start to jiffies. 

If the hardware stalls but NAPI is still being polled (e.g., due to userspace
wakeups or RX packets), trans_start will continually be pushed forward.

Will this mask genuine hardware TX stalls from the kernel and prevent the
netdev watchdog from triggering a recovery?

>  	netif_txq = netdev_get_tx_queue(ndev, chn);
>  	netdev_tx_completed_queue(netif_txq, num_tx, total_bytes);
>  
> @@ -306,7 +300,9 @@ int emac_tx_complete_packets(struct prueth_emac *emac, int chn,
>  
>  		netif_txq = netdev_get_tx_queue(ndev, chn);
>  		txq_trans_cond_update(netif_txq);
> +		__netif_tx_lock(netif_txq, smp_processor_id());
>  		emac_xsk_xmit_zc(emac, chn);
> +		__netif_tx_unlock(netif_txq);
>  	}
>  
>  	return num_tx;

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