2019-05-17 22:41:20

by Jerry Hoemann

[permalink] [raw]
Subject: [PATCH 0/6] watchdog/hpwdt: cleanups and kdump accommodations

First two changes makes hpwdt more generic.
Next two changes make hpwdt work better with kdump.


Jerry Hoemann (6):
watchdog/hpwdt: Stop hpwdt on unregister.
watchdog/hpwdt: Advertize max_hw_heartbeat_ms
watchdog/hpwdt: Have core ping watchdog.
watchdog/hpwdt: Add module parameter kdumptimeout.
watchdog/hpwdt: Update documentation
watchdog/hpwdt: Reflect changes

Documentation/watchdog/hpwdt.txt | 4 +++
drivers/watchdog/hpwdt.c | 55 ++++++++++++++++++++++++++++++----------
2 files changed, 45 insertions(+), 14 deletions(-)

--
1.8.3.1


2019-05-17 22:42:27

by Jerry Hoemann

[permalink] [raw]
Subject: [PATCH 1/6] watchdog/hpwdt: Stop hpwdt on unregister.

Have the WD core stop the watchdog on unregister instead of explicitly
calling hpwdt_stop() in hpwdt_exit().

Signed-off-by: Jerry Hoemann <[email protected]>
---
drivers/watchdog/hpwdt.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index ef30c7e..8c49f13 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -310,6 +310,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
if (retval != 0)
goto error_init_nmi_decoding;

+ watchdog_stop_on_unregister(&hpwdt_dev);
watchdog_set_nowayout(&hpwdt_dev, nowayout);
if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin);
@@ -350,9 +351,6 @@ static int hpwdt_init_one(struct pci_dev *dev,

static void hpwdt_exit(struct pci_dev *dev)
{
- if (!nowayout)
- hpwdt_stop();
-
watchdog_unregister_device(&hpwdt_dev);
hpwdt_exit_nmi_decoding();
pci_iounmap(dev, pci_mem_addr);
--
1.8.3.1

2019-05-17 22:50:28

by Jerry Hoemann

[permalink] [raw]
Subject: [PATCH 2/6] watchdog/hpwdt: Advertize max_hw_heartbeat_ms

Set max_hw_heartbeat_ms instead of max_timeout so that user client can
set timeout range in excess of what the underlying hardware supports.

Signed-off-by: Jerry Hoemann <[email protected]>
---
drivers/watchdog/hpwdt.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 8c49f13..9f7a370 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -62,9 +62,9 @@
static int hpwdt_start(struct watchdog_device *wdd)
{
int control = 0x81 | (pretimeout ? 0x4 : 0);
- int reload = SECS_TO_TICKS(wdd->timeout);
+ int reload = SECS_TO_TICKS(min(wdd->timeout, wdd->max_hw_heartbeat_ms/1000));

- dev_dbg(wdd->parent, "start watchdog 0x%08x:0x%02x\n", reload, control);
+ dev_dbg(wdd->parent, "start watchdog 0x%08x:0x%08x:0x%02x\n", wdd->timeout, reload, control);
iowrite16(reload, hpwdt_timer_reg);
iowrite8(control, hpwdt_timer_con);

@@ -91,9 +91,9 @@ static int hpwdt_stop_core(struct watchdog_device *wdd)

static int hpwdt_ping(struct watchdog_device *wdd)
{
- int reload = SECS_TO_TICKS(wdd->timeout);
+ int reload = SECS_TO_TICKS(min(wdd->timeout, wdd->max_hw_heartbeat_ms/1000));

- dev_dbg(wdd->parent, "ping watchdog 0x%08x\n", reload);
+ dev_dbg(wdd->parent, "ping watchdog 0x%08x:0x%08x\n", wdd->timeout, reload);
iowrite16(reload, hpwdt_timer_reg);

return 0;
@@ -208,9 +208,9 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
.info = &ident,
.ops = &hpwdt_ops,
.min_timeout = 1,
- .max_timeout = HPWDT_MAX_TIMER,
.timeout = DEFAULT_MARGIN,
.pretimeout = PRETIMEOUT_SEC,
+ .max_hw_heartbeat_ms = HPWDT_MAX_TIMER * 1000,
};


--
1.8.3.1

2019-05-17 22:54:57

by Jerry Hoemann

[permalink] [raw]
Subject: [PATCH 4/6] watchdog/hpwdt: Add module parameter kdumptimeout.

Instead of unconditionally stopping the watchdog timer after receipt of
a pretimeout NMI, reprogram the timeout based upon module parameter
kdumptimeout.

The provides a more flexible override than the depricated allow_kdump.

Signed-off-by: Jerry Hoemann <[email protected]>
---
drivers/watchdog/hpwdt.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index aa4101c..dc65006 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -29,7 +29,8 @@
#define HPWDT_VERSION "2.0.2"
#define SECS_TO_TICKS(secs) ((secs) * 1000 / 128)
#define TICKS_TO_SECS(ticks) ((ticks) * 128 / 1000)
-#define HPWDT_MAX_TIMER TICKS_TO_SECS(65535)
+#define HPWDT_MAX_TICKS 65535
+#define HPWDT_MAX_TIMER TICKS_TO_SECS(HPWDT_MAX_TICKS)
#define DEFAULT_MARGIN 30
#define PRETIMEOUT_SEC 9

@@ -37,6 +38,7 @@
static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */
static bool nowayout = WATCHDOG_NOWAYOUT;
static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);
+static int kdumptimeout = -1;

static void __iomem *pci_mem_addr; /* the PCI-memory address */
static unsigned long __iomem *hpwdt_nmistat;
@@ -56,6 +58,7 @@
{0}, /* terminate list */
};

+static struct watchdog_device hpwdt_dev;
/*
* Watchdog operations
*/
@@ -94,12 +97,18 @@ static int hpwdt_stop_core(struct watchdog_device *wdd)
return 0;
}

