Re: [PATCH v30 net-next 8/8] net/nebula-matrix: add common dev start/stop operation

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] nbl_dev_rpc_timeout() in drivers/net/ethernet/nebula-
  matrix/nbl/nbl_core/nbl_dev.c (lines 85-90) turns every -ETIMEDOUT…
- [Medium] In nbl_dev_start() (nbl_dev.c:513-516), map_ready is set only
  after cfg_msix_map returns success.
- [Low] The commit message says teardown is best-effort 'with
  pci_clear_master() serving as the final safety net' when the MSI-X…
- [Low] Two problems here.
- [Low] The commit message says clearing NBL_CHAN_IRQ_RDY before masking
  the hardware interrupt is done 'preventing in-flight ACK discard and…

--

net/nebula-matrix: add common dev start/stop operation

This patch adds nbl_dev_start() and nbl_dev_stop(). Start configures the
device MSI-X map, fetches the VSI/ETH ids, allocates MSI-X vectors and sets
up the mailbox interrupt; stop does the matching teardown. Startup RPC
timeouts on non-control PFs become -EPROBE_DEFER, and both functions are
called from nbl_probe()/nbl_remove() through nbl_core_start()/nbl_core_stop().

> Teardown is best-effort: failed MSI-X destroy RPC leaves stale hardware
> entries which are reclaimed by firmware on chip reset, with
> pci_clear_master() serving as the final safety net. The start/stop
> pair is single-shot and non-repeatable, tied strictly to PCI probe/remove
> device lifecycle.

[Severity: Low]
Is the pci_clear_master() safety net claim accurate for non-control PFs?
The comments this patch adds in nbl_dev_start() and nbl_dev_stop() say the
opposite:

 * pci_clear_master() on a non-control PF cannot stop DMA using the
 * management PF's BDF.

nbl_res_intr_cfg_msix_map() programs the PCOMPLETER map with the control
PF's common->hw_bus/devid/function. If destroy_msix_map fails, clearing bus
mastering on the departing PF would not stop those map-table fetches.

Could the commit message be changed to match the code comments?

> 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
> index e094b97acdfb..79c62c169d70 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c

[ ... ]

> @@ -18,6 +29,233 @@ static void nbl_dev_init_msix_cnt(struct nbl_dev_mgt *dev_mgt)

[ ... ]

> +/*
> + * Translate a mailbox RPC timeout on a non-control PF into deferred
> + * probing: the management PF/firmware is not responsive yet and the
> + * driver core should retry once PF0 is ready. Other errors, and all
> + * errors on the control PF itself, pass through unchanged.
> + */
> +static int nbl_dev_rpc_timeout(struct nbl_common_info *common, int ret)
> +{
> +	if (!common->has_ctrl && ret == -ETIMEDOUT)
> +		return -EPROBE_DEFER;
> +	return ret;
> +}

[Severity: Medium]
Can the "management PF/firmware is not responsive yet" case still happen
when these RPCs are sent? nbl_probe() calls nbl_probe_chip_deps() before
nbl_core_init()/nbl_core_start(), and that function already defers until
func 0 is bound:

nbl_probe_chip_deps() {
    ...
	if (READ_ONCE(mgt->dev.links.status) != DL_DEV_DRIVER_BOUND) {
		pci_dev_put(mgt);
		return -EPROBE_DEFER;
	}
    ...
}

DL_DEV_DRIVER_BOUND is set only after PF0's nbl_probe() returns 0. With
this patch, that probe includes PF0's own nbl_core_start() (cfg_msix_map,
request_irq and set_mailbox_irq(true)). The managed device link also stops
PF0 from unbinding while a consumer is probing.

So on a sibling PF, a -ETIMEDOUT from cfg_msix_map, get_vsi_id, get_eth_id
or set_mailbox_irq(true) comes after a 5-6 second polling wait. That looks
like a real failure, not PF0 still starting up.

Will returning -EPROBE_DEFER here actually cause a retry? really_probe()
only reports the deferral with dev_dbg(). Deferred devices are retried from
driver_bound()->driver_deferred_probe_trigger() when some other device
binds, and PF0's bind trigger has already fired by this point.

If nothing else binds later (the last sibling PF, or a module loaded after
boot), could the PF stay unbound with no probe failure printed at the
default log level? If other devices do bind, does each retry pay another
multi-second timeout for every startup RPC?

If this changes, the commit message wording "to trigger deferred probing
when the control PF is not ready" would need updating too.

[ ... ]

> +static int nbl_dev_disable_mailbox_irq(struct nbl_dev_mgt *dev_mgt)
> +{

[ ... ]

> +	/*
> +	 * Disable sequence invariant: update software state first, then mask
> +	 * hardware interrupt. Must not reverse the order.
> +	 *
> +	 * If hardware interrupt is masked before clearing INTERRUPT_READY,
> +	 * the hardware may still transmit outstanding ACK packets for in-flight
> +	 * messages. Subsequent switch to polling mode discards pending ACK
> +	 * processing, triggering "Channel waiting ack failed" and "Skip ack
> +	 * with invalid status" errors.
> +	 *
> +	 * By entering polling mode first, any late hardware interrupts are
> +	 * ignored without pending ACK expectations, then hardware interrupt
> +	 * can be safely disabled.
> +	 *

[ ... ]

> +	 */
> +	chan_ops->set_queue_state(dev_mgt->chan_ops_tbl->priv,
> +				  NBL_CHAN_IRQ_RDY,
> +				  NBL_CHAN_TYPE_MAILBOX, false);
> +
> +	return disp_ops->set_mailbox_irq(dev_mgt->disp_ops_tbl->priv,
> +					 lvec, false);
> +}

