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
>
