Re: [PATCH v29 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 5 potential
issue(s) to consider.

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

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

- [High] nbl_dev_remove_ctrl_dev() (drivers/net/ethernet/nebula-
  matrix/nbl/nbl_core/nbl_dev.c:177-183) calls disp_ops->deinit_module()…
- [High] There is no synchronization between the chip-global operations
  this patch adds and sibling (non-control) PFs that are concurrently…
- [Medium] The commit message lists "Program mailbox QINFO routing map
  (bus/devid for each PF)" under "Control device setup…
- [Medium] nbl_dev_remove_common_dev() (nbl_dev.c:128-142) reduces a
  failed mailbox queue teardown to dev_warn("mailbox queue teardown…
- [Low] nbl_dev_setup_ctrl_dev() (nbl_dev.c:144-155) contains a pointless
  round trip: `ret = disp_ops->init_module(...); if (ret) return ret;…

--

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

This patch adds the nbl_dev layer with nbl_dev_setup_common_dev() and
nbl_dev_setup_ctrl_dev() plus their teardown counterparts, and wires
nbl_dev_init() / nbl_dev_remove() into nbl_core_init() / nbl_core_remove().
Common device setup creates the mailbox channel queue and the mailbox clean
work, while control device setup runs the chip-wide init_module() callback.

A few questions below about the control-PF paths and about some commit
message and comment text that does not seem to match the code.

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

[ ... ]

> +/* ----------  Channel config  ---------- */
> +static void nbl_dev_setup_chan_qinfo(struct nbl_dev_mgt *dev_mgt, u8 chan_type)
> +{
> +	struct nbl_channel_ops *chan_ops = dev_mgt->chan_ops_tbl->ops;
> +	struct nbl_channel_mgt *priv = dev_mgt->chan_ops_tbl->priv;
> +	struct nbl_common_info *common = dev_mgt->common;
> +
> +	if (!chan_ops->check_queue_exist(priv, chan_type))
> +		return;
> +
> +	/*
> +	 * common->hw_bus is the control PF's real bus number, captured in
> +	 * nbl_res_ctrl_dev_sriov_info_init() during nbl_res_init_leonis().
> +	 * nbl_core_init() runs resource init before nbl_dev_init(), so the
> +	 * value is always initialized when this control-PF-only path runs;
> +	 * nbl_res_intr_cfg_msix_map() consumes it for cfg_msix_map() the
> +	 * same way.
> +	 */
> +	chan_ops->cfg_chan_qinfo_map_table(priv, common->hw_bus, common->devid);
> +}

[ ... ]

> +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);
> +	nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX, NULL);
> +}

[Severity: Medium]
Should the teardown failure be propagated instead of only warned about?

nbl_dev_remove_common_dev() returns void, so nbl_dev_remove() continues to
nbl_dev_remove_ctrl_dev() -> deinit_module() even after this dev_warn()
fires, and devres then releases the mailbox rings once remove() returns.

nbl_chan_teardown_queue() returns -ETIMEDOUT when the inflight TX drain
times out, and on that same branch it can also fail the trylock and skip
the hardware stop entirely:

nbl_channel.c:nbl_chan_teardown_queue() {
	...
	} else if (mutex_trylock(&chan_info->txq_lock)) {
		nbl_chan_stop_queue(chan_mgt);
		mutex_unlock(&chan_info->txq_lock);
	} else {
		dev_crit(... "zombie channel: ... HW stop skipped ...");
	...
}

nbl_chan_stop_queue() is what disables both the mailbox RX and TX queues,
so on that path the RX queue stays enabled pointing at the
dmam_alloc_coherent() rings that devres frees right after remove().

That also seems to conflict with the precondition documented in the
hardware layer:

nbl_hw_leonis.c:nbl_hw_deinit_module() {
 * Caller must ensure no new DMA is initiated after this point.
 * The mailbox channel is stopped by nbl_chan_teardown_queue()
 * before this function is called, so no in-flight mailbox DMA
 * remains.
}

Can the device write into the freed coherent mailbox buffers in that case?

> +
> +static int nbl_dev_setup_ctrl_dev(struct nbl_adapter *adapter)
> +{
> +	struct nbl_dev_mgt *dev_mgt = adapter->core.dev_mgt;
> +	struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
> +	int ret;
> +
> +	ret = disp_ops->init_module(dev_mgt->disp_ops_tbl->priv);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[Severity: Medium]
The commit message lists, under "Control device setup
(nbl_dev_setup_ctrl_dev, control PF only)":

    - Program mailbox QINFO routing map (bus/devid for each PF)

and the teardown comment below says "The qinfo map programmed in
setup_ctrl_dev".  Does nbl_dev_setup_ctrl_dev() program the QINFO map?

The body above only calls disp_ops->init_module(); the QINFO map is
programmed from nbl_dev_init() via nbl_dev_setup_chan_qinfo(), which runs
before nbl_dev_setup_common_dev() creates the mailbox queue and before
init_module():

	if (common->has_ctrl)
		nbl_dev_setup_chan_qinfo(dev_mgt, NBL_CHAN_TYPE_MAILBOX);

That call performs chip-wide register writes for every active PF:

nbl_channel.c:nbl_chan_cfg_qinfo_map_table() {
	for (func_id = 0; func_id < NBL_MAX_PF; func_id++)
		if (!(pf_mask & (1 << func_id)))
			hw_ops->cfg_mailbox_qinfo(p, func_id, bus, devid, func_id);
}

So the documented "Init order: create mailbox(common_dev) → ctrl dev init"
does not seem to describe the order in which hardware writes actually
happen, and this QINFO programming is also outside the setup_ctrl_dev_fail
unwind path.  Could the changelog and the comment be adjusted to match, or
the call moved into nbl_dev_setup_ctrl_dev()?

[Severity: Low]
This isn't a bug, but there is nothing between the call and the return, so
the local ret and the error check are redundant here:

	return disp_ops->init_module(dev_mgt->disp_ops_tbl->priv);

> +
> +/*
> + * 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 setup_ctrl_dev 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).
> + *
> + * Safety net: this registry check additionally blocks the chip-global
> + * deinit if a sibling PF is somehow still bound on the same chip
> + * (defense in depth for paths that bypass device-link ordering).
> + * Skipping deinit leaves firmware/qinfo routing alive for the siblings;
> + * their control plane stays functional at the cost of deferring the
> + * global firmware cleanup until the next control PF init_module().
> + * Direct control-PF FLR (which never runs driver teardown) cannot be
> + * guarded here.
> + */
> +static void nbl_dev_remove_ctrl_dev(struct nbl_adapter *adapter)
> +{
> +	struct nbl_dev_mgt *dev_mgt = adapter->core.dev_mgt;
> +	struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
> +
> +	disp_ops->deinit_module(dev_mgt->disp_ops_tbl->priv);
> +}

[Severity: High]
Where is the registry check that this comment and the commit message
describe?

The commit message says:

    A registry check is added as defense-in-depth, to skip global chip deinit
    if sibling PFs remain bound.

and the comment above says "this registry check additionally blocks the
chip-global deinit if a sibling PF is somehow still bound on the same chip".

The function body is a single unconditional deinit_module() call, and the
only condition below it is has_ctrl:

nbl_chip.c:nbl_res_chip_deinit_module() {
	if (!common->has_ctrl)
		return;
	hw_ops->deinit_module(res_mgt->hw_ops_tbl->priv);
}

The only chip-dependency code in the driver is nbl_probe_chip_deps() in
nbl_main.c, and it just creates a device link:

	if (!device_link_add(&pdev->dev, &mgt->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {

There is no registry, sibling list or sibling-bound check anywhere for the
teardown path to consult.  Should the check be implemented, or should the
changelog and the comment be corrected so reviewers do not assume a guard
that is not there?

> +
> +static struct nbl_dev_mgt *nbl_dev_setup_dev_mgt(struct nbl_common_info *common)

[ ... ]

> +int nbl_dev_init(struct nbl_adapter *adapter)
> +{

[ ... ]

> +	dev_mgt->disp_ops_tbl = disp_ops_tbl;
> +	dev_mgt->chan_ops_tbl = chan_ops_tbl;
> +	adapter->core.dev_mgt = dev_mgt;
> +	if (common->has_ctrl)
> +		nbl_dev_setup_chan_qinfo(dev_mgt, NBL_CHAN_TYPE_MAILBOX);

[Severity: High]
What serializes these chip-global operations against sibling PFs that are
already bound or still probing?

This call rewrites the shared mailbox QINFO entry of every hardware-active
PF, clearing MSIX_IDX / MSIX_IDX_VALID in nbl_hw_cfg_mailbox_qinfo(), with
no check for siblings that are already live.  On the other side,
nbl_dev_remove_ctrl_dev() -> deinit_module() sets driver_status=false, which
per the comments makes firmware asynchronously wipe per-PF state including
sibling QINFO registers.

The commit message states:

    The device link guarantees the kernel unbinds all non-control PFs before
    the control PF can be removed, preventing broken cross-PF mailbox RPC.

Does the managed link created inside the consumer's own probe give that
guarantee?  In drivers/base/core.c, device_link_init_status() assigns
DL_STATE_DORMANT when the supplier has no driver, device_links_driver_bound()
later promotes DORMANT only to DL_STATE_AVAILABLE, and
device_links_unbind_consumers() force-unbinds only ACTIVE links:

drivers/base/core.c:device_links_unbind_consumers() {
		WRITE_ONCE(link->status, DL_STATE_SUPPLIER_UNBIND);
		if (status == DL_STATE_ACTIVE) { /* only then force-unbind consumer */
}

So a sequence like "unbind func0; bind func1; bind func0; unbind func0"
leaves func1 bound and live while the control PF runs deinit_module().

In the opposite direction, device_link_add() returns a valid link with
status DL_STATE_SUPPLIER_UNBIND when the supplier is already unbinding, and
nbl_probe_chip_deps() only checks for NULL, so a sibling can keep probing
into nbl_dev_init() and enable its mailbox while func0 is inside
nbl_dev_remove().  PCI does not set need_parent_lock, so no common device
lock serializes the two.  Can this leave a sibling PF with its mailbox
routing torn down underneath it?

[ ... ]

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