RE: [RFC] watchdog: iTCO_wdt: Introduce panic_on_timeout module param

["Julius Hemanth Pitti -X (jpitti - MONTA VISTA SOFTWARE INC at Cisco)" <jpitti@cisco.com>]

Hi Philip,

I learned about "unknown_nmi_panic",  which servers the same purpose, hence I stopped working on this patch.
BTW, I tested it on Xeon C3539 and Xeon L5238.


-----Original Message-----
From: Philip Prindeville [mailto:philipp_subx@redfish-solutions.com] 
Sent: Monday, September 18, 2017 7:52 PM
To: Julius Hemanth Pitti -X (jpitti - MONTA VISTA SOFTWARE INC at Cisco) <jpitti@cisco.com>
Cc: Wim Van Sebroeck <wim@iguana.be>; Guenter Roeck <linux@roeck-us.net>; xe-kernel@external.cisco.com; jpitti@mvista.com; linux-watchdog@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [RFC] watchdog: iTCO_wdt: Introduce panic_on_timeout module param

Sorry for being late to the party…

I tried to use this patch backported to 4.9.49 but could only get the system to reset (presumably the SMI but not certain since I’m not that current on BMC/SMI architecture) when it hung.

It’s a Lanner FW-8771 with AMI BIOS 4.6.5 on a Xeon E3-1225 v3.

Also, it seems to me that the TCO stuff has several versions (that affect bit placement and semantics) but I’m assuming that this code hasn’t been tested on all versions?

Thus special handling might be required based on the generation of TCO that’s on-chip which I’m not seeing here.  What processor was this code developed/tested on?

Comments on the particulars of the code itself inline.


> On Dec 5, 2016, at 3:48 PM, Julius Hemanth Pitti <jpitti@cisco.com> wrote:
> 
> Currently iTCO_wdt silently resets the board when timeout occurs.
> 
> This patch introduces new "panic_on_timeout" module param, which when 
> set allows the iTCO_wdt to call panic when watchdog timeout occurs, 
> this help to boot to crash kernel and collect core dump for further 
> analysis.
> 
> Cc: xe-kernel@external.cisco.com
> Cc: jpitti@mvista.com
> Signed-off-by: Julius Hemanth Pitti <jpitti@cisco.com>
> ---
> drivers/watchdog/iTCO_wdt.c | 61 
> +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c 
> index 06fcb6c..23ddcf4 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -66,6 +66,7 @@
> #include <linux/spinlock.h>		/* For spin_lock/spin_unlock/... */
> #include <linux/uaccess.h>		/* For copy_to_user/put_user/... */
> #include <linux/io.h>			/* For inb/outb/... */
> +#include <linux/nmi.h>
> #include <linux/platform_data/itco_wdt.h>
> 
> #include "iTCO_vendor.h"
> @@ -76,6 +77,24 @@
> /* SMI Control and Enable Register */
> #define SMI_EN		(iTCO_wdt_private.smi_res->start)
> 
> +static int panic_on_timeout;


Don’t seem the point of making this ‘int’ rather than ‘bool’.


> +module_param(panic_on_timeout, int, 0);


Ditto


