lkml.org 
[lkml]   [2026]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH v13 net-next 07/11] net/nebula-matrix: add intr resource implementation
Date
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?

\
 
 \ /
  Last update: 2026-05-01 01:49    [W:0.220 / U:73.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and my Meterkast|Read the blog