Re: [PATCH net-next v2 6/9] net: sparx5: vcap: add lpm vcap implementation

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Mon, 2026-08-17 at 14:13 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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: sparx5: vcap: add lpm vcap implementation
> 
> This adds a VCAP_TYPE_LPM instance to the sparx5 and lan969x VCAP instance
> tables and extends the per-vtype switches in sparx5_vcap_impl.c so the LPM
> VCAP reuses the Super VCAP cache, update and move helpers.  On sparx5 the
> IS0 CLM-0 instance drops from two Super VCAP blocks to one to make room.
> 
> A few questions below.
> 
> > diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_vcap_impl.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_vcap_impl.c
> > index 543a1f2bf6bd9..6e6a1f833deac 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_vcap_impl.c
> > +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_vcap_impl.c
> > @@ -82,4 +82,16 @@ const struct sparx5_vcap_inst lan969x_vcap_inst_cfg[] = {
> >               .count = 1024,
> >               .ingress = false,
> >       },
> > +     {
> > +             .vtype = VCAP_TYPE_LPM,
> > +             .vinst = 0,
> > +             .map_id = 6,
> > +             .lookups = SPARX5_LPM_LOOKUPS,
> > +             .lookups_per_instance = SPARX5_LPM_LOOKUPS,
> > +             .first_cid = SPARX5_VCAP_CID_LPM_L0,
> > +             .last_cid = SPARX5_VCAP_CID_LPM_MAX,
> > +             .blockno = 5,
> > +             .blocks = 1,
> > +             .ingress = true,
> > +     },
> >  };
> 
> [Severity: Low]
> Does the commit message match what happens on lan969x?  The message says:
> 
>     "The LPM VCAP is allocated from the Super VCAP Blocks, which is shared
>      amongst different VCAPs. All blocks are used, so we steal a block from
>      IS0, which had 2 assigned."
> 
> The lan969x hunk here is purely additive.  Before the patch
> lan969x_vcap_inst_cfg[] used blocks 0-4 (IS2-0 = 0, IS2-1 = 1, IS0-0 = 2,
> IS0-1 = 3, IS0-2 = 4), each with .blocks = 1, and this new entry takes
> block 5 without reducing anything.
> 
> So nothing is stolen from IS0 on lan969x, and "all blocks are used" does
> not hold for that platform.  Could the message say where lan969x block 5
> comes from?
> 
> Related to that: sparx5_vcap_block_alloc() derives the address range
> directly from blockno without ever consulting VCAP_SUPER_CORE_CNT:
> 
>         admin->first_valid_addr = cfg->blockno * SUPER_VCAP_BLK_SIZE;
>         admin->last_used_addr = admin->first_valid_addr +
>                 cfg->blocks * SUPER_VCAP_BLK_SIZE;
> 
> so for lan969x this maps addresses 15360..18431.  Is block 5 actually
> present in the lan969x Super VCAP?  I could not confirm the lan969x core
> count from the tree.

You are right on both counts, and the block accounting is correct even though
the changelog describing it is not.

The two platforms differ in a way the message flattens into one story:

  platform   Super VCAP blocks   assigned before   LPM block   IS0 loses
  sparx5     10                  0-9, 2 per inst   9           yes
  lan969x    6                   0-4, 1 per inst   5           no

I will update the commit message to reflect the state of things on both sparx5
and lan969x.

