Re: [PATCH net v3 1/2] bnge: fix initial HWRM sequence

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

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
---
bnge: fix initial HWRM sequence

Firmware may not advertize correct resources if backing store is not
enabled before resource information is queried.
Fix the initial sequence of HWRMs so that driver gets capabilities
and resource information correctly.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_core.c b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> index 1c14c5fe8d613..68b74eb2c3a29 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
[ ... ]
> @@ -133,23 +140,28 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)
>  
>  	bnge_hwrm_fw_set_time(bd);
>  
> -	rc =  bnge_hwrm_func_drv_rgtr(bd);
> +	/* Get the resources and configuration from firmware */
> +	rc = bnge_func_qcaps(bd);
>  	if (rc) {
> -		dev_err(bd->dev, "Failed to rgtr with firmware rc: %d\n", rc);
> +		dev_err(bd->dev, "Failed querying caps rc: %d\n", rc);
>  		return rc;
>  	}
>  
>  	rc = bnge_alloc_ctx_mem(bd);
>  	if (rc) {
>  		dev_err(bd->dev, "Failed to allocate ctx mem rc: %d\n", rc);
> -		goto err_func_unrgtr;
> +		goto err_free_ctx_mem;
>  	}
>  
> -	/* Get the resources and configuration from firmware */
> -	rc = bnge_func_qcaps(bd);
> +	rc = bnge_hwrm_func_drv_rgtr(bd);
>  	if (rc) {
> -		dev_err(bd->dev, "Failed initial configuration rc: %d\n", rc);
> -		rc = -ENODEV;
> +		dev_err(bd->dev, "Failed to rgtr with firmware rc: %d\n", rc);
> +		goto err_free_ctx_mem;

Does this regression leave the hardware with active DMA pointers to freed
memory?

If bnge_alloc_ctx_mem() allocates host DMA memory and passes the addresses
to the firmware via bnge_hwrm_func_backing_store(), and then
bnge_hwrm_func_drv_rgtr() fails, the code jumps directly to err_free_ctx_mem.

> +	}
> +
> +	rc = bnge_func_qrcaps_qcfg(bd);
> +	if (rc) {
> +		dev_err(bd->dev, "Failed querying resources rc: %d\n", rc);
>  		goto err_func_unrgtr;
>  	}
>  
> @@ -158,7 +170,9 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)
>  	return 0;
>  
>  err_func_unrgtr:
> -	bnge_fw_unregister_dev(bd);
> +	bnge_hwrm_func_drv_unrgtr(bd);
> +err_free_ctx_mem:
> +	bnge_free_ctx_mem(bd);

Since the err_func_unrgtr label is bypassed, is the firmware ever told to
drop the backing store configuration before the host frees the memory in
bnge_free_ctx_mem()?

In the pre-patch sequence, the driver was registered first, so any failure
subsequently triggered a driver unregister, which implicitly told the
firmware to drop the backing store configuration. With the new order, does
the firmware maintain active DMA pointers to the freed host memory?