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

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/nebula-matrix: add common/ctrl dev init/remove operation

This adds nbl_dev.c with nbl_dev_setup_common_dev() and
nbl_dev_setup_ctrl_dev() plus their teardown paths, hooks them into
nbl_core_init()/nbl_core_remove(), adds a per-adapter workqueue helper, and
extends nbl_chan_send_msg() with a polling ACK path used when
NBL_CHAN_INTERRUPT_READY is not set.

A few questions on the changelog and on the new code below.

[Severity: Low]
Is this paragraph describing the state of the tree at this commit?

Mailbox message processing is implemented via work_struct. This task
registration framework is reused for interrupt-driven receive now, and
can be shared with polling receive mode introduced in follow-up patches.

At this commit NBL_CHAN_INTERRUPT_READY is only ever tested, in
nbl_chan_send_msg(); nothing sets it, and there is no request_irq() or
pci_alloc_irq_vectors() anywhere under drivers/net/ethernet/nebula-matrix/.
The bit is first set (and the mailbox IRQ first requested) by the later
"net/nebula-matrix: add common dev start/stop operation" patch.

So the relationship looks inverted: polling is what is live now, and the
wait_event_timeout() branch of nbl_chan_send_msg() is unreachable here.
Every synchronous request issued by this patch (get_vsi_id/get_eth_id from
nbl_dev_setup_common_dev()) goes through the new polling loop, which can
run NBL_CHAN_TX_WAIT_ACK_TIMES (5000) iterations of roughly 1 ms, i.e. up
to about 5 seconds per message during probe. Would it be worth saying that
explicitly?

Also on this sentence:

requests rely on polling to wait for firmware ACK, avoiding silent
success and uninitialized variable reads when NBL_CHAN_INTERRUPT_READY
is not yet set.

Before this patch, ret still held 0 from nbl_chan_kick_tx_ring() when the
if/else fell through, so the old behaviour was returning success with an
unpopulated response buffer rather than reading an uninitialized variable.
Could the wording be adjusted?

[Severity: Low]
Should the changelog mention that nbl_chan_clean_queue() becomes a
self-rearming worker here? The changelog only says "Mailbox message
processing is implemented via work_struct" and describes the polling ACK
fallback, but this hunk also changes the semantics of an existing function
in nbl_channel.c: it now re-queues its own work item on budget exhaustion
and returns early on shutdown.

Small related point: more_work is set purely on budget exhaustion, so a
ring holding exactly 64 used descriptors re-arms the work once with nothing
left to clean. Is that intended?

+	/* Prevent queue_work after teardown clears clean_task */
+	if (READ_ONCE(chan_info->shutdown))
+		return;

[Severity: Low]
Does this comment hold? The guard is two independent unordered loads
followed by queue_work(), with nothing serializing them against
nbl_chan_teardown_queue().

Looking at nbl_chan_teardown_queue():

	while (atomic_read(&chan_info->inflight_tx_cnt) != 0) {
		if (time_after(jiffies, wait_start + wait_timeout)) {
			dev_warn(chan_mgt->common->dev,
				 "teardown: wait inflight_tx_cnt timeout, force continue\n");
			ret = -ETIMEDOUT;
			break;
		}
		...
	}

	/* Step3: All send paths drained, safely cancel cleanup work */
	if (chan_info->clean_task) {
		cancel_work_sync(chan_info->clean_task);
		chan_info->clean_task = NULL;
	}

The drain is bounded at 5 s and teardown continues regardless, so a context
that has already observed shutdown == false and a non-NULL clean_task can
still call queue_work() after teardown finished. The flag check narrows the
window but does not close it, and the same pattern is repeated in the new
polling loop of nbl_chan_send_msg(). Could the comment be reworded, or the
publish/clear of clean_task be made to actually interlock with the readers?

