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

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

On 11/7/25 5:49 AM, Neeraj Kumar wrote:
> On 06/10/25 09:06AM, Dave Jiang wrote:
>>
>>
>> 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.
>>
>> Hmm....maybe relegate the introduction of core/pmem_region.c new file and only the moving of the existing bits into the new file to a patch. And then your patch will be rid of the delete/add bits of the old code? Would that work?
>>
>> DJ
> 
> 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
> 
> I have moved following 7 pmem related functions from core/region.c to core/pmem_region.c
>  - cxl_pmem_region_release()
>  - cxl_pmem_region_alloc()
>  - cxlr_release_nvdimm()
>  - cxlr_pmem_unregister()
>  - devm_cxl_add_pmem_region()
>  - is_cxl_pmem_region()
>  - to_cxl_pmem_region()
> 
> I have created region_label_update_show/store() and region_label_delete_store() which
> internally calls following libnvdimm exported function
>  - region_label_update_show/store()
>  - region_label_delete_store()
> 
> I have added above attributes as following
>    {{{
>     static struct attribute *cxl_pmem_region_attrs[] = {
>             &dev_attr_region_label_update.attr,
>             &dev_attr_region_label_delete.attr,
>             NULL
>     };
>     static struct attribute_group cxl_pmem_region_group = {
>             .attrs = cxl_pmem_region_attrs,
>     };
>     static const struct attribute_group *cxl_pmem_region_attribute_groups[] = {
>             &cxl_base_attribute_group,
>             &cxl_pmem_region_group,      ------> New addition in this patch
>             NULL
>     };
>     const struct device_type cxl_pmem_region_type = {
>             .name = "cxl_pmem_region",
>             .release = cxl_pmem_region_release,
>             .groups = cxl_pmem_region_attribute_groups,
>     };
> 
>     static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
>     {
>             <snip>
>             dev = &cxlr_pmem->dev;
>             dev->parent = &cxlr->dev;
>             dev->bus = &cxl_bus_type;
>             dev->type = &cxl_pmem_region_type;
>         <snip>
>     }
>    }}}
> 
> So I mean to say all above mentioned functions are inter-connected and dependent on libnvdimm
> Keeping any of them in core/region.c to avoid churn, throws following linking error
>    {{{
>     ERROR: modpost: "nd_region_label_delete" [drivers/cxl/core/cxl_core.ko] undefined!
>     ERROR: modpost: "nd_region_label_update" [drivers/cxl/core/cxl_core.ko] undefined!
>     make[2]: *** [scripts/Makefile.modpost:147: Module.symvers] Error 1
>    }}}
> 
> Keeping these functions in core/region.c (CONFIG_REGION)) and manually enabling CONFIG_LIBNVDIMM=y
> will make it pass.
> 
> Even if we can put these functions in core/region.c and forcefully make
> libnvdimm (CONFIG_LIBNVDIMM) dependent using Kconfig. But I find it little improper as
> this new dependency is not for all type of cxl devices (vmem and pmem) but only for cxl pmem
> device region.
> 
> Therefore I have seperated it out in core/pmem_region.c under Kconfig control
> making libnvdimm forcefully enable if CONFIG_CXL_PMEM_REGION == y
> 
> So, I believe this prep patch is required for this LSA 2.1 support.

I think you misunderstood what I said. What I was trying to say is if possible to move all the diff changes of moving the existing code to a different place to a different patch. That way this patch is not full of those diff changes and make it easier to review.

DJ


> 
> 
> Regards,
> Neeraj
>