[PATCH v2 0/5] watchdog: hpwdt: Bug Fixes/Enhancement

[PATCH v2 0/5] watchdog: hpwdt: Bug Fixes/Enhancement

[Jerry Hoemann]

Changes for v2

1) Patch 0001: Simplify initialization of pretimeout removing #ifdef.
2) Patch 0002: Loosen check on mynmi to accommodate potential FW issue.
3) Patch 0003: Split dev_info into mulitple calls as output was getting long.
4) Patch 0004: New: Add alias to module parameter soft_margin.


-----------------------------

Two defect fixes and one enhancement.  

0001: Initialize pretimeout from module parameter.

Bug Fix.

Pretimeout was getting programmed correctly in the hardware,
but this issue caused it to not be displayed correctly
in sysfs.



0002: Claim NMI from iLO

Bug Fix.

Default configuration worked, but if user were to disable the
pretimeout for the watchdog, then an explicit request to NMI
the system from the iLO virutal button would fail.


0003: Display module parameters

Enhancement.

Display all the module parameters when loading the driver.
Makes it easier to diagnose problems.


Jerry


Jerry Hoemann (5):
  watchdog: hpwdt: Initialize pretimeout from module parameter.
  watchdog: hpwdt: Claim NMI from iLO
  watchdog: hpwdt: Display module parameters.
  watchdog: hpwdt: Module paramerter alias.
  watchdog: hpwdt: Update version number.

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

-- 
1.8.3.1

[PATCH v2 1/5] watchdog: hpwdt: Initialize pretimeout from module parameter.

[Jerry Hoemann]

When the pretimeout is specified as a module parameter, the
value should be reflected in hpwdt_dev.pretimeout.  The default
(on) case is correct.  But, when disabling pretimeout, the value
should be set to zero in hpwdt_dev.

When compiling w/o CONFIG_HPWDT_NMI_DECODING defined, the pretimeout
module parameter is ignored and the value internally will be 0.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 9dc62a4..fae9364 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -205,9 +205,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 	.min_timeout	= 1,
 	.max_timeout	= HPWDT_MAX_TIMER,
 	.timeout	= DEFAULT_MARGIN,
-#ifdef CONFIG_HPWDT_NMI_DECODING
 	.pretimeout	= PRETIMEOUT_SEC,
-#endif
 };
 
 
@@ -313,6 +311,8 @@ static int hpwdt_init_one(struct pci_dev *dev,
 	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
 		dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin);
 
