I got some warnings from the i915 CI with the ref_tracker debugfs
patches applied, that indicated that these ref_tracker_dir_init() calls
were being called more than once. If references were held on these
objects between the initializations, then that could lead to leaked ref
tracking objects.

Since these objects are zalloc'ed, ensure that they are only initialized
once by testing whether the first byte of the name field is 0.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ++-
 drivers/gpu/drm/i915/intel_wakeref.c    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 8d9f4c410546e4144d4bc8bbc6696f3bd9498848..1b2ad1e0aef7d317f63a23b39193ea81c90401f0 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -59,7 +59,8 @@ static struct drm_i915_private *rpm_to_i915(struct intel_runtime_pm *rpm)
 
 static void init_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm)
 {
-	ref_tracker_dir_init(&rpm->debug, INTEL_REFTRACK_DEAD_COUNT, dev_name(rpm->kdev));
+	if (!rpm->debug.name[0])
+		ref_tracker_dir_init(&rpm->debug, INTEL_REFTRACK_DEAD_COUNT, dev_name(rpm->kdev));
 }
 
 static intel_wakeref_t
diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
index 07e81be4d3920febece34709c63a63204a41583c..3cfd68c98023fef75faa4dd69eba55e093130dd7 100644
--- a/drivers/gpu/drm/i915/intel_wakeref.c
+++ b/drivers/gpu/drm/i915/intel_wakeref.c
@@ -114,7 +114,8 @@ void __intel_wakeref_init(struct intel_wakeref *wf,
 			 "wakeref.work", &key->work, 0);
 
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_WAKEREF)
-	ref_tracker_dir_init(&wf->debug, INTEL_REFTRACK_DEAD_COUNT, name);
+	if (!wf->debug.name[0])
+		ref_tracker_dir_init(&wf->debug, INTEL_REFTRACK_DEAD_COUNT, name);
 #endif
 }
 

-- 
2.49.0

Hi Jeff,

> I got some warnings from the i915 CI with the ref_tracker debugfs
> patches applied, that indicated that these ref_tracker_dir_init() calls
> were being called more than once. If references were held on these
> objects between the initializations, then that could lead to leaked ref
> tracking objects.
> 
> Since these objects are zalloc'ed, ensure that they are only initialized
> once by testing whether the first byte of the name field is 0.

Are you referring to these warnings?
<3> [314.043410] debugfs: File 'intel_wakeref@ffff88815111a308' in directory 'ref_tracker' already present!
<4> [314.043427] ref_tracker: ref_tracker: unable to create debugfs file for intel_wakeref@ffff88815111a308: -EEXIST

I think those might be caused by introduction of:
"ref_tracker: automatically register a file in debugfs for a ref_tracker_dir".

Current version of "ref_tracker: add a static classname string
to each ref_tracker_dir" further in this series should prevent
multiple calls to "ref_tracker_dir_init()", so this patch could
be dropped I think.
If my reasoning is wrong however, then please add a note to the
commit message which explains why this is needed in more detail
or/and move this patch right before it is necessary. Otherwise
it looks like a vague workaround.

Best Regards,
Krzysztof

On Fri, 2025-05-30 at 11:01 +0000, Krzysztof Karas wrote:
> Hi Jeff,
> 
> > I got some warnings from the i915 CI with the ref_tracker debugfs
> > patches applied, that indicated that these ref_tracker_dir_init() calls
> > were being called more than once. If references were held on these
> > objects between the initializations, then that could lead to leaked ref
> > tracking objects.
> > 
> > Since these objects are zalloc'ed, ensure that they are only initialized
> > once by testing whether the first byte of the name field is 0.
> 
> Are you referring to these warnings?
> <3> [314.043410] debugfs: File 'intel_wakeref@ffff88815111a308' in directory 'ref_tracker' already present!
> <4> [314.043427] ref_tracker: ref_tracker: unable to create debugfs file for intel_wakeref@ffff88815111a308: -EEXIST
> 
> I think those might be caused by introduction of:
> "ref_tracker: automatically register a file in debugfs for a ref_tracker_dir".
> 
> Current version of "ref_tracker: add a static classname string
> to each ref_tracker_dir" further in this series should prevent
> multiple calls to "ref_tracker_dir_init()", so this patch could
> be dropped I think.
> If my reasoning is wrong however, then please add a note to the
> commit message which explains why this is needed in more detail
> or/and move this patch right before it is necessary. Otherwise
> it looks like a vague workaround.
> 

I'm fine with dropping this patch.

Those are the messages that demonstrate the problem, but the problem is
potentially bigger than those messages. ref_tracker_dir_init() is being
called (at least) twice:

struct ref_tracker_dir {                                              
#ifdef CONFIG_REF_TRACKER                                             
        spinlock_t              lock;                                 
        unsigned int            quarantine_avail;                     
        refcount_t              untracked;                            
        refcount_t              no_tracker;                           
        bool                    dead;                                 
        struct list_head        list; /* List of active trackers */   
        struct list_head        quarantine; /* List of dead trackers */
        const char              *class; /* object classname */        
#ifdef CONFIG_DEBUG_FS                                                
        struct dentry           *dentry;                              
        struct dentry           *symlink;                             
#endif                                                                
#endif                                                                
}; 

This structure contains two list_heads that can contain ref_tracker
objects. If that list was populated when ref_tracker_dir_init() is
called the second time, then those objects will now be sitting on a
corrupt list. At best they'll just leak, but with them sitting on a
now-corrupt list, they could cause a panic too.

It may be that there can be no objects on that list when it's called
the second time. But with this patchset initializing it twice will
cause dentry leaks at least.
-- 
Jeff Layton <jlayton@kernel.org>