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