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

[Neeraj Kumar]

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@xxxxxxxxx/

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