From: Ivan Mironov <mironov.ivan@gmail.com>
To: Jerry.Hoemann@hpe.com
Cc: linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org,
Wim Van Sebroeck <wim@linux-watchdog.org>,
Guenter Roeck <linux@roeck-us.net>
Subject: Re: [RFC PATCH 4/4] watchdog: hpwdt: Make panic behaviour configurable
Date: Sat, 02 Feb 2019 10:13:54 +0500 [thread overview]
Message-ID: <6f3eec21920413fdbc774d89a6f6bbc2c7863370.camel@gmail.com> (raw)
In-Reply-To: <20190116023022.GE18342@anatevka>
On Tue, 2019-01-15 at 19:30 -0700, Jerry Hoemann wrote:
> On Mon, Jan 14, 2019 at 07:36:17AM +0500, Ivan Mironov wrote:
> > This adds an option to not panic on NMI even if it was caused by iLO.
> >
> > Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
> > ---
> > drivers/watchdog/hpwdt.c | 19 ++++++++++++++++---
> > 1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index 95d002b5b120..b12858491189 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -37,6 +37,10 @@ static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */
> > static bool nowayout = WATCHDOG_NOWAYOUT;
> > static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);
> >
> > +#ifdef CONFIG_HPWDT_NMI_DECODING
> > +static bool panic_on_nmi = true;
> > +#endif /* CONFIG_HPWDT_NMI_DECODING */
> > +
> > static void __iomem *pci_mem_addr; /* the PCI-memory address */
> > static unsigned long __iomem *hpwdt_nmistat;
> > static unsigned long __iomem *hpwdt_timer_reg;
> > @@ -146,7 +150,10 @@ static int hpwdt_set_pretimeout(struct watchdog_device *wdd, unsigned int req)
> >
> > static int hpwdt_my_nmi(void)
> > {
> > - return ioread8(hpwdt_nmistat) & 0x6;
> > + int nmistat = ioread8(hpwdt_nmistat);
> > +
> > + iowrite8(nmistat & ~0x6, hpwdt_nmistat);
> > + return nmistat & 0x6;
>
> This is a read only register.
>
Oops... At least on my system this code has the desired effect:
subsequent function call returns zero.
Probably it would be better to use additional variable to determine
whether NMI from iLO already happened or not.
>
> > }
> >
> > /*
> > @@ -168,7 +175,10 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
> > "4. iLO Event Log\n",
> > mynmi, ulReason, smp_processor_id());
> >
> > - nmi_panic(regs, "hpwdt: NMI: Not continuing");
> > + if (panic_on_nmi)
> > + nmi_panic(regs, "hpwdt: NMI: Not continuing");
> > +
> > + pr_emerg("Dazed and confused, but trying to continue\n");
> >
>
> The watchdog core provides a way to enable/disable the NMI pretimeout dynamically
> via ioctl. The pretimeout NMI can also be disabled via module parameter to hpwdt.
> This adds complexity without really adding functionality.
>
It looks like disabling pretimout will disable panics only for iLO 5
(mine system has iLO 2). Or am I missing something?
>
> BTW, don't reuse error messages. Makes it difficult to tell where the error
> originated from.
>
Would it be better to just return NMI_DONE and thus reuse normal NMI
handling from kernel, with logging and panic/don't panic logic?
>
> > return NMI_HANDLED;
> > }
> > @@ -376,6 +386,9 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> > #ifdef CONFIG_HPWDT_NMI_DECODING
> > module_param(pretimeout, bool, 0);
> > MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
> > -#endif
> > +
> > +module_param(panic_on_nmi, bool, 0);
> > +MODULE_PARM_DESC(panic_on_nmi, "Cause panic on NMI induced by iLO (default=1)");
> > +#endif /* CONFIG_HPWDT_NMI_DECODING */
> >
> > module_pci_driver(hpwdt_driver);
> > --
> > 2.20.1
next prev parent reply other threads:[~2019-02-02 5:13 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-14 2:36 [RFC PATCH 0/4] watchdog: hpwdt: Fix NMI-related behaviour when CONFIG_HPWDT_NMI_DECODING is enabled Ivan Mironov
2019-01-14 2:36 ` [RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI Ivan Mironov
2019-01-16 2:27 ` Jerry Hoemann
2019-01-16 2:52 ` Guenter Roeck
2019-02-02 4:55 ` Ivan Mironov
2019-02-08 1:26 ` Jerry Hoemann
2019-02-08 4:17 ` Guenter Roeck
2019-02-14 19:49 ` Ivan Mironov
2019-01-14 2:36 ` [RFC PATCH 2/4] watchdog: hpwdt: Don't panic on foreign NMI Ivan Mironov
2019-01-14 2:36 ` [RFC PATCH 3/4] watchdog: hpwdt: Add more information into message Ivan Mironov
2019-01-14 2:36 ` [RFC PATCH 4/4] watchdog: hpwdt: Make panic behaviour configurable Ivan Mironov
2019-01-16 2:30 ` Jerry Hoemann
2019-02-02 5:13 ` Ivan Mironov [this message]
2019-01-16 2:22 ` [RFC PATCH 0/4] watchdog: hpwdt: Fix NMI-related behaviour when CONFIG_HPWDT_NMI_DECODING is enabled Jerry Hoemann
2019-02-02 6:24 ` Ivan Mironov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6f3eec21920413fdbc774d89a6f6bbc2c7863370.camel@gmail.com \
--to=mironov.ivan@gmail.com \
--cc=Jerry.Hoemann@hpe.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).