Re: [PATCH v20 11/22] cxl: Define a driver interface for HPA free space enumeration

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

On 11/12/25 16:10, Jonathan Cameron wrote:
> On Mon, 10 Nov 2025 15:36:46 +0000
> <alejandro.lucero-palau@amd.com> wrote:
>
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> CXL region creation involves allocating capacity from Device Physical
>> Address (DPA) and assigning it to decode a given Host Physical Address
>> (HPA). Before determining how much DPA to allocate the amount of available
>> HPA must be determined. Also, not all HPA is created equal, some HPA
>> targets RAM, some targets PMEM, some is prepared for device-memory flows
>> like HDM-D and HDM-DB, and some is HDM-H (host-only).
>>
>> In order to support Type2 CXL devices, wrap all of those concerns into
>> an API that retrieves a root decoder (platform CXL window) that fits the
>> specified constraints and the capacity available for a new region.
>>
>> Add a complementary function for releasing the reference to such root
>> decoder.
>>
>> Based on https://lore.kernel.org/linux-cxl/168592159290.1948938.13522227102445462976.stgit@dwillia2-xfh.jf.intel.com/
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Took a fresh look and I think there are some algorithm optimizations
> available that also simplify the code by getting rid of next.
>
>
>> ---
>>   drivers/cxl/core/region.c | 164 ++++++++++++++++++++++++++++++++++++++
>>   drivers/cxl/cxl.h         |   3 +
>>   include/cxl/cxl.h         |   6 ++
>>   3 files changed, 173 insertions(+)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index b06fee1978ba..99e47d261c9f 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> +static int find_max_hpa(struct device *dev, void *data)
>> +{
>> +	struct cxlrd_max_context *ctx = data;
> ...
>
>> +	for (prev = NULL; res; prev = res, res = res->sibling) {
>> +		struct resource *next = res->sibling;
>> +		resource_size_t free = 0;
>> +
>> +		/*
>> +		 * Sanity check for preventing arithmetic problems below as a
>> +		 * resource with size 0 could imply using the end field below
>> +		 * when set to unsigned zero - 1 or all f in hex.
>> +		 */
>> +		if (prev && !resource_size(prev))
>> +			continue;
>> +
>> +		if (!prev && res->start > cxlrd->res->start) {
>> +			free = res->start - cxlrd->res->start;
>> +			max = max(free, max);
>> +		}
>> +		if (prev && res->start > prev->end + 1) {
>> +			free = res->start - prev->end + 1;
>> +			max = max(free, max);
>> +		}
>> +		if (next && res->end + 1 < next->start) {
>> +			free = next->start - res->end + 1;
>> +			max = max(free, max);
>> +		}
> Why doesn't the next case happen on the following loop iteration?
> I think this if (next ... ) is checking same thing as the if (prev ...)
> of the following iteration.
>
> Given the !next only happens on final iteration you should also be bale
> to do that out side the loop which might simplify thing further...
>

I think you are right and we are doing the same thing twice, so I'll do 
that simplification.


Thank you!


>> +		if (!next && res->end + 1 < cxlrd->res->end + 1) {
>> +			free = cxlrd->res->end + 1 - res->end + 1;
>> +			max = max(free, max);
>> +		}
>> +	}
> outside the loop the final test is something like
> 	if (prev && prev->end + 1 < cxlrd->res->end + 1) {
> 		free = cxlrd->res->end + 1 - prev->end + 1;
> 		max = max(free, max);
> 	}
> as prev is now what was res above due to the final loop variable update.
>
>> +
>> +	dev_dbg(cxlrd_dev(cxlrd), "found %pa bytes of free space\n", &max);
>> +	if (max > ctx->max_hpa) {
>> +		if (ctx->cxlrd)
>> +			put_device(cxlrd_dev(ctx->cxlrd));
>> +		get_device(cxlrd_dev(cxlrd));
>> +		ctx->cxlrd = cxlrd;
>> +		ctx->max_hpa = max;
>> +	}
>> +	return 0;
>> +}
>