[Severity: Low]
Does clearing NBL_CHAN_IRQ_RDY first actually keep in-flight ACKs? When
IRQ_RDY is cleared, nbl_chan_set_queue_state() wakes every waiter:

nbl_chan_set_queue_state() {
    ...
	if (!set && state == NBL_CHAN_IRQ_RDY) {
		for (i = 0; i < chan_info->num_txq_entries; i++)
			wake_up_all(&chan_info->wait[i].wait_queue);
	}
}

An interrupt-mode waiter in nbl_chan_send_msg() then gives up:

			if (!test_bit(NBL_CHAN_IRQ_RDY, chan_info->state)) {
				ret = -EIO;
				goto out_clear_wait_slot;
			}

The slot goes back to IDLE. When the peer's ACK arrives later,
nbl_chan_recv_ack_msg() sees w_status != NBL_MBX_STATUS_WAITING, logs
"Skip ack invalid status" and drops it. The comment says this ordering
prevents exactly that; the only difference is that the sender gets -EIO
instead of -ETIMEDOUT.

In this patch every ACK-requesting sender runs from probe/remove, and the
clean task only sends replies without ACKs, so nothing hits this today.

Should the comment and the commit message ("preventing in-flight ACK
discard and "Channel waiting ack failed" errors", "to preserve ACK
integrity") be corrected before later patches rely on this invariant?
Only draining in-flight waiters before clearing IRQ_RDY would provide that
guarantee.

[ ... ]

> @@ -251,3 +497,129 @@ void nbl_dev_remove(struct nbl_adapter *adapter)

[ ... ]

> +/* ----------  Dev start process  ---------- */
> +int nbl_dev_start(struct nbl_adapter *adapter)
> +{

[ ... ]

> +	ret = nbl_dev_rpc_timeout(common, nbl_dev_cfg_msix_map(dev_mgt));
> +	if (ret)
> +		goto err_destroy_map;
> +	map_ready = true;

[ ... ]

> +	ret = nbl_dev_enable_mailbox_irq(dev_mgt);
> +	if (ret)
> +		goto err_disable_irq;
> +
> +	return 0;
> +
> +err_disable_irq:
> +	cleanup_ret = nbl_dev_disable_mailbox_irq(dev_mgt);
> +	if (cleanup_ret)
> +		dev_err(dev_mgt->common->dev,
> +			"rollback: disable mailbox IRQ failed: %d\n",
> +			cleanup_ret);
> +	nbl_dev_free_mailbox_irq(dev_mgt);

[Severity: Low]
Is the set_mailbox_irq(false) RPC needed on this rollback path? If
nbl_dev_enable_mailbox_irq() failed, NBL_CHAN_IRQ_RDY was never set, yet
nbl_dev_disable_mailbox_irq() still sends a polling-mode RPC that requests
an ACK.

Control then falls through to err_destroy_map. On the control PF, the
destroy path already turns off mailbox routing:

__nbl_res_intr_prepare_destroy_msix_map() {
    ...
	/* Step 0: disable mailbox IRQ routing before tearing down map */
	ret = __nbl_res_intr_set_mailbox_irq(res_mgt, func, 0, false);
    ...
}

With an unresponsive peer, this adds another polling timeout of about 5-6
seconds, and every deferred retry repeats it. That works against the stated
goal of removing unnecessary polling timeouts during probe rollback.

Separately, some commit message wording seems to describe changes from an
earlier revision:

  "Skip redundant MSI-X map destroy RPC on unconfigured state during probe
  rollback to eliminate unnecessary polling timeouts. Adjust interrupt
  teardown ordering to ..."

This patch introduces nbl_dev_start()/nbl_dev_stop(), so should these
version notes go below the --- line instead?

> +err_destroy_map:
> +	/*
> +	 * Destroy the device-side MSI-X map only when it was configured.
> +	 * On a cfg RPC failure there is no known-good remote map; when
> +	 * the failure is a timeout against an unready/unresponsive
> +	 * management PF, issuing the destroy RPC would just burn another
> +	 * multi-second ACK timeout. Partial remote state is best-effort
> +	 * and reclaimed by firmware on chip reset.

[ ... ]

> +	 */
> +	if (map_ready) {
> +		cleanup_ret = nbl_dev_destroy_msix_map(dev_mgt);

[Severity: Medium]
If cfg_msix_map times out, can skipping destroy here leave control-PF
resources held for a function that has no driver? A -ETIMEDOUT from the
polling send only means the ACK did not arrive in time.
nbl_chan_send_msg() explicitly handles ACKs that arrive after the sender
has timed out, and the request may still be in the control PF's RX ring.

On the control PF, nbl_disp_chan_cfg_msix_map_resp()->
nbl_res_intr_cfg_msix_map() finishes all of its work before it sends the
ACK:

- kcalloc() for the interrupts array
- dma_alloc_coherent() for the map table
- set_bit() in intr_net_bmap/intr_other_bmap
- setting the PCOMPLETER map to VALID

and finally:

	intr_mgt->func_intr_res[func_id].state = NBL_INTR_FUNC_CONFIGURED;

With map_ready false, the sibling returns -EPROBE_DEFER without sending
destroy. The resources are then freed only when the same PF later
re-probes successfully (the had_config path) or when the control PF
unbinds (nbl_res_remove_leonis()->nbl_intr_mgt_stop()). A re-probe after
deferral is not guaranteed.

The comment "Partial remote state is best-effort and reclaimed by firmware
on chip reset" also looks inaccurate, as does the similar comment in
nbl_dev_stop(). This is host memory and bitmap state owned by the control
PF driver, and nothing requests a chip reset.

Would it be safer to always send destroy after a cfg timeout?
__nbl_res_intr_prepare_destroy_msix_map() returns 0 when the function is
not NBL_INTR_FUNC_CONFIGURED, so it looks harmless in the unconfigured
case.

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