Re: [RFC PATCH 18/20] cxl/pmem: Add support of cxl lsa 2.1 support in cxl pmem

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

On 7/18/25 5:51 AM, Neeraj Kumar wrote:
> On 10/07/25 10:18AM, Dave Jiang wrote:
>>
>>
>> On 6/17/25 5:39 AM, Neeraj Kumar wrote:
>>> Add support of cxl lsa 2.1 using NDD_CXL_LABEL flag. It also creates cxl
>>> region based on region information parsed from LSA.
>>>
>>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>>> ---
>>>  drivers/cxl/pmem.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 59 insertions(+)
>>>
>>> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
>>> index ffcebb8d382f..2733d79b32d5 100644
>>> --- a/drivers/cxl/pmem.c
>>> +++ b/drivers/cxl/pmem.c
>>> @@ -58,6 +58,63 @@ static const struct attribute_group *cxl_dimm_attribute_groups[] = {
>>>      NULL
>>>  };
>>>
>>> +static int match_ep_decoder(struct device *dev, void *data)
>>> +{
>>> +    struct cxl_decoder *cxld = to_cxl_decoder(dev);
>>> +
>>> +    if (!cxld->region)
>>> +        return 1;
>>> +    else
>>> +        return 0;
>>> +}
>>
>> return !cxld->region;
>>
> 
> Thanks, I will fix it in next patch-set
> 
>>
>>> +
>>> +static struct cxl_decoder *cxl_find_free_decoder(struct cxl_port *port)
>>> +{
>>> +    struct device *dev;
>>> +
>>> +    dev = device_find_child(&port->dev, NULL, match_ep_decoder);
>>> +    if (!dev)
>>> +        return NULL;
>>> +
>>> +    return to_cxl_decoder(dev);
>>> +}
>>> +
>>> +static int create_pmem_region(struct nvdimm *nvdimm)
>>> +{
>>> +    struct cxl_nvdimm *cxl_nvd;
>>> +    struct cxl_memdev *cxlmd;
>>> +    struct cxl_nvdimm_bridge *cxl_nvb;
>>> +    struct cxl_pmem_region_params *params;
>>> +    struct cxl_root_decoder *cxlrd;
>>> +    struct cxl_decoder *cxld;
>>> +    struct cxl_region *cxlr;
>>> +
>>
>> probably need a lockdep_assert_held(&cxl_region_rwsem).
>>
> 
> Thanks Dave, Sure i will fix it with V2
> 
>>> +    if (!nvdimm)
>>> +        return -ENOTTY;
>>
>> -ENODEV?
> 
> Sure I will fix it with V2
> 
>>
>>> +
>>> +    if (!nvdimm_has_cxl_region(nvdimm))
>>> +        return 0;
>>> +
>>> +    cxl_nvd = nvdimm_provider_data(nvdimm);
>>> +    params = nvdimm_get_cxl_region_param(nvdimm);
>>> +    cxlmd = cxl_nvd->cxlmd;
>>> +    cxl_nvb = cxlmd->cxl_nvb;
>>> +    cxlrd = cxlmd->cxlrd;
>>> +
>>> +    /* FIXME: Limitation: Region creation only when interleave way == 1 */
>>> +    if (params->nlabel == 1) {
>>> +        cxld = cxl_find_free_decoder(cxlmd->endpoint);
>>> +        cxlr = cxl_create_pmem_region(cxlrd, cxld, params,
>>> +                atomic_read(&cxlrd->region_id));
>>> +        if (IS_ERR(cxlr))
>>> +            dev_dbg(&cxlmd->dev, "Region Creation failed\n");
>>
>> return PTR_ERR(cxlr); ?
>>
> 
> Thanks, I will fix it in next patch-set
> 
>>> +    } else {
>>> +        dev_dbg(&cxlmd->dev, "Region Creation is not supported with iw > 1\n");
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static int cxl_nvdimm_probe(struct device *dev)
>>>  {
>>>      struct cxl_nvdimm *cxl_nvd = to_cxl_nvdimm(dev);
>>> @@ -74,6 +131,7 @@ static int cxl_nvdimm_probe(struct device *dev)
>>>          return rc;
>>>
>>>      set_bit(NDD_LABELING, &flags);
>>> +    set_bit(NDD_CXL_LABEL, &flags);
>>
>> Ok here's the NDD_CXL_LABEL set. I think the driver should be probing the label index block and retrieve the label version and determine how to support from there instead of hard coding a flag.
> 
> Hi Dave,
> 
> We actually write label index information into LSA during namespace/region label updation.
> During that time we update its major/minor.
> 
> So during first time updation of LSA we must need some way to inform nvdimm about LSA versioning.

Gotcha. I'm not sure I like adding just a single version (flag bit) to the flags for __nvdimm_create(). Maybe we need some refactoring on the nvdimm side to allow passing in an enum for LSA versioning looking to the future. Thoughts? Also I wonder on the CXL side we need to add some checking of LSA size when retrieved from Identify Memory Device payload in order to make sure the LSA we want is supported properly by the device. 
> 
> 
> Regards,
> Neeraj
>