+	hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
+
 	hpwdt_dev.parent = &dev->dev;
 	retval = watchdog_register_device(&hpwdt_dev);
 	if (retval < 0) {
-- 
1.8.3.1

Re: [PATCH v2 1/5] watchdog: hpwdt: Initialize pretimeout from module parameter.

[Guenter Roeck]

On 08/08/2018 12:13 PM, Jerry Hoemann wrote:
> When the pretimeout is specified as a module parameter, the
> value should be reflected in hpwdt_dev.pretimeout.  The default
> (on) case is correct.  But, when disabling pretimeout, the value
> should be set to zero in hpwdt_dev.
> 
> When compiling w/o CONFIG_HPWDT_NMI_DECODING defined, the pretimeout
> module parameter is ignored and the value internally will be 0.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/hpwdt.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 9dc62a4..fae9364 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -205,9 +205,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>   	.min_timeout	= 1,
>   	.max_timeout	= HPWDT_MAX_TIMER,
>   	.timeout	= DEFAULT_MARGIN,
> -#ifdef CONFIG_HPWDT_NMI_DECODING
>   	.pretimeout	= PRETIMEOUT_SEC,
> -#endif
>   };
>   
>   
> @@ -313,6 +311,8 @@ static int hpwdt_init_one(struct pci_dev *dev,
>   	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
>   		dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin);
>   
> +	hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
> +
>   	hpwdt_dev.parent = &dev->dev;
>   	retval = watchdog_register_device(&hpwdt_dev);
>   	if (retval < 0) {
> 

[PATCH v2 2/5] watchdog: hpwdt: Claim NMI from iLO

[Jerry Hoemann]

The hwpdt driver is overloaded for handling both the iLO
watchdog and the explicit "Generate NMI to System" virutal
button.  Hence NMI handler needs to claim NMI resulting
from the virutal button.

Claim if iLO generated accommodating firmware that might
set wrong bit.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index fae9364..bb41714 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -162,7 +162,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 	if (ilo5 && ulReason == NMI_UNKNOWN && !mynmi)
 		return NMI_DONE;
 
-	if (ilo5 && !pretimeout)
+	if (ilo5 && !pretimeout && !mynmi)
 		return NMI_DONE;
 
 	hpwdt_stop();
-- 
1.8.3.1

Re: [PATCH v2 2/5] watchdog: hpwdt: Claim NMI from iLO

[Guenter Roeck]

On 08/08/2018 12:13 PM, Jerry Hoemann wrote:
> The hwpdt driver is overloaded for handling both the iLO
> watchdog and the explicit "Generate NMI to System" virutal
> button.  Hence NMI handler needs to claim NMI resulting
> from the virutal button.
> 
> Claim if iLO generated accommodating firmware that might
> set wrong bit.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/hpwdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index fae9364..bb41714 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -162,7 +162,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>   	if (ilo5 && ulReason == NMI_UNKNOWN && !mynmi)
>   		return NMI_DONE;
>   
> -	if (ilo5 && !pretimeout)
> +	if (ilo5 && !pretimeout && !mynmi)
>   		return NMI_DONE;
>   
>   	hpwdt_stop();
> 

[PATCH v2 3/5] watchdog: hpwdt: Display module parameters.

[Jerry Hoemann]

Print module parameters when the driver is loaded.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index bb41714..69a88b1 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -320,9 +320,12 @@ static int hpwdt_init_one(struct pci_dev *dev,
 		goto error_wd_register;
 	}
 
-	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
-			", timer margin: %d seconds (nowayout=%d).\n",
-			HPWDT_VERSION, hpwdt_dev.timeout, nowayout);
+	dev_info(&dev->dev, "HPE Watchdog Timer Driver: Version: %s\n",
+				HPWDT_VERSION);
+	dev_info(&dev->dev, "timeout: %d seconds (nowayout=%d)\n",
+				hpwdt_dev.timeout, nowayout);
+	dev_info(&dev->dev, "pretimeout: %s.\n",
+				pretimeout ? "on" : "off");
 
 	if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR)
 		ilo5 = true;
-- 
1.8.3.1

Re: [PATCH v2 3/5] watchdog: hpwdt: Display module parameters.

[Guenter Roeck]

On 08/08/2018 12:13 PM, Jerry Hoemann wrote:
> Print module parameters when the driver is loaded.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/hpwdt.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index bb41714..69a88b1 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -320,9 +320,12 @@ static int hpwdt_init_one(struct pci_dev *dev,
>   		goto error_wd_register;
>   	}
>   
> -	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
> -			", timer margin: %d seconds (nowayout=%d).\n",
> -			HPWDT_VERSION, hpwdt_dev.timeout, nowayout);
> +	dev_info(&dev->dev, "HPE Watchdog Timer Driver: Version: %s\n",
> +				HPWDT_VERSION);
> +	dev_info(&dev->dev, "timeout: %d seconds (nowayout=%d)\n",
> +				hpwdt_dev.timeout, nowayout);
> +	dev_info(&dev->dev, "pretimeout: %s.\n",
> +				pretimeout ? "on" : "off");
>   
>   	if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR)
>   		ilo5 = true;
> 

[PATCH v2 4/5] watchdog: hpwdt: Module paramerter alias.

[Jerry Hoemann]

Add module parameter "timeout" as an alias to "soft_margin."
This aligns hpwdt usage more closely with other WDT while
retaining backwards compatibility.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 69a88b1..eb947bc 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -367,6 +367,9 @@ static void hpwdt_exit(struct pci_dev *dev)
 module_param(soft_margin, int, 0);
 MODULE_PARM_DESC(soft_margin, "Watchdog timeout in seconds");
 
+module_param_named(timeout, soft_margin, int, 0);
+MODULE_PARM_DESC(timeout, "Alias of soft_margin");
+
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 		__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-- 
1.8.3.1

Re: [PATCH v2 4/5] watchdog: hpwdt: Module paramerter alias.

[Guenter Roeck]

