Re: [PATCH net-next v5 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 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?

[ ... ]

>  	/* 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?

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