+static void hpwdt_ping_ticks(int val)
+{
+ val = min(val, HPWDT_MAX_TICKS);
+ iowrite16(val, hpwdt_timer_reg);
+}
+
static int hpwdt_ping(struct watchdog_device *wdd)
{
int reload = SECS_TO_TICKS(min(wdd->timeout, wdd->max_hw_heartbeat_ms/1000));

dev_dbg(wdd->parent, "ping watchdog 0x%08x:0x%08x\n", wdd->timeout, reload);
- iowrite16(reload, hpwdt_timer_reg);
+ hpwdt_ping_ticks(reload);

return 0;
}
@@ -175,7 +184,14 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
if (ilo5 && !pretimeout && !mynmi)
return NMI_DONE;

- hpwdt_stop();
+ if (kdumptimeout < 0)
+ hpwdt_stop();
+ else if (kdumptimeout == 0)
+ ;
+ else {
+ unsigned int val = max((unsigned int)kdumptimeout, hpwdt_dev.timeout);
+ hpwdt_ping_ticks(SECS_TO_TICKS(val));
+ }

hex_byte_pack(panic_msg, mynmi);
nmi_panic(regs, panic_msg);
@@ -328,6 +344,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
pretimeout = 0;
}
hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
+ kdumptimeout = min(kdumptimeout, HPWDT_MAX_TIMER);

hpwdt_dev.parent = &dev->dev;
retval = watchdog_register_device(&hpwdt_dev);
@@ -342,6 +359,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
hpwdt_dev.timeout, nowayout);
dev_info(&dev->dev, "pretimeout: %s.\n",
pretimeout ? "on" : "off");
+ dev_info(&dev->dev, "kdumptimeout: %d.\n", kdumptimeout);

if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR)
ilo5 = true;
@@ -387,6 +405,9 @@ static void hpwdt_exit(struct pci_dev *dev)
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");

+module_param(kdumptimeout, int, 0444);
+MODULE_PARM_DESC(kdumptimeout, "Timeout applied for crash kernel transition in seconds");
+
#ifdef CONFIG_HPWDT_NMI_DECODING
module_param(pretimeout, bool, 0);
MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
--
1.8.3.1

2019-06-05 22:34:03

by Jerry Hoemann

[permalink] [raw]
Subject: Re: [PATCH 0/6] watchdog/hpwdt: cleanups and kdump accommodations

On Fri, May 17, 2019 at 02:59:37PM -0600, Jerry Hoemann wrote:
> First two changes makes hpwdt more generic.
> Next two changes make hpwdt work better with kdump.
>

Hi Guenter,

Did you have feedback on this patch set?

Thanks

Jerry


>
> Jerry Hoemann (6):
> watchdog/hpwdt: Stop hpwdt on unregister.
> watchdog/hpwdt: Advertize max_hw_heartbeat_ms
> watchdog/hpwdt: Have core ping watchdog.
> watchdog/hpwdt: Add module parameter kdumptimeout.
> watchdog/hpwdt: Update documentation
> watchdog/hpwdt: Reflect changes
>
> Documentation/watchdog/hpwdt.txt | 4 +++
> drivers/watchdog/hpwdt.c | 55 ++++++++++++++++++++++++++++++----------
> 2 files changed, 45 insertions(+), 14 deletions(-)
>
> --
> 1.8.3.1

