[RFC PATCH 0/4] watchdog: hpwdt: Fix NMI-related behaviour when CONFIG_HPWDT_NMI_DECODING is enabled

[RFC PATCH 0/4] watchdog: hpwdt: Fix NMI-related behaviour when CONFIG_HPWDT_NMI_DECODING is enabled

[Ivan Mironov]

Hi,

I found out that hpwdt alters NMI behaviour unexpectedly if compiled
with enabled CONFIG_HPWDT_NMI_DECODING:

 * System starts to panic on any NMI with misleading message.
 * Watchdog provided by hpwdt is not working after such panic.

Here are the patches that should fix this.

This is an RFC patch series because I am not sure that patches are
correct. Questions:

 * Are "mynmi" flags always set on all supported iLO versions when iLO
   is the source of NMI?
 * Is it safe to reset "mynmi" flags to zero if code decides to not panic?

Ivan Mironov (4):
  watchdog: hpwdt: Don't disable watchdog on NMI
  watchdog: hpwdt: Don't panic on foreign NMI
  watchdog: hpwdt: Add more information into message
  watchdog: hpwdt: Make panic behaviour configurable

 drivers/watchdog/hpwdt.c | 45 ++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

-- 
2.20.1

[RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI

[Ivan Mironov]

Existing code disables watchdog on NMI right before completely hanging
the system.

There are two problems here:

 * First, watchdog is expected to reset the system in a case of such
   failure, no matter what.
 * Second, this code has no effect if there are more than one watchdog.

Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
---
 drivers/watchdog/hpwdt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index ef30c7e9728d..2467e6bc25c2 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -170,8 +170,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 	if (ilo5 && !pretimeout && !mynmi)
 		return NMI_DONE;
 
-	hpwdt_stop();
-
 	hex_byte_pack(panic_msg, mynmi);
 	nmi_panic(regs, panic_msg);
 
-- 
2.20.1

Re: [RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI

[Jerry Hoemann]

On Mon, Jan 14, 2019 at 07:36:14AM +0500, Ivan Mironov wrote:
> Existing code disables watchdog on NMI right before completely hanging
> the system.
> 
> There are two problems here:
> 
>  * First, watchdog is expected to reset the system in a case of such
>    failure, no matter what.

Documentation/watchdog/watchdog-api.txt

explicitly allows for pretimeout NMI and generation of kernel crash dumps.

By removing hpwdt_stop the system will likely fail to crash dump
as there is only 9 seconds between receipt of a NMI and the iLO
resetting the system.

Unfortunately, kdump is not without issues and can also be difficult
to properly configure either of which can result in failure to dump
and reset.

Customers who value availability over kdump collection, the pretimeout
NMI can be disabled and hardware will not issue the pretimeout NMI
and will only do reset.

A middle ground for those who want tombstones but not kdump, would
be to leave the pretimeout NMI enabled and add "panic=N" to the
Linux command line.  That way after the panic, the tombstone is
printed and the system resets after N seconds.



>  * Second, this code has no effect if there are more than one watchdog.

That is correct.  Hpwdt will not turn off any other WDT.

I don't see a current method of notifying other watchdogs
that a given watchdog is going to take the system down.

The closest I hook see is watchdog_notify_pretimeout, but I don't
see that notifying other WDT.  Its not clear to me that it should.
(e.g. the second WDT could be of longer duration and protect against
kdump hanging. This would need to be thought through.)



> 
> Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
> ---
>  drivers/watchdog/hpwdt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index ef30c7e9728d..2467e6bc25c2 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -170,8 +170,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>  	if (ilo5 && !pretimeout && !mynmi)
>  		return NMI_DONE;
>  
> -	hpwdt_stop();
> -
>  	hex_byte_pack(panic_msg, mynmi);
>  	nmi_panic(regs, panic_msg);
>  
> -- 
> 2.20.1

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI

[Guenter Roeck]

On 1/15/19 6:27 PM, Jerry Hoemann wrote:
> On Mon, Jan 14, 2019 at 07:36:14AM +0500, Ivan Mironov wrote:
>> Existing code disables watchdog on NMI right before completely hanging
>> the system.
>>
>> There are two problems here:
>>
>>   * First, watchdog is expected to reset the system in a case of such
>>     failure, no matter what.
> 
> Documentation/watchdog/watchdog-api.txt
> 
> explicitly allows for pretimeout NMI and generation of kernel crash dumps.
> 
> By removing hpwdt_stop the system will likely fail to crash dump
> as there is only 9 seconds between receipt of a NMI and the iLO
> resetting the system.
> 
> Unfortunately, kdump is not without issues and can also be difficult
> to properly configure either of which can result in failure to dump
> and reset.
> 
> Customers who value availability over kdump collection, the pretimeout
> NMI can be disabled and hardware will not issue the pretimeout NMI
> and will only do reset.
> 
> A middle ground for those who want tombstones but not kdump, would
> be to leave the pretimeout NMI enabled and add "panic=N" to the
> Linux command line.  That way after the panic, the tombstone is
> printed and the system resets after N seconds.
> 
> 
> 
>>   * Second, this code has no effect if there are more than one watchdog.
> 
> That is correct.  Hpwdt will not turn off any other WDT.
> 
> I don't see a current method of notifying other watchdogs
> that a given watchdog is going to take the system down.
> 
And there should not be.

> The closest I hook see is watchdog_notify_pretimeout, but I don't
> see that notifying other WDT.  Its not clear to me that it should.
> (e.g. the second WDT could be of longer duration and protect against
> kdump hanging. This would need to be thought through.)
> 

Watchdogs are independent of each other. If there is more than one,
they need to be configured carefully (just like pretimeout vs. timeout).
It is not up to the kernel to let watchdogs interfere with each other.

Guenter

> 
> 
>>
>> Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
>> ---
>>   drivers/watchdog/hpwdt.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
>> index ef30c7e9728d..2467e6bc25c2 100644
>> --- a/drivers/watchdog/hpwdt.c
>> +++ b/drivers/watchdog/hpwdt.c
>> @@ -170,8 +170,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>>   	if (ilo5 && !pretimeout && !mynmi)
>>   		return NMI_DONE;
>>   
>> -	hpwdt_stop();
>> -
>>   	hex_byte_pack(panic_msg, mynmi);
>>   	nmi_panic(regs, panic_msg);
>>   
>> -- 
>> 2.20.1
> 

Re: [RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI

[Ivan Mironov]

On Tue, 2019-01-15 at 19:27 -0700, Jerry Hoemann wrote:
> On Mon, Jan 14, 2019 at 07:36:14AM +0500, Ivan Mironov wrote:
> > Existing code disables watchdog on NMI right before completely hanging
> > the system.
> > 
> > There are two problems here:
> > 
> >  * First, watchdog is expected to reset the system in a case of such
> >    failure, no matter what.
> 
> Documentation/watchdog/watchdog-api.txt
> 
> explicitly allows for pretimeout NMI and generation of kernel crash dumps.
> 
> By removing hpwdt_stop the system will likely fail to crash dump
> as there is only 9 seconds between receipt of a NMI and the iLO
> resetting the system.
> 
> Unfortunately, kdump is not without issues and can also be difficult
> to properly configure either of which can result in failure to dump
> and reset.
> 
> Customers who value availability over kdump collection, the pretimeout
> NMI can be disabled and hardware will not issue the pretimeout NMI
> and will only do reset.
> 
> A middle ground for those who want tombstones but not kdump, would
> be to leave the pretimeout NMI enabled and add "panic=N" to the
> Linux command line.  That way after the panic, the tombstone is
> printed and the system resets after N seconds.
> 
> 

Somehow I missed the whole pretimout thing when reading about the
watchdog API. Thanks for clarification, now code makes much more sense
=).

Still, I do not really understand the point of enabling of kdump
support in hpwdt driver by default while kdump is not enabled by
default.

Also, existing code may call hpwdt_stop() (and thus break watchdog)
even if pretimout is disabled.

Also, "panic=N" option is not providing a way to *not* panic on NMI
unrelated with iLO. This could be circumvented by blacklisting the
hpwdt module entirely, but normal watchdog functionality would be lost
then.

It is possible to rebuild kernel without HPWDT_NMI_DECODING (which is
enabled in Fedora, for example). But it is nearly impossible to come to
this solution without examining the source code, because description of
this option does not mention that it is really about pretimout support
and panics and not about something else...

I would say that current default behavior of hpwd is slightly confusing
in multiple different ways.

> 
> >  * Second, this code has no effect if there are more than one watchdog.
> 
> That is correct.  Hpwdt will not turn off any other WDT.
> 
> I don't see a current method of notifying other watchdogs
> that a given watchdog is going to take the system down.
> 
> The closest I hook see is watchdog_notify_pretimeout, but I don't
> see that notifying other WDT.  Its not clear to me that it should.
> (e.g. the second WDT could be of longer duration and protect against
> kdump hanging. This would need to be thought through.)
> 
> 
> 
> > Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
> > ---
> >  drivers/watchdog/hpwdt.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index ef30c7e9728d..2467e6bc25c2 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -170,8 +170,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
> >  	if (ilo5 && !pretimeout && !mynmi)
> >  		return NMI_DONE;
> >  
> > -	hpwdt_stop();
> > -
> >  	hex_byte_pack(panic_msg, mynmi);
> >  	nmi_panic(regs, panic_msg);
> >  
> > -- 
> > 2.20.1

Re: [RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI

[Jerry Hoemann]

On Sat, Feb 02, 2019 at 09:55:29AM +0500, Ivan Mironov wrote:
> On Tue, 2019-01-15 at 19:27 -0700, Jerry Hoemann wrote:
> > On Mon, Jan 14, 2019 at 07:36:14AM +0500, Ivan Mironov wrote:
> 
> Somehow I missed the whole pretimout thing when reading about the
> watchdog API. Thanks for clarification, now code makes much more sense
> =).
> 
> Still, I do not really understand the point of enabling of kdump
> support in hpwdt driver by default while kdump is not enabled by
> default.

Kdump is enabled by default by our Distro partners.

HPE works with distro partners to deliver a validated system which we support.

The ability to generate crash dumps is one of the means we use to
support our customers.  Even if kdump isn't configured, panic will
at least print stack trace to indicate system activity.


> 
> Also, existing code may call hpwdt_stop() (and thus break watchdog)
> even if pretimout is disabled.
> 
> Also, "panic=N" option is not providing a way to *not* panic on NMI
> unrelated with iLO. This could be circumvented by blacklisting the
> hpwdt module entirely, but normal watchdog functionality would be lost
> then.

panic=N provides for reset upon receipt of NMI if user wants timeout
to reset system but not a crash dump.

The panic is for error containment.  On the legacy systems within
the context of hpwdt_pretimeout we cannot determine if the error
is recoverable or not.  So, we have little choice but to panic.


> 
> It is possible to rebuild kernel without HPWDT_NMI_DECODING (which is
> enabled in Fedora, for example). But it is nearly impossible to come to
> this solution without examining the source code, because description of
> this option does not mention that it is really about pretimout support
> and panics and not about something else...

The name is not the best given its current use, but I'm not sure a
name change would be allowed.

However, I will send a patch to update the documentation in Kconfig.


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI

[Guenter Roeck]

On 2/7/19 5:26 PM, Jerry Hoemann wrote:
> On Sat, Feb 02, 2019 at 09:55:29AM +0500, Ivan Mironov wrote:
>> On Tue, 2019-01-15 at 19:27 -0700, Jerry Hoemann wrote:
>>> On Mon, Jan 14, 2019 at 07:36:14AM +0500, Ivan Mironov wrote:
>>
>> Somehow I missed the whole pretimout thing when reading about the
>> watchdog API. Thanks for clarification, now code makes much more sense
>> =).
>>
>> Still, I do not really understand the point of enabling of kdump
>> support in hpwdt driver by default while kdump is not enabled by
>> default.
> 
> Kdump is enabled by default by our Distro partners.
> 
> HPE works with distro partners to deliver a validated system which we support.
> 
> The ability to generate crash dumps is one of the means we use to
> support our customers.  Even if kdump isn't configured, panic will
> at least print stack trace to indicate system activity.
> 
> 
>>
>> Also, existing code may call hpwdt_stop() (and thus break watchdog)
>> even if pretimout is disabled.
>>
>> Also, "panic=N" option is not providing a way to *not* panic on NMI
>> unrelated with iLO. This could be circumvented by blacklisting the
>> hpwdt module entirely, but normal watchdog functionality would be lost
>> then.
> 
> panic=N provides for reset upon receipt of NMI if user wants timeout
> to reset system but not a crash dump.
> 
> The panic is for error containment.  On the legacy systems within
> the context of hpwdt_pretimeout we cannot determine if the error
> is recoverable or not.  So, we have little choice but to panic.
> 
> 
>>
>> It is possible to rebuild kernel without HPWDT_NMI_DECODING (which is
>> enabled in Fedora, for example). But it is nearly impossible to come to
>> this solution without examining the source code, because description of
>> this option does not mention that it is really about pretimout support
>> and panics and not about something else...
> 
> The name is not the best given its current use, but I'm not sure a
> name change would be allowed.
> 

I would be open to accepting an improved help text of this configuration
option. I am not going to entertain (or accept) a name change.
That would open up a can of worms, with everyone in the world requesting
name changes. That would be pretty pointless and make the kernel all but
unmanageable.

Guenter

> However, I will send a patch to update the documentation in Kconfig.
> 
> 

Re: [RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI

[Ivan Mironov]

On Thu, 2019-02-07 at 18:26 -0700, Jerry Hoemann wrote:
> The name is not the best given its current use, but I'm not sure a
> name change would be allowed.
> 
> However, I will send a patch to update the documentation in Kconfig.
> 
> 

Thanks!

[RFC PATCH 2/4] watchdog: hpwdt: Don't panic on foreign NMI

[Ivan Mironov]

Currently, hpwdt unconditionally panics on foreign NMIs in some cases.
This goes against the default kernel behaviour (which is configured by
unknown_nmi_panic and panic_on_unrecovered_nmi).

With this patch, hpwdt will simply ignore NMI unless one of "mynmi"
flags is set by iLO.

Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
---
 drivers/watchdog/hpwdt.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 2467e6bc25c2..e2958df46c69 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -33,7 +33,6 @@
 #define DEFAULT_MARGIN			30
 #define PRETIMEOUT_SEC			9
 
-static bool ilo5;
 static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);
@@ -164,10 +163,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 		"3. OA Forward Progress Log\n"
 		"4. iLO Event Log";
 
-	if (ilo5 && ulReason == NMI_UNKNOWN && !mynmi)
-		return NMI_DONE;
-
-	if (ilo5 && !pretimeout && !mynmi)
+	if (!mynmi)
 		return NMI_DONE;
 
 	hex_byte_pack(panic_msg, mynmi);
@@ -332,9 +328,6 @@ static int hpwdt_init_one(struct pci_dev *dev,
 	dev_info(&dev->dev, "pretimeout: %s.\n",
 				pretimeout ? "on" : "off");
 
-	if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR)
-		ilo5 = true;
-
 	return 0;
 
 error_wd_register:
-- 
2.20.1

[RFC PATCH 3/4] watchdog: hpwdt: Add more information into message

[Ivan Mironov]

Default NMI handling code prints the same into the kernel log.

Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
---
 drivers/watchdog/hpwdt.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index e2958df46c69..95d002b5b120 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -155,19 +155,20 @@ static int hpwdt_my_nmi(void)
 static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 {
 	unsigned int mynmi = hpwdt_my_nmi();
-	static char panic_msg[] =
-		"00: An NMI occurred. Depending on your system the reason "
-		"for the NMI is logged in any one of the following resources:\n"
-		"1. Integrated Management Log (IML)\n"
-		"2. OA Syslog\n"
-		"3. OA Forward Progress Log\n"
-		"4. iLO Event Log";
 
 	if (!mynmi)
 		return NMI_DONE;
 
-	hex_byte_pack(panic_msg, mynmi);
-	nmi_panic(regs, panic_msg);
+	pr_emerg("%02x: An NMI occurred (reason %02x, CPU %d). Depending on "
+		 "your system the reason for the NMI is logged in any one of "
+		 "the following resources:\n"
+		 "1. Integrated Management Log (IML)\n"
+		 "2. OA Syslog\n"
+		 "3. OA Forward Progress Log\n"
+		 "4. iLO Event Log\n",
+		 mynmi, ulReason, smp_processor_id());
+
+	nmi_panic(regs, "hpwdt: NMI: Not continuing");
 
 	return NMI_HANDLED;
 }
-- 
2.20.1

[RFC PATCH 4/4] watchdog: hpwdt: Make panic behaviour configurable

[Ivan Mironov]

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;
 }
 
 /*
@@ -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");
 
 	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

Re: [RFC PATCH 4/4] watchdog: hpwdt: Make panic behaviour configurable

[Jerry Hoemann]

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.


>  }
>  
>  /*
> @@ -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.


BTW, don't reuse error messages.  Makes it difficult to tell where the error
originated from.


>  	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

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [RFC PATCH 4/4] watchdog: hpwdt: Make panic behaviour configurable

[Ivan Mironov]

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

Re: [RFC PATCH 0/4] watchdog: hpwdt: Fix NMI-related behaviour when CONFIG_HPWDT_NMI_DECODING is enabled

[Jerry Hoemann]

On Mon, Jan 14, 2019 at 07:36:13AM +0500, Ivan Mironov wrote:
> Hi,
> 
> I found out that hpwdt alters NMI behaviour unexpectedly if compiled
> with enabled CONFIG_HPWDT_NMI_DECODING:
> 
>  * System starts to panic on any NMI with misleading message.

hpwdt doesn't start to panic on any NMI.  It starts to panic on:

1) NMI_SERR		associated with NMI
2) NMI_IO_CHECK		associated with IO errors
3) NMI_UNKNOWN		NMI unclaimed by all local handlers.

On Gen10 going forward we plan to restrict to just iLO
generated NMIs.

There is a long history on hp/hpe proliant systems where hpwdt
was handler of general IO errors (at least ones that would cause
NMI to be generated) and we chose to panic in these situation
as the errors were generally quite serious.

Yes, this has caused some problems in the past as Linux has
overloaded NMI and some subsystems didn't claim the NMIs that
they generated (think profiling.)  But, I haven't seen these
types of problems for several years now.

The more modern platforms have more robust error handling built
into them and to linux so going forward we'll restrict hpwdt to a more
traditional WDT role.  But we're retaining the more conservative
approach for legacy platforms.

How would you suggest that the message be enhanced?


>  * Watchdog provided by hpwdt is not working after such panic.
> 
> Here are the patches that should fix this.
> 
> This is an RFC patch series because I am not sure that patches are
> correct. Questions:
> 
>  * Are "mynmi" flags always set on all supported iLO versions when iLO
>    is the source of NMI?


Unfortunately no.

hpwdt is a dual purpose driver.  It handles the iLO watchdog timer
and the "Generate NMI to System" button.  These are closely related
hardware wise.

However, some platforms generate NMI for "Generate NMI to System" button but aren't
signaled via iLO registers.  These will show up as NMI_UNKNOWN, hence while
hpwdt still claims these.

There are also some systems that do not set the nmistat bits correctly.

So as to not break legacy platforms, the use the nmistat bits for
control will be for Gen10 going forward.



>  * Is it safe to reset "mynmi" flags to zero if code decides to not panic?

The reading of the registers is itself destructive (sets to zero) but the real
issue is that some proliant systems lack the ability to acknowledge the NMI so
only one can ever be received.  So returning is not advisable as no
further NMI will be generated via this path.  A reset through firmware
is required to restore the feature.


> 
> Ivan Mironov (4):
>   watchdog: hpwdt: Don't disable watchdog on NMI
>   watchdog: hpwdt: Don't panic on foreign NMI
>   watchdog: hpwdt: Add more information into message
>   watchdog: hpwdt: Make panic behaviour configurable
> 
>  drivers/watchdog/hpwdt.c | 45 ++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> -- 
> 2.20.1

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [RFC PATCH 0/4] watchdog: hpwdt: Fix NMI-related behaviour when CONFIG_HPWDT_NMI_DECODING is enabled

[Ivan Mironov]

On Tue, 2019-01-15 at 19:22 -0700, Jerry Hoemann wrote:
> On Mon, Jan 14, 2019 at 07:36:13AM +0500, Ivan Mironov wrote:
> > Hi,
> > 
> > I found out that hpwdt alters NMI behaviour unexpectedly if compiled
> > with enabled CONFIG_HPWDT_NMI_DECODING:
> > 
> >  * System starts to panic on any NMI with misleading message.
> 
> hpwdt doesn't start to panic on any NMI.  It starts to panic on:
> 
> 1) NMI_SERR		associated with NMI
> 2) NMI_IO_CHECK		associated with IO errors
> 3) NMI_UNKNOWN		NMI unclaimed by all local handlers.
> 
> On Gen10 going forward we plan to restrict to just iLO
> generated NMIs.
> 
> There is a long history on hp/hpe proliant systems where hpwdt
> was handler of general IO errors (at least ones that would cause
> NMI to be generated) and we chose to panic in these situation
> as the errors were generally quite serious.
> 

I would prefer to have this at least configurable by some module
parameter.

> Yes, this has caused some problems in the past as Linux has
> overloaded NMI and some subsystems didn't claim the NMIs that
> they generated (think profiling.)  But, I haven't seen these
> types of problems for several years now.
> 
> The more modern platforms have more robust error handling built
> into them and to linux so going forward we'll restrict hpwdt to a more
> traditional WDT role.  But we're retaining the more conservative
> approach for legacy platforms.
> 

I've seen NMI panic on my old ProLiant BL460c G6 at least once.
hpwdt.ko "handled" this NMI by disabling watchdog before hanging the
system 8). mynmi was equal to zero.

That is why I decided to check the code and try to understand how
exactly it works.

> How would you suggest that the message be enhanced?
> 

Maybe mention that "false positives" are possible and the actual reason
of NMI is not always logged in OA/iLO/etc. logs.

> 
> >  * Watchdog provided by hpwdt is not working after such panic.
> > 
> > Here are the patches that should fix this.
> > 
> > This is an RFC patch series because I am not sure that patches are
> > correct. Questions:
> > 
> >  * Are "mynmi" flags always set on all supported iLO versions when iLO
> >    is the source of NMI?
> 
> Unfortunately no.
> 
> hpwdt is a dual purpose driver.  It handles the iLO watchdog timer
> and the "Generate NMI to System" button.  These are closely related
> hardware wise.
> 
> However, some platforms generate NMI for "Generate NMI to System" button but aren't
> signaled via iLO registers.  These will show up as NMI_UNKNOWN, hence while
> hpwdt still claims these.
> 
> There are also some systems that do not set the nmistat bits correctly.
> 
> So as to not break legacy platforms, the use the nmistat bits for
> control will be for Gen10 going forward.
> 

It seems that iLO 2 sets these bits correctly. Bit 1 is set on
pretimout NMI, bit 2 is set on "iLO web button" NMI.

> 
> 
> >  * Is it safe to reset "mynmi" flags to zero if code decides to not panic?
> 
> The reading of the registers is itself destructive (sets to zero)

Could you elaborate what exactly you mean here?

I tried to read nmistat register multiple times using ioread8(), and
every time returned value were the same, with one of mynmi flags set.
Even with mdelay() between calls.

>  but the real
> issue is that some proliant systems lack the ability to acknowledge the NMI so
> only one can ever be received.  So returning is not advisable as no
> further NMI will be generated via this path.  A reset through firmware
> is required to restore the feature.
> 

Yes, I noticed this.

> 
> > Ivan Mironov (4):
> >   watchdog: hpwdt: Don't disable watchdog on NMI
> >   watchdog: hpwdt: Don't panic on foreign NMI
> >   watchdog: hpwdt: Add more information into message
> >   watchdog: hpwdt: Make panic behaviour configurable
> > 
> >  drivers/watchdog/hpwdt.c | 45 ++++++++++++++++++++++------------------
> >  1 file changed, 25 insertions(+), 20 deletions(-)
> > 
> > -- 
> > 2.20.1

By the way, is it possible to implement something like this
(pseudocode):

*******
bool handle_unknown_nmi_on_old_systems = true; // module parameter

int nmi_handler()
{
	if (mynmi_flags_supported(current_hw)) {
		if (mynmi & MYNMI_PRETIMOUT_FLAG) {
			if (pretimout) {
				hpwdt_stop();
				panic("hpwdt pretimout");
				return NMI_HANDLED;
			} else {
				warn("pretimout flag set, but pretimout
is not enabled, ignoring");
			}
		}

		if (mynmi & MYNMI_BUTTON_FLAG) {
			panic("iLO button pressed");
			return NMI_HANDLED;
		}
	} else if (handle_all_nmi_on_old_systems) {
		if (pretimout) {
			hpwdt_stop();
			panic("maybe hpwdt pretimout");
		} else {
			panic("unknown NMI, see OA/iLO logs...");
		}
		return NMI_HANDLED;
	}

	// Proceed with regular NMI handling code.
	return NMI_DONE;
}
*******

Or such logic does not make sense?