[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@lists.infradead.org
Cc: intel-gfx@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

Re: [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@lists.infradead.org
> Cc: intel-gfx@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@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

Re: [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@lists.infradead.org
> > Cc: intel-gfx@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".

Re: [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@lists.infradead.org
>> > Cc: intel-gfx@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

Re: [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@lists.infradead.org
>>> > Cc: intel-gfx@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

Re: [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@lists.infradead.org
> >>> > Cc: intel-gfx@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

Re: [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@lists.infradead.org
> > >>> > Cc: intel-gfx@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

Re: [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@lists.infradead.org
> > > >>> > Cc: intel-gfx@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

[PATCH 3/6] drm/radeon: Use new drm debugfs file helper

[Ben Widawsky]

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

WARNING: only compile tested

Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/radeon/radeon.h     |  5 -----
 drivers/gpu/drm/radeon/radeon_ttm.c | 43 ++++++++++++++++++++++---------------
 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index c5519ca..ceec468 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -418,11 +418,6 @@ struct radeon_mman {
 	struct ttm_bo_device		bdev;
 	bool				mem_global_referenced;
 	bool				initialized;
-
-#if defined(CONFIG_DEBUG_FS)
-	struct dentry			*vram;
-	struct dentry			*gtt;
-#endif
 };
 
 /* bo virtual address in a specific vm */
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 77f5b0c..cf7caf2 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -971,27 +971,34 @@ static const struct file_operations radeon_ttm_gtt_fops = {
 	.llseek = default_llseek
 };
 
+static const struct radeon_debugfs_files {
+	const char *name;
+	const struct file_operations *fops;
+} radeon_debugfs_files[] = {
+	{"radeon_vram", &radeon_ttm_vram_fops},
+	{"radeon_gtt", &radeon_ttm_gtt_fops},
+};
 #endif
 
 static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
 {
 #if defined(CONFIG_DEBUG_FS)
 	unsigned count;
+	int i, ret;
 
 	struct drm_minor *minor = rdev->ddev->primary;
-	struct dentry *ent, *root = minor->debugfs_root;
-
-	ent = debugfs_create_file("radeon_vram", S_IFREG | S_IRUGO, root,
-				  rdev, &radeon_ttm_vram_fops);
-	if (IS_ERR(ent))
-		return PTR_ERR(ent);
-	rdev->mman.vram = ent;
-
-	ent = debugfs_create_file("radeon_gtt", S_IFREG | S_IRUGO, root,
-				  rdev, &radeon_ttm_gtt_fops);
-	if (IS_ERR(ent))
-		return PTR_ERR(ent);
-	rdev->mman.gtt = ent;
+	struct dentry *root = minor->debugfs_root;
+
+	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
+		ret = drm_debugfs_create_file(root, minor,
+					      radeon_debugfs_files[i].name,
+					      radeon_debugfs_files[i].fops,
+					      S_IFREG | S_IWUSR);
+		if (ret) {
+			radeon_ttm_debugfs_fini(rdev);
+			return ret;
+		}
+	}
 
 	count = ARRAY_SIZE(radeon_ttm_debugfs_list);
 
@@ -1010,11 +1017,13 @@ static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
 static void radeon_ttm_debugfs_fini(struct radeon_device *rdev)
 {
 #if defined(CONFIG_DEBUG_FS)
+	int i;
 
-	debugfs_remove(rdev->mman.vram);
-	rdev->mman.vram = NULL;
+	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
+		struct drm_info_list *info_list =
+			(struct drm_info_list *)&radeon_debugfs_files[i];
 
-	debugfs_remove(rdev->mman.gtt);
-	rdev->mman.gtt = NULL;
+		drm_debugfs_remove_files(info_list, 1, rdev->ddev->primary);
+	}
 #endif
 }
-- 
1.8.5.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm/radeon: Use new drm debugfs file helper

[Christian König]

Am 21.01.2014 21:33, schrieb Ben Widawsky:
> The debugfs helper duplicates the functionality used by Armada, so let's
> just use that.
>
> WARNING: only compile tested
>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>   drivers/gpu/drm/radeon/radeon.h     |  5 -----
>   drivers/gpu/drm/radeon/radeon_ttm.c | 43 ++++++++++++++++++++++---------------
>   2 files changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index c5519ca..ceec468 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -418,11 +418,6 @@ struct radeon_mman {
>   	struct ttm_bo_device		bdev;
>   	bool				mem_global_referenced;
>   	bool				initialized;
> -
> -#if defined(CONFIG_DEBUG_FS)
> -	struct dentry			*vram;
> -	struct dentry			*gtt;
> -#endif
>   };
>   
>   /* bo virtual address in a specific vm */
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 77f5b0c..cf7caf2 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -971,27 +971,34 @@ static const struct file_operations radeon_ttm_gtt_fops = {
>   	.llseek = default_llseek
>   };
>   
> +static const struct radeon_debugfs_files {
> +	const char *name;
> +	const struct file_operations *fops;
> +} radeon_debugfs_files[] = {
> +	{"radeon_vram", &radeon_ttm_vram_fops},
> +	{"radeon_gtt", &radeon_ttm_gtt_fops},
> +};
>   #endif
>   
>   static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
>   {
>   #if defined(CONFIG_DEBUG_FS)
>   	unsigned count;
> +	int i, ret;
>   
>   	struct drm_minor *minor = rdev->ddev->primary;
> -	struct dentry *ent, *root = minor->debugfs_root;
> -
> -	ent = debugfs_create_file("radeon_vram", S_IFREG | S_IRUGO, root,
> -				  rdev, &radeon_ttm_vram_fops);
> -	if (IS_ERR(ent))
> -		return PTR_ERR(ent);
> -	rdev->mman.vram = ent;
> -
> -	ent = debugfs_create_file("radeon_gtt", S_IFREG | S_IRUGO, root,
> -				  rdev, &radeon_ttm_gtt_fops);
> -	if (IS_ERR(ent))
> -		return PTR_ERR(ent);
> -	rdev->mman.gtt = ent;
> +	struct dentry *root = minor->debugfs_root;
> +
> +	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
> +		ret = drm_debugfs_create_file(root, minor,
> +					      radeon_debugfs_files[i].name,
> +					      radeon_debugfs_files[i].fops,
> +					      S_IFREG | S_IWUSR);
> +		if (ret) {
> +			radeon_ttm_debugfs_fini(rdev);
> +			return ret;
> +		}
> +	}
>   
>   	count = ARRAY_SIZE(radeon_ttm_debugfs_list);
>   
> @@ -1010,11 +1017,13 @@ static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
>   static void radeon_ttm_debugfs_fini(struct radeon_device *rdev)
>   {
>   #if defined(CONFIG_DEBUG_FS)
> +	int i;
>   
> -	debugfs_remove(rdev->mman.vram);
> -	rdev->mman.vram = NULL;
> +	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
> +		struct drm_info_list *info_list =
> +			(struct drm_info_list *)&radeon_debugfs_files[i];
>   
> -	debugfs_remove(rdev->mman.gtt);
> -	rdev->mman.gtt = NULL;
> +		drm_debugfs_remove_files(info_list, 1, rdev->ddev->primary);

This won't work correctly because info_ent doesn't contain this pointer 
but a pointer to the fops instead. Additional to that the fops might not 
be unique (you can use the same fops for different files) so they are 
probably not such a good choice as the key.

On the other hand why do the drm layer want to do this bookkeeping of 
all the created files in the first place? We could just use 
debugfs_remove_recursive instead and don't need to mess with this at all.

Regards,
Christian.

> +	}
>   #endif
>   }


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm/radeon: Use new drm debugfs file helper

[Ben Widawsky]

On Wed, Jan 22, 2014 at 11:11:10AM +0100, Christian König wrote:
> Am 21.01.2014 21:33, schrieb Ben Widawsky:
> >The debugfs helper duplicates the functionality used by Armada, so let's
> >just use that.
> >
> >WARNING: only compile tested
> >
> >Cc: Christian König <christian.koenig@amd.com>
> >Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >---
> >  drivers/gpu/drm/radeon/radeon.h     |  5 -----
> >  drivers/gpu/drm/radeon/radeon_ttm.c | 43 ++++++++++++++++++++++---------------
> >  2 files changed, 26 insertions(+), 22 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> >index c5519ca..ceec468 100644
> >--- a/drivers/gpu/drm/radeon/radeon.h
> >+++ b/drivers/gpu/drm/radeon/radeon.h
> >@@ -418,11 +418,6 @@ struct radeon_mman {
> >  	struct ttm_bo_device		bdev;
> >  	bool				mem_global_referenced;
> >  	bool				initialized;
> >-
> >-#if defined(CONFIG_DEBUG_FS)
> >-	struct dentry			*vram;
> >-	struct dentry			*gtt;
> >-#endif
> >  };
> >  /* bo virtual address in a specific vm */
> >diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> >index 77f5b0c..cf7caf2 100644
> >--- a/drivers/gpu/drm/radeon/radeon_ttm.c
> >+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> >@@ -971,27 +971,34 @@ static const struct file_operations radeon_ttm_gtt_fops = {
> >  	.llseek = default_llseek
> >  };
> >+static const struct radeon_debugfs_files {
> >+	const char *name;
> >+	const struct file_operations *fops;
> >+} radeon_debugfs_files[] = {
> >+	{"radeon_vram", &radeon_ttm_vram_fops},
> >+	{"radeon_gtt", &radeon_ttm_gtt_fops},
> >+};
> >  #endif
> >  static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
> >  {
> >  #if defined(CONFIG_DEBUG_FS)
> >  	unsigned count;
> >+	int i, ret;
> >  	struct drm_minor *minor = rdev->ddev->primary;
> >-	struct dentry *ent, *root = minor->debugfs_root;
> >-
> >-	ent = debugfs_create_file("radeon_vram", S_IFREG | S_IRUGO, root,
> >-				  rdev, &radeon_ttm_vram_fops);
> >-	if (IS_ERR(ent))
> >-		return PTR_ERR(ent);
> >-	rdev->mman.vram = ent;
> >-
> >-	ent = debugfs_create_file("radeon_gtt", S_IFREG | S_IRUGO, root,
> >-				  rdev, &radeon_ttm_gtt_fops);
> >-	if (IS_ERR(ent))
> >-		return PTR_ERR(ent);
> >-	rdev->mman.gtt = ent;
> >+	struct dentry *root = minor->debugfs_root;
> >+
> >+	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
> >+		ret = drm_debugfs_create_file(root, minor,
> >+					      radeon_debugfs_files[i].name,
> >+					      radeon_debugfs_files[i].fops,
> >+					      S_IFREG | S_IWUSR);
> >+		if (ret) {
> >+			radeon_ttm_debugfs_fini(rdev);
> >+			return ret;
> >+		}
> >+	}
> >  	count = ARRAY_SIZE(radeon_ttm_debugfs_list);
> >@@ -1010,11 +1017,13 @@ static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
> >  static void radeon_ttm_debugfs_fini(struct radeon_device *rdev)
> >  {
> >  #if defined(CONFIG_DEBUG_FS)
> >+	int i;
> >-	debugfs_remove(rdev->mman.vram);
> >-	rdev->mman.vram = NULL;
> >+	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
> >+		struct drm_info_list *info_list =
> >+			(struct drm_info_list *)&radeon_debugfs_files[i];
> >-	debugfs_remove(rdev->mman.gtt);
> >-	rdev->mman.gtt = NULL;
> >+		drm_debugfs_remove_files(info_list, 1, rdev->ddev->primary);
> 
> This won't work correctly because info_ent doesn't contain this pointer but
> a pointer to the fops instead.

You are correct. It should have been &radeon_debugfs_files[i].fops.

> Additional to that the fops might not be unique (you can use the same
> fops for different files) so they are probably not such a good choice
> as the key.

This is correct, and I elaborate a bit on this in a later patch (or
earlier, I forget). Essentially we have exactly one usage (afaict) of a
caller that wants a fops != key. I didn't want to create the interface
around this possibility, but rather let other extend it when it becomes
more common.

However, if this is requested/required, what I would do is provide a
void *key argument, where a NULL value will result in using fops.


> 
> On the other hand why do the drm layer want to do this bookkeeping of
> all the created files in the first place? We could just use
> debugfs_remove_recursive instead and don't need to mess with this at
> all.
> 
> Regards, Christian.
> 

The answer to the question predates me. I think having the helpers for
the common case, where you only want a show() function is handy. The
decision to split off and continue to shoehorn newer, more featured
debugfs files (as you have done in Radeon with these two, and as we have
done in i915 all over) was not one I made, nor one I really care to fix.

So assuming I fix the bug, what do you want to me do?

> >+	} #endif }
> 
> 

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm/radeon: Use new drm debugfs file helper

[Christian König]

Am 23.01.2014 04:20, schrieb Ben Widawsky:
> On Wed, Jan 22, 2014 at 11:11:10AM +0100, Christian König wrote:
>> Am 21.01.2014 21:33, schrieb Ben Widawsky:
>>> The debugfs helper duplicates the functionality used by Armada, so let's
>>> just use that.
>>>
>>> WARNING: only compile tested
>>>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>> ---
>>>   drivers/gpu/drm/radeon/radeon.h     |  5 -----
>>>   drivers/gpu/drm/radeon/radeon_ttm.c | 43 ++++++++++++++++++++++---------------
>>>   2 files changed, 26 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
>>> index c5519ca..ceec468 100644
>>> --- a/drivers/gpu/drm/radeon/radeon.h
>>> +++ b/drivers/gpu/drm/radeon/radeon.h
>>> @@ -418,11 +418,6 @@ struct radeon_mman {
>>>   	struct ttm_bo_device		bdev;
>>>   	bool				mem_global_referenced;
>>>   	bool				initialized;
>>> -
>>> -#if defined(CONFIG_DEBUG_FS)
>>> -	struct dentry			*vram;
>>> -	struct dentry			*gtt;
>>> -#endif
>>>   };
>>>   /* bo virtual address in a specific vm */
>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
>>> index 77f5b0c..cf7caf2 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>> @@ -971,27 +971,34 @@ static const struct file_operations radeon_ttm_gtt_fops = {
>>>   	.llseek = default_llseek
>>>   };
>>> +static const struct radeon_debugfs_files {
>>> +	const char *name;
>>> +	const struct file_operations *fops;
>>> +} radeon_debugfs_files[] = {
>>> +	{"radeon_vram", &radeon_ttm_vram_fops},
>>> +	{"radeon_gtt", &radeon_ttm_gtt_fops},
>>> +};
>>>   #endif
>>>   static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
>>>   {
>>>   #if defined(CONFIG_DEBUG_FS)
>>>   	unsigned count;
>>> +	int i, ret;
>>>   	struct drm_minor *minor = rdev->ddev->primary;
>>> -	struct dentry *ent, *root = minor->debugfs_root;
>>> -
>>> -	ent = debugfs_create_file("radeon_vram", S_IFREG | S_IRUGO, root,
>>> -				  rdev, &radeon_ttm_vram_fops);
>>> -	if (IS_ERR(ent))
>>> -		return PTR_ERR(ent);
>>> -	rdev->mman.vram = ent;
>>> -
>>> -	ent = debugfs_create_file("radeon_gtt", S_IFREG | S_IRUGO, root,
>>> -				  rdev, &radeon_ttm_gtt_fops);
>>> -	if (IS_ERR(ent))
>>> -		return PTR_ERR(ent);
>>> -	rdev->mman.gtt = ent;
>>> +	struct dentry *root = minor->debugfs_root;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
>>> +		ret = drm_debugfs_create_file(root, minor,
>>> +					      radeon_debugfs_files[i].name,
>>> +					      radeon_debugfs_files[i].fops,
>>> +					      S_IFREG | S_IWUSR);
>>> +		if (ret) {
>>> +			radeon_ttm_debugfs_fini(rdev);
>>> +			return ret;
>>> +		}
>>> +	}
>>>   	count = ARRAY_SIZE(radeon_ttm_debugfs_list);
>>> @@ -1010,11 +1017,13 @@ static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
>>>   static void radeon_ttm_debugfs_fini(struct radeon_device *rdev)
>>>   {
>>>   #if defined(CONFIG_DEBUG_FS)
>>> +	int i;
>>> -	debugfs_remove(rdev->mman.vram);
>>> -	rdev->mman.vram = NULL;
>>> +	for (i = 0; i < ARRAY_SIZE(radeon_debugfs_files); i++) {
>>> +		struct drm_info_list *info_list =
>>> +			(struct drm_info_list *)&radeon_debugfs_files[i];
>>> -	debugfs_remove(rdev->mman.gtt);
>>> -	rdev->mman.gtt = NULL;
>>> +		drm_debugfs_remove_files(info_list, 1, rdev->ddev->primary);
>> This won't work correctly because info_ent doesn't contain this pointer but
>> a pointer to the fops instead.
> You are correct. It should have been &radeon_debugfs_files[i].fops.
>
>> Additional to that the fops might not be unique (you can use the same
>> fops for different files) so they are probably not such a good choice
>> as the key.
> This is correct, and I elaborate a bit on this in a later patch (or
> earlier, I forget). Essentially we have exactly one usage (afaict) of a
> caller that wants a fops != key. I didn't want to create the interface
> around this possibility, but rather let other extend it when it becomes
> more common.
>
> However, if this is requested/required, what I would do is provide a
> void *key argument, where a NULL value will result in using fops.
>
>
>> On the other hand why do the drm layer want to do this bookkeeping of
>> all the created files in the first place? We could just use
>> debugfs_remove_recursive instead and don't need to mess with this at
>> all.
>>
>> Regards, Christian.
>>
> The answer to the question predates me. I think having the helpers for
> the common case, where you only want a show() function is handy. The
> decision to split off and continue to shoehorn newer, more featured
> debugfs files (as you have done in Radeon with these two, and as we have
> done in i915 all over) was not one I made, nor one I really care to fix.
>
> So assuming I fix the bug, what do you want to me do?

If you want to clean it up then clean it up completely.

The helper to add a debugfs file with only a show function should 
probably be a general debugfs feature instead of being part of DRM.

So as long as nobody can come up with a good reason why DRM does this 
housekeeping of all created debugfs file I would say use 
debugfs_remove_recursive instead and kill the complete drm debugfs helpers.

Christian.

>
>>> +	} #endif }
>>


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/i915: Use new drm debugfs file helper

[Ben Widawsky]

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

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 473dda2..72b8388 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3221,24 +3221,6 @@ static int i915_forcewake_create(struct dentry *root, struct drm_minor *minor)
 	return drm_add_fake_info_node(minor, ent, &i915_forcewake_fops);
 }
 
-static int i915_debugfs_create(struct dentry *root,
-			       struct drm_minor *minor,
-			       const char *name,
-			       const struct file_operations *fops)
-{
-	struct drm_device *dev = minor->dev;
-	struct dentry *ent;
-
-	ent = debugfs_create_file(name,
-				  S_IRUGO | S_IWUSR,
-				  root, dev,
-				  fops);
-	if (!ent)
-		return -ENOMEM;
-
-	return drm_add_fake_info_node(minor, ent, fops);
-}
-
 static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_capabilities", i915_capabilities, 0},
 	{"i915_gem_objects", i915_gem_object_info, 0},
@@ -3328,9 +3310,10 @@ int i915_debugfs_init(struct drm_minor *minor)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(i915_debugfs_files); i++) {
-		ret = i915_debugfs_create(minor->debugfs_root, minor,
-					  i915_debugfs_files[i].name,
-					  i915_debugfs_files[i].fops);
+		ret = drm_debugfs_create_file(minor->debugfs_root, minor,
+					      i915_debugfs_files[i].name,
+					      i915_debugfs_files[i].fops,
+					      S_IRUGO | S_IWUSR);
 		if (ret)
 			return ret;
 	}
-- 
1.8.5.3

[PATCH 5/6] drm/i915: Move forcewake debugfs setup also

[Ben Widawsky]

The forcewake setup is eerily similar, except it needs special mode
flags. Inserting that info into our structure is trivial, and with that
forcewake easily converts to using the new interface as well.

Notice that CRC is lacking from this patch (CRC being very similar to
forcewake in code). CRC is targeted for a new file, so there is no
reason to move it.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 48 +++++++++++--------------------------
 1 file changed, 14 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 72b8388..d092631 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3206,21 +3206,6 @@ static const struct file_operations i915_forcewake_fops = {
 	.release = i915_forcewake_release,
 };
 
-static int i915_forcewake_create(struct dentry *root, struct drm_minor *minor)
-{
-	struct drm_device *dev = minor->dev;
-	struct dentry *ent;
-
-	ent = debugfs_create_file("i915_forcewake_user",
-				  S_IRUSR,
-				  root, dev,
-				  &i915_forcewake_fops);
-	if (!ent)
-		return -ENOMEM;
-
-	return drm_add_fake_info_node(minor, ent, &i915_forcewake_fops);
-}
-
 static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_capabilities", i915_capabilities, 0},
 	{"i915_gem_objects", i915_gem_object_info, 0},
@@ -3267,18 +3252,20 @@ static const struct drm_info_list i915_debugfs_list[] = {
 static const struct i915_debugfs_files {
 	const char *name;
 	const struct file_operations *fops;
+	umode_t mode;
 } i915_debugfs_files[] = {
-	{"i915_wedged", &i915_wedged_fops},
-	{"i915_max_freq", &i915_max_freq_fops},
-	{"i915_min_freq", &i915_min_freq_fops},
-	{"i915_cache_sharing", &i915_cache_sharing_fops},
-	{"i915_ring_stop", &i915_ring_stop_fops},
-	{"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
-	{"i915_ring_test_irq", &i915_ring_test_irq_fops},
-	{"i915_gem_drop_caches", &i915_drop_caches_fops},
-	{"i915_error_state", &i915_error_state_fops},
-	{"i915_next_seqno", &i915_next_seqno_fops},
-	{"i915_display_crc_ctl", &i915_display_crc_ctl_fops},
+	{"i915_wedged", &i915_wedged_fops, S_IRUGO | S_IWUSR},
+	{"i915_max_freq", &i915_max_freq_fops, S_IRUGO | S_IWUSR},
+	{"i915_min_freq", &i915_min_freq_fops, S_IRUGO | S_IWUSR},
+	{"i915_cache_sharing", &i915_cache_sharing_fops, S_IRUGO | S_IWUSR},
+	{"i915_ring_stop", &i915_ring_stop_fops, S_IRUGO | S_IWUSR},
+	{"i915_ring_missed_irq", &i915_ring_missed_irq_fops, S_IRUGO | S_IWUSR},
+	{"i915_ring_test_irq", &i915_ring_test_irq_fops, S_IRUGO | S_IWUSR},
+	{"i915_gem_drop_caches", &i915_drop_caches_fops, S_IRUGO | S_IWUSR},
+	{"i915_error_state", &i915_error_state_fops, S_IRUGO | S_IWUSR},
+	{"i915_next_seqno", &i915_next_seqno_fops, S_IRUGO | S_IWUSR},
+	{"i915_display_crc_ctl", &i915_display_crc_ctl_fops, S_IRUGO | S_IWUSR},
+	{"i915_forcewake_user", &i915_forcewake_fops, S_IRUGO},
 };
 
 void intel_display_crc_init(struct drm_device *dev)
@@ -3299,10 +3286,6 @@ int i915_debugfs_init(struct drm_minor *minor)
 {
 	int ret, i;
 
-	ret = i915_forcewake_create(minor->debugfs_root, minor);
-	if (ret)
-		return ret;
-
 	for (i = 0; i < ARRAY_SIZE(i915_pipe_crc_data); i++) {
 		ret = i915_pipe_crc_create(minor->debugfs_root, minor, i);
 		if (ret)
@@ -3313,7 +3296,7 @@ int i915_debugfs_init(struct drm_minor *minor)
 		ret = drm_debugfs_create_file(minor->debugfs_root, minor,
 					      i915_debugfs_files[i].name,
 					      i915_debugfs_files[i].fops,
-					      S_IRUGO | S_IWUSR);
+					      i915_debugfs_files[i].mode);
 		if (ret)
 			return ret;
 	}
@@ -3330,9 +3313,6 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
 	drm_debugfs_remove_files(i915_debugfs_list,
 				 I915_DEBUGFS_ENTRIES, minor);
 
-	drm_debugfs_remove_files((struct drm_info_list *) &i915_forcewake_fops,
-				 1, minor);
-
 	for (i = 0; i < ARRAY_SIZE(i915_pipe_crc_data); i++) {
 		struct drm_info_list *info_list =
 			(struct drm_info_list *)&i915_pipe_crc_data[i];
-- 
1.8.5.3