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

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

[Greg Kroah-Hartman]

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: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/hwtracing/coresight/coresight-cpu-debug.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
index e8819d750938..6446ed69ab2f 100644
--- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -525,23 +525,12 @@ static const struct file_operations debug_func_knob_fops = {
 
 static int debug_func_init(void)
 {
-	struct dentry *file;
 	int ret;
 
 	/* Create debugfs node */
 	debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
-	if (!debug_debugfs_dir) {
-		pr_err("%s: unable to create debugfs directory\n", __func__);
-		return -ENOMEM;
-	}
-
-	file = debugfs_create_file("enable", 0644, debug_debugfs_dir, NULL,
-				   &debug_func_knob_fops);
-	if (!file) {
-		pr_err("%s: unable to create enable knob file\n", __func__);
-		ret = -ENOMEM;
-		goto err;
-	}
+	debugfs_create_file("enable", 0644, debug_debugfs_dir, NULL,
+			    &debug_func_knob_fops);
 
 	/* Register function to be called for panic */
 	ret = atomic_notifier_chain_register(&panic_notifier_list,
-- 
2.22.0


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

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

[Mathieu Poirier]

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.

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?

Thanks,
Mathieu

>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/hwtracing/coresight/coresight-cpu-debug.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> index e8819d750938..6446ed69ab2f 100644
> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -525,23 +525,12 @@ static const struct file_operations debug_func_knob_fops = {
>
>  static int debug_func_init(void)
>  {
> -       struct dentry *file;
>         int ret;
>
>         /* Create debugfs node */
>         debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> -       if (!debug_debugfs_dir) {
> -               pr_err("%s: unable to create debugfs directory\n", __func__);
> -               return -ENOMEM;
> -       }
> -
> -       file = debugfs_create_file("enable", 0644, debug_debugfs_dir, NULL,
> -                                  &debug_func_knob_fops);
> -       if (!file) {
> -               pr_err("%s: unable to create enable knob file\n", __func__);
> -               ret = -ENOMEM;
> -               goto err;
> -       }
> +       debugfs_create_file("enable", 0644, debug_debugfs_dir, NULL,
> +                           &debug_func_knob_fops);
>
>         /* Register function to be called for panic */
>         ret = atomic_notifier_chain_register(&panic_notifier_list,
> --
> 2.22.0
>

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

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

[Greg Kroah-Hartman]

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
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.

thanks,

greg k-h

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

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

[Greg Kroah-Hartman]

On Tue, Jun 18, 2019 at 07:46:37PM +0200, Greg Kroah-Hartman 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
> 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 :)

Oh, forgot the pointer to the tree:
	https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/log/?h=debugfs_cleanup
this is all public, just not in linux-next as it's being fed through 50+
different subsystem trees.

thanks,

greg k-h

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

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

[Mathieu Poirier]

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>

Regards,
Mathieu

>
> thanks,
>
> greg k-h

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

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

[Greg Kroah-Hartman]

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