Re: [PATCH net v3] net: airoha: keep PPE offload when NPU refuses flow stats

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · 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).

- [High] Kernel oops (NULL iomem dereference) on the exact refusal path
  this patch claims to handle.
- [Medium] Even apart from the NULL dereference, the 'continue without
  stats' fallback still programs the PPE as if NPU-assisted flow…
- [Medium] The failure paths in airoha_npu_ppe_stats_setup(), including
  the new zero-address -ENOSPC exit, return without resetting…
- [Low] The new PSE_FC_CFG (0x0098) and PSE_PORT6_SHARE_DIS defines sit
  between PSE_FQ_CFG (0x008c) and PSE_FQ_LIMIT_MASK…

--

net: airoha: keep PPE offload when NPU refuses flow stats

With this patch, airoha_ppe_offload_setup() logs a warning and keeps going
when the NPU refuses the flow statistics request, where it used to abort.
It also treats a zero npu_stats_addr from the firmware as a refusal. When
flow stats entries are available, it clears PSE_PORT6_SHARE_DIS in
PSE_FC_CFG.

[Severity: Medium]
Can this leave an old npu->stats mapping in use?

This exit returns without touching npu->stats, and so do the other failure
exits in airoha_npu_ppe_stats_setup(). struct airoha_npu is the NPU
platform device's drvdata, and npu->stats is a devm mapping owned by
npu->dev. Both outlive any airoha_eth instance.

airoha_ppe_deinit() does not clear npu->stats either:

airoha_ppe_deinit() {
	...
	if (npu) {
		synchronize_rcu();
		npu->ops.ppe_deinit(npu);
		airoha_npu_put(npu);
	}
	...
}

Suppose an earlier airoha_eth instance mapped the stats region. Then a
later airoha_ppe_offload_setup(), for example after an eth unbind/rebind
or a module reload, gets a refusal or a zero npu_stats_addr. In that case
npu->stats still points at the old region.

The caller now continues past the failure. So
airoha_ppe_foe_flow_stats_reset(), airoha_ppe_foe_flow_stats_update() and
airoha_ppe_foe_entry_get_stats() would read and write that region as live
counters, even though the firmware may have reassigned it after
ppe_deinit().

Separately, each successful setup adds a new devm_ioremap() that is only
released when the NPU device unbinds. That part predates this patch.

[Severity: Medium]
When the NPU refuses flow stats, sram_num_stats_entries still comes from
the static airoha_ppe_get_num_stats_entries() check. Is the PPE then still
programmed as if NPU flow counting were active?

In that case airoha_ppe_hw_init() still takes PPE_STATS_NUM_ENTRIES out
of the SRAM data table and still clears PSE_PORT6_SHARE_DIS.

After that, airoha_ppe_foe_commit_entry() calls
airoha_ppe_foe_flow_stats_update() for every non-WLAN bound flow. That
function checks only the static count, then redirects the entry to the
NPU:

airoha_ppe_foe_flow_stats_update() {
	...
	nbq = pse_port == 1 ? 6 : 5;
	*ib2 &= ~(AIROHA_FOE_IB2_NBQ | AIROHA_FOE_IB2_PSE_PORT |
		  AIROHA_FOE_IB2_PSE_QOS);
	*ib2 |= FIELD_PREP(AIROHA_FOE_IB2_PSE_PORT, 6) |
		FIELD_PREP(AIROHA_FOE_IB2_NBQ, nbq);
}

The original CHANNEL/QID and NBQ/PSE_PORT/PSE_QOS/FAST_PATH values are
saved in ACTDP and TUNNEL_MTU, so the firmware can count the packet and
forward it on. The firmware has just refused the flow stats service. If
it does not do this for port 6 traffic, could offloaded flows be dropped
or misrouted?

That would contradict the commit message, which says PPE "continues to
operate without NPU flow counters while retaining normal offload
functionality". Even if the firmware does forward correctly, a refusal
still costs PPE_STATS_NUM_ENTRIES SRAM data entries per PPE.

Right now the NULL dereference in airoha_ppe_offload_setup() (below)
hides this. A fix for that crash would need to handle this as well.

[Severity: High]
Does this path oops in the very refusal case the patch is meant to handle?

npu is allocated with devm_kzalloc() in airoha_npu_probe().
airoha_npu_ppe_stats_setup() sets npu->stats only after devm_ioremap()
succeeds. On every failure exit npu->stats stays NULL: an
airoha_npu_send_msg() error, the new zero address -ENOSPC exit, or an
ioremap failure.

Right after airoha_ppe_hw_init(), airoha_ppe_offload_setup() calls
airoha_ppe_foe_flow_stats_reset(). Its only gate is the static
CONFIG_NET_AIROHA_FLOW_STATS/SoC entry count:

airoha_ppe_offload_setup()
  airoha_ppe_foe_flow_stats_reset()
    airoha_ppe_foe_flow_stat_entry_reset()
      memset_io(&npu->stats[index], 0, sizeof(*npu->stats));

That writes through a NULL npu->stats, starting at index 0.

Even if that call were skipped, rcu_assign_pointer() still publishes
eth->npu, and two more paths use npu->stats without checking it.

The first runs whenever a forwarded flow binds:

airoha_ppe_foe_commit_entry()
  airoha_ppe_foe_flow_stats_update()
    airoha_ppe_foe_flow_stat_entry_reset()

The second is airoha_ppe_foe_entry_get_stats(), reached from
FLOW_CLS_STATS and the debugfs FOE dump. It checks only npu:

	npu = rcu_dereference(eth->npu);
	if (npu) {
		...
		memcpy_fromio(&npu_stats, &npu->stats[index],
			      sizeof(*npu->stats));

Before this patch, a failure took goto error_npu_put and offload setup
failed cleanly. Rather than just ignoring the error, could the driver
record whether the NPU accepted the stats setup and check that before
every use of npu->stats?

[Severity: Low]
This isn't a bug, but the new defines sit between PSE_FQ_CFG and its
field mask, so PSE_FQ_LIMIT_MASK now reads as a field of PSE_FC_CFG. The
only user of the mask, in airoha_eth.c, pairs it with PSE_FQ_CFG:

	fq_limit = airoha_fe_get(eth, PSE_FQ_CFG, PSE_FQ_LIMIT_MASK);

The new block also breaks address order: 0x0098 now comes before
REG_FE_PSE_BUF_SET (0x0090) and REG_PSE_SHARE_USED_THD (0x0094). Could
PSE_FC_CFG and PSE_PORT6_SHARE_DIS go after the REG_PSE_SHARE_USED_THD
block?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015045607.12999-1-sayantann11%40gmail.com