Re: [PATCH net] octeontx2-af: Fix limiting SRIOV VF count logic

[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.
---
octeontx2-af: Fix limiting SRIOV VF count logic

This restricts the LBK channel based clamping of the AF VF count in
rvu_enable_sriov() so that it only applies when the SR-IOV VF device ID
matches the LBK RVU AFVF device.  The clamp is moved inside a new
conditional block and the absolute 62 VF limit is replaced with a limit
derived from the reported channel count.

A few questions about the new logic below.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> index ffba56ee8a60c..67a7766f000b2 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> @@ -3463,6 +3463,8 @@ int rvu_get_num_lbk_chans(void)
>  	return ret;
>  }
>  
> +#define PCI_DEVID_OCTEONTX2_RVU_AFVF	0xA0F8
> +

[Severity: Low]
This isn't a bug, but does this duplicate a constant that already exists
in a header this file includes?  af/rvu.h already has:

	#define	RVU_LBK_VF_DEVID	0xA0F8
	static inline bool is_lbk_vf(struct rvu *rvu, u16 pcifunc)
	{
		return (!(pcifunc & ~RVU_PFVF_FUNC_MASK) &&
			(rvu->vf_devid == RVU_LBK_VF_DEVID));
	}

and the same macro name with the same value also exists in the companion
driver in nic/otx2_common.h.  With this hunk there are three copies of
0xA0F8 to keep in sync.  Would rvu->vf_devid == RVU_LBK_VF_DEVID work
here instead?

>  static int rvu_enable_sriov(struct rvu *rvu)
>  {
>  	struct pci_dev *pdev = rvu->pdev;
> @@ -3481,24 +3483,25 @@ static int rvu_enable_sriov(struct rvu *rvu)
>  		return 0;
>  	pci_read_config_word(pdev, pos + PCI_SRIOV_VF_DID, &rvu->vf_devid);
>  
> -	chans = rvu_get_num_lbk_chans();
> -	if (chans < 0)
> -		return chans;
> -
>  	vfs = pci_sriov_get_totalvfs(pdev);
> -
> -	/* Limit VFs in case we have more VFs than LBK channels available. */
> -	if (vfs > chans)
> -		vfs = chans;
> -
>  	if (!vfs)
>  		return 0;
>  
> -	/* LBK channel number 63 is used for switching packets between
> -	 * CGX mapped VFs. Hence limit LBK pairs till 62 only.
> -	 */
> -	if (vfs > 62)
> -		vfs = 62;
> +	if (rvu->vf_devid == PCI_DEVID_OCTEONTX2_RVU_AFVF) {
> +		chans = rvu_get_num_lbk_chans();
> +		if (chans < 0)
> +			return chans;
> +
> +		/* Limit VFs in case VFs are more than LBK channels available */
> +		if (vfs > chans)
> +			vfs = chans;

[Severity: Medium]
The "if (!vfs) return 0;" test now runs before the clamp instead of after
it, and the clamp below has no lower bound.  Can vfs end up zero or
negative here?

rvu_get_num_lbk_chans() returns a plain 16-bit register field:

	ret = (readq(base + 0x10) >> 32) & 0xffff;

so it can never be negative and the "chans < 0" test cannot catch a
report of 0, 1 or 2 channels.  In that case vfs becomes 0, -1 or -2 and
nothing revalidates it before:

	rvu->vfs = vfs;

	err = rvu_mbox_init(rvu, &rvu->afvf_wq_info, TYPE_AFVF, vfs,
			    rvu_afvf_mbox_handler, rvu_afvf_mbox_up_handler);

For vfs == 0, rvu_mbox_init() passes all of its allocation checks because
bitmap_zalloc(0) and kcalloc(0, ...) return ZERO_SIZE_PTR rather than
NULL, otx2_mbox_setup() accepts ndevs == 0, and otx2_mbox_regions_init()
in af/mbox.c then does:

	mbox->hwbase = hwbase[0];

on that ZERO_SIZE_PTR array.

For vfs < 0 the num argument promotes to a huge unsigned value in
BITS_TO_LONGS(), the bitmap allocation fails with -ENOMEM, and
rvu_probe() treats that as fatal:

	err = rvu_enable_sriov(rvu);
	if (err) {
		dev_err(dev, "%s: Failed to enable sriov\n", __func__);
		goto err_dl;
	}

Also, rvu->vfs is used to build INTR_MASK(vfs) masks in
rvu_enable_afvf_intr() / rvu_disable_afvf_intr(), where a negative shift
count is undefined.

Before this change the order was clamp first, then "if (!vfs) return 0;",
so a zero channel count simply skipped SR-IOV enablement and vfs could
never go negative.  Would keeping the zero test after the clamp, or
bounding the clamp at zero, preserve that?

> +
> +		/* LBK channel number 63 is used for switching packets between
> +		 * CGX mapped VFs. Hence limit LBK pairs.
> +		 */
> +		if (vfs > (chans - 2))
> +			vfs = chans - 2;
> +	}

[Severity: Medium]
The commit message only describes gating the LBK clamp on the VF device
ID, but this hunk also replaces the absolute cap

	if (vfs > 62)
		vfs = 62;

with a count-relative one.  Is that change intentional, and could it be
mentioned in the changelog?

The two forms only agree when LBK reports exactly 64 channels.  For a
smaller count the new code creates up to two fewer AF VFs than before
(chans == 32 gives min(totalvfs, 30) where it used to give
min(totalvfs, 32)), and the preceding

	if (vfs > chans)
		vfs = chans;

becomes dead because the following clamp is strictly tighter.

For a larger count the cap no longer protects channel 63, which is still
a hardcoded absolute index in this tree:

	af/rvu.h:	#define RVU_SWITCH_LBK_CHAN	63
	af/rvu_switch.c: req.index = (lbkid << 8) | RVU_SWITCH_LBK_CHAN;
	af/rvu_nix.c:	 cfg = enable ? (BIT_ULL(12) | RVU_SWITCH_LBK_CHAN) : 0;

while nix_interface_init() maps an LBK VF index one-to-one onto a channel:

	pfvf->rx_chan_base = rvu_nix_chan_lbk(rvu, lbkid, vf);

so VF number 64 would own the channel the switch and representor paths
also program.

The retained comment still justifies the limit with the absolute channel
number 63 while the arithmetic is now relative to the reported count, so
the comment and the code no longer say the same thing.

>  
>  	/* Save VFs number for reference in VF interrupts handlers.
>  	 * Since interrupts might start arriving during SRIOV enablement

[Severity: Medium]
This is a pre-existing issue and not something introduced by this patch,
but it sits in the rvu_mbox_init() call that follows this hunk in
rvu_enable_sriov().  Does rvu->ng_rvu get leaked here?

rvu_mbox_init() allocates unconditionally on every call:

	ng_rvu_mbox = kzalloc_obj(*ng_rvu_mbox);
	if (!ng_rvu_mbox) {
		err = -ENOMEM;
		goto free_bitmap;
	}
	...
	rvu->ng_rvu = ng_rvu_mbox;

and it is called twice per probe: once as TYPE_AFPF from rvu_probe() and
once as TYPE_AFVF from rvu_enable_sriov().  The second call overwrites
the pointer stored by the first.

On cn20k the orphaned struct also owns the AFPF mailbox qmem
(cn20k/mbox_init.c stores rvu->ng_rvu->pf_mbox_addr = mbox_addr), and
rvu_remove() only frees the surviving pointer:

	if (is_cn20k(rvu->pdev))
		cn20k_free_mbox_memory(rvu);
	kfree(rvu->ng_rvu);

so that DMA allocation is never released either.