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

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

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] On the control PF, nbl_dev_init() runs its steps in an order
  that conflicts with the stale-status recovery in nbl_hw_init_module().
- [Low] On the control PF, nbl_dev_init() writes
  NBL_MAILBOX_QINFO_MAP_REG_ARR entries for every enabled PF (via…
- [Low] The error-path comment in nbl_dev_setup_common_dev() says no
  unwinding is needed after a failed setup_queue() because the partially…
- [Low] nbl_dev_remove_common_dev() calls
  nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX, NULL) right…

--

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

This adds nbl_dev.c with nbl_dev_init() and nbl_dev_remove().  On every
PF they set up the mailbox channel queue and its clean work.  On the
control PF they also program the mailbox QINFO routing and run
init_module.  The new entry points are called from nbl_core_init() and
nbl_core_remove().

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

[ ... ]

> +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) {
> +		/*
> +		 * Nothing to unwind: chan_info->active is only set on a
> +		 * successful setup_queue(), so a failed one left no
> +		 * sender, no registered work item and no mailbox
> +		 * traffic.  Calling teardown_queue() here would only
> +		 * take its "channel not active" early exit and emit a
> +		 * misleading duplicate-teardown warning.  The partially
> +		 * allocated DMA rings are intentionally kept until devres
> +		 * releases them at detach (chan_info->dma_allocated
> +		 * blocks a re-setup).
> +		 */
> +		return ret;
> +	}

[Severity: Low]
Is the "chan_info->dma_allocated blocks a re-setup" part of this comment
accurate?  nbl_chan_setup_queue() clears the flag again when
nbl_chan_init_queue() fails:

nbl_channel/nbl_channel.c:nbl_chan_setup_queue() {
    ...
	chan_info->dma_allocated = true;
	err = nbl_chan_init_queue(common, chan_info);
	if (err) {
		chan_info->dma_allocated = false;
		return err;
	}
    ...
}

The TX and RX init helpers use dmam_alloc_coherent() and devm_kcalloc().
They free nothing on a partial failure, for example when the TX rings
are allocated and the RX descriptor allocation then fails.

After such a failure, active and dma_allocated are both false.  A second
setup_queue() would get past both checks and allocate a second set of
coherent rings, and the first set would be orphaned until detach.

The channel code has its own comment just above that reset ("a retry
must be rejected instead of allocating a second set and orphaning the
first one"), and the reset contradicts it as well.

Nothing retries today, so for now the comment just doesn't match the
code.

> +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: Low]
Does the nbl_dev_register_chan_task(..., NULL) call here have any
effect?

common_dev is only set after setup_queue() succeeds, so the channel is
always active by the time this runs.  nbl_chan_teardown_queue() then
sets shutdn and clears clean_task under state_lock.  After that,
nbl_chan_register_chan_task() ignores the write:

nbl_channel/nbl_channel.c:nbl_chan_register_chan_task() {
    ...
	mutex_lock(&chan_info->state_lock);
	if (!READ_ONCE(chan_info->shutdn))
		WRITE_ONCE(chan_info->clean_task, task);
	mutex_unlock(&chan_info->state_lock);
}

teardown_queue() does the real unregister and drain: it clears
clean_task and calls cancel_work_sync() on it.  The cancel_work_sync()
above runs before teardown, while the sender poll paths can still
re-queue the work.

This has no functional effect today.  The problem is that this sequence
reads as if the dev layer does the unregister and drain, when the
channel layer actually does it.  Could the NULL registration be dropped,
or a comment added saying which layer is responsible?

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

[ ... ]

> +	adapter->core.dev_mgt = dev_mgt;
> +	if (common->has_ctrl)
> +		nbl_dev_setup_chan_qinfo(dev_mgt, NBL_CHAN_TYPE_MAILBOX);
> +	/*
> +	 * 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: High]
Can this ordering make firmware wipe the mailbox state that this probe
has just programmed?

On the control PF the sequence is:

nbl_dev_init()
    nbl_dev_setup_chan_qinfo()      /* QINFO routing for all PFs */
    nbl_dev_setup_common_dev()
        nbl_chan_setup_queue()      /* TX/RX queue config, active=true */
    nbl_dev_setup_ctrl_dev()
        init_module -> nbl_hw_init_module()

If the active bit in NBL_DRIVER_STATUS_REG is still set,
nbl_hw_init_module() forces it from 1 to 0:

nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c:nbl_hw_init_module() {
    ...
	status = nbl_hw_rd32(hw_mgt, NBL_DRIVER_STATUS_REG);
	if (status & BIT(NBL_DRIVER_STATUS_BIT)) {
		dev_warn(hw_mgt->common->dev,
			 "driver_status already set at init, cleanup forced\n");
		nbl_hw_set_driver_status(hw_mgt, false);
		nbl_flush_writes(hw_mgt);
		usleep_range(NBL_FW_CLEANUP_SYNC_MIN_US,
			     NBL_FW_CLEANUP_SYNC_MAX_US);
    ...
}

The bit can still be set after kexec or kdump, because nbl_pci_driver
has no .shutdown callback.  It can also be set after an unload that
never reached deinit_module().

The commit message says firmware asynchronously reclaims "mailbox QINFO
routing, qinfo registers and the like" when the bit goes from 1 to 0.
Here that happens after the new routing and the live queue registers are
in place.  Nothing later in init, or in the start path, programs them
again.

Wouldn't that leave the control PF mailbox dead, so that sibling-PF RPCs
in nbl_dev_start() time out?  Firmware could also reclaim the routing
while a mailbox transfer is in flight, which is the hazard the teardown
ordering is meant to avoid.

Should the stale-status cleanup run before nbl_dev_setup_chan_qinfo()
and setup_queue()?

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

[Severity: Low]
Do these error paths leave the NBL_MAILBOX_QINFO_MAP_REG_ARR entries
written by nbl_dev_setup_chan_qinfo() in hardware?

The comment above nbl_dev_remove_ctrl_dev() says the map "is not
explicitly cleared; firmware handles it on driver_status change".  But
neither label clears the entries or calls deinit_module().

This applies in two cases:

- nbl_dev_setup_common_dev() fails.
- nbl_hw_init_module() fails, either in the eth_speed/eth_num checks or
  in nbl_dp_init().  Both return before
  nbl_hw_set_driver_status(true).

In both cases driver_status never goes from 1 to 0, so the firmware
reclaim never runs.  The routing entries stay until a later successful
probe overwrites them or the chip is reset.

The practical impact looks small, since sibling PFs can't bind without
the control PF.  The hardware state still no longer matches what the
driver thinks it is.

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