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

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

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.

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