+	if (common->wq && more_work) {
+		task = READ_ONCE(chan_info->clean_task);
+		if (task)
+			queue_work(common->wq, task);
+	}
 }
 
 static void nbl_chan_clean_queue_subtask(struct nbl_channel_mgt *chan_mgt,

[ ... ]

[Severity: Low]
Is it intentional that this success path leaves wait_head->ack_data and
wait_head->ack_data_len set while publishing the slot as
NBL_MBX_STATUS_IDLE?

Every other exit of nbl_chan_send_msg() clears both, for example the
interrupt-wait success path just above:

	WRITE_ONCE(wait_head->acked, 0);
	WRITE_ONCE(wait_head->status, NBL_MBX_STATUS_IDLE);
	WRITE_ONCE(wait_head->ack_data, NULL);
	WRITE_ONCE(wait_head->ack_data_len, 0);

and so do the new polling shutdown and polling timeout blocks. Here the
slot is released as IDLE with ack_data still pointing at the caller's stack
response buffer (for instance the local result struct in
nbl_disp_chan_get_vsi_id_req()) and a non-zero ack_data_len.

Today nothing dereferences it, because nbl_chan_recv_ack_msg() only writes
through ack_data when status == NBL_MBX_STATUS_WAITING, and the send path
re-assigns both fields before re-arming a slot as WAITING. The visible
effect is a stale length in the "Skip ack with invalid status" diagnostic.
Should this path clear them anyway to keep the slot-release contract
uniform?

+			spin_unlock_irq(&wait_head->status_lock);
+
+			usleep_range(NBL_CHAN_TX_WAIT_ACK_US_MIN,
+				     NBL_CHAN_TX_WAIT_ACK_US_MAX);
+			cond_resched();
+		}
+
+		/* Polling loop exhausted, reclaim slot */
+		spin_lock_irq(&wait_head->status_lock);
+		WRITE_ONCE(wait_head->acked, 0);
+		WRITE_ONCE(wait_head->ack_data, NULL);
+		WRITE_ONCE(wait_head->ack_data_len, 0);
+		WRITE_ONCE(wait_head->status, NBL_MBX_STATUS_TIMEOUT);
+		spin_unlock_irq(&wait_head->status_lock);

[Severity: Low]
Can this block throw away an ACK that arrived during the last sleep?

The final iteration tests acked before its usleep_range(1000, 1200), and
this cleanup then clears acked and forces NBL_MBX_STATUS_TIMEOUT without
re-testing it. An ACK delivered in that last window is processed by the
clean work item, which reaches nbl_chan_recv_ack_msg():

	ack_done:
		WRITE_ONCE(wait_head->acked, 1);
		spin_unlock_irq(&wait_head->status_lock);

That only sets acked/ack_err/ack_data_len and leaves status at
NBL_MBX_STATUS_WAITING, so the cleanup here cannot distinguish "never
acked" from "just acked". The caller gets -ETIMEDOUT even though the
response was already copied into its buffer, and in
nbl_dev_setup_common_dev() that aborts nbl_dev_init() and the whole probe.
The interrupt path re-tests the status before overwriting it; should this
one do the same?

[Severity: Medium]
Should this set common->wq = NULL after destroy_workqueue()?

Both new queue_work() sites use the pointer itself as the liveness test:

nbl_chan_clean_queue():
	if (common->wq && more_work) {
		task = READ_ONCE(chan_info->clean_task);
		if (task)
			queue_work(common->wq, task);
	}

nbl_chan_send_msg() polling loop:
	task = READ_ONCE(chan_info->clean_task);
	if (common->wq && task && !READ_ONCE(chan_info->shutdown))
		queue_work(common->wq, task);

Can this end up as a use-after-free of the workqueue? nbl_dev_remove()
runs nbl_dev_remove_common_dev() (which reaches nbl_chan_teardown_queue(),
whose inflight drain is explicitly best-effort: after 5 s it prints
"teardown: wait inflight_tx_cnt timeout, force continue", sets -ETIMEDOUT
and proceeds to cancel_work_sync() plus clean_task = NULL), then calls
nbl_common_destroy_wq(common) unconditionally, ignoring that result.

Meanwhile the polling loop itself can run 5000 iterations of 1.0-1.2 ms,
which is longer than the 5 s drain bound. A sender preempted between its
shutdown check and queue_work(common->wq, task) would then queue onto a
freed workqueue and re-arm a work item after cancel_work_sync(). Would
clearing common->wq under the same ordering that clears clean_task, plus a
NULL guard here, close that?

+
+int nbl_common_create_wq(struct nbl_common_info *common)
+{
+	char wq_name[32];
+
+	snprintf(wq_name, sizeof(wq_name), "nbl_wq_%s", pci_name(common->pdev));
+	common->wq = alloc_workqueue("%s", WQ_UNBOUND, 0, wq_name);
+	if (!common->wq) {
+		pr_err("Failed to create workqueue nbl_wq\n");
+		goto alloc_wq_failed;
+	}
+
+	return 0;
+alloc_wq_failed:
+	return -ENOMEM;
+}
+

[ ... ]

[ ... ]

