Re: [PATCH 6/8] drm/i915/gt: Fix memory leaks in per-gt sysfs

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 29/04/2022 20:56, Ashutosh Dixit wrote:
> All kmalloc'd kobjects need a kobject_put() to free memory. For example in
> previous code, kobj_gt_release() never gets called. The requirement of
> kobject_put() now results in a slightly different code organization.
>
> v2: s/gtn/gt/ (Andi)
>
> Cc: Andi Shyti <andi.shyti@xxxxxxxxx>
> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> Fixes: b770bcfae9ad ("drm/i915/gt: create per-tile sysfs interface")
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt.c       |  1 +
>   drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 29 ++++++++++--------------
>   drivers/gpu/drm/i915/gt/intel_gt_sysfs.h |  6 +----
>   drivers/gpu/drm/i915/gt/intel_gt_types.h |  3 +++
>   drivers/gpu/drm/i915/i915_sysfs.c        |  2 ++
>   5 files changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 92394f13b42f..9aede288eb86 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -785,6 +785,7 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
>   {
>   	intel_wakeref_t wakeref;
>   
> +	intel_gt_sysfs_unregister(gt);
>   	intel_rps_driver_unregister(&gt->rps);
>   	intel_gsc_fini(&gt->gsc);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
> index 8ec8bc660c8c..9e4ebf53379b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
> @@ -24,7 +24,7 @@ bool is_object_gt(struct kobject *kobj)
>   
>   static struct intel_gt *kobj_to_gt(struct kobject *kobj)
>   {
> -	return container_of(kobj, struct kobj_gt, base)->gt;
> +	return container_of(kobj, struct intel_gt, sysfs_gt);
>   }
>   
>   struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
> @@ -72,9 +72,9 @@ static struct attribute *id_attrs[] = {
>   };
>   ATTRIBUTE_GROUPS(id);
>   
> +/* A kobject needs a release() method even if it does nothing */
>   static void kobj_gt_release(struct kobject *kobj)
>   {
> -	kfree(kobj);
>   }
>   
>   static struct kobj_type kobj_gt_type = {
> @@ -85,8 +85,6 @@ static struct kobj_type kobj_gt_type = {
>   
>   void intel_gt_sysfs_register(struct intel_gt *gt)
>   {
> -	struct kobj_gt *kg;
> -
>   	/*
>   	 * We need to make things right with the
>   	 * ABI compatibility. The files were originally
> @@ -98,25 +96,22 @@ void intel_gt_sysfs_register(struct intel_gt *gt)
>   	if (gt_is_root(gt))
>   		intel_gt_sysfs_pm_init(gt, gt_get_parent_obj(gt));
>   
> -	kg = kzalloc(sizeof(*kg), GFP_KERNEL);
> -	if (!kg)
> +	/* init and xfer ownership to sysfs tree */
> +	if (kobject_init_and_add(&gt->sysfs_gt, &kobj_gt_type,
> +				 gt->i915->sysfs_gt, "gt%d", gt->info.id))

Was there closure/agreement on the matter of whether or not there is a 
potential race between "kfree(gt)" and sysfs access (last put from sysfs 
that is)? I've noticed Andrzej and Ashutosh were discussing it but did 
not read all the details.

Regards,

Tvrtko

>   		goto exit_fail;
>   
> -	kobject_init(&kg->base, &kobj_gt_type);
> -	kg->gt = gt;
> -
> -	/* xfer ownership to sysfs tree */
> -	if (kobject_add(&kg->base, gt->i915->sysfs_gt, "gt%d", gt->info.id))
> -		goto exit_kobj_put;
> -
> -	intel_gt_sysfs_pm_init(gt, &kg->base);
> +	intel_gt_sysfs_pm_init(gt, &gt->sysfs_gt);
>   
>   	return;
>   
> -exit_kobj_put:
> -	kobject_put(&kg->base);
> -
>   exit_fail:
> +	kobject_put(&gt->sysfs_gt);
>   	drm_warn(&gt->i915->drm,
>   		 "failed to initialize gt%d sysfs root\n", gt->info.id);
>   }
> +
> +void intel_gt_sysfs_unregister(struct intel_gt *gt)
> +{
> +	kobject_put(&gt->sysfs_gt);
> +}
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
> index 9471b26752cf..a99aa7e8b01a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
> @@ -13,11 +13,6 @@
>   
>   struct intel_gt;
>   
> -struct kobj_gt {
> -	struct kobject base;
> -	struct intel_gt *gt;
> -};
> -
>   bool is_object_gt(struct kobject *kobj);
>   
>   struct drm_i915_private *kobj_to_i915(struct kobject *kobj);
> @@ -28,6 +23,7 @@ intel_gt_create_kobj(struct intel_gt *gt,
>   		     const char *name);
>   
>   void intel_gt_sysfs_register(struct intel_gt *gt);
> +void intel_gt_sysfs_unregister(struct intel_gt *gt);
>   struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
>   					    const char *name);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index b06611c1d4ad..edd7a3cf5f5f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -224,6 +224,9 @@ struct intel_gt {
>   	} mocs;
>   
>   	struct intel_pxp pxp;
> +
> +	/* gt/gtN sysfs */
> +	struct kobject sysfs_gt;
>   };
>   
>   enum intel_gt_scratch_field {
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 8521daba212a..3f06106cdcf5 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -259,4 +259,6 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
>   
>   	device_remove_bin_file(kdev,  &dpf_attrs_1);
>   	device_remove_bin_file(kdev,  &dpf_attrs);
> +
> +	kobject_put(dev_priv->sysfs_gt);
>   }