Re: [PATCH 21/22] sfc: create cxl region

[Gregory Price]

This review was generated by kreview-0811365ff2. 

This is not an automated email, the reviewer though this report
looked valid enough to consider discussion.

----
sfc code so won't speak to the internals, but ordering question seems
legit.

~Gregory
----

On Sat, Feb 01, 2026, Alejandro Lucero wrote:

> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 2cfd0a46225f..4d5f3974e51d 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c

[ ... ]

> @@ -147,11 +155,11 @@ void efx_cxl_exit(struct efx_probe_data *probe_data)
>
>  	if (probe_data->cxl->hdm_was_committed) {
>  		iounmap(probe_data->cxl->ctpio_cxl);
> -		cxl_unregister_region(probe_data->cxl->efx_region);
>  	} else {
>  		cxl_dpa_free(probe_data->cxl->cxled);
>  		cxl_put_root_decoder(probe_data->cxl->cxlrd);
>  	}
> +	cxl_unregister_region(probe_data->cxl->efx_region);

In the non-committed path, cxl_dpa_free() is called while the
endpoint decoder is still attached to the region.  Looking at
cxl_dpa_free() in drivers/cxl/core/hdm.c:

    if (cxled->cxld.region) {
        dev_dbg(dev, "decoder assigned to: %s\n",
            dev_name(&cxled->cxld.region->dev));
        return -EBUSY;
    }

Since cxl_unregister_region() has not run yet, cxled->cxld.region
is still set, and cxl_dpa_free() returns -EBUSY without freeing
the DPA.  The return value is not checked.

Should cxl_unregister_region() be called before cxl_dpa_free()
and cxl_put_root_decoder() in the else branch, matching the
reverse order of allocation in efx_cxl_init()?

The cover letter notes that v23 expects committed decoders as the
primary flow, and uncommitted decoder support is deferred to
follow-up work, so this else branch may not be reachable in
practice today.  Still worth fixing the ordering now so it
doesn't bite when the uncommitted path is enabled later.

This issue is not fixed by the remaining commits in the series
(through 10fe989f9e85).