On 08/08/2018 12:13 PM, Jerry Hoemann wrote:
> Add module parameter "timeout" as an alias to "soft_margin."
> This aligns hpwdt usage more closely with other WDT while
> retaining backwards compatibility.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/hpwdt.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 69a88b1..eb947bc 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -367,6 +367,9 @@ static void hpwdt_exit(struct pci_dev *dev)
>   module_param(soft_margin, int, 0);
>   MODULE_PARM_DESC(soft_margin, "Watchdog timeout in seconds");
>   
> +module_param_named(timeout, soft_margin, int, 0);
> +MODULE_PARM_DESC(timeout, "Alias of soft_margin");
> +
>   module_param(nowayout, bool, 0);
>   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>   		__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> 

[PATCH v2 5/5] watchdog: hpwdt: Update version number.

[Jerry Hoemann]

Bump version number to reflect recent bug fixes.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index eb947bc..7af358b 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -26,7 +26,7 @@
 #include <linux/watchdog.h>
 #include <asm/nmi.h>
 
-#define HPWDT_VERSION			"2.0.0"
+#define HPWDT_VERSION			"2.0.1"
 #define SECS_TO_TICKS(secs)		((secs) * 1000 / 128)
 #define TICKS_TO_SECS(ticks)		((ticks) * 128 / 1000)
 #define HPWDT_MAX_TIMER			TICKS_TO_SECS(65535)
-- 
1.8.3.1

Re: [PATCH v2 5/5] watchdog: hpwdt: Update version number.

[Guenter Roeck]

On 08/08/2018 12:13 PM, Jerry Hoemann wrote:
> Bump version number to reflect recent bug fixes.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/hpwdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index eb947bc..7af358b 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -26,7 +26,7 @@
>   #include <linux/watchdog.h>
>   #include <asm/nmi.h>
>   
> -#define HPWDT_VERSION			"2.0.0"
> +#define HPWDT_VERSION			"2.0.1"
>   #define SECS_TO_TICKS(secs)		((secs) * 1000 / 128)
>   #define TICKS_TO_SECS(ticks)		((ticks) * 128 / 1000)
>   #define HPWDT_MAX_TIMER			TICKS_TO_SECS(65535)
> 

Re: [PATCH v2 0/5] watchdog: hpwdt: Bug Fixes/Enhancement

[Jerry Hoemann]

On Wed, Aug 08, 2018 at 01:13:22PM -0600, Jerry Hoemann wrote:
> Changes for v2
> 
> 1) Patch 0001: Simplify initialization of pretimeout removing #ifdef.
> 2) Patch 0002: Loosen check on mynmi to accommodate potential FW issue.
> 3) Patch 0003: Split dev_info into mulitple calls as output was getting long.
> 4) Patch 0004: New: Add alias to module parameter soft_margin.
> 

Wim,

I would like to get our Distro partners to back port these changes,
but they must first be upstream.  :)

Do you know when you will include these in a pull request so that I can
pass on to them when to schedule the work?

Thanks

The patch e-mails are at:
	https://lkml.org/lkml/2018/8/8/769

Jerry

> 
> -----------------------------
> 
> Two defect fixes and one enhancement.  
> 
> 0001: Initialize pretimeout from module parameter.
> 
> Bug Fix.
> 
> Pretimeout was getting programmed correctly in the hardware,
> but this issue caused it to not be displayed correctly
> in sysfs.
> 
> 
> 
> 0002: Claim NMI from iLO
> 
> Bug Fix.
> 
> Default configuration worked, but if user were to disable the
> pretimeout for the watchdog, then an explicit request to NMI
> the system from the iLO virutal button would fail.
> 
> 
> 0003: Display module parameters
> 
> Enhancement.
> 
> Display all the module parameters when loading the driver.
> Makes it easier to diagnose problems.
> 
> 
> Jerry
> 
> 
> Jerry Hoemann (5):
>   watchdog: hpwdt: Initialize pretimeout from module parameter.
>   watchdog: hpwdt: Claim NMI from iLO
>   watchdog: hpwdt: Display module parameters.
>   watchdog: hpwdt: Module paramerter alias.
>   watchdog: hpwdt: Update version number.
> 
>  drivers/watchdog/hpwdt.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> -- 
> 1.8.3.1

-- 

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