Re: [PATCH v13 net-next 07/11] net/nebula-matrix: add intr resource implementation

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/nebula-matrix: add intr resource implementation

This patch adds an interrupt resource module that tracks MSI-X vector
allocation per function via two bitmaps and a per-function array, and
introduces three resource ops (configure_msix_map, destroy_msix_map,
enable_mailbox_irq) plus the matching hw_ops (configure_msix_map,
configure_msix_info, enable_mailbox_irq) that program the device MSI-X
mapping and info registers.

A few questions below on the new code.

> 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 1ec1c28d99ff..1cceb0df5e1e 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
> @@ -12,6 +12,7 @@ static struct nbl_resource_ops res_ops = {
>  	.get_eth_id = nbl_res_get_eth_id,
>  };
>  
> +static bool is_ops_inited;

Is this lazy initialization of a file-scope vtable safe against concurrent
probes?

nbl_res_setup_ops() does an unlocked check-then-set on is_ops_inited and
then mutates the file-scope res_ops by calling nbl_intr_setup_ops():

	if (!is_ops_inited) {
		ret = nbl_intr_setup_ops(&res_ops);
		if (ret)
			return ERR_PTR(-ENOMEM);
		is_ops_inited = true;
	}

Two probe threads for different functions on the same device could both
observe is_ops_inited == false and race on writing the same pointer fields
into res_ops.  Would a static designated initializer in res_ops (keeping
res_ops const and dropping is_ops_inited / nbl_intr_setup_ops entirely)
remove the race and simplify the scaffolding?

Related: nbl_intr_setup_ops() only ever returns 0, so the caller's
"if (ret) return ERR_PTR(-ENOMEM);" is unreachable, and mapping any future
failure to -ENOMEM would also be incorrect.  Could this function be void?

[ ... ]

> 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 000000000000..92a6fff5523e
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_interrupt.c
> @@ -0,0 +1,246 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Nebula Matrix Limited.
> + */
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include "nbl_interrupt.h"
> +
> +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;
> +	struct nbl_hw_ops *hw_ops = res_mgt->hw_ops_tbl->ops;
> +	struct device *dev = res_mgt->common->dev;
> +	struct nbl_msix_map_table *msix_map_table;
> +	u16 *interrupts;
> +	u16 intr_num, i;
> +
> +	/* use ctrl dev bdf */
> +	hw_ops->configure_msix_map(res_mgt->hw_ops_tbl->priv, func_id, false, 0,
> +				   0, 0, 0);
> +
> +	intr_num = intr_mgt->func_intr_res[func_id].num_interrupts;
> +	interrupts = intr_mgt->func_intr_res[func_id].interrupts;

Is func_id bounds-checked anywhere on this path?  intr_mgt->func_intr_res[]
is sized NBL_MAX_FUNC (520) but the u16 func_id (0..65535) is indexed
directly, and nbl_res_func_id_to_bdf() (which validates against max_pf) is
not called here.  If func_id arrives from a mailbox message in a later
patch, can this index out of bounds?

The same question applies to nbl_res_intr_enable_mailbox_irq() and to the
first use of func_intr_res[func_id] in nbl_res_intr_configure_msix_map()
below, which happens before nbl_res_func_id_to_bdf() is called.

