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

[Simon Horman]

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@xxxxxxxxxxxxxxxxx>

> 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()?