Re: [PATCH net-next v6 12/12] net: airoha: Better handle MIB for GDM with multiple port attached

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The RUNT_CNT statistic is overwritten and lost due to incorrect assignment logic.
- [High] 32-bit free-running hardware statistics will wrap around and reset 64-bit software counters to zero.
- [High] Torn hardware reads on 64-bit registers due to free-running counters.
- [High] Pre-existing potential deadlock due to using `spin_lock()` instead of `spin_lock_bh()` for stats lock.
--

commit 2f0d0c633539afc37cceb4ad426afb56465e7b6d
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: net: airoha: Better handle MIB for GDM with multiple port attached

The commit enables the split MIB feature for GDM3 and GDM4, moving stats
from port to device, and modifying the stats update logic from clear-on-read
to direct assignment since the MIB clear was removed.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 533ffe20f8333..27ccc636d8009 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1621,143 +1621,146 @@ static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
>  	struct airoha_eth *eth = dev->eth;
>  	u32 val, i = 0;
>  
> -	spin_lock(&port->stats.lock);
> -	u64_stats_update_begin(&port->stats.syncp);
> +	spin_lock(&port->stats_lock);

This isn't a bug introduced by this patch, but could this spin_lock()
cause a deadlock? Since airoha_update_hw_stats() is called by
ndo_get_stats64() (which can run in softirq context) and ethtool
callbacks (which run in process context), a softirq could interrupt the
process context and spin forever trying to acquire the same lock. Should
this be spin_lock_bh()?

> +	u64_stats_update_begin(&dev->stats.syncp);
> +
> +	/* Read relevant MIB for GDM with multiple port attached */
> +	if (port->id == AIROHA_GDM3_IDX || port->id == AIROHA_GDM4_IDX)
> +		airoha_fe_rmw(eth, REG_FE_GDM_MIB_CFG(port->id),
> +			      FE_TX_MIB_ID_MASK | FE_RX_MIB_ID_MASK,
> +			      FIELD_PREP(FE_TX_MIB_ID_MASK, dev->nbq) |
> +			      FIELD_PREP(FE_RX_MIB_ID_MASK, dev->nbq));
>  
>  	/* TX */
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_H(port->id));
> -	port->stats.tx_ok_pkts += ((u64)val << 32);
> +	dev->stats.tx_ok_pkts = (u64)val << 32;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_L(port->id));
> -	port->stats.tx_ok_pkts += val;

Now that the hardware counters are free-running (the MIB clear operation
was removed later in the patch), can a torn read occur here? If the low
32-bit register overflows and increments the high 32-bit register in the
brief window between the two reads, the resulting 64-bit value will be
corrupted and off by ~4.29 billion.

[ ... ]
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_DROP_CNT(port->id));
> -	port->stats.tx_drops += val;
> +	dev->stats.tx_drops = val;

Since TX_ETH_DROP_CNT is a 32-bit hardware register and the MIB clear
operation is removed, the hardware counter will eventually wrap back to
zero. By assigning it directly to dev->stats.tx_drops, won't this
cause the 64-bit software counter to also silently drop back to zero and
violate the monotonically increasing requirement of ndo_get_stats64?
Does this need to calculate the delta instead of directly assigning the
value?

[ ... ]
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_RUNT_CNT(port->id));
> -	port->stats.tx_len[i] += val;
> +	dev->stats.tx_len[i] = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_E64_CNT_H(port->id));
> -	port->stats.tx_len[i] += ((u64)val << 32);
> +	dev->stats.tx_len[i] = (u64)val << 32;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_E64_CNT_L(port->id));
> -	port->stats.tx_len[i++] += val;

Does this overwrite the RUNT_CNT statistic? The code assigns RUNT_CNT
to tx_len[i] (where i=0), but immediately overwrites it with
E64_CNT_H at the same index because i is not incremented between
the assignments.

[ ... ]
>  	i = 0;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_RUNT_CNT(port->id));
> -	port->stats.rx_len[i] += val;
> +	dev->stats.rx_len[i] = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_E64_CNT_H(port->id));
> -	port->stats.rx_len[i] += ((u64)val << 32);
> +	dev->stats.rx_len[i] = (u64)val << 32;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_E64_CNT_L(port->id));
> -	port->stats.rx_len[i++] += val;

Does this also overwrite the RUNT_CNT statistic for the RX path in the
same way?

[ ... ]
> -	/* reset mib counters */
> -	airoha_fe_set(eth, REG_FE_GDM_MIB_CLEAR(port->id),
> -		      FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);
> +	dev->stats.rx_len[i++] = val;
>  
> -	u64_stats_update_end(&port->stats.syncp);
> -	spin_unlock(&port->stats.lock);
> +	u64_stats_update_end(&dev->stats.syncp);
> +	spin_unlock(&port->stats_lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-airoha-eth-multi-serdes-v6-0-c899462c4f75@kernel.org?part=12