[PATCH 0/3] dma-buf: debugfs fixes

[PATCH 0/3] dma-buf: debugfs fixes

[Mathias Krause]

This small series is the v2 of the patch posted initially here:

  http://www.spinics.net/lists/linux-media/msg101347.html

It not only fixes the type mix-up and addresses Daniel's remark (patch 1),
it also smoothes out the error handling in dma_buf_init_debugfs() (patch 2)
and removes the then unneeded function dma_buf_debugfs_create_file() (patch
3).

Please apply!

Mathias Krause (3):
  dma-buf: propagate errors from dma_buf_describe() on debugfs read
  dma-buf: remove dma_buf directory on bufinfo file creation errors
  dma-buf: remove dma_buf_debugfs_create_file()

 drivers/dma-buf/dma-buf.c |   44 ++++++++++++++------------------------------
 include/linux/dma-buf.h   |    2 --
 2 files changed, 14 insertions(+), 32 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] dma-buf: propagate errors from dma_buf_describe() on debugfs read

[Mathias Krause]

The callback function dma_buf_describe() returns an int not void so the
function pointer cast in dma_buf_show() is wrong. dma_buf_describe() can
also fail when acquiring the mutex gets interrupted so always returning
0 in dma_buf_show() is wrong, too.

Fix both issues by avoiding the indirection via dma_buf_show() and call
dma_buf_describe() directly. Rename it to dma_buf_debug_show() to get it
in line with the other functions.

This type mismatch was caught by the PaX RAP plugin.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Brad Spengler <spender@grsecurity.net>
Cc: PaX Team <pageexec@freemail.hu>
---
 drivers/dma-buf/dma-buf.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 6355ab38d630..7094b19bb495 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -824,7 +824,7 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
 EXPORT_SYMBOL_GPL(dma_buf_vunmap);
 
 #ifdef CONFIG_DEBUG_FS
-static int dma_buf_describe(struct seq_file *s)
+static int dma_buf_debug_show(struct seq_file *s, void *unused)
 {
 	int ret;
 	struct dma_buf *buf_obj;
@@ -879,17 +879,9 @@ static int dma_buf_describe(struct seq_file *s)
 	return 0;
 }
 
-static int dma_buf_show(struct seq_file *s, void *unused)
-{
-	void (*func)(struct seq_file *) = s->private;
-
-	func(s);
-	return 0;
-}
-
 static int dma_buf_debug_open(struct inode *inode, struct file *file)
 {
-	return single_open(file, dma_buf_show, inode->i_private);
+	return single_open(file, dma_buf_debug_show, NULL);
 }
 
 static const struct file_operations dma_buf_debug_fops = {
@@ -913,7 +905,7 @@ static int dma_buf_init_debugfs(void)
 		return err;
 	}
 
-	err = dma_buf_debugfs_create_file("bufinfo", dma_buf_describe);
+	err = dma_buf_debugfs_create_file("bufinfo", NULL);
 
 	if (err)
 		pr_debug("dma_buf: debugfs: failed to create node bufinfo\n");
-- 
1.7.10.4

Re: [PATCH 1/3] dma-buf: propagate errors from dma_buf_describe() on debugfs read

[Daniel Vetter]

On Sun, Jun 19, 2016 at 02:31:29PM +0200, Mathias Krause wrote:
> The callback function dma_buf_describe() returns an int not void so the
> function pointer cast in dma_buf_show() is wrong. dma_buf_describe() can
> also fail when acquiring the mutex gets interrupted so always returning
> 0 in dma_buf_show() is wrong, too.
> 
> Fix both issues by avoiding the indirection via dma_buf_show() and call
> dma_buf_describe() directly. Rename it to dma_buf_debug_show() to get it
> in line with the other functions.
> 
> This type mismatch was caught by the PaX RAP plugin.
> 
> Signed-off-by: Mathias Krause <minipli@googlemail.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Brad Spengler <spender@grsecurity.net>
> Cc: PaX Team <pageexec@freemail.hu>
> ---
>  drivers/dma-buf/dma-buf.c |   14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 6355ab38d630..7094b19bb495 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -824,7 +824,7 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
>  EXPORT_SYMBOL_GPL(dma_buf_vunmap);
>  
>  #ifdef CONFIG_DEBUG_FS
> -static int dma_buf_describe(struct seq_file *s)
> +static int dma_buf_debug_show(struct seq_file *s, void *unused)
>  {
>  	int ret;
>  	struct dma_buf *buf_obj;
> @@ -879,17 +879,9 @@ static int dma_buf_describe(struct seq_file *s)
>  	return 0;
>  }
>  
> -static int dma_buf_show(struct seq_file *s, void *unused)
> -{
> -	void (*func)(struct seq_file *) = s->private;
> -
> -	func(s);
> -	return 0;
> -}
> -
>  static int dma_buf_debug_open(struct inode *inode, struct file *file)
>  {
> -	return single_open(file, dma_buf_show, inode->i_private);
> +	return single_open(file, dma_buf_debug_show, NULL);
>  }
>  
>  static const struct file_operations dma_buf_debug_fops = {
> @@ -913,7 +905,7 @@ static int dma_buf_init_debugfs(void)
>  		return err;
>  	}
>  
> -	err = dma_buf_debugfs_create_file("bufinfo", dma_buf_describe);
> +	err = dma_buf_debugfs_create_file("bufinfo", NULL);

This indirection now doesn't make much sense I think. I think more
sensible to instead pass drm_buf_debug_show, since that's the
parametrization that matters. Or just inline that one too.
-Daniel

>  
>  	if (err)
>  		pr_debug("dma_buf: debugfs: failed to create node bufinfo\n");
> -- 
> 1.7.10.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 2/3] dma-buf: remove dma_buf directory on bufinfo file creation errors

[Mathias Krause]

Change the error handling in dma_buf_init_debugfs() to remove the
"dma_buf" directory if creating the "bufinfo" file fails. No need to
have an empty debugfs directory around.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/dma-buf/dma-buf.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 7094b19bb495..f03e51561199 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -907,8 +907,11 @@ static int dma_buf_init_debugfs(void)
 
 	err = dma_buf_debugfs_create_file("bufinfo", NULL);
 
-	if (err)
+	if (err) {
 		pr_debug("dma_buf: debugfs: failed to create node bufinfo\n");
+		debugfs_remove_recursive(dma_buf_debugfs_dir);
+		dma_buf_debugfs_dir = NULL;
+	}
 
 	return err;
 }
-- 
1.7.10.4

[PATCH 3/3] dma-buf: remove dma_buf_debugfs_create_file()

[Mathias Krause]

There is only a single user of dma_buf_debugfs_create_file() and that
one got the function pointer cast wrong. With that one fixed, there is
no need to have a wrapper for debugfs_create_file(), just call it
directly.

With no users left, we can remove dma_buf_debugfs_create_file().

While at it, simplify the error handling in dma_buf_init_debugfs()
slightly.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/dma-buf/dma-buf.c |   29 +++++++++--------------------
 include/linux/dma-buf.h   |    2 --
 2 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index f03e51561199..20ce0687b111 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -895,22 +895,22 @@ static struct dentry *dma_buf_debugfs_dir;
 
 static int dma_buf_init_debugfs(void)
 {
+	struct dentry *d;
 	int err = 0;
 
-	dma_buf_debugfs_dir = debugfs_create_dir("dma_buf", NULL);
+	d = debugfs_create_dir("dma_buf", NULL);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
 
-	if (IS_ERR(dma_buf_debugfs_dir)) {
-		err = PTR_ERR(dma_buf_debugfs_dir);
-		dma_buf_debugfs_dir = NULL;
-		return err;
-	}
+	dma_buf_debugfs_dir = d;
 
-	err = dma_buf_debugfs_create_file("bufinfo", NULL);
-
-	if (err) {
+	d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir,
+				NULL, &dma_buf_debug_fops);
+	if (IS_ERR(d)) {
 		pr_debug("dma_buf: debugfs: failed to create node bufinfo\n");
 		debugfs_remove_recursive(dma_buf_debugfs_dir);
 		dma_buf_debugfs_dir = NULL;
+		err = PTR_ERR(d);
 	}
 
 	return err;
@@ -921,17 +921,6 @@ static void dma_buf_uninit_debugfs(void)
 	if (dma_buf_debugfs_dir)
 		debugfs_remove_recursive(dma_buf_debugfs_dir);
 }
-
-int dma_buf_debugfs_create_file(const char *name,
-				int (*write)(struct seq_file *))
-{
-	struct dentry *d;
-
-	d = debugfs_create_file(name, S_IRUGO, dma_buf_debugfs_dir,
-			write, &dma_buf_debug_fops);
-
-	return PTR_ERR_OR_ZERO(d);
-}
 #else
 static inline int dma_buf_init_debugfs(void)
 {
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 4551c6f2a6c4..e0b0741ae671 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -242,6 +242,4 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *,
 		 unsigned long);
 void *dma_buf_vmap(struct dma_buf *);
 void dma_buf_vunmap(struct dma_buf *, void *vaddr);
-int dma_buf_debugfs_create_file(const char *name,
-				int (*write)(struct seq_file *));
 #endif /* __DMA_BUF_H__ */
-- 
1.7.10.4

Re: [PATCH 3/3] dma-buf: remove dma_buf_debugfs_create_file()

[Daniel Vetter]

On Sun, Jun 19, 2016 at 02:31:31PM +0200, Mathias Krause wrote:
> There is only a single user of dma_buf_debugfs_create_file() and that
> one got the function pointer cast wrong. With that one fixed, there is
> no need to have a wrapper for debugfs_create_file(), just call it
> directly.
> 
> With no users left, we can remove dma_buf_debugfs_create_file().
> 
> While at it, simplify the error handling in dma_buf_init_debugfs()
> slightly.
> 
> Signed-off-by: Mathias Krause <minipli@googlemail.com>

