| Message ID | 20190614094756.2965-1-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 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 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 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 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.
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: 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(-)