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