Re: [PATCH] coresight: cpu-debug: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Jun 18, 2019 at 01:26:12PM -0600, Mathieu Poirier wrote:
> On Tue, 18 Jun 2019 at 11:46, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jun 18, 2019 at 11:23:25AM -0600, Mathieu Poirier wrote:
> > > Hi Greg,
> > >
> > > On Tue, 18 Jun 2019 at 09:52, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> 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.
> > >
> > > Looking around in the kernel there is no shortage of instances where
> > > the return value of debugfs functions are checked and the logic
> > > altered based on these values.  But there are also just as many that
> > > don't...  It also seems counter intuitive to ignore the return value
> > > of any function, something that in most case is guaranteed to raise
> > > admonition.
> >
> > In my tree, those instances are almost all gone.  I've also posted over
> > 100+ patches in the past few weeks to clean this up.
> >
> > > That being said I am sure there is a good reason to support your
> > > position - would you mind expanding a little so that I can follow?
> >
> > No kernel code should ever care if debugfs works or not.  No user code
> > should ever require it for normal operation either.  debugfs was written
> > to be simple and easy to use, no need to check any return values at all.
> >
> > Any return value of a debugfs call can be fed back into another call
> > with no issues at all.
> >
> > Also, due to some debugfs core changes a few kernel releases ago, the
> > checks:
> >         if (!debug_debugfs_dir) {
> > ...
> >         if (!file) {
> > can never trigger as debugfs_create_dir() or debugfs_create_file() can
> > never return NULL (and in the past, it almost never would either).  So
> 
> That is the rational I was looking for.
> 
> > as it is, that code isn't correct anyway (my fault, I know, hey, I'm
> > trying to fix it!)
> >
> > I'm trying to make things simple, and easy, and impossible to get wrong.
> > I know it goes against the normal "robust" kernel development mentality,
> > but there is no need to ever care about debugfs at all.
> >
> > The reason I started all of this is that we have found places where
> > userspace, and the kernel, was depending on the proper operation of
> > debugfs.  In one horrid example, a device would not display the batter
> > level if debugfs was disabled.  In another case, the kernel was actually
> > relying on a debugfs call to fail in order to handle some logic the
> > subsystem should have been doing on its own.  All of that has now been
> > cleaned up, and I am working on making debugfs just not return any
> > values at all to prevent this type of mess happening again.
> >
> > And hey, I am removing code, here's my current tree as a diff from
> > what is not already merged into linux-next:
> >          301 files changed, 1394 insertions(+), 4637 deletions(-)
> > that's always a good thing :)
> >
> > Hopefully this helps explain things better.
> 
> It does - thanks for taking the time to write all this.
> 
> Do you want me to take the patch through my tree (only to see it
> coming back to you later this week) or you'll add it directly to
> yours?  In the latter case:
> 
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Tested-by: Mathieu Poirier <mathieu.poirier@linaro.org>

I can just take it directly, thanks!

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel