[alsa-devel] [PATCH 1/5] sound: SoC: sof: 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.
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; }
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; }
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
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)
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
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)
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); } }
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); } }
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);
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.
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
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.
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.
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
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.
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
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.
participants (5)
-
Cezary Rojewski -
Greg Kroah-Hartman -
Mark Brown -
Richard Fitzgerald -
Takashi Iwai