Re: [PATCH v28 net-next 09/10] net/nebula-matrix: add common/ctrl dev init/remove operation

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 5 · Low: 0

- [Medium] Commit message contradicts the code: it states that common
  device setup does "On the control PF only: fetch VSI/ETH identity via…
- [Medium] nbl_dev_setup_common_dev() calls
  cancel_work_sync(&common_dev->clean_mbx_task) on a work_struct that…
- [Medium] The patch introduces an unguarded chip-global cross-PF
  dependency: nbl_dev_setup_ctrl_dev() -> nbl_dev_setup_chan_qinfo() ->…
- [Medium] nbl_dev_remove_common_dev() (nbl_dev.c:115-125) discards the
  int return value of nbl_dev_remove_chan_queue() ->…
- [Medium] Hardware sequencing bug: the mailbox TX/RX rings are armed in
  hardware before the chip-side mailbox routing entry that identifies…

--

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

This adds the nbl_dev layer with nbl_dev_setup_common_dev() and
nbl_dev_setup_ctrl_dev() plus their teardown counterparts, wired into
nbl_dev_init() / nbl_dev_remove() from nbl_core_init() / nbl_core_remove().
Common dev setup brings up the mailbox channel queue and registers the
mailbox cleanup work; ctrl dev setup calls init_module() and programs the
mailbox QINFO routing map.

A few questions below.

>     Common device setup (nbl_dev_setup_common_dev):
>     - Allocate per-device state (nbl_dev_common)
>     - Set up mailbox channel queue and register cleanup work
>     - Initialize MSI-X service counts (mailbox = 1 vector; actual
>       allocation deferred to nbl_dev_start() in the next patch; this
>       patch only populates service vector counts)
>     - On the control PF only: fetch VSI/ETH identity via local
>       dispatch ops. Non-control PFs defer this to nbl_dev_start(),
>       when the mailbox channel is fully operational and the control
>       PF's responder is running; this keeps every intermediate commit
>       bisect-bootable on multi-PF systems.

[Severity: Medium]
Does the code match this description?  nbl_dev_setup_common_dev() only
allocates nbl_dev_common, sets up the mailbox channel queue, does
INIT_WORK() plus nbl_dev_register_chan_task(), stores common_dev and calls
nbl_dev_init_msix_cnt().  There is no get_vsi_id()/get_eth_id() call in
nbl_dev.c at all, and the in-code comment says the opposite of the
changelog:

	/*
	 * VSI/ETH identity fetch moved to nbl_dev_start().
	 * This avoids cross-PF probe race when manager PF is not ready.
	 */

So common->vsi_id / common->eth_id / common->eth_num stay zero after
nbl_dev_init() on every PF, including the control PF.

Should the changelog say the identity fetch is deferred to nbl_dev_start()
for all PFs rather than "On the control PF only: fetch VSI/ETH identity via
local dispatch ops"?

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

[ ... ]

> +/* ----------  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;
> +
> +	chan_ops->cfg_chan_qinfo_map_table(priv, common->hw_bus, common->devid);
> +}

[ ... ]

> +/* ----------  Dev init process  ---------- */
> +static int nbl_dev_setup_common_dev(struct nbl_adapter *adapter)
> +{
> +	struct nbl_dev_mgt *dev_mgt = adapter->core.dev_mgt;
> +	struct nbl_dev_common *common_dev;
> +	int ret;
> +
> +	common_dev = devm_kzalloc(&adapter->pdev->dev, sizeof(*common_dev),
> +				  GFP_KERNEL);
> +	if (!common_dev)
> +		return -ENOMEM;
> +	common_dev->dev_mgt = dev_mgt;
> +
> +	ret = nbl_dev_setup_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);
> +	if (ret)
> +		goto err_cleanup;

[Severity: Medium]
Is the hardware ordering here safe with respect to the mailbox routing map?
This call arms the mailbox rings:

nbl_dev_setup_chan_queue() -> nbl_chan_setup_queue()
    nbl_hw_config_mailbox_txq() / nbl_hw_config_mailbox_rxq()  (QUEUE_EN=1)
    nbl_chan_update_tail_ptr()                                 (rx doorbell)

but the per-function BUS/DEVID/FUNCTION routing entry is only written later,
and only when common->has_ctrl:

nbl_dev_init()
    nbl_dev_setup_common_dev()          <- queues enabled here
    if (common->has_ctrl)
        nbl_dev_setup_ctrl_dev()
            nbl_dev_setup_chan_qinfo()  <- QINFO_MAP written here
                nbl_chan_cfg_qinfo_map_table() -> nbl_hw_cfg_mailbox_qinfo()

	data |= FIELD_PREP(NBL_MAILBOX_QINFO_MAP_FUNCTION_MASK, function) |
	       FIELD_PREP(NBL_MAILBOX_QINFO_MAP_DEVID_MASK, devid) |
	       FIELD_PREP(NBL_MAILBOX_QINFO_MAP_BUS_MASK, bus);

