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

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

Hi Ashutosh,

On 20.04.2022 07:21, 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.
>
> Cc: Andi Shyti <andi.shyti@xxxxxxxxx>
> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@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 | 35 ++++++++++--------------
>   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, 22 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index f0014c5072c9..f0c56ca12c0b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -783,6 +783,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_pxp_fini(&gt->pxp);
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c
> index 8ec8bc660c8c..6f1b081ca5b7 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_gtn);
>   }
>   
>   struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
> @@ -72,21 +72,19 @@ static struct attribute *id_attrs[] = {
>   };
>   ATTRIBUTE_GROUPS(id);
>   
> -static void kobj_gt_release(struct kobject *kobj)
> +/* A kobject needs a release() method even if it does nothing */
> +static void kobj_gtn_release(struct kobject *kobj)
>   {
> -	kfree(kobj);
>   }
>   
> -static struct kobj_type kobj_gt_type = {
> -	.release = kobj_gt_release,
> +static struct kobj_type kobj_gtn_type = {
> +	.release = kobj_gtn_release,
>   	.sysfs_ops = &kobj_sysfs_ops,
>   	.default_groups = id_groups,
>   };
>   
>   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_gtn, &kobj_gtn_type,
> +				 gt->i915->sysfs_gt, "gt%d", gt->info.id))
>   		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_gtn);
>   
>   	return;
>   
> -exit_kobj_put:
> -	kobject_put(&kg->base);
> -
>   exit_fail:
> +	kobject_put(&gt->sysfs_gtn);
>   	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_gtn);
> +}
> 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 937b2e1a305e..4c72b4f983a6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -222,6 +222,9 @@ struct intel_gt {
>   	} mocs;
>   
>   	struct intel_pxp pxp;
> +
> +	/* gt/gtN sysfs */
> +	struct kobject sysfs_gtn;

If you put kobject as a part of intel_gt what assures you that lifetime 
of kobject is shorter than intel_gt? Ie its refcounter is 0 on removal 
of intel_gt?

Regards
Andrzej

>   };
>   
>   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);
>   }