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

[Upadhyay, Tejas]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Sent: Wednesday, March 26, 2025 12:43 AM
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>
> Subject: Re: [PATCH] drm/xe: sysfs_ops needs to be defined on parent
> directory
> 
> 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.

Earlier we were not doing any common ops for all attr set/get, each attribute has individual setter/getter. With 3f0e14651ab0 ("drm/xe: Runtime PM wake on every sysfs call"), we introduced common ops on all attr set/get and which is must to be called now. Thus I though of adding fixes. Please let me know if you guys think otherwise I will just remove, not strong objection.
 
> 
> 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?

The change is, immediate parent directory should be defined with attr_show/store ops if we want it to be called on every attr set/get call. Currently we had sysfs_ops defined on parent to parent directory which does not get called on underlying child's attr set/get call. 

Tejas
> 
> >  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
> >