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

[netdev-bot+sashiko@kernel.org]

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

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

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] In nbl_probe_chip_deps(), a failed device_trylock(&mgt->dev)
  is turned into -EPROBE_DEFER, on the assumption that a later bind of…
- [Low] The comments in nbl_chan_cfg_qinfo_map_table() and the
  get_host_pf_mask kernel-doc say pf_mask is validated by…
- [Low] The commit message says 'a channel costs one devres node and one
  IOMMU mapping per ring', and the code comment in…

--

net/nebula-matrix: add channel layer

This adds a mailbox-based channel layer that carries control messages
between PF0 and the other PFs. It uses TX/RX rings backed by coherent
DMA, an xarray of message handlers and ACK wait slots. It also adds
nbl_probe_chip_deps(), which defers non-management PFs until function 0
is bound to this driver, and links each of them to function 0 with a
device link.

> 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..a4b1110134f83
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> @@ -0,0 +1,1522 @@

[ ... ]

> +static int nbl_chan_init_tx_queue(struct nbl_common_info *common,
> +				  struct nbl_chan_info *chan_info)
> +{
> +	struct nbl_chan_ring *txq = &chan_info->txq;
> +	struct device *dev = common->dev;
> +	dma_addr_t base_dma;
> +	size_t size =
> +		chan_info->num_txq_entries * sizeof(struct nbl_chan_tx_desc);
> +	void *base;
> +	u16 i;
> +
> +	txq->desc.tx_desc = dmam_alloc_coherent(dev, size, &txq->dma,
> +						GFP_KERNEL);
> +	if (!txq->desc.tx_desc)
> +		return -ENOMEM;
> +
> +	/*
> +	 * All per-entry buffers come from one contiguous coherent block, so a
> +	 * ring costs exactly one allocation, one devres node and one IOMMU
> +	 * mapping whatever the page size is.  Per-entry dmam_alloc_coherent()
> +	 * instead rounds every NBL_CHAN_BUF_LEN buffer up to a whole page
> +	 * (8 MiB per PF on 16 KiB pages, 32 MiB on 64 KiB pages), and a
> +	 * dma_pool only fixes the large-page case: with NBL_CHAN_BUF_LEN equal
> +	 * to PAGE_SIZE it still degenerates to one allocation per buffer on
> +	 * 4 KiB pages.  NBL_CHAN_BUF_LEN is a power of two, so the block
> +	 * always divides evenly into per-entry slots.
> +	 */
> +	base = dmam_alloc_coherent(dev,
> +				   chan_info->num_txq_entries *
> +				   chan_info->txq_buf_size,
> +				   &base_dma, GFP_KERNEL);
> +	if (!base)
> +		return -ENOMEM;
> +
> +	chan_info->wait = devm_kcalloc(dev, chan_info->num_txq_entries,
> +				       sizeof(*chan_info->wait), GFP_KERNEL);

[Severity: Low]
This isn't a bug, but is the "exactly one allocation, one devres node
and one IOMMU mapping" wording accurate for a ring?

This function calls dmam_alloc_coherent() twice, once for the descriptor
ring and once for the buffer block. It also makes devm_kcalloc() calls
for chan_info->wait and txq->buf. nbl_chan_init_rx_queue() likewise
makes two dmam_alloc_coherent() calls plus a devm_kcalloc() for
rxq->buf.

The commit message makes the same claim:

  Each ring uses one contiguous coherent DMA block sliced into
  fixed-size slots, so a channel costs one devres node and one IOMMU
  mapping per ring instead of one per slot.

Could both say that this only applies to the per-slot data buffers?

[ ... ]

> +static void nbl_chan_cfg_qinfo_map_table(struct nbl_channel_mgt *chan_mgt,
> +					 u8 bus, u8 devid)
> +{
> +	struct nbl_hw_ops *hw_ops = chan_mgt->hw_ops_tbl->ops;
> +	struct nbl_hw_mgt *p = chan_mgt->hw_ops_tbl->priv;
> +	u32 pf_mask = 0;
> +	u8 func_id;
> +
> +	/*
> +	 * k_pf_mask rule: bit N == 0 means PF#N enabled, bit N == 1 masked out.
> +	 * Program mailbox QINFO entry for each hardware-active PF func_id.
> +	 *
> +	 * Note: This loop iterates over the raw hardware PF func_id.
> +	 * Product constraints limit supported PF counts to the contiguous
> +	 * sets PF0, PF0~1 and PF0~3.  The mask is validated by
> +	 * nbl_res_init_pf_num() in the resource layer, which runs before
> +	 * this call and rejects any other mask with -EINVAL, so pf_mask
> +	 * here is always one of 0xfe/0xfc/0xf0.
> +	 */

[Severity: Low]
This isn't a bug, but nbl_res_init_pf_num() and the resource layer do
not exist yet at this commit. They arrive later in the series. The
get_host_pf_mask kernel-doc added to nbl_def_hw.h makes the same
ordering claim.

cfg_chan_qinfo_map_table() has no caller here yet, and once the whole
series is applied the described ordering does hold. Could these
comments move into the patch that adds nbl_res_init_pf_num(), or be
reworded so they are accurate at this point in the series?

[ ... ]

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

[ ... ]

> +	/*
> +	 * func 0's device lock is taken with a trylock.  This probe already
> +	 * runs under device_lock(funcN), so a blocking device_lock(func0)
> +	 * would establish the N -> 0 order, while __pci_bus_lock() (AER
> +	 * recovery, slot reset) walks the bus in devfn order, i.e. 0 -> N,
> +	 * and dev->mutex is lockdep_set_novalidate_class(), so lockdep
> +	 * cannot report that inversion.  A trylock never blocks, so it
> +	 * cannot take part in a deadlock cycle; failing it defers this
> +	 * probe.

[ ... ]

> +	if (!device_trylock(&mgt->dev)) {
> +		pci_dev_put(mgt);
> +		return -EPROBE_DEFER;
> +	}

[Severity: Medium]
Can a failed trylock here leave the sibling PF deferred with nothing to
retry it later?

When func 0 binds, really_probe()->driver_bound() runs with
device_lock(func0) held. device_links_driver_bound() puts the
AUTOPROBE consumer on the pending list. After that:

drivers/base/dd.c:driver_bound() {
    ...
	driver_deferred_probe_trigger();

	bus_notify(dev, BUS_NOTIFY_BOUND_DRIVER);
	kobject_uevent(&dev->kobj, KOBJ_BIND);
}

So deferred_probe_work_func() can re-probe funcN on another CPU while
func 0's lock is still held. The trylock then fails and funcN returns
-EPROBE_DEFER again.

driver_probe_device() only retriggers when deferred_trigger_count
changed during the probe:

	if (trigger_count != atomic_read(&deferred_trigger_count) &&
	    !defer_all_probes)
		driver_deferred_probe_trigger();

Here the increment happened before funcN's probe read trigger_count.
So nothing retriggers, and funcN stays on deferred_probe_pending_list.

Other holders of func 0's device lock that never bind a driver seem to
have the same effect. Examples are __pci_bus_lock() during AER recovery
or a bus/slot reset, a reset through sysfs, or a write to
sriov_numvfs. No bind event follows any of these.

In both cases the sibling PF and its ports stay unbound until some
unrelated driver binds elsewhere in the system. Doesn't that break the
"re-probes them when function 0 binds again" behaviour described in the
commit message?

> +
> +	/*
> +	 * The management PF must be bound to *this* driver, with its probe
> +	 * completed.  links.status == DL_DEV_DRIVER_BOUND alone only says
> +	 * "bound to some driver", so func 0 on vfio-pci or pci-stub
> +	 * (driver_override) would pass it and this PF would run with func
> +	 * 0's chip-global init never performed.  pdev->dev.driver is our
> +	 * own driver, set by the core before ->probe() is called.
> +	 *
> +	 * Deferring is enough to recover: a successful bind re-triggers
> +	 * deferred probing for every pending device (driver_bound() ->
> +	 * driver_deferred_probe_trigger()), so this PF is retried once func
> +	 * 0 is bound to this driver.
> +	 */

[ ... ]

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