Re: [PATCH net-next 05/15] net: airoha: update stats on flow deletion

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Sat, Nov 14, 2026 at 03:23:43AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] airoha_ppe_flow_update_stats()
>   (drivers/net/ethernet/airoha/airoha_ppe.c, ~1288-1303) computes…
> - [Low] In airoha_ppe_flow_offload_destroy() the final counters are
>   sampled by airoha_ppe_flow_update_stats() (line 1315) before the…
> 
> --
> 
> net: airoha: update stats on flow deletion
> 
> This commit moves the stats collection code out of
> airoha_ppe_flow_offload_stats() into a new helper,
> airoha_ppe_flow_update_stats(). The helper is also called from
> airoha_ppe_flow_offload_destroy(), so the final counters are reported on
> flow deletion, as introduced by "netfilter: flowtable: retrieve stats on
> flow deletion".
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
> > index f5369c45766d2..af8c7fe9b7f09 100644
> > --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> > +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> > @@ -1285,6 +1285,23 @@ static int airoha_ppe_flow_offload_replace(struct airoha_eth *eth,
> >  	return err;
> >  }
> >  
> > +static void airoha_ppe_flow_update_stats(struct airoha_eth *eth,
> > +					 struct flow_cls_offload *f,
> > +					 struct airoha_flow_table_entry *e)
> > +{
> > +	u32 idle = airoha_ppe_entry_idle_time(eth->ppe, e);
> > +
> > +	f->stats.lastused = jiffies - idle * HZ;
> > +	if (e->hash != 0xffff) {
> > +		struct airoha_foe_stats64 stats = {};
> > +
> > +		airoha_ppe_foe_entry_get_stats(eth->ppe, e->hash, &stats);
> > +		f->stats.pkts += (stats.packets - e->stats.packets);
> > +		f->stats.bytes += (stats.bytes - e->stats.bytes);
> > +		e->stats = stats;
> 
> [Severity: Medium]
> Can these subtractions wrap if the entry was re-bound after the last stats
> read?

Maybe, but that is a pre-existing issue.
 
This patch is just adapting the driver to use the new call to stats
when deleting a flow, it reuses the existing codebase.

Same as with the previous patch.

> Every time the driver commits a binding, it resets the hardware/NPU counter
> slot to zero:
> 
> airoha_ppe_foe_insert_entry()
>   airoha_ppe_foe_commit_entry(ppe, &e->data, hash, rx_wlan)
>     airoha_ppe_foe_flow_stats_update()
>       airoha_ppe_foe_flow_stat_entry_reset()
>         memset_io(&npu->stats[index], 0, sizeof(*npu->stats));
>         memset(&ppe->foe_stats[index], 0, sizeof(*ppe->foe_stats));
> 
> e->stats is not cleared on that path. Its only writer is the assignment
> above.
> 
> Suppose the FOE entry goes UNBIND, either through hardware aging or
> because the slot was overwritten. airoha_ppe_foe_insert_entry() then
> re-binds it and sets e->hash = hash. The next call here computes
> small - old_total in u64, which gives a value near 2^64.
> 
> On the DESTROY path, flow_offload_update_stats() passes that value to
> nf_ct_acct_add(), which takes unsigned int packets/bytes. The conntrack
> counters would then jump by roughly 2^32 - old_total.
> 
> The stale baseline already affected the periodic FLOW_CLS_STATS path. But
> nf_flow_offload_gc_step() never sends FLOW_CLS_STATS for a flow that has
> been torn down. A re-bind between teardown and the DESTROY work used to
> go unreported. With this patch it becomes the flow's final accounting
> value.
> 
> Should e->stats be reset wherever e is recommitted and the hardware counter
> is zeroed?
> 
> > +	}
> > +}
> > +
> >  static int airoha_ppe_flow_offload_destroy(struct airoha_eth *eth,
> >  					   struct flow_cls_offload *f)
> >  {
> > @@ -1295,6 +1312,7 @@ static int airoha_ppe_flow_offload_destroy(struct airoha_eth *eth,
> >  	if (!e)
> >  		return -ENOENT;
> >  
> > +	airoha_ppe_flow_update_stats(eth, f, e);
> >  	airoha_ppe_foe_flow_remove_entry(eth->ppe, e);
> 
> [Severity: Low]
> Could the final stats miss packets that the hardware forwards between
> these two calls?
> 
> The counters are read without ppe_lock while the FOE entry is still BIND,
> so the PPE keeps forwarding and counting. After that,
> airoha_ppe_foe_flow_remove_entry() takes ppe_lock and does:
> 
> airoha_ppe_foe_remove_flow()
>   e->data.ib1 |= FIELD_PREP(AIROHA_FOE_IB1_BIND_STATE, AIROHA_FOE_STATE_INVALID);
>   airoha_ppe_foe_commit_entry(ppe, &e->data, e->hash, false)
>     airoha_ppe_foe_flow_stats_update()
>       airoha_ppe_foe_flow_stat_entry_reset()
>     airoha_ppe_foe_commit_sram_entry()
> 
> Packets counted after the snapshot and before the reset are lost. So are
> packets counted after the reset and before the SRAM write that
> invalidates the entry.
> 
> The window is small. Since the goal here is accurate final counters,
> would it be better to invalidate the hardware entry first, then read and
> reset the counter, all under ppe_lock?
> 
> >  	rhashtable_remove_fast(&eth->flow_table, &e->node,
> >  			       airoha_flow_table_params);
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113031251.2437074-2-pablo%40netfilter.org