[PATCH 1/5] sound: SoC: sof: no need to check return value of debugfs_create functions

[PATCH 1/5] sound: SoC: sof: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/soc/sof/debug.c    | 32 ++++++++++----------------------
 sound/soc/sof/sof-priv.h |  1 -
 sound/soc/sof/trace.c    |  9 ++-------
 3 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/sound/soc/sof/debug.c b/sound/soc/sof/debug.c
index 55f1d808dba0..429daa0dc9bf 100644
--- a/sound/soc/sof/debug.c
+++ b/sound/soc/sof/debug.c
@@ -77,8 +77,7 @@ static ssize_t sof_dfsentry_read(struct file *file, char __user *buffer,
 		if (!pm_runtime_active(sdev->dev) &&
 		    dfse->access_type == SOF_DEBUGFS_ACCESS_D0_ONLY) {
 			dev_err(sdev->dev,
-				"error: debugfs entry %s cannot be read in DSP D3\n",
-				dfse->dfsentry->d_name.name);
+				"error: debugfs entry cannot be read in DSP D3\n");
 			kfree(buf);
 			return -EINVAL;
 		}
@@ -142,17 +141,11 @@ int snd_sof_debugfs_io_item(struct snd_sof_dev *sdev,
 	}
 #endif
 
-	dfse->dfsentry = debugfs_create_file(name, 0444, sdev->debugfs_root,
-					     dfse, &sof_dfs_fops);
-	if (!dfse->dfsentry) {
-		/* can't rely on debugfs, only log error and keep going */
-		dev_err(sdev->dev, "error: cannot create debugfs entry %s\n",
-			name);
-	} else {
-		/* add to dfsentry list */
-		list_add(&dfse->list, &sdev->dfsentry_list);
+	debugfs_create_file(name, 0444, sdev->debugfs_root, dfse,
+			    &sof_dfs_fops);
 
-	}
+	/* add to dfsentry list */
+	list_add(&dfse->list, &sdev->dfsentry_list);
 
 	return 0;
 }
@@ -177,16 +170,11 @@ int snd_sof_debugfs_buf_item(struct snd_sof_dev *sdev,
 	dfse->size = size;
 	dfse->sdev = sdev;
 
-	dfse->dfsentry = debugfs_create_file(name, 0444, sdev->debugfs_root,
-					     dfse, &sof_dfs_fops);
-	if (!dfse->dfsentry) {
-		/* can't rely on debugfs, only log error and keep going */
-		dev_err(sdev->dev, "error: cannot create debugfs entry %s\n",
-			name);
-	} else {
-		/* add to dfsentry list */
-		list_add(&dfse->list, &sdev->dfsentry_list);
-	}
+	debugfs_create_file(name, 0444, sdev->debugfs_root, dfse,
+			    &sof_dfs_fops);
+
+	/* add to dfsentry list */
+	list_add(&dfse->list, &sdev->dfsentry_list);
 
 	return 0;
 }
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 1e85d6f9c5c3..d0f7cc3ddcbf 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -217,7 +217,6 @@ enum sof_debugfs_access_type {
 
 /* FS entry for debug files that can expose DSP memories, registers */
 struct snd_sof_dfsentry {
-	struct dentry *dfsentry;
 	size_t size;
 	enum sof_dfsentry_type type;
 	/*
diff --git a/sound/soc/sof/trace.c b/sound/soc/sof/trace.c
index d588e4b70fad..2986d9a563b0 100644
--- a/sound/soc/sof/trace.c
+++ b/sound/soc/sof/trace.c
@@ -119,13 +119,8 @@ static int trace_debugfs_create(struct snd_sof_dev *sdev)
 	dfse->size = sdev->dmatb.bytes;
 	dfse->sdev = sdev;
 
-	dfse->dfsentry = debugfs_create_file("trace", 0444, sdev->debugfs_root,
-					     dfse, &sof_dfs_trace_fops);
-	if (!dfse->dfsentry) {
-		/* can't rely on debugfs, only log error and keep going */
-		dev_err(sdev->dev,
-			"error: cannot create debugfs entry for trace\n");
-	}
+	debugfs_create_file("trace", 0444, sdev->debugfs_root, dfse,
+			    &sof_dfs_trace_fops);
 
 	return 0;
 }
-- 
2.22.0

[PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Cc: Jie Yang <yang.jie@linux.intel.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: alsa-devel@alsa-project.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/soc/intel/skylake/skl-debug.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-debug.c b/sound/soc/intel/skylake/skl-debug.c
index 5d7ac2ee7a3c..5481e3362414 100644
--- a/sound/soc/intel/skylake/skl-debug.c
+++ b/sound/soc/intel/skylake/skl-debug.c
@@ -170,10 +170,8 @@ void skl_debug_init_module(struct skl_debug *d,
 			struct snd_soc_dapm_widget *w,
 			struct skl_module_cfg *mconfig)
 {
-	if (!debugfs_create_file(w->name, 0444,
-				d->modules, mconfig,
-				&mcfg_fops))
-		dev_err(d->dev, "%s: module debugfs init failed\n", w->name);
+	debugfs_create_file(w->name, 0444, d->modules, mconfig,
+			    &mcfg_fops);
 }
 
 static ssize_t fw_softreg_read(struct file *file, char __user *user_buf,
@@ -230,32 +228,16 @@ struct skl_debug *skl_debugfs_init(struct skl *skl)
 		return NULL;
 
 	/* create the debugfs dir with platform component's debugfs as parent */
-	d->fs = debugfs_create_dir("dsp",
-				   skl->component->debugfs_root);
-	if (IS_ERR(d->fs) || !d->fs) {
-		dev_err(&skl->pci->dev, "debugfs root creation failed\n");
-		return NULL;
-	}
+	d->fs = debugfs_create_dir("dsp", skl->component->debugfs_root);
 
 	d->skl = skl;
 	d->dev = &skl->pci->dev;
 
 	/* now create the module dir */
 	d->modules = debugfs_create_dir("modules", d->fs);
-	if (IS_ERR(d->modules) || !d->modules) {
-		dev_err(&skl->pci->dev, "modules debugfs create failed\n");
-		goto err;
-	}
 
-	if (!debugfs_create_file("fw_soft_regs_rd", 0444, d->fs, d,
-				 &soft_regs_ctrl_fops)) {
-		dev_err(d->dev, "fw soft regs control debugfs init failed\n");
-		goto err;
-	}
+	debugfs_create_file("fw_soft_regs_rd", 0444, d->fs, d,
+			    &soft_regs_ctrl_fops);
 
 	return d;
-
-err:
-	debugfs_remove_recursive(d->fs);
-	return NULL;
 }
-- 
2.22.0

Re: [PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Cezary Rojewski]

On 2019-06-14 11:47, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 

This change heavily impacts user space and development kits used by us 
internally, and our clients. That is, if anything goes wrong during 
debugfs initialization process.

Currently, apps may safely assume entire debugfs tree is up and running 
once audio stack gets enumerated successfully. With your patch this is 
no longer the case and user space is forced to verify status of all 
debugfs files and/ or directories manually.

Most of this you knew already - I see Mark was very vocal about 
consequences and possible regression. Nonetheless we have had a short 
meeting with our coe-audio members regarding this change, specifically. 
Conclusion was simple: losing ability to test debugfs objects status 
during their creation e.g.: via IS_ERR family is considered harmful.

Czarek

> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> Cc: Jie Yang <yang.jie@linux.intel.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: alsa-devel@alsa-project.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   sound/soc/intel/skylake/skl-debug.c | 28 +++++-----------------------
>   1 file changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl-debug.c b/sound/soc/intel/skylake/skl-debug.c
> index 5d7ac2ee7a3c..5481e3362414 100644
> --- a/sound/soc/intel/skylake/skl-debug.c
> +++ b/sound/soc/intel/skylake/skl-debug.c
> @@ -170,10 +170,8 @@ void skl_debug_init_module(struct skl_debug *d,
>   			struct snd_soc_dapm_widget *w,
>   			struct skl_module_cfg *mconfig)
>   {
> -	if (!debugfs_create_file(w->name, 0444,
> -				d->modules, mconfig,
> -				&mcfg_fops))
> -		dev_err(d->dev, "%s: module debugfs init failed\n", w->name);
> +	debugfs_create_file(w->name, 0444, d->modules, mconfig,
> +			    &mcfg_fops);
>   }
>   
>   static ssize_t fw_softreg_read(struct file *file, char __user *user_buf,
> @@ -230,32 +228,16 @@ struct skl_debug *skl_debugfs_init(struct skl *skl)
>   		return NULL;
>   
>   	/* create the debugfs dir with platform component's debugfs as parent */
> -	d->fs = debugfs_create_dir("dsp",
> -				   skl->component->debugfs_root);
> -	if (IS_ERR(d->fs) || !d->fs) {
> -		dev_err(&skl->pci->dev, "debugfs root creation failed\n");
> -		return NULL;
> -	}
> +	d->fs = debugfs_create_dir("dsp", skl->component->debugfs_root);
>   
>   	d->skl = skl;
>   	d->dev = &skl->pci->dev;
>   
>   	/* now create the module dir */
>   	d->modules = debugfs_create_dir("modules", d->fs);
> -	if (IS_ERR(d->modules) || !d->modules) {
> -		dev_err(&skl->pci->dev, "modules debugfs create failed\n");
> -		goto err;
> -	}
>   
> -	if (!debugfs_create_file("fw_soft_regs_rd", 0444, d->fs, d,
> -				 &soft_regs_ctrl_fops)) {
> -		dev_err(d->dev, "fw soft regs control debugfs init failed\n");
> -		goto err;
> -	}
> +	debugfs_create_file("fw_soft_regs_rd", 0444, d->fs, d,
> +			    &soft_regs_ctrl_fops);
>   
>   	return d;
> -
> -err:
> -	debugfs_remove_recursive(d->fs);
> -	return NULL;
>   }
> 

Re: [PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Takashi Iwai]

On Sat, 22 Jun 2019 21:57:07 +0200,
Cezary Rojewski wrote:
> 
> 
> On 2019-06-14 11:47, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value.  The function can work or not, but the code logic should
> > never do something different based on this.
> >
> 
> This change heavily impacts user space and development kits used by us
> internally, and our clients. That is, if anything goes wrong during
> debugfs initialization process.
> 
> Currently, apps may safely assume entire debugfs tree is up and
> running once audio stack gets enumerated successfully. With your patch
> this is no longer the case and user space is forced to verify status
> of all debugfs files and/ or directories manually.
> 
> Most of this you knew already - I see Mark was very vocal about
> consequences and possible regression. Nonetheless we have had a short
> meeting with our coe-audio members regarding this change,
> specifically. Conclusion was simple: losing ability to test debugfs
> objects status during their creation e.g.: via IS_ERR family is
> considered harmful.

Well, I just wonder whether you have ever seen a case where the
debugfs creation failed.  Or more practically, would it fail silently
at all?

If there can be a silent failure, then it's bad to just ignore, yes.
We may need either to make it more obvious or return an error.

Of course, in a tight memory pressure, the creation may fail; but the
whole system won't work properly in anyway (not only that the clear
error would be shown in kernel messages), so relying on the debugfs
itself make little sense in such a situation.


thanks,

Takashi

Re: [PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Takashi Iwai]

On Mon, 24 Jun 2019 12:53:34 +0200,
Mark Brown wrote:
> 
> On Sat, Jun 22, 2019 at 10:39:11PM +0200, Takashi Iwai wrote:
> 
> > Well, I just wonder whether you have ever seen a case where the
> > debugfs creation failed.  Or more practically, would it fail silently
> > at all?
> 
> > If there can be a silent failure, then it's bad to just ignore, yes.
> > We may need either to make it more obvious or return an error.
> 
> Currently debugfs doesn't report any errors other than via the return
> codes (at least in the common creation stuff) so it's up to the callers
> to do that.

So this should be changed to follow a la sysfs creation error, IMO.
At least, the name conflicts etc should be reported more obviously.


thanks,

Takashi

Re: [PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 581 bytes --]

On Mon, Jun 24, 2019 at 03:15:26PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > Currently debugfs doesn't report any errors other than via the return
> > codes (at least in the common creation stuff) so it's up to the callers
> > to do that.

> So this should be changed to follow a la sysfs creation error, IMO.
> At least, the name conflicts etc should be reported more obviously.

Indeed, that'd mitigate the problems with just making everything
silently fail a lot - so long as we say there's a problem people are a
lot less likely to be mislead if anything goes wrong.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Mon, Jun 24, 2019 at 02:33:36PM +0100, Mark Brown wrote:
> On Mon, Jun 24, 2019 at 03:15:26PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > Currently debugfs doesn't report any errors other than via the return
> > > codes (at least in the common creation stuff) so it's up to the callers
> > > to do that.
> 
> > So this should be changed to follow a la sysfs creation error, IMO.
> > At least, the name conflicts etc should be reported more obviously.
> 
> Indeed, that'd mitigate the problems with just making everything
> silently fail a lot - so long as we say there's a problem people are a
> lot less likely to be mislead if anything goes wrong.

Ok, let me work on doing this in the debugfs core and will cc: you all
on the resulting patches to see if it will satisfy your objections here.

sorry for the delay, am on the road all this week with limited
internet...

greg k-h

