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(-)
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");
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
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; }
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__ */
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
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
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
linaro-mm-sig@lists.linaro.org
-
Daniel Vetter -
Mathias Krause -
Sumit Semwal