Re: [PATCH v3 net] octeontx2-af: Block VFs from clobbering special CGX PKIND state

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260713121902.3938875-1-rkannoth@marvell.com?part=1
---
octeontx2-af: Block VFs from clobbering special CGX PKIND state

This commit prevents CGX-mapped VFs from resetting the shared LMAC RX
PKIND and default TX parse configuration when the LMAC is running with
HiGig2 or EDSA parsing enabled. It adds cgx_get_pkind() and
rvu_cgx_is_pkind_config_permitted() to gate the affected register
writes.

[Medium]
The three call sites of rvu_cgx_is_pkind_config_permitted() interpret a
false return in two different ways.  In nix_interface_init() and
rvu_mbox_handler_nix_lf_alloc() a false result silently skips the write,
but the TX branch of rvu_npc_set_parse_mode() converts it into an
-EINVAL to the caller.

Pre-patch, rvu_mbox_handler_npc_set_pkind() (via rvu_npc_set_parse_mode)
always wrote NIX_AF_LFX_TX_PARSE_CFG.  Post-patch, a VF whose parent
PF's LMAC is programmed with NPC_RX_HIGIG_PKIND or NPC_RX_EDSA_PKIND
now receives a hard error where the call previously succeeded.

The same path is also reached from rvu_nix_lf_teardown() ->
rvu_npc_set_parse_mode(), where the -EINVAL is ignored but the intended
reset write to NIX_AF_LFX_TX_PARSE_CFG is skipped.

Is this mbox-level behavior change intended?  The commit message uses
the word "skip", which reads like a silent no-op, but this call site
aborts with -EINVAL rather than skipping.

[ ... ]

[Medium]
This check-then-write pair reads the CGX RX pkind register (via
cgx_get_pkind() inside rvu_cgx_is_pkind_config_permitted()) without
holding cgxd->lock, but the corresponding cgx_set_pkind() writers were
just wrapped in cgxd->lock in this same patch.

Consider the following interleaving:

    CPU0 (VF nix_lf_alloc)                CPU1 (PF set_parse_mode)
    -----------------------------------   ---------------------------
    rvu_cgx_is_pkind_config_permitted()
       cgx_get_pkind() -> normal pkind
       returns true
                                          mutex_lock(&cgxd->lock);
                                          cgx_set_pkind(..., HIGIG);
                                          mutex_unlock(&cgxd->lock);
    rvu_write64(NIX_AF_LFX_TX_PARSE_CFG,
                NPC_TX_DEF_PKIND);

Can a concurrent PF that transitions the LMAC to NPC_RX_HIGIG_PKIND or
NPC_RX_EDSA_PKIND between the check and the write flip the state the
check depends on?

The nix_interface_init() call site holds cgxd->lock across both the
check and the writes; should the same pattern be applied here (and in
the TX branch of rvu_npc_set_parse_mode()) for consistency?

[Medium]
NIX_AF_LFX_TX_PARSE_CFG(nixlf) is indexed by NIX LF, not by LMAC.
Skipping the write when the LMAC is in HiGig/EDSA mode means the
register is left at whatever value it currently holds — either the
hardware reset value on a fresh LF, or stale contents from a previous
owner when an LF is reused across configurations.

If a HiGig VF needs NIX_AF_LFX_TX_PARSE_CFG programmed (for example to
NPC_TX_HIGIG_PKIND) to emit correctly formatted frames, where is that
value installed?  This hunk unconditionally elides the write when the
LMAC is HiGig/EDSA, and the teardown path via rvu_nix_lf_teardown() ->
rvu_npc_set_parse_mode() is likewise blocked from restoring the
default, so a subsequent non-HiGig owner reusing the same LF would
observe the previous owner's TX parse configuration.