[PATCH] spi: remove return value check of debugfs_create_dir()

[PATCH] spi: remove return value check of debugfs_create_dir()

[Yingsha Xu]

Smatch complains that:
dw_spi_debugfs_init() warn: 'dws->debugfs' is an error pointer
 or valid

Debugfs checks are generally not supposed to be checked for errors
and it is not necessary here.

Just delete the dead code.

Signed-off-by: Yingsha Xu <ysxu@hust.edu.cn>
Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
---
 drivers/spi/spi-dw-core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index c3bfb6c84cab..64eb6dcdaac3 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -63,8 +63,6 @@ static int dw_spi_debugfs_init(struct dw_spi *dws)
 
 	snprintf(name, 32, "dw_spi%d", dws->master->bus_num);
 	dws->debugfs = debugfs_create_dir(name, NULL);
-	if (!dws->debugfs)
-		return -ENOMEM;
 
 	dws->regset.regs = dw_spi_dbgfs_regs;
 	dws->regset.nregs = ARRAY_SIZE(dw_spi_dbgfs_regs);
-- 
2.17.1

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Sun, Apr 23, 2023 at 02:11:54PM +0800, Yingsha Xu wrote:
> Smatch complains that:
> dw_spi_debugfs_init() warn: 'dws->debugfs' is an error pointer
>  or valid
> 
> Debugfs checks are generally not supposed to be checked for errors
> and it is not necessary here.
> 
> Just delete the dead code.

This is very much a question of taste with a range of use cases
available.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Peter Enderborg]

On 4/24/23 13:54, Mark Brown wrote:
> On Sun, Apr 23, 2023 at 02:11:54PM +0800, Yingsha Xu wrote:
>> Smatch complains that:
>> dw_spi_debugfs_init() warn: 'dws->debugfs' is an error pointer
>>  or valid
>>
>> Debugfs checks are generally not supposed to be checked for errors
>> and it is not necessary here.
>>
>> Just delete the dead code.
> 
> This is very much a question of taste with a range of use cases
> available.

No. 
https://lkml.iu.edu/hypermail/linux/kernel/1901.2/05993.html
We can do things with the debug information without filesystem enabled.

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Mon, Apr 24, 2023 at 02:20:45PM +0200, Peter Enderborg wrote:
> On 4/24/23 13:54, Mark Brown wrote:

> > This is very much a question of taste with a range of use cases
> > available.

> No. 
> https://lkml.iu.edu/hypermail/linux/kernel/1901.2/05993.html
> We can do things with the debug information without filesystem enabled.

There's issues with partially created debugfs structures getting in the
way of people trying to debug things, just completely ignoring all
errors can create confusion as the diagnostic information people believe
is being shown to them ends up being partial or mistructured without any
indication that this has happened.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Greg Kroah-Hartman]

On Mon, Apr 24, 2023 at 01:32:14PM +0100, Mark Brown wrote:
> On Mon, Apr 24, 2023 at 02:20:45PM +0200, Peter Enderborg wrote:
> > On 4/24/23 13:54, Mark Brown wrote:
> 
> > > This is very much a question of taste with a range of use cases
> > > available.
> 
> > No. 
> > https://lkml.iu.edu/hypermail/linux/kernel/1901.2/05993.html

Please use lore.kernel.org links in the future.

> > We can do things with the debug information without filesystem enabled.

What exactly do you mean by this?

> There's issues with partially created debugfs structures getting in the
> way of people trying to debug things, just completely ignoring all
> errors can create confusion as the diagnostic information people believe
> is being shown to them ends up being partial or mistructured without any
> indication that this has happened.

How do you end up with partially created debugfs structures?

thanks,

greg k-h

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

On Mon, Apr 24, 2023 at 02:53:12PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 24, 2023 at 01:32:14PM +0100, Mark Brown wrote:

> > There's issues with partially created debugfs structures getting in the
> > way of people trying to debug things, just completely ignoring all
> > errors can create confusion as the diagnostic information people believe
> > is being shown to them ends up being partial or mistructured without any
> > indication that this has happened.

> How do you end up with partially created debugfs structures?

The ones I've seen have been name collisions caused by for example the
debugfs structure created being flatter than the device model structure,
though obviously something unanticipated could come up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Greg Kroah-Hartman]

On Mon, Apr 24, 2023 at 02:00:14PM +0100, Mark Brown wrote:
> On Mon, Apr 24, 2023 at 02:53:12PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Apr 24, 2023 at 01:32:14PM +0100, Mark Brown wrote:
> 
> > > There's issues with partially created debugfs structures getting in the
> > > way of people trying to debug things, just completely ignoring all
> > > errors can create confusion as the diagnostic information people believe
> > > is being shown to them ends up being partial or mistructured without any
> > > indication that this has happened.
> 
> > How do you end up with partially created debugfs structures?
> 
> The ones I've seen have been name collisions caused by for example the
> debugfs structure created being flatter than the device model structure,
> though obviously something unanticipated could come up.