Re: [PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Sat, Jun 22, 2019 at 09:57:07PM +0200, Cezary Rojewski wrote:
> 
> On 2019-06-14 11:47, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value.  The function can work or not, but the code logic should
> > never do something different based on this.
> > 
> 
> This change heavily impacts user space and development kits used by us
> internally, and our clients. That is, if anything goes wrong during debugfs
> initialization process.

As Takashi said, and as I said numerous times, how can anything go wrong
during debugfs file creation that does not also cause the rest of your
system to just crash.

userspace should NEVER care about a debugfs file being present or not.
If it does, then you should not be using debugfs as it is never
guaranteed to be present on a system (and is locked down and removed on
many shipping systems for good reason.)

For development, it's wonderful, but it truely is just a debugging aid.

> Currently, apps may safely assume entire debugfs tree is up and running once
> audio stack gets enumerated successfully. With your patch this is no longer
> the case and user space is forced to verify status of all debugfs files and/
> or directories manually.

What apps rely on debugfs for audio?  We need to fix those.

Again, my goal with these changes is two things:
  - no kernel operation should ever modify its behavior if debugfs is
    enabled, or working, at all.
  - no normal userspace code should ever care if debugfs is working

debugfs is for debugging things, that is all.  If you have system
functionality relying on files in debugfs, they need to be moved to a
system functionality that is always going to be present for your users
(i.e. sysfs, configfs, tracefs, etc.)

thanks,

greg k-h

Re: [PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Cezary Rojewski]

On 2019-06-23 06:57, Greg Kroah-Hartman wrote:
> On Sat, Jun 22, 2019 at 09:57:07PM +0200, Cezary Rojewski wrote:
>>
>> On 2019-06-14 11:47, Greg Kroah-Hartman wrote:
>>> When calling debugfs functions, there is no need to ever check the
>>> return value.  The function can work or not, but the code logic should
>>> never do something different based on this.
>>>
>>
>> This change heavily impacts user space and development kits used by us
>> internally, and our clients. That is, if anything goes wrong during debugfs
>> initialization process.
> 
> As Takashi said, and as I said numerous times, how can anything go wrong
> during debugfs file creation that does not also cause the rest of your
> system to just crash. >
> userspace should NEVER care about a debugfs file being present or not.
> If it does, then you should not be using debugfs as it is never
> guaranteed to be present on a system (and is locked down and removed on
> many shipping systems for good reason.)
> 
> For development, it's wonderful, but it truely is just a debugging aid.
> 
>> Currently, apps may safely assume entire debugfs tree is up and running once
>> audio stack gets enumerated successfully. With your patch this is no longer
>> the case and user space is forced to verify status of all debugfs files and/
>> or directories manually.
> 
> What apps rely on debugfs for audio?  We need to fix those.
> 

Takashi,
Thanks for reply. I hope you don't mind me replying here and not 
explicitly on your post. My message would be exactly the same as the one 
you see below.


Greg,
Forgive me for not clarifying: by userspace of course I meant any 
development/ debug related app which we use exhaustively.

Look at this from different perspective: I'm "just" a user of debugfs 
interface. I call a function and given its declaration I receive either 
0 on success or != 0 on failure. Definition of said function may change 
in time and -ENOMEM might not be the only possible outcome, but that I 
leave to other developers and as long as behavior remains the same, 
changes are welcome.

Moreover, if we're compiling with CONFIG_DEBUGFS=1, driver may choose to 
collapse during probe if any of debugfs objects fail to initialize: in 
this case one can say CONFIG_DEBUGFS adds yet another condition for 
enumeration to be considered successful.

> Again, my goal with these changes is two things:
>    - no kernel operation should ever modify its behavior if debugfs is
>      enabled, or working, at all.
>    - no normal userspace code should ever care if debugfs is working
> 
> debugfs is for debugging things, that is all.  If you have system
> functionality relying on files in debugfs, they need to be moved to a
> system functionality that is always going to be present for your users
> (i.e. sysfs, configfs, tracefs, etc.)
> 
> thanks,
> 
> greg k-h
> 

With mindset "may or may not succeed" one might as well resign from 
debugfs entirely and move to sysfs and other fs you'd mentioned. And why 
would he otherwise, when the only way to verify debugfs object state is 
either log parsing (filtering errors) or file-exists check.

My app should not be guessing, instead, it should know upfront with what 
its working with.

Czarek

Re: [PATCH 2/5] sound: soc: skylake: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Sun, Jun 23, 2019 at 05:18:39PM +0200, Cezary Rojewski wrote:
> 
> On 2019-06-23 06:57, Greg Kroah-Hartman wrote:
> > On Sat, Jun 22, 2019 at 09:57:07PM +0200, Cezary Rojewski wrote:
> > > 
> > > On 2019-06-14 11:47, Greg Kroah-Hartman wrote:
> > > > When calling debugfs functions, there is no need to ever check the
> > > > return value.  The function can work or not, but the code logic should
> > > > never do something different based on this.
> > > > 
> > > 
> > > This change heavily impacts user space and development kits used by us
> > > internally, and our clients. That is, if anything goes wrong during debugfs
> > > initialization process.
> > 
> > As Takashi said, and as I said numerous times, how can anything go wrong
> > during debugfs file creation that does not also cause the rest of your
> > system to just crash. >
> > userspace should NEVER care about a debugfs file being present or not.
> > If it does, then you should not be using debugfs as it is never
> > guaranteed to be present on a system (and is locked down and removed on
> > many shipping systems for good reason.)
> > 
> > For development, it's wonderful, but it truely is just a debugging aid.
> > 
> > > Currently, apps may safely assume entire debugfs tree is up and running once
> > > audio stack gets enumerated successfully. With your patch this is no longer
> > > the case and user space is forced to verify status of all debugfs files and/
> > > or directories manually.
> > 
> > What apps rely on debugfs for audio?  We need to fix those.
> > 
> 
> Takashi,
> Thanks for reply. I hope you don't mind me replying here and not explicitly
> on your post. My message would be exactly the same as the one you see below.
> 
> 
> Greg,
> Forgive me for not clarifying: by userspace of course I meant any
> development/ debug related app which we use exhaustively.
> 
> Look at this from different perspective: I'm "just" a user of debugfs
> interface. I call a function and given its declaration I receive either 0 on
> success or != 0 on failure. Definition of said function may change in time
> and -ENOMEM might not be the only possible outcome, but that I leave to
> other developers and as long as behavior remains the same, changes are
> welcome.

Again, you should not even care if a debugfs call succeeds or not.

> Moreover, if we're compiling with CONFIG_DEBUGFS=1, driver may choose to
> collapse during probe if any of debugfs objects fail to initialize: in this
> case one can say CONFIG_DEBUGFS adds yet another condition for enumeration
> to be considered successful.

It should never matter to your code.

Debugfs was written to be much simpler and easier to use than procfs.
It goes against the "normal" defensive mode of kernel programming in
that you should not care if it works or not, your code should just keep
on working no matter what.

> > Again, my goal with these changes is two things:
> >    - no kernel operation should ever modify its behavior if debugfs is
> >      enabled, or working, at all.
> >    - no normal userspace code should ever care if debugfs is working
> > 
> > debugfs is for debugging things, that is all.  If you have system
> > functionality relying on files in debugfs, they need to be moved to a
> > system functionality that is always going to be present for your users
> > (i.e. sysfs, configfs, tracefs, etc.)
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> With mindset "may or may not succeed" one might as well resign from debugfs
> entirely and move to sysfs and other fs you'd mentioned.

That's fine, but do not put debugging stuff in sysfs please.  There are
strict rules on what you can and can not do in sysfs and other
filesystems.  Debugfs has no such rules except that no userspace code
should ever care about it.

> And why would he otherwise, when the only way to verify debugfs object
> state is either log parsing (filtering errors) or file-exists check.
> 
> My app should not be guessing, instead, it should know upfront with what its
> working with.

What app digs around in debugfs that any user should care about?  The
goal is to never have any functionality in there that the system
requires in order to work properly.

Again, we have found places where this is not the case, and it is being
fixed to remove that dependancy.

debugfs is "fire and forget" and used for debugging stuff only.  No
working system should care at all about it.

thanks,

greg k-h

[PATCH 3/5] sound: soc: codecs: wm_adsp: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: <patches@opensource.cirrus.com>
Cc: <alsa-devel@alsa-project.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/soc/codecs/wm_adsp.c | 37 +++++++------------------------------
 1 file changed, 7 insertions(+), 30 deletions(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index b26e6b825a90..8f301cb07745 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -731,41 +731,18 @@ static void wm_adsp2_init_debugfs(struct wm_adsp *dsp,
 	struct dentry *root = NULL;
 	int i;
 
-	if (!component->debugfs_root) {
-		adsp_err(dsp, "No codec debugfs root\n");
-		goto err;
-	}
-
 	root = debugfs_create_dir(dsp->name, component->debugfs_root);
 
-	if (!root)
-		goto err;
-
-	if (!debugfs_create_bool("booted", 0444, root, &dsp->booted))
-		goto err;
+	debugfs_create_bool("booted", 0444, root, &dsp->booted);
+	debugfs_create_bool("running", 0444, root, &dsp->running);
+	debugfs_create_x32("fw_id", 0444, root, &dsp->fw_id);
+	debugfs_create_x32("fw_version", 0444, root, &dsp->fw_id_version);
 
-	if (!debugfs_create_bool("running", 0444, root, &dsp->running))
-		goto err;
-
-	if (!debugfs_create_x32("fw_id", 0444, root, &dsp->fw_id))
-		goto err;
-
-	if (!debugfs_create_x32("fw_version", 0444, root, &dsp->fw_id_version))
-		goto err;
-
-	for (i = 0; i < ARRAY_SIZE(wm_adsp_debugfs_fops); ++i) {
-		if (!debugfs_create_file(wm_adsp_debugfs_fops[i].name,
-					 0444, root, dsp,
-					 &wm_adsp_debugfs_fops[i].fops))
-			goto err;
-	}
+	for (i = 0; i < ARRAY_SIZE(wm_adsp_debugfs_fops); ++i)
+		debugfs_create_file(wm_adsp_debugfs_fops[i].name, 0444, root,
+				    dsp, &wm_adsp_debugfs_fops[i].fops);
 
 	dsp->debugfs_root = root;
-	return;
-
-err:
-	debugfs_remove_recursive(root);
-	adsp_err(dsp, "Failed to create debugfs\n");
 }
 
 static void wm_adsp2_cleanup_debugfs(struct wm_adsp *dsp)
-- 
2.22.0

Re: [PATCH 3/5] sound: soc: codecs: wm_adsp: no need to check return value of debugfs_create functions

[Richard Fitzgerald]

On 14/06/19 10:47, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: <patches@opensource.cirrus.com>
> Cc: <alsa-devel@alsa-project.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   sound/soc/codecs/wm_adsp.c | 37 +++++++------------------------------
>   1 file changed, 7 insertions(+), 30 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
> index b26e6b825a90..8f301cb07745 100644
> --- a/sound/soc/codecs/wm_adsp.c
> +++ b/sound/soc/codecs/wm_adsp.c
> @@ -731,41 +731,18 @@ static void wm_adsp2_init_debugfs(struct wm_adsp *dsp,
>   	struct dentry *root = NULL;
>   	int i;
>   
> -	if (!component->debugfs_root) {
> -		adsp_err(dsp, "No codec debugfs root\n");
> -		goto err;
> -	}
> -
>   	root = debugfs_create_dir(dsp->name, component->debugfs_root);
>   
> -	if (!root)
> -		goto err;
> -
> -	if (!debugfs_create_bool("booted", 0444, root, &dsp->booted))
> -		goto err;
> +	debugfs_create_bool("booted", 0444, root, &dsp->booted);
> +	debugfs_create_bool("running", 0444, root, &dsp->running);
> +	debugfs_create_x32("fw_id", 0444, root, &dsp->fw_id);
> +	debugfs_create_x32("fw_version", 0444, root, &dsp->fw_id_version);
>   
> -	if (!debugfs_create_bool("running", 0444, root, &dsp->running))
> -		goto err;
> -
> -	if (!debugfs_create_x32("fw_id", 0444, root, &dsp->fw_id))
> -		goto err;
> -
> -	if (!debugfs_create_x32("fw_version", 0444, root, &dsp->fw_id_version))
> -		goto err;
> -
> -	for (i = 0; i < ARRAY_SIZE(wm_adsp_debugfs_fops); ++i) {
> -		if (!debugfs_create_file(wm_adsp_debugfs_fops[i].name,
> -					 0444, root, dsp,
> -					 &wm_adsp_debugfs_fops[i].fops))
> -			goto err;
> -	}
> +	for (i = 0; i < ARRAY_SIZE(wm_adsp_debugfs_fops); ++i)
> +		debugfs_create_file(wm_adsp_debugfs_fops[i].name, 0444, root,
> +				    dsp, &wm_adsp_debugfs_fops[i].fops);
>   
>   	dsp->debugfs_root = root;
> -	return;
> -
> -err:
> -	debugfs_remove_recursive(root);
> -	adsp_err(dsp, "Failed to create debugfs\n");
>   }
>   
>   static void wm_adsp2_cleanup_debugfs(struct wm_adsp *dsp)
> 

Reviewed-by: Richard Fitzgerald <rf@opensource.cirrus.com>

Applied "ASoC: wm_adsp: no need to check return value of debugfs_create functions" to the asoc tree

[Mark Brown]

The patch

   ASoC: wm_adsp: no need to check return value of debugfs_create functions

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.3

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 7f807f280964e31fb32fe6aaa263cfa2488236d8 Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Fri, 14 Jun 2019 11:47:54 +0200
Subject: [PATCH] ASoC: wm_adsp: no need to check return value of
 debugfs_create functions

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/wm_adsp.c | 37 +++++++------------------------------
 1 file changed, 7 insertions(+), 30 deletions(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index b26e6b825a90..8f301cb07745 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -731,41 +731,18 @@ static void wm_adsp2_init_debugfs(struct wm_adsp *dsp,
 	struct dentry *root = NULL;
 	int i;
 
-	if (!component->debugfs_root) {
-		adsp_err(dsp, "No codec debugfs root\n");
-		goto err;
-	}
-
 	root = debugfs_create_dir(dsp->name, component->debugfs_root);
 
-	if (!root)
-		goto err;
-
-	if (!debugfs_create_bool("booted", 0444, root, &dsp->booted))
-		goto err;
+	debugfs_create_bool("booted", 0444, root, &dsp->booted);
+	debugfs_create_bool("running", 0444, root, &dsp->running);
+	debugfs_create_x32("fw_id", 0444, root, &dsp->fw_id);
+	debugfs_create_x32("fw_version", 0444, root, &dsp->fw_id_version);
 
-	if (!debugfs_create_bool("running", 0444, root, &dsp->running))
-		goto err;
-
-	if (!debugfs_create_x32("fw_id", 0444, root, &dsp->fw_id))
-		goto err;
-
-	if (!debugfs_create_x32("fw_version", 0444, root, &dsp->fw_id_version))
-		goto err;
-
-	for (i = 0; i < ARRAY_SIZE(wm_adsp_debugfs_fops); ++i) {
-		if (!debugfs_create_file(wm_adsp_debugfs_fops[i].name,
-					 0444, root, dsp,
-					 &wm_adsp_debugfs_fops[i].fops))
-			goto err;
-	}
+	for (i = 0; i < ARRAY_SIZE(wm_adsp_debugfs_fops); ++i)
+		debugfs_create_file(wm_adsp_debugfs_fops[i].name, 0444, root,
+				    dsp, &wm_adsp_debugfs_fops[i].fops);
 
 	dsp->debugfs_root = root;
-	return;
-
-err:
-	debugfs_remove_recursive(root);
-	adsp_err(dsp, "Failed to create debugfs\n");
 }
 
 static void wm_adsp2_cleanup_debugfs(struct wm_adsp *dsp)
-- 
2.20.1

[PATCH 4/5] sound: soc: fsl: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Cc: Timur Tabi <timur@kernel.org>
Cc: Nicolin Chen <nicoleotsuka@gmail.com>
Cc: Xiubo Li <Xiubo.Lee@gmail.com>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: <alsa-devel@alsa-project.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 sound/soc/fsl/fsl_ssi.c     |  4 +---
 sound/soc/fsl/fsl_ssi.h     |  8 +++-----
 sound/soc/fsl/fsl_ssi_dbg.c | 18 ++++--------------
 sound/soc/fsl/imx-audmux.c  | 10 ++--------
 4 files changed, 10 insertions(+), 30 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 09b2967befd9..fa862af25c1a 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1582,9 +1582,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = fsl_ssi_debugfs_create(&ssi->dbg_stats, dev);
-	if (ret)
-		goto error_asoc_register;
+	fsl_ssi_debugfs_create(&ssi->dbg_stats, dev);
 
 	/* Initially configures SSI registers */
 	fsl_ssi_hw_init(ssi);
diff --git a/sound/soc/fsl/fsl_ssi.h b/sound/soc/fsl/fsl_ssi.h
index 0bdda608d414..db57cad80449 100644
--- a/sound/soc/fsl/fsl_ssi.h
+++ b/sound/soc/fsl/fsl_ssi.h
@@ -270,7 +270,6 @@ struct device;
 
 struct fsl_ssi_dbg {
 	struct dentry *dbg_dir;
-	struct dentry *dbg_stats;
 
 	struct {
 		unsigned int rfrc;
@@ -299,7 +298,7 @@ struct fsl_ssi_dbg {
 
 void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *ssi_dbg, u32 sisr);
 
-int fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg, struct device *dev);
+void fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg, struct device *dev);
 
 void fsl_ssi_debugfs_remove(struct fsl_ssi_dbg *ssi_dbg);
 
@@ -312,10 +311,9 @@ static inline void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *stats, u32 sisr)
 {
 }
 
-static inline int fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg,
-					 struct device *dev)
+static inline void fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg,
+					  struct device *dev)
 {
-	return 0;
 }
 
 static inline void fsl_ssi_debugfs_remove(struct fsl_ssi_dbg *ssi_dbg)
