Re: [PATCH V2] drm/xe/hw_engine: define sysfs_ops on all directories

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

On 27-03-2025 17:56, Tejas Upadhyay wrote:
> Sysfs_ops needs to be defined on all directories which
> can have attr files with set/get method. Add sysfs_ops
> to even those directories which is currently empty but
> would have attr files with set/get method in future.
> Leave .default with default sysfs_ops as it will never
> have setter method.
> 
> V2(Himal/Rodrigo):
>   - use single sysfs_ops for all dir and attr with set/get
>   - add default ops as ./default does not need runtime pm at all
> 
> Fixes: 3f0e14651ab0 ("drm/xe: Runtime PM wake on every sysfs call")
> Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com>

LGTM
Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

> ---
>   drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c | 108 +++++++++---------
>   1 file changed, 52 insertions(+), 56 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 e238c0e9fdd0..640950172088 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c
> @@ -32,14 +32,61 @@ bool xe_hw_engine_timeout_in_range(u64 timeout, u64 min, u64 max)
>   	return timeout >= min && timeout <= max;
>   }
>   
> -static void kobj_xe_hw_engine_release(struct kobject *kobj)
> +static void xe_hw_engine_sysfs_kobj_release(struct kobject *kobj)
>   {
>   	kfree(kobj);
>   }
>   
> +static ssize_t xe_hw_engine_class_sysfs_attr_show(struct kobject *kobj,
> +						  struct attribute *attr,
> +						  char *buf)
> +{
> +	struct xe_device *xe = kobj_to_xe(kobj);
> +	struct kobj_attribute *kattr;
> +	ssize_t ret = -EIO;
> +
> +	kattr = container_of(attr, struct kobj_attribute, attr);
> +	if (kattr->show) {
> +		xe_pm_runtime_get(xe);
> +		ret = kattr->show(kobj, kattr, buf);
> +		xe_pm_runtime_put(xe);
> +	}
> +
> +	return ret;
> +}
> +
> +static ssize_t xe_hw_engine_class_sysfs_attr_store(struct kobject *kobj,
> +						   struct attribute *attr,
> +						   const char *buf,
> +						   size_t count)
> +{
> +	struct xe_device *xe = kobj_to_xe(kobj);
> +	struct kobj_attribute *kattr;
> +	ssize_t ret = -EIO;
> +
> +	kattr = container_of(attr, struct kobj_attribute, attr);
> +	if (kattr->store) {
> +		xe_pm_runtime_get(xe);
> +		ret = kattr->store(kobj, kattr, buf, count);
> +		xe_pm_runtime_put(xe);
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct sysfs_ops xe_hw_engine_class_sysfs_ops = {
> +	.show = xe_hw_engine_class_sysfs_attr_show,
> +	.store = xe_hw_engine_class_sysfs_attr_store,
> +};
> +
>   static const struct kobj_type kobj_xe_hw_engine_type = {
> -	.release = kobj_xe_hw_engine_release,
> -	.sysfs_ops = &kobj_sysfs_ops
> +	.release = xe_hw_engine_sysfs_kobj_release,
> +	.sysfs_ops = &xe_hw_engine_class_sysfs_ops,
> +};
> +
> +static const struct kobj_type kobj_xe_hw_engine_type_def = {
> +	.release = xe_hw_engine_sysfs_kobj_release,
> +	.sysfs_ops = &kobj_sysfs_ops,
>   };
>   
>   static ssize_t job_timeout_max_store(struct kobject *kobj,
> @@ -543,7 +590,7 @@ static int xe_add_hw_engine_class_defaults(struct xe_device *xe,
>   	if (!kobj)
>   		return -ENOMEM;
>   
> -	kobject_init(kobj, &kobj_xe_hw_engine_type);
> +	kobject_init(kobj, &kobj_xe_hw_engine_type_def);
>   	err = kobject_add(kobj, parent, "%s", ".defaults");
>   	if (err)
>   		goto err_object;
> @@ -560,57 +607,6 @@ static int xe_add_hw_engine_class_defaults(struct xe_device *xe,
>   }
>   ALLOW_ERROR_INJECTION(xe_add_hw_engine_class_defaults, ERRNO); /* See xe_pci_probe() */
>   
> -static void xe_hw_engine_sysfs_kobj_release(struct kobject *kobj)
> -{
> -	kfree(kobj);
> -}
> -
> -static ssize_t xe_hw_engine_class_sysfs_attr_show(struct kobject *kobj,
> -						  struct attribute *attr,
> -						  char *buf)
> -{
> -	struct xe_device *xe = kobj_to_xe(kobj);
> -	struct kobj_attribute *kattr;
> -	ssize_t ret = -EIO;
> -
> -	kattr = container_of(attr, struct kobj_attribute, attr);
> -	if (kattr->show) {
> -		xe_pm_runtime_get(xe);
> -		ret = kattr->show(kobj, kattr, buf);
> -		xe_pm_runtime_put(xe);
> -	}
> -
> -	return ret;
> -}
> -
> -static ssize_t xe_hw_engine_class_sysfs_attr_store(struct kobject *kobj,
> -						   struct attribute *attr,
> -						   const char *buf,
> -						   size_t count)
> -{
> -	struct xe_device *xe = kobj_to_xe(kobj);
> -	struct kobj_attribute *kattr;
> -	ssize_t ret = -EIO;
> -
> -	kattr = container_of(attr, struct kobj_attribute, attr);
> -	if (kattr->store) {
> -		xe_pm_runtime_get(xe);
> -		ret = kattr->store(kobj, kattr, buf, count);
> -		xe_pm_runtime_put(xe);
> -	}
> -
> -	return ret;
> -}
> -
> -static const struct sysfs_ops xe_hw_engine_class_sysfs_ops = {
> -	.show = xe_hw_engine_class_sysfs_attr_show,
> -	.store = xe_hw_engine_class_sysfs_attr_store,
> -};
> -
> -static const struct kobj_type xe_hw_engine_sysfs_kobj_type = {
> -	.release = xe_hw_engine_sysfs_kobj_release,
> -	.sysfs_ops = &xe_hw_engine_class_sysfs_ops,
> -};
>   
>   static void hw_engine_class_sysfs_fini(void *arg)
>   {
> @@ -641,7 +637,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);
>   
>   	err = kobject_add(kobj, gt->sysfs, "engines");
>   	if (err)