[PATCH 0/4] watchdog: hpwdt: Bug Fixes/Enhancement

[PATCH 0/4] watchdog: hpwdt: Bug Fixes/Enhancement

[Jerry Hoemann]

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.


0004: Update version number.

Bump version number to reflect changes.




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

 drivers/watchdog/hpwdt.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] 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.

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

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 9dc62a4..369022d 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -313,6 +313,11 @@ 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);
 
+#ifdef CONFIG_HPWDT_NMI_DECODING
+	if (!pretimeout)
+		hpwdt_dev.pretimeout = 0;
+#endif
+
 	hpwdt_dev.parent = &dev->dev;
 	retval = watchdog_register_device(&hpwdt_dev);
 	if (retval < 0) {
-- 
1.8.3.1

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

[Guenter Roeck]

On 08/02/2018 02:15 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.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>   drivers/watchdog/hpwdt.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 9dc62a4..369022d 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -313,6 +313,11 @@ 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);
>   
> +#ifdef CONFIG_HPWDT_NMI_DECODING
> +	if (!pretimeout)
> +		hpwdt_dev.pretimeout = 0;
> +#endif
> +

Seems to me that
	hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
would accomplish the same without ifdef. Also, that would make
the conditional initialization in hpwdt_dev unnecessary,
saving us some more ifdefs.

Guenter

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

[Jerry Hoemann]

On Sat, Aug 04, 2018 at 06:08:17PM -0700, Guenter Roeck wrote:
> On 08/02/2018 02:15 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.
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >   drivers/watchdog/hpwdt.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index 9dc62a4..369022d 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -313,6 +313,11 @@ 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);
> > +#ifdef CONFIG_HPWDT_NMI_DECODING
> > +	if (!pretimeout)
> > +		hpwdt_dev.pretimeout = 0;
> > +#endif
> > +
> 
> Seems to me that
> 	hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
> would accomplish the same without ifdef. Also, that would make
> the conditional initialization in hpwdt_dev unnecessary,
> saving us some more ifdefs.
> 
> Guenter

Will do.

Thanks.

-- 

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

[PATCH 2/4] 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.

Claim the iLO NMI virtual button even if we are not claiming
the iLO watchdog pretimeout.

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 369022d..8a85ddd 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 & 0x4))
 		return NMI_DONE;
 
 	hpwdt_stop();
-- 
1.8.3.1

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

[Guenter Roeck]

On 08/02/2018 02:15 PM, Jerry Hoemann wrote:
> The hwpdt driver is overloaded for handling both the iLO
> watchdog and the explicit "Generate NMI to System" virutal
> button.
> 
> Claim the iLO NMI virtual button even if we are not claiming
> the iLO watchdog pretimeout.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

Guess you know what you are doing here.

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 369022d..8a85ddd 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 & 0x4))
>   		return NMI_DONE;
>   
>   	hpwdt_stop();
> 

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

[Jerry Hoemann]

On Sat, Aug 04, 2018 at 06:09:05PM -0700, Guenter Roeck wrote:
> On 08/02/2018 02:15 PM, Jerry Hoemann wrote:
> > The hwpdt driver is overloaded for handling both the iLO
> > watchdog and the explicit "Generate NMI to System" virutal
> > button.
> > 
> > Claim the iLO NMI virtual button even if we are not claiming
> > the iLO watchdog pretimeout.
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> 
> Guess you know what you are doing here.
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Unfortunately the underlying documentation isn't publically available.
I am going loosen the check in version two, but current upstream
is definitely wrong for reasons above.

> 
> > ---
> >   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 369022d..8a85ddd 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 & 0x4))
> >   		return NMI_DONE;
> >   	hpwdt_stop();
> > 

-- 

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

[PATCH 3/4] 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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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

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

[Guenter Roeck]

On 08/02/2018 02:15 PM, Jerry Hoemann wrote:
> Print module parameters when the driver is loaded.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>   drivers/watchdog/hpwdt.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 8a85ddd..f098371 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -326,8 +326,9 @@ static int hpwdt_init_one(struct pci_dev *dev,
>   	}
>   
>   	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
> -			", timer margin: %d seconds (nowayout=%d).\n",
> -			HPWDT_VERSION, hpwdt_dev.timeout, nowayout);
> +		", timeout : %d seconds (nowayout=%d) pretimeout=%s.\n",
> +		HPWDT_VERSION, hpwdt_dev.timeout, nowayout,
> +		pretimeout ? "on" : "off");
>   
When touching that, you might as well address