diff --git a/sound/soc/fsl/fsl_ssi_dbg.c b/sound/soc/fsl/fsl_ssi_dbg.c
index 6f6294149476..2a20ee23dc52 100644
--- a/sound/soc/fsl/fsl_ssi_dbg.c
+++ b/sound/soc/fsl/fsl_ssi_dbg.c
@@ -126,25 +126,15 @@ static int fsl_ssi_stats_show(struct seq_file *s, void *unused)
 
 DEFINE_SHOW_ATTRIBUTE(fsl_ssi_stats);
 
-int fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg, struct device *dev)
+void fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg, struct device *dev)
 {
 	ssi_dbg->dbg_dir = debugfs_create_dir(dev_name(dev), NULL);
-	if (!ssi_dbg->dbg_dir)
-		return -ENOMEM;
 
-	ssi_dbg->dbg_stats = debugfs_create_file("stats", 0444,
-						 ssi_dbg->dbg_dir, ssi_dbg,
-						 &fsl_ssi_stats_fops);
-	if (!ssi_dbg->dbg_stats) {
-		debugfs_remove(ssi_dbg->dbg_dir);
-		return -ENOMEM;
-	}
-
-	return 0;
+	debugfs_create_file("stats", 0444, ssi_dbg->dbg_dir, ssi_dbg,
+			    &fsl_ssi_stats_fops);
 }
 
 void fsl_ssi_debugfs_remove(struct fsl_ssi_dbg *ssi_dbg)
 {
-	debugfs_remove(ssi_dbg->dbg_stats);
-	debugfs_remove(ssi_dbg->dbg_dir);
+	debugfs_remove_recursive(ssi_dbg->dbg_dir);
 }
