[PATCH] drm/xe: sysfs_ops needs to be defined on parent directory

[Rodrigo Vivi]

On Wed, Mar 19, 2025 at 05:43:49PM +0530, Tejas Upadhyay wrote:
> Currently, xe_hw_engine_sysfs_kobj_type is defining sysfs_ops
> on wrong directory. Sysfs_ops needs to be defined on immediate
> parent directory to be able to called on each attribute set/get.

Please bare with me, but I'm having a hard time to follow this
reasoning and the patch, why, and everything else going on here...

> 
> Fixes: 3f0e14651ab0 ("drm/xe: Runtime PM wake on every sysfs call")

Why this patch is claiming to fix this? Please note that this mentioned
patch just replace the default kobj_sysfs_ops per the new introduced
xe_hw_engine_class_sysfs_ops. They are basically identical functions.
The only difference is that the new one call our runtime pm wrappers
around the calls that we need.

It never touched anything that this patch is touching below.

Perhaps if we also need on the sysfs on upper directory, perhaps
we need to also replace the default sysfs_ops on other places like
in the upper directory?

> Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c | 67 +++++++++----------
>  1 file changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c b/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c
> index b53e8d2accdb..25592f178482 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c
> @@ -492,39 +492,6 @@ static const struct attribute * const files[] = {
>  	NULL
>  };
>  
> -static void kobj_xe_hw_engine_class_fini(void *arg)
> -{
> -	struct kobject *kobj = arg;
> -
> -	sysfs_remove_files(kobj, files);
> -	kobject_put(kobj);
> -}
> -
> -static struct kobj_eclass *
> -kobj_xe_hw_engine_class(struct xe_device *xe, struct kobject *parent, const char *name)
> -{
> -	struct kobj_eclass *keclass;
> -	int err = 0;
> -
> -	keclass = kzalloc(sizeof(*keclass), GFP_KERNEL);
> -	if (!keclass)
> -		return NULL;
> -
> -	kobject_init(&keclass->base, &kobj_xe_hw_engine_type);
> -	if (kobject_add(&keclass->base, parent, "%s", name)) {
> -		kobject_put(&keclass->base);
> -		return NULL;
> -	}
> -	keclass->xe = xe;
> -
> -	err = devm_add_action_or_reset(xe->drm.dev, kobj_xe_hw_engine_class_fini,
> -				       &keclass->base);
> -	if (err)
> -		return NULL;
> -
> -	return keclass;
> -}
> -
>  static void hw_engine_class_defaults_fini(void *arg)
>  {
>  	struct kobject *kobj = arg;
> @@ -611,6 +578,38 @@ static const struct kobj_type xe_hw_engine_sysfs_kobj_type = {
>  	.sysfs_ops = &xe_hw_engine_class_sysfs_ops,
>  };
>  
> +static void kobj_xe_hw_engine_class_fini(void *arg)
> +{
> +	struct kobject *kobj = arg;
> +
> +	sysfs_remove_files(kobj, files);
> +	kobject_put(kobj);
> +}
> +
> +static struct kobj_eclass *
> +kobj_xe_hw_engine_class(struct xe_device *xe, struct kobject *parent, const char *name)
> +{
> +	struct kobj_eclass *keclass;
> +	int err = 0;
> +
> +	keclass = kzalloc(sizeof(*keclass), GFP_KERNEL);
> +	if (!keclass)
> +		return NULL;
> +
> +	kobject_init(&keclass->base, &xe_hw_engine_sysfs_kobj_type);
> +	if (kobject_add(&keclass->base, parent, "%s", name)) {
> +		kobject_put(&keclass->base);
> +		return NULL;
> +	}
> +	keclass->xe = xe;
> +
> +	err = devm_add_action_or_reset(xe->drm.dev, kobj_xe_hw_engine_class_fini,
> +				       &keclass->base);
> +	if (err)
> +		return NULL;
> +
> +	return keclass;
> +}

I couldn't spot any difference from the both chunks above. So this patch is more
moving things around than doing any change right? perhaps a different patch or
a mention on the commit message itself?

>  static void hw_engine_class_sysfs_fini(void *arg)
>  {
>  	struct kobject *kobj = arg;
> @@ -640,7 +639,7 @@ int xe_hw_engine_class_sysfs_init(struct xe_gt *gt)
>  	if (!kobj)
>  		return -ENOMEM;
>  
> -	kobject_init(kobj, &xe_hw_engine_sysfs_kobj_type);
> +	kobject_init(kobj, &kobj_xe_hw_engine_type);

now it looks like this is the only real chunk of this patch, last touched
when you added it:
038ff941afe2 ("drm/xe: Add sysfs entries for engines under its GT")

And now, after this patch, who is now using xe_hw_engine_sysfs_kobj_type?

Looking further to both types, perhaps we need to kill the
kobj_xe_hw_engine_type itself in favor of the xe_ one?

Or perhaps we need something like this:

 static const struct kobj_type kobj_xe_hw_engine_type = {
        .release = kobj_xe_hw_engine_release,
-       .sysfs_ops = &kobj_sysfs_ops
+       .sysfs_ops = &xe_hw_engine_class_sysfs_ops,
 };

>  
>  	err = kobject_add(kobj, gt->sysfs, "engines");
>  	if (err)
> -- 
> 2.34.1
>