Messages in this thread | | | Date | Mon, 8 Apr 2019 19:07:28 -0700 | From | Ricardo Neri <> | Subject | Re: [RFC PATCH v2 13/14] watchdog/hardlockup/hpet: Only enable the HPET watchdog via a boot parameter |
| |
On Tue, Mar 26, 2019 at 10:29:52PM +0100, Thomas Gleixner wrote: > On Wed, 27 Feb 2019, Ricardo Neri wrote: > > + When hpet is specified, the NMI watchdog will be driven > > + by an HPET timer, if available in the system. Otherwise, > > + the perf-based implementation will be used. Specifying > > + hpet implies that nmi_watchdog is on. > > How so? > I meant to say that the user does not need to provide nmi_watchdog=1 and nmi_watchdog=hpet separately.
I think this is true because watchdog_user_enabled in kernel/watchdog.c is set to 1 when CONFIG_HARDLOCKUP_DETECTOR is selected. Also, if nmi_watchdog_available is set to true if watchdog_nmi_probe() is successful.
Perhaps I can add a warning in case nmi_watchdog=hpet and either CONFIG_HARDLOCKUP_DETECTOR or CONFIG_HARDLOCKUP_DETECTOR_HPET are not selected?
> > +static int __init hardlockup_detector_hpet_setup(char *str) > > +{ > > + if (strstr(str, "hpet")) > > + hardlockup_use_hpet = true; > > strstr()? Not really.
Is strncmp(str, "hpet", 5) more acceptable?
> > > + > > + return 0; > > +} > > +__setup("nmi_watchdog=", hardlockup_detector_hpet_setup); > > + > > /** > > * hardlockup_detector_hpet_init() - Initialize the hardlockup detector > > * > > @@ -405,6 +422,9 @@ int __init hardlockup_detector_hpet_init(void) > > { > > int ret; > > > > + if (!hardlockup_use_hpet) > > + return -ENODEV; > > This should have been there in the patch which introduces > hardlockup_detector_hpet_init(). And this patch merily adds the command > line magic which sets that flag.
Sure, I will move this check into the patch that introduces hardlockup_detector_hpet_init().
> > > + > > if (!is_hpet_enabled()) > > return -ENODEV; > > > > diff --git a/kernel/watchdog.c b/kernel/watchdog.c > > index 367aa81294ef..28cad7310378 100644 > > --- a/kernel/watchdog.c > > +++ b/kernel/watchdog.c > > @@ -78,7 +78,7 @@ static int __init hardlockup_panic_setup(char *str) > > nmi_watchdog_user_enabled = 0; > > else if (!strncmp(str, "1", 1)) > > nmi_watchdog_user_enabled = 1; > > - return 1; > > + return 0; > > Why?
My understanding is that this is needed so that other __setup functions that also want to check "nmi_watchdog" are able to do it. Is this understanding not correct?
Thanks and BR, Ricardo
| |