Re: [PATCH v25 06/11] cxl/hdm: Add support for getting region from committed decoder

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

On 4/1/26 06:18, Dan Williams wrote:
> alejandro.lucero-palau@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> A Type2 device configured by the BIOS can have its HDM committed and
>> a cxl region linked by auto discovery when the device memdev is created.
>>
>> Add a function for a Type2 driver to obtain such a region.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> [..]
>> +/**
>> + * cxl_get_region_from_committed_decoder - obtain a pointer to a region
>> + * @cxlmd: CXL memdev from an endpoint device
>> + *
>> + * An accelerator decoder can be set up by the firmware/BIOS and the auto
>> + * discovery region creation triggered by the memdev object initialization.
>> + * Using this function the related driver can obtain such a region.
>> + *
>> + * Only one committed HDM is expected, returning the first one found.
>> + *
>> + * Return pointer to a region or NULL
>> + */
>> +struct cxl_region *cxl_get_region_from_committed_decoder(struct cxl_memdev *cxlmd)
>> +{
>> +	struct cxl_port *endpoint = cxlmd->endpoint;
>> +	struct cxl_endpoint_decoder *cxled;
>> +
>> +	if (!endpoint)
>> +		return NULL;
>> +
>> +	guard(rwsem_read)(&cxl_rwsem.dpa);
>> +	struct device *cxled_dev __free(put_device) =
>> +		device_find_child(&endpoint->dev, NULL,
>> +				  find_committed_endpoint_decoder);
>> +
>> +	if (!cxled_dev)
>> +		return NULL;
>> +
>> +	cxled = to_cxl_endpoint_decoder(cxled_dev);
>> +
>> +	return cxled->cxld.region;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_region_from_committed_decoder, "CXL");
> As soon as this function returns there is no guarantee that the region
> is still valid.


 From my comment on your patch trying to solve this problem:


https://lore.kernel.org/linux-cxl/20260403210050.1058650-1-dan.j.williams@intel.com/T/#m467dc88199645865a05b5fef858a67f3a608895b


I think the protection for getting the region needs to cover the use 
from the type2 driver, what is likely the ioremapping and some internal 
updates based on the success of these "atomic" actions. If the region 
disappears after that point, the same type2 driver should get an event 
for stopping that usage, what can be implemented as a callback/action 
for the cxl region device removal as done in v17.


If we agree on this, the main issue is how to implement it. I do not 
think adding specific functions or params covering the different 
possibilities (ioremap, ioremap_wc, ...) is desirable, and it will lack 
the flexibility needs for syncing things internally by the type2 driver 
against the region removal race. I wonder if we could have something 
like this:


cxl_get_locked_region_from_committed_decoder()   --> implying locking on 
cxlrd->dev, if regions does exist


cxl_release_region_lock  --> implying unlocking cxlrd->dev


  to be used by the type2 driver allowing the safe ioremap and any 
internal variable updates between them.