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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Moving the statistics lock from the port level to the device level introduces a race condition on shared hardware registers, causing cross-interface statistics corruption.
> --
> 
> commit 29334a9bde18f0777df549f0d135356a4b8bd3ee
> Author: Christian Marangi <ansuelsmth@gmail.com>
> 
> net: airoha: Better handle MIB for GDM with multiple port attached
> 
> This commit enables a split MIB feature for GDM3/4 to track counters per
> hardware channel instead of per GDM port. It moves the statistics structure,
> including its protecting lock, from the port level to the device level.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 623cac42dfb71..b1a6586293457 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -1621,143 +1621,150 @@ 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(&dev->stats.lock);
> > +	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));
> 
> Does moving the lock from the port to the device level introduce a race
> condition on the shared port MIB registers?
> 
> Because ndo_get_stats64 can be called concurrently without the RTNL lock, if two
> CPUs request stats for two different devices on the same port:
> 
> CPU1 calls airoha_update_hw_stats() for dev_A:
>     spin_lock(&dev_A->stats.lock);
>     airoha_fe_rmw(..., REG_FE_GDM_MIB_CFG(port->id), ... dev_A->nbq);
> 
> CPU2 concurrently calls airoha_update_hw_stats() for dev_B:
>     spin_lock(&dev_B->stats.lock);
>     airoha_fe_rmw(..., REG_FE_GDM_MIB_CFG(port->id), ... dev_B->nbq);
> 
> Could CPU1 then read the hardware counters for dev_B instead of dev_A?

ack, I will fix it in v6.

> 
> [ ... ]
> 
> >  	/* 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);
> 
> When CPU1 then clears the counters here, could it wipe out the unread
> hardware statistics for dev_B?
> 
> Is a port-level lock still needed to exclusively protect access to these shared
> hardware MIB registers across multiple devices?

ack, I will fix it in v6.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260509-airoha-eth-multi-serdes-v5-0-805e38edc2aa@kernel.org?part=12

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