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

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

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)
    |
    |-> <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
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.

So, I believe defining the device as a config_item (which would contain
the attributes) would be enough, no?

--
Gustavo Sousa