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

[Jakub Kicinski <kuba@kernel.org>]

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

This commit modifies the initial HWRM sequence to allocate the context
memory before querying resource information. This addresses an issue where
firmware might not advertise correct resources.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_core.c b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_core.c
> @@ -73,25 +73,35 @@ static int bnge_func_qcaps(struct bnge_dev *bd)

[ ... ]

>  	return 0;
> +
> +err_free_ctx_mem:
> +	bnge_free_ctx_mem(bd);
> +	return rc;
>  }

Could this error path leave the firmware with active DMA pointers to freed
memory?

Looking at bnge_alloc_ctx_mem(), it configures the firmware with host DMA
addresses for the backing store via bnge_backing_store_cfg(). If a
subsequent step like bnge_hwrm_func_resc_qcaps() fails, this new error path
directly frees the context memory.

Previously, errors in this sequence triggered bnge_fw_unregister_dev(),
which safely unregistered the firmware before freeing the memory. There is
even a comment in bnge_fw_unregister_dev() documenting this requirement:

static void bnge_fw_unregister_dev(struct bnge_dev *bd)
{
        /* ctx mem free after unrgtr only */
        bnge_hwrm_func_drv_unrgtr(bd);
        bnge_free_ctx_mem(bd);
}

Since the firmware isn't told to stop using the memory before it is freed
here, is there a risk the hardware might DMA into the freed host pages?

> @@ -132,32 +142,25 @@ static int bnge_fw_register_dev(struct bnge_dev *bd)

[ ... ]

> -err_func_unrgtr:
> -	bnge_fw_unregister_dev(bd);
> +err_free_ctx_mem:
> +	bnge_free_ctx_mem(bd);
>  	return rc;
>  }

This error path appears to have the same regression. If
bnge_hwrm_func_drv_rgtr() fails after bnge_func_qcaps() has already
configured the backing store, freeing the context memory directly without
unregistering might allow the hardware to access freed memory.
-- 
pw-bot: cr