Re: [PATCH net-next v9 2/3] net: airoha: fix ETS QoS stats counter underflow and cross-channel corruption

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> On 7/20/2026 3:03 PM, Lorenzo Bianconi wrote:
> > airoha_qdma_get_tx_ets_stats() has two bugs:
> > - The hardware counters read via airoha_qdma_rr() are 32-bit values
> >   but are stored in u64 locals and subtracted from u64 baselines. When
> >   a 32-bit hardware counter wraps around, the subtraction produces a
> >   large underflow value passed to _bstats_update().
> 
> This issue would only be a problem during rollover, which depending on
> how fast the counts increment may not be a big problem. I could see this
> not being worth going to net since it could be rare enough that it isn't
> considered a widespread issue...
> 
> > - The baseline counters (cpu_tx_packets, fwd_tx_packets) are stored as
> >   single per-device fields, but airoha_qdma_get_tx_ets_stats() is
> >   called with different channel values (0-3). Each call reads a
> >   different channel's hardware counter but overwrites the same
> >   baseline, corrupting the delta computation for other channels.
> > 
> 
> However, this issue seems like its going to cause a problem every time
> you read because any time you use a mix of channels you will get
> corrupted values?

Hi Jacob,

I agree this is a real bug (and it needs to be fixed). However, the real
use-case is having a single channel per net_device (a single HTB offloaded
qdisc) and multiple hw queues (connected to the ETS offloaded classes).
In this scenario we do not trigger this issue.

> 
> > Fix both by:
> > - Narrowing the counter locals and baselines to u32 so that 32-bit
> >   unsigned subtraction handles wrap-around naturally.
> > - Grouping the baselines into a per-channel qos_stats array so each
> >   channel tracks its own previous counter value independently.
> > - Splitting the delta addition into two statements so the first u32
> >   delta is widened to u64 on assignment and the second is added in
> >   u64 arithmetic, preventing overflow when both deltas are large.
> > 
> > Fixes: 20bf7d07c956 ("net: airoha: Add sched ETS offload support")
> 
> This targets a commit which merged in v6.14, but the patch is part of a
> series aimed at net-next. Could you explain why this shouldn't be
> separated out and put as a fix in net? It seems pretty obvious that
> users can easily reproduce problems by requesting stats from each
> channel? Or is this not really possible to trigger from userspace until
> patch 3/3?

For the reason described above and to avoid any possible conflicts with patch
3/3 I decided to add this patch here (adding the proper Fixes tag for the
backport) but if you prefer I can remove patch 2/3 from this series and send
it to net. What do you prefer?

Regards,
Lorenzo

> 
> > Reviewed-by: Simon Horman <horms@kernel.org>
> > Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/net/ethernet/airoha/airoha_eth.c | 18 +++++++++++-------
> >  drivers/net/ethernet/airoha/airoha_eth.h |  7 ++++---
> >  2 files changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 41c1a0ffbdd8..aaf2a4717d12 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -2482,16 +2482,20 @@ static int airoha_qdma_get_tx_ets_stats(struct net_device *netdev, int channel,
> >  {
> >  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> >  	struct airoha_qdma *qdma = dev->qdma;
> > +	u32 cpu_tx_packets, fwd_tx_packets;
> > +	u64 tx_packets;
> >  
> > -	u64 cpu_tx_packets = airoha_qdma_rr(qdma, REG_CNTR_VAL(channel << 1));
> > -	u64 fwd_tx_packets = airoha_qdma_rr(qdma,
> > -					    REG_CNTR_VAL((channel << 1) + 1));
> > -	u64 tx_packets = (cpu_tx_packets - dev->cpu_tx_packets) +
> > -			 (fwd_tx_packets - dev->fwd_tx_packets);
> > +	cpu_tx_packets = airoha_qdma_rr(qdma, REG_CNTR_VAL(channel << 1));
> > +	fwd_tx_packets = airoha_qdma_rr(qdma,
> > +					REG_CNTR_VAL((channel << 1) + 1));
> > +	tx_packets = (u32)(cpu_tx_packets -
> > +			   dev->qos_stats[channel].cpu_tx_packets);
> > +	tx_packets += (u32)(fwd_tx_packets -
> > +			    dev->qos_stats[channel].fwd_tx_packets);
> >  
> >  	_bstats_update(opt->stats.bstats, 0, tx_packets);
> > -	dev->cpu_tx_packets = cpu_tx_packets;
> > -	dev->fwd_tx_packets = fwd_tx_packets;
> > +	dev->qos_stats[channel].cpu_tx_packets = cpu_tx_packets;
> > +	dev->qos_stats[channel].fwd_tx_packets = fwd_tx_packets;
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > index bf1c249255bd..bf44be9f0954 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> > @@ -553,9 +553,10 @@ struct airoha_gdm_dev {
> >  	struct airoha_eth *eth;
> >  
> >  	DECLARE_BITMAP(qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS);
> > -	/* qos stats counters */
> > -	u64 cpu_tx_packets;
> > -	u64 fwd_tx_packets;
> > +	struct {
> > +		u32 cpu_tx_packets;
> > +		u32 fwd_tx_packets;
> > +	} qos_stats[AIROHA_NUM_QOS_CHANNELS];
> >  
> >  	u32 flags;
> >  	int nbq;
> > 
> 

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