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

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

[ ... ]

+/* ----------  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