Re: [PATCH v33 net-next 7/8] net/nebula-matrix: add common/ctrl dev init/remove operation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The comment in nbl_dev_init_msix_cnt()
  (drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c) says the…
- [Low] nbl_dev_setup_common_dev() (nbl_dev.c) explains why VSI/ETH
  identity is not fetched at init: "the control PF's responder may still…
- [Low] This patch adds the first caller of cfg_chan_qinfo_map_table
  (nbl_dev_setup_chan_qinfo()) and puts the responder-teardown…

--

net/nebula-matrix: add common/ctrl dev init/remove operation

This commit adds nbl_dev.c with nbl_dev_init() and nbl_dev_remove(). These
set up the mailbox clean work and the mailbox channel queue. On the control
PF they also program the mailbox QINFO routing and run init_module. The new
entry points are called from nbl_core_init() and nbl_core_remove().

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> new file mode 100644
> index 0000000000000..01287c39a9655
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> @@ -0,0 +1,303 @@

[ ... ]

> +static void nbl_dev_init_msix_cnt(struct nbl_dev_mgt *dev_mgt)
> +{
> +	struct nbl_dev_common *dev_common = dev_mgt->common_dev;
> +	struct nbl_msix_info *msix_info = &dev_common->msix_info;
> +
> +	/* Reserve one MSI-X vector for the mailbox.  Nothing here
> +	 * allocates or requests it: vectors are allocated and the handler
> +	 * attached when the device is started, after probe, from this
> +	 * count.
> +	 */

[Severity: Low]
This isn't a bug, but is "after probe" accurate here?

nbl_dev_start() arrives later in the series, in "net/nebula-matrix: add
common dev start/stop operation". By the end of the series it is called
from inside nbl_probe():

nbl_probe() {
    ...
	pci_set_master(pdev);
	pci_set_drvdata(pdev, adapter);
	err = nbl_core_start(adapter);
	if (err) {
		nbl_core_remove(adapter);
		pci_clear_master(pdev);
	}
	return err;
}

So if start fails, the probe fails. The comment above
nbl_hw_clear_stale_status() in nbl_hw_leonis.c agrees: it says sibling
PFs' RPCs "time out in nbl_dev_start() and their probes fail".

The "after probe" wording is still there at the end of the series. Should
it say the vectors are allocated in the start path, during probe?

> +	msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].num = 1;
> +}
> +
> +/* ----------  Channel config  ---------- */
> +static void nbl_dev_setup_chan_qinfo(struct nbl_dev_mgt *dev_mgt, u8 chan_type)
> +{

[ ... ]

> +	chan_ops->cfg_chan_qinfo_map_table(priv, common->hw_bus, common->devid);
> +}

[Severity: Low]
This isn't a bug, but this patch adds the first caller of
cfg_chan_qinfo_map_table, and that makes a few existing comments wrong.
All three are unchanged at the end of the series.

nbl_chan_cfg_qinfo_map_table() in nbl_channel.c still says:

	 * 0xfe/0xfc/0xf0.  Nothing enforces that here: the resource layer
	 * that validates the mask, and every caller of this op, arrive in
	 * later patches of this series.

But nbl_res_init_pf_num() already rejects non-contiguous masks and any
pf_num outside {1, 2, 4}, and this patch adds the caller.

nbl_res_init_pf_num() in nbl_resource_leonis.c says:

	 * this running first: resource init completes before the channel
	 * layer is set up.

nbl_core_init() actually calls nbl_chan_init_common() before
nbl_res_init_leonis(). The dependency only holds because the QINFO
programming happens here in nbl_dev_init(), which runs after resource
init.

The kernel-doc for struct nbl_dispatch_ops in nbl_def_dispatch.h says the
teardown order "is documented with the code that performs it, in the next
patch in this series". This is that patch, and the text went into
nbl_disp_remove(), which does no teardown itself.

Could these three comments be updated? Could the "next patch in this
series" reference also be removed before the code is merged?

> +/* ----------  Dev init process  ---------- */
> +static int nbl_dev_setup_common_dev(struct nbl_adapter *adapter)
> +{

[ ... ]

> +	nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX,
> +				   &common_dev->clean_mbx_task);
> +	/*
> +	 * VSI/ETH identity is deliberately not fetched here: it is only
> +	 * known to the control PF, so it has to come from a mailbox RPC,
> +	 * and the control PF's responder may still be probing while this
> +	 * function runs.  The fetch is therefore left to the start path.
> +	 */

[Severity: Low]
Can the control PF's responder still be probing when a sibling PF reaches
this point?

nbl_probe() calls nbl_probe_chip_deps() before nbl_core_init(). For a
non-control PF, nbl_probe_chip_deps() holds func 0's device lock and
defers the probe:

nbl_probe_chip_deps() {
    ...
	if (mgt->dev.driver != pdev->dev.driver ||
	    mgt->dev.links.status != DL_DEV_DRIVER_BOUND) {
		device_unlock(&mgt->dev);
		pci_dev_put(mgt);
		return -EPROBE_DEFER;
	}
    ...
}

driver_bound() only sets DL_DEV_DRIVER_BOUND after func 0's entire
->probe() has returned, and that includes nbl_dev_init() and init_module.

The header comment for nbl_dev_remove_ctrl_dev() in this same file says the
opposite:

 * between.  A sibling therefore cannot
 * reach its mailbox setup before the control PF is fully probed, nor

The commit message of the last patch in the series says the same thing.

Should this comment give a different reason for deferring the fetch? It is
still present at the end of the series.

[ ... ]

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