Skip to contentSkip to conversation

Fix debugfs_create_dir's error checking method for sound/soc/

PATCH In review Zhaolei 3 participants 5 messages Oct 17, 2008

Subsystems (2)
Sound ASoC
lore
Share & tools
lore

Zhaolei

author
#0 lore raw

PATCH 1/1 Fix debugfs_create_dir's error checking method for sound/soc/

Hi,

debugfs_create_dir() returns NULL if an error occurs, returns -ENODEV when debugfs is not enabled in the kernel.

Signed-off-by Zhao Lei <[email protected]>
---
 sound/soc/soc-dapm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
sound/soc/soc-dapm.c +1 -1
index efbd0b3..7351db9 100644
@@ -831,7 +831,7 @@ int snd_soc_dapm_sys_add(struct device *dev)
 		return ret;
 
 	asoc_debugfs = debugfs_create_dir("asoc", NULL);
-	if (!IS_ERR(asoc_debugfs))
+	if (!IS_ERR(asoc_debugfs) && asoc_debugfs)
 		debugfs_create_u32("dapm_pop_time", 0744, asoc_debugfs,
 				   &pop_time);
 	else
Signature · 1 line
1.5.5.3
Acked-by on v1 1/1
#1 lore raw
Quoted 2 lines · Zhaolei

debugfs_create_dir() returns NULL if an error occurs, returns -ENODEV when debugfs is not enabled in the kernel.

...

sound/soc/soc-dapm.c int snd_soc_dapm_sys_add(struct device *dev) · line 836

Quoted 5 lines
sound/soc/soc-dapm.c +1 -1
 	asoc_debugfs = debugfs_create_dir("asoc", NULL);
-	if (!IS_ERR(asoc_debugfs))
+	if (!IS_ERR(asoc_debugfs) && asoc_debugfs)
 		debugfs_create_u32("dapm_pop_time", 0744, asoc_debugfs,
 				   &pop_time);

Hrmpf. This looks like something that should be fixed in debugfs - using both error reporting strategies is rather unhelpful and we're actually loosing information in the case where debugfs is built. I'll send a patch and see what people thing.

Still in the meantime,

Acked-by Mark Brown <[email protected]>
#2 lore raw

At Fri, 17 Oct 2008 14:23:42 +0100,

Quoted 8 lines · Mark Brown
Quoted 2 lines · Zhaolei

debugfs_create_dir() returns NULL if an error occurs, returns -ENODEV when debugfs is not enabled in the kernel.

...

Quoted 10 lines

sound/soc/soc-dapm.c int snd_soc_dapm_sys_add(struct device *dev) · line 836

Quoted 5 lines
sound/soc/soc-dapm.c +1 -1
 	asoc_debugfs = debugfs_create_dir("asoc", NULL);
-	if (!IS_ERR(asoc_debugfs))
+	if (!IS_ERR(asoc_debugfs) && asoc_debugfs)
 		debugfs_create_u32("dapm_pop_time", 0744, asoc_debugfs,
 				   &pop_time);

Hrmpf. This looks like something that should be fixed in debugfs - using both error reporting strategies is rather unhelpful and we're actually loosing information in the case where debugfs is built. I'll send a patch and see what people thing.

This looks like a design. In linux/debugfs.h:

/*
* We do not return NULL from these functions if CONFIG_DEBUG_FS is not enabled
* so users have a chance to detect if there was a real error or not. We don't
* want to duplicate the design decision mistakes of procfs and devfs again. */

Though, I agree that the detailed error information is lost by returning NULL...

Takashi

#3 lore raw

At Fri, 17 Oct 2008 17:21:21 +0200,

Quoted 22 lines · I wrote:

At Fri, 17 Oct 2008 14:23:42 +0100,

Quoted 8 lines · Mark Brown
Quoted 2 lines · Zhaolei

debugfs_create_dir() returns NULL if an error occurs, returns -ENODEV when debugfs is not enabled in the kernel.

...

Quoted 10 lines

sound/soc/soc-dapm.c int snd_soc_dapm_sys_add(struct device *dev) · line 836

Quoted 5 lines
sound/soc/soc-dapm.c +1 -1
 	asoc_debugfs = debugfs_create_dir("asoc", NULL);
-	if (!IS_ERR(asoc_debugfs))
+	if (!IS_ERR(asoc_debugfs) && asoc_debugfs)
 		debugfs_create_u32("dapm_pop_time", 0744, asoc_debugfs,
 				   &pop_time);

Hrmpf. This looks like something that should be fixed in debugfs - using both error reporting strategies is rather unhelpful and we're actually loosing information in the case where debugfs is built. I'll send a patch and see what people thing.

Quoted 1 line

This looks like a design. In linux/debugfs.h:

Oh, and I applied the patch now to sound git tree, BTW. Thanks!

Takashi

#4 lore raw
In reply to Takashi Iwai
Quoted 1 line · Takashi Iwai

This looks like a design. In linux/debugfs.h:

Yeah, so my grovelling through the code after I wrote that e-mail showed.

Quoted 8 lines

/*
* We do not return NULL from these functions if CONFIG_DEBUG_FS is not enabled
* so users have a chance to detect if there was a real error or not. We don't
* want to duplicate the design decision mistakes of procfs and devfs again. */

Though, I agree that the detailed error information is lost by returning NULL...

Yes, not to mention the fact that using both error reporting interfaces makes writing client code more error prone. Looking at the users most seem to only check one kind of error code is returned.

For ASoC checking only for null would actually be good enough - even if -ENODEV is returned we only ever pass the pointers into other debugfs functions so the fact that the pointer is bogus makes no odds, it'll never actually be dereferenced.