Re: [v4,net-next,11/11] net/nebula-matrix: add common dev start/stop operation
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-02-07 02:25:05
Also in:
linux-doc, lkml
This is an AI-generated review of your patch. The human sending this email says "This is an automated AI code reviewer, please treat what it says with a grain of salt." For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- net/nebula-matrix: add common dev start/stop operation This commit adds MSI-X interrupt initialization for the common device layer, including configuring MSI-X vector mappings, allocating interrupt resources, and registering mailbox interrupt handlers.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c index 9927dd3a1791..59d23e93e923 100644 --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -14,6 +25,179 @@ static void nbl_dev_register_common_irq(struct nbl_dev_mgt *dev_mgt) msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].num = 1; } +static int nbl_dev_request_mailbox_irq(struct nbl_dev_mgt *dev_mgt)
[ ... ]
+static int nbl_dev_configure_msix_map(struct nbl_dev_mgt *dev_mgt)
+{
+ struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
+ struct nbl_dev_common *dev_common = dev_mgt->common_dev;
+ struct nbl_msix_info *msix_info = &dev_common->msix_info;
+ bool mask_en = msix_info->serv_info[NBL_MSIX_NET_TYPE].hw_self_mask_en;
+ u16 msix_net_num = msix_info->serv_info[NBL_MSIX_NET_TYPE].num;
+ u16 msix_not_net_num = 0;
+ int err, i;
+
+ for (i = NBL_MSIX_NET_TYPE; i < NBL_MSIX_TYPE_MAX; i++)
+ msix_info->serv_info[i].base_vector_id =
+ msix_info->serv_info[i - 1].base_vector_id +
+ msix_info->serv_info[i - 1].num;^^^^^^ Does this loop cause an array underflow? NBL_MSIX_NET_TYPE equals 0 (from the enum in nbl_dev.h), so on the first iteration i=0 and the code accesses serv_info[-1]. The serv_info array is declared with NBL_MSIX_TYPE_MAX (2) elements in struct nbl_msix_info, giving valid indices [0,1]. Accessing index -1 reads uninitialized memory before the array start. Call chain: nbl_core_start()->nbl_dev_start()->nbl_dev_start_common_dev() ->nbl_dev_configure_msix_map() guarantees this occurs during device start. Should the loop start at i=1 (NBL_MSIX_MAILBOX_TYPE) instead, or should serv_info[0].base_vector_id be initialized before the loop?
+
+ for (i = NBL_MSIX_MAILBOX_TYPE; i < NBL_MSIX_TYPE_MAX; i++) {[ ... ]
+static int nbl_dev_alloc_msix_intr(struct nbl_dev_mgt *dev_mgt)
+{
+ struct nbl_dev_common *dev_common = dev_mgt->common_dev;
+ struct nbl_msix_info *msix_info = &dev_common->msix_info;
+ struct nbl_common_info *common = dev_mgt->common;
+ int needed = 0;
+ int err;
+ int i;
+
+ for (i = 0; i < NBL_MSIX_TYPE_MAX; i++)
+ needed += msix_info->serv_info[i].num;
+
+ err = pci_alloc_irq_vectors(common->pdev, needed, needed,
+ PCI_IRQ_MSIX | PCI_IRQ_AFFINITY);
+ if (err < 0) {
+ pr_err("pci_enable_msix_range failed, err = %d.\n", err);^^^^^^^^ This isn't a bug, but the error message says "pci_enable_msix_range failed" while the actual function being called is pci_alloc_irq_vectors.
+ goto enable_msix_failed; + }
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c index a16a685e4f29..55ed560624d3 100644 --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c@@ -283,7 +283,38 @@ static struct pci_driver nbl_driver = { .remove = nbl_remove, }; -module_pci_driver(nbl_driver); +static int __init nbl_module_init(void) +{ + int status; + + status = nbl_common_create_wq(); + if (status) { + pr_err("Failed to create wq, err = %d\n", status); + goto wq_create_failed; + } + status = pci_register_driver(&nbl_driver); + if (status) { + pr_err("Failed to register PCI driver, err = %d\n", status); + goto pci_register_driver_failed; + } + pr_info("nbl module loaded\n"); + return 0;
Previous review feedback from Andrew Lunn on v3 noted that the config is named 'NBL_CORE' and the module is named 'nbl_core'. He pointed out that '_CORE' naming is unusual for a driver - CORE is typically a library shared by multiple drivers. The Kconfig still shows: config NBL_CORE tristate "Nebula-matrix Ethernet Controller m18100 Family support" Should this be renamed to follow the naming convention for standalone drivers? Link: https://lore.kernel.org/netdev/88c7e349-f941-4216-89ca-70a49e82b378@lunn.ch/ (local) Andrew Lunn also noted in v3 that variable declarations should follow reverse Christmas tree ordering (longest line first). Looking at functions in this commit like nbl_dev_request_mailbox_irq(): struct nbl_dev_common *dev_common = dev_mgt->common_dev; struct nbl_msix_info *msix_info = &dev_common->msix_info; struct nbl_common_info *common = dev_mgt->common; the declarations are not in reverse Christmas tree order. Link: https://lore.kernel.org/netdev/c6d37ecd-7dc6-4f83-b76c-2735a5f116fd@lunn.ch/ (local)