Re: [PATCH] drm/xe/configfs: Use config_group_put()

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Gustavo Sousa (2025-09-09 10:21:14-03:00)
>Quoting Lucas De Marchi (2025-09-09 10:01:52-03:00)
>>On Mon, Sep 08, 2025 at 11:08:52AM -0300, Gustavo Sousa wrote:
>>>Quoting Lucas De Marchi (2025-09-05 18:38:28-03:00)
>>>>On Fri, Sep 05, 2025 at 04:42:25PM -0300, Gustavo Sousa wrote:
>>>>>Quoting Lucas De Marchi (2025-09-05 13:22:37-03:00)
>>>>>>configfs has a config_group_put() helper that was adopted by
>>>>>>commit 88df7939d728 ("drm/xe/configfs: Rename struct xe_config_device").
>>>>>>Another pending work to add psmi later landed in commit
>>>>>>afe902848b41 ("drm/xe/configfs: Allow to enable PSMI") and didn't use
>>>>>>the helper.
>>>>>>
>>>>>>Use config_group_put() consistently to hide the inner workings of
>>>>>>configfs. No change in behavior since it does exactly the same thing
>>>>>>as currently being done.
>>>>>>
>>>>>>Cc: John Harrison <John.C.Harrison@Intel.com>
>>>>>>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>>
>>>>>Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>>>>
>>>>>By the way, after taking a look at how we are using configfs, I wonder
>>>>>why we are defining each "device" as a config group instead of a config
>>>>>item. Shouldn't defining them as config items suffice?
>>>>
>>>>The xe_device maps 1:1 to a group, i.e. the <configfs>/xe/<bdf>
>>>>directory. When setting/getting a file (aka item, create from the
>>>>attributes), we find the group they belong to and use that single
>>>>lock to synchronize:
>>>>
>>>>                  1:1                                1:n
>>>>        xe_device ---> configfs_group (xe/bdf) <---> configfs_item
>>>>                                `->lock
>>>
>>>Unless I missed something in the documentation for configfs, the
>>>attributes themselves are not config_items, but they are rather part of
>>>a config_item.
>>>
>>>I believe our current hierarchy is as follows:
>>>
>>>    xe (the root config_group for the "xe" subsystem)
>>
>>which corresponds to static struct configfs_subsystem xe_configfs,
>>that has xe_configfs_type as .su_group.cg_item.
>>
>>This is responsible for creating new device groups with the device bdf.
>>
>>>    |
>>>    |-> <device0> (the config_group for the "device")
>>
>>
>>>    |    |
>>>    |    |-> enable_psmi (attribute)
>>>    |    |-> engines_allowed (attribute)
>>>    |    |-> survivability_mode (attribute)
>>>    |
>>>    |-> <device1> (the config_group for the "device")
>>>    |    |
>>>    |    |-> enable_psmi (attribute)
>>>    |    |-> engines_allowed (attribute)
>>>    |    |-> survivability_mode (attribute)
>>>    |
>>>    |-> (...) (and so on)
>>>
>>>The way I see it, we are defining each "device" as a config_group, but
>>
>>why would device0 (which in our case is actually defined by the bdf)
>>not be a config_group? the rough mapping is basically: each directory is
>>a config_group. In sysfs it's possible to create a group without it
>>being a dir, but it doesn't seem that is the case in configfs.
>
>By what I read in the documentation, both config_group and config_item
>are directories in configfs:
>
>  - a config_item is supposed to represent an object that contains
>    attributes, and each attribute would be a file inside the
>    config_item's directory;
>
>  - a config_group is supposed to contain multiple config_item
>    instances.
>
>>
>>xe/0000:03:00.0 is a config group created by
>>xe_config_make_device_group(), that is called when userspace calls mkdir().
>>
>>And then we have the plain config_items inside it:
>>     |-> enable_psmi (attribute)
>>     |-> engines_allowed (attribute)
>>     |-> survivability_mode (attribute)
>
>By what I read in the documentation of configfs, those are *NOT*
>config_item instances, but rather attributes that belong to
>the config_item facet of xe/0000:03:00.0.
>
>>
>>Maybe the confusion is because there isn't a .make_item defined?
>>
>>>we are not using it to create child config_items; and we are using the
>>>group itself only as a config_item.
>>>
>>>I don't see much need for us to use config_group to represent the
>>>device, at least not today, since we don't create child config_items for
>>>it.
>>
>>not following. Maybe it's easier if you write a patch to refactor it as
>>you are saying would be  correct. From the docs:
>
>Yep... I could try something.

I was going to take a stab at this today, however I see that, with
commit c09a9933af08 ("drm/xe/pf: Add max_vfs configfs attribute to
control PF mode"), we are already adding yet another config sub-group
called "sriov".

I believe we are misusing the configfs API.  Here is the way I
understand configfs API from its documentation:

(A) A config item is a *directory*, which contains one or more
    attributes, which are represented as files in that directory.
(B) A config group is a special kind of directory that is meant to contain
    multiple config items of the same type (our "xe" subsystem is a
    valid example).
(C) It appears a config item was not designed to have a hierarchy of
    attributes.

So, I see two issues with our current implementation in Xe:

(1) We are defining <bdf> as a config group, but we don't
    implement child config items.  We are basically creating a config
    group, but only using it as a config item (i.e. adding attributes
    like enable_psmi, engines_allowed etc).

    Similarly, we are defining "sriov" as a subgroup, but then only to
    behave as a config item.

(2) Because of (1), (A) and (C), I think defining "sriov" as a subgroup
    is also some sort of misuse of the API.  I agree it would be nice to
    group attributes by topics, like we tried with "sriov", but,
    unfortunately, I don't think that's something really supported by
    configfs API today.

--
Gustavo Sousa

>
>By the way, I'm not saying that our implementation is really incorrect;
>it works.  I just think that we are defining devices as config_group
>without really using its features and that the config_item abstraction
>for them would suffice.
>
>--
>Gustavo Sousa
>
>>
>>        If the subsystem wants the child to be a group itself, the subsystem
>>        provides ct_group_ops->make_group().  Everything else behaves the same,
>>        using the group _init() functions on the group.
>>
>>That's what we are providing: a make_group that creates the bdf group
>>and has the items inside. Compare that to e.g. one that has the items in
>>their subsystem's root: kernel/crash_dump_dm_crypt.c. In that case we
>>have a make_item() defined and this is what we see in configfs:
>>
>>        # tree /sys/kernel/config/crash_dm_crypt_keys/
>>        /sys/kernel/config/crash_dm_crypt_keys/
>>        ├── count
>>        └── reuse
>>
>>Lucas De Marchi
>>
>>
>>>
>>>So, I believe defining the device as a config_item (which would contain
>>>the attributes) would be enough, no?
>>>
>>>--
>>>Gustavo Sousa