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

[netdev-bot+sashiko]

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?

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> new file mode 100644
> index 0000000000000..cf40ddc451921
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> @@ -0,0 +1,153 @@

[ ... ]

> +	if (common->has_ctrl) {
> +		/*
> +		 * Hardware layout: MEMORY BAR total size is 64M.

[ ... ]

> +		hw_size = PAGE_ALIGN_DOWN(NBL_MEM_BAR_TOTAL_SIZE -
> +					  NBL_RDMA_NOTIFY_LEN);
> +		hw_mgt->hw_addr =
> +			pcim_iomap(pdev, NBL_MEMORY_BAR,
> +				   hw_size);
> +	} else {
> +		if (bar_len < NBL_REG_NET_ONLY_LEN) {

[ ... ]

> +		hw_size = NBL_REG_NET_ONLY_LEN;
> +		hw_mgt->hw_addr = pcim_iomap(pdev, NBL_MEMORY_BAR,
> +					     hw_size);
> +	}

[ ... ]

> +	if (bar_len < NBL_BAR2_MAX_LEN) {
> +		dev_err(&pdev->dev, "MAILBOX BAR length %pa too small\n",
> +			&bar_len);
> +		ret = -EINVAL;
> +		goto setup_mgt_fail;
> +	}
> +	hw_mgt->mailbox_bar_hw_addr = pcim_iomap(pdev, NBL_MAILBOX_BAR,
> +						 bar_len);

[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?

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> index 199626159a4ce..f2552bc732937 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> @@ -8,16 +8,108 @@

[ ... ]

> +static void nbl_get_func_param(struct pci_dev *pdev, kernel_ulong_t driver_data,
> +			       struct nbl_init_param *param)
> +{
> +	param->caps.has_net = !!(driver_data & BIT(NBL_CAP_HAS_NET_BIT));
> +
> +	/*
> +	 * Hardware fixed rule: physical PF0 is the only management PF with
> +	 * global ctrl capability. All PFs share identical PCI device ID, so
> +	 * distinguish control PF via physical function ID.
> +	 *
> +	 * Hardware & firmware design FORBID passing any PF through to virtual
> +	 * machines, there is no scenario where a non-management PF appears
> +	 * as Func 0 inside guest. Thus using PCI_FUNC(pdev->devfn) to identify
> +	 *  control PF is safe on our platform.
> +	 */
> +	if ((PCI_FUNC(pdev->devfn) == 0) && !pdev->is_virtfn)
> +		param->caps.has_ctrl = 1;
> +}

[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.

> +
>  static int nbl_probe(struct pci_dev *pdev,
>  		     const struct pci_device_id *id)
>  {
> -	return -ENODEV;
> +	struct nbl_init_param param = { { 0 } };

[ ... ]

> +	nbl_get_func_param(pdev, id->driver_data, &param);
> +	/* never return fail when DMA_BIT_MASK(64) */
> +	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +
> +	pci_set_master(pdev);
> +
> +	adapter = nbl_core_init(pdev, &param);
> +	if (IS_ERR(adapter)) {
> +		dev_err(dev, "Nbl adapter init fail: %pe\n", adapter);
> +		err = PTR_ERR(adapter);
> +		goto adapter_init_err;
> +	}
> +	pci_set_drvdata(pdev, adapter);
> +	return 0;

[ ... ]

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