ah, here's the 2nd part, feel free to ignore my earlier comments. On the
series:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/dma-buf/dma-buf.c |   29 +++++++++--------------------
>  include/linux/dma-buf.h   |    2 --
>  2 files changed, 9 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index f03e51561199..20ce0687b111 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -895,22 +895,22 @@ static struct dentry *dma_buf_debugfs_dir;
>  
>  static int dma_buf_init_debugfs(void)
>  {
> +	struct dentry *d;
>  	int err = 0;
>  
> -	dma_buf_debugfs_dir = debugfs_create_dir("dma_buf", NULL);
> +	d = debugfs_create_dir("dma_buf", NULL);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);
>  
> -	if (IS_ERR(dma_buf_debugfs_dir)) {
> -		err = PTR_ERR(dma_buf_debugfs_dir);
> -		dma_buf_debugfs_dir = NULL;
> -		return err;
> -	}
> +	dma_buf_debugfs_dir = d;
>  
> -	err = dma_buf_debugfs_create_file("bufinfo", NULL);
> -
> -	if (err) {
> +	d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir,
> +				NULL, &dma_buf_debug_fops);
> +	if (IS_ERR(d)) {
>  		pr_debug("dma_buf: debugfs: failed to create node bufinfo\n");
>  		debugfs_remove_recursive(dma_buf_debugfs_dir);
>  		dma_buf_debugfs_dir = NULL;
> +		err = PTR_ERR(d);
>  	}
>  
>  	return err;
> @@ -921,17 +921,6 @@ static void dma_buf_uninit_debugfs(void)
>  	if (dma_buf_debugfs_dir)
>  		debugfs_remove_recursive(dma_buf_debugfs_dir);
>  }
> -
> -int dma_buf_debugfs_create_file(const char *name,
> -				int (*write)(struct seq_file *))
> -{
> -	struct dentry *d;
> -
> -	d = debugfs_create_file(name, S_IRUGO, dma_buf_debugfs_dir,
> -			write, &dma_buf_debug_fops);
> -
> -	return PTR_ERR_OR_ZERO(d);
> -}
>  #else
>  static inline int dma_buf_init_debugfs(void)
>  {
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 4551c6f2a6c4..e0b0741ae671 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -242,6 +242,4 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *,
>  		 unsigned long);
>  void *dma_buf_vmap(struct dma_buf *);
>  void dma_buf_vunmap(struct dma_buf *, void *vaddr);
> -int dma_buf_debugfs_create_file(const char *name,
> -				int (*write)(struct seq_file *));
>  #endif /* __DMA_BUF_H__ */
> -- 
> 1.7.10.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/3] dma-buf: remove dma_buf_debugfs_create_file()

[Mathias Krause]

On 20 June 2016 at 15:31, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Jun 19, 2016 at 02:31:31PM +0200, Mathias Krause wrote:
>> [...]
>> With no users left, we can remove dma_buf_debugfs_create_file().
>>
>> While at it, simplify the error handling in dma_buf_init_debugfs()
>> slightly.
>>
>> Signed-off-by: Mathias Krause <minipli@googlemail.com>
>
> ah, here's the 2nd part, feel free to ignore my earlier comments. On the
> series:

Yeah, I've split the original patch into three to separate bug fixes
(patch 1+2) from enhancements (this patch) -- just in case anybody
wants to backport the fixes.

Also, this way this patch can easily be left out without missing the
fixes, in case new debugfs files below dma_buf/ are expected in the
near future. Those might want to make use of
dma_buf_debugfs_create_file(). But, as there haven't been any since
its introduction in commit
b89e35636bc7 ("dma-buf: Add debugfs support") in 2013, I guess, we can
just remove that function. ;)
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks,
Mathias

Re: [PATCH 0/3] dma-buf: debugfs fixes

[Sumit Semwal]

Hi Mathias,

On 19 June 2016 at 18:01, Mathias Krause <minipli@googlemail.com> wrote:
> This small series is the v2 of the patch posted initially here:
>
>   http://www.spinics.net/lists/linux-media/msg101347.html
>
> It not only fixes the type mix-up and addresses Daniel's remark (patch 1),
> it also smoothes out the error handling in dma_buf_init_debugfs() (patch 2)
> and removes the then unneeded function dma_buf_debugfs_create_file() (patch
> 3).
>
> Please apply!
>
Thanks for your patchset; applied via drm-misc!

> Mathias Krause (3):
>   dma-buf: propagate errors from dma_buf_describe() on debugfs read
>   dma-buf: remove dma_buf directory on bufinfo file creation errors
>   dma-buf: remove dma_buf_debugfs_create_file()
>
>  drivers/dma-buf/dma-buf.c |   44 ++++++++++++++------------------------------
>  include/linux/dma-buf.h   |    2 --
>  2 files changed, 14 insertions(+), 32 deletions(-)
>
> --
> 1.7.10.4
>



-- 
Thanks and regards,

Sumit Semwal
Linaro Mobile Group - Kernel Team Lead
Linaro.org │ Open source software for ARM SoCs