WARNING: quoted string split across lines

Why did you add a space before ':' ? Personal preference ?

Guenter

>   	if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR)
>   		ilo5 = true;
> 

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

[Jerry Hoemann]

On Sat, Aug 04, 2018 at 06:13:20PM -0700, Guenter Roeck wrote:
> On 08/02/2018 02:15 PM, Jerry Hoemann wrote:
> > Print module parameters when the driver is loaded.
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >   drivers/watchdog/hpwdt.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index 8a85ddd..f098371 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -326,8 +326,9 @@ static int hpwdt_init_one(struct pci_dev *dev,
> >   	}
> >   	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
> > -			", timer margin: %d seconds (nowayout=%d).\n",
> > -			HPWDT_VERSION, hpwdt_dev.timeout, nowayout);
> > +		", timeout : %d seconds (nowayout=%d) pretimeout=%s.\n",
> > +		HPWDT_VERSION, hpwdt_dev.timeout, nowayout,
> > +		pretimeout ? "on" : "off");
> When touching that, you might as well address
> 
> WARNING: quoted string split across lines


k. Think I'll split into two dev_info calls as line is too long
to fit into 80 chars w/o splitting.


> 
> Why did you add a space before ':' ? Personal preference ?

I think you're referring to "timeout : %d seconds".  Bad editting when
going from "timer margin:" to "timeout :".  I'll fix.

If you referring to the spaces around the ternary operator, that is
in coding-style although checkpatch accepts w/o spaces around the
operators.


> 
> Guenter
> 
> >   	if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR)
> >   		ilo5 = true;
> > 

-- 

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

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

[Guenter Roeck]

On 08/06/2018 04:19 PM, Jerry Hoemann wrote:
> On Sat, Aug 04, 2018 at 06:13:20PM -0700, Guenter Roeck wrote:
>> On 08/02/2018 02:15 PM, Jerry Hoemann wrote:
>>> Print module parameters when the driver is loaded.
>>>
>>> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
>>> ---
>>>    drivers/watchdog/hpwdt.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
>>> index 8a85ddd..f098371 100644
>>> --- a/drivers/watchdog/hpwdt.c
>>> +++ b/drivers/watchdog/hpwdt.c
>>> @@ -326,8 +326,9 @@ static int hpwdt_init_one(struct pci_dev *dev,
>>>    	}
>>>    	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
>>> -			", timer margin: %d seconds (nowayout=%d).\n",
>>> -			HPWDT_VERSION, hpwdt_dev.timeout, nowayout);
>>> +		", timeout : %d seconds (nowayout=%d) pretimeout=%s.\n",
>>> +		HPWDT_VERSION, hpwdt_dev.timeout, nowayout,
>>> +		pretimeout ? "on" : "off");
>> When touching that, you might as well address
>>
>> WARNING: quoted string split across lines
> 
> 
> k. Think I'll split into two dev_info calls as line is too long
> to fit into 80 chars w/o splitting.
> 
> 
>>
>> Why did you add a space before ':' ? Personal preference ?
> 
> I think you're referring to "timeout : %d seconds".  Bad editting when
> going from "timer margin:" to "timeout :".  I'll fix.
> 

Yes, that is what I referred to.

> If you referring to the spaces around the ternary operator, that is
> in coding-style although checkpatch accepts w/o spaces around the
> operators.
> 
Nope, those spaces are desirable.

Guenter

[PATCH 4/4] 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 f098371..27091f3 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 4/4] watchdog: hpwdt: Update version number.

[Guenter Roeck]

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

As usual, my recommendation is to drop version numbers. The version number is
reflected by the Linux kernel version and SHA. Yet, I understand that people
feel quite strong about having version numbers in drivers, so,

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

Just don't expect anyone outside hpe to keep it up to date.

Guenter

> ---
>   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 f098371..27091f3 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)
>