Re: [PATCH net-next v3] net: dsa: yt921x: Add ACL support

[David Yang]

Here are the reasons why I believe some of the changes should not be
made (to suppress further AI comments on the same line)

On Wed, May 27, 2026 at 9:55 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> [Low]
> Could the L4 entry slot be allocated only when at least one mask is
> non-zero?
>
> The IPV4_ADDRS, IPV6_ADDRS and ETH_ADDRS branches above all compute
> want_dst/want_src first and only call yt921x_acl_entries_find() when a
> mask is set, but here the L4 entry is reserved as soon as the dissector
> key is present.
>
> A rule that exposes the PORTS key with both masks zero will consume one
> of the eight per-block entry slots with an all-zero key/mask, and force
> an L4 type match the user did not request.

Unreachable path. TC ensures that both masks cannot be zero.

> [Medium]
> If any of the register writes between the "Open the block" and
> "Close the block" sequence fails, the function returns immediately
> with the block still latched in MODIFY mode and no compensating close
> is performed.
>
>     res = yt921x_reg_write(priv, YT921X_ACL_BLK_CMD, ctrl);
>     if (res)
>         return res;
>     ...
>     res = yt921x_reg64_write(priv, YT921X_ACLn_KEYm(blkid, o), ...);
>     if (res)
>         return res;       /* block still in MODIFY */
>     ...
>
> Would a goto-style unwind that always issues the close-block command
> and then returns the original error be safer here?

Leaving a block open does no harm, no ACL or other functionalities are
blocked from processing. Any subsequent ACL commit request will
overwrite BLK_CMD without any error.

> [Medium]
> What happens to a meter that has been programmed in hardware when one
> of these later steps fails?
>
> yt921x_meter_config() writes YT921X_METERn_CTRL with METER_EN set
> before any of the kernel-side bookkeeping has succeeded. If the
> following devm_kzalloc(aclblk), devm_kmemdup(aclrule), or
> yt921x_acl_commit() call fails, the function returns or jumps to err:
> without:
>
>   - reverting the meter hardware programming, and
>   - reserving the meter id in priv->meters_map.
>
> The slot stays active in hardware with the user's policer config, and
> only set_bit(meterid, priv->meters_map) on the success path tracks it.
> A subsequent successful add will pick the same id from
> find_first_zero_bit() and reprogram it, but in the meantime the device
> polices traffic through that meter.

In that case, no traffic will be policed by that meter since the ACL
rule referring to it is not activated (and then rejected). Any
subsequent ACL rule that picks the meter will reset it before
activating itself.

> [Low]
> Is there a reason cls_flower_stats is not implemented?
>
> The action register definitions YT921X_ACL_ACTa_FLOWSTAT_EN and
> YT921X_ACL_ACTa_FLOWSTAT_ID_M suggest the hardware can report per-rule
> match counters, but the bits are never set and no stats callback is
> hooked up, so tc -s filter show will not show match counts for
> offloaded rules.

FLOWSTAT has its own quirk that deserves justification on a separate commit.