A non-control PF never programs its own entry, and this patch states firmware
clears those chip-global entries when the control PF sets driver_status=false.

Can a PF then run with mailbox queues enabled while its QINFO_MAP entry is
zero or stale, for example when a non-control PF is bound while function 0 is
not bound, or in the window on control PF re-probe between arming its rings
and writing QINFO_MAP for function 0?  Would mailbox DMA in that window be
tagged with 00:00.0 or a stale bus?

Would it be safer to program a function's QINFO routing entry before enabling
that function's mailbox queue?

> +
> +	INIT_WORK(&common_dev->clean_mbx_task, nbl_dev_clean_mailbox_task);
> +	nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX,
> +				   &common_dev->clean_mbx_task);
> +	/*
> +	 * VSI/ETH identity fetch moved to nbl_dev_start().
> +	 * This avoids cross-PF probe race when manager PF is not ready.
> +	 */
> +	dev_mgt->common_dev = common_dev;
> +	nbl_dev_init_msix_cnt(dev_mgt);
> +
> +	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;
> +}

[Severity: Medium]
Can this warn on an uninitialized work item?  The only jump to err_cleanup is
the nbl_dev_setup_chan_queue() failure, which is before the INIT_WORK(), so
clean_mbx_task is still all zeros from devm_kzalloc() (work->func == NULL, no
lockdep map) when cancel_work_sync() runs:

cancel_work_sync() -> __cancel_work_sync() -> __flush_work(work, true)

kernel/workqueue.c:__flush_work() {
	...
	if (WARN_ON(!work->func))
		return false;
	...
}

That gives a WARNING backtrace (panic with panic_on_warn) instead of a clean
probe failure.

The same label also tears down a queue that was never brought up
(nbl_chan_teardown_queue() then logs "channel not active, skip duplicate
teardown") and unregisters a task that was never registered.

Would moving INIT_WORK() above nbl_dev_setup_chan_queue(), or just returning
ret directly on that path, be preferable?

> +
> +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;
> +
> +	if (!common_dev)
> +		return;
> +	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);
> +}

[Severity: Medium]
Should the return value of nbl_dev_remove_chan_queue() be checked here?  It
propagates -ETIMEDOUT from the drain in nbl_chan_teardown_queue():

nbl_channel.c:nbl_chan_teardown_queue() {
	...
	ret = wait_event_timeout(chan_info->inflight_wait,
				 atomic_read(&chan_info->inflight_tx_cnt) == 0,
				 msecs_to_jiffies(5000));

	if (!ret) {
		dev_warn(chan_mgt->common->dev,
			 "teardown: inflight tx drain timeout\n");
		ret = -ETIMEDOUT;
	...
}

whose comment says callers must not access queue resources while an inflight
sender may still be active.  nbl_dev_remove() then proceeds to
nbl_dev_remove_ctrl_dev() -> deinit_module() -> nbl_hw_deinit_module(), which
documents a premise nobody verifies:

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

On the timeout path that premise does not hold: driver_status=false starts the
asynchronous firmware cleanup while a sender may still be writing descriptors
into the dmam_alloc_coherent() rings, which devres frees right after remove()
returns.

Can this at least log or bail out on the -ETIMEDOUT case rather than
discarding it?

> +
> +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;
> +
> +	nbl_dev_setup_chan_qinfo(dev_mgt, NBL_CHAN_TYPE_MAILBOX);
> +
> +	return 0;
> +}
> +
> +/*
> + * 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.
> + *
> + * NOTE: Firmware clears chip-global qinfo routing entries when control PF
> + * deinit runs.  MUST unbind all sibling non-control PFs on the same chip
> + * BEFORE unbinding the control PF. Unbinding control PF while siblings are
> + * still bound will leave those sibling PFs with broken mailbox RPC,
> + * causing RPC ACK timeouts. This operation sequence is NOT supported.
> + */
> +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: Medium]
Can anything stop the unbind order that this comment says is unsupported?

nbl_dev_setup_ctrl_dev() -> nbl_dev_setup_chan_qinfo() ->
nbl_chan_cfg_qinfo_map_table() programs NBL_MAILBOX_QINFO_MAP_REG_ARR() for
every hardware-active PF, and nbl_dev_remove_ctrl_dev() calls deinit_module()
-> nbl_hw_deinit_module() (driver_status=false) unconditionally.

nbl_remove() runs per PCI function with no cross-function coordination:

nbl_remove()
    pci_set_drvdata(pdev, NULL);
    nbl_core_remove(adapter);
        nbl_dev_remove()
            if (common->has_ctrl)
                nbl_dev_remove_ctrl_dev()   <- chip-global deinit

So a plain

  echo <pf0-bdf> > /sys/bus/pci/drivers/nbl/unbind

silently breaks every still-bound sibling PF: their mailbox RPCs are no longer
routed, so each dispatch RPC ends in an ACK timeout (3s ACK wait, 5s drain),
and their own later teardown RPCs fail too.

Would a refusal or a reference count on the control PF be better than
documenting the hazard, or can the chip-global dependency be avoided?

[ ... ]

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