This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: microchip: vcap: expose helpers in vcap api and update debugfs
This adds two new client API helpers, vcap_val_add_rule() and
vcap_rule_mod_action_bit(), renames VCAP_CID_PREROUTING to
VCAP_CID_PREROUTING_L0 and adds VCAP_USER_L3. The debugfs dump learns
about the IP4_XIP and IP6_XIP key fields, and the action-field printer
mask expression is changed to a GENMASK() based form.
A couple of questions below.
> diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> index 6e1ee15b82b7e..1ad2c44da8bd7 100644
> --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> @@ -2389,6 +2389,19 @@ int vcap_add_rule(struct vcap_rule *rule)
> }
> EXPORT_SYMBOL_GPL(vcap_add_rule);
>
> +/* Validate and add rule to a VCAP instance */
> +int vcap_val_add_rule(struct vcap_rule *rule, u16 l3_proto)
> +{
> + int err;
> +
> + err = vcap_val_rule(rule, l3_proto);
> + if (err)
> + return err;
> +
> + return vcap_add_rule(rule);
> +}
> +EXPORT_SYMBOL_GPL(vcap_val_add_rule);
[Severity: Medium]
This isn't a pattern introduced by this patch (every existing caller
already does vcap_val_rule() followed by vcap_add_rule() without holding
vctrl->lock across both), but now that it is packaged as an exported
helper, is the check-then-act window worth documenting or closing?
vcap_val_rule() ends with the capacity test and runs entirely without
vctrl->lock:
/* Finally check if there is room for the rule in the VCAP */
return vcap_rule_space(ri->admin, ri->size);
vcap_rule_space() reads the shared instance state unlocked:
if (admin->last_used_addr - size < admin->first_valid_addr) {
...
return -ENOSPC;
}
vcap_add_rule() then takes the mutex and the append branch of
vcap_insert_rule() consumes address space without re-testing against
admin->first_valid_addr:
ri->addr = vcap_next_rule_addr(admin->last_used_addr, ri);
admin->last_used_addr = ri->addr;
If two adds to the same VCAP instance interleave between validation and
insertion on a nearly full instance, can both pass the stale space check
and the second one push last_used_addr below first_valid_addr, so the
rule is encoded into addresses owned by another VCAP block instead of
returning -ENOSPC?
For the users present at this commit I could not construct such an
interleaving: the L3/LPM adders added later in the series all run from an
ordered workqueue under sparx5->router->lock, and the TC and matchall
paths are serialized by rtnl. The one case I could not rule out is a tc
filter installed directly into the LPM chain range concurrently with a FIB
work item, since sparx5_tc_flower_replace() resolves the target instance
purely from the user-supplied chain index:
vrule = vcap_alloc_rule(vctrl, ndev, fco->common.chain_index,
VCAP_USER_TC,
fco->common.prio, 0);
Is such a tc configuration rejected somewhere? If not, would holding
vctrl->lock across validate plus insert, or re-checking vcap_rule_space()
in the append branch of vcap_insert_rule(), be preferable?
[ ... ]
> diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
> b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
> index e0c65c7ab23e1..36150822ce153 100644
> --- a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
> +++ b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
> @@ -133,7 +135,12 @@ vcap_debugfs_show_rule_actionfield(struct vcap_control
> *vctrl,
> out->prf(out->dst, "%d", value[0]);
> break;
> case VCAP_FIELD_U32:
> - fmsk = (1 << actionfield[action].width) - 1;
> + if (action == VCAP_AF_MAC_LSB || action == VCAP_AF_MAC_MSB) {
> + hex = true;
> + break;
> + }
> + fmsk = actionfield[action].width ?
> + GENMASK(actionfield[action].width - 1, 0) : 0;
> val = *(u32 *)value;
> out->prf(out->dst, "%u", val & fmsk);
> break;
[Severity: Low]
This isn't a bug introduced by this patch, but should the sibling
key-field printer get the same treatment? vcap_debugfs_show_rule_keyfield()
in the same file still evaluates the shift first and only repairs the
value afterwards:
} else {
u32 fmsk = (1 << keyfield[key].width) - 1;
if (keyfield[key].width == 32)
fmsk = ~0;
out->prf(out->dst, "%u/%u", data->u32.value & fmsk,
data->u32.mask & fmsk);
}
The printed value is correct because of the width == 32 fixup, but the
1 << 32 on a signed int has already been evaluated at that point, which
looks like the same undefined shift the commit message describes fixing
in the action-field printer.
This branch is reachable today: VCAP_KF_IP_PAYLOAD_5TUPLE is
VCAP_FIELD_U32 with width 32 in is0_normal_5tuple_ip4_keyfield for
sparx5 and lan969x, and in the lan966x is1 keyset, and VCAP_KF_RT_FRMID
is another W32 U32 key in lan966x is1. None of them are in the %pI4h or
hex special cases, so dumping a rule carrying one of those keys via the
VCAP debugfs rule dump would hit the shift with a width of 32. Would
converting this site to the same GENMASK() form make sense while the
area is being touched?
Reported by an automated review pipeline; both items are pre-existing and
neither is claimed by the commit message, so they may well be better as
separate cleanups.
