Re: [PATCH net] net: cadence: macb: Synchronize stats calculations

[Sean Anderson <sean.anderson@linux.dev>]

On 2/19/25 03:48, Maxime Chevallier wrote:
> Hello Sean,
> 
> On Tue, 18 Feb 2025 14:50:36 -0500
> Sean Anderson <sean.anderson@linux.dev> wrote:
> 
>> Stats calculations involve a RMW to add the stat update to the existing
>> value. This is currently not protected by any synchronization mechanism,
>> so data races are possible. Add a spinlock to protect the update. The
>> reader side could be protected using u64_stats, but we would still need
>> a spinlock for the update side anyway. And we always do an update
>> immediately before reading the stats anyway.
>> 
>> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>>  drivers/net/ethernet/cadence/macb.h      |  2 ++
>>  drivers/net/ethernet/cadence/macb_main.c | 12 ++++++++++--
>>  2 files changed, 12 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 5740c98d8c9f..2847278d9cd4 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -1279,6 +1279,8 @@ struct macb {
>>  	struct clk		*rx_clk;
>>  	struct clk		*tsu_clk;
>>  	struct net_device	*dev;
>> +	/* Protects hw_stats and ethtool_stats */
>> +	spinlock_t		stats_lock;
>>  	union {
>>  		struct macb_stats	macb;
>>  		struct gem_stats	gem;
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 48496209fb16..990a3863c6e1 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -1978,10 +1978,12 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>>  
>>  		if (status & MACB_BIT(ISR_ROVR)) {
>>  			/* We missed at least one packet */
>> +			spin_lock(&bp->stats_lock);
>>  			if (macb_is_gem(bp))
>>  				bp->hw_stats.gem.rx_overruns++;
>>  			else
>>  				bp->hw_stats.macb.rx_overruns++;
>> +			spin_unlock(&bp->stats_lock);
>>  
>>  			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>>  				queue_writel(queue, ISR, MACB_BIT(ISR_ROVR));
>> @@ -3102,6 +3104,7 @@ static struct net_device_stats *gem_get_stats(struct macb *bp)
>>  	if (!netif_running(bp->dev))
>>  		return nstat;
>>  
>> +	spin_lock(&bp->stats_lock);
>>  	gem_update_stats(bp);
>>  
>>  	nstat->rx_errors = (hwstat->rx_frame_check_sequence_errors +
>> @@ -3131,6 +3134,7 @@ static struct net_device_stats *gem_get_stats(struct macb *bp)
>>  	nstat->tx_aborted_errors = hwstat->tx_excessive_collisions;
>>  	nstat->tx_carrier_errors = hwstat->tx_carrier_sense_errors;
>>  	nstat->tx_fifo_errors = hwstat->tx_underrun;
>> +	spin_unlock(&bp->stats_lock);
>>  
>>  	return nstat;
>>  }
>> @@ -3138,12 +3142,13 @@ static struct net_device_stats *gem_get_stats(struct macb *bp)
>>  static void gem_get_ethtool_stats(struct net_device *dev,
>>  				  struct ethtool_stats *stats, u64 *data)
>>  {
>> -	struct macb *bp;
>> +	struct macb *bp = netdev_priv(dev);
>>  
>> -	bp = netdev_priv(dev);
>> +	spin_lock(&bp->stats_lock);
> 
> Sorry if I missed something, but as you're using that lock within the
> macb_interrupt(), shouldn't it be a spin_lock_irqsave() for all the
> callsites that aren't in irq context ?
> 
> You would risk a deadlock otherwise.

Yeah, looks like I missed this. Although I tested on a kernel with
lockdep enabled, and I thought that was supposed to catch this sort of
thing. Will send a v2.

--Sean