| Message ID | 20220427203833.44449-1-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Fix memory leaks in per-gt sysfs | expand |
On Wed, 27 Apr 2022 15:45:40 -0700, Patchwork wrote: > > Possible regressions > > * igt@kms_flip@flip-vs-suspend-interruptible@a-edp1: > > * shard-skl: PASS -> INCOMPLETE > > * igt@syncobj_timeline@wait-all-for-submit-snapshot: > > * shard-skl: PASS -> FAIL > > Warnings > > * igt@gem_eio@unwedge-stress: > > * shard-tglb: FAIL (i915#232) -> FAIL +1 similar issue These failures are unrelated, the patch is related only to per-gt sysfs.
Addressed the failure and re-reported. Re-opened the issue https://gitlab.freedesktop.org/drm/intel/-/issues/5196 igt@syncobj_timeline@wait-all-for-submit-snapshot - fail - Failed assertion: __syncobj_timeline_wait_ioctl(wait->fd, &wait->wait) == 0, error: -62 != 0 Filed a new issue https://gitlab.freedesktop.org/drm/intel/-/issues/5864 igt@kms_flip@flip-vs-suspend-interruptible@a-edp1 - incomplete - PM: suspend entry (deep) Thanks, Lakshmi. -----Original Message----- From: Dixit, Ashutosh <ashutosh.dixit@intel.com> Sent: Wednesday, April 27, 2022 5:42 PM To: intel-gfx@lists.freedesktop.org; Vudum, Lakshminarayana <lakshminarayana.vudum@intel.com> Subject: Re: ✗ Fi.CI.IGT: failure for drm/i915/gt: Fix memory leaks in per-gt sysfs On Wed, 27 Apr 2022 15:45:40 -0700, Patchwork wrote: > > Possible regressions > > * igt@kms_flip@flip-vs-suspend-interruptible@a-edp1: > > * shard-skl: PASS -> INCOMPLETE > > * igt@syncobj_timeline@wait-all-for-submit-snapshot: > > * shard-skl: PASS -> FAIL > > Warnings > > * igt@gem_eio@unwedge-stress: > > * shard-tglb: FAIL (i915#232) -> FAIL +1 similar issue These failures are unrelated, the patch is related only to per-gt sysfs.
Hi Ashutosh, On Wed, Apr 27, 2022 at 01:38:33PM -0700, 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@intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Fixes: b770bcfae9ad ("drm/i915/gt: create per-tile sysfs interface") > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> I'm still not convinced this patch is 100% correct, but I think it's better thank what it was and it's not addressing Andrzej's comment. As of now, though, I'm not able to think of a better way of doing it and if Andrzej doesn't have a better solution I would just tag it: Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> As soon as I will have some time I will test it to proof Andrzej's concern. Thanks for taking care of this, Andi
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(>->rps); intel_gsc_fini(>->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(>->sysfs_gt, &kobj_gt_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, >->sysfs_gt); return; -exit_kobj_put: - kobject_put(&kg->base); - exit_fail: + kobject_put(>->sysfs_gt); drm_warn(>->i915->drm, "failed to initialize gt%d sysfs root\n", gt->info.id); } + +void intel_gt_sysfs_unregister(struct intel_gt *gt) +{ + kobject_put(>->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); }
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@intel.com> Cc: Andrzej Hajda <andrzej.hajda@intel.com> Fixes: b770bcfae9ad ("drm/i915/gt: create per-tile sysfs interface") Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- 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(-)