2022-05-09 04:23:24

by Ricardo Neri

[permalink] [raw]
Subject: [PATCH v6 24/29] watchdog/hardlockup: Use parse_option_str() to handle "nmi_watchdog"

Prepare hardlockup_panic_setup() to handle a comma-separated list of
options. Thus, it can continue parsing its own command-line options while
ignoring parameters that are relevant only to specific implementations of
the hardlockup detector. Such implementations may use an early_param to
parse their own options.

Cc: Andi Kleen <[email protected]>
Cc: Nicholas Piggin <[email protected]>
Cc: Stephane Eranian <[email protected]>
Cc: "Ravi V. Shankar" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Ricardo Neri <[email protected]>
---
Changes since v5:
* Corrected typo in commit message. (Tony)

Changes since v4:
* None

Changes since v3:
* None

Changes since v2:
* Introduced this patch.

Changes since v1:
* None
---
kernel/watchdog.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 9166220457bc..6443841a755f 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -73,13 +73,13 @@ void __init hardlockup_detector_disable(void)

static int __init hardlockup_panic_setup(char *str)
{
- if (!strncmp(str, "panic", 5))
+ if (parse_option_str(str, "panic"))
hardlockup_panic = 1;
- else if (!strncmp(str, "nopanic", 7))
+ else if (parse_option_str(str, "nopanic"))
hardlockup_panic = 0;
- else if (!strncmp(str, "0", 1))
+ else if (parse_option_str(str, "0"))
nmi_watchdog_user_enabled = 0;
- else if (!strncmp(str, "1", 1))
+ else if (parse_option_str(str, "1"))
nmi_watchdog_user_enabled = 1;
return 1;
}
--
2.17.1



2022-05-10 19:52:43

by Nicholas Piggin

[permalink] [raw]
Subject: Re: [PATCH v6 24/29] watchdog/hardlockup: Use parse_option_str() to handle "nmi_watchdog"

Excerpts from Ricardo Neri's message of May 6, 2022 10:00 am:
> Prepare hardlockup_panic_setup() to handle a comma-separated list of
> options. Thus, it can continue parsing its own command-line options while
> ignoring parameters that are relevant only to specific implementations of
> the hardlockup detector. Such implementations may use an early_param to
> parse their own options.

It can't really handle comma separated list though, until the next
patch. nmi_watchdog=panic,0 does not make sense, so you lost error
handling of that.

And is it kosher to double handle options like this? I'm sure it
happens but it's ugly.

Would you consider just add a new option for x86 and avoid changing
this? Less code and patches.

Thanks,
Nick

>
> Cc: Andi Kleen <[email protected]>
> Cc: Nicholas Piggin <[email protected]>
> Cc: Stephane Eranian <[email protected]>
> Cc: "Ravi V. Shankar" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Reviewed-by: Tony Luck <[email protected]>
> Signed-off-by: Ricardo Neri <[email protected]>
> ---
> Changes since v5:
> * Corrected typo in commit message. (Tony)
>
> Changes since v4:
> * None
>
> Changes since v3:
> * None
>
> Changes since v2:
> * Introduced this patch.
>
> Changes since v1:
> * None
> ---
> kernel/watchdog.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 9166220457bc..6443841a755f 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -73,13 +73,13 @@ void __init hardlockup_detector_disable(void)
>
> static int __init hardlockup_panic_setup(char *str)
> {
> - if (!strncmp(str, "panic", 5))
> + if (parse_option_str(str, "panic"))
> hardlockup_panic = 1;
> - else if (!strncmp(str, "nopanic", 7))
> + else if (parse_option_str(str, "nopanic"))
> hardlockup_panic = 0;
> - else if (!strncmp(str, "0", 1))
> + else if (parse_option_str(str, "0"))
> nmi_watchdog_user_enabled = 0;
> - else if (!strncmp(str, "1", 1))
> + else if (parse_option_str(str, "1"))
> nmi_watchdog_user_enabled = 1;
> return 1;
> }
> --
> 2.17.1
>
>

2022-05-14 04:23:06

by Ricardo Neri

[permalink] [raw]
Subject: Re: [PATCH v6 24/29] watchdog/hardlockup: Use parse_option_str() to handle "nmi_watchdog"

On Tue, May 10, 2022 at 08:46:41PM +1000, Nicholas Piggin wrote:
> Excerpts from Ricardo Neri's message of May 6, 2022 10:00 am:
> > Prepare hardlockup_panic_setup() to handle a comma-separated list of
> > options. Thus, it can continue parsing its own command-line options while
> > ignoring parameters that are relevant only to specific implementations of
> > the hardlockup detector. Such implementations may use an early_param to
> > parse their own options.
>
> It can't really handle comma separated list though, until the next
> patch. nmi_watchdog=panic,0 does not make sense, so you lost error
> handling of that.

Yes that is true. All possible combinations need to be checked.

>
> And is it kosher to double handle options like this? I'm sure it
> happens but it's ugly.
>
> Would you consider just add a new option for x86 and avoid changing
> this? Less code and patches.

Sure, I can not modify this code and add a x86-specific command-line
option.

Thanks and BR,
Ricardo