> +MODULE_PARM_DESC(panic_on_timeout,
> +		 "Panic on NMI instead of Reset (1 = panic), default=0.");
> +
> +/* NMI2SMI_EN is bit 9 of TCO1_CNT register
> + * Read/Write
> + * 0 = Normal NMI functionality.
> + * 1 = Forces all NMIs to instead cause SMIs
> + *     This depends on NMI_EN and GBL_SMI_EN bits.
> + */
> +#define NMI2SMI_EN	(1 << 9)
> +
> +/* NMI_NOW is bit 8 of TCO1_CNT register.
> + * Read/'Write to Clear'
> + */
> +#define NMI_NOW		(1 << 8)
> +
> #define TCO_RLD		(TCOBASE + 0x00) /* TCO Timer Reload and Curr. Value */
> #define TCOv1_TMR	(TCOBASE + 0x01) /* TCOv1 Timer Initial Value	*/
> #define TCO_DAT_IN	(TCOBASE + 0x02) /* TCO Data In Register	*/
> @@ -236,6 +255,15 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
> 	val &= 0xf7ff;
> 	outw(val, TCO1_CNT);
> 	val = inw(TCO1_CNT);
> +
> +	if (panic_on_timeout) {
> +		/* Make sure NMIs are allowed to fire */
> +		if (NMI2SMI_EN & val) {


The Yoda notation was already called out…


> +			val &= ~(NMI2SMI_EN);


… as were the extraneous parens.


> +			outw(val, TCO1_CNT);
> +			pr_info("NMIs are no longer routed to SMIs\n");
> +		}
> +	}
> 	spin_unlock(&iTCO_wdt_private.io_lock);
> 
> 	if (val & 0x0800)


Do we need:

+@@ -372,6 +373,7 @@ static unsigned int iTCO_wdt_get_timelef
+ 
+ static const struct watchdog_info ident = {
+       .options =              WDIOF_SETTIMEOUT |
++                              WDIOF_PRETIMEOUT |
+                               WDIOF_KEEPALIVEPING |
+                               WDIOF_MAGICCLOSE,
+       .firmware_version =     0,

also?  Because without WDIOF_PRETIMEOUT, it seems that watchdog_set_pretimeout() would return -EOPNOTSUPP when you try to do the ioctl(WDIOC_SETPRETIMEOUT)…


> @@ -422,6 +450,26 @@ static void iTCO_wdt_cleanup(void)
> 	iTCO_wdt_private.gcs_pmc = NULL;
> }
> 
> +/*
> + * iTCO_wdt_timeout_handler: Handler for watchdog timeout NMI event.
> + */
> +int iTCO_wdt_timeout_handler(unsigned int ulReason, struct pt_regs 
> +*regs) {
> +	unsigned long val32 = inw(TCO1_CNT);
> +
> +	if (val32 & NMI_NOW) {
> +		/* Clear NMI - Bit 8 within TCO1_CNT is write to clear */
> +		outw(val32, TCO1_CNT);
> +
> +		/* Crash the system */
> +		nmi_panic(regs, "iTCO_wdt: Watchdog timeout");
> +
> +		/* Never returns */
> +		return NMI_HANDLED;
> +	}
> +	return NMI_DONE;
> +}
> +
> static int iTCO_wdt_probe(struct platform_device *dev) {
> 	int ret = -ENODEV;
> @@ -552,11 +600,21 @@ static int iTCO_wdt_probe(struct platform_device *dev)
> 		goto unreg_tco;
> 	}
> 
> +	if (panic_on_timeout) {
> +		ret = register_nmi_handler(NMI_UNKNOWN, iTCO_wdt_timeout_handler, 0, "iTCO_wdt");
> +		if (ret != 0) {
> +			pr_err("cannot register NMI Handler for iTCO_wdt watchdog (err=%d)\n", ret);
> +			goto unreg_wd;
> +		}
> +	}
> +
> 	pr_info("initialized. heartbeat=%d sec (nowayout=%d)\n",
> 		heartbeat, nowayout);
> 
> 	return 0;
> 
> +unreg_wd:
> +	watchdog_unregister_device(&iTCO_wdt_watchdog_dev);
> unreg_tco:
> 	release_region(iTCO_wdt_private.tco_res->start,
> 			resource_size(iTCO_wdt_private.tco_res));
> @@ -581,6 +639,9 @@ static int iTCO_wdt_probe(struct platform_device 
> *dev)
> 
> static int iTCO_wdt_remove(struct platform_device *dev) {
> +	if (panic_on_timeout)
> +		unregister_nmi_handler(NMI_UNKNOWN, "iTCO_wdt");
> +
> 	if (iTCO_wdt_private.tco_res || iTCO_wdt_private.smi_res)
> 		iTCO_wdt_cleanup();
>