> +
> +	if (!interrupts) {
> +		dev_err(dev, "No interrupts to clr for func_id %u\n", func_id);
> +		return -EINVAL;
> +	}
> +	for (i = 0; i < intr_num; i++) {
> +		if (interrupts[i] >= NBL_MAX_OTHER_INTERRUPT)
> +			clear_bit(interrupts[i] - NBL_MAX_OTHER_INTERRUPT,
> +				  intr_mgt->interrupt_net_bitmap);
> +		else
> +			clear_bit(interrupts[i],
> +				  intr_mgt->interrupt_others_bitmap);

What serializes access to intr_mgt->interrupt_net_bitmap,
intr_mgt->interrupt_others_bitmap and intr_mgt->func_intr_res[func_id]
across concurrent invocations of these three ops?

set_bit() / clear_bit() are individually atomic, but the sequence used in
nbl_res_intr_configure_msix_map():

	intr_index = find_first_zero_bit(intr_mgt->interrupt_net_bitmap,
					 NBL_MAX_NET_INTERRUPT);
	...
	set_bit(intr_index, intr_mgt->interrupt_net_bitmap);

is a non-atomic read-modify-write.  Two concurrent calls for different
func_ids could each observe the same zero bit and both claim it, resulting
in two functions being programmed with the same global MSI-X index.

The per-function state has a similar concern: configure, destroy and
enable_mailbox_irq all read/write func_intr_res[func_id].interrupts,
num_interrupts and msix_map_table without any lock held.  Concurrent
callers on the same func_id can race the "if (interrupts) destroy" TOCTOU
and realloc, double-kfree() the interrupts array, or dma_free_coherent()
twice.  enable_mailbox_irq dereferences interrupts[vector_id] which
destroy_msix_map has just freed and set to NULL -- is this a
use-after-free?

The WARN_ON(msix_map_table->base_addr) below suggests single-threaded use
is assumed; what enforces that?

[ ... ]

> +	kfree(interrupts);
> +	intr_mgt->func_intr_res[func_id].interrupts = NULL;
> +	intr_mgt->func_intr_res[func_id].num_interrupts = 0;
> +
> +	msix_map_table = &intr_mgt->func_intr_res[func_id].msix_map_table;
> +	dma_free_coherent(dev, msix_map_table->size, msix_map_table->base_addr,
> +			  msix_map_table->dma);

Is there a teardown path that iterates over func_intr_res[] and calls
destroy_msix_map() for each active func_id on module unload / device
remove?

The interrupts array (kcalloc) and the msix_map_table (dma_alloc_coherent)
allocated in configure_msix_map are not devm-managed, and
nbl_res_remove_leonis() does not walk func_intr_res[].  Any func_id still
mapped at unload time would leak both allocations and leave a stale bus
address programmed in the hardware MSI-X map register.

> +	msix_map_table->size = 0;
> +	msix_map_table->base_addr = NULL;
> +	msix_map_table->dma = 0;
> +
> +	return 0;
> +}
> +
> +static int nbl_res_intr_configure_msix_map(struct nbl_resource_mgt *res_mgt,
> +					   u16 func_id, u16 num_net_msix,
> +					   u16 num_others_msix,
> +					   bool net_msix_mask_en)
> +{
> +	struct nbl_interrupt_mgt *intr_mgt = res_mgt->intr_mgt;
> +	struct nbl_hw_ops *hw_ops = res_mgt->hw_ops_tbl->ops;
> +	struct nbl_common_info *common = res_mgt->common;
> +	struct nbl_msix_map_table *msix_map_table;
> +	struct nbl_msix_map *msix_map_entries;
> +	struct device *dev = common->dev;
> +	u16 requested, intr_index;
> +	u8 bus, devid, function;
> +	bool msix_mask_en;
> +	u16 *interrupts;
> +	int ret = 0;
> +	u16 i;
> +
> +	requested = num_net_msix + num_others_msix;
> +	if (requested > NBL_MSIX_MAP_TABLE_MAX_ENTRIES)
> +		return -EINVAL;
> +	if (intr_mgt->func_intr_res[func_id].interrupts)
> +		nbl_res_intr_destroy_msix_map(res_mgt, func_id);

Is this reconfigure ordering intentional?

The existing mapping is torn down (dma_free_coherent, kfree, and
hw_ops->configure_msix_map(valid=false)) before the new allocations are
attempted.  If the subsequent dma_alloc_coherent or kcalloc fails, the
function returns -ENOMEM with the previously working interrupt setup
already gone and no rollback.  Would allocate-then-swap be safer?

> +
> +	ret = nbl_res_func_id_to_bdf(res_mgt, func_id, &bus, &devid, &function);
> +	if (ret == U32_MAX)
> +		return -EINVAL;

nbl_res_func_id_to_bdf() is declared to return int but returns U32_MAX on
error, and the caller stores it in int ret and compares against U32_MAX.
This currently works only because -1 sign-extends to 0xFFFFFFFF.  If the
callee is later changed to return a normal negative errno (for example
-EINVAL), will this check silently stop detecting failures and let
uninitialised bus/devid/function be programmed into hardware?

> +
> +	msix_map_table = &intr_mgt->func_intr_res[func_id].msix_map_table;
> +	WARN_ON(msix_map_table->base_addr);
> +	msix_map_table->size =
> +		sizeof(struct nbl_msix_map) * NBL_MSIX_MAP_TABLE_MAX_ENTRIES;
> +	msix_map_table->base_addr = dma_alloc_coherent(dev,
> +						       msix_map_table->size,
> +						       &msix_map_table->dma,
> +						       GFP_KERNEL);

[ ... ]

> +	msix_map_entries = msix_map_table->base_addr;
> +	for (i = 0; i < requested; i++) {
> +		msix_map_entries[i].global_msix_index = interrupts[i];
> +		msix_map_entries[i].valid = 1;
> +
> +		if (i < num_net_msix && net_msix_mask_en)
> +			msix_mask_en = 1;
> +		else
> +			msix_mask_en = 0;
> +		hw_ops->configure_msix_info(res_mgt->hw_ops_tbl->priv, func_id,
> +					    true, interrupts[i], bus, devid,
> +					    function, msix_mask_en);
> +	}
> +
> +	/* use ctrl dev bdf */
> +	hw_ops->configure_msix_map(res_mgt->hw_ops_tbl->priv, func_id, true,
> +				   msix_map_table->dma, common->hw_bus,
> +				   common->devid, common->function);

Is a dma_wmb() (or equivalent) needed between the stores into the
dma_alloc_coherent table (msix_map_entries[i] fields above) and the MMIO
writel() in hw_ops->configure_msix_map that arms the device to DMA-read
that table?

dma_alloc_coherent guarantees cache coherence but not ordering between
CPU stores into the coherent buffer and subsequent MMIO stores on
weakly-ordered architectures such as arm64 and ppc.

> +
> +	return 0;

[ ... ]

> +static int nbl_res_intr_enable_mailbox_irq(struct nbl_resource_mgt *res_mgt,
> +					   u16 func_id, u16 vector_id,
> +					   bool enable_msix)
> +{
> +	struct nbl_interrupt_mgt *intr_mgt = res_mgt->intr_mgt;
> +	struct nbl_hw_ops *hw_ops = res_mgt->hw_ops_tbl->ops;
> +	u16 global_vec_id;
> +
> +	global_vec_id = intr_mgt->func_intr_res[func_id].interrupts[vector_id];

Is this dereference guarded anywhere?  There is no check that
func_intr_res[func_id].interrupts is non-NULL or that vector_id is less
than num_interrupts.

After destroy_msix_map() runs, interrupts is NULL; and before
configure_msix_map() has ever run for that func_id it is also NULL, so
this becomes a NULL-pointer read at offset 2 * vector_id.  When interrupts
is non-NULL but vector_id is out of range, this is an out-of-bounds slab
read whose value is then programmed into a hardware MSI-X register via
hw_ops->enable_mailbox_irq().

> +	hw_ops->enable_mailbox_irq(res_mgt->hw_ops_tbl->priv, func_id,
> +				   enable_msix, global_vec_id);
> +
> +	return 0;
> +}
> +
> +/* NBL_INTR_SET_OPS(ops_name, func)
> + *
> + * Use X Macros to reduce setup and remove codes.
> + */
> +#define NBL_INTR_OPS_TBL						\
> +do {									\
> +	NBL_INTR_SET_OPS(configure_msix_map,				\
> +			 nbl_res_intr_configure_msix_map);		\
> +	NBL_INTR_SET_OPS(destroy_msix_map,				\
> +			 nbl_res_intr_destroy_msix_map);		\
> +	NBL_INTR_SET_OPS(enable_mailbox_irq,				\
> +			 nbl_res_intr_enable_mailbox_irq);		\
> +} while (0)

This isn't a bug, but could this X-macro scheme be replaced with a plain
designated initializer on a const struct nbl_resource_ops?  The indirection
through NBL_INTR_OPS_TBL / NBL_INTR_SET_OPS and the NBL_NAME(x) helper
(whose stated purpose is "Used for macros to pass checkpatch") add no
type safety over:

	static const struct nbl_resource_ops res_ops = {
		.configure_msix_map  = nbl_res_intr_configure_msix_map,
		.destroy_msix_map    = nbl_res_intr_destroy_msix_map,
		.enable_mailbox_irq  = nbl_res_intr_enable_mailbox_irq,
		...
	};

[ ... ]

> 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 90c78aefa498..d4a1374e0dd2 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
> @@ -18,6 +18,37 @@
>  
>  struct nbl_resource_mgt;
>  
> +/* --------- INTERRUPT ---------- */
> +#define NBL_MAX_OTHER_INTERRUPT			1024
> +#define NBL_MAX_NET_INTERRUPT			4096
> +
> +struct nbl_msix_map {
> +	u16 valid:1;
> +	u16 global_msix_index:13;
> +	u16 rsv:2;
> +};

Is it safe to rely on C bitfield layout for a structure that is written
into dma_alloc_coherent memory and then consumed by the device?  The bit
ordering of bitfields within a storage unit is implementation-defined, and
GCC reverses bit ordering on big-endian targets.

The same question applies to nbl_mailbox_qinfo_map_table,
nbl_host_msix_info, nbl_pcompleter_host_msix_fid_table and
nbl_function_msix_map, which are assembled with C bitfields and then cast
to u32 * to be written into MMIO registers in nbl_hw_leonis.c.  Would
explicit FIELD_PREP / shifts-and-masks or __le* encoding be more portable?

> +
> +struct nbl_msix_map_table {
> +	struct nbl_msix_map *base_addr;
> +	dma_addr_t dma;
> +	size_t size;
> +};
> +
> +struct nbl_func_interrupt_resource_mng {
> +	u16 num_interrupts;
> +	u16 num_net_interrupts;
> +	u16 msix_base;
> +	u16 msix_max;

msix_base and msix_max do not appear to be read or written anywhere in
this patch.  Could these be added in the patch that first uses them?

> +	u16 *interrupts;
> +	struct nbl_msix_map_table msix_map_table;
> +};

[ ... ]

One last question on the commit message:

    MSI-X Interrupt Configuration:
    Dynamically allocate and manage MSI-X interrupt vectors, including
    network interrupts and other types of interrupts.
    ...
    Interrupt Information Query: Provide interfaces to obtain the
    hardware register addresses and data of interrupts.

The patch programs the device-private MSI-X mapping and info registers and
tracks index bitmaps, but there is no pci_alloc_irq_vectors(), no
request_irq(), no IRQ handler, and no "interrupt information query" getter
exposed through nbl_resource_ops -- only configure_msix_map,
destroy_msix_map and enable_mailbox_irq are added.  Could the commit
message be narrowed to what this patch actually implements?