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

[Jakub Kicinski <kuba@kernel.org>]

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?

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> index 9e0f9b6c4c3a..3b7f3876a075 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> @@ -636,10 +636,13 @@ static void nbl_chan_advance_rx_ring(struct nbl_channel_mgt *chan_mgt,
>  static void nbl_chan_clean_queue(struct nbl_channel_mgt *chan_mgt,
>  				 struct nbl_chan_info *chan_info)
>  {
> +	struct nbl_common_info *common = chan_mgt->common;
>  	struct nbl_chan_ring *rxq = &chan_info->rxq;
>  	struct device *dev = chan_mgt->common->dev;
>  	struct nbl_chan_rx_desc *rx_desc;
>  	struct nbl_chan_buf *rx_buf;
> +	struct work_struct *task;
> +	bool more_work = false;
>  	u16 next_to_clean;
>  	u32 budget = 64;
>  	u16 flags;
> @@ -666,11 +669,22 @@ static void nbl_chan_clean_queue(struct nbl_channel_mgt *chan_mgt,
>  			next_to_clean = 0;
>  		rx_desc = NBL_CHAN_RX_RING_TO_DESC(rxq, next_to_clean);
>  		rx_buf = NBL_CHAN_RX_RING_TO_BUF(rxq, next_to_clean);
> -		if (--budget == 0)
> +		if (--budget == 0) {
> +			more_work = true;
>  			break;
> +		}
>  		cond_resched();
>  	}
>  	rxq->next_to_clean = next_to_clean;
> +

[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,

[ ... ]

> @@ -869,6 +885,60 @@ static int nbl_chan_send_msg(struct nbl_channel_mgt *chan_mgt,
>  		WRITE_ONCE(wait_head->ack_data_len, 0);
>  		spin_unlock_irq(&wait_head->status_lock);
>  		goto inflight_dec_out;
> +	} else {
> +		/* Polling wait path for synchronous ACK */
> +		while (i--) {
> +			if (READ_ONCE(chan_info->shutdown)) {
> +				ret = -ESHUTDOWN;
> +				/*
> +				 * Shutdown happens during polling wait,
> +				 * 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);
> +				goto inflight_dec_out;
> +			}
> +
> +			task = READ_ONCE(chan_info->clean_task);
> +			if (common->wq && task &&
> +			    !READ_ONCE(chan_info->shutdown))
> +				queue_work(common->wq, task);
> +
> +			spin_lock_irq(&wait_head->status_lock);
> +			if (READ_ONCE(wait_head->acked)) {
> +				chan_send->ack_len = wait_head->ack_data_len;
> +				ret = wait_head->ack_err;
> +				WRITE_ONCE(wait_head->acked, 0);
> +				WRITE_ONCE(wait_head->status,
> +					   NBL_MBX_STATUS_IDLE);
> +				spin_unlock_irq(&wait_head->status_lock);
> +				goto inflight_dec_out;
> +			}

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

> +
> +		dev_err_ratelimited(dev,
> +				    "Channel polling ack failed, message type: %d msg id: %u\n",
> +				    chan_send->msg_type, msgid);
> +		ret = -ETIMEDOUT;
> +		goto inflight_dec_out;
>  	}
>  
>  inflight_dec_out:
> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.h
> index 92db39e1b05e..d59e660be9c7 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.h
> @@ -24,6 +24,9 @@
>  #define NBL_CHAN_TX_WAIT_US			100
>  #define NBL_CHAN_TX_WAIT_US_MAX			120
>  #define NBL_CHAN_TX_WAIT_TIMES			100
> +#define NBL_CHAN_TX_WAIT_ACK_US_MIN		1000
> +#define NBL_CHAN_TX_WAIT_ACK_US_MAX		1200
> +#define NBL_CHAN_TX_WAIT_ACK_TIMES		5000
>  #define NBL_CHAN_QUEUE_LEN			256
>  #define NBL_CHAN_BUF_LEN			4096
>  #define NBL_CHAN_TX_DESC_EMBEDDED_DATA_LEN	16
> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_common/nbl_common.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_common/nbl_common.c
> index 149681dd216d..f10ef78ee7f6 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_common/nbl_common.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_common/nbl_common.c
> @@ -7,6 +7,27 @@
>  #include <linux/jhash.h>
>  #include "nbl_common.h"
>  
> +void nbl_common_destroy_wq(struct nbl_common_info *common)
> +{
> +	destroy_workqueue(common->wq);
> +}

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

[ ... ]

> 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..bdb4d6a45fbc
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c

[ ... ]

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

[ ... ]