[PATCH 1/2] drm: share drm_add_fake_info_node

[PATCH 1/2] drm: share drm_add_fake_info_node

[Ben Widawsky]

Both i915 and Armada had the exact same implementation. For an upcoming
patch, I'd like to call this function from two different source files in
i915, and having it available externally helps there too.

While moving, add 'debugfs_' to the name in order to match the other drm
debugfs helper functions.

Cc: linux-arm-kernel at lists.infradead.org
Cc: intel-gfx at lists.freedesktop.org
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/armada/armada_debugfs.c | 24 +-----------------------
 drivers/gpu/drm/drm_debugfs.c           | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_debugfs.c     | 32 +++-----------------------------
 include/drm/drmP.h                      |  9 +++++++++
 4 files changed, 37 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c
index 471e456..02f2978 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -107,28 +107,6 @@ static struct drm_info_list armada_debugfs_list[] = {
 };
 #define ARMADA_DEBUGFS_ENTRIES ARRAY_SIZE(armada_debugfs_list)
 
-static int drm_add_fake_info_node(struct drm_minor *minor, struct dentry *ent,
-	const void *key)
-{
-	struct drm_info_node *node;
-
-	node = kmalloc(sizeof(struct drm_info_node), GFP_KERNEL);
-	if (node == NULL) {
-		debugfs_remove(ent);
-		return -ENOMEM;
-	}
-
-	node->minor = minor;
-	node->dent = ent;
-	node->info_ent = (void *) key;
-
-	mutex_lock(&minor->debugfs_lock);
-	list_add(&node->list, &minor->debugfs_list);
-	mutex_unlock(&minor->debugfs_lock);
-
-	return 0;
-}
-
 static int armada_debugfs_create(struct dentry *root, struct drm_minor *minor,
 	const char *name, umode_t mode, const struct file_operations *fops)
 {
@@ -136,7 +114,7 @@ static int armada_debugfs_create(struct dentry *root, struct drm_minor *minor,
 
 	de = debugfs_create_file(name, mode, root, minor->dev, fops);
 
-	return drm_add_fake_info_node(minor, de, fops);
+	return drm_debugfs_add_fake_info_node(minor, de, fops);
 }
 
 int armada_drm_debugfs_init(struct drm_minor *minor)
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index b4b51d4..1edaac0 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -237,5 +237,29 @@ int drm_debugfs_cleanup(struct drm_minor *minor)
 	return 0;
 }
 
+int drm_debugfs_add_fake_info_node(struct drm_minor *minor,
+				   struct dentry *ent,
+				   const void *key)
+{
+	struct drm_info_node *node;
+
+	node = kmalloc(sizeof(*node), GFP_KERNEL);
+	if (node == NULL) {
+		debugfs_remove(ent);
+		return -ENOMEM;
+	}
+
+	node->minor = minor;
+	node->dent = ent;
+	node->info_ent = (void *) key;
+
+	mutex_lock(&minor->debugfs_lock);
+	list_add(&node->list, &minor->debugfs_list);
+	mutex_unlock(&minor->debugfs_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_debugfs_add_fake_info_node);
+
 #endif /* CONFIG_DEBUG_FS */
 
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 430eb3e..f2ef30c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -51,32 +51,6 @@ static const char *yesno(int v)
 	return v ? "yes" : "no";
 }
 
