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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> 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()?

ndo_get_stats64() does not run in softirq context.

> 
> > +	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.

ack, I will fit it in v7.

> 
> [ ... ]
> >  	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?

ack, I will fit it in v7.

> 
> [ ... ]
> >  	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.

ack, I will fit it in v7.

> 
> [ ... ]
> >  	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?

ack, I will fit it in v7.

Regards,
Lorenzo

> 
> [ ... ]
> > -	/* 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

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