Re: [PATCH net-next v5 1/3] net: airoha: use int instead of atomic_t for qdma users counter

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> QDMA users counter is always accessed holding RTNL lock so we do not
> require atomic_t for it.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 4 ++--
>  drivers/net/ethernet/airoha/airoha_eth.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 504247c8bae9..9eb9b6a139b3 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1812,7 +1812,7 @@ static int airoha_dev_open(struct net_device *netdev)
>  	airoha_qdma_set(qdma, REG_QDMA_GLOBAL_CFG,
>  			GLOBAL_CFG_TX_DMA_EN_MASK |
>  			GLOBAL_CFG_RX_DMA_EN_MASK);
> -	atomic_inc(&qdma->users);
> +	qdma->users++;
>  
>  	if (!airoha_is_lan_gdm_dev(dev) &&
>  	    airoha_ppe_is_enabled(qdma->eth, 1))
> @@ -1866,7 +1866,7 @@ static int airoha_dev_stop(struct net_device *netdev)
>  					    REG_GDM_FWD_CFG(port->id),
>  					    FE_PSE_PORT_DROP);
>  
> -	if (atomic_dec_and_test(&qdma->users)) {
> +	if (!--qdma->users) {
>  		airoha_qdma_clear(qdma, REG_QDMA_GLOBAL_CFG,
>  				  GLOBAL_CFG_TX_DMA_EN_MASK |
>  				  GLOBAL_CFG_RX_DMA_EN_MASK);

commenting on sashiko's report:
https://sashiko.dev/#/patchset/20260611-airoha-ethtool-priv_flags-v5-0-c11de08486d1%40kernel.org

- This is a pre-existing issue, but by not cleaning the TX queues when
  qdma->users > 0, does this leave the stopped device's packets in flight?
  When airoha_dev_stop() is called, it resets the device's BQL counters via
  netdev_tx_reset_subqueue(). By skipping the cleanup when other devices are
  still sharing this QDMA ring, any pending skbs originating from the stopped
  device are left to linger in the hardware TX ring.
  Because the hardware continues to process the shared ring on behalf of the
  remaining interfaces, the active NAPI poll will process their completions in
  airoha_qdma_tx_napi_poll():
  		if (skb) {
  			struct netdev_queue *txq;
  			txq = skb_get_tx_queue(skb->dev, skb);
  			netdev_tx_completed_queue(txq, 1, skb->len);
  			dev_kfree_skb_any(skb);
  		}
  Can this cause an unsigned underflow in dql->num_queued since the BQL
  state was already reset to 0 by ndo_stop?

  - This issue is valid, but as pointed out by sashiko, this problem has not
    been introduced by this patch. In particular, I think we should remove
    netdev_tx_reset_subqueue() in airoha_dev_stop() and run
    netdev_tx_completed_queue() in airoha_qdma_cleanup_tx_queue(). We need to
    pay attention to not loop over already unregistered net_devices in
    airoha_qdma_cleanup_tx_queue() run from airoha_remove().
    I will post a dedicated patch for this issue.

- Also, if the device was being unregistered, unregister_netdev() frees the
  netdev memory without waiting for TX skbs (which do not hold a reference to
  dev). Could this lead to a use-after-free when dereferencing skb->dev here?
  Should the driver selectively purge skbs belonging to the stopping netdev
  from the shared TX ring before ndo_stop returns?

  - I do not think this is a problem in the current codebase since the
    net_devices are unregistered just during module unload running
    airoha_remove() where we run airoha_qdma_stop_napi() to stop TX/RX NAPIs.

Regards,
Lorenzo

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 8f42973f9cf5..24fd8dcf7fca 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -526,7 +526,7 @@ struct airoha_qdma {
>  	struct airoha_eth *eth;
>  	void __iomem *regs;
>  
> -	atomic_t users;
> +	int users;
>  
>  	struct airoha_irq_bank irq_banks[AIROHA_MAX_NUM_IRQ_BANKS];
>  
> 
> -- 
> 2.54.0
> 

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