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

Alejandro Lucero Palau
2025-11-19T17:16:20Z

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;
>> +}
>