Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK").
A lot of warn_unused_result warnings existed in 2006, but until now
they have been fixed thanks to people doing allmodconfig tests.
Our goal is to always enable __must_check where appropriate, so this
CONFIG option is no longer needed.
I see a lot of defconfig (arch/*/configs/*_defconfig) files having:
# CONFIG_ENABLE_MUST_CHECK is not set
I did not touch them for now since it would be a big churn. If arch
maintainers want to clean them up, please go ahead.
While I was here, I also moved __must_check to compiler_attributes.h
from compiler_types.h
Signed-off-by: Masahiro Yamada <[email protected]>
Acked-by: Jason A. Donenfeld <[email protected]>
---
Changes in v3:
- Fix a typo
Changes in v2:
- Move __must_check to compiler_attributes.h
include/linux/compiler_attributes.h | 7 +++++++
include/linux/compiler_types.h | 6 ------
lib/Kconfig.debug | 8 --------
tools/testing/selftests/wireguard/qemu/debug.config | 1 -
4 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index b2a3f4f641a7..5f3b7edad1a7 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -171,6 +171,13 @@
*/
#define __mode(x) __attribute__((__mode__(x)))
+/*
+ * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-warn_005funused_005fresult-function-attribute
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
+ *
+ */
+#define __must_check __attribute__((__warn_unused_result__))
+
/*
* Optional: only supported since gcc >= 7
*
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index ac3fa37a84f9..7ef20d1a6c28 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -110,12 +110,6 @@ struct ftrace_likely_data {
unsigned long constant;
};
-#ifdef CONFIG_ENABLE_MUST_CHECK
-#define __must_check __attribute__((__warn_unused_result__))
-#else
-#define __must_check
-#endif
-
#if defined(CC_USING_HOTPATCH)
#define notrace __attribute__((hotpatch(0, 0)))
#elif defined(CC_USING_PATCHABLE_FUNCTION_ENTRY)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c789b39ed527..cb8ef4fd0d02 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -286,14 +286,6 @@ config GDB_SCRIPTS
endif # DEBUG_INFO
-config ENABLE_MUST_CHECK
- bool "Enable __must_check logic"
- default y
- help
- Enable the __must_check logic in the kernel build. Disable this to
- suppress the "warning: ignoring return value of 'foo', declared with
- attribute warn_unused_result" messages.
-
config FRAME_WARN
int "Warn for stack frames larger than"
range 0 8192
diff --git a/tools/testing/selftests/wireguard/qemu/debug.config b/tools/testing/selftests/wireguard/qemu/debug.config
index b50c2085c1ac..fe07d97df9fa 100644
--- a/tools/testing/selftests/wireguard/qemu/debug.config
+++ b/tools/testing/selftests/wireguard/qemu/debug.config
@@ -1,5 +1,4 @@
CONFIG_LOCALVERSION="-debug"
-CONFIG_ENABLE_MUST_CHECK=y
CONFIG_FRAME_POINTER=y
CONFIG_STACK_VALIDATION=y
CONFIG_DEBUG_KERNEL=y
--
2.27.0
On Sun, Nov 29, 2020 at 04:33:35AM +0900, Masahiro Yamada wrote:
> Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK").
>
> A lot of warn_unused_result warnings existed in 2006, but until now
> they have been fixed thanks to people doing allmodconfig tests.
>
> Our goal is to always enable __must_check where appropriate, so this
> CONFIG option is no longer needed.
>
> I see a lot of defconfig (arch/*/configs/*_defconfig) files having:
>
> # CONFIG_ENABLE_MUST_CHECK is not set
>
> I did not touch them for now since it would be a big churn. If arch
> maintainers want to clean them up, please go ahead.
>
> While I was here, I also moved __must_check to compiler_attributes.h
> from compiler_types.h
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> Acked-by: Jason A. Donenfeld <[email protected]>
Acked-by: Nathan Chancellor <[email protected]>
On Sat, Nov 28, 2020 at 11:34 AM Masahiro Yamada <[email protected]> wrote:
>
> Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK").
>
> A lot of warn_unused_result warnings existed in 2006, but until now
> they have been fixed thanks to people doing allmodconfig tests.
>
> Our goal is to always enable __must_check where appropriate, so this
> CONFIG option is no longer needed.
>
> I see a lot of defconfig (arch/*/configs/*_defconfig) files having:
>
> # CONFIG_ENABLE_MUST_CHECK is not set
>
> I did not touch them for now since it would be a big churn. If arch
> maintainers want to clean them up, please go ahead.
>
> While I was here, I also moved __must_check to compiler_attributes.h
> from compiler_types.h
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> Acked-by: Jason A. Donenfeld <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
>
> Changes in v3:
> - Fix a typo
>
> Changes in v2:
> - Move __must_check to compiler_attributes.h
>
> include/linux/compiler_attributes.h | 7 +++++++
> include/linux/compiler_types.h | 6 ------
> lib/Kconfig.debug | 8 --------
> tools/testing/selftests/wireguard/qemu/debug.config | 1 -
> 4 files changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> index b2a3f4f641a7..5f3b7edad1a7 100644
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -171,6 +171,13 @@
> */
> #define __mode(x) __attribute__((__mode__(x)))
>
> +/*
> + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-warn_005funused_005fresult-function-attribute
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
> + *
> + */
> +#define __must_check __attribute__((__warn_unused_result__))
> +
> /*
> * Optional: only supported since gcc >= 7
> *
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index ac3fa37a84f9..7ef20d1a6c28 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -110,12 +110,6 @@ struct ftrace_likely_data {
> unsigned long constant;
> };
>
> -#ifdef CONFIG_ENABLE_MUST_CHECK
> -#define __must_check __attribute__((__warn_unused_result__))
> -#else
> -#define __must_check
> -#endif
> -
> #if defined(CC_USING_HOTPATCH)
> #define notrace __attribute__((hotpatch(0, 0)))
> #elif defined(CC_USING_PATCHABLE_FUNCTION_ENTRY)
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index c789b39ed527..cb8ef4fd0d02 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -286,14 +286,6 @@ config GDB_SCRIPTS
>
> endif # DEBUG_INFO
>
> -config ENABLE_MUST_CHECK
> - bool "Enable __must_check logic"
> - default y
> - help
> - Enable the __must_check logic in the kernel build. Disable this to
> - suppress the "warning: ignoring return value of 'foo', declared with
> - attribute warn_unused_result" messages.
> -
> config FRAME_WARN
> int "Warn for stack frames larger than"
> range 0 8192
> diff --git a/tools/testing/selftests/wireguard/qemu/debug.config b/tools/testing/selftests/wireguard/qemu/debug.config
> index b50c2085c1ac..fe07d97df9fa 100644
> --- a/tools/testing/selftests/wireguard/qemu/debug.config
> +++ b/tools/testing/selftests/wireguard/qemu/debug.config
> @@ -1,5 +1,4 @@
> CONFIG_LOCALVERSION="-debug"
> -CONFIG_ENABLE_MUST_CHECK=y
> CONFIG_FRAME_POINTER=y
> CONFIG_STACK_VALIDATION=y
> CONFIG_DEBUG_KERNEL=y
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
On Sat, Nov 28, 2020 at 8:34 PM Masahiro Yamada <[email protected]> wrote:
>
> Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK").
>
> A lot of warn_unused_result warnings existed in 2006, but until now
> they have been fixed thanks to people doing allmodconfig tests.
>
> Our goal is to always enable __must_check where appropriate, so this
> CONFIG option is no longer needed.
>
> I see a lot of defconfig (arch/*/configs/*_defconfig) files having:
>
> # CONFIG_ENABLE_MUST_CHECK is not set
>
> I did not touch them for now since it would be a big churn. If arch
> maintainers want to clean them up, please go ahead.
>
> While I was here, I also moved __must_check to compiler_attributes.h
> from compiler_types.h
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> Acked-by: Jason A. Donenfeld <[email protected]>
Picked this new version with the Acks etc., plus I moved it within
compiler_attributes.h to keep it sorted (it's sorted by the second
column, rather than the first).
Thanks a lot!
Cheers,
Miguel
On Sun, Nov 29, 2020 at 04:33:35AM +0900, Masahiro Yamada wrote:
> Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK").
>
> A lot of warn_unused_result warnings existed in 2006, but until now
> they have been fixed thanks to people doing allmodconfig tests.
>
> Our goal is to always enable __must_check where appropriate, so this
> CONFIG option is no longer needed.
>
> I see a lot of defconfig (arch/*/configs/*_defconfig) files having:
>
> # CONFIG_ENABLE_MUST_CHECK is not set
>
> I did not touch them for now since it would be a big churn. If arch
> maintainers want to clean them up, please go ahead.
>
> While I was here, I also moved __must_check to compiler_attributes.h
> from compiler_types.h
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> Acked-by: Jason A. Donenfeld <[email protected]>
> Acked-by: Nathan Chancellor <[email protected]>
> Reviewed-by: Nick Desaulniers <[email protected]>
This patch results in:
arch/sh/kernel/cpu/sh4a/smp-shx3.c: In function 'shx3_prepare_cpus':
arch/sh/kernel/cpu/sh4a/smp-shx3.c:76:3: error: ignoring return value of 'request_irq' declared with attribute 'warn_unused_result'
when building sh:defconfig. Checking for calls to request_irq()
suggests that there will be other similar errors in various builds.
Reverting the patch fixes the problem.
Guenter
On Sat, Dec 12, 2020 at 5:18 PM Guenter Roeck <[email protected]> wrote:
>
> This patch results in:
>
> arch/sh/kernel/cpu/sh4a/smp-shx3.c: In function 'shx3_prepare_cpus':
> arch/sh/kernel/cpu/sh4a/smp-shx3.c:76:3: error: ignoring return value of 'request_irq' declared with attribute 'warn_unused_result'
>
> when building sh:defconfig. Checking for calls to request_irq()
> suggests that there will be other similar errors in various builds.
> Reverting the patch fixes the problem.
Which ones? From a quick grep and some filtering I could only find one
file with wrong usage apart from this one:
drivers/net/ethernet/lantiq_etop.c:
request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
drivers/net/ethernet/lantiq_etop.c:
request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
Of course, this does not cover other functions, but it means there
aren't many issues and/or people building the code if nobody complains
within a few weeks. So I think we can fix them as they come.
Cheers,
Miguel
On 12/12/20 9:04 PM, Miguel Ojeda wrote:
> On Sat, Dec 12, 2020 at 5:18 PM Guenter Roeck <[email protected]> wrote:
>>
>> This patch results in:
>>
>> arch/sh/kernel/cpu/sh4a/smp-shx3.c: In function 'shx3_prepare_cpus':
>> arch/sh/kernel/cpu/sh4a/smp-shx3.c:76:3: error: ignoring return value of 'request_irq' declared with attribute 'warn_unused_result'
>>
>> when building sh:defconfig. Checking for calls to request_irq()
>> suggests that there will be other similar errors in various builds.
>> Reverting the patch fixes the problem.
>
> Which ones? From a quick grep and some filtering I could only find one
> file with wrong usage apart from this one:
>
> drivers/net/ethernet/lantiq_etop.c:
> request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv);
> drivers/net/ethernet/lantiq_etop.c:
> request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
>
> Of course, this does not cover other functions, but it means there
> aren't many issues and/or people building the code if nobody complains
> within a few weeks. So I think we can fix them as they come.
>
Witz komm raus, Du bist umzingelt.
The key here is "if nobody complains". I would argue that it is _your_
responsibility to do those builds, and not the reponsibility of others
to do it for you.
But, sure, your call. Please feel free to ignore my report.
Guenter
On Sun, Dec 13, 2020 at 03:58:20PM +0100, Miguel Ojeda wrote:
> > The key here is "if nobody complains". I would argue that it is _your_
> > responsibility to do those builds, and not the reponsibility of others
> > to do it for you.
>
> Testing allmodconfig for a popular architecture, agreed, it is due
> diligence to avoid messing -next that day.
>
> Testing a matrix of configs * arches * gcc/clang * compiler versions?
> No, sorry, that is what CI/-next/-rcs are for and that is where the
> "if nobody complains" comes from.
>
> If you think building a set of code for a given arch/config/etc. is
> particularly important, then it is _your_ responsibility to build it
> once in a while in -next (as you have done). If it is not that
> important, somebody will speak up in one -rc. If not, is anyone
> actually building that code at all?
>
> Otherwise, changing core/shared code would be impossible. Please don't
> blame the author for making a sensible change that will improve code
> quality for everyone.
>
> > But, sure, your call. Please feel free to ignore my report.
>
> I'm not ignoring the report, quite the opposite. I am trying to
> understand why you think reverting is needed for something that has
> been more than a week in -next without any major breakage and still
> has a long road to v5.11.
Because if you get a report of something breaking for your change, you
need to work to resolve it, not argue about it. Otherwise it needs to
be dropped/reverted.
Please fix.
thanks,
greg k-h
On Sun, Dec 13, 2020 at 4:16 PM Greg KH <[email protected]> wrote:
>
> Because if you get a report of something breaking for your change, you
> need to work to resolve it, not argue about it. Otherwise it needs to
> be dropped/reverted.
Nobody has argued that. In fact, I explicitly said the opposite: "So I
think we can fix them as they come.".
I am expecting Masahiro to follow up. It has been less than 24 hours
since the report, on a weekend.
Cheers,
Miguel
On Sun, Dec 13, 2020 at 1:55 PM Guenter Roeck <[email protected]> wrote:
>
> Witz komm raus, Du bist umzingelt.
Please, explain this reference. :-)
> The key here is "if nobody complains". I would argue that it is _your_
> responsibility to do those builds, and not the reponsibility of others
> to do it for you.
Testing allmodconfig for a popular architecture, agreed, it is due
diligence to avoid messing -next that day.
Testing a matrix of configs * arches * gcc/clang * compiler versions?
No, sorry, that is what CI/-next/-rcs are for and that is where the
"if nobody complains" comes from.
If you think building a set of code for a given arch/config/etc. is
particularly important, then it is _your_ responsibility to build it
once in a while in -next (as you have done). If it is not that
important, somebody will speak up in one -rc. If not, is anyone
actually building that code at all?
Otherwise, changing core/shared code would be impossible. Please don't
blame the author for making a sensible change that will improve code
quality for everyone.
> But, sure, your call. Please feel free to ignore my report.
I'm not ignoring the report, quite the opposite. I am trying to
understand why you think reverting is needed for something that has
been more than a week in -next without any major breakage and still
has a long road to v5.11.
Cheers,
Miguel
On Sun, Dec 13, 2020 at 4:38 PM 'Matthias Urlichs' via Clang Built
Linux <[email protected]> wrote:
>
> If your change to a function breaks its callers, it's your job to fix
No function has changed. This patch enables a warning (that for some
reason is an error in the case of Guenter).
Even if this was a hard error, the same applies: the function hasn't
changed. It just means callers never tested with
`CONFIG_ENABLE_MUST_CHECK` for *years*.
> the callers proactively instead of waiting for "as they come" bug
> reports. (Assuming, of course, that you know about the breakage. Which
> you do when you tell us that the bad pattern can simply be grepped for.)
No, *we don't know about the breakage*. The grep was for the
particular function Guenter reported, and done to validate his
concern.
If you want to manually inspect every caller of every `__must_check`
function, or to write a cocci patch or a clang-tidy check or similar
(that would be obsolete as soon as `__must_check` is enabled), you are
welcome to do so. But a much better usage of our time would be letting
machines do their job.
> If nothing else, that's far more efficient than [number_of_callers]
> separate patches by other people who each need to find the offending
> change, figure out what to change and/or who to report the problem to,
> and so on until the fix lands in the kernel.
This change is not in Linus' tree, it is on -next.
> Moreover, this wouldn't leave the kernel sources in a non-bisect-able
> state during that time.
Again, the change is in -next. That is the point: to do integration
testing and let the bots run against it.
Cheers,
Miguel
On Mon, Dec 14, 2020 at 12:27 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Sun, Dec 13, 2020 at 4:16 PM Greg KH <[email protected]> wrote:
> >
> > Because if you get a report of something breaking for your change, you
> > need to work to resolve it, not argue about it. Otherwise it needs to
> > be dropped/reverted.
>
> Nobody has argued that. In fact, I explicitly said the opposite: "So I
> think we can fix them as they come.".
>
> I am expecting Masahiro to follow up. It has been less than 24 hours
> since the report, on a weekend.
>
> Cheers,
> Miguel
Sorry for the delay.
Now I sent out the fix for lantiq_etop.c
https://lore.kernel.org/patchwork/patch/1355595/
The reason of the complication was
I was trying to merge the following patch in the same development cycle:
https://patchwork.kernel.org/project/linux-kbuild/patch/[email protected]/
-Werror=return-type gives a bigger impact
because any instance of __must_check violation
results in build breakage.
So, I just dropped it from my tree (and, I will aim for 5.12).
The removal of CONFIG_ENABLE_MUST_CHECK is less impactive,
because we are still able to build with some warnings.
Tomorrow's linux-next should be OK
and, you can send my patch in this merge window.
--
Best Regards
Masahiro Yamada
On 12/20/20 10:18 PM, Masahiro Yamada wrote:
> On Mon, Dec 14, 2020 at 12:27 AM Miguel Ojeda
> <[email protected]> wrote:
>>
>> On Sun, Dec 13, 2020 at 4:16 PM Greg KH <[email protected]> wrote:
>>>
>>> Because if you get a report of something breaking for your change, you
>>> need to work to resolve it, not argue about it. Otherwise it needs to
>>> be dropped/reverted.
>>
>> Nobody has argued that. In fact, I explicitly said the opposite: "So I
>> think we can fix them as they come.".
>>
>> I am expecting Masahiro to follow up. It has been less than 24 hours
>> since the report, on a weekend.
>>
>> Cheers,
>> Miguel
>
>
> Sorry for the delay.
>
> Now I sent out the fix for lantiq_etop.c
>
> https://lore.kernel.org/patchwork/patch/1355595/
>
next-20201218, alpha:allmodconfig:
fs/binfmt_em86.c: In function 'load_em86':
fs/binfmt_em86.c:66:2: error: ignoring return value of 'remove_arg_zero' declared with attribute 'warn_unused_result'
With a change like this, I'd have expected that there is a coccinelle
script or similar to ensure that claims made in the commit message
are true.
Guenter
On Mon, Dec 21, 2020 at 7:20 AM Masahiro Yamada <[email protected]> wrote:
>
> Sorry for the delay.
No problem!
> Now I sent out the fix for lantiq_etop.c
>
> https://lore.kernel.org/patchwork/patch/1355595/
I saw it, thanks for the Cc!
> The reason of the complication was
> I was trying to merge the following patch in the same development cycle:
> https://patchwork.kernel.org/project/linux-kbuild/patch/[email protected]/
Ah, then that explains why Guenter's had an error instead of a warning.
> Tomorrow's linux-next should be OK
> and, you can send my patch in this merge window.
Thanks!
Cheers,
Miguel
On Mon, Dec 21, 2020 at 11:02 AM Guenter Roeck <[email protected]> wrote:
>
> On 12/20/20 10:18 PM, Masahiro Yamada wrote:
> With a change like this, I'd have expected that there is a coccinelle
> script or similar to ensure that claims made in the commit message
> are true.
It is only a warning -- the compiler already tells us what is wrong.
Cheers,
Miguel
request_irq is marked __must_check, but the call in shx3_prepare_cpus
has a void return type, so it can't propagate failure to the caller.
Follow cues from hexagon and just print an error.
Fixes: c7936b9abcf5 ("sh: smp: Hook in to the generic IPI handler for SH-X3 SMP.")
Cc: Miguel Ojeda <[email protected]>
Cc: Paul Mundt <[email protected]>
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
arch/sh/kernel/cpu/sh4a/smp-shx3.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/sh/kernel/cpu/sh4a/smp-shx3.c b/arch/sh/kernel/cpu/sh4a/smp-shx3.c
index f8a2bec0f260..1261dc7b84e8 100644
--- a/arch/sh/kernel/cpu/sh4a/smp-shx3.c
+++ b/arch/sh/kernel/cpu/sh4a/smp-shx3.c
@@ -73,8 +73,9 @@ static void shx3_prepare_cpus(unsigned int max_cpus)
BUILD_BUG_ON(SMP_MSG_NR >= 8);
for (i = 0; i < SMP_MSG_NR; i++)
- request_irq(104 + i, ipi_interrupt_handler,
- IRQF_PERCPU, "IPI", (void *)(long)i);
+ if (request_irq(104 + i, ipi_interrupt_handler,
+ IRQF_PERCPU, "IPI", (void *)(long)i))
+ pr_err("Failed to request irq %d\n", i);
for (i = 0; i < max_cpus; i++)
set_cpu_present(i, true);
--
2.29.2.729.g45daf8777d-goog
remove_arg_zero is declared as __must_check. Looks like it can return
-EFAULT on failure.
Cc: Masahiro Yamada <[email protected]>
Cc: Miguel Ojeda <[email protected]>
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
fs/binfmt_em86.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
index 06b9b9fddf70..6e98fcfca66e 100644
--- a/fs/binfmt_em86.c
+++ b/fs/binfmt_em86.c
@@ -63,7 +63,8 @@ static int load_em86(struct linux_binprm *bprm)
* This is done in reverse order, because of how the
* user environment and arguments are stored.
*/
- remove_arg_zero(bprm);
+ retval = remove_arg_zero(bprm);
+ if (retval < 0) return retval;
retval = copy_string_kernel(bprm->filename, bprm);
if (retval < 0) return retval;
bprm->argc++;
--
2.29.2.729.g45daf8777d-goog
On Wed, Dec 23, 2020 at 5:54 AM Nick Desaulniers
<[email protected]> wrote:
>
> request_irq is marked __must_check, but the call in shx3_prepare_cpus
> has a void return type, so it can't propagate failure to the caller.
> Follow cues from hexagon and just print an error.
>
> Fixes: c7936b9abcf5 ("sh: smp: Hook in to the generic IPI handler for SH-X3 SMP.")
> Cc: Miguel Ojeda <[email protected]>
> Cc: Paul Mundt <[email protected]>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
Thanks for the patch, Nick.
I just wondered if there was a better error handling than
printing the message. I have no idea if the system will
boot up correctly when the request_irq() fails here.
I hope the maintainers will suggest something, if any.
> ---
> arch/sh/kernel/cpu/sh4a/smp-shx3.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/sh/kernel/cpu/sh4a/smp-shx3.c b/arch/sh/kernel/cpu/sh4a/smp-shx3.c
> index f8a2bec0f260..1261dc7b84e8 100644
> --- a/arch/sh/kernel/cpu/sh4a/smp-shx3.c
> +++ b/arch/sh/kernel/cpu/sh4a/smp-shx3.c
> @@ -73,8 +73,9 @@ static void shx3_prepare_cpus(unsigned int max_cpus)
> BUILD_BUG_ON(SMP_MSG_NR >= 8);
>
> for (i = 0; i < SMP_MSG_NR; i++)
> - request_irq(104 + i, ipi_interrupt_handler,
> - IRQF_PERCPU, "IPI", (void *)(long)i);
> + if (request_irq(104 + i, ipi_interrupt_handler,
> + IRQF_PERCPU, "IPI", (void *)(long)i))
> + pr_err("Failed to request irq %d\n", i);
>
> for (i = 0; i < max_cpus; i++)
> set_cpu_present(i, true);
> --
> 2.29.2.729.g45daf8777d-goog
>
--
Best Regards
Masahiro Yamada
Hi Nick!
On 12/22/20 9:54 PM, Nick Desaulniers wrote:
> request_irq is marked __must_check, but the call in shx3_prepare_cpus
> has a void return type, so it can't propagate failure to the caller.
> Follow cues from hexagon and just print an error.
>
> Fixes: c7936b9abcf5 ("sh: smp: Hook in to the generic IPI handler for SH-X3 SMP.")
> Cc: Miguel Ojeda <[email protected]>
> Cc: Paul Mundt <[email protected]>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> arch/sh/kernel/cpu/sh4a/smp-shx3.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/sh/kernel/cpu/sh4a/smp-shx3.c b/arch/sh/kernel/cpu/sh4a/smp-shx3.c
> index f8a2bec0f260..1261dc7b84e8 100644
> --- a/arch/sh/kernel/cpu/sh4a/smp-shx3.c
> +++ b/arch/sh/kernel/cpu/sh4a/smp-shx3.c
> @@ -73,8 +73,9 @@ static void shx3_prepare_cpus(unsigned int max_cpus)
> BUILD_BUG_ON(SMP_MSG_NR >= 8);
>
> for (i = 0; i < SMP_MSG_NR; i++)
> - request_irq(104 + i, ipi_interrupt_handler,
> - IRQF_PERCPU, "IPI", (void *)(long)i);
> + if (request_irq(104 + i, ipi_interrupt_handler,
> + IRQF_PERCPU, "IPI", (void *)(long)i))
> + pr_err("Failed to request irq %d\n", i);
>
> for (i = 0; i < max_cpus; i++)
> set_cpu_present(i, true);
>
Verified on my SH-7785LCR board. Boots fine.
Tested-by: John Paul Adrian Glaubitz <[email protected]>
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On Fri, Jan 1, 2021 at 2:50 PM John Paul Adrian Glaubitz
<[email protected]> wrote:
>
> Verified on my SH-7785LCR board. Boots fine.
>
> Tested-by: John Paul Adrian Glaubitz <[email protected]>
Thanks for testing, John!
I think Masahiro was concerned about the error case (I assume you
tested the happy path).
In any case, if no maintainer suggests something else, looks good to
me (and it is no worse than the status quo unless the `pr_err()` can
somehow kill it), so:
Reviewed-by: Miguel Ojeda <[email protected]>
Cheers,
Miguel
On Tue, Dec 22, 2020 at 10:03 PM Nick Desaulniers
<[email protected]> wrote:
>
> remove_arg_zero is declared as __must_check. Looks like it can return
> -EFAULT on failure.
>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Miguel Ojeda <[email protected]>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
Cc'ing Alpha list too.
Alexander, fs-devel: pinging about this... We removed
ENABLE_MUST_CHECK in 196793946264 ("Compiler Attributes: remove
CONFIG_ENABLE_MUST_CHECK"), so this should be giving a warning now
unconditionally. In 5.12 it will likely become a build error.
Nick: thanks for the patch! (I missed it in December, sorry)
Reviewed-by: Miguel Ojeda <[email protected]>
Cheers,
Miguel
Hi Miguel!
On 1/1/21 9:42 PM, Miguel Ojeda wrote:
> On Fri, Jan 1, 2021 at 2:50 PM John Paul Adrian Glaubitz
> <[email protected]> wrote:
>>
>> Verified on my SH-7785LCR board. Boots fine.
>>
>> Tested-by: John Paul Adrian Glaubitz <[email protected]>
>
> Thanks for testing, John!
>
> I think Masahiro was concerned about the error case (I assume you
> tested the happy path).
Not sure about testing the error case though. How do I make request_irq()
fail?
> In any case, if no maintainer suggests something else, looks good to
> me (and it is no worse than the status quo unless the `pr_err()` can
> somehow kill it), so:
>
> Reviewed-by: Miguel Ojeda <[email protected]>
I agree.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913