--

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

2019-06-05 22:52:54

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 0/6] watchdog/hpwdt: cleanups and kdump accommodations

On Wed, Jun 05, 2019 at 04:31:03PM -0600, Jerry Hoemann wrote:
> On Fri, May 17, 2019 at 02:59:37PM -0600, Jerry Hoemann wrote:
> > First two changes makes hpwdt more generic.
> > Next two changes make hpwdt work better with kdump.
> >
>
> Hi Guenter,
>
> Did you have feedback on this patch set?
>
I have been out on vacation, and I am still digging myself out of a big hole.

Guenter

> Thanks
>
> Jerry
>
>
> >
> > Jerry Hoemann (6):
> > watchdog/hpwdt: Stop hpwdt on unregister.
> > watchdog/hpwdt: Advertize max_hw_heartbeat_ms
> > watchdog/hpwdt: Have core ping watchdog.
> > watchdog/hpwdt: Add module parameter kdumptimeout.
> > watchdog/hpwdt: Update documentation
> > watchdog/hpwdt: Reflect changes
> >
> > Documentation/watchdog/hpwdt.txt | 4 +++
> > drivers/watchdog/hpwdt.c | 55 ++++++++++++++++++++++++++++++----------
> > 2 files changed, 45 insertions(+), 14 deletions(-)
> >
> > --
> > 1.8.3.1
>
> --
>
> -----------------------------------------------------------------------------
> Jerry Hoemann Software Engineer Hewlett Packard Enterprise
> -----------------------------------------------------------------------------

2019-06-06 22:32:24

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 1/6] watchdog/hpwdt: Stop hpwdt on unregister.

On Fri, May 17, 2019 at 02:59:38PM -0600, Jerry Hoemann wrote:
> Have the WD core stop the watchdog on unregister instead of explicitly
> calling hpwdt_stop() in hpwdt_exit().
>
> Signed-off-by: Jerry Hoemann <[email protected]>

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

