Re: [PATCH v20 net-next 09/10] net/nebula-matrix: add common/ctrl dev init/remove operation

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
net/nebula-matrix: add common/ctrl dev init/remove operation

This patch adds nbl_dev_setup_common_dev and nbl_dev_setup_ctrl_dev helpers,
plus matching teardown paths, and wires them into nbl_dev_init and
nbl_dev_remove.

> 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 5deb21e35f8ef..8feae561d0f13 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

[ ... ]

> @@ -38,11 +190,29 @@ int nbl_dev_init(struct nbl_adapter *adapter)
>  	dev_mgt->chan_ops_tbl = chan_ops_tbl;
>  	adapter->core.dev_mgt = dev_mgt;
>  

[Severity: High]
Does this initialize the common device components before the chip is actually
initialized?

If nbl_dev_setup_common_dev() configures queues (which accesses hardware)
before the hardware is brought up via nbl_dev_setup_ctrl_dev(), could writing
to those queue registers cause hardware faults?

> +	ret = nbl_dev_setup_common_dev(adapter);
> +	if (ret)
> +		return ret;
> +
> +	if (common->has_ctrl) {
> +		ret = nbl_dev_setup_ctrl_dev(adapter);
> +		if (ret)
> +			goto setup_ctrl_dev_fail;
> +	}
>  
>  	return 0;
> +setup_ctrl_dev_fail:
> +	nbl_dev_remove_common_dev(adapter);
> +	return ret;
>  }
>  
>  void nbl_dev_remove(struct nbl_adapter *adapter)
>  {

[Severity: High]
Are the teardown operations in the correct order here?

The comment in nbl_dev_remove_ctrl_dev() mentions that setting the driver
status to false triggers the firmware to clean up the qinfo registers.

If nbl_dev_remove_ctrl_dev() deinitializes the chip first, will the
subsequent call to nbl_dev_remove_common_dev() attempt to tear down the
queues by writing directly to those deactivated qinfo registers? 

Could this lead to PCIe master aborts or panics during driver unbind?

> +	struct nbl_common_info *common = &adapter->common;
> +
> +	if (common->has_ctrl)
> +		nbl_dev_remove_ctrl_dev(adapter);
> +	nbl_dev_remove_common_dev(adapter);
>  }
>  
>  /* ----------  Dev start process  ---------- */
-- 
This is an AI-generated review.