Re: [net-next,v8,02/13] octeontx2-af: npc: cn20k: KPM profile changes

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
octeontx2-af: npc: cn20k: KPM profile changes

This patch adds support for the new KPM (Kangaroo Processing Module) profile
configuration in CN20K silicon. It groups 16 KPUs into 8 KPM profiles, where
each KPM contains two KPUs (KPM0 = KPU0 + KPU8, etc.).

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c

[ ... ]

> +static void
> +npc_config_kpmaction(struct rvu *rvu, int blkaddr,
> +		     const struct npc_kpu_profile_action *kpuaction,
> +		     int kpm, int entry, bool pkind)
> +{
> +	struct npc_kpm_action0 action0 = {0};
> +	struct npc_kpu_action1 action1 = {0};

[ ... ]

> +	action0.capture_ltype = kpuaction->ltype & 0xf;
> +	action0.capture_flags = kpuaction->flags & 0xf;
> +	action0.ptr_advance = kpuaction->ptr_advance;

The npc_kpm_action0 structure defines capture_flags as a 4-bit field, while
kpuaction->flags is a u8 (8 bits). KPU profiles use flag values such as
NPC_F_LA_U_HAS_IH_NIX (0x20), NPC_F_LA_U_HAS_HIGIG2 (0x40), and
NPC_F_LB_U_UNK_ETYPE (0x80), which exceed what 4 bits can represent.

Is this truncation intentional for the CN20K hardware register layout? If
so, will the parser functionality work correctly when these upper flag bits
are silently discarded?

This was raised by Jakub Kicinski in v6 review:
https://lore.kernel.org/all/20260129051303.376551-1-kuba@kernel.org/

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c

[ ... ]

> @@ -1982,6 +1990,15 @@ static void rvu_npc_hw_init(struct rvu *rvu, int blkaddr)
>  	hw->npc_pkinds = (npc_const1 >> 12) & 0xFFULL;
>  	hw->npc_kpu_entries = npc_const1 & 0xFFFULL;
>  	hw->npc_kpus = (npc_const >> 8) & 0x1FULL;
> +	/* For Cn20k silicon, check if enhanced parser
> +	 * is present, then set the NUM_KPMS = NUM_KPUS / 2 and
> +	 * number of LDATA extractors per KEX.
> +	 */
> +	if (is_cn20k(rvu->pdev) && (npc_const1 & BIT_ULL(62))) {
> +		hw->npc_kpms = hw->npc_kpus / 2;
> +		hw->npc_kex_extr = (npc_const1 >> 36) & 0x3FULL;
> +	}

[ ... ]

> @@ -2116,7 +2133,10 @@ int rvu_npc_init(struct rvu *rvu)
>  		return -ENOMEM;
>
>  	/* Configure KPU profile */
> -	npc_parser_profile_init(rvu, blkaddr);
> +	if (is_cn20k(rvu->pdev))
> +		npc_cn20k_parser_profile_init(rvu, blkaddr);
> +	else
> +		npc_parser_profile_init(rvu, blkaddr);

In rvu_npc_hw_init(), hw->npc_kpms is set only when both is_cn20k() returns
true AND bit 62 is set in npc_const1:

    if (is_cn20k(rvu->pdev) && (npc_const1 & BIT_ULL(62))) {
        hw->npc_kpms = hw->npc_kpus / 2;

However, rvu_npc_init() calls npc_cn20k_parser_profile_init() based solely
on is_cn20k(), without checking for enhanced parser support.

If CN20K hardware exists without enhanced parser support (bit 62 not set),
npc_kpms will be 0. The npc_cn20k_parser_profile_init() function would then:

    for (idx = 0; idx < hw->npc_kpms; idx++)  /* loops 0 times */
        ...disable KPM entries...

    for (idx = 0; idx < hw->npc_kpus; idx++)
        rvu_write64(rvu, blkaddr, NPC_AF_KPUX_CFG(idx), 0x00);  /* disables all KPUs */

    npc_program_kpm_profile(rvu, blkaddr, hw->npc_kpms);  /* loops 0 times, never re-enables KPUs */

Should the condition at line 2136 match the condition at line 1997 to
ensure KPUs are not left disabled when enhanced parser support is absent?
-- 
pw-bot: cr