Re: [PATCH V3 14/20] cxl/region: Add devm_cxl_pmem_add_region() for pmem region creation

Neeraj Kumar
2025-10-06T04:55:05Z

[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]

On 23/09/25 04:50PM, Dave Jiang wrote:
>
>> +static ssize_t alloc_region_hpa(struct cxl_region *cxlr, u64 size)
>> +{
>> + int rc;
>> +
>> + ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
>> + rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem);
>> + if (rc)
>> + return rc;
>
>Just a nit. Please conform to existing style in the subsystem for this new usage.
>
> + ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
> + if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem))))
> + return rc;
>

Actually because of checkpatch.pl error, it is different from existing
style. But recent fix by Alison at [1] will allow to fix it as per others.
Sure, I will fix it in next patch-set
[1]: https://lore.kernel.org/linux-cxl/20250815010645.2980846-1-alison.schofield@intel.com/

>> +
>> + if (!size)
>> + return -EINVAL;
>> +
>> + return alloc_hpa(cxlr, size);
>> +}
>
>I think you can create another helper free_region_hpa() and call them in size_store() function to remove the duplicate code.

Sure Dave, I will fix it in next patch-set

>> +
>> +static ssize_t alloc_region_dpa(struct cxl_endpoint_decoder *cxled, u64 size)
>> +{
>> + int rc;
>> +
>> + if (!size)
>> + return -EINVAL;
>> +
>> + if (!IS_ALIGNED(size, SZ_256M))
>> + return -EINVAL;
>> +
>> + rc = cxl_dpa_free(cxled);
>> + if (rc)
>> + return rc;
>> +
>> + return cxl_dpa_alloc(cxled, size);
>> +}
>> +
>> +static struct cxl_region *
>> +devm_cxl_pmem_add_region(struct cxl_root_decoder *cxlrd, int id,
>> + enum cxl_partition_mode mode,
>
>Wouldn't this not needed since it would be CXL_PARTMODE_PMEM always? I also wonder if we need to rename devm_cxl_add_region() to devm_cxl_add_ram_region() to be explicit.
>

Yes devm_cxl_pmem_add_region() always need CXL_PARTMODE_PMEM, So I will
modify it accordingly. Also I will rename devm_cxl_add_region() to
devm_cxl_add_ram_region().

>> + enum cxl_decoder_type type,
>> + struct cxl_pmem_region_params *params,
>> + struct cxl_decoder *cxld)
>> +{
>> + struct cxl_endpoint_decoder *cxled;
>> + struct cxl_region_params *p;
>> + struct cxl_port *root_port;
>> + struct device *dev;
>> + int rc;
>> +

<snip>

>> + rc = alloc_region_dpa(cxled, params->rawsize);
>> + if (rc)
>> + return ERR_PTR(rc);
>> +
>> + /*
>> + * TODO: Currently we have support of interleave_way == 1, where
>> + * we can only have one region per mem device. It means mem device
>> + * position (params->position) will always be 0. It is therefore
>> + * attaching only one target at params->position
>> + */
>> + if (params->position)
>> + return ERR_PTR(-EINVAL);
>
>EOPNOTSUPP?

Yes, EOPNOTSUPP would be more appropriate than EINVAL. I will fix it in
next patch-set

>
>Speaking of which, are there plans to support interleave in the near future?
>
>DJ

My current focus is to get this upstreamed and after that will focus on
multi-interleave support. Multi-interleave support will require some more
efforts on top of this change. So will take it in another series.

Regards,
Neeraj

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]