Re: [PATCH v31 net-next 1/8] net/nebula-matrix: add channel layer

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 5 · 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] Lock-order inversion (ABBA) between nbl_probe_chip_deps() and
  the PCI core's bus/slot locking.
- [Medium] nbl_probe_chip_deps() in drivers/net/ethernet/nebula-
  matrix/nbl/nbl_main.c changes how every non-function-0 PF probes, but…
- [Medium] nbl_chan_setup_queue() sets chan_info->dma_allocated = true
  before allocating, and the comment says this is so that "a retry must…
- [Medium] nbl_probe_chip_deps() can self-deadlock when the probing
  device is an SR-IOV VF whose devfn has function number 0.
- [Medium] In nbl_probe_chip_deps(), the PF0 checks and the device-link
  creation are not atomic, and the second check does not close the race.
- [Medium] nbl_hw_stop_mailbox_rxq() and nbl_hw_stop_mailbox_txq() write
  the whole 4-dword QINFO_CFG entry through nbl_hw_write_mbx_regs(), one…
- [Low] A comment in nbl_chan_send_msg() says `active` "is published last
  in setup and cleared last in teardown, so a false value here means the…
- [Low] nbl_chan_teardown_queue() waits for inflight_tx_cnt to reach 0
  and then does one mutex_lock()/mutex_unlock() of state_lock.
- [Low] (Baseline: not a defect.) In the external-buffer branch of
  nbl_chan_update_txqueue(), buf_addr and buf_len are programmed even…

--

net/nebula-matrix: add channel layer

This adds a mailbox-based channel layer that carries control messages
between PF0 and the other PFs. It has xarray handler lookup, ACK-based
sends, coherent DMA rings and a quiescing teardown. It also hooks
channel init/cleanup into probe/remove, and adds a device link from
each PF other than function 0 to function 0.

> 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
> new file mode 100644
> index 0000000000000..ac3115a3ab55f
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> @@ -0,0 +1,1486 @@

[ ... ]