+/* ----------  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);
+}

[Severity: Low]
This isn't a bug introduced here, but this patch adds the only caller of
cfg_chan_qinfo_map_table(), which makes an existing comment checkable and
wrong. In nbl_res_ctrl_dev_sriov_info_init() the hw_bus assignment carries:

	/*
	 * common->hw_bus supplies bus number for channel mailbox QINFO mapping.
	 * Execution order guarantee: this assignment runs before
	 * cfg_chan_qinfo_map_table() in nbl_dev_start(), only executed
	 * on control PF path.
	 */

There is no nbl_dev_start() at this commit, and even after
"net/nebula-matrix: add common dev start/stop operation" adds one, it does
not call cfg_chan_qinfo_map_table(); the call lives in
nbl_dev_setup_chan_qinfo() from nbl_dev_setup_ctrl_dev(). The ordering
claim itself holds, only the named consumer is wrong. Could the comment be
updated to name nbl_dev_setup_ctrl_dev()?

[Severity: Medium]
Can this write clobber the mailbox MSI-X fields of other PFs?

nbl_chan_cfg_qinfo_map_table() loops over every host PF:

	hw_ops->get_host_pf_mask(p, &pf_mask);
	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);

and nbl_hw_cfg_mailbox_qinfo() is a full-word write built from zero:

	u32 data = 0;

	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);
	nbl_hw_wr_regs_lock(hw_mgt, NBL_MAILBOX_QINFO_MAP_REG_ARR(func_id),
			    &data, sizeof(data));

so NBL_MAILBOX_QINFO_MAP_MSIX_IDX and MSIX_IDX_VALID are zeroed. Those are
exactly the fields nbl_hw_set_mailbox_irq() programs in the same register,
and that function already warns about this:

	/*
	 * Note: This RMW is currently safe because the two callers are strictly
	 * sequential: nbl_hw_cfg_mailbox_qinfo() runs at init, nbl_hw_set_mailbox_irq()
	 * runs at start. Future reset/hot-add/VF-reinit paths must preserve this
	 * ordering; ...
	 */

This patch puts the clobbering write on the ctrl-PF probe path, and it
rewrites entries belonging to other functions. If PF0 is unbound and
rebound while other PF instances stay bound with their mailbox MSI-X
already programmed on their behalf by the ctrl PF, do those functions lose
MSIX_IDX_VALID in hardware while their drivers still have
NBL_CHAN_INTERRUPT_READY set and keep waiting on interrupts? Would making
this write an RMW under reg_lock, or limiting it to func_ids owned by this
PF, be preferable?

+
+static int nbl_dev_setup_chan_queue(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;
+	int ret = 0;
+
+	if (chan_ops->check_queue_exist(priv, chan_type))
+		ret = chan_ops->setup_queue(priv, chan_type);
+
+	return ret;
+}
+
+static int nbl_dev_remove_chan_queue(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;
+	int ret = 0;
+
+	if (chan_ops->check_queue_exist(priv, chan_type))
+		ret = chan_ops->teardown_queue(priv, chan_type);
+
+	return ret;
+}

[ ... ]

+/* ----------  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)
+		return ret;
+
+	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);

[Severity: Low]
Should the publication of clean_task here be annotated? The store side is a
plain assignment in nbl_chan_register_chan_task():

	chan_info->clean_task = task;

and the clearing side in nbl_chan_teardown_queue() is likewise a plain
chan_info->clean_task = NULL, while all consumers added by this patch read
it with READ_ONCE() and immediately queue_work() the result. The
asymmetry is an unmarked-write/marked-read race that KCSAN would flag,
and in principle a cross-CPU reader could observe the pointer before the
INIT_WORK() stores to work->func. I could not find such a remote reader at
this commit or at the end of the series, so this looks latent rather than
live. Would WRITE_ONCE() or smp_store_release() here match the readers?

+	/*
+	 * Even if has_ctrl=false (no dedicated control PF channel), we fetch
+	 * VSI/ETH info via regular mailbox message  instead of
+	 * dedicated control command.
+	 */
+	ret = disp_ops->get_vsi_id(priv, NBL_VSI_DATA, &common->vsi_id);
+	if (ret)
+		goto err_cleanup;
+	ret = disp_ops->get_eth_id(priv, common->vsi_id, &common->eth_num,
+			     &common->eth_id, &common->logic_eth_id);
+	if (ret)
+		goto err_cleanup;

[ ... ]

+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;
+	nbl_dev_remove_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);
+	cancel_work_sync(&common_dev->clean_mbx_task);
+	nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX, NULL);
+}

