Re: [PATCH net-next] net: dsa: yt921x: Add limited ACL flow statistics support

[David Yang]

On Sun, Jun 14, 2026 at 8:45 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email says "I thought this is a false positive and the register is
> Clear-on-Read but you seem to be writing 0 to it when allocating."

...

> [Low]
> On rule delete the priv->flowstats_map bit is cleared, but
> YT921X_FLOWSTATn_CTRL(statid) is not written to clear FLOWSTAT_CTRL_EN.
> The counter remains physically enabled with TYPE_FLOW until something
> else reprograms it.
>
> Today this is benign because acl_commit() zeros the action register so
> no rule references the counter, and reuse rewrites CTRL. However, the
> header file enumerates other types (TYPE_CPU_CODE, TYPE_DROP_CODE,
> TYPE_PORT) for the same slot pool. If a future caller picks one of
> these types without first disabling the slot, could it observe stale
> counts from the previous flow user?

It's an absolute hardware counter that requires manual resetting before using.

> [Medium]
> This block runs unconditionally for every ACL rule, regardless of
> whether the user requested statistics. When all 64 counters are in use
> (statid >= YT921X_FLOWSTAT_NUM), the rule is still installed but
> silently without stats; the user only learns by querying and getting
> -EOPNOTSUPP from yt921x_acl_stat().
>
> There is no NL_SET_ERR_MSG_MOD() at install time and no kernel log
> message. The same function explicitly emits
>
>         NL_SET_ERR_MSG_MOD(extack, "No more meters available");
>
> for the meter pool right above this hunk.
>
> Should the flow-stat path either be gated on whether the rule actually
> needs stats (so unrelated rules do not consume counters), or surface a
> diagnostic via extack when the pool is exhausted? Otherwise an old set
> of un-queried rules can starve newer rules that do need stats.

As I know there is no way to specify whether the rule needs a stat; it
always assumes rule has one.

> [Low]
> After the YT921X_FLOWSTATn_CTRL write succeeds, the bare "return res;"
> on a failing YT921X_FLOWSTATn_STAT write leaves the counter enabled in
> hardware while priv->flowstats_map remains clear. Likewise, if any
> later step (kvmemdup, yt921x_acl_commit) fails, the err: label does not
> unwind the CTRL write or the meter configuration done in the prior
> hunk.
>
> The same asymmetric rollback pre-exists for meters and is implicitly
> healed by the next allocation reprogramming the slot, but the new
> flow-stat path's bare returns bypass the err: label entirely. Should
> this teardown path be made symmetric?

No traffic will be measured by the counter. Any subsequent calls that
pick the meter / counter should reinit and reset it before activating
it for themselves.