Re: [PATCH v11 net-next 02/11] net/nebula-matrix: add our driver architecture

Simon Horman
2026-04-12T12:58:00Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net/nebula-matrix: add our driver architecture

This commit introduces a layered architecture for the nebula-matrix
driver, consisting of Device, Dispatch, Resource, Hardware, Channel, and
Common layers. It adds initialization and cleanup functions for each layer
with struct-based operation registration.

Signed-off-by: illusion.wang <illusion.wang@nebula-matrix.com>

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> index 10c3536b327b..db035136e09c 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c

[ ... ]

> +struct nbl_adapter *nbl_core_init(struct pci_dev *pdev,
> + struct nbl_init_param *param)
> +{
> + struct nbl_product_base_ops *product_base_ops;
> + struct nbl_common_info *common;
> + struct nbl_adapter *adapter;
> + int ret;
> +
> + adapter = devm_kzalloc(&pdev->dev, sizeof(*adapter), GFP_KERNEL);
> + if (!adapter)
> + return NULL;
> +
> + adapter->pdev = pdev;
> + common = &adapter->common;
> +
> + common->pdev = pdev;
> + common->dev = &pdev->dev;
> + common->has_ctrl = param->caps.has_ctrl;
> + common->has_net = param->caps.has_net;
> + common->pci_using_dac = param->pci_using_dac;
> + common->function = PCI_FUNC(pdev->devfn);
> + common->devid = PCI_SLOT(pdev->devfn);
> + common->bus = pdev->bus->number;
> + common->product_type = param->product_type;
> +
> + product_base_ops = nbl_core_setup_product_ops(adapter, param);
> +
> + /*
> + *every product's hw/chan/res layer has a great difference,
> + *so call their own init ops
> + */
> + ret = product_base_ops->hw_init(adapter);
> + if (ret)
> + goto hw_init_fail;
> +
> + ret = product_base_ops->chan_init(adapter);
> + if (ret)
> + goto chan_init_fail;
> +
> + ret = product_base_ops->res_init(adapter);
> + if (ret)
> + goto res_init_fail;
> +
> + ret = nbl_disp_init(adapter);
> + if (ret)
> + goto res_init_fail;
> +
> + ret = nbl_dev_init(adapter);
> + if (ret)
> + goto res_init_fail;

Does the error path correctly clean up all initialized layers?

The initialization order is:
1. hw_init
2. chan_init
3. res_init
4. nbl_disp_init
5. nbl_dev_init

When nbl_disp_init() or nbl_dev_init() fails, control jumps to the
'res_init_fail' label, which only tears down the channel and hardware
layers:

> +res_init_fail:
> + product_base_ops->chan_remove(adapter);
> +chan_init_fail:
> + product_base_ops->hw_remove(adapter);

However, res_init has already succeeded at this point and is never cleaned
up. Looking at nbl_product_base_ops, there is no res_remove callback
defined, and nbl_core_remove() also shows the same asymmetry:

> +void nbl_core_remove(struct nbl_adapter *adapter)
> +{
> + struct nbl_product_base_ops *product_base_ops;
> +
> + product_base_ops = adapter->product_base_ops;
> + nbl_dev_remove(adapter);
> + product_base_ops->chan_remove(adapter);
> + product_base_ops->hw_remove(adapter);
> +}

This function calls nbl_dev_remove(), chan_remove(), and hw_remove(), but
never tears down the resource layer or the dispatch layer.

Currently, all allocations in res_init (nbl_res_init_leonis) and
nbl_disp_init use devm_kzalloc, so resources are auto-freed. However, the
error path structure is concretely wrong: if res_init or disp_init ever
needs non-devm resources (as nbl_hw_init_leonis already uses ioremap and
pci_request_selected_regions), the missing cleanup will cause resource
leaks.

Should the init/remove paths be symmetric, with every successful init
having a corresponding remove in both the error cleanup path and
nbl_core_remove()?