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

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The control-PF teardown and init added here rely on
  nbl_probe_chip_deps() to guarantee that no sibling PF probes unless…
- [Low] The commit message says 'The firmware handles global hardware
  cleanup and QINFO routing state reclamation asynchronously after…
- [Low] The new callers disagree with how nbl_chan_teardown_queue()
  actually behaves.
- [Low] Two comments on the same control-PF remove sequence contradict
  each other.

--

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

This commit adds nbl_dev.c with nbl_dev_init() and nbl_dev_remove(). They
set up the mailbox channel queue and cleanup work, program mailbox QINFO
routing on the control PF, and call chip-level init_module/deinit_module.
Teardown destroys the mailbox queue before notifying firmware through
deinit_module.

> 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 000000000000..e094b97acdfb
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> @@ -0,0 +1,253 @@

[ ... ]

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

[ ... ]

> +	ret = nbl_dev_setup_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);
> +	if (ret)
> +		goto err_cleanup;

[ ... ]

> +	return 0;
> +err_cleanup:
> +	cancel_work_sync(&common_dev->clean_mbx_task);
> +	nbl_dev_remove_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);
> +	nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX, NULL);
> +	return ret;
> +}
> +
> +static void nbl_dev_remove_common_dev(struct nbl_adapter *adapter)
> +{
> +	struct nbl_dev_mgt *dev_mgt = adapter->core.dev_mgt;
> +	struct nbl_dev_common *common_dev = dev_mgt->common_dev;
> +	int ret;
> +
> +	if (!common_dev)
> +		return;
> +	cancel_work_sync(&common_dev->clean_mbx_task);
> +	ret = nbl_dev_remove_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);
> +	if (ret)
> +		dev_warn(&adapter->pdev->dev, "mailbox queue teardown failed, inflight DMA may exist: %d\n",
> +			 ret);

[Severity: Low]
Can nbl_dev_remove_chan_queue() return non-zero here? Both exits of
nbl_chan_teardown_queue() return 0: the early return when the channel is
not active, and the normal path after draining. That makes this dev_warn()
unreachable.

The message also suggests teardown can fail with DMA still in flight. If
it could, nbl_dev_remove() would still go on to call deinit_module, and
nbl_hw_deinit_module() assumes no mailbox DMA is left at that point. Does
this warning cover a real failure mode, or can it be dropped?

There is a related issue on the err_cleanup path of
nbl_dev_setup_common_dev() above. That path only runs when setup_queue
failed, and chan_info->active is only set to true when setup succeeds. So
the teardown lands here:

nbl_chan_teardown_queue() {
	if (!READ_ONCE(chan_info->active)) {
		dev_warn(chan_mgt->common->dev, "channel not active, skip duplicate teardown\n");
		return 0;
	}
	...
}

Won't this log "skip duplicate teardown" on the first cleanup after a
failed setup, when no duplicate teardown has happened?

[ ... ]

> +/*
> + * Tear down control device: deinit_module sets driver_status=false
> + * to notify firmware to clean all per-PF hardware state (including
> + * qinfo registers).  The qinfo map programmed in nbl_dev_init() via
> + * nbl_dev_setup_chan_qinfo() is not explicitly cleared; firmware
> + * handles it on driver_status change.
> + *
> + * Teardown ordering guarantee: every non-management PF creates a
> + * consumer->control PF device link in its probe path, so the driver
> + * core always unbinds all siblings before allowing the control PF to
> + * be detached (sysfs unbind, driver unregister and hot-unplug alike).

[Severity: Low]
This comment conflicts with the one in nbl_res_remove_leonis(), which
nbl_core_remove() calls right after nbl_dev_remove():

nbl_res_remove_leonis() {
	...
	/*
	 * Tear down all MSI-X maps before destroying coherent tables.
	 * This is critical on the control PF, which may hold
	 * maps for remote PFs that are still bound.
	 */
	...
}

