Re: [PATCH v4 13/26] cxl: prepare memdev creation for type2

[Ben Cheatham <benjamin.cheatham@amd.com>]

On 10/18/24 5:49 AM, Alejandro Lucero Palau wrote:
> 
> On 10/17/24 22:49, Ben Cheatham wrote:
>> On 10/17/24 11:52 AM, alejandro.lucero-palau@amd.com wrote:
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device when
>>> creating a memdev leading to problems when obtaining cxl_memdev_state
>>> references from a CXL_DEVTYPE_DEVMEM type. This last device type is
>>> managed by a specific vendor driver and does not need same sysfs files
>>> since not userspace intervention is expected.
>>>
>>> Create a new cxl_mem device type with no attributes for Type2.
>>>
>> I agree with the sentiment that type 2 devices shouldn't have the same sysfs files,
>> but I think they should have *some* sysfs files. I would like to be able to see
>> these devices show up in something like "cxl list", which this patch would prevent.
>> I really think that it would be fine to only have the bare minimum though, such as
>> ram resource size/location, NUMA node, serial, etc.
> 
> 
> But this patch does not avoid all sysfs files at all, just those depending on specific type3 fields.
> 
> I can see the endpoint directory related to the accelerator cxl device, and information about the region, size, start, type, ...
> 
> Not sure if the ndctl cxl command should be modified for this kind of change, but I can see "cxl list -E" working.
> 

Sorry, I guess that's what I get for just looking at it without testing! That should be fine
then.

> 
>>> Avoid debugfs files relying on existence of clx_memdev_state.
>>>
>>> Make devm_cxl_add_memdev accesible from a accel driver.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>> ---
>>>   drivers/cxl/core/memdev.c | 15 +++++++++++++--
>>>   drivers/cxl/core/region.c |  3 ++-
>>>   drivers/cxl/mem.c         | 25 +++++++++++++++++++------
>>>   include/linux/cxl/cxl.h   |  2 ++
>>>   4 files changed, 36 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>> index 56fddb0d6a85..f168cd42f8a5 100644
>>> --- a/drivers/cxl/core/memdev.c
>>> +++ b/drivers/cxl/core/memdev.c
>>> @@ -546,9 +546,17 @@ static const struct device_type cxl_memdev_type = {
>>>       .groups = cxl_memdev_attribute_groups,
>>>   };
>>>   +static const struct device_type cxl_accel_memdev_type = {
>>> +    .name = "cxl_memdev",
>>> +    .release = cxl_memdev_release,
>>> +    .devnode = cxl_memdev_devnode,
>>> +};
>>> +
>>>   bool is_cxl_memdev(const struct device *dev)
>>>   {
>>> -    return dev->type == &cxl_memdev_type;
>>> +    return (dev->type == &cxl_memdev_type ||
>>> +        dev->type == &cxl_accel_memdev_type);
>>> +
>>>   }
>>>   EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, CXL);
>>>   @@ -659,7 +667,10 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>>>       dev->parent = cxlds->dev;
>>>       dev->bus = &cxl_bus_type;
>>>       dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>>> -    dev->type = &cxl_memdev_type;
>>> +    if (cxlds->type == CXL_DEVTYPE_DEVMEM)
>>> +        dev->type = &cxl_accel_memdev_type;
>>> +    else
>>> +        dev->type = &cxl_memdev_type;
>>>       device_set_pm_not_required(dev);
>>>       INIT_WORK(&cxlmd->detach_work, detach_memdev);
>>>   diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 21ad5f242875..7e7761ff9fc4 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -1941,7 +1941,8 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>>>           return -EINVAL;
>>>       }
>>>   -    cxl_region_perf_data_calculate(cxlr, cxled);
>>> +    if (cxlr->type == CXL_DECODER_HOSTONLYMEM)
>>> +        cxl_region_perf_data_calculate(cxlr, cxled);
>>>         if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>>>           int i;
>>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>>> index 7de232eaeb17..3a250ddeef35 100644
>>> --- a/drivers/cxl/mem.c
>>> +++ b/drivers/cxl/mem.c
>>> @@ -131,12 +131,18 @@ static int cxl_mem_probe(struct device *dev)
>>>       dentry = cxl_debugfs_create_dir(dev_name(dev));
>>>       debugfs_create_devm_seqfile(dev, "dpamem", dentry, cxl_mem_dpa_show);
>>>   -    if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
>>> -        debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
>>> -                    &cxl_poison_inject_fops);
>>> -    if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
>>> -        debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
>>> -                    &cxl_poison_clear_fops);
>>> +    /*
>>> +     * Avoid poison debugfs files for Type2 devices as they rely on
>>> +     * cxl_memdev_state.
>>> +     */
>>> +    if (mds) {
>>> +        if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
>>> +            debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
>>> +                        &cxl_poison_inject_fops);
>>> +        if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
>>> +            debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
>>> +                        &cxl_poison_clear_fops);
>>> +    }
>>>         rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
>>>       if (rc)
>>> @@ -222,6 +228,13 @@ static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n)
>>>       struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>>>       struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>>>   +    /*
>>> +     * Avoid poison sysfs files for Type2 devices as they rely on
>>> +     * cxl_memdev_state.
>>> +     */
>>> +    if (!mds)
>>> +        return 0;
>>> +
>>>       if (a == &dev_attr_trigger_poison_list.attr)
>>>           if (!test_bit(CXL_POISON_ENABLED_LIST,
>>>                     mds->poison.enabled_cmds))
>>> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h
>>> index b8ee42b38f68..bbbcf6574246 100644
>>> --- a/include/linux/cxl/cxl.h
>>> +++ b/include/linux/cxl/cxl.h
>>> @@ -57,4 +57,6 @@ int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>>>   int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>>>   int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>>>   void cxl_set_media_ready(struct cxl_dev_state *cxlds);
>>> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>>> +                       struct cxl_dev_state *cxlds);
>>>   #endif