| Message ID | 1692005246-18399-2-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Modify die() for MIPS | expand |
On Mon, 14 Aug 2023, Tiezhu Yang wrote: > In the later patch, we will remove noreturn attribute for die(), in order > to make each patch can be built without errors and warnings, just remove > noreturn attribute for nmi_exception_handler() earlier because it calls > die(), otherwise there exists the following build error after the later > patch: I find the wording a bit odd here, but you'll have to rewrite the change description for the update requested below, so let's defer any style fixes to v4. > arch/mips/kernel/traps.c:2001:1: error: 'noreturn' function does return [-Werror] Now that I've looked into it in detail, this change is incomplete and will make the kernel go astray if `nmi_exception_handler' actually ever does return. See code in arch/mips/kernel/genex.S, which calls this function and doesn't expect it to return. It has to be fixed before 2/3 can be considered. I wonder how you didn't catch it: you did check how this code is used, didn't you? Before submitting an updated version can you actually arrange for the NOTIFY_STOP condition to happen in your lab and verify it is handled as expected? And what was the motivation for this code update, just a hypothetical scenario? Maciej
On 08/18/2023 10:39 AM, Maciej W. Rozycki wrote: > On Mon, 14 Aug 2023, Tiezhu Yang wrote: > >> In the later patch, we will remove noreturn attribute for die(), in order >> to make each patch can be built without errors and warnings, just remove >> noreturn attribute for nmi_exception_handler() earlier because it calls >> die(), otherwise there exists the following build error after the later >> patch: > > I find the wording a bit odd here, but you'll have to rewrite the change > description for the update requested below, so let's defer any style fixes > to v4. > >> arch/mips/kernel/traps.c:2001:1: error: 'noreturn' function does return [-Werror] > > Now that I've looked into it in detail, this change is incomplete and > will make the kernel go astray if `nmi_exception_handler' actually ever > does return. See code in arch/mips/kernel/genex.S, which calls this > function and doesn't expect it to return. It has to be fixed before 2/3 > can be considered. I wonder how you didn't catch it: you did check how > this code is used, didn't you? > I think the proper way is to keep the noreturn attribute for nmi_exception_handler(), and add a noreturn function BUG() at the end of nmi_exception_handler() to make sure it does not return. > Before submitting an updated version can you actually arrange for the > NOTIFY_STOP condition to happen in your lab and verify it is handled as > expected? And what was the motivation for this code update, just a > hypothetical scenario? Yes, just a hypothetical scenario. Thanks, Tiezhu
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c index 246c6a6..7a34674 100644 --- a/arch/mips/kernel/traps.c +++ b/arch/mips/kernel/traps.c @@ -1986,7 +1986,7 @@ int register_nmi_notifier(struct notifier_block *nb) return raw_notifier_chain_register(&nmi_chain, nb); } -void __noreturn nmi_exception_handler(struct pt_regs *regs) +void nmi_exception_handler(struct pt_regs *regs) { char str[100];
In the later patch, we will remove noreturn attribute for die(), in order to make each patch can be built without errors and warnings, just remove noreturn attribute for nmi_exception_handler() earlier because it calls die(), otherwise there exists the following build error after the later patch: arch/mips/kernel/traps.c:2001:1: error: 'noreturn' function does return [-Werror] Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- arch/mips/kernel/traps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)