Re: [PATCH net] octeontx2-af: nix: Fix NIX LF allocation for representor devices

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] Representor TX-link inconsistency inside the AF:
  rvu_mbox_handler_nix_lf_alloc() now reports `rsp->tx_link =…
- [Medium] Two uncoordinated writers now program the same
  NIX_AF_TL3_TL2X_LINKX_CFG(schq, LBK link) register for a representor's…

--

octeontx2-af: nix: Fix NIX LF allocation for representor devices

This patch reports the LBK TX link and configures the LBK PKIND for
representor NIX LFs during nix_lf_alloc, and skips the MAC address copy
into the allocation response for representors.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index 153eb57bad067..ed62ef336a730 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> @@ -1713,6 +1713,9 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
>  	if (is_rep_dev(rvu, pcifunc)) {
>  		pfvf->tx_chan_base = RVU_SWITCH_LBK_CHAN;
>  		pfvf->tx_chan_cnt = 1;
> +		/* Setting the TX link as that of LBK */
> +		rsp->tx_link = hw->cgx_links;

[Severity: Medium]
Should nix_get_tx_link() also learn about representors here?

The response now tells the PF driver that the representor's TX link is
LBK link 0 (hw->cgx_links), but nix_get_tx_link() in the same file has no
is_rep_dev() case:

	if (is_lbk_vf(rvu, pcifunc)) {/* LBK links */
		return hw->cgx_links;
	} else if (is_pf_cgxmapped(rvu, pf)) {
		...
	}

	/* SDP link */
	return hw->cgx_links + hw->lbk_links;

The representor is a dedicated PF, so is_lbk_vf() is false (non-zero PF
bits) and is_pf_cgxmapped() is false, which appears to leave the SDP link
as the return value.

The sibling helpers already special-case representors, for example
nix_get_txschq_range():

	if (is_lbk_vf(rvu, pcifunc) || is_rep_dev(rvu, pcifunc)) {

and rvu_mbox_handler_nix_set_hw_frs():

	} else if (is_rep_dev(rvu, pcifunc)) {
		link = hw->cgx_links + 0;
	}

That value from nix_get_tx_link() is what rvu_mbox_handler_nix_txsch_alloc()
uses as the representor's aggregation-level schq index:

		link = nix_get_tx_link(rvu, pcifunc);

		if (lvl >= hw->cap.nix_tx_aggr_lvl) {
			start = link;
			end = link;

so the representor would be handed TL1 of the SDP link, and is_valid_txschq()
would group it with SDP functions for TL1 sharing. nix_txschq_free() then
clears SW_XOFF and CFG_DONE on that same TL1:

	nix_clear_tx_xoff(rvu, blkaddr, NIX_TXSCH_LVL_TL1,
			  nix_get_tx_link(rvu, pcifunc));

On a board that also has an SDP PF, can the representor and the SDP function
end up overwriting each other's TL1 configuration this way, while the link
reported to the driver for the same LF is LBK link 0?

[Severity: Medium]
With tx_link now pointing at the LBK link, does the PF driver end up
programming the same NIX_AF_TL3_TL2X_LINKX_CFG register that the AF switch
path programs, but with a different value?

The driver side in otx2_txschq_config() writes:

		if (lvl == hw->txschq_link_cfg_lvl &&
		    !is_otx2_sdp_rep(pfvf->pdev)) {
			req->reg[2] = NIX_AF_TL3_TL2X_LINKX_CFG(schq, hw->tx_link);
			req->regval[2] = BIT_ULL(13) | BIT_ULL(12) | prio;

that is ENA | BP_ENA | relative channel 0, and is_otx2_sdp_rep() only matches
0xA0F7, so the 0xA0E0 representor is not excluded.

The AF side programs the same registers for the same representor schqs in
rvu_nix_tx_tl2_cfg():

	cfg = enable ? (BIT_ULL(12) | RVU_SWITCH_LBK_CHAN) : 0;
	lbk_link_start = hw->cgx_links;
	...
			rvu_write64(rvu, blkaddr,
				    NIX_AF_TL3_TL2X_LINKX_CFG(schq, lbk_link_start + lbk_links), cfg);

that is ENA | relative channel 63, without BP_ENA. Prior to this change the
driver's write targeted link 0 (a CGX link), so it did not overlap the AF's
LBK link programming.

Which write lands last looks ordering dependent. If the AF devlink is already
in switchdev mode when the representors are created,
rvu_devlink_eswitch_mode_set() returns early:

		if (rswitch->mode == mode)
			return 0;

so rvu_rep_pf_init() -> rvu_switch_enable_lbk_link() does not re-run and the
driver's relative channel 0 plus BP_ENA remains the final state, rather than
channel 63 that the switch path and the TX MCAM rule
(req.index = (lbkid << 8) | RVU_SWITCH_LBK_CHAN in rvu_rep.c) expect. In the
other order the AF write clears the driver's BP_ENA. Is one of the two writers
meant to be the owner of this register for representors?

>  		goto exit;
>  	}
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915072711.985075-1-rkannoth%40marvell.com