Re: [PATCH V3 18/20] cxl/pmem_region: Prep patch to accommodate pmem_region attributes

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

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

On 24/09/25 11:53AM, Dave Jiang wrote:
>
>
>On 9/17/25 6:41 AM, Neeraj Kumar wrote:
>> Created a separate file core/pmem_region.c along with CONFIG_PMEM_REGION
>> Moved pmem_region related code from core/region.c to core/pmem_region.c
>> For region label update, need to create device attribute, which calls
>> nvdimm exported function thus making pmem_region dependent on libnvdimm.
>> Because of this dependency of pmem region on libnvdimm, segregated pmem
>> region related code from core/region.c
>
>We can minimize the churn in this patch by introduce the new core/pmem_region.c and related bits in the beginning instead of introduce new functions and then move them over from region.c.

Hi Dave,

As per LSA 2.1, during region creation we need to intract with nvdimmm
driver to write region label into LSA.
This dependency of libnvdimm is only for PMEM region, therefore I have
created a seperate file core/pmem_region.c and copied pmem related functions
present in core/region.c into core/pmem_region.c.
Because of this movemement of code we have churn introduced in this patch.
Can you please suggest optimized way to handle dependency on libnvdimm
with minimum code changes.

>
>> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
>> index 48b7314afdb8..532eaa1bbdd6 100644
>> --- a/drivers/cxl/Kconfig
>> +++ b/drivers/cxl/Kconfig
>> @@ -211,6 +211,20 @@ config CXL_REGION
>>
>>  	  If unsure say 'y'
>>
>> +config CXL_PMEM_REGION
>> +	bool "CXL: Pmem Region Support"
>> +	default CXL_BUS
>> +	depends on CXL_REGION
>
>> +	depends on PHYS_ADDR_T_64BIT
>> +	depends on BLK_DEV
>These 2 deps are odd. What are the actual dependencies?
>

We need to add these 2 deps to fix v2 0Day issue [1]
I have taken reference from bdf97013ced5f [2]
Seems, I also have to add depends on ARCH_HAS_PMEM_API. I will update it
in V3.

[1] https://lore.kernel.org/linux-cxl/202507311017.7ApKmtQc-lkp@intel.com/
[2] https://elixir.bootlin.com/linux/v6.13.7/source/drivers/acpi/nfit/Kconfig#L4

>
>> +	select LIBNVDIMM
>> +	help
>> +	  Enable the CXL core to enumerate and provision CXL pmem regions.
>> +	  A CXL pmem region need to update region label into LSA. For LSA
>> +	  updation/deletion libnvdimm is required.
>
>s/updation/update/
>

Sure, Will fix it

>> +
>> +	  If unsure say 'y'
>> +
>>  config CXL_REGION_INVALIDATION_TEST
>>  	bool "CXL: Region Cache Management Bypass (TEST)"
>>  	depends on CXL_REGION

<snip>

>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -53,7 +53,7 @@ static int cxl_device_id(const struct device *dev)
>>  		return CXL_DEVICE_NVDIMM_BRIDGE;
>>  	if (dev->type == &cxl_nvdimm_type)
>>  		return CXL_DEVICE_NVDIMM;
>> -	if (dev->type == CXL_PMEM_REGION_TYPE())
>> +	if (dev->type == CXL_PMEM_REGION_TYPE)
>
>Stray edit? I don't think anything changed in the declaration.
>

Sure, Will fix it

>>  		return CXL_DEVICE_PMEM_REGION;
>>  	if (dev->type == CXL_DAX_REGION_TYPE())
>>  		return CXL_DEVICE_DAX_REGION;

<snip>

>> @@ -2382,7 +2380,7 @@ bool is_cxl_region(struct device *dev)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(is_cxl_region, "CXL");
>>
>> -static struct cxl_region *to_cxl_region(struct device *dev)
>> +struct cxl_region *to_cxl_region(struct device *dev)
>>  {
>>  	if (dev_WARN_ONCE(dev, dev->type != &cxl_region_type,
>>  			  "not a cxl_region device\n"))
>> @@ -2390,6 +2388,7 @@ static struct cxl_region *to_cxl_region(struct device *dev)
>>
>>  	return container_of(dev, struct cxl_region, dev);
>>  }
>> +EXPORT_SYMBOL_NS_GPL(to_cxl_region, "CXL");
>
>Maybe just move this into the header file instead.
>
>DJ

Actually to_cxl_region() is internal to cxl/core and especially to core/region.c
So, Its better to compeletly remove EXPORT_SYMBOL_NS_GPL(to_cxl_region, "CXL")

Even EXPORT_SYMBOL_NS_GPL(is_cxl_region, "CXL") is internal to cxl/core/region.c
Should I also remove it?

Even we can remove declaration of is_cxl_region() and to_cxl_region()
from drivers/cxl/cxl.h as these functions are internal to cxl/core/region.c


Regards,
Neeraj


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