diff --git a/sound/soc/fsl/imx-audmux.c b/sound/soc/fsl/imx-audmux.c
index 04e59e66711d..b2351cd33b0f 100644
--- a/sound/soc/fsl/imx-audmux.c
+++ b/sound/soc/fsl/imx-audmux.c
@@ -141,17 +141,11 @@ static void audmux_debugfs_init(void)
 	char buf[20];
 
 	audmux_debugfs_root = debugfs_create_dir("audmux", NULL);
-	if (!audmux_debugfs_root) {
-		pr_warning("Failed to create AUDMUX debugfs root\n");
-		return;
-	}
 
 	for (i = 0; i < MX31_AUDMUX_PORT7_SSI_PINS_7 + 1; i++) {
 		snprintf(buf, sizeof(buf), "ssi%lu", i);
-		if (!debugfs_create_file(buf, 0444, audmux_debugfs_root,
-					 (void *)i, &audmux_debugfs_fops))
-			pr_warning("Failed to create AUDMUX port %lu debugfs file\n",
-				   i);
+		debugfs_create_file(buf, 0444, audmux_debugfs_root,
+				    (void *)i, &audmux_debugfs_fops);
 	}
 }
 
-- 
2.22.0

Applied "ASoC: fsl: no need to check return value of debugfs_create functions" to the asoc tree

[Mark Brown]

The patch

   ASoC: fsl: no need to check return value of debugfs_create functions

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.3

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 227ab8baa15bdd7a48acfb7b61c52a7a5eb87e72 Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Fri, 14 Jun 2019 11:47:55 +0200
Subject: [PATCH] ASoC: fsl: no need to check return value of debugfs_create
 functions

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/fsl/fsl_ssi.c     |  4 +---
 sound/soc/fsl/fsl_ssi.h     |  8 +++-----
 sound/soc/fsl/fsl_ssi_dbg.c | 18 ++++--------------
 sound/soc/fsl/imx-audmux.c  | 10 ++--------
 4 files changed, 10 insertions(+), 30 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 09b2967befd9..fa862af25c1a 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1582,9 +1582,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = fsl_ssi_debugfs_create(&ssi->dbg_stats, dev);
-	if (ret)
-		goto error_asoc_register;
+	fsl_ssi_debugfs_create(&ssi->dbg_stats, dev);
 
 	/* Initially configures SSI registers */
 	fsl_ssi_hw_init(ssi);
diff --git a/sound/soc/fsl/fsl_ssi.h b/sound/soc/fsl/fsl_ssi.h
index 0bdda608d414..db57cad80449 100644
--- a/sound/soc/fsl/fsl_ssi.h
+++ b/sound/soc/fsl/fsl_ssi.h
@@ -270,7 +270,6 @@ struct device;
 
 struct fsl_ssi_dbg {
 	struct dentry *dbg_dir;
-	struct dentry *dbg_stats;
 
 	struct {
 		unsigned int rfrc;
@@ -299,7 +298,7 @@ struct fsl_ssi_dbg {
 
 void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *ssi_dbg, u32 sisr);
 
-int fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg, struct device *dev);
+void fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg, struct device *dev);
 
 void fsl_ssi_debugfs_remove(struct fsl_ssi_dbg *ssi_dbg);
 
@@ -312,10 +311,9 @@ static inline void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *stats, u32 sisr)
 {
 }
 
-static inline int fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg,
-					 struct device *dev)
+static inline void fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg,
+					  struct device *dev)
 {
-	return 0;
 }
 
 static inline void fsl_ssi_debugfs_remove(struct fsl_ssi_dbg *ssi_dbg)
diff --git a/sound/soc/fsl/fsl_ssi_dbg.c b/sound/soc/fsl/fsl_ssi_dbg.c
index 6f6294149476..2a20ee23dc52 100644
--- a/sound/soc/fsl/fsl_ssi_dbg.c
+++ b/sound/soc/fsl/fsl_ssi_dbg.c
@@ -126,25 +126,15 @@ static int fsl_ssi_stats_show(struct seq_file *s, void *unused)
 
 DEFINE_SHOW_ATTRIBUTE(fsl_ssi_stats);
 