If the driver core always unbinds all siblings before the control PF, no
remote PF can still be bound at that point. Which of these does the
teardown actually rely on? One of the two comments probably needs
updating.

This patch relies on the stronger claim to justify calling deinit_module
unconditionally. The next comment describes a case where it does not
fully hold.

> + * nbl_probe_chip_deps() defers a sibling's probe until the control PF
> + * is fully bound (DL_DEV_DRIVER_BOUND), so no sibling can race its
> + * mailbox setup against this deinit.

[Severity: Medium]
Is this exclusion guaranteed? nbl_probe_chip_deps() in nbl_main.c checks
the supplier state without a lock, then creates the link, and rejects only
one link state:

	if (READ_ONCE(mgt->dev.links.status) != DL_DEV_DRIVER_BOUND) {
		pci_dev_put(mgt);
		return -EPROBE_DEFER;
	}

	link = device_link_add(&pdev->dev, &mgt->dev,
			       DL_FLAG_AUTOREMOVE_CONSUMER);
	...
	if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND)
		return -EPROBE_DEFER;

device_link_init_status() in drivers/base/core.c sets
DL_STATE_SUPPLIER_UNBIND only while the supplier is DL_DEV_UNBINDING:

	case DL_DEV_UNBINDING:
		link->status = DL_STATE_SUPPLIER_UNBIND;
		break;
	default:
		link->status = DL_STATE_DORMANT;

Two cases seem to get past the check:

(a) The control PF finishes unbinding, including nbl_dev_remove() ->
deinit_module, between the check and device_link_add(). It is then
DL_DEV_NO_DRIVER, so the link starts as DL_STATE_DORMANT and passes.

(b) The control PF has started re-probing (DL_DEV_PROBING) while the
sibling is probing. The link starts as DL_STATE_CONSUMER_PROBE and also
passes.

In both cases the sibling goes on to nbl_core_init() -> nbl_dev_init()
and sets up its mailbox queue. In (a) this happens right after the
control PF set driver_status=false, while the firmware's asynchronous
per-PF cleanup may still be running. In (b) it races the control PF's
nbl_dev_setup_chan_qinfo() and init_module programming.

The sibling and the control PF hold separate device_lock()s, so the
driver core does not serialize these paths.

The commit message makes the same claim:

  Device link dependency established during non-control PF probe ensures
  sibling PFs are unbound before the control PF is removed.

Should nbl_probe_chip_deps() re-check the supplier state after
device_link_add() and also reject DORMANT links and links to a supplier
that is still probing?

[ ... ]

> +/*
> + * Teardown order: Stop mailbox channel and drain all inflight DMA first,
> + * then invoke deinit_module to notify firmware.
> + *
> + * This intentionally breaks strict init/teardown mirror symmetry due to
> + * hardware constraint: firmware may perform asynchronous global hardware
> + * cleanup once driver_status=false is set. We must guarantee no ongoing
> + * mailbox DMA before deinit_module to avoid invalid DMA write.

[Severity: Low]
The commit message says:

  The firmware handles global hardware cleanup and QINFO routing state
  reclamation asynchronously after driver status is marked inactive.

This comment likewise refers to "asynchronous global hardware cleanup".
Is that accurate? This path reaches nbl_hw_deinit_module() in
nbl_hw_leonis.c through deinit_module, and that function documents a
narrower contract:

 * Setting driver status to false notifies firmware to clean up per-PF
 * hardware state such as qinfo registers.
 *
 * Note: firmware does NOT automatically revert chip-wide registers
 * configured in this init flow. Those chip-wide settings remain valid
 * until chip reset or explicitly overwritten by driver.

The earlier "add chip-wide hardware init/deinit implementation" commit
also says that firmware does not restore chip-wide datapath registers on
deinit.

So after the control PF is removed, the registers programmed by
nbl_dp_init() and nbl_intf_init() stay in place until chip reset. Could
the commit message and this comment say per-PF cleanup instead of global
cleanup?

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