Re: [PATCH v29 net-next 3/8] net/nebula-matrix: add intr resource implementation

[netdev-bot+sashiko@kernel.org]

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

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

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] The deferred vector-recycling deadline in
  nbl_intr_retire_vectors_locked() is `retired->expires = jiffies +…
- [Medium] On reconfiguration, nbl_res_intr_cfg_msix_map() rewrites the
  coherent MSI-X map table in place while the hardware FUNCTION_MSIX_MAP…
- [Medium] The new interrupt module never invalidates the chip tables it
  owns at init, and nbl_hw_cfg_msix_map() programs them in an unsafe…
- [Low] Commit message says nbl_intr_mgt_stop() cleans up "leftover MSI-X
  maps (including remote PF maps via mailbox RPC)" and the new resource…
- [Low] The ownership note in nbl_res_remove_leonis() documents the wrong
  memory-management API family: it states "Tear down all MSI-X maps…
- [Low] struct nbl_interrupt_mgt embeds func_intr_res[NBL_MAX_FUNC] with
  NBL_MAX_FUNC == 520, but nbl_res_intr_cfg_msix_map() calls…
- [Low] Concern: __nbl_res_intr_destroy_msix_map() drops intr_mgt->lock
  and sleeps ~1 ms, then re-acquires the lock through a cached intr_mgt…
- [Low] Concern: __nbl_res_intr_set_mailbox_irq() validates only
  func_res->interrupts != NULL and vector_id < num_interrupts, but not…

--

net/nebula-matrix: add intr resource implementation

This adds a new nbl_interrupt module that owns the chip-internal MSI-X
vector index space (split into net and control bitmaps) and programs the
chip MSI-X mapping registers.  It also adds the hardware register
definitions and the resource/hw ops plumbing to reach the new code.

A few questions about the new module below.

Before the diff, one question about the commit message:

> - Global cleanup via nbl_intr_mgt_stop(): iterate all 520 function IDs to
> clean up leftover MSI-X maps (including remote PF maps via mailbox RPC)
> before devres releases coherent resources.

[Severity: Low]
nbl_intr_mgt_stop() only walks its own intr_mgt->func_intr_res[] state and
writes registers directly through hw_ops; there is no channel or mailbox
call anywhere in the new module.

Is the "via mailbox RPC" part meant to describe how those remote PF maps
were originally created (later in the series,
nbl_disp_chan_cfg_msix_map_resp() calls res_ops->cfg_msix_map(p, src_id,
...)), rather than how they are cleaned up here?

Related: none of cfg_msix_map, destroy_msix_map or set_mailbox_irq has an
in-tree caller in this patch; they are wired up by a later patch in the
series.  Would it help to say so explicitly, the same way the message
already does for pci_alloc_irq_vectors()?

> 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 82efe2b40a8c7..457e0212a7bfa 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
> @@ -104,6 +104,20 @@ static void nbl_hw_rd_regs_lock(struct nbl_hw_mgt *hw_mgt, u64 reg, u32 *data,
>  	spin_unlock(&hw_mgt->reg_lock);
>  }
>  
> +static void nbl_hw_wr_regs_lock(struct nbl_hw_mgt *hw_mgt, u64 reg,
> +				const u32 *data, u32 len)
> +{
> +	u32 size = len / 4;
> +	u32 i;
> +
> +	if (len % 4)
> +		return;
> +	spin_lock(&hw_mgt->reg_lock);
> +	for (i = 0; i < size; i++)
> +		wr32(hw_mgt->hw_addr, reg + i * sizeof(u32), data[i]);
> +	spin_unlock(&hw_mgt->reg_lock);
> +}
> +

[ ... ]

> @@ -130,6 +144,126 @@ static void nbl_hw_get_fw_eth_map(struct nbl_hw_mgt *hw_mgt, u32 *eth_map)
>  	*eth_map = FIELD_GET(NBL_FW_BOARD_DW6_ETH_BITMAP_MASK, data);
>  }
>  

[ ... ]

> +static void nbl_hw_cfg_msix_map(struct nbl_hw_mgt *hw_mgt, u16 func_id,
> +				bool valid, dma_addr_t dma_addr, u8 bus,
> +				u8 devid, u8 function)
> +{
> +	struct nbl_function_msix_map function_msix_map;
> +
> +	memset(&function_msix_map, 0, sizeof(function_msix_map));
> +	if (valid) {
> +		function_msix_map.data[0] = lower_32_bits(dma_addr);
> +		function_msix_map.data[1] = upper_32_bits(dma_addr);
> +		/* use ctrl dev's bdf, because the dma memory was
> +		 * allocated by it
> +		 */
> +		function_msix_map.data[2] =
> +			FIELD_PREP(NBL_FUNCTION_MSIX_MAP_FUNCTION_MASK,
> +				   function) |
> +			FIELD_PREP(NBL_FUNCTION_MSIX_MAP_DEVID_MASK, devid) |
> +			FIELD_PREP(NBL_FUNCTION_MSIX_MAP_BUS_MASK, bus) |
> +			FIELD_PREP(NBL_FUNCTION_MSIX_MAP_VALID_MASK, 1);

[Severity: Medium]
This programs the map entry in ascending dword order through
nbl_hw_wr_regs_lock(): low address dword, high address dword, then
BDF plus VALID.  If the entry is already VALID from a previous driver
instance, can the pcompleter fetch the map with a spliced base address
(new low dword, stale high dword) before data[2] is rewritten?

The teardown path in this same function deliberately does the opposite
(keeps a live address, clears VALID) with the comment:

	 * Keep valid live dma address here, only clear VALID flag.

Would clearing VALID first, flushing, then writing the address and
setting VALID last be the safer order here as well?

Related: nbl_intr_mgt_start() -> nbl_intr_setup_mgt() only allocates and
zeroes software state; it does not invalidate
NBL_PCOMPLETER_FUNCTION_MSIX_MAP, NBL_PADPT_HOST_MSIX_INFO or
NBL_PCOMPLETER_HOST_MSIX_FID_TABLE at init.  The comment added above
nbl_hw_set_mailbox_irq() says these chip registers "survive kexec/forced
unload without FLR", which is why nbl_hw_cfg_mailbox_qinfo() clears
MSIX_IDX_VALID at init.  Does the same reasoning apply to the tables
added here, which additionally hold a host DMA address that the new
kernel now owns?

> +	} else {
> +		/*
> +		 * reg_lock prevents concurrent CPU writes to the same
> +		 * function's MSIX entry, but cannot synchronize hardware DMA
> +		 * reads. Upper layer uses two-stage destruction + sync sleep
> +		 * to avoid torn hardware read of partial MSIX entry.
> +		 * Keep valid live dma address here, only clear VALID flag.
> +		 */
> +		function_msix_map.data[0] = lower_32_bits(dma_addr);
> +		function_msix_map.data[1] = upper_32_bits(dma_addr);
> +		function_msix_map.data[2] = 0;
> +	}
> +
> +	nbl_hw_wr_regs_lock(hw_mgt,
> +			    NBL_PCOMPLETER_FUNCTION_MSIX_MAP_REG_ARR(func_id),
> +			    function_msix_map.data, sizeof(function_msix_map));
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_resource_leonis.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_resource_leonis.c
> index 7804762a96e05..62af1738e2fdf 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_resource_leonis.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_resource_leonis.c

[ ... ]

> @@ -322,8 +331,26 @@ int nbl_res_init_leonis(struct nbl_adapter *adap)
>  
>  void nbl_res_remove_leonis(struct nbl_adapter *adap)
>  {
> +	struct nbl_resource_mgt *res_mgt = adap->core.res_mgt;
> +	struct nbl_common_info *common = &adap->common;
> +
> +	if (!res_mgt)
> +		return;
> +
>  	/*
> -	 * No resource release here because all memory uses devm managed
> -	 * allocation
> +	 * Tear down all MSI-X maps before devres releases the coherent
> +	 * tables.	This is critical on the control PF, which may hold
> +	 * maps for remote PFs that are still bound.
> +	 */
> +	if (common->has_ctrl && res_mgt->intr_mgt)
> +		nbl_intr_mgt_stop(res_mgt);
> +
> +	/* Note:
> +	 * per-function interrupts arrays (kcalloc) are freed by
> +	 * nbl_intr_mgt_stop().
> +	 * MSIX coherent tables are explicitly freed by dmam_free_coherent()
> +	 * inside the intr destroy path, before nbl_intr_mgt_stop() returns.

[Severity: Low]
Does this note describe the wrong API family?  The new module allocates
with dma_alloc_coherent() in nbl_res_intr_cfg_msix_map() and frees with
dma_free_coherent() in __nbl_res_intr_complete_destroy_msix_map() and in
the release_vecs_unlock error path; dmam_ is never used, so there is no
devres record for these tables and nothing for devres to release.

Either correcting the comment or switching to dmam_alloc_coherent() would
make the ownership model match the code.  As written, a future error path
that skips the explicit free would leak the coherent table for the
lifetime of the device with no devres safety net.

> +	 * intr_mgt itself (devm_kzalloc) is released by devres after this
> +	 * function returns
>  	 */
>  }
> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c
> new file mode 100644
> index 0000000000000..e99a9d65c758b
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c
> @@ -0,0 +1,742 @@

[ ... ]

> +#define NBL_MSIX_DMA_SYNC_MIN_US	1000 /* us */
> +#define NBL_MSIX_DMA_SYNC_MAX_US	1200 /* us */

[ ... ]

> +static void nbl_intr_retire_vectors_locked(struct nbl_resource_mgt *res_mgt,
> +					   u16 *vectors, u16 cnt)
> +{
> +	struct nbl_interrupt_mgt *intr_mgt = res_mgt->intr_mgt;
> +	struct nbl_intr_retired_vectors *retired;
> +
> +	retired = kzalloc_obj(*retired, GFP_KERNEL);
> +	if (retired) {
> +		retired->vectors = vectors;
> +		retired->cnt = cnt;
> +		retired->expires = jiffies +
> +			usecs_to_jiffies(NBL_MSIX_DMA_SYNC_MIN_US);

[Severity: Medium]
Does this deadline actually deliver the ~1000 us quiesce window?

usecs_to_jiffies(1000) is 1 for the common HZ values, and adding a jiffy
count to the current jiffies only guarantees (count - 1) full ticks of
elapsed time.  With count == 1 the guaranteed wait is ~0 us, since the
current tick may be about to end.

nbl_intr_sweep_retired_locked() then releases the indices as soon as:

	if (!time_after_eq(jiffies, retired->expires))
		return;

holds, and nbl_res_intr_cfg_msix_map() calls the sweep immediately before
allocating from the same bitmaps with find_first_zero_bit().  So a just
retired global index can be reprogrammed into PADPT_HOST_MSIX_INFO and
PCOMPLETER_HOST_MSIX_FID_TABLE with a different function's BDF and FID
microseconds later.

The synchronous path uses usleep_range(NBL_MSIX_DMA_SYNC_MIN_US,
NBL_MSIX_DMA_SYNC_MAX_US) for exactly this purpose, and the struct field
is documented as "jiffies at which recycling is safe".  Would adding one
extra jiffy, or using a ktime based deadline, keep the documented window?

> +		list_add_tail(&retired->node, &intr_mgt->retired_list);
> +		return;
> +	}

[ ... ]

> +static int __nbl_res_intr_set_mailbox_irq(struct nbl_resource_mgt *res_mgt,
> +					  u16 func_id, u16 vector_id,
> +					  bool en_msix)
> +{

[ ... ]

> +	if (!intr_mgt->func_intr_res[func_id].interrupts) {
> +		dev_err(dev, "func %u MSIX map not configured\n", func_id);
> +		return -ENODEV;
> +	}

[Severity: Low]
This isn't reachable today, but should the enable path also test
state == NBL_INTR_FUNC_CONFIGURED (and intr_mgt->stopping), the way
nbl_res_intr_cfg_msix_map() rejects NBL_INTR_FUNC_DESTROYING with -EBUSY?

During the unlocked quiesce window of a teardown the function is in
NBL_INTR_FUNC_DESTROYING but interrupts[] is still populated, so only the
pointer and range checks here would pass.  An en_msix=true call in that
window would arm NBL_MAILBOX_QINFO_MAP MSIX_IDX/MSIX_IDX_VALID with a
global vector that __nbl_res_intr_complete_destroy_msix_map() is about to
release, and the complete phase never re-disables routing, so the routing
would outlive the vector.

I could not build that race with the current callers: every caller of
res_ops->set_mailbox_irq and res_ops->destroy_msix_map holds
disp_mgt->ops_mutex_lock across the whole resource call, and
nbl_intr_mgt_stop() runs only after the mailbox work item is cancelled.
The asymmetry between the two entry points still looks worth closing.

> +
> +	if (vector_id >= intr_mgt->func_intr_res[func_id].num_interrupts) {

[ ... ]

> +static int __nbl_res_intr_destroy_msix_map(struct nbl_resource_mgt *res_mgt,
> +					   u16 func_id)
> +{
> +	struct nbl_interrupt_mgt *intr_mgt = res_mgt->intr_mgt;
> +	int ret;
> +
> +	lockdep_assert_held(&intr_mgt->lock);
> +
> +	ret = __nbl_res_intr_prepare_destroy_msix_map(res_mgt, func_id);
> +	if (ret)
> +		return ret;
> +	mutex_unlock(&intr_mgt->lock);
> +	usleep_range(NBL_MSIX_DMA_SYNC_MIN_US, NBL_MSIX_DMA_SYNC_MAX_US);
> +	mutex_lock(&intr_mgt->lock);

[Severity: Low]
This isn't reachable with the callers in this series, but the lock is
dropped for ~1 ms and then retaken through the cached intr_mgt pointer,
while nbl_intr_mgt_stop() can set res_mgt->intr_mgt = NULL outside the
lock and let devres free the object:

nbl_intr_mgt_stop()
	mutex_unlock(&intr_mgt->lock);
	res_mgt->intr_mgt = NULL;

A thread parked in the window above would then either take a freed mutex
here, or dereference NULL when
__nbl_res_intr_complete_destroy_msix_map() re-reads res_mgt->intr_mgt.

I traced the callers and could not construct the overlap: all three public
ops are invoked under disp_mgt->ops_mutex_lock, the local path runs from
nbl_core_stop() before nbl_core_remove(), and nbl_dev_remove_common_dev()
does cancel_work_sync(&common_dev->clean_mbx_task) and tears down the
mailbox queue before nbl_res_remove_leonis() calls nbl_intr_mgt_stop().

Since nbl_intr_mgt_stop() has no admission or drain protocol, would an
explicit "no users in flight" guarantee (or clearing res_mgt->intr_mgt
under the lock) be worth adding so this stays safe if a caller outside the
dispatch mutex appears later?

> +
> +	return __nbl_res_intr_complete_destroy_msix_map(res_mgt, func_id);
> +}

[ ... ]

> +	/*
> +	 * Phase2: tear down old hardware MSIX config. Old vectors are
> +	 * moved to the retired list and recycled only after the
> +	 * hardware pipeline has quiesced.
> +	 * NOTE: NO DMA table free here, only invalidate HW entries.
> +	 */
> +	if (intr_mgt->func_intr_res[func_id].interrupts) {
> +		old_interrupts = intr_mgt->func_intr_res[func_id].interrupts;
> +		old_num = intr_mgt->func_intr_res[func_id].num_interrupts;
> +
> +		ret = __nbl_res_intr_set_mailbox_irq(res_mgt, func_id, 0,
> +						     false);
> +		if (ret) {
> +			dev_err(dev, "%s: disable old mailbox irq failed, keep old config\n",
> +				__func__);
> +			goto release_vecs_unlock;
> +		}
> +		for (j = 0; j < old_num; j++) {
> +			hw_ops->cfg_msix_info(res_mgt->hw_ops_tbl->priv,
> +					      func_id, false, old_interrupts[j],
> +					      0, 0, 0, false);
> +		}

[Severity: Medium]
On the reconfiguration path, only the per-vector PADPT_HOST_MSIX_INFO
entries are invalidated here; cfg_msix_map(..., valid=false, ...) is never
called, so NBL_FUNCTION_MSIX_MAP_VALID_MASK stays set for this function
and the pcompleter can DMA-read the table at any point.

The code then rewrites that same live table in place below, one entry at a
time, with the only barrier issued after the whole loop.  Can the device
observe a torn mix of old and new translations, for example a new gvec
whose PADPT_HOST_MSIX_INFO and FID entries are not valid yet, or a cleared
entry for a vector still in use?

__nbl_res_intr_prepare_destroy_msix_map() treats this as a real hazard and
does clear the map VALID bit, flush, and wait ~1 ms before touching the
table.  Should the reconfiguration path follow the same sequence, or
double-buffer the table?

> +		hw_ops->flush_write(res_mgt->hw_ops_tbl->priv);
> +		nbl_intr_retire_vectors_locked(res_mgt, old_interrupts,
> +					       old_num);

[ ... ]

> +	msix_map_entries = official_tbl->base_addr;
> +	memset(msix_map_entries + requested, 0,
> +	       (NBL_MSIX_MAP_TABLE_MAX_ENTRIES - requested) *
> +	       sizeof(*msix_map_entries));
> +
> +	for (i = 0; i < requested; i++) {
> +		gvec = intr_mgt->func_intr_res[func_id].interrupts[i];
> +		msix_map_entries[i].data =
> +			cpu_to_le16(FIELD_PREP(NBL_MSIX_MAP_VALID_MASK, 1) |
> +				    FIELD_PREP(NBL_MSIX_MAP_INDEX_MASK,
> +					       gvec));
> +		entry_masked = (i < num_net_msix && net_msix_mask_en);
> +		hw_ops->cfg_msix_info(res_mgt->hw_ops_tbl->priv,
> +				      func_id, true, gvec,
> +				      bus, devid, function,
> +				      entry_masked);
> +	}
> +	hw_ops->flush_write(res_mgt->hw_ops_tbl->priv);
> +
> +	/* Ensure coherent memory writes are visible before HW DMA fetch */
> +	dma_wmb();

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.c
> index b316fb8e70519..635f34312c569 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.c
> @@ -68,6 +68,38 @@ int nbl_res_vsi_id_to_pf_id(struct nbl_resource_mgt *res_mgt, u16 vsi_id)

[ ... ]

> +	if (rel_pf_id >= common->max_pf) {
> +		dev_err(common->dev,
> +			"func_id=%u rel_pf_id=%u exceeds max_pf=%u, VF BDF unsupported\n",
> +			pfid, rel_pf_id,
> +			common->max_pf);
> +		return -EOPNOTSUPP;
> +	}

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.h
> index ae0a3d33198dc..c69d6f2ca3218 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.h

[ ... ]

> +struct nbl_interrupt_mgt {
> +	struct mutex lock; /* Protects bitmap + func_intr_res[] */
> +	DECLARE_BITMAP(intr_net_bmap, NBL_MAX_NET_INTERRUPT);
> +	DECLARE_BITMAP(intr_other_bmap, NBL_MAX_OTHER_INTERRUPT);
> +	struct list_head retired_list;
> +	bool stopping; /* set on teardown, rejects new configurations */
> +	struct nbl_func_interrupt_resource_mng func_intr_res[NBL_MAX_FUNC];
> +};

[Severity: Low]
Can any func_id above max_pf ever occupy one of these 520 entries?

nbl_res_intr_cfg_msix_map() calls nbl_res_func_id_to_bdf() before
recording any state, and that helper returns -EOPNOTSUPP whenever
rel_pf_id >= common->max_pf, with max_pf bounded by NBL_MAX_PF (8).  So at
most 8 of the 520 entries can reach NBL_INTR_FUNC_CONFIGURED.

struct nbl_func_interrupt_resource_mng is 48 bytes after padding, so the
array is roughly 25 KB embedded in the single devm_kzalloc for intr_mgt
(an order-3 allocation), of which about 24.5 KB can never be used, and
both nbl_intr_mgt_stop() loops scan 520 entries that can never leave
NBL_INTR_FUNC_IDLE.

The commit message lists both "iterate all 520 function IDs" and "PF-only
support: explicitly reject VF function IDs with -EOPNOTSUPP" as features.
Which of the two is the intended design?

> +
>  /* --------- INFO ---------- */
>  struct nbl_sriov_info {
>  	unsigned int bdf;

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_include.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_include.h
> index 59e44feab44fb..2c959832c32f4 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_include.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_include.h
> @@ -13,6 +13,7 @@
>  #define NBL_MAX_PF					8
>  #define NBL_NEXT_ID(id, max) (((id) + 1) % ((max) + 1))
>  
> +#define NBL_MAX_FUNC					520
>  #define NBL_MAX_ETHERNET				4

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