Re: [v4,net-next,11/11] net/nebula-matrix: add common dev start/stop operation

[Jakub Kicinski <kuba@kernel.org>]

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.

> 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

[ ... ]

> @@ -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;
> +	}

[ ... ]

> 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/


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/