-int fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg, struct device *dev)
+void fsl_ssi_debugfs_create(struct fsl_ssi_dbg *ssi_dbg, struct device *dev)
 {
 	ssi_dbg->dbg_dir = debugfs_create_dir(dev_name(dev), NULL);
-	if (!ssi_dbg->dbg_dir)
-		return -ENOMEM;
 
-	ssi_dbg->dbg_stats = debugfs_create_file("stats", 0444,
-						 ssi_dbg->dbg_dir, ssi_dbg,
-						 &fsl_ssi_stats_fops);
-	if (!ssi_dbg->dbg_stats) {
-		debugfs_remove(ssi_dbg->dbg_dir);
-		return -ENOMEM;
-	}
-
-	return 0;
+	debugfs_create_file("stats", 0444, ssi_dbg->dbg_dir, ssi_dbg,
+			    &fsl_ssi_stats_fops);
 }
 
 void fsl_ssi_debugfs_remove(struct fsl_ssi_dbg *ssi_dbg)
 {
-	debugfs_remove(ssi_dbg->dbg_stats);
-	debugfs_remove(ssi_dbg->dbg_dir);
+	debugfs_remove_recursive(ssi_dbg->dbg_dir);
 }
diff --git a/sound/soc/fsl/imx-audmux.c b/sound/soc/fsl/imx-audmux.c
index 04e59e66711d..b2351cd33b0f 100644
--- a/sound/soc/fsl/imx-audmux.c
+++ b/sound/soc/fsl/imx-audmux.c
@@ -141,17 +141,11 @@ static void audmux_debugfs_init(void)
 	char buf[20];
 
 	audmux_debugfs_root = debugfs_create_dir("audmux", NULL);
-	if (!audmux_debugfs_root) {
-		pr_warning("Failed to create AUDMUX debugfs root\n");
-		return;
-	}
 
 	for (i = 0; i < MX31_AUDMUX_PORT7_SSI_PINS_7 + 1; i++) {
 		snprintf(buf, sizeof(buf), "ssi%lu", i);
-		if (!debugfs_create_file(buf, 0444, audmux_debugfs_root,
-					 (void *)i, &audmux_debugfs_fops))
-			pr_warning("Failed to create AUDMUX port %lu debugfs file\n",
-				   i);
+		debugfs_create_file(buf, 0444, audmux_debugfs_root,
+				    (void *)i, &audmux_debugfs_fops);
 	}
 }
 
-- 
2.20.1

[PATCH 5/5] sound: soc: core: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Also, there is no need to store the individual debugfs file name, just
remove the whole directory all at once, saving a local variable.

Note, the soc-pcm "state" file has now moved to a subdirectory, as it is
only a good idea to save the dentries for debugfs directories, not
individual files, as the individual file debugfs functions are changing
to not return a dentry.

Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: alsa-devel@alsa-project.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/sound/soc.h  |  1 -
 sound/soc/soc-core.c | 31 ++++++-------------------------
 sound/soc/soc-dapm.c | 26 ++++----------------------
 sound/soc/soc-pcm.c  | 14 ++++----------
 4 files changed, 14 insertions(+), 58 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 482b4ea87c3c..4acd8f6bf460 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1177,7 +1177,6 @@ struct snd_soc_card {
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs_card_root;
-	struct dentry *debugfs_pop_time;
 #endif
 	u32 pop_time;
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 2403bec2fccf..8d047e0a3e74 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -207,23 +207,11 @@ DEFINE_SHOW_ATTRIBUTE(component_list);
 
 static void soc_init_card_debugfs(struct snd_soc_card *card)
 {
-	if (!snd_soc_debugfs_root)
-		return;
-
 	card->debugfs_card_root = debugfs_create_dir(card->name,
 						     snd_soc_debugfs_root);
-	if (!card->debugfs_card_root) {
-		dev_warn(card->dev,
-			 "ASoC: Failed to create card debugfs directory\n");
-		return;
-	}
 
-	card->debugfs_pop_time = debugfs_create_u32("dapm_pop_time", 0644,
-						    card->debugfs_card_root,
-						    &card->pop_time);
-	if (!card->debugfs_pop_time)
-		dev_warn(card->dev,
-			 "ASoC: Failed to create pop time debugfs file\n");
+	debugfs_create_u32("dapm_pop_time", 0644, card->debugfs_card_root,
+			   &card->pop_time);
 }
 
 static void soc_cleanup_card_debugfs(struct snd_soc_card *card)
@@ -234,19 +222,12 @@ static void soc_cleanup_card_debugfs(struct snd_soc_card *card)
 static void snd_soc_debugfs_init(void)
 {
 	snd_soc_debugfs_root = debugfs_create_dir("asoc", NULL);
-	if (IS_ERR_OR_NULL(snd_soc_debugfs_root)) {
-		pr_warn("ASoC: Failed to create debugfs directory\n");
-		snd_soc_debugfs_root = NULL;
-		return;
-	}
 
-	if (!debugfs_create_file("dais", 0444, snd_soc_debugfs_root, NULL,
-				 &dai_list_fops))
-		pr_warn("ASoC: Failed to create DAI list debugfs file\n");
+	debugfs_create_file("dais", 0444, snd_soc_debugfs_root, NULL,
+			    &dai_list_fops);
 
-	if (!debugfs_create_file("components", 0444, snd_soc_debugfs_root, NULL,
-				 &component_list_fops))
-		pr_warn("ASoC: Failed to create component list debugfs file\n");
+	debugfs_create_file("components", 0444, snd_soc_debugfs_root, NULL,
+			    &component_list_fops);
 }
 
 static void snd_soc_debugfs_exit(void)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 81a7a12196ff..bc43060e8a70 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -2153,42 +2153,24 @@ static const struct file_operations dapm_bias_fops = {
 void snd_soc_dapm_debugfs_init(struct snd_soc_dapm_context *dapm,
 	struct dentry *parent)
 {
-	struct dentry *d;
-
 	if (!parent)
 		return;
 
 	dapm->debugfs_dapm = debugfs_create_dir("dapm", parent);
 
-	if (!dapm->debugfs_dapm) {
-		dev_warn(dapm->dev,
-		       "ASoC: Failed to create DAPM debugfs directory\n");
-		return;
-	}
-
-	d = debugfs_create_file("bias_level", 0444,
-				dapm->debugfs_dapm, dapm,
-				&dapm_bias_fops);
-	if (!d)
-		dev_warn(dapm->dev,
-			 "ASoC: Failed to create bias level debugfs file\n");
+	debugfs_create_file("bias_level", 0444, dapm->debugfs_dapm, dapm,
+			    &dapm_bias_fops);
 }
 
 static void dapm_debugfs_add_widget(struct snd_soc_dapm_widget *w)
 {
 	struct snd_soc_dapm_context *dapm = w->dapm;
-	struct dentry *d;
 
 	if (!dapm->debugfs_dapm || !w->name)
 		return;
 
-	d = debugfs_create_file(w->name, 0444,
-				dapm->debugfs_dapm, w,
-				&dapm_widget_power_fops);
-	if (!d)
-		dev_warn(w->dapm->dev,
-			"ASoC: Failed to create %s debugfs file\n",
-			w->name);
+	debugfs_create_file(w->name, 0444, dapm->debugfs_dapm, w,
+			    &dapm_widget_power_fops);
 }
 
 static void dapm_debugfs_cleanup(struct snd_soc_dapm_context *dapm)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 0a4f60c7a188..00ab01377909 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1258,9 +1258,9 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
 			stream ? "<-" : "->", be->dai_link->name);
 
 #ifdef CONFIG_DEBUG_FS
-	if (fe->debugfs_dpcm_root)
-		dpcm->debugfs_state = debugfs_create_u32(be->dai_link->name, 0644,
-				fe->debugfs_dpcm_root, &dpcm->state);
+	dpcm->debugfs_state = debugfs_create_dir(be->dai_link->name,
+						 fe->debugfs_dpcm_root);
+	debugfs_create_u32("state", 0644, dpcm->debugfs_state, &dpcm->state);
 #endif
 	return 1;
 }
