diff mbox series

spi: remove return value check of debugfs_create_dir()

Message ID 20230423061155.2540-1-ysxu@hust.edu.cn (mailing list archive)
State New, archived
Headers show
Series spi: remove return value check of debugfs_create_dir() | expand

Commit Message

Yingsha Xu April 23, 2023, 6:11 a.m. UTC
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(-)

Comments

Mark Brown April 24, 2023, 11:54 a.m. UTC | #1
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.
Peter Enderborg April 24, 2023, 12:20 p.m. UTC | #2
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.
Mark Brown April 24, 2023, 12:32 p.m. UTC | #3
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.
Greg Kroah-Hartman April 24, 2023, 12:53 p.m. UTC | #4
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
Mark Brown April 24, 2023, 1 p.m. UTC | #5
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.
Greg Kroah-Hartman April 24, 2023, 1:08 p.m. UTC | #6
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
Peter Enderborg April 24, 2023, 1:17 p.m. UTC | #7
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
Mark Brown April 24, 2023, 1:17 p.m. UTC | #8
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.
Greg Kroah-Hartman April 24, 2023, 1:22 p.m. UTC | #9
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
Peter Enderborg April 24, 2023, 1:54 p.m. UTC | #10
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
Greg Kroah-Hartman April 25, 2023, 5:31 a.m. UTC | #11
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
diff mbox series

Patch

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);