| Message ID | 20250409-mdb-max7360-support-v6-5-7a2535876e39@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for MAX7360 | expand |
On Wed, Apr 09, 2025 at 04:55:52PM +0200, Mathieu Dubois-Briand wrote:
> BUG() never returns, so code after it is unreachable: remove it.
BUG() can be compiled out, CONFIG_BUG.
On Wed, Apr 09, 2025 at 04:19:34PM +0100, Mark Brown wrote: > On Wed, Apr 09, 2025 at 04:55:52PM +0200, Mathieu Dubois-Briand wrote: > > BUG() never returns, so code after it is unreachable: remove it. > > BUG() can be compiled out, CONFIG_BUG. Also, please don't mix irrelevant patches into random serieses. It just makes everything noisier for everyone.
On Wed, Apr 09, 2025 at 04:19:27PM +0100, Mark Brown wrote: > On Wed, Apr 09, 2025 at 04:55:52PM +0200, Mathieu Dubois-Briand wrote: > > BUG() never returns, so code after it is unreachable: remove it. > > BUG() can be compiled out, CONFIG_BUG. Yes, and it's still has unreachable() there. So, this change is correct. See include/asm-generic/bug.h for the details of the implementation. And yes, if we have an architecture that does not do this way, it has to be fixed.
On Wed, Apr 09, 2025 at 04:55:52PM +0200, Mathieu Dubois-Briand wrote: > BUG() never returns, so code after it is unreachable: remove it. Thank you, this is the right change to do. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Wed, Apr 09, 2025 at 06:38:32PM +0300, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 04:19:27PM +0100, Mark Brown wrote: > > BUG() can be compiled out, CONFIG_BUG. > Yes, and it's still has unreachable() there. So, this change is correct. > See include/asm-generic/bug.h for the details of the implementation. > And yes, if we have an architecture that does not do this way, it has to > be fixed. unreachable() just annotates things, AFAICT it doesn't actually guarantee to do anything in particular if the annotation turns out to be incorrect.
On Wed, Apr 09, 2025 at 06:38:33PM +0300, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 04:19:27PM +0100, Mark Brown wrote: > > On Wed, Apr 09, 2025 at 04:55:52PM +0200, Mathieu Dubois-Briand wrote: > > > BUG() never returns, so code after it is unreachable: remove it. > > > > BUG() can be compiled out, CONFIG_BUG. > > Yes, and it's still has unreachable() there. So, this change is correct. > See include/asm-generic/bug.h for the details of the implementation. > And yes, if we have an architecture that does not do this way, it has to > be fixed. FWIW, I just have briefly checked the architectures and all of them include asm-generic/bug.h independently on the configuration option, some of them even define BUG() despite the configuration option (e.g. arc).
On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > On Wed, Apr 09, 2025 at 06:38:32PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 09, 2025 at 04:19:27PM +0100, Mark Brown wrote: > > > > BUG() can be compiled out, CONFIG_BUG. > > > Yes, and it's still has unreachable() there. So, this change is correct. > > See include/asm-generic/bug.h for the details of the implementation. > > And yes, if we have an architecture that does not do this way, it has to > > be fixed. > > unreachable() just annotates things, AFAICT it doesn't actually > guarantee to do anything in particular if the annotation turns out to be > incorrect. I;m not sure I follow. unreachable is a wrapper on top of __builtin_unreachable() which is intrinsic of the compiler. https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable
On Wed, Apr 09, 2025 at 07:00:24PM +0300, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > > unreachable() just annotates things, AFAICT it doesn't actually > > guarantee to do anything in particular if the annotation turns out to be > > incorrect. > I;m not sure I follow. unreachable is a wrapper on top of > __builtin_unreachable() which is intrinsic of the compiler. > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable That just says that the program is undefined if we get to the __builtin_undefined() and documents some behaviour around warnings. One example of undefined behaviour would be doing nothing.
On Wed, Apr 09, 2025 at 05:32:55PM +0100, Mark Brown wrote: > On Wed, Apr 09, 2025 at 07:00:24PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > > > > unreachable() just annotates things, AFAICT it doesn't actually > > > guarantee to do anything in particular if the annotation turns out to be > > > incorrect. > > > I;m not sure I follow. unreachable is a wrapper on top of > > __builtin_unreachable() which is intrinsic of the compiler. > > > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable > > That just says that the program is undefined if we get to the > __builtin_undefined() and documents some behaviour around warnings. One > example of undefined behaviour would be doing nothing. Theoretically yes, practically return after a BUG() makes no sense. Note, that compiler effectively removes 'goto exit;' here (that's also mentioned in the documentation independently on the control flow behaviour), so I don't know what you expect from it.
On Wed, Apr 09, 2025 at 07:45:42PM +0300, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 05:32:55PM +0100, Mark Brown wrote: > > On Wed, Apr 09, 2025 at 07:00:24PM +0300, Andy Shevchenko wrote: > > > On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > > > > > > unreachable() just annotates things, AFAICT it doesn't actually > > > > guarantee to do anything in particular if the annotation turns out to be > > > > incorrect. > > > > > I;m not sure I follow. unreachable is a wrapper on top of > > > __builtin_unreachable() which is intrinsic of the compiler. > > > > > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable > > > > That just says that the program is undefined if we get to the > > __builtin_undefined() and documents some behaviour around warnings. One > > example of undefined behaviour would be doing nothing. > > Theoretically yes, practically return after a BUG() makes no sense. Note, > that compiler effectively removes 'goto exit;' here (that's also mentioned > in the documentation independently on the control flow behaviour), so > I don't know what you expect from it. So unreachable() sometimes lears to weird behavior from compiler, for example as mentioned here where we ended up removing it to prevent miscompilations: https://lore.kernel.org/all/20241010222451.GA3571761@thelio-3990X/ Thanks.
On Wed Apr 9, 2025 at 5:21 PM CEST, Mark Brown wrote: > On Wed, Apr 09, 2025 at 04:19:34PM +0100, Mark Brown wrote: >> On Wed, Apr 09, 2025 at 04:55:52PM +0200, Mathieu Dubois-Briand wrote: >> > BUG() never returns, so code after it is unreachable: remove it. >> >> BUG() can be compiled out, CONFIG_BUG. > > Also, please don't mix irrelevant patches into random serieses. It just > makes everything noisier for everyone. Hi Mark, Just to provide the context about why this change is part of this series: this goto, if left unmodified, would have to replaced by a return. This is how the topic of dropping it came in the previous iteration of this series. Thanks for your review. Mathieu
Wed, Apr 09, 2025 at 10:16:40AM -0700, Dmitry Torokhov kirjoitti: > On Wed, Apr 09, 2025 at 07:45:42PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 09, 2025 at 05:32:55PM +0100, Mark Brown wrote: > > > On Wed, Apr 09, 2025 at 07:00:24PM +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 09, 2025 at 04:46:04PM +0100, Mark Brown wrote: > > > > > > > > unreachable() just annotates things, AFAICT it doesn't actually > > > > > guarantee to do anything in particular if the annotation turns out to be > > > > > incorrect. > > > > > > > I;m not sure I follow. unreachable is a wrapper on top of > > > > __builtin_unreachable() which is intrinsic of the compiler. > > > > > > > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable > > > > > > That just says that the program is undefined if we get to the > > > __builtin_undefined() and documents some behaviour around warnings. One > > > example of undefined behaviour would be doing nothing. > > > > Theoretically yes, practically return after a BUG() makes no sense. Note, > > that compiler effectively removes 'goto exit;' here (that's also mentioned > > in the documentation independently on the control flow behaviour), so > > I don't know what you expect from it. > > So unreachable() sometimes lears to weird behavior from compiler, for > example as mentioned here where we ended up removing it to prevent > miscompilations: > > https://lore.kernel.org/all/20241010222451.GA3571761@thelio-3990X/ How does it affect the BUG()? From your link: "I tested using BUG() in lieu of unreachable() like the second change I mentioned above, which resolves the issue cleanly, ..."
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 6c6869188c31..14f5fcc3ec1d 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -436,7 +436,6 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) break; default: BUG(); - goto exit; } }
BUG() never returns, so code after it is unreachable: remove it. Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> --- drivers/base/regmap/regmap-irq.c | 1 - 1 file changed, 1 deletion(-)