On 20.04.2022 18:12, Dixit, Ashutosh wrote:
On Wed, 20 Apr 2022 05:17:57 -0700, Andrzej Hajda wrote:
Hi Ashutosh,
Hi Andrzej,
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")
/snip/
+void intel_gt_sysfs_unregister(struct intel_gt *gt)
+{
+ kobject_put(>->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?
Because we are explicitly doing a kobject_put() in
intel_gt_sysfs_unregister(). Which is exactly what we are *not* doing in
the previous code.
Let me explain a bit about the previous code (but feel free to skip since
the patch should speak for itself):
* Previously we kzalloc a 'struct kobj_gt'
* But we don't save a pointer to the 'struct kobj_gt' so we don't have the
pointer to the kobject to be able to do a kobject_put() on it later
* Therefore we need to store the pointer in 'struct intel_gt'
* But if we have to put the pointer in 'struct intel_gt' we might as well
put the kobject as part of 'struct intel_gt' and that also removes the
need to have a 'struct kobj_gt' (kobj_to_gt() can just use container_of()
to get gt from kobj).
* So I think this patch simpler/cleaner than the original code if you take
the requirement for kobject_put() into account.
I fully agree that previous code is incorrect but I am not convinced
current code is correct.
If some objects are kref-counted it means usually they can have multiple
concurrent users and kobject_put does not work as traditional
destructor/cleanup/unregister.
So in this particular case after calling kobject_init_and_add sysfs core
can get multiple references on the object. Later, during driver
unregistration kobject_put is called, but if the object is still in use
by sysfs core, the object will not be destroyed/released. If the driver
unregistration continues memory will be freed, leaving sysfs-core (or
other users) with dangling pointers. Unless there is some additional
synchronization mechanism I am not aware of.
Regards
Andrzej
Thanks.
--
Ashutosh
