Re: [PATCH v2 net] octeontx2-af: fix PF/CGX debugfs PCI bus lookup

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 (1) · Low: 1

- [High] Incomplete fix: the identical PF->PCI-device lookup in the
  sibling CGX debugfs file is left broken.
- [Low] The recorded commit message describes only the bus-number change
  ('It assumed PF0 always sits on PCI bus 1 and derived other PF bus…

Pre-existing issues:
- [High] get_cpt_pf_num() (rvu_cpt.c:387-407) derives the RVU PF number
  from the PCI bus number using the assumption this patch declares…
- [Medium] The inverse bus->PF translation in rvu_sdp_init() still
  hardcodes base bus 1 and is consumed from inside the function this…

--

octeontx2-af: fix PF/CGX debugfs PCI bus lookup

This changes rvu_dbg_rvu_pf_cgx_map_display() so that the PF-to-CGX map
debugfs file derives the PCI domain and the base bus number from the AF
device (rvu->pdev->bus) instead of assuming domain 2 and bus 1. Each PF
lookup now uses pf + start.

A few questions below.

>     Use rvu->pdev->bus->number as the base bus instead, so each PF lookup
>     uses pf + start on systems where RVU functions are on contiguous buses
>     but do not start at bus 1.

[Severity: Low]
The changelog only describes the bus-number change, but the diff also
replaces domain = 2 with pci_domain_nr(rvu->pdev->bus), which is a second
independent behaviour change for systems whose RVU devices are not in PCI
domain 2.

The only mention of it is the "v1 -> v2: Added fixes tag, and fixed domain
number" note below the --- separator, which is dropped when the patch is
applied.

Since this carries a Fixes: tag, could the commit body also state the
domain change so backporters can see the full scope?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> index 22ee996768796..904374baae6f3 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> @@ -829,19 +829,25 @@ static int rvu_dbg_rvu_pf_cgx_map_display(struct seq_file *filp, void *unused)
>  	int pf, domain, blkid;
>  	u8 cgx_id, lmac_id;
>  	u16 pcifunc;
> +	u8 start;
>  
> -	domain = 2;
> +	domain = pci_domain_nr(rvu->pdev->bus);
>  	mac_ops = get_mac_ops(rvu_first_cgx_pdata(rvu));
>  	/* There can be no CGX devices at all */
>  	if (!mac_ops)
>  		return 0;
>  	seq_printf(filp, "PCI dev\t\tRVU PF Func\tNIX block\t%s\tLMAC\tCHAN\n",
>  		   mac_ops->name);
> +
> +	/* All the PF devices are on contiguous PCI bus numbers, but the PF0(AF)
> +	 * may not start from 1 always. Hence get domain and bus from PCI device.
> +	 */
> +	start = rvu->pdev->bus->number;
>  	for (pf = 0; pf < rvu->hw->total_pfs; pf++) {
>  		if (!is_pf_cgxmapped(rvu, pf))
>  			continue;
>  
> -		pdev =  pci_get_domain_bus_and_slot(domain, pf + 1, 0);
> +		pdev =  pci_get_domain_bus_and_slot(domain, pf + start, 0);
>  		if (!pdev)
>  			continue;

[Severity: High]
The subject says "PF/CGX debugfs PCI bus lookup", but the sibling CGX
debugfs lookup in the same file is left with the old assumptions.
cgx_print_dmac_flt() still has:

	pf = cgxlmac_to_pf(rvu, cgx_get_cgxid(cgxd), lmac_id);
	domain = 2;

	pdev = pci_get_domain_bus_and_slot(domain, pf + 1, 0);
	if (!pdev)
		return 0;

and it is reachable through a registered file:

	static int rvu_dbg_cgx_dmac_flt_display(struct seq_file *s, void *unused)
	{
		return cgx_print_dmac_flt(s, rvu_dbg_derive_lmacid(s));
	}

On the very systems this patch targets (AF not on bus 1, or not in domain
2), does reading the CGX dmac_filter file just produce empty output because
of the "if (!pdev) return 0;" early exit, with no header and no error?

And if the domain does happen to be 2 while the base bus differs, can the
lookup resolve an unrelated PCI device at that bus/slot and print its
dev_name() as the RVU PF?

After this change the driver holds two contradictory rules for the same
PF-to-BDF mapping. Would it make sense to convert cgx_print_dmac_flt() in
the same patch?

[Severity: High]
This isn't a bug introduced by this patch, but get_cpt_pf_num() in
rvu_cpt.c has the same wrong bus base in a non-debugfs path. It reads the
domain dynamically yet still probes bus i + 1:

	domain_nr = pci_domain_nr(rvu->pdev->bus);
	for (i = 0; i < rvu->hw->total_pfs; i++) {
		pdev = pci_get_domain_bus_and_slot(domain_nr, i + 1, 0);
		...
			cpt_pf_num = i;

With the AF at bus start != 1, either no CPT device is found and
rvu->cpt_pf_num stays -1, or the device found at bus i + 1 is really RVU PF
(i + 1 - start), so cpt_pf_num is shifted by (start - 1).

Can that shift then deny the genuine CPT PF/VF, given is_cpt_pf() /
is_cpt_vf() compare rvu_get_pf(pcifunc) against cpt_pf_num and gate
CPT_RD_WR_REGISTER, CPT_STATS and CPT_RXC_TIME_CFG with
CPT_AF_ERR_ACCESS_DENIED?

	if (rvu_get_pf(rvu->pdev, pcifunc) != cpt_pf_num)
		return false;

And with cpt_pf_num == -1, does cpt_inline_inb_lf_cmd_send() index the mbox
array out of bounds?

	req = (struct cpt_inst_lmtst_req *)
	       otx2_mbox_alloc_msg_rsp(&rvu->afpf_wq_info.mbox_up,
				       cpt_pf_num, sizeof(*req),
				       sizeof(struct msg_rsp));

otx2_mbox_alloc_msg_rsp() in mbox.c does not validate devid:

	struct otx2_mbox_dev *mdev = &mbox->dev[devid];
	...
	spin_lock(&mdev->mbox_lock);

This path is reached without any is_cpt_pf() gating on non-otx2 silicon:

	rvu_nix_lf_teardown() -> rvu_cpt_ctx_flush() ->
		cpt_inline_inb_lf_cmd_send()

Should get_cpt_pf_num() also start its scan at rvu->pdev->bus->number, or
at least should cpt_pf_num be validated before it is used as an mbox device
index?

[Severity: Medium]
This is a pre-existing issue, but the inverse bus-to-PF translation in
rvu_sdp_init() still encodes the base bus 1 that this patch declares
invalid:

	/* The RVU PF number is one less than bus number */
	sdp_pf_num[i] = pdev->bus->number - 1;
	pfvf = &rvu->pf[sdp_pf_num[i]];

Under this patch's premise, shouldn't the inverse be
pdev->bus->number - rvu->pdev->bus->number?

With a base bus start != 1, sdp_pf_num[] is offset by (start - 1), so
is_sdp_pfvf() compares rvu_get_pf(pcifunc) against a wrong PF number. Can
that make a non-SDP PF pick up SDP channel/ring parameters through
pfvf->sdp_info while the real SDP PF is treated as an ordinary PF?

This is also visible from the loop being changed here, since
is_pf_cgxmapped() consults is_sdp_pf():

	return (pf >= PF_CGXMAP_BASE && pf <= rvu->cgx_mapped_pfs) &&
		!is_sdp_pf(rvu, rvu_make_pcifunc(rvu->pdev, pf, 0));

Separately, the index is unchecked while rvu->pf is allocated with only
hw->total_pfs entries in rvu_setup_hw_resources():

	rvu->pf = devm_kcalloc(rvu->dev, hw->total_pfs,
			       sizeof(struct rvu_pfvf), GFP_KERNEL);

If pdev->bus->number - 1 >= hw->total_pfs, does &rvu->pf[sdp_pf_num[i]] and
the following pfvf->sdp_info assignment write past the array? I could not
confirm from the tree that a shipping board combines a non-1 base bus with
an SDP PF, so this part is unproven.

Since the patch makes the two halves of the bus/PF convention openly
contradictory, would it be reasonable to fix them together?

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