| Message ID | 20190614094756.2965-2-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] sound: SoC: sof: no need to check return value of debugfs_create functions | expand |
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; > } >
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
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
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
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
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
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.
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
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; }
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(-)