-/* As the drm_debugfs_init() routines are called before dev->dev_private is
- * allocated we need to hook into the minor for release. */
-static int
-drm_add_fake_info_node(struct drm_minor *minor,
-		       struct dentry *ent,
-		       const void *key)
-{
-	struct drm_info_node *node;
-
-	node = kmalloc(sizeof(*node), GFP_KERNEL);
-	if (node == NULL) {
-		debugfs_remove(ent);
-		return -ENOMEM;
-	}
-
-	node->minor = minor;
-	node->dent = ent;
-	node->info_ent = (void *) key;
-
-	mutex_lock(&minor->debugfs_lock);
-	list_add(&node->list, &minor->debugfs_list);
-	mutex_unlock(&minor->debugfs_lock);
-
-	return 0;
-}
-
 static int i915_capabilities(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
@@ -2148,7 +2122,7 @@ static int i915_pipe_crc_create(struct dentry *root, struct drm_minor *minor,
 	if (!ent)
 		return -ENOMEM;
 
-	return drm_add_fake_info_node(minor, ent, info);
+	return drm_debugfs_add_fake_info_node(minor, ent, info);
 }
 
 static const char * const pipe_crc_sources[] = {
@@ -3164,7 +3138,7 @@ static int i915_forcewake_create(struct dentry *root, struct drm_minor *minor)
 	if (!ent)
 		return -ENOMEM;
 
-	return drm_add_fake_info_node(minor, ent, &i915_forcewake_fops);
+	return drm_debugfs_add_fake_info_node(minor, ent, &i915_forcewake_fops);
 }
 
 static int i915_debugfs_create(struct dentry *root,
@@ -3182,7 +3156,7 @@ static int i915_debugfs_create(struct dentry *root,
 	if (!ent)
 		return -ENOMEM;
 
-	return drm_add_fake_info_node(minor, ent, fops);
+	return drm_debugfs_add_fake_info_node(minor, ent, fops);
 }
 
 static const struct drm_info_list i915_debugfs_list[] = {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 2fe9b5d..5788fb1 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1471,6 +1471,9 @@ extern int drm_debugfs_create_files(const struct drm_info_list *files,
 extern int drm_debugfs_remove_files(const struct drm_info_list *files,
 				    int count, struct drm_minor *minor);
 extern int drm_debugfs_cleanup(struct drm_minor *minor);
+extern int drm_debugfs_add_fake_info_node(struct drm_minor *minor,
+					  struct dentry *ent,
+					  const void *key);
 #else
 static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 				   struct dentry *root)
@@ -1495,6 +1498,12 @@ static inline int drm_debugfs_cleanup(struct drm_minor *minor)
 {
 	return 0;
 }
+static int drm_debugfs_add_fake_info_node(struct drm_minor *minor,
+					  struct dentry *ent,
+					  const void *key)
+{
+	return 0;
+}
 #endif
 
 				/* Info file support */
-- 
1.8.5.2

[Intel-gfx] [PATCH 1/2] drm: share drm_add_fake_info_node

[Daniel Vetter]

On Tue, Jan 14, 2014 at 06:14:06AM -0800, Ben Widawsky wrote:
> Both i915 and Armada had the exact same implementation. For an upcoming
> patch, I'd like to call this function from two different source files in
> i915, and having it available externally helps there too.
> 
> While moving, add 'debugfs_' to the name in order to match the other drm
> debugfs helper functions.
> 
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: intel-gfx at lists.freedesktop.org
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

drm_debugfs_create_files in drm_debugfs.c has the almost same code again.
Now the problem here is that the interface is a bit botched up, since all
the users in i915 and armada actaully faile to clean up teh debugfs dentry
if drm_add_fake_info_node.

So I think the right approach is to extrace a new drm_debugfs_create_node
helper which is used by i915, armada and drm_debugfs_create_files. Also I
think it's better to split this up into a drm core patch and then
i915/armada driver patches, for easier merging.
-Daniel

> ---
>  drivers/gpu/drm/armada/armada_debugfs.c | 24 +-----------------------
>  drivers/gpu/drm/drm_debugfs.c           | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_debugfs.c     | 32 +++-----------------------------
>  include/drm/drmP.h                      |  9 +++++++++
>  4 files changed, 37 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c
> index 471e456..02f2978 100644
> --- a/drivers/gpu/drm/armada/armada_debugfs.c
> +++ b/drivers/gpu/drm/armada/armada_debugfs.c
> @@ -107,28 +107,6 @@ static struct drm_info_list armada_debugfs_list[] = {
>  };
>  #define ARMADA_DEBUGFS_ENTRIES ARRAY_SIZE(armada_debugfs_list)
>  
> -static int drm_add_fake_info_node(struct drm_minor *minor, struct dentry *ent,
> -	const void *key)
> -{
> -	struct drm_info_node *node;
> -
> -	node = kmalloc(sizeof(struct drm_info_node), GFP_KERNEL);
> -	if (node == NULL) {
> -		debugfs_remove(ent);
> -		return -ENOMEM;
> -	}
> -
> -	node->minor = minor;
> -	node->dent = ent;
> -	node->info_ent = (void *) key;
> -
> -	mutex_lock(&minor->debugfs_lock);
> -	list_add(&node->list, &minor->debugfs_list);
> -	mutex_unlock(&minor->debugfs_lock);
> -
> -	return 0;
> -}
> -
>  static int armada_debugfs_create(struct dentry *root, struct drm_minor *minor,
>  	const char *name, umode_t mode, const struct file_operations *fops)
>  {
> @@ -136,7 +114,7 @@ static int armada_debugfs_create(struct dentry *root, struct drm_minor *minor,
>  
>  	de = debugfs_create_file(name, mode, root, minor->dev, fops);
>  
> -	return drm_add_fake_info_node(minor, de, fops);
> +	return drm_debugfs_add_fake_info_node(minor, de, fops);
>  }
>  
>  int armada_drm_debugfs_init(struct drm_minor *minor)
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index b4b51d4..1edaac0 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -237,5 +237,29 @@ int drm_debugfs_cleanup(struct drm_minor *minor)
>  	return 0;
>  }
>  
> +int drm_debugfs_add_fake_info_node(struct drm_minor *minor,
> +				   struct dentry *ent,
> +				   const void *key)
> +{
> +	struct drm_info_node *node;
> +
> +	node = kmalloc(sizeof(*node), GFP_KERNEL);
> +	if (node == NULL) {
> +		debugfs_remove(ent);
> +		return -ENOMEM;
> +	}
> +
> +	node->minor = minor;
> +	node->dent = ent;
> +	node->info_ent = (void *) key;
> +
> +	mutex_lock(&minor->debugfs_lock);
> +	list_add(&node->list, &minor->debugfs_list);
> +	mutex_unlock(&minor->debugfs_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_debugfs_add_fake_info_node);
> +
>  #endif /* CONFIG_DEBUG_FS */
>  
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 430eb3e..f2ef30c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -51,32 +51,6 @@ static const char *yesno(int v)
>  	return v ? "yes" : "no";
>  }
>  
> -/* As the drm_debugfs_init() routines are called before dev->dev_private is
> - * allocated we need to hook into the minor for release. */
> -static int
> -drm_add_fake_info_node(struct drm_minor *minor,
> -		       struct dentry *ent,
> -		       const void *key)
> -{
> -	struct drm_info_node *node;
> -
> -	node = kmalloc(sizeof(*node), GFP_KERNEL);
> -	if (node == NULL) {
> -		debugfs_remove(ent);
> -		return -ENOMEM;
> -	}
> -
> -	node->minor = minor;
> -	node->dent = ent;
> -	node->info_ent = (void *) key;
> -
> -	mutex_lock(&minor->debugfs_lock);
> -	list_add(&node->list, &minor->debugfs_list);
> -	mutex_unlock(&minor->debugfs_lock);
> -
> -	return 0;
> -}
> -
>  static int i915_capabilities(struct seq_file *m, void *data)
>  {
>  	struct drm_info_node *node = (struct drm_info_node *) m->private;
> @@ -2148,7 +2122,7 @@ static int i915_pipe_crc_create(struct dentry *root, struct drm_minor *minor,
>  	if (!ent)
>  		return -ENOMEM;
>  
> -	return drm_add_fake_info_node(minor, ent, info);
> +	return drm_debugfs_add_fake_info_node(minor, ent, info);
>  }
>  
>  static const char * const pipe_crc_sources[] = {
> @@ -3164,7 +3138,7 @@ static int i915_forcewake_create(struct dentry *root, struct drm_minor *minor)
>  	if (!ent)
>  		return -ENOMEM;
>  
> -	return drm_add_fake_info_node(minor, ent, &i915_forcewake_fops);
> +	return drm_debugfs_add_fake_info_node(minor, ent, &i915_forcewake_fops);
>  }
>  
>  static int i915_debugfs_create(struct dentry *root,
> @@ -3182,7 +3156,7 @@ static int i915_debugfs_create(struct dentry *root,
>  	if (!ent)
>  		return -ENOMEM;
>  
> -	return drm_add_fake_info_node(minor, ent, fops);
> +	return drm_debugfs_add_fake_info_node(minor, ent, fops);
>  }
>  
>  static const struct drm_info_list i915_debugfs_list[] = {
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 2fe9b5d..5788fb1 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1471,6 +1471,9 @@ extern int drm_debugfs_create_files(const struct drm_info_list *files,
>  extern int drm_debugfs_remove_files(const struct drm_info_list *files,
>  				    int count, struct drm_minor *minor);
>  extern int drm_debugfs_cleanup(struct drm_minor *minor);
> +extern int drm_debugfs_add_fake_info_node(struct drm_minor *minor,
> +					  struct dentry *ent,
> +					  const void *key);
>  #else
>  static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  				   struct dentry *root)
> @@ -1495,6 +1498,12 @@ static inline int drm_debugfs_cleanup(struct drm_minor *minor)
>  {
>  	return 0;
>  }
> +static int drm_debugfs_add_fake_info_node(struct drm_minor *minor,
> +					  struct dentry *ent,
> +					  const void *key)
> +{
> +	return 0;
> +}
>  #endif
>  
>  				/* Info file support */
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[Intel-gfx] [PATCH 1/2] drm: share drm_add_fake_info_node

[Russell King - ARM Linux]

On Wed, Jan 15, 2014 at 12:25:10AM +0100, Daniel Vetter wrote:
> On Tue, Jan 14, 2014 at 06:14:06AM -0800, Ben Widawsky wrote:
> > Both i915 and Armada had the exact same implementation. For an upcoming
> > patch, I'd like to call this function from two different source files in
> > i915, and having it available externally helps there too.
> > 
> > While moving, add 'debugfs_' to the name in order to match the other drm
> > debugfs helper functions.
> > 
> > Cc: linux-arm-kernel at lists.infradead.org
> > Cc: intel-gfx at lists.freedesktop.org
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> drm_debugfs_create_files in drm_debugfs.c has the almost same code again.
> Now the problem here is that the interface is a bit botched up, since all
> the users in i915 and armada actaully faile to clean up teh debugfs dentry
> if drm_add_fake_info_node.

That's not correct - armada does clean up these, I think you need to
take a closer look at the code.

We do this by setting node->info_ent to the file operations structure,
which is a unique key to the file being registered.

Upon failure to create the fake node, we appropriately call
drm_debugfs_remove_files() with the first argument being a pointer to
the file operations.  This causes drm_debugfs_remove_files() to match
the fake entry, call debugfs_remove() on the dentry, and remove the
node from the list, and free the node structure we allocated.

Upon driver teardown, we also call drm_debugfs_remove_files() but with
each fops which should be registered, thus cleaning up each fake node
which was created.

So, Armada does clean up these entries properly.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[Intel-gfx] [PATCH 1/2] drm: share drm_add_fake_info_node

[Daniel Vetter]

On Wed, Jan 15, 2014 at 12:40 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jan 15, 2014 at 12:25:10AM +0100, Daniel Vetter wrote:
>> On Tue, Jan 14, 2014 at 06:14:06AM -0800, Ben Widawsky wrote:
>> > Both i915 and Armada had the exact same implementation. For an upcoming
>> > patch, I'd like to call this function from two different source files in
>> > i915, and having it available externally helps there too.
>> >
>> > While moving, add 'debugfs_' to the name in order to match the other drm
>> > debugfs helper functions.
>> >
>> > Cc: linux-arm-kernel at lists.infradead.org
>> > Cc: intel-gfx at lists.freedesktop.org
>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>
>> drm_debugfs_create_files in drm_debugfs.c has the almost same code again.
>> Now the problem here is that the interface is a bit botched up, since all
>> the users in i915 and armada actaully faile to clean up teh debugfs dentry
>> if drm_add_fake_info_node.
>
> That's not correct - armada does clean up these, I think you need to
> take a closer look at the code.
>
> We do this by setting node->info_ent to the file operations structure,
> which is a unique key to the file being registered.
>
> Upon failure to create the fake node, we appropriately call
> drm_debugfs_remove_files() with the first argument being a pointer to
> the file operations.  This causes drm_debugfs_remove_files() to match
> the fake entry, call debugfs_remove() on the dentry, and remove the
> node from the list, and free the node structure we allocated.
>
> Upon driver teardown, we also call drm_debugfs_remove_files() but with
> each fops which should be registered, thus cleaning up each fake node
> which was created.
>
> So, Armada does clean up these entries properly.

Indeed I've missed that and it's just i915 that botches this. I still
think the helper would be saner if it cleans up all its leftovers in
the failure case.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[Intel-gfx] [PATCH 1/2] drm: share drm_add_fake_info_node

[Daniel Vetter]

On Wed, Jan 15, 2014 at 9:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jan 15, 2014 at 12:40 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Wed, Jan 15, 2014 at 12:25:10AM +0100, Daniel Vetter wrote:
>>> On Tue, Jan 14, 2014 at 06:14:06AM -0800, Ben Widawsky wrote:
>>> > Both i915 and Armada had the exact same implementation. For an upcoming
>>> > patch, I'd like to call this function from two different source files in
>>> > i915, and having it available externally helps there too.
>>> >
>>> > While moving, add 'debugfs_' to the name in order to match the other drm
>>> > debugfs helper functions.
>>> >
>>> > Cc: linux-arm-kernel at lists.infradead.org
>>> > Cc: intel-gfx at lists.freedesktop.org
>>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>>
>>> drm_debugfs_create_files in drm_debugfs.c has the almost same code again.
>>> Now the problem here is that the interface is a bit botched up, since all
>>> the users in i915 and armada actaully faile to clean up teh debugfs dentry
>>> if drm_add_fake_info_node.
>>
>> That's not correct - armada does clean up these, I think you need to
>> take a closer look at the code.
>>
>> We do this by setting node->info_ent to the file operations structure,
>> which is a unique key to the file being registered.
>>
>> Upon failure to create the fake node, we appropriately call
>> drm_debugfs_remove_files() with the first argument being a pointer to
>> the file operations.  This causes drm_debugfs_remove_files() to match
>> the fake entry, call debugfs_remove() on the dentry, and remove the
>> node from the list, and free the node structure we allocated.
>>
>> Upon driver teardown, we also call drm_debugfs_remove_files() but with
>> each fops which should be registered, thus cleaning up each fake node
>> which was created.
>>
>> So, Armada does clean up these entries properly.
>
> Indeed I've missed that and it's just i915 that botches this. I still
> think the helper would be saner if it cleans up all its leftovers in
> the failure case.

Ok, now I've actually page all the stuff back in - if
drm_add_fake_info_node fails we don't set up a drm_info_node structure
and link it anywhere. Which means drm_debugfs_remove_files won't ever
find it and hence can't possibly call debugfs_remove. Which means the
debugfs dentry is leaked. So I think the semantics of that new debugfs
helper should get fixed to also allocate and clean up the debugfs
node.

I agree that i915 is even worse since it doesn't bother to clean up
any debugfs files at all in the failure case.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[Intel-gfx] [PATCH 1/2] drm: share drm_add_fake_info_node

[Ben Widawsky]

On Wed, Jan 15, 2014 at 09:45:28AM +0100, Daniel Vetter wrote:
> On Wed, Jan 15, 2014 at 9:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Jan 15, 2014 at 12:40 AM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> >> On Wed, Jan 15, 2014 at 12:25:10AM +0100, Daniel Vetter wrote:
> >>> On Tue, Jan 14, 2014 at 06:14:06AM -0800, Ben Widawsky wrote:
> >>> > Both i915 and Armada had the exact same implementation. For an upcoming
> >>> > patch, I'd like to call this function from two different source files in
> >>> > i915, and having it available externally helps there too.
> >>> >
> >>> > While moving, add 'debugfs_' to the name in order to match the other drm
> >>> > debugfs helper functions.
> >>> >
> >>> > Cc: linux-arm-kernel at lists.infradead.org
> >>> > Cc: intel-gfx at lists.freedesktop.org
> >>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >>>
> >>> drm_debugfs_create_files in drm_debugfs.c has the almost same code again.
> >>> Now the problem here is that the interface is a bit botched up, since all
> >>> the users in i915 and armada actaully faile to clean up teh debugfs dentry
> >>> if drm_add_fake_info_node.
> >>
> >> That's not correct - armada does clean up these, I think you need to
> >> take a closer look at the code.
> >>
> >> We do this by setting node->info_ent to the file operations structure,
> >> which is a unique key to the file being registered.
> >>
> >> Upon failure to create the fake node, we appropriately call
> >> drm_debugfs_remove_files() with the first argument being a pointer to
> >> the file operations.  This causes drm_debugfs_remove_files() to match
> >> the fake entry, call debugfs_remove() on the dentry, and remove the
> >> node from the list, and free the node structure we allocated.
> >>
> >> Upon driver teardown, we also call drm_debugfs_remove_files() but with
> >> each fops which should be registered, thus cleaning up each fake node
> >> which was created.
> >>
> >> So, Armada does clean up these entries properly.
> >
> > Indeed I've missed that and it's just i915 that botches this. I still
> > think the helper would be saner if it cleans up all its leftovers in
> > the failure case.
> 
> Ok, now I've actually page all the stuff back in - if
> drm_add_fake_info_node fails we don't set up a drm_info_node structure
> and link it anywhere. Which means drm_debugfs_remove_files won't ever
> find it and hence can't possibly call debugfs_remove. Which means the
> debugfs dentry is leaked. So I think the semantics of that new debugfs
> helper should get fixed to also allocate and clean up the debugfs
> node.
> 
> I agree that i915 is even worse since it doesn't bother to clean up
> any debugfs files at all in the failure case.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Perhaps I don't understand what you want here. The only failure path in
the fake entry creation does already call debugfs_remove.

        if (node == NULL) {
                debugfs_remove(ent);
                return -ENOMEM;
        }

So long as the function succeeds, the node will be findable and removable.

-- 
Ben Widawsky, Intel Open Source Technology Center

[Intel-gfx] [PATCH 1/2] drm: share drm_add_fake_info_node

[Daniel Vetter]

On Wed, Jan 15, 2014 at 12:08:19PM -0800, Ben Widawsky wrote:
> On Wed, Jan 15, 2014 at 09:45:28AM +0100, Daniel Vetter wrote:
> > On Wed, Jan 15, 2014 at 9:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Wed, Jan 15, 2014 at 12:40 AM, Russell King - ARM Linux
> > > <linux@arm.linux.org.uk> wrote:
> > >> On Wed, Jan 15, 2014 at 12:25:10AM +0100, Daniel Vetter wrote:
> > >>> On Tue, Jan 14, 2014 at 06:14:06AM -0800, Ben Widawsky wrote:
> > >>> > Both i915 and Armada had the exact same implementation. For an upcoming
> > >>> > patch, I'd like to call this function from two different source files in
> > >>> > i915, and having it available externally helps there too.
> > >>> >
> > >>> > While moving, add 'debugfs_' to the name in order to match the other drm
> > >>> > debugfs helper functions.
> > >>> >
> > >>> > Cc: linux-arm-kernel at lists.infradead.org
> > >>> > Cc: intel-gfx at lists.freedesktop.org
> > >>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > >>>
> > >>> drm_debugfs_create_files in drm_debugfs.c has the almost same code again.
> > >>> Now the problem here is that the interface is a bit botched up, since all
> > >>> the users in i915 and armada actaully faile to clean up teh debugfs dentry
> > >>> if drm_add_fake_info_node.
> > >>
> > >> That's not correct - armada does clean up these, I think you need to
> > >> take a closer look at the code.
> > >>
> > >> We do this by setting node->info_ent to the file operations structure,
> > >> which is a unique key to the file being registered.
> > >>
> > >> Upon failure to create the fake node, we appropriately call
> > >> drm_debugfs_remove_files() with the first argument being a pointer to
> > >> the file operations.  This causes drm_debugfs_remove_files() to match
> > >> the fake entry, call debugfs_remove() on the dentry, and remove the
> > >> node from the list, and free the node structure we allocated.
> > >>
> > >> Upon driver teardown, we also call drm_debugfs_remove_files() but with
> > >> each fops which should be registered, thus cleaning up each fake node
> > >> which was created.
> > >>
> > >> So, Armada does clean up these entries properly.
> > >
> > > Indeed I've missed that and it's just i915 that botches this. I still
> > > think the helper would be saner if it cleans up all its leftovers in
> > > the failure case.
> > 
> > Ok, now I've actually page all the stuff back in - if
> > drm_add_fake_info_node fails we don't set up a drm_info_node structure
> > and link it anywhere. Which means drm_debugfs_remove_files won't ever
> > find it and hence can't possibly call debugfs_remove. Which means the
> > debugfs dentry is leaked. So I think the semantics of that new debugfs
> > helper should get fixed to also allocate and clean up the debugfs
> > node.
> > 
> > I agree that i915 is even worse since it doesn't bother to clean up
> > any debugfs files at all in the failure case.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> Perhaps I don't understand what you want here. The only failure path in
> the fake entry creation does already call debugfs_remove.
> 
>         if (node == NULL) {
>                 debugfs_remove(ent);
>                 return -ENOMEM;
>         }
> 
> So long as the function succeeds, the node will be findable and removable.

Oh dear, I didn't see that. Still stand by my opinion though that we
should shovel the debugfs_create_file into the helper - callers allocating
something and then the helper freeing it (but only if it fails) is rather
funky semantics.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[Intel-gfx] [PATCH 1/2] drm: share drm_add_fake_info_node

[Ben Widawsky]

On Thu, Jan 16, 2014 at 12:42:22AM +0100, Daniel Vetter wrote:
> On Wed, Jan 15, 2014 at 12:08:19PM -0800, Ben Widawsky wrote:
> > On Wed, Jan 15, 2014 at 09:45:28AM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 15, 2014 at 9:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > On Wed, Jan 15, 2014 at 12:40 AM, Russell King - ARM Linux
> > > > <linux@arm.linux.org.uk> wrote:
> > > >> On Wed, Jan 15, 2014 at 12:25:10AM +0100, Daniel Vetter wrote:
> > > >>> On Tue, Jan 14, 2014 at 06:14:06AM -0800, Ben Widawsky wrote:
> > > >>> > Both i915 and Armada had the exact same implementation. For an upcoming
> > > >>> > patch, I'd like to call this function from two different source files in
> > > >>> > i915, and having it available externally helps there too.
> > > >>> >
> > > >>> > While moving, add 'debugfs_' to the name in order to match the other drm
> > > >>> > debugfs helper functions.
> > > >>> >
> > > >>> > Cc: linux-arm-kernel at lists.infradead.org
> > > >>> > Cc: intel-gfx at lists.freedesktop.org
> > > >>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > >>>
> > > >>> drm_debugfs_create_files in drm_debugfs.c has the almost same code again.
> > > >>> Now the problem here is that the interface is a bit botched up, since all
> > > >>> the users in i915 and armada actaully faile to clean up teh debugfs dentry
> > > >>> if drm_add_fake_info_node.
> > > >>
> > > >> That's not correct - armada does clean up these, I think you need to
> > > >> take a closer look at the code.
> > > >>
> > > >> We do this by setting node->info_ent to the file operations structure,
> > > >> which is a unique key to the file being registered.
> > > >>
> > > >> Upon failure to create the fake node, we appropriately call
> > > >> drm_debugfs_remove_files() with the first argument being a pointer to
> > > >> the file operations.  This causes drm_debugfs_remove_files() to match
> > > >> the fake entry, call debugfs_remove() on the dentry, and remove the
> > > >> node from the list, and free the node structure we allocated.
> > > >>
> > > >> Upon driver teardown, we also call drm_debugfs_remove_files() but with
> > > >> each fops which should be registered, thus cleaning up each fake node
> > > >> which was created.
> > > >>
> > > >> So, Armada does clean up these entries properly.
> > > >
> > > > Indeed I've missed that and it's just i915 that botches this. I still
> > > > think the helper would be saner if it cleans up all its leftovers in
> > > > the failure case.
> > > 
> > > Ok, now I've actually page all the stuff back in - if
> > > drm_add_fake_info_node fails we don't set up a drm_info_node structure
> > > and link it anywhere. Which means drm_debugfs_remove_files won't ever
> > > find it and hence can't possibly call debugfs_remove. Which means the
> > > debugfs dentry is leaked. So I think the semantics of that new debugfs
> > > helper should get fixed to also allocate and clean up the debugfs
> > > node.
> > > 
> > > I agree that i915 is even worse since it doesn't bother to clean up
> > > any debugfs files at all in the failure case.
> > > -Daniel
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > 
> > Perhaps I don't understand what you want here. The only failure path in
> > the fake entry creation does already call debugfs_remove.
> > 
> >         if (node == NULL) {
> >                 debugfs_remove(ent);
> >                 return -ENOMEM;
> >         }
> > 
> > So long as the function succeeds, the node will be findable and removable.
> 
> Oh dear, I didn't see that. Still stand by my opinion though that we
> should shovel the debugfs_create_file into the helper - callers allocating
> something and then the helper freeing it (but only if it fails) is rather
> funky semantics.
> -Daniel

This actually falls apart for the one usage I originally cared about.
For CRC, we store our own info structure instead of fops as the key in
the drm debug list. Therefore, it doesn't align with the usage in ours
or other drivers. One option is to make the helper function take both a
fops as well as a key (an ugly interface if you ask me).

I've already written the patches as you wanted, so I can submit them -
it's just unfortunate that the duplication I was hoping to get rid of
will need to remain.

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 1/6] drm: Create a debugfs file creation helper

[Ben Widawsky]

DRM layer already provides a helper function to create a set of files
following a specific idiom primarily characterized by the access flags
of the file, and the file operations. This is great for writing concise
code to handle these very common cases.

This new helper function is provided to drivers that wish to change
either the access flags, or the fops for the file - in particular the
latter. This usage has become cropping up over many of the drivers.
Providing the helper is therefore here for the usual reasons.

Upcoming patches will update the callers. Currently the key is the same
as fops. This is what most callers want, the exception is when you want
to use 1 fops for multiple file nodes. I have found one case for this
(in i915), and don't feel it's a good idea to have the interface reflect
this one, currently fringe usage. In the future if more callers want to
have a separate fops and key, the interface should be extended.

v2: The original patch simply changes the way in which we wedge special
files into the normal drm node list. Daniel requested a fuller helper
function, which this patch addresses. His original claim that v1 was
buggy is unfounded.

Requested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/drm_debugfs.c | 49 +++++++++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h            | 14 +++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index b4b51d4..ea470b7 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -237,5 +237,54 @@ int drm_debugfs_cleanup(struct drm_minor *minor)
 	return 0;
 }
 
+/**
+ * Helper function for DRM drivers to create a debugfs file.
+ *
+ * \param root parent directory entry for the new file
+ * \param minor minor device minor number
+ * \param name the new file's name
+ * \param fops file operations for the new file
+ * \param mode permissions for access
+ * \return zero on success, negative errno otherwise
+ *
+ * Instantiate a debugfs file with aforementioned data. The file will be added
+ * to the drm debugfs file list in the standard way, thus making cleanup
+ * possible through the normal drm_debugfs_remove_files() call.
+ *
+ * The primary motivation for this interface over the existing one is this
+ * interface provides explicit mode, and fops flags at the cost of some extra
+ * bookkeeping to be done by the driver.
+ */
+int drm_debugfs_create_file(struct dentry *root,
+			    struct drm_minor *minor,
+			    const char *name,
+			    const struct file_operations *fops,
+			    umode_t mode)
+{
+	struct drm_info_node *node;
+	struct dentry *ent;
+
+	ent = debugfs_create_file(name, mode, root, minor->dev, fops);
+	if (!ent)
+		return PTR_ERR(ent);
+
+	node = kmalloc(sizeof(*node), GFP_KERNEL);
+	if (node == NULL) {
+		debugfs_remove(ent);
+		return -ENOMEM;
+	}
+
+	node->minor = minor;
+	node->dent = ent;
+	node->info_ent = (void *)fops;
+
+	mutex_lock(&minor->debugfs_lock);
+	list_add(&node->list, &minor->debugfs_list);
+	mutex_unlock(&minor->debugfs_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_debugfs_create_file);
+
 #endif /* CONFIG_DEBUG_FS */
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 63eab2b..b8e2baf 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1458,6 +1458,11 @@ extern struct drm_local_map *drm_getsarea(struct drm_device *dev);
 #if defined(CONFIG_DEBUG_FS)
 extern int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 			    struct dentry *root);
+extern int drm_debugfs_create_file(struct dentry *root,
+				   struct drm_minor *minor,
+				   const char *name,
+				   const struct file_operations *fops,
+				   umode_t mode);
 extern int drm_debugfs_create_files(const struct drm_info_list *files,
 				    int count, struct dentry *root,
 				    struct drm_minor *minor);
@@ -1478,6 +1483,15 @@ static inline int drm_debugfs_create_files(const struct drm_info_list *files,
 	return 0;
 }
 
+static inline int drm_debugfs_create_file(struct dentry *root,
+					  struct drm_minor *minor,
+					  const char *name,
+					  const struct file_operations *fops,
+					  umode_t mode)
+{
+	return 0;
+}
+
 static inline int drm_debugfs_remove_files(const struct drm_info_list *files,
 					   int count, struct drm_minor *minor)
 {
-- 
1.8.5.3

[PATCH 2/6] drm/armada: Use new drm debugfs file helper

[Ben Widawsky]

The debugfs helper duplicates the functionality used by Armada, so let's
just use that.

WARNING: Not even compile tested. I can compile test this if required,
but I figured there'd be other kind people who already have the cross
toolchain setup.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/armada/armada_debugfs.c | 40 ++++-----------------------------
 1 file changed, 4 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c
index 471e456..64e4361 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -107,38 +107,6 @@ static struct drm_info_list armada_debugfs_list[] = {
 };
 #define ARMADA_DEBUGFS_ENTRIES ARRAY_SIZE(armada_debugfs_list)
 
-static int drm_add_fake_info_node(struct drm_minor *minor, struct dentry *ent,
-	const void *key)
-{
-	struct drm_info_node *node;
-
-	node = kmalloc(sizeof(struct drm_info_node), GFP_KERNEL);
-	if (node == NULL) {
-		debugfs_remove(ent);
-		return -ENOMEM;
-	}
-
-	node->minor = minor;
-	node->dent = ent;
-	node->info_ent = (void *) key;
-
-	mutex_lock(&minor->debugfs_lock);
-	list_add(&node->list, &minor->debugfs_list);
-	mutex_unlock(&minor->debugfs_lock);
-
-	return 0;
-}
-
-static int armada_debugfs_create(struct dentry *root, struct drm_minor *minor,
-	const char *name, umode_t mode, const struct file_operations *fops)
-{
-	struct dentry *de;
-
-	de = debugfs_create_file(name, mode, root, minor->dev, fops);
-
-	return drm_add_fake_info_node(minor, de, fops);
-}
-
 int armada_drm_debugfs_init(struct drm_minor *minor)
 {
 	int ret;
@@ -149,13 +117,13 @@ int armada_drm_debugfs_init(struct drm_minor *minor)
 	if (ret)
 		return ret;
 
-	ret = armada_debugfs_create(minor->debugfs_root, minor,
-				   "reg", S_IFREG | S_IRUSR, &fops_reg_r);
+	ret = drm_debugfs_create_file(minor->debugfs_root, minor, "reg",
+				      S_IFREG | S_IRUSR, &fops_reg_r);
 	if (ret)
 		goto err_1;
 
-	ret = armada_debugfs_create(minor->debugfs_root, minor,
-				"reg_wr", S_IFREG | S_IWUSR, &fops_reg_w);
+	ret = drm_debugfs_create_file(minor->debugfs_root, minor,
+				      "reg_wr", S_IFREG | S_IWUSR, &fops_reg_w);
 	if (ret)
 		goto err_2;
 	return ret;
-- 
1.8.5.3