2018-08-02 21:25:46

by Jerry Hoemann

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


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



2018-08-02 21:25:27

by Jerry Hoemann

[permalink] [raw]
Subject: [PATCH 4/4] watchdog: hpwdt: Update version number.

Bump version number to reflect recent bug fixes.

Signed-off-by: Jerry Hoemann <[email protected]>
---
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


2018-08-02 21:25:35

by Jerry Hoemann

[permalink] [raw]
Subject: [PATCH 3/4] watchdog: hpwdt: Display module parameters.

Print module parameters when the driver is loaded.

Signed-off-by: Jerry Hoemann <[email protected]>
---
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


2018-08-02 21:25:56

by Jerry Hoemann

[permalink] [raw]
Subject: [PATCH 1/4] watchdog: hpwdt: Initialize pretimeout from module parameter.

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 <[email protected]>
---
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


2018-08-02 21:26:44

by Jerry Hoemann

[permalink] [raw]
Subject: [PATCH 2/4] watchdog: hpwdt: Claim NMI from iLO

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 <[email protected]>
---
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


2018-08-05 01:09:22

by Guenter Roeck

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

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 <[email protected]>
> ---
> 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

2018-08-05 01:10:17

by Guenter Roeck

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

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 <[email protected]>

Guess you know what you are doing here.

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> 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();
>


2018-08-05 01:14:25

by Guenter Roeck

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

On 08/02/2018 02:15 PM, Jerry Hoemann wrote:
> Print module parameters when the driver is loaded.
>
> Signed-off-by: Jerry Hoemann <[email protected]>
> ---
> 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;
>


2018-08-05 01:17:06

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 4/4] watchdog: hpwdt: Update version number.

On 08/02/2018 02:15 PM, Jerry Hoemann wrote:
> Bump version number to reflect recent bug fixes.
>
> Signed-off-by: Jerry Hoemann <[email protected]>

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 <[email protected]>

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)
>


2018-08-06 23:23:52

by Jerry Hoemann

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

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 <[email protected]>
> > ---
> > 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
-----------------------------------------------------------------------------

2018-08-07 02:30:09

by Guenter Roeck

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

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 <[email protected]>
>>> ---
>>> 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

2018-08-07 15:47:32

by Jerry Hoemann

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

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 <[email protected]>
> > ---
> > 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
-----------------------------------------------------------------------------

2018-08-08 19:13:15

by Jerry Hoemann

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

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 <[email protected]>
>
> Guess you know what you are doing here.
>
> Reviewed-by: Guenter Roeck <[email protected]>

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
-----------------------------------------------------------------------------