Re: [PATCH net] net: airoha: stop net_device TX queue before updating CPU index

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2890 bytes --]

> Currently, airoha_eth driver updates the CPU index register prior of
> verifying whether the number of free descriptors has fallen below the
> threshold.
> Move net_device TX queue length check before updating the TX CPU index
> in order to update TX CPU index even if there are more packets to be
> transmitted but the net_device TX queue is going to be stopped
> accounting the inflight packets.
> 
> Fixes: 1d304174106c ("net: airoha: Implement BQL support")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 19f67c7dd8e1..5d327237e274 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2058,17 +2058,16 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
>  
>  	skb_tx_timestamp(skb);
>  	netdev_tx_sent_queue(txq, skb->len);
> +	if (q->ndesc - q->queued < q->free_thr) {
> +		netif_tx_stop_queue(txq);
> +		q->txq_stopped = true;
> +	}
>  
>  	if (netif_xmit_stopped(txq) || !netdev_xmit_more())
>  		airoha_qdma_rmw(qdma, REG_TX_CPU_IDX(qid),
>  				TX_RING_CPU_IDX_MASK,
>  				FIELD_PREP(TX_RING_CPU_IDX_MASK, index));
>  
> -	if (q->ndesc - q->queued < q->free_thr) {
> -		netif_tx_stop_queue(txq);
> -		q->txq_stopped = true;
> -	}
> -
>  	spin_unlock_bh(&q->lock);
>  
>  	return NETDEV_TX_OK;
> 
> ---
> base-commit: a663bac71a2f0b3ac6c373168ca57b2a6e6381aa
> change-id: 20260421-airoha-xmit-stop-condition-344dc0292a19
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo@kernel.org>
> 

commenting on Sashiko retported issues:
https://sashiko.dev/#/patchset/20260421-airoha-xmit-stop-condition-v1-1-e670d6a48467%40kernel.org

- Could this cause a deadlock if exactly q->free_thr descriptors are free?
  This does not seem a problem to me since, even if the netdev tx queue is
  stopped as described in the report, the airoha_qdma_tx_napi_poll() will free
  space in the queue and subsequent packets will update REG_TX_CPU_IDX register.

- Is it possible for this loop to read past the end of the frags array?
  As pointed out by Sashiko, this issue is not introduced by this patch and I
  will fix with a dedicated patch.

- Might this lead to memory corruption if the tcp header is not in the linear area?
  This issue is not introduced by this patch and I will fix with a dedicated patch.

- If an error occurs during transmission, the driver jumps to the error label
  frees the skb, and returns NETDEV_TX_OK without ringing the qdma cpu index doorbell?
  Similar to the first issue, this does not seem a problem to me since subsequent
  packets will update REG_TX_CPU_IDX register.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]