[Severity: Low]
nbl_chan_teardown_queue() and nbl_chan_setup_queue() came from the earlier
"net/nebula-matrix: add channel layer" patch, so this isn't introduced
here, but this patch adds the first callers, so it becomes reachable.

Teardown puts both mailbox queues into hardware reset via
stop_mailbox_rxq()/stop_mailbox_txq() but leaves chan_info->init_done
true, chan_info->shutdown true, and all ring indices (txq->next_to_use,
txq->tail_ptr, rxq->next_to_clean/next_to_use/tail_ptr) at stale values.
Since nbl_chan_setup_queue() starts with:

	if (READ_ONCE(chan_info->init_done))
		return 0;

would a later re-init silently report success without re-programming the
queue base/size/QUEUE_EN registers, re-arming RX descriptors, or clearing
shutdown? Today the only setup caller is nbl_dev_setup_common_dev() and
chan_info is freshly devm_kzalloc()'d per probe, so nothing re-inits within
one adapter lifetime, but the reset/recovery paths the changelog mentions
would hit it. Should teardown clear init_done/shutdown and reset the ring
indices?

Related ordering question in the same function: nbl_chan_stop_queue() runs
before the inflight-sender drain, so a sender that already passed its
shutdown check can still publish an AVAIL descriptor, bump txq->tail_ptr
and ring the doorbell of an already-reset queue. At the end of the series
the mailbox worker sends ACKs via nbl_chan_send_ack() concurrently with
removal, so this is reachable; the effect looks like a dropped ACK plus a
tail counter permanently ahead of the zeroed hardware tail. Would draining
before stopping the queues be safer?

+
+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;
+}

[ ... ]

+int nbl_dev_init(struct nbl_adapter *adapter)
+{

[ ... ]

+	/*
+	 * Chip hardware initialization is completed by firmware at power-up.
+	 * Only driver functional table/register config follows here, safe to
+	 * access hardware registers before ctrl dev setup.
+	 */
+	ret = nbl_dev_setup_common_dev(adapter);
+	if (ret)
+		goto setup_err;
+
+	if (common->has_ctrl) {
+		ret = nbl_dev_setup_ctrl_dev(adapter);
+		if (ret)
+			goto setup_ctrl_dev_fail;
+	}

[Severity: Medium]
Does the firmware really cover everything this ordering depends on?

nbl_dev_setup_common_dev() reaches nbl_chan_setup_queue(), which enables
mailbox DMA before nbl_dev_setup_ctrl_dev() has run:

	nbl_chan_config_queue(chan_mgt, chan_info, true); /* tx */
	nbl_chan_config_queue(chan_mgt, chan_info, false); /* rx */
	nbl_chan_update_tail_ptr(hw_ops, chan_mgt->hw_ops_tbl->priv,
				 rxq->tail_ptr, NBL_MB_RX_QID);

Only afterwards does nbl_dev_setup_ctrl_dev() call init_module(), which
goes to nbl_hw_init_module() -> nbl_intf_init() -> nbl_host_padpt_init()
writing NBL_HOST_PADPT_HOST_CFG_FC_CPLH_UP and the other flow-control
credit registers, plus nbl_hw_set_driver_status(true); and then
nbl_dev_setup_chan_qinfo() programs the per-function routing map
NBL_MAILBOX_QINFO_MAP_REG_ARR(func_id). Those are driver writes, not
firmware ones, so the changelog statement

Chip core hardware initialization is fully handled by firmware during
power-on; the driver only configures functional table entries and
registers after hardware is ready.

does not seem to cover them. Per the driver's own comment in
nbl_hw_get_board_info(), these registers read zero after cold boot, FLR or
bus reset, so between queue enable and the qinfo write the mailbox block is
armed with an all-zero (00:00.0) routing entry.

For a PF with has_ctrl == false, nbl_dev_setup_ctrl_dev() is skipped
entirely, yet nbl_dev_setup_common_dev() deliberately issues real mailbox
transactions (get_vsi_id/get_eth_id resolve to the request variants). Does
that leave such a PF depending on a separate ctrl PF instance having
already programmed its qinfo entry, with nothing in the code enforcing or
checking it? If the ctrl PF is unbound or assigned to vfio-pci, the new
polling fallback turns this into a silent multi-second stall before
-ETIMEDOUT. Would it be safer to program the credits, driver status and
qinfo map before enabling the mailbox queues?

+
+	return 0;
+setup_ctrl_dev_fail:
+	nbl_dev_remove_common_dev(adapter);
+setup_err:
+	nbl_common_destroy_wq(common);
+	return ret;
+}

[ ... ]