@@ -1315,7 +1315,7 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
 		dpcm_be_reparent(fe, dpcm->be, stream);
 
 #ifdef CONFIG_DEBUG_FS
-		debugfs_remove(dpcm->debugfs_state);
+		debugfs_remove_recursive(dpcm->debugfs_state);
 #endif
 		spin_lock_irqsave(&fe->card->dpcm_lock, flags);
 		list_del(&dpcm->list_be);
@@ -3449,12 +3449,6 @@ void soc_dpcm_debugfs_add(struct snd_soc_pcm_runtime *rtd)
 
 	rtd->debugfs_dpcm_root = debugfs_create_dir(rtd->dai_link->name,
 			rtd->card->debugfs_card_root);
-	if (!rtd->debugfs_dpcm_root) {
-		dev_dbg(rtd->dev,
-			 "ASoC: Failed to create dpcm debugfs directory %s\n",
-			 rtd->dai_link->name);
-		return;
-	}
 
 	debugfs_create_file("state", 0444, rtd->debugfs_dpcm_root,
 			    rtd, &dpcm_state_fops);
-- 
2.22.0

Re: [PATCH 5/5] sound: soc: core: no need to check return value of debugfs_create functions

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1410 bytes --]

On Fri, Jun 14, 2019 at 11:47:56AM +0200, Greg Kroah-Hartman wrote:

> Note, the soc-pcm "state" file has now moved to a subdirectory, as it is
> only a good idea to save the dentries for debugfs directories, not
> individual files, as the individual file debugfs functions are changing
> to not return a dentry.

It'd be better to split this out into a separate change for ease of
review.

> -	d = debugfs_create_file(w->name, 0444,
> -				dapm->debugfs_dapm, w,
> -				&dapm_widget_power_fops);
> -	if (!d)
> -		dev_warn(w->dapm->dev,
> -			"ASoC: Failed to create %s debugfs file\n",
> -			w->name);
> +	debugfs_create_file(w->name, 0444, dapm->debugfs_dapm, w,
> +			    &dapm_widget_power_fops);

The majority of this is removing error prints rather than code that
actively does something different.  If this was like kmalloc() where the
API is itself reported errors then this wouldn't be an issue but unless
I'm missing something debugfs fails silently so this means that if
something goes wrong it's going to be harder for the user to figure out
where the debugfs files they wanted to check went to.  I'm guessing you
don't want to add error prints in debugfs itself so I'd rather they
stayed here.

Yes, the error check is looking for NULL not an error pointer - it was
correct when written but I see that the debugfs API changed earlier this
year to return error pointers so we ought to fix that up.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 5/5] sound: soc: core: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Fri, Jun 14, 2019 at 04:34:05PM +0100, Mark Brown wrote:
> On Fri, Jun 14, 2019 at 11:47:56AM +0200, Greg Kroah-Hartman wrote:
> 
> > Note, the soc-pcm "state" file has now moved to a subdirectory, as it is
> > only a good idea to save the dentries for debugfs directories, not
> > individual files, as the individual file debugfs functions are changing
> > to not return a dentry.
> 
> It'd be better to split this out into a separate change for ease of
> review.
> 
> > -	d = debugfs_create_file(w->name, 0444,
> > -				dapm->debugfs_dapm, w,
> > -				&dapm_widget_power_fops);
> > -	if (!d)
> > -		dev_warn(w->dapm->dev,
> > -			"ASoC: Failed to create %s debugfs file\n",
> > -			w->name);
> > +	debugfs_create_file(w->name, 0444, dapm->debugfs_dapm, w,
> > +			    &dapm_widget_power_fops);
> 
> The majority of this is removing error prints rather than code that
> actively does something different.  If this was like kmalloc() where the
> API is itself reported errors then this wouldn't be an issue but unless
> I'm missing something debugfs fails silently so this means that if
> something goes wrong it's going to be harder for the user to figure out
> where the debugfs files they wanted to check went to.  I'm guessing you
> don't want to add error prints in debugfs itself so I'd rather they
> stayed here.
> 
> Yes, the error check is looking for NULL not an error pointer - it was
> correct when written but I see that the debugfs API changed earlier this
> year to return error pointers so we ought to fix that up.

No, the long-term goal is for debugfs_create_file() to just return void.
I have already done enough cleanups in my local tree to do that already
for many of the helper functions.

No one should care one bit if a debugfs file succeeds or not, no logic
should ever change, nor should it matter.  debugfs is for debugging
kernel code, not for anything that anyone should ever rely on.

So yes, this patch does remove a bunch of error messages (that as you
point out can never be triggered), but the main goal here is to not
check the return value of the function at all, which is what this patch
does.

thanks,

greg k-h

Re: [PATCH 5/5] sound: soc: core: no need to check return value of debugfs_create functions

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2470 bytes --]

On Fri, Jun 14, 2019 at 06:13:39PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jun 14, 2019 at 04:34:05PM +0100, Mark Brown wrote:
> > On Fri, Jun 14, 2019 at 11:47:56AM +0200, Greg Kroah-Hartman wrote:

> > The majority of this is removing error prints rather than code that
> > actively does something different.  If this was like kmalloc() where the
> > API is itself reported errors then this wouldn't be an issue but unless

> No, the long-term goal is for debugfs_create_file() to just return void.
> I have already done enough cleanups in my local tree to do that already
> for many of the helper functions.

> No one should care one bit if a debugfs file succeeds or not, no logic
> should ever change, nor should it matter.  debugfs is for debugging
> kernel code, not for anything that anyone should ever rely on.

I don't think this would be a helpful change.  Nobody should care about
debugfs file creation for *production* use but that doesn't mean that
people using it for debugging shouldn't care about it.  Debugging tools
that are fragile can be very misleading for developers and hence
intensely frustrating; it's never fun to be building on quicksand.  This
is particularly true for code like here where the core is providing a
bunch of internal data that is hopefully useful to people developing
drivers or system integrations, the users have a hopefully more black
box view of the core than someone directly working on the core would.
If that view of the data ends up being corrupted because some of the
files or directories don't get created that is something we should be
flagging up to people to try to help them avoid being mislead by what
they are seeing.  Developers need to be able to trust their debugging
tools.

> So yes, this patch does remove a bunch of error messages (that as you
> point out can never be triggered), but the main goal here is to not
> check the return value of the function at all, which is what this patch
> does.

Like I said in reply to the other patch the error checking here did the
right thing up until January when debugfs was changed to return error
pointers instead of NULL.  I know that I've found this error checking to
be helpful in the past, not having it would be a loss.  