> +static int nbl_chan_teardown_queue(struct nbl_channel_mgt *chan_mgt,
> +				   u8 chan_type)
> +{

[ ... ]

> +	/*
> +	 * Join the last exiting sender through state_lock: it drops
> +	 * inflight_tx_cnt and wakes this drain while still holding
> +	 * state_lock, so observing the counter reach zero does not prove
> +	 * the sender has finished its mutex_unlock(). Acquiring the lock
> +	 * once here guarantees no sender still holds it - mutex_unlock()
> +	 * is the sender's last touch of chan_info - before returning lets
> +	 * devres free chan_info. shutdn blocks new senders, so one
> +	 * acquire/release pair is sufficient.
> +	 */
> +	mutex_lock(&chan_info->state_lock);
> +	mutex_unlock(&chan_info->state_lock);

[Severity: Low]
Does one lock/unlock pair give the guarantee this comment claims?

Documentation/locking/mutex-design.rst says:

  mutex_unlock() may access the mutex structure even after it has
  internally released the lock already - so it's not safe for another
  context to acquire the mutex and assume that the mutex_unlock()
  context is not using the structure anymore.

The last sender exits nbl_chan_send_msg() at out_clean_inflight like
this:

	mutex_lock(&chan_info->state_lock);
	if (atomic_dec_and_test(&chan_info->inflight_tx_cnt))
		wake_up(&chan_info->inflight_wait);
	mutex_unlock(&chan_info->state_lock);

The unlock slowpath of that sender can still be touching state_lock
after teardown has acquired the lock, released it and returned.

Today chan_info is devm-allocated and freed only at detach, so this
series cannot reach a use-after-free. Still, the claim would no longer
hold if chan_info were freed, or mutex_destroy() were called, right
after teardown.

[ ... ]

> +static int nbl_chan_setup_queue(struct nbl_channel_mgt *chan_mgt, u8 chan_type)
> +{

[ ... ]

> +	if (chan_info->dma_allocated) {
> +		dev_warn(common->dev,
> +			 "channel DMA already allocated, re-setup not supported\n");
> +		return -EBUSY;
> +	}
> +
> +	nbl_chan_init_queue_param(chan_info, NBL_CHAN_QUEUE_LEN,
> +				  NBL_CHAN_QUEUE_LEN, NBL_CHAN_BUF_LEN,
> +				  NBL_CHAN_BUF_LEN);
> +	/*
> +	 * Set the flag before the first allocation: if setup fails halfway,
> +	 * the buffers allocated so far already live until detach, so a retry
> +	 * must be rejected instead of allocating a second set and orphaning
> +	 * the first one.
> +	 */
> +	chan_info->dma_allocated = true;
> +	err = nbl_chan_init_queue(common, chan_info);
> +	if (err) {
> +		chan_info->dma_allocated = false;
> +		return err;
> +	}

[Severity: Medium]
Clearing dma_allocated here seems to allow the very retry that the
comment above says must be rejected. Is that intended?

nbl_chan_init_tx_queue() and nbl_chan_init_rx_queue() allocate with
dmam_alloc_coherent() and devm_kcalloc(). Anything allocated before a
failure part-way through stays allocated until detach.

A second setup_queue() call would then pass the dma_allocated check and
allocate a whole new set: about 2 MiB of coherent DMA, plus the
descriptor rings and wait[]. It would overwrite txq/rxq/wait and leave
the first set unreferenced.

A later patch in the series ("add common/ctrl dev init/remove
operation") depends on this guard. Its comment in
nbl_dev_setup_common_dev() says "chan_info->dma_allocated blocks a
re-setup".

Also, nbl_chan_teardown_queue() never clears dma_allocated, so the
channel cannot be set up again after teardown. Does that fit the "init,
configuration, start, stop and teardown" lifecycle described in the
commit message?

[ ... ]

> +	if (param->arg_len > NBL_CHAN_TX_DESC_EMBEDDED_DATA_LEN) {
> +		if (param->arg)
> +			memcpy(tx_buf->va, param->arg, param->arg_len);
> +		tx_desc->buf_addr = cpu_to_le64(tx_buf->pa);
> +		tx_desc->buf_len = cpu_to_le16(param->arg_len);

[Severity: Low]
This isn't a bug, but in nbl_chan_update_txqueue() buf_addr and buf_len
are programmed even when param->arg is NULL. A NULL arg with a non-zero
arg_len would send whatever is left in the slot from before. No caller
in the series passes that combination today. Would it be simpler to
reject it up front?

[ ... ]

> +	/*
> +	 * Entry points are only reached after a successful setup_queue()
> +	 * (probe path), but the channel is a dynamic object: fail cleanly
> +	 * instead of dereferencing rings/wait[] that were never allocated
> +	 * (num_txq_entries == 0).  active is published last in setup and
> +	 * cleared last in teardown, so a false value here means the channel
> +	 * was never set up; the teardown window itself is covered by the
> +	 * shutdn check below.
> +	 */
> +	if (!READ_ONCE(chan_info->active))
> +		return -ENODEV;

[Severity: Low]
Is this comment still true once teardown has completed?

nbl_chan_teardown_queue() ends with WRITE_ONCE(chan_info->active, false)
and leaves shutdn set. dma_allocated then stops setup from running
again.

A sender that arrives after teardown takes this branch and gets -ENODEV
instead of -ESHUTDOWN, even though the channel was set up.

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> index cf40ddc451921..eeff6216e4aa5 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c

[ ... ]

> +static void nbl_hw_stop_mailbox_rxq(struct nbl_hw_mgt *hw_mgt)
> +{
> +	struct nbl_mailbox_qinfo_cfg_table cfg_tbl;
> +
> +	memset(&cfg_tbl, 0, sizeof(cfg_tbl));
> +	cfg_tbl.data[3] = FIELD_PREP(NBL_MAILBOX_QINFO_CFG_QUEUE_RST_MASK, 1);
> +	nbl_hw_write_mbx_regs(hw_mgt, NBL_MAILBOX_QINFO_CFG_RX_TABLE_ADDR,
> +			      cfg_tbl.data, sizeof(cfg_tbl));

[Severity: Medium]
Is it safe to zero the ring base and size before QUEUE_RST takes effect?

nbl_hw_write_mbx_regs() writes the entry one dword at a time, data[0]
through data[3]:

	for (i = 0; i < len / 4; i++)
		nbl_mbx_wr32(hw_mgt, reg + i * sizeof(u32), data[i]);

data[0..2] (base low/high and QUEUE_SIZE_BWID) are therefore written as
zero while QUEUE_EN is still set. QUEUE_RST arrives only with data[3].

Teardown calls nbl_chan_stop_rx_queue() while the peer can still send.
nbl_chan_quiesce_and_reclaim_tx() stops TX while a descriptor is still
outstanding.

If the device applies each dword as it is written, could a transfer in
that window DMA to IOVA 0 or raise PTR_ERR/DIF_ERR? The same applies to
nbl_hw_stop_mailbox_txq(), and to the first {0, 0, 0, RST} write in
nbl_hw_config_mailbox_rxq() and nbl_hw_config_mailbox_txq().

If the hardware does not commit the entry on the data[3] write, would
writing only data[3] with QUEUE_RST set first avoid this?

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> index f2552bc732937..62484fbf1fef6 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c

[ ... ]

> +static int nbl_probe_chip_deps(struct pci_dev *pdev, bool has_ctrl)
> +{
> +	struct device_link *link;
> +	struct pci_dev *mgt;
> +
> +	if (has_ctrl)
> +		return 0;
> +
> +	mgt = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
> +					  pdev->bus->number,
> +					  PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
> +	if (!mgt) {
> +		dev_err(&pdev->dev,
> +			"management PF (func 0) not found on this chip\n");
> +		return -ENODEV;
> +	}

[Severity: Medium]
The commit message only says:

  This patch also hooks channel init/cleanup into the PF probe/remove
  paths in nbl_main.c.

nbl_probe_chip_deps() also changes how every PF with PCI_FUNC != 0
probes:

  - if function 0 is missing from the slot, probe fails with -ENODEV
  - if function 0 is unbound, or bound to vfio-pci or pci-stub, probe
    returns -EPROBE_DEFER indefinitely
  - a persistent DL_FLAG_AUTOPROBE_CONSUMER link is created, so
    unbinding function 0 force-unbinds every sibling PF

Nothing in this patch uses that ordering yet, because setup_queue and
cfg_chan_qinfo_map_table have no callers. Later patches ("add
common/ctrl dev init/remove operation", "add common dev start/stop
operation") rely on the link but only mention it in passing.

Could this go in its own patch, or at least be described and justified
in this commit message? A setup with PF0 on vfio-pci and PF1..3 on nbl
would now leave the sibling PFs deferred forever.

> +
> +	/*
> +	 * The management PF must have COMPLETED probing with this driver.
> +	 * While func 0 is still PROBING, its chip-global init (mailbox
> +	 * QINFO map programming) can race a sibling's mailbox setup, and a
> +	 * func 0 bound to another driver never ran it at all.  Defer until
> +	 * func 0 is bound to nbl; this also covers the unbound case.
> +	 */
> +	device_lock(&mgt->dev);

[Severity: Medium]
Can this self-deadlock when the probing device is an SR-IOV VF whose
devfn decodes to function 0?

nbl_get_func_param() sets has_ctrl only for a function 0 that is not a
VF:

	if ((PCI_FUNC(pdev->devfn) == 0) && !pdev->is_virtfn)
		param->caps.has_ctrl = 1;

Such a VF therefore gets has_ctrl == 0 and reaches the lookup. With ARI,
for example, a VF at devfn 0x08 decodes to slot 1, function 0, so
pci_get_domain_bus_and_slot() returns pdev itself.

device_lock(&mgt->dev) then takes the same dev->mutex that the driver
core already holds across really_probe(). The probe task would block
forever on a lock it owns. Later unbind, VF removal when SR-IOV is
disabled, and anything else that takes that device lock would then hang
too.

VFs can be created through another PF driver and bound to nbl with
driver_override. Should there be an is_virtfn or mgt == pdev check
before taking the lock?

[Severity: High]
Is there a lock ordering inversion here with the PCI core?

During funcN's probe the driver core holds device_lock(funcN), and this
code then takes device_lock(func0). That is the order N -> 0.

__pci_bus_lock() in drivers/pci/pci.c takes them in the opposite order.
It walks bus->devices in devfn order with the blocking pci_dev_lock():

	if (bridge)
		pci_dev_lock(bridge);
	list_for_each_entry(dev, &bus->devices, bus_list) {
		...
		else
			pci_dev_lock(dev);
	}

AER recovery reaches that blocking path:

aer_root_reset()
  pci_bus_error_reset()
    pci_reset_bridge(bridge, PCI_RESET_NO_RESTORE)
      pci_bus_reset(bus, PCI_RESET_DO_RESET)
        pci_bus_lock()

pci_slot_reset()->pci_slot_lock() reaches it as well.

Suppose the AER thread holds func0's lock and waits for funcN's. At the
same time funcN's probe, for example a deferred probe triggered when
func0 binds, holds funcN's lock and waits here for func0's. Wouldn't
both sides block forever?

dev->mutex uses lockdep_set_novalidate_class(), so lockdep would not
report this.

> +	if (mgt->dev.driver != &nbl_driver.driver ||
> +	    mgt->dev.links.status != DL_DEV_DRIVER_BOUND) {
> +		device_unlock(&mgt->dev);
> +		pci_dev_put(mgt);
> +		return -EPROBE_DEFER;
> +	}
> +	device_unlock(&mgt->dev);
> +
> +	link = device_link_add(&pdev->dev, &mgt->dev,
> +			       DL_FLAG_AUTOPROBE_CONSUMER);

[ ... ]

> +	device_lock(&mgt->dev);
> +	if (mgt->dev.links.status != DL_DEV_DRIVER_BOUND) {
> +		device_unlock(&mgt->dev);
> +		pci_dev_put(mgt);
> +		return -EPROBE_DEFER;
> +	}

[Severity: Medium]
Does this second check close the race described in the comment above
it?

device_lock(func0) is dropped before device_link_add(). If func0
unbinds in that gap, device_link_init_status() creates the managed link
in DL_STATE_DORMANT. If func0 then rebinds before this check,
device_links_driver_bound() on the supplier moves the link to
DL_STATE_AVAILABLE, not DL_STATE_CONSUMER_PROBE.

This check reads only mgt->dev.links.status == DL_DEV_DRIVER_BOUND, so
it accepts that state. device_links_busy() does not treat AVAILABLE as
busy, so another func0 unbind can run during the sibling's probe. When
the consumer finishes binding, device_links_driver_bound() hits:

	WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);

In a related interleaving func0 is rebound to pci-stub or vfio-pci. This
check does not re-test mgt->dev.driver against &nbl_driver.driver, so
the sibling carries on against a PF0 that never ran nbl's chip-global
setup.

Should this check also verify the bound driver, as the first check
does?

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