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

[Neeraj Kumar <s.neeraj@samsung.com>]

[-- 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 --]