Re: [PATCH v28 net-next 02/10] net/nebula-matrix: add core driver architecture and HW layer initialization

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

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

- [Medium] nbl_probe() starts claiming every device in nbl_id_table()
  from this patch onwards (patch 1 of the series deliberately returned…
- [Medium] nbl_get_func_param() grants the chip-global management role
  solely from host/guest-visible PCI topology: `if…
- [Low] The constant NBL_BAR2_MAX_LEN (nbl_hw_leonis.h) is named as the
  *maximum* mailbox BAR length (0x300) but nbl_hw_init_leonis() uses it…

--

net/nebula-matrix: add core driver architecture and HW layer initialization

This patch adds the nbl_adapter/nbl_core/nbl_common_info device context,
turns nbl_probe() into a real probe that enables the device and maps the
Leonis BARs, and adds the capability parsing that decides whether a
function gets the chip-global control role.

A few questions below about the commit message wording and about how
has_ctrl and NBL_BAR2_MAX_LEN are used.

    Note:
    This commit binds the PCI device during probe, but does not create or
    register netdev. Netdev support will be added in later patches of this
    series.

[Severity: Medium]
Is this accurate for the series as posted?  Grepping the driver directory
at the tip of the series for register_netdev, alloc_etherdev or netdev
returns no matches, so no network interface appears to be created in any
of the following patches.

From this patch onward nbl_probe() claims every ID in nbl_id_table(),

enables bus mastering, maps the BARs, and (on the control function)
programs chip-global registers, but returns success without providing a
netdev or any other user-visible interface:

	adapter = nbl_core_init(pdev, &param);
	...
	pci_set_drvdata(pdev, adapter);
	return 0;

Would it be better to either keep the -ENODEV stub until the netdev
patch lands, or reword this note so it does not promise a netdev that
the series does not add?

[ ... ]

[ ... ]

[ ... ]

[ ... ]

[Severity: Low]
This isn't a bug, but the constant name and its use disagree.
NBL_BAR2_MAX_LEN reads as a maximum, while the test here treats it as the
minimum required register extent:

	if (bar_len < NBL_BAR2_MAX_LEN)
		... -EINVAL;

The mapping and the recorded window size then use the full advertised BAR
length rather than the constant:

	hw_mgt->mailbox_bar_hw_addr = pcim_iomap(pdev, NBL_MAILBOX_BAR,
						 bar_len);
	...
	hw_mgt->mailbox_bar_size = bar_len;

Could the constant be renamed to something like NBL_BAR2_MIN_LEN or
NBL_BAR2_REG_LEN, or alternatively the pcim_iomap() length and
mailbox_bar_size clamped to it?

[ ... ]

[ ... ]

[Severity: Medium]
Is PCI topology a sufficient source for the chip-global control role?

The comment states that hardware and firmware forbid passing a PF through
to a VM, but nothing in the kernel enforces that.  A host admin can bind
vfio-pci to any of these PFs; inside the guest pdev->is_virtfn is 0 and
VMMs commonly place the first assigned function at devfn 0x00, so:

	if ((PCI_FUNC(pdev->devfn) == 0) && !pdev->is_virtfn)
		param->caps.has_ctrl = 1;

is satisfied for a non-management PF.

has_ctrl then selects privileged, chip-wide behaviour.  In
nbl_hw_init_leonis() it picks the ~64 MiB global register window instead
of the 8 KiB net-only window:

	if (common->has_ctrl) {
		...
		hw_size = PAGE_ALIGN_DOWN(NBL_MEM_BAR_TOTAL_SIZE -
					  NBL_RDMA_NOTIFY_LEN);

Later in the series it also gates nbl_res_start() -> nbl_res_init_pf_num()
/ nbl_intr_mgt_start(), which read global registers and own MSI-X map
entries of remote PFs, and nbl_dev_init() -> nbl_dev_setup_ctrl_dev(),
which programs chip-wide datapath registers and the mailbox qinfo routing
table for every PF.  nbl_res_remove_leonis() acknowledges this with "the
control PF, which may hold maps for remote PFs that are still bound".

Could the control role be derived from a firmware/hardware identity read
(and default to non-ctrl when that read is unavailable), rather than from
devfn?  Whether the ASIC actually filters BAR0 writes from a
non-management function to the global blocks is not visible from the
source here, so it would help to have that stated explicitly.

[ ... ]

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914123429.56596-1-illusion.wang%40nebula-matrix.com