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.