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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 19-03-2025 17:43, 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.
> 
> Fixes: 3f0e14651ab0 ("drm/xe: Runtime PM wake on every sysfs call")
> Signed-off-by: Tejas Upadhyay <tejas.upadhyay@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;
> +}
>   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);


The patch makes sense if we need to use xe_pm_runtime_get and 
xe_pm_runtime_put to show and store the attributes. Are there any 
hardware read/writes associated with these attribute store/show 
operations? If not, why do we need pm_runtime just for software states?

+Rodrigo, for his views here.

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