Re: [PATCH net] net: airoha: fix BQL imbalance in TX path

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Fix a possible BQL imbalance in airoha_dev_xmit(), where inflight
> packets are accounted only for the AIROHA_NUM_TX_RING netdev TX
> queues. The queue index is computed as:
> 
>     qid = skb_get_queue_mapping(skb) % ARRAY_SIZE(qdma->q_tx)
>     txq = netdev_get_tx_queue(dev, qid);
> 
> However, airoha_qdma_tx_napi_poll() accounts completions across all
> netdev TX queues (num_tx_queues), leading to inconsistent BQL
> accounting.
> 
> Also reset all netdev TX queues in the ndo_stop callback.
> 
> Fixes: 1d304174106c ("net: airoha: Implement BQL support")
> Fixes: c9f947769b77 ("net: airoha: Reset BQL stopping the netdevice")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 19f67c7dd8e1..6c7390f0de5d 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -929,10 +929,9 @@ static int airoha_qdma_tx_napi_poll(struct napi_struct *napi, int budget)
>  		q->queued--;
>  
>  		if (skb) {
> -			u16 queue = skb_get_queue_mapping(skb);
>  			struct netdev_queue *txq;
>  
> -			txq = netdev_get_tx_queue(skb->dev, queue);
> +			txq = skb_get_tx_queue(skb->dev, skb);
>  			netdev_tx_completed_queue(txq, 1, skb->len);
>  			dev_kfree_skb_any(skb);
>  		}
> @@ -1711,7 +1710,7 @@ static int airoha_dev_stop(struct net_device *dev)
>  	if (err)
>  		return err;
>  
> -	for (i = 0; i < ARRAY_SIZE(qdma->q_tx); i++)
> +	for (i = 0; i < dev->num_tx_queues; i++)
>  		netdev_tx_reset_subqueue(dev, i);
>  
>  	airoha_set_gdm_port_fwd_cfg(qdma->eth, REG_GDM_FWD_CFG(port->id),
> @@ -2002,7 +2001,7 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
>  
>  	spin_lock_bh(&q->lock);
>  
> -	txq = netdev_get_tx_queue(dev, qid);
> +	txq = skb_get_tx_queue(dev, skb);
>  	nr_frags = 1 + skb_shinfo(skb)->nr_frags;
>  
>  	if (q->queued + nr_frags >= q->ndesc) {
> 
> ---
> base-commit: a663bac71a2f0b3ac6c373168ca57b2a6e6381aa
> change-id: 20260421-airoha-fix-bql-7fff7cebbc9a
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo@kernel.org>
> 

commenting on Sashiko reported issues:
https://sashiko.dev/#/patchset/20260421-airoha-fix-bql-v1-1-f135afe4275b%40kernel.org

- This isn't a bug in this patch, but does using 0xff as a sentinel value cause a permanent stall?
  I do not think this is a real issue since, according to my understanding, the NIC
  never writes 0xff in irq_q queue.

- This is another pre-existing issue, but does freeing the SKB here cause a DMA use-after-free
  for multi-fragment packets?
  This issue is not related to this patch, and I will fix it in a dedicated
  patch storing the skb pointer in the last descriptor in airoha_dev_xmit()

- Since the QDMA hardware and NAPI instance are shared among multiple ports (qdma->users),
  could active NAPI polling cause a BUG_ON() in dql_completed()?
  This is not an issue related to this patch since here we are just resetting
  all the netdev tx queues instead of just the first ARRAY_SIZE(qdma->q_tx)
  ones.

- This isn't a bug in this patch, but does failing to wait for the DMA engines to become
  idle before unmapping buffers cause memory corruption?
  This issue is not related to this patch and it will be fixed with a dedicated
  patch.

- This is also pre-existing, but can this mapping cause a kernel panic on highmem systems?
  Can we have fragments in high memory? e.g on ARM architecture? Anyway, as
  pointed out by Sashiko, this issue is not related to this patch.

- This isn't introduced here, but does this logic cause a permanent TX stall?
  this issue is already fixed in the following patch:
  https://patchwork.kernel.org/project/netdevbpf/patch/20260421-airoha-xmit-stop-condition-v1-1-e670d6a48467@kernel.org/

Regards,
Lorenzo

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