> 
> > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c b/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c
> > index cf332de6bf736..20450462e2d80 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c
> > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c
> > @@ -54,8 +54,8 @@ const struct sparx5_vcap_inst sparx5_vcap_inst_cfg[] = {
> >               .lookups_per_instance = SPARX5_IS0_LOOKUPS / 3,
> >               .first_cid = SPARX5_VCAP_CID_IS0_L0,
> >               .last_cid = SPARX5_VCAP_CID_IS0_L2 - 1,
> > -             .blockno = 8, /* Maps block 8-9 */
> > -             .blocks = 2,
> > +             .blockno = 8, /* Maps block 8 */
> > +             .blocks = 1,
> >               .ingress = true,
> >       },
> >       {
> 
> [Severity: Low]
> The message also says:
> 
>     "Since we statically assign blocks, this means IS0 has half the
>      available address space for vcap rules, compared to before."
> 
> Is "IS0" the right scope here?  Only the CLM-0 instance changes; CLM-1
> (blocks 6-7) and CLM-2 (blocks 4-5) keep two blocks each, so IS0 loses 1
> of 6 blocks in total.
> 
> What actually halves is the address space of chains
> SPARX5_VCAP_CID_IS0_L0..SPARX5_VCAP_CID_IS0_L2 - 1 (1000000-1199999),
> which goes from 6144 to 3072 rule addresses.  Could the message be
> narrowed to that range?
> 

Correct, "IS0" is too broad and the numbers you give are exact. Only the CLM-0
instance changes; CLM-1 keeps blocks 6-7 and CLM-2 keeps blocks 4-5, so IS0
goes from six blocks to five rather than losing half of anything.

The affected range is CLM-0's, first_cid SPARX5_VCAP_CID_IS0_L0 through
last_cid SPARX5_VCAP_CID_IS0_L2 - 1, so chains 1000000-1199999. With
SPARX5_IS0_LOOKUPS at 6 and lookups_per_instance at 6 / 3, that instance covers
IS0 lookups 0 and 1, and its address space goes from 6144 to 3072.

I will fold this into the commit message as well.


> > @@ -124,6 +124,18 @@ const struct sparx5_vcap_inst sparx5_vcap_inst_cfg[] = {
> >               .count = 12288, /* Addresses according to datasheet */
> >               .ingress = false,
> >       },
> > +     {
> > +             .vtype = VCAP_TYPE_LPM,
> > +             .vinst = 0,
> > +             .map_id = 6,
> > +             .lookups = SPARX5_LPM_LOOKUPS,
> > +             .lookups_per_instance = SPARX5_LPM_LOOKUPS,
> > +             .first_cid = SPARX5_VCAP_CID_LPM_L0,
> > +             .last_cid = SPARX5_VCAP_CID_LPM_MAX,
> > +             .blockno = 9,
> > +             .blocks = 1,
> > +             .ingress = true,
> > +     },
> >  };
> 
> [Severity: Medium]
> This registers chain ids VCAP_CID_PREROUTING_L0 (6000000) through
> SPARX5_VCAP_CID_LPM_MAX (6099999) as an ingress VCAP admin on both
> platforms, but the per-vtype switches in sparx5_tc_flower.c were not
> given a VCAP_TYPE_LPM arm.  Is that intentional?
> 
> Before the patch, sparx5_tc_flower() rejected chain 6000000 because no
> admin matched:
> 
>         admin = vcap_find_admin(vctrl, fco->common.chain_index);
>         if (!admin) {
>                 NL_SET_ERR_MSG_MOD(fco->common.extack, "Invalid chain");
> 
> Now the same request resolves to the LPM admin and continues into
> sparx5_tc_flower_replace(), where sparx5_tc_add_rule_counter() falls
> into its default arm:
> 
>         default:
>                 pr_err("%s:%d: vcap type: %d not supported\n",
>                        __func__, __LINE__, admin->vtype);
>                 break;
> 
> That arm still returns 0, so does the rule end up being built without a
> counter id?  And for an "action pass" filter, sparx5_tc_set_actionset()
> hits its default arm and returns -EINVAL with a bare pr_err() and no
> extack.  sparx5_tc_action_trap() has no LPM arm either.
> 
> So something like:
> 
>         tc filter add dev eth0 ingress chain 6000000 protocol ip \
>             flower dst_ip 10.0.0.0/8 action pass
> 
> now reaches those pr_err() calls from a netlink request, unratelimited,
> and reports an opaque -EINVAL.  Should the tc-flower path reject the LPM
> chain explicitly instead?  I checked at 010230ef1142 and VCAP_TYPE_LPM
> never appears in sparx5_tc_flower.c, so nothing later in the series adds
> these arms.
> 

Not intentional, and yes, the tc-flower path should reject the LPM chain
explicitly. The LPM VCAP is meant to be system-only, owned by the router, and
not reachable from tc at all. v3 does that.

Rather than add VCAP_TYPE_LPM arms to those switches, v3 rejects the chain up
front, which also means the LPM range stays indistinguishable from a chain that
maps to no VCAP.

> [Severity: Medium]
> This isn't a bug introduced by this patch, but the new entry adds an
> eighth iteration to a loop that leaks on failure.  In
> sparx5_vcap_init(), sparx5->vcap_ctrl = ctrl is stored before the loop
> and an allocation failure just returns:
> 
>         admin = sparx5_vcap_admin_alloc(sparx5, ctrl, cfg);
>         if (IS_ERR(admin)) {
>                 err = PTR_ERR(admin);
>                 pr_err("%s:%d: vcap allocation failed: %d\n", __func__, __LINE__, err);
>                 return err;
>         }
> 
> ctrl is not freed, mutex_destroy(&ctrl->lock) is not called, and the
> vcap_admin objects already put on ctrl->list (each with three
> STREAMSIZE cache buffers) are not unwound.
> 
> The caller cannot clean up either, because cleanup_ports sits below the
> cleanup_vcap label in sparx5_main.c:
> 
>         err = sparx5_vcap_init(sparx5);
>         if (err) {
>                 dev_err(sparx5->dev, "Failed to initialize VCAP\n");
>                 goto cleanup_ports;
>         }
>         ...
>         cleanup_vcap:
>                 sparx5_vcap_deinit(sparx5);
>         cleanup_ports:
>                 sparx5_destroy_netdevs(sparx5);
> 
> so sparx5_vcap_deinit() never runs for this failure.  Should the failure
> path unwind the admins allocated so far, or should the goto target be
> cleanup_vcap?
> 

Confirmed. The goto target should be cleanup_vcap. No extra unwind code is
needed, because sparx5_vcap_deinit() already does the right thing: it
NULL-guards ctrl, iterates ctrl->list with list_for_each_entry_safe so
a partial list is fine, frees all three cache buffers plus the admin via
sparx5_vcap_admin_free(), then destroys the mutex and frees ctrl. There is
no double-free risk either, since the admin that failed is already freed inside
sparx5_vcap_admin_alloc() before it returns ERR_PTR. And cleanup_vcap falls
through to cleanup_ports, so the netdevs are still destroyed.

I would rather not carry that in this series. It is a memory leak on a probe
error path with nothing to do with L3 routing, it is pre-existing and belongs
in the net tree.


> [ ... ]
> 
> > @@ -195,6 +213,7 @@ static void _sparx5_vcap_range_init(struct sparx5 *sparx5,
> >       u32 size = count - 1;
> > 
> >       switch (admin->vtype) {
> > +     case VCAP_TYPE_LPM:
> >       case VCAP_TYPE_IS0:
> >       case VCAP_TYPE_IS2:
> >               spx5_wr(VCAP_SUPER_CFG_MV_NUM_POS_SET(0) |
> 
> [Severity: Medium]
> This is a pre-existing issue, but the new LPM block now inherits it.
> sparx5_vcap_block_init() calls this helper with one less than the number
> of addresses in the range:
> 
>         _sparx5_vcap_range_init(sparx5, admin, admin->first_valid_addr,
>                                 admin->last_valid_addr -
>                                         admin->first_valid_addr);
> 
> The range is inclusive, as set up in sparx5_vcap_block_alloc():
> 
>         admin->first_valid_addr = cfg->blockno * SUPER_VCAP_BLK_SIZE;
>         admin->last_used_addr = admin->first_valid_addr +
>                 cfg->blocks * SUPER_VCAP_BLK_SIZE;
>         admin->last_valid_addr = admin->last_used_addr - 1;
> 
> and here size = count - 1 is programmed into VCAP_SUPER_CFG_MV_SIZE, so
> the INITIALIZE shot clears MV_SIZE + 1 == count addresses.
> 
> That count is an address count is visible from the other user of the same
> callback in vcap_api.c:
> 
>         vctrl->ops->init(ri->ndev, admin, ri->addr, ri->size);
> 
> where ri->size of 1 erases exactly one address.
> 
> Does this leave the address equal to admin->last_valid_addr uninitialized
> at probe, even though it is handed out as a usable rule address?  For the
> LPM instance that would be 30719 on sparx5 and 18431 on lan969x, i.e. a
> row of the longest-prefix-match table used by the router in the later
> "net: sparx5: add L3 FIB, nexthop and neighbour entry management" patch.
> 
> Would passing admin->last_valid_addr - admin->first_valid_addr + 1 (or
> cfg->blocks * SUPER_VCAP_BLK_SIZE) be correct here?
> 
> [ ... ]

Yes on all of it, and the +1 form is the one to use. The range is inclusive so
its length is last_valid_addr - first_valid_addr + 1, the helper clears
MV_SIZE + 1 addresses as you say, and the result is that first_valid_addr
through last_valid_addr - 1 get cleared while last_valid_addr does not. 30719
on sparx5 and 18431 on lan969x are right.

Prefer last_valid_addr - first_valid_addr + 1 over
cfg->blocks * SUPER_VCAP_BLK_SIZE. The latter is only correct for the three
Super VCAP types and would be wrong for ES0 and ES2, which derive the range
from cfg->count, and sparx5_vcap_block_init() has no cfg argument anyway. No
overflow risk from the +1 either: MV_SIZE is 16 bits and the widest instance is
two Super VCAP blocks, 6144 addresses.


The scope is wider than LPM or even sparx5. Every VCAP instance on both
platforms is affected, since the ES0 and ES2 arms reach the same helper through
the same block_init. And lan966x_vcap_block_init() has the identical expression
feeding a __lan966x_vcap_range_init() that also does MV_SIZE_SET(count - 1), so
lan966x has the same bug.


As with the sparx5_vcap_init() leak, I would rather not carry this in the
series. It is pre-existing, it affects lan966x as well as sparx5, and it wants
a fixes tag and the net tree.