Sure name collisions will happen, when people aren't precise about how
they create their debugfs files (I just had to insist on this type of
fixups for a USB patch last week.)  But, debugfs failures should never
stop a driver from working properly, only the debugging functionalities.
So there's no need to error out from debugfs errors as the only one
affected is the kernel developer involved, not users.

thanks,

greg k-h

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

On Mon, Apr 24, 2023 at 03:08:03PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 24, 2023 at 02:00:14PM +0100, Mark Brown wrote:

> > The ones I've seen have been name collisions caused by for example the
> > debugfs structure created being flatter than the device model structure,
> > though obviously something unanticipated could come up.

> Sure name collisions will happen, when people aren't precise about how
> they create their debugfs files (I just had to insist on this type of
> fixups for a USB patch last week.)  But, debugfs failures should never
> stop a driver from working properly, only the debugging functionalities.
> So there's no need to error out from debugfs errors as the only one
> affected is the kernel developer involved, not users.

Developers can be users, and multiple people can work with a single bit
of code.  If these things are being done as a result of looking at the
code and reasonably deciding that actually the error handling wasn't
needed that'd be fine but when it's just silencing a static checker with
no obvious consideration and it's super obviously the right call just
from the diff that's a different thing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Peter Enderborg]

On 4/24/23 14:53, Greg Kroah-Hartman wrote:
>
>>> We can do things with the debug information without filesystem enabled.
> What exactly do you mean by this?
>
>
We can read out data from kernel with a ramdumper and analyse with crash.

See https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/kernel_administration_guide/kernel_crash_dump_guide

If it is useful or not I can not say, but the dws->regset. is lost and can not be read with a post mortem debugger.


> thanks,
>
> greg k-h

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Greg Kroah-Hartman]

On Mon, Apr 24, 2023 at 03:17:09PM +0200, Peter Enderborg wrote:
> On 4/24/23 14:53, Greg Kroah-Hartman wrote:
> >
> >>> We can do things with the debug information without filesystem enabled.
> > What exactly do you mean by this?
> >
> >
> We can read out data from kernel with a ramdumper and analyse with crash.
> 
> See https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/kernel_administration_guide/kernel_crash_dump_guide
> 
> If it is useful or not I can not say, but the dws->regset. is lost and can not be read with a post mortem debugger.

What is "dws"?  What is "regset"?

What is the root problem here?

confused,

greg k-h

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Peter Enderborg]

On 4/24/23 15:22, Greg Kroah-Hartman wrote:
> On Mon, Apr 24, 2023 at 03:17:09PM +0200, Peter Enderborg wrote:
>> On 4/24/23 14:53, Greg Kroah-Hartman wrote:
>>>>> We can do things with the debug information without filesystem enabled.
>>> What exactly do you mean by this?
>>>
>>>
>> We can read out data from kernel with a ramdumper and analyse with crash.
>>
>> See https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/kernel_administration_guide/kernel_crash_dump_guide 
>>
>> If it is useful or not I can not say, but the dws->regset. is lost and can not be read with a post mortem debugger.
> What is "dws"?  What is "regset"?

That is from the patch. It is used as an example.


> What is the root problem here?

That it is a mater of taste.  It should not be a mater of taste.

      if (!dws->debugfs)
                return -ENOMEM;

        dws->regset.regs = dw_spi_dbgfs_regs;
        dws->regset.nregs = ARRAY_SIZE(dw_spi_dbgfs_regs);
        dws->regset.base = dws->regs;
        debugfs_create_regset32("registers", 0400, dws->debugfs, &dws->regset);

Even if it does not have a impact on the function of the driver, it has a impact on debugging that
is not needed.


> confused,
>
> greg k-h

Re: [PATCH] spi: remove return value check of debugfs_create_dir()

[Greg Kroah-Hartman]

On Mon, Apr 24, 2023 at 03:54:13PM +0200, Peter Enderborg wrote:
> On 4/24/23 15:22, Greg Kroah-Hartman wrote:
> > On Mon, Apr 24, 2023 at 03:17:09PM +0200, Peter Enderborg wrote:
> >> On 4/24/23 14:53, Greg Kroah-Hartman wrote:
> >>>>> We can do things with the debug information without filesystem enabled.
> >>> What exactly do you mean by this?
> >>>
> >>>
> >> We can read out data from kernel with a ramdumper and analyse with crash.
> >>
> >> See https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/kernel_administration_guide/kernel_crash_dump_guide 
> >>
> >> If it is useful or not I can not say, but the dws->regset. is lost and can not be read with a post mortem debugger.
> > What is "dws"?  What is "regset"?
> 
> That is from the patch. It is used as an example.
> 
> 
> > What is the root problem here?
> 
> That it is a mater of taste.  It should not be a mater of taste.
> 
>       if (!dws->debugfs)
>                 return -ENOMEM;

Right here, you abort the normal operation of the driver if something
went wrong with debugfs, which is not a good idea.  That's the goal
here, nothing else.

thanks,

greg k-h