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

[Alejandro Lucero Palau <alucerop@amd.com>]

On 2/13/26 16:14, Gregory Price wrote:
> 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.


Hi Gregory,


Yes, it makes sense and pointing out to those changes introduced in v22 
and mainly in v23.

I'll fix it.


Regarding the below comment, which if I am not wrong comes from kreview, 
I think the patchset needs to support both cases and therefore the code 
needs to deal with both module exit paths.


Thank you


>
> ~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).