If there were some visible error reporting in debugfs I'd not be
bothered about this quite so much (though it'd still be less clear) but
there isn't so it really feels like a step back.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/5] sound: SoC: sof: no need to check return value of debugfs_create functions

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 383 bytes --]

On Fri, Jun 14, 2019 at 11:47:52AM +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.

Please use subject lines matching the style for the subsystem.  This
makes it easier for people to identify relevant patches.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/5] sound: SoC: sof: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Fri, Jun 14, 2019 at 03:59:33PM +0100, Mark Brown wrote:
> On Fri, Jun 14, 2019 at 11:47:52AM +0200, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value.  The function can work or not, but the code logic should
> > never do something different based on this.
> 
> Please use subject lines matching the style for the subsystem.  This
> makes it easier for people to identify relevant patches.

Sorry about that, tough to catch when doing 100+ different patches all
across the kernel for this type of thing...

greg k-h

Re: [PATCH 1/5] sound: SoC: sof: no need to check return value of debugfs_create functions

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1752 bytes --]

On Fri, Jun 14, 2019 at 11:47:52AM +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.

> +++ b/sound/soc/sof/debug.c
> @@ -77,8 +77,7 @@ static ssize_t sof_dfsentry_read(struct file *file, char __user *buffer,
>  		if (!pm_runtime_active(sdev->dev) &&
>  		    dfse->access_type == SOF_DEBUGFS_ACCESS_D0_ONLY) {
>  			dev_err(sdev->dev,
> -				"error: debugfs entry %s cannot be read in DSP D3\n",
> -				dfse->dfsentry->d_name.name);
> +				"error: debugfs entry cannot be read in DSP D3\n");
>  			kfree(buf);
>  			return -EINVAL;
>  		}

This appears to be an unrelated change with no description in the
changelog, please split it out into a separate change with a description
of the change.

> @@ -119,13 +119,8 @@ static int trace_debugfs_create(struct snd_sof_dev *sdev)
>  	dfse->size = sdev->dmatb.bytes;
>  	dfse->sdev = sdev;
>  
> -	dfse->dfsentry = debugfs_create_file("trace", 0444, sdev->debugfs_root,
> -					     dfse, &sof_dfs_trace_fops);
> -	if (!dfse->dfsentry) {
> -		/* can't rely on debugfs, only log error and keep going */
> -		dev_err(sdev->dev,
> -			"error: cannot create debugfs entry for trace\n");
> -	}
> +	debugfs_create_file("trace", 0444, sdev->debugfs_root, dfse,
> +			    &sof_dfs_trace_fops);

I might be missing something but I can't see any error logging in
debugfs_create_file() so this isn't equivalent (though the current code
is broken, it should be using IS_ERR()).  Logging creation failures is
helpful to developers trying to figure out what happened to the trace
files they're trying to use.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/5] sound: SoC: sof: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Fri, Jun 14, 2019 at 04:14:10PM +0100, Mark Brown wrote:
> On Fri, Jun 14, 2019 at 11:47:52AM +0200, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value.  The function can work or not, but the code logic should
> > never do something different based on this.
> 
> > +++ b/sound/soc/sof/debug.c
> > @@ -77,8 +77,7 @@ static ssize_t sof_dfsentry_read(struct file *file, char __user *buffer,
> >  		if (!pm_runtime_active(sdev->dev) &&
> >  		    dfse->access_type == SOF_DEBUGFS_ACCESS_D0_ONLY) {
> >  			dev_err(sdev->dev,
> > -				"error: debugfs entry %s cannot be read in DSP D3\n",
> > -				dfse->dfsentry->d_name.name);
> > +				"error: debugfs entry cannot be read in DSP D3\n");
> >  			kfree(buf);
> >  			return -EINVAL;
> >  		}
> 
> This appears to be an unrelated change with no description in the
> changelog, please split it out into a separate change with a description
> of the change.

The whole "dfsentry" variable is now gone, so it is very related.  Why
split this out?

> > @@ -119,13 +119,8 @@ static int trace_debugfs_create(struct snd_sof_dev *sdev)
> >  	dfse->size = sdev->dmatb.bytes;
> >  	dfse->sdev = sdev;
> >  
> > -	dfse->dfsentry = debugfs_create_file("trace", 0444, sdev->debugfs_root,
> > -					     dfse, &sof_dfs_trace_fops);
> > -	if (!dfse->dfsentry) {
> > -		/* can't rely on debugfs, only log error and keep going */
> > -		dev_err(sdev->dev,
> > -			"error: cannot create debugfs entry for trace\n");
> > -	}
> > +	debugfs_create_file("trace", 0444, sdev->debugfs_root, dfse,
> > +			    &sof_dfs_trace_fops);
> 
> I might be missing something but I can't see any error logging in
> debugfs_create_file() so this isn't equivalent (though the current code
> is broken, it should be using IS_ERR()).  Logging creation failures is
> helpful to developers trying to figure out what happened to the trace
> files they're trying to use.

There is no need to log anything in in-kernel, working code.  If a
developer wants to do this on their own when writing the code the first
time and debugging it, great, but for stuff we know works, there's no
need for being noisy.

Also, the check is incorrect, there's no way for this function to return
NULL, so that code today, will never trigger.  So obviously no one
counted on this message anyway :)

Just call the function and move on, like the rest of the kernel is being
converted over to do.

thanks,

greg k-h

Re: [PATCH 1/5] sound: SoC: sof: no need to check return value of debugfs_create functions

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2497 bytes --]

On Fri, Jun 14, 2019 at 05:27:04PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jun 14, 2019 at 04:14:10PM +0100, Mark Brown wrote:
> > On Fri, Jun 14, 2019 at 11:47:52AM +0200, Greg Kroah-Hartman wrote:

> > >  			dev_err(sdev->dev,
> > > -				"error: debugfs entry %s cannot be read in DSP D3\n",
> > > -				dfse->dfsentry->d_name.name);
> > > +				"error: debugfs entry cannot be read in DSP D3\n");

> > This appears to be an unrelated change with no description in the
> > changelog, please split it out into a separate change with a description
> > of the change.

> The whole "dfsentry" variable is now gone, so it is very related.  Why
> split this out?

The removal of the variable isn't mentioned in the changelog at all or
otherwise explained in what *should* be a mostly mechanical change.
When a patch is doing something that doesn't match the changelog that
sets off alarm bells, and when there's something that's mostly lots of
repetitive mechanical changes with some more other reorganization
mixed in it's a lot easier to review if those other changes are split
out.

> > I might be missing something but I can't see any error logging in
> > debugfs_create_file() so this isn't equivalent (though the current code
> > is broken, it should be using IS_ERR()).  Logging creation failures is
> > helpful to developers trying to figure out what happened to the trace
> > files they're trying to use.

> There is no need to log anything in in-kernel, working code.  If a
> developer wants to do this on their own when writing the code the first
> time and debugging it, great, but for stuff we know works, there's no
> need for being noisy.

If it helps maintainability for people to get diagnostics I'm all for
it; it's not like this is a hot path or anything.

> Also, the check is incorrect, there's no way for this function to return
> NULL, so that code today, will never trigger.  So obviously no one
> counted on this message anyway :)

I went and checked into this having seen that the core code (which I
definitely know used to trigger) also checks for NULL and discovered
that the reason this happens is that in January you applied a commit
which changed the return value from NULL to an error pointer which broke
any caller doing error checking.  That's not exactly the same thing as
nobody ever finding something useful.

> Just call the function and move on, like the rest of the kernel is being
> converted over to do.

This is something you've unilaterally decided to do.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]