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

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20190614/8c321f8e/attachment.sig>