Re: [PATCH V6 12/18] cxl/region: Add devm_cxl_pmem_add_region() for pmem region creation

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

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

On 24/02/26 06:13PM, Alejandro Lucero Palau wrote:
>Hi Neeraj,
>
>
>I could get some free time for reviewing this series. Dan pointed out 
>to potential conflicts with my Type2 series, so I'm focused on those 
>patches modifying the cxl region creation and how it is being used.
>
>
>I do not know if you are aware of the Type2 work, although I dare to 
>say you are not since some of the functionality is duplicated ... and 
>in your case skipping some steps.
>

Hi Alejandro,

Thanks for your review.
Yes I have not looked at Type2 work in detail. I will have a look
and specially the conflicting part with my changes.

>
>Below my comments.
>
>
>On 1/23/26 11:31, Neeraj Kumar wrote:
>>devm_cxl_pmem_add_region() is used to create cxl region based on region
>>information scanned from LSA.
>>
>>devm_cxl_add_region() is used to just allocate cxlr and its fields are
>>filled later by userspace tool using device attributes (*_store()).
>>
>>Inspiration for devm_cxl_pmem_add_region() is taken from these device
>>attributes (_store*) calls. It allocates cxlr and fills information
>>parsed from LSA and calls device_add(&cxlr->dev) to initiate further
>>region creation porbes
>>
>>Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>>Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>>---
>>  drivers/cxl/core/region.c | 118 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 118 insertions(+)
>>
>>diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>index 2e60e5e72551..e384eacc46ae 100644
>>--- a/drivers/cxl/core/region.c
>>+++ b/drivers/cxl/core/region.c
>>@@ -2621,6 +2621,121 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
>>  	return ERR_PTR(rc);
>>  }
>>+static ssize_t alloc_region_hpa(struct cxl_region *cxlr, u64 size)
>>+{
>>+	int rc;
>>+
>>+	if (!size)
>>+		return -EINVAL;
>>+
>>+	ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
>>+	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
>>+		return rc;
>>+
>>+	return alloc_hpa(cxlr, size);
>>+}
>
>
>Type2 has a more elaborated function preceding the final hpa 
>allocation.  First of all, the root decoder needs to match the 
>endpoint type or to have support for it. Then interleave ways taken 
>into account. I know you are only supporting 1 way, what I guess makes 
>the initial support easier, but although some check for not supporting 
>> 1 is fine (I guess this takes a lot of work for matching at least 
>LSA labels and maybe something harder), the core code should support 
>that, mainly because there is ongoing work adding it. In my case I did 
>not need interleaving and it is unlikely other Type2 devices will need 
>it in the near future, but the support is there:
>
>https://lore.kernel.org/linux-cxl/20260201155438.2664640-1-alejandro.lucero-palau@amd.com/T/#m56bf70b58bb995082680bf363fd3f6d6f2b074d2
>
>
>>+
>>+static ssize_t alloc_region_dpa(struct cxl_endpoint_decoder *cxled, u64 size)
>>+{
>>+	if (!size)
>>+		return -EINVAL;
>>+
>>+	if (!IS_ALIGNED(size, SZ_256M))
>>+		return -EINVAL;
>>+
>>+	return cxl_dpa_alloc(cxled, size);
>>+}
>
>
>I'm not sure if the same applies here as you are passing an endpoint 
>decoder already, but FWIW:
>
>
>https://lore.kernel.org/linux-cxl/20260201155438.2664640-1-alejandro.lucero-palau@amd.com/T/#m38ff266e931fd9c932bc54d000b7ea72186493be
>
>
>I'm sending type2 individual patches for facilitating the integration, 
>and I'll focus next on these two referenced ones so you could use them 
>asap.
>
>
>Thank you,
>
>Alejandro

Actually I have created alloc_region_hpa() and alloc_region_dpa() taking
inspiration from device attributes (_store*) calls used to create cxl
region using cxl userspace tool.

I am adding the support of multi interleave, there these routines will
not be required as I would be re-using the existing auto region assembly infra.

Even though I will re-check any conflicts with Type2 changes.



Regards,
Neeraj





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