> ---
> drivers/watchdog/hpwdt.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index ef30c7e..8c49f13 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -310,6 +310,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
> if (retval != 0)
> goto error_init_nmi_decoding;
>
> + watchdog_stop_on_unregister(&hpwdt_dev);
> watchdog_set_nowayout(&hpwdt_dev, nowayout);
> if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
> dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin);
> @@ -350,9 +351,6 @@ static int hpwdt_init_one(struct pci_dev *dev,
>
> static void hpwdt_exit(struct pci_dev *dev)
> {
> - if (!nowayout)
> - hpwdt_stop();
> -
> watchdog_unregister_device(&hpwdt_dev);
> hpwdt_exit_nmi_decoding();
> pci_iounmap(dev, pci_mem_addr);

2019-06-06 22:34:03

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 2/6] watchdog/hpwdt: Advertize max_hw_heartbeat_ms

On Fri, May 17, 2019 at 02:59:39PM -0600, Jerry Hoemann wrote:
> Set max_hw_heartbeat_ms instead of max_timeout so that user client can
> set timeout range in excess of what the underlying hardware supports.
>
> Signed-off-by: Jerry Hoemann <[email protected]>

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

> ---
> drivers/watchdog/hpwdt.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 8c49f13..9f7a370 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -62,9 +62,9 @@
> static int hpwdt_start(struct watchdog_device *wdd)
> {
> int control = 0x81 | (pretimeout ? 0x4 : 0);
> - int reload = SECS_TO_TICKS(wdd->timeout);
> + int reload = SECS_TO_TICKS(min(wdd->timeout, wdd->max_hw_heartbeat_ms/1000));
>
> - dev_dbg(wdd->parent, "start watchdog 0x%08x:0x%02x\n", reload, control);
> + dev_dbg(wdd->parent, "start watchdog 0x%08x:0x%08x:0x%02x\n", wdd->timeout, reload, control);
> iowrite16(reload, hpwdt_timer_reg);
> iowrite8(control, hpwdt_timer_con);
>
> @@ -91,9 +91,9 @@ static int hpwdt_stop_core(struct watchdog_device *wdd)
>
> static int hpwdt_ping(struct watchdog_device *wdd)
> {
> - int reload = SECS_TO_TICKS(wdd->timeout);
> + int reload = SECS_TO_TICKS(min(wdd->timeout, wdd->max_hw_heartbeat_ms/1000));
>
> - dev_dbg(wdd->parent, "ping watchdog 0x%08x\n", reload);
> + dev_dbg(wdd->parent, "ping watchdog 0x%08x:0x%08x\n", wdd->timeout, reload);
> iowrite16(reload, hpwdt_timer_reg);
>
> return 0;
> @@ -208,9 +208,9 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
> .info = &ident,
> .ops = &hpwdt_ops,
> .min_timeout = 1,
> - .max_timeout = HPWDT_MAX_TIMER,
> .timeout = DEFAULT_MARGIN,
> .pretimeout = PRETIMEOUT_SEC,
> + .max_hw_heartbeat_ms = HPWDT_MAX_TIMER * 1000,
> };
>
>

2019-06-06 22:35:50

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 4/6] watchdog/hpwdt: Add module parameter kdumptimeout.

On Fri, May 17, 2019 at 02:59:41PM -0600, Jerry Hoemann wrote:
> Instead of unconditionally stopping the watchdog timer after receipt of
> a pretimeout NMI, reprogram the timeout based upon module parameter
> kdumptimeout.
>
> The provides a more flexible override than the depricated allow_kdump.
>
> Signed-off-by: Jerry Hoemann <[email protected]>

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

> ---
> drivers/watchdog/hpwdt.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index aa4101c..dc65006 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -29,7 +29,8 @@
> #define HPWDT_VERSION "2.0.2"
> #define SECS_TO_TICKS(secs) ((secs) * 1000 / 128)
> #define TICKS_TO_SECS(ticks) ((ticks) * 128 / 1000)
> -#define HPWDT_MAX_TIMER TICKS_TO_SECS(65535)
> +#define HPWDT_MAX_TICKS 65535
> +#define HPWDT_MAX_TIMER TICKS_TO_SECS(HPWDT_MAX_TICKS)
> #define DEFAULT_MARGIN 30
> #define PRETIMEOUT_SEC 9
>
> @@ -37,6 +38,7 @@
> static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */
> static bool nowayout = WATCHDOG_NOWAYOUT;
> static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);
> +static int kdumptimeout = -1;
>
> static void __iomem *pci_mem_addr; /* the PCI-memory address */
> static unsigned long __iomem *hpwdt_nmistat;
> @@ -56,6 +58,7 @@
> {0}, /* terminate list */
> };
>
> +static struct watchdog_device hpwdt_dev;
> /*
> * Watchdog operations
> */
> @@ -94,12 +97,18 @@ static int hpwdt_stop_core(struct watchdog_device *wdd)
> return 0;
> }
>
> +static void hpwdt_ping_ticks(int val)
> +{
> + val = min(val, HPWDT_MAX_TICKS);
> + iowrite16(val, hpwdt_timer_reg);
> +}
> +
> static int hpwdt_ping(struct watchdog_device *wdd)
> {
> int reload = SECS_TO_TICKS(min(wdd->timeout, wdd->max_hw_heartbeat_ms/1000));
>
> dev_dbg(wdd->parent, "ping watchdog 0x%08x:0x%08x\n", wdd->timeout, reload);
> - iowrite16(reload, hpwdt_timer_reg);
> + hpwdt_ping_ticks(reload);
>
> return 0;
> }
> @@ -175,7 +184,14 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
> if (ilo5 && !pretimeout && !mynmi)
> return NMI_DONE;
>
> - hpwdt_stop();
> + if (kdumptimeout < 0)
> + hpwdt_stop();
> + else if (kdumptimeout == 0)
> + ;
> + else {
> + unsigned int val = max((unsigned int)kdumptimeout, hpwdt_dev.timeout);
> + hpwdt_ping_ticks(SECS_TO_TICKS(val));
> + }
>
> hex_byte_pack(panic_msg, mynmi);
> nmi_panic(regs, panic_msg);
> @@ -328,6 +344,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
> pretimeout = 0;
> }
> hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
> + kdumptimeout = min(kdumptimeout, HPWDT_MAX_TIMER);
>
> hpwdt_dev.parent = &dev->dev;
> retval = watchdog_register_device(&hpwdt_dev);
> @@ -342,6 +359,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
> hpwdt_dev.timeout, nowayout);
> dev_info(&dev->dev, "pretimeout: %s.\n",
> pretimeout ? "on" : "off");
> + dev_info(&dev->dev, "kdumptimeout: %d.\n", kdumptimeout);
>
> if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR)
> ilo5 = true;
> @@ -387,6 +405,9 @@ static void hpwdt_exit(struct pci_dev *dev)
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
> +module_param(kdumptimeout, int, 0444);
> +MODULE_PARM_DESC(kdumptimeout, "Timeout applied for crash kernel transition in seconds");
> +
> #ifdef CONFIG_HPWDT_NMI_DECODING
> module_param(pretimeout, bool, 0);
> MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");