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

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.

> 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?

> @@ -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.

[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?

[ ... ]

> @@ -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?

[ ... ]