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

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] rvu_dbg_rvu_pf_cgx_map_display()
  (drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c) now derives…
- [Medium] The patch fixes only one of the driver's PF→PCI-device
  conversions, leaving the same 'RVU PF n is on bus n + 1' assumption in…
- [Low] The commit fixes a user-visible debugfs defect (the
  rvu_pf_cgx_map file silently omitted every mapped PF when the AF was…

--

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

The rvu_pf_cgx_map debugfs file looked up each RVU PF PCI device assuming
PF0 always lives on PCI bus 1. This commit takes the base bus number from
rvu->pdev->bus->number instead, so the per-PF lookup uses pf + start.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> index 22ee996768796..bf0302c0c1fc9 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> @@ -829,6 +829,7 @@ 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;

[Severity: Medium]
The bus half of the lookup is now taken from the real device, but the
segment stays a literal in rvu_dbg_rvu_pf_cgx_map_display().  Should the
domain come from the same PCI hierarchy?

The driver already has an idiom for this in get_cpt_pf_num():

  drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c:get_cpt_pf_num() {
	...
	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);
	...
  }

On a system whose RVU AF is not enumerated in segment 2 - the same class
of non-default enumeration this commit describes - would every iteration
hit the "if (!pdev) continue;" path, leaving rvu_pf_cgx_map with only its
header line?

And if some unrelated device happens to sit at segment 2 / bus
(start + pf) / devfn 0, is there anything after the lookup that validates
vendor/device ID before dev_name(&pdev->dev) is printed next to real
CGX/LMAC/channel state?

>  	mac_ops = get_mac_ops(rvu_first_cgx_pdata(rvu));
> @@ -837,11 +838,16 @@ static int rvu_dbg_rvu_pf_cgx_map_display(struct seq_file *filp, void *unused)
>  		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 bus number from PCI device.
> +	 */
> +	start = rvu->pdev->bus->number;

[Severity: Medium]
This new comment states an invariant that several peer sites in the same
driver contradict, and there is no shared helper keeping them in sync.

The closest one is in the same file, cgx_print_dmac_flt(), which does the
identical PF-to-PCI-device conversion with the old base:

  drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c:cgx_print_dmac_flt() {
	...
	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;
	...
	seq_printf(s, "%s  PF%d  %9s  %9s",
		   dev_name(&pdev->dev), pf, bcast, mcast);
	...
  }

On the topologies this commit targets, does the CGX dmac_filter debugfs
file end up empty, or print real CGX DMAC control/CAM values next to an
unrelated device's name?

The following two are pre-existing and not introduced by this patch, but
they carry the same assumption in non-debug paths.

get_cpt_pf_num() scans buses i + 1:

  drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c:get_cpt_pf_num() {
	...
		pdev = pci_get_domain_bus_and_slot(domain_nr, i + 1, 0);
	...
  }

With a shifted bus base, would rvu->cpt_pf_num stay at -1, so that
is_cpt_pf()/is_cpt_vf() never recognize the real CPT PF/VF mailbox
requests?

  drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c:is_cpt_pf() {
	int cpt_pf_num = rvu->cpt_pf_num;

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

rvu_sdp_init() encodes the exact inverse of the new comment and derives an
rvu->pf[] index from it:

  drivers/net/ethernet/marvell/octeontx2/af/rvu_sdp.c:rvu_sdp_init() {
	...
		/* 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]];
	...
  }

rvu->pf is only devm_kcalloc(rvu->dev, hw->total_pfs, ...) in
rvu_setup_hw_resources().  Can this index exceed total_pfs when the
discovered SDP PF bus number is shifted, and can that write out of bounds
through pfvf->sdp_info?

That same sdp_pf_num feeds is_sdp_pf() -> is_pf_cgxmapped(), which is the
only per-PF filter in the loop this patch fixes, so the stale assumption
still reaches the corrected listing.

>  	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: Low]
This isn't a bug, but the commit has no Fixes: tag even though it repairs a
user-visible debugfs defect (rvu_pf_cgx_map silently omitting every mapped
PF when the AF is not on bus 1).

git blame on the pre-patch tree attributes both the "domain = 2;"
assignment and the "pci_get_domain_bus_and_slot(domain, pf + 1, 0)" lookup
in rvu_dbg_rvu_pf_cgx_map_display() to commit e2fb3730386541
("octeontx2-af: Add debug messages for failures").  Could
"Fixes: e2fb3730386541" be added so stable tooling can determine the
affected history?

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