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

[Dave Jiang <dave.jiang@intel.com>]

On 9/29/25 6:57 AM, Neeraj Kumar wrote:
> 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

Yes we should probably clean that up if it's not being used externally. I don't think it should break cxl_test, but you should compile verify after you make the changes.

make M=tools/testing/cxl

> 
> 
> Regards,
> Neeraj
> 
>