Re: [PATCH v1 0/2] Watchdog Core Global Parameters
ameters_init()*, in the same file. > >>> > >>> Global parameters use > >>> = > >>> > >>> Each watchdog driver, to check if one of the global parameters is > >>> enabled, can use the corresponding in-line function declared in > >>> include/linux/watchdog.h. > >>> At the moment the following functions are ready to use: > >>> > >>> * watchdog_global_param_verbose_enabled() > >>> * watchdog_global_param_test_mode_enabled() > >>> * watchdog_global_param_start_enabled() > >>> * watchdog_global_param_nowayout_enabled() > >>> > >>> > >>> > >>> Flavio Suligoi (2): > >>> watchdog: add global watchdog kernel module parameters structure > >>> watchdog: wdat: add start_enable global parameter > >>> > >>> Documentation/watchdog/index.rst | 1 + > >>> .../watchdog-core-global-parameters.rst | 74 +++ > >>> drivers/watchdog/watchdog_core.c | 74 +++ > >>> drivers/watchdog/wdat_wdt.c | 2 + > >>> include/linux/watchdog.h | 42 +++ > >>> 5 files changed, 193 insertions(+) > >>> create mode 100644 > >>> Documentation/watchdog/watchdog-core-global-parameters.rst > >>> > > -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [PATCH] watchdog/hpwdt: Assign boolean values to a bool variable
On Wed, Jan 20, 2021 at 03:48:10PM +0800, Jiapeng Zhong wrote:
> Fix the following coccicheck warnings:
>
> ./drivers/watchdog/hpwdt.c:345:2-12: WARNING: Assignment of
> 0/1 to bool variable.
>
> ./drivers/watchdog/hpwdt.c:126:2-12: WARNING: Assignment of
> 0/1 to bool variable.
>
> Reported-by: Abaci Robot
> Signed-off-by: Jiapeng Zhong
> ---
> drivers/watchdog/hpwdt.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Jerry Hoemann
>
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index cbd1498..22ddba3 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -123,7 +123,7 @@ static int hpwdt_settimeout(struct watchdog_device *wdd,
> unsigned int val)
> if (val <= wdd->pretimeout) {
> dev_dbg(wdd->parent, "pretimeout < timeout. Setting to zero\n");
> wdd->pretimeout = 0;
> - pretimeout = 0;
> + pretimeout = false;
> if (watchdog_active(wdd))
> hpwdt_start(wdd);
> }
> @@ -336,13 +336,13 @@ static int hpwdt_init_one(struct pci_dev *dev,
> watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);
>
> if (is_kdump_kernel()) {
> - pretimeout = 0;
> + pretimeout = false;
> kdumptimeout = 0;
> }
>
> if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) {
> dev_warn(&dev->dev, "timeout <= pretimeout. Setting pretimeout
> to zero\n");
> - pretimeout = 0;
> + pretimeout = false;
> }
> hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
> kdumptimeout = min(kdumptimeout, HPWDT_MAX_TIMER);
> --
> 1.8.3.1
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [RFC PATCH] PCI, kdump: Clear bus master bit upon shutdown in kdump kernel
gt; > > Kairui said it's a device which driver is not loaded in kdump kernel > > > because it's not needed by kdump. We try to only load kernel modules > > > which are needed, e.g one device is the dump target, its driver has to > > > be loaded in. In this case, the device is more like a out of control > > > device to kdump kernel. > > > > Hi Bao, Deepa, sorry for this very late response. The test machine was > > not available for sometime, and I restarted to work on this problem. > > > > For the workaround mention by Deepa (by remote the is_kdump_kernel() > > check), it didn't work, the machine still hangs upon shutdown. > > The devices that were left in an unknown state and sending interrupt > > could be a problem, but it's irrelevant to this hanging problem. > > > > I think I didn't make one thing clear, The PCI UR error never arrives > > in kernel, it's the iLo BMC on that HPE machine caught the error, and > > send kernel an NMI. kernel is panicked by NMI, I'm still trying to > > figure out why the NMI hanged kernel, even with panic=-1, > > panic_on_io_nmi, panic_on_unknown_nmi all set. But if we can avoid the > > NMI by shutdown the devices in right order, that's also a solution. > > I'm not sure how much sympathy to have for this situation. A PCIe UR > is fatal for the transaction and maybe even the device, but from the > overall system point of view, it *should* be a recoverable error and > we shouldn't panic. > > Errors like that should be reported via the normal AER or ACPI/APEI > mechanisms. It sounds like in this case, the platform has decided > these aren't enough and it is trying to force a reboot? If this is > "special" platform behavior, I'm not sure how much we need to cater > for it. > Are these AER errors the type processed by the GHES code? I'll note that RedHat runs their crash kernel with: hest_disable. So, the ghes code is disabled in the crash kernel. -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp
On Thu, Aug 08, 2019 at 02:33:24PM -0600, Jerry Hoemann wrote: > On Fri, Aug 02, 2019 at 06:33:28PM +0200, Peter Zijlstra wrote: > > On Fri, Aug 02, 2019 at 06:20:15PM +0200, Peter Zijlstra wrote: > > > On Fri, Aug 02, 2019 at 02:33:41PM +, Lendacky, Thomas wrote: > > > > > > Talking to the hardware folks, they say setting CR8 is a serializing > > > > instruction and has to communicate out to the APIC, so it's better to > > > > use CLI/STI. > > > > > > Bah; the Intel SDM states: "MOV CR* instructions, except for MOV CR8, > > > are serializing instructions", which had given me a little hope. > > > > > > At the same time, all these chips still have the APIC TPR field too, so > > > much like how the TSC DEADLINE MSR is a hidden APIC write, so too is CR8 > > > I suppose :-( > > > > > > I'll still finish the patches I started, just to see what it would look > > > like. > > > > Another 'fun' issue I ran into while doing these patches; STI has a 1 > > instruction shadow, which we rely on, MOV CR8 does not. So things like: > > > > native_safe_halt: > > sti > > hlt > > > > turn into: > > > > native_safe_halt: > > cli > > movl $0, %rax > > movq %rax, %cr8 > > sti > > hlt > > > > Hi Peter, > > What is our the next step here? > > Are you still looking to make this change? > > Do we want to pick up Tom Lendacky's patch on an interim basis while > you're working on the bigger change? (I can say we tested Tom's > patch and it does address the issue we were seeing.) > > Thanks > > Jerry > Hi Peter, Have you gotten a chance to look into this more? Thanks Jerry -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [PATCH] perf/x86/amd: Change NMI latency mitigation to use a timestamp
On Fri, Aug 02, 2019 at 06:33:28PM +0200, Peter Zijlstra wrote: > On Fri, Aug 02, 2019 at 06:20:15PM +0200, Peter Zijlstra wrote: > > On Fri, Aug 02, 2019 at 02:33:41PM +, Lendacky, Thomas wrote: > > > > Talking to the hardware folks, they say setting CR8 is a serializing > > > instruction and has to communicate out to the APIC, so it's better to > > > use CLI/STI. > > > > Bah; the Intel SDM states: "MOV CR* instructions, except for MOV CR8, > > are serializing instructions", which had given me a little hope. > > > > At the same time, all these chips still have the APIC TPR field too, so > > much like how the TSC DEADLINE MSR is a hidden APIC write, so too is CR8 > > I suppose :-( > > > > I'll still finish the patches I started, just to see what it would look > > like. > > Another 'fun' issue I ran into while doing these patches; STI has a 1 > instruction shadow, which we rely on, MOV CR8 does not. So things like: > > native_safe_halt: > sti > hlt > > turn into: > > native_safe_halt: > cli > movl $0, %rax > movq %rax, %cr8 > sti > hlt > Hi Peter, What is our the next step here? Are you still looking to make this change? Do we want to pick up Tom Lendacky's patch on an interim basis while you're working on the bigger change? (I can say we tested Tom's patch and it does address the issue we were seeing.) Thanks Jerry -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [PATCH] watchdog: convert remaining drivers to use SPDX license identifier
On Thu, Jun 20, 2019 at 09:28:46AM -0700, Guenter Roeck wrote: > This gets rid of the unnecessary license boilerplate, and avoids > having to deal with individual patches one by one. > > No functional changes. > > Signed-off-by: Guenter Roeck > --- > Note: Several drivers include a paragraph such as > > "Neither nor admit liability nor > provide warranty for any of this software. This material is > provided "AS-IS" and at no charge." > > Presumably this is covered by the GPL license. However, since I am not > an attorney, I am not sure, and I opted for leaving such paragraphs in > place. > For hpwdt.c changes: Reviewed-by: Jerry Hoemann > drivers/watchdog/acquirewdt.c | 6 +- > drivers/watchdog/advantechwdt.c| 6 +- > drivers/watchdog/ath79_wdt.c | 6 +- > drivers/watchdog/davinci_wdt.c | 6 ++ > drivers/watchdog/ebc-c384_wdt.c| 9 - > drivers/watchdog/eurotechwdt.c | 6 +- > drivers/watchdog/ftwdt010_wdt.c| 5 + > drivers/watchdog/hpwdt.c | 6 +- > drivers/watchdog/iTCO_vendor_support.c | 7 +-- > drivers/watchdog/iTCO_wdt.c| 6 +- > drivers/watchdog/ib700wdt.c| 6 +- > drivers/watchdog/ie6xx_wdt.c | 17 + > drivers/watchdog/imgpdc_wdt.c | 5 + > drivers/watchdog/intel_scu_watchdog.c | 19 +-- > drivers/watchdog/intel_scu_watchdog.h | 17 + > drivers/watchdog/iop_wdt.c | 14 +- > drivers/watchdog/kempld_wdt.c | 10 +- > drivers/watchdog/ks8695_wdt.c | 5 + > drivers/watchdog/lantiq_wdt.c | 5 + > drivers/watchdog/lpc18xx_wdt.c | 5 + > drivers/watchdog/max77620_wdt.c| 5 + > drivers/watchdog/mt7621_wdt.c | 5 + > drivers/watchdog/mv64x60_wdt.c | 6 ++ > drivers/watchdog/nuc900_wdt.c | 6 +- > drivers/watchdog/nv_tco.h | 6 +- > drivers/watchdog/octeon-wdt-main.c | 11 +-- > drivers/watchdog/omap_wdt.c| 6 ++ > drivers/watchdog/omap_wdt.h| 21 + > drivers/watchdog/pc87413_wdt.c | 6 +- > drivers/watchdog/pcwd_pci.c| 6 +- > drivers/watchdog/pcwd_usb.c| 6 +- > drivers/watchdog/pnx4008_wdt.c | 5 + > drivers/watchdog/qcom-wdt.c| 14 ++ > drivers/watchdog/retu_wdt.c| 10 +- > drivers/watchdog/rn5t618_wdt.c | 8 +--- > drivers/watchdog/rt2880_wdt.c | 5 + > drivers/watchdog/sa1100_wdt.c | 6 +- > drivers/watchdog/sbc7240_wdt.c | 11 +-- > drivers/watchdog/sbc8360.c | 6 +- > drivers/watchdog/sbsa_gwdt.c | 10 +- > drivers/watchdog/sch311x_wdt.c | 6 +- > drivers/watchdog/softdog.c | 6 +- > drivers/watchdog/txx9wdt.c | 5 + > drivers/watchdog/w83627hf_wdt.c| 6 +- > drivers/watchdog/wafer5823wdt.c| 6 +- > drivers/watchdog/watchdog_core.c | 6 +- > drivers/watchdog/watchdog_core.h | 6 +- > drivers/watchdog/watchdog_dev.c| 6 +- > drivers/watchdog/wd501p.h | 6 +- > drivers/watchdog/wdt.c | 6 +- > drivers/watchdog/wdt_pci.c | 6 +- > 51 files changed, 54 insertions(+), 336 deletions(-) > > diff --git a/drivers/watchdog/acquirewdt.c b/drivers/watchdog/acquirewdt.c > index 957d1255d4ca..848db958411e 100644 > --- a/drivers/watchdog/acquirewdt.c > +++ b/drivers/watchdog/acquirewdt.c > @@ -1,3 +1,4 @@ > +// SPDX-License-Identifier: GPL-2.0+ > /* > * Acquire Single Board Computer Watchdog Timer driver > * > @@ -6,11 +7,6 @@ > * (c) Copyright 1996 Alan Cox , > * All Rights Reserved. > * > - * This program is free software; you can redistribute it and/or > - * modify it under the terms of the GNU General Public License > - * as published by the Free Software Foundation; either version > - * 2 of the License, or (at your option) any later version. > - * > * Neither Alan Cox nor CymruNet Ltd. admit liability nor provide > * warranty for any of this software. This material is provided > * "AS-IS" and at no charge. > diff --git a/drivers/watchdog/advantechwdt.c b/drivers/watchdog/advantechwdt.c > index 2766af292a71..0d02bb275b3d 100644 > --- a/drivers/watchdog/advantechwdt.c > +++ b/drivers/watchdog/advantechwdt.c > @@
[PATCH 0/1] docs: watchdog: make htmldocs failed.
Guenter, While making htmldocs from linux-staging.git watchdog-next, the build failed due what I think is a missing blank line after a header separator in: Documentation/watchdog/watchdog-parameters.rst I'm a newbie w.r.t. to the .rst file formats, but adding the blank lines got the "make htmldocs" to complete. Jerry Hoemann (1): docs: watchdog: Fix build error. Documentation/watchdog/watchdog-parameters.rst | 2 ++ 1 file changed, 2 insertions(+) -- 2.20.1
[PATCH 1/1] docs: watchdog: Fix build error.
make htmldocs fails due to missing blank line following header. Signed-off-by: Jerry Hoemann --- Documentation/watchdog/watchdog-parameters.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst index 01023382ea73..a3985cc5aeda 100644 --- a/Documentation/watchdog/watchdog-parameters.rst +++ b/Documentation/watchdog/watchdog-parameters.rst @@ -12,6 +12,7 @@ providing kernel parameters for builtin drivers versus loadable modules. - + watchdog core: open_timeout: Maximum time, in seconds, for which the watchdog framework will take @@ -22,6 +23,7 @@ watchdog core: fallback logic in the bootloader to try something else. - + acquirewdt: wdt_stop: Acquire WDT 'stop' io port (default 0x43) -- 2.20.1
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 -
[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
---
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
[PATCH 6/6] watchdog/hpwdt: Reflect changes
Bump driver number to reflect recent changes. Signed-off-by: Jerry Hoemann --- 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 dc65006..9e02f88 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -26,7 +26,7 @@ #include #include -#define HPWDT_VERSION "2.0.2" +#define HPWDT_VERSION "2.0.3" #define SECS_TO_TICKS(secs)((secs) * 1000 / 128) #define TICKS_TO_SECS(ticks) ((ticks) * 128 / 1000) #define HPWDT_MAX_TICKS65535 -- 1.8.3.1
[PATCH 5/6] watchdog/hpwdt: Update documentation
Update documentation to explain new module parameter kdumptimeout. Signed-off-by: Jerry Hoemann --- Documentation/watchdog/hpwdt.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/watchdog/hpwdt.txt b/Documentation/watchdog/hpwdt.txt index 55df692..35da141 100644 --- a/Documentation/watchdog/hpwdt.txt +++ b/Documentation/watchdog/hpwdt.txt @@ -33,6 +33,10 @@ Last reviewed: 08/20/2018 Default value is set when compiling the kernel. If it is set to "Y", then there is no way of disabling the watchdog once it has been started. + kdumptimeout Minimum timeout in seconds to apply upon receipt of an NMI + before calling panic. (-1) disables the watchdog. When value + is > 0, the timer is reprogrammed with the greater of + value or current timeout value. NOTE: More information about watchdog drivers in general, including the ioctl interface to /dev/watchdog can be found in -- 1.8.3.1
[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
[PATCH 3/6] watchdog/hpwdt: Have core ping watchdog.
Instead of stopping the hw timer during probe, have the core update
the timer if the timer is already running.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 9f7a370..aa4101c 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -59,6 +59,11 @@
/*
* Watchdog operations
*/
+static int hpwdt_hw_is_running(void)
+{
+ return ioread8(hpwdt_timer_con) & 0x01;
+}
+
static int hpwdt_start(struct watchdog_device *wdd)
{
int control = 0x81 | (pretimeout ? 0x4 : 0);
@@ -302,8 +307,11 @@ static int hpwdt_init_one(struct pci_dev *dev,
hpwdt_timer_reg = pci_mem_addr + 0x70;
hpwdt_timer_con = pci_mem_addr + 0x72;
- /* Make sure that timer is disabled until /dev/watchdog is opened */
- hpwdt_stop();
+ /* Have the core update running timer until user space is ready */
+ if (hpwdt_hw_is_running()) {
+ dev_info(&dev->dev, "timer is running\n");
+ set_bit(WDOG_HW_RUNNING, &hpwdt_dev.status);
+ }
/* Initialize NMI Decoding functionality */
retval = hpwdt_init_nmi_decoding(dev);
--
1.8.3.1
[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
---
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
[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
---
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_TIMERTICKS_TO_SECS(65535)
+#define HPWDT_MAX_TICKS65535
+#define HPWDT_MAX_TIMERTICKS_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
Re: [PATCH 4/4] misc: hpilo: Update driver version
On Fri, Feb 22, 2019 at 07:49:28AM +0100, Greg KH wrote: > On Thu, Feb 21, 2019 at 09:11:11PM -0700, Jerry Hoemann wrote: > > > Our primary means of supporting Linux to our customers is via our > > distro partners. While we prefer to use in distro drivers, HPE does > > from time to time deliver driver updates via the "Service Pack for > > Proliants" -- The SPP. > > That's fine, but again, does not matter to the in-kernel driver at all. Your point? No one claimed that changing the version number of the module changes its functionality. We're changing the driver version number to reflect that the driver's functionality changed. We do this to help determine the version running on a system in the event we have problems. It's a support issue. > I understand that in your viewpoint, your driver's version means > something. But in reality, it's only the kernel's version that means > something because your driver is just part of the overall kernel, it > does not stand alone. I never claimed a driver stood alone. jeezz. When you say kernel "version", are you trying to say that the version string printed by the kernel determines the source of the drivers? (I ask, because I have heard other maintainers make this claim.) The kernel version string only reliably determines the base kernel build. Modules can be unloaded and replaced by totally new versions drastically different from the version that existed at the time of the base kernel build. The delivery of drivers updates independent of base kernel was old practice when I started Unix development 30 years ago. It was not unique to HPE then or now. I don't see it stopping. So while Linux delivers drivers built to a baseline kernel build, we cannot rely that the bits being used on a system still reflect that initial install. We can't just assume a driver version. And without knowing the driver version, it makes support more difficult. If you're trying to be profound, the "version" of the OS running is more than just the base kernel. That is only the beginning. We then have to consider the modules loaded and the order that they're loaded. This sequence is unbounded as modules can be repeatedly loaded and unloaded. When you know that, then you know the "version" of the kernel running. But that isn't what the kernel version string gives you. So why have it/print it? By your reasoning it's meaningless. It should be tossed. We have it because it gives us info, but it is only a start. - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [PATCH 2/4] misc: hpilo: Exclude unsupported device via blacklist
On Thu, Feb 21, 2019 at 09:33:55AM +0100, Greg KH wrote:
> On Thu, Feb 21, 2019 at 04:04:40PM +0800, Matt Hsiao wrote:
> > +static const struct pci_device_id ilo_blacklist[] = {
> > + /* auxiliary iLO */
> > + {PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3307, PCI_VENDOR_ID_HP, 0x1979)},
> > + {}
> > +};
> >
...
>
> And why do some devices need to be blacklisted, shouldn't there only be
> a whitelist in the first place? Do you need to tighten up your original
> device ids?
Hi Greg,
I related the underlying reason for the black listing on another message
of this thread. I can fill you in on why we've taken this approach to
white/black listing.
HPE hardware/firmware teams will put out minor updates to the iLO using
the same device info except for the subsystem device id.
The approach we've taken in both the hpilo and hpwdt drivers is
to claim based upon {Vendor, PC DevID, SubVendor}.
This allows old software to work on new hardware without patching.
As our primary way to support our customers is via distros, this patching
when it does happen requires us to not just submit a patch upstream, but
to then to have the patches back ported to multiple releases of multiple
distros. This process takes many many months.
So far, the approach we've taken has worked fairly well as this is only
the second time in 10+ years that we've needed to blacklist an instance.
Hope this helps.
Jerry
--
---------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [PATCH 4/4] misc: hpilo: Update driver version
On Thu, Feb 21, 2019 at 09:32:56AM +0100, Greg KH wrote:
...
> >
> > -MODULE_VERSION("1.5.0");
> > +MODULE_VERSION("1.5.1");
>
> This line means nothing, it should just be removed entirely. The
> "version" of the driver is the kernel version itself.
Hi Greg,
This doesn't hold when we do driver updates.
Our primary means of supporting Linux to our customers is via our
distro partners. While we prefer to use in distro drivers, HPE does
from time to time deliver driver updates via the "Service Pack for
Proliants" -- The SPP.
An SPP driver update can supply an updated module without modifying
the underlying base kernel version. Because of this, the underlying
kernel version number doesn't always imply the version of a module
being used.
Even when we use only in kernel drivers, we also have the case where
our distro partners will back port newer driver versions to an older
distro release. This gives the situation where an older kernel
version can have a newer module than a newer kernel version for
that distro.
We have found that having module version bumped when modifying the
driver helps us identify the version module actually being used.
Take care,
Jerry
>
> Want me to drop it?
No. please keep it. :)
--
-----
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [PATCH 3/4] misc: hpilo: Do not claim unsupported hardware
On Thu, Feb 21, 2019 at 09:35:15AM +0100, Greg KH wrote: > On Thu, Feb 21, 2019 at 04:04:41PM +0800, Matt Hsiao wrote: > > Do not claim when SSID 0x0289 as the iLO features > > are not enabled/validated by the firmware. > > Can you put more information here, like _what_ hardware is not being > supported anymore? As it is, this has nothing to do with "validation by > the firmware", you are just deciding to not support a device that > previously was supported by this driver. As such you should provide > more information why you are taking away functionality. Hi Greg, The SSID 0x0289 correspond to the recent CL2600/CL2800 servers. These servers leveraged Proliant hardware but are targeted to a different market segment with different requirements. They also come with a different firmware base. Based upon the targeted market needs, these server de-featured certain aspects of Proliants with this being one. As a result, the linux hpilo driver still claims the hardware but is not functional. We decided to blacklist the driver to reduce confusion to customers. Matt will update the documentation in the second version of the patch set. Hope this helps. Take care, Jerry -- --------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
[PATCH] watchdog/hpwdt: Update Kconfig documentation
Update documentation relating to HPWDT_NMI_DECODING to reflect its current usage. Signed-off-by: Jerry Hoemann --- drivers/watchdog/Kconfig | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 57f017d..846dd07 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -1145,7 +1145,7 @@ config HP_WATCHDOG select WATCHDOG_CORE depends on X86 && PCI help - A software monitoring watchdog and NMI sourcing driver. This driver + A software monitoring watchdog and NMI handling driver. This driver will detect lockups and provide a stack trace. This is a driver that will only load on an HP ProLiant system with a minimum of iLO2 support. To compile this driver as a module, choose M here: the module will be @@ -1163,12 +1163,13 @@ config KEMPLD_WDT called kempld_wdt. config HPWDT_NMI_DECODING - bool "NMI decoding support for the HP ProLiant iLO2+ Hardware Watchdog Timer" + bool "NMI support for the HP ProLiant iLO2+ Hardware Watchdog Timer" depends on HP_WATCHDOG default y help - When an NMI occurs this feature will make the necessary BIOS calls to - log the cause of the NMI. + Enables the NMI handler for the watchdog pretimeout NMI and the iLO + "Generate NMI to System" virtual button. When an NMI is claimed + by the driver, panic is called. config SC1200_WDT tristate "National Semiconductor PC87307/PC97307 (ala SC1200) Watchdog" -- 1.8.3.1
Re: [RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI
On Sat, Feb 02, 2019 at 09:55:29AM +0500, Ivan Mironov wrote: > On Tue, 2019-01-15 at 19:27 -0700, Jerry Hoemann wrote: > > On Mon, Jan 14, 2019 at 07:36:14AM +0500, Ivan Mironov wrote: > > Somehow I missed the whole pretimout thing when reading about the > watchdog API. Thanks for clarification, now code makes much more sense > =). > > Still, I do not really understand the point of enabling of kdump > support in hpwdt driver by default while kdump is not enabled by > default. Kdump is enabled by default by our Distro partners. HPE works with distro partners to deliver a validated system which we support. The ability to generate crash dumps is one of the means we use to support our customers. Even if kdump isn't configured, panic will at least print stack trace to indicate system activity. > > Also, existing code may call hpwdt_stop() (and thus break watchdog) > even if pretimout is disabled. > > Also, "panic=N" option is not providing a way to *not* panic on NMI > unrelated with iLO. This could be circumvented by blacklisting the > hpwdt module entirely, but normal watchdog functionality would be lost > then. panic=N provides for reset upon receipt of NMI if user wants timeout to reset system but not a crash dump. The panic is for error containment. On the legacy systems within the context of hpwdt_pretimeout we cannot determine if the error is recoverable or not. So, we have little choice but to panic. > > It is possible to rebuild kernel without HPWDT_NMI_DECODING (which is > enabled in Fedora, for example). But it is nearly impossible to come to > this solution without examining the source code, because description of > this option does not mention that it is really about pretimout support > and panics and not about something else... The name is not the best given its current use, but I'm not sure a name change would be allowed. However, I will send a patch to update the documentation in Kconfig. -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
[PATCH] watchdog: Update sysfs documentation.
Document the sysfs attributes: pretimeout pretimeout_available_governors pretimeout_governor Signed-off-by: Jerry Hoemann --- Documentation/ABI/testing/sysfs-class-watchdog | 23 +++ 1 file changed, 23 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-class-watchdog b/Documentation/ABI/testing/sysfs-class-watchdog index 736046b..6317ade 100644 --- a/Documentation/ABI/testing/sysfs-class-watchdog +++ b/Documentation/ABI/testing/sysfs-class-watchdog @@ -49,3 +49,26 @@ Contact: Wim Van Sebroeck Description: It is a read only file. It is read to know about current value of timeout programmed. + +What: /sys/class/watchdog/watchdogn/pretimeout +Date: December 2016 +Contact: Wim Van Sebroeck +Description: + It is a read only file. It specifies the time in seconds before + timeout when the pretimeout interrupt is delivered. Pretimeout + is an optional feature. + +What: /sys/class/watchdog/watchdogn/pretimeout_avaialable_governors +Date: February 2017 +Contact: Wim Van Sebroeck +Description: + It is a read only file. It shows the pretimeout governors + available for this watchdog. + +What: /sys/class/watchdog/watchdogn/pretimeout_governor +Date: February 2017 +Contact: Wim Van Sebroeck +Description: + It is a read/write file. When read, the currently assigned + pretimeout governor is returned. When written, it sets + the pretimeout governor. -- 1.8.3.1
Re: [PATCHv7] x86/kdump: bugfix, make the behavior of crashkernel=X consistent with kaslr
On Fri, Feb 01, 2019 at 12:47:40AM +0100, Borislav Petkov wrote: > On Thu, Jan 31, 2019 at 03:27:32PM -0700, Jerry Hoemann wrote: > > So even if a system administrator is diligent and tests > > that a chosen kdump configuration works, that configuration > > might not work on some random reboot 7 months in the future. > > Jerry, did you read the rest of the thread where I'm *actually* > suggesting to make the allocation code more robust against such > failures? Boris, I may have misunderstood your earlier comment: So we don't really need this - we simply need to tell people to use high if it fails with KASLR, AFAICT To imply an iterative approach to crashkernel size discovery. Whereas you may simply have ment: Always use ,high as the old way is broken. > Now let's look at the code: > > The "high" allocation does: > > crash_base = memblock_find_in_range(CRASH_ALIGN, > high ? CRASH_ADDR_HIGH_MAX > : CRASH_ADDR_LOW_MAX, > crash_size, CRASH_ALIGN); > > where high=true and CRASH_ADDR_HIGH_MAX on 64-bit is MAXMEM: > > # define CRASH_ADDR_HIGH_MAXMAXMEM > > The second fallback in the suggested patch does the same: > > + /* > +* crashkernel=X reserve below 4G fails? Try MAXMEM > +*/ > + if (!high && !crash_base) > + crash_base = memblock_find_in_range(CRASH_ALIGN, > + CRASH_ADDR_HIGH_MAX, > + crash_size, CRASH_ALIGN); > > and yet I get back that falling back to "high" if the first allocation > doesn't succeed is not something we should do by default because of > reasons. But this patch *practically* *does* it. Is your objection only to the second fallback of allocating memory above >= 4GB? Or are you objecting to allocating from (896 .. 4GB) as well? Falling back to allocating < 4GB probably satisfes most of the cases where the original allocation fails. thanks -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [PATCHv7] x86/kdump: bugfix, make the behavior of crashkernel=X consistent with kaslr
On Thu, Jan 31, 2019 at 11:57:32AM +0100, Borislav Petkov wrote: > On Thu, Jan 31, 2019 at 03:59:07PM +0800, Dave Young wrote: > > As Pingfan/me mentioned in another reply, there are two reasons: > > 1. old kexec-tools can not load kernel to high memory > > 2. ,high will not work on some systems without some amounts of low > > memory so it nees reserve extra low memory, it is bad for people who do > > not want it. > > Let's see: we don't enable high by default because of old kexec-tools > and some systems which do some funky reservations. > > But this patch kinda enables high by trying a couple more regions. > > So we don't really need this - we simply need to tell people to use high > if it fails with KASLR, AFAICT. Borris, The testing I've done shows that the allocation failure caused by KASLR is intermittent. On one SUT that I've seen this issue on, the crash kernel allocation fails less than 10% of the time. So even if a system administrator is diligent and tests that a chosen kdump configuration works, that configuration might not work on some random reboot 7 months in the future. Unfortunately, that will be the time that the system crashes and we won't be able to collect the crash dump due to the crashkernel allocation failure. Jerry -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
On Tue, Jan 29, 2019 at 09:01:03AM +0200, Matti Vaittinen wrote:
> On Mon, Jan 28, 2019 at 01:26:56PM -0700, Jerry Hoemann wrote:
> > On Sat, Jan 26, 2019 at 08:30:24AM -0800, Guenter Roeck wrote:
> > > On 1/25/19 3:05 AM, Matti Vaittinen wrote:
> > > > +static int bd70528_set_wake(struct bd70528 *bd70528,
> > > > + int enable, int *old_state)
> > > > +{
> > > > + int ret;
> > > > + unsigned int ctrl_reg;
> > > > +
> > > > + ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WAKE_EN,
> > > > &ctrl_reg);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + if (old_state) {
> > > > + if (ctrl_reg & BD70528_MASK_WAKE_EN)
> > > > + *old_state |= BD70528_WAKE_STATE_BIT;
> > > > + else
> > > > + *old_state &= ~BD70528_WAKE_STATE_BIT;
> > > > +
> > > > + if ((!enable) == (!(*old_state &
> > > > BD70528_WAKE_STATE_BIT)))
> > > > + return 0;
> > >
> > > I think
> > > if (enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
> > > would be much better readable. Even if not, there are way too many ()
> > > in the above conditional.
> > >
> >
> > The substitution is not equivalent to original. I think you mean:
> >
> > if (!!enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
>
> Thanks Jerry! Good catch! I originally wanted that all non-zero values
> of 'enable' would be 'true'. So maybe I just use the original approach
> but get rid of extra parenthesis which were pointed out by Guenter.
>
> if (!enable == !(*old_state & BD70528_WAKE_STATE_BIT))
> should do it just fine, right?
>
The use of "!!" to turn an int into one of two Boolean values (0 | 1)
is used extensively in Linux and as such might make the intent of
the code a bit clearer which I take as checking to see the states
match.
But, I will defer to you and Guenter on the question.
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [RFC PATCH v2 08/10] rtc: bd70528: Initial support for ROHM bd70528 RTC
On Sat, Jan 26, 2019 at 08:30:24AM -0800, Guenter Roeck wrote:
> On 1/25/19 3:05 AM, Matti Vaittinen wrote:
> > +static int bd70528_set_wake(struct bd70528 *bd70528,
> > + int enable, int *old_state)
> > +{
> > + int ret;
> > + unsigned int ctrl_reg;
> > +
> > + ret = regmap_read(bd70528->chip.regmap, BD70528_REG_WAKE_EN, &ctrl_reg);
> > + if (ret)
> > + return ret;
> > +
> > + if (old_state) {
> > + if (ctrl_reg & BD70528_MASK_WAKE_EN)
> > + *old_state |= BD70528_WAKE_STATE_BIT;
> > + else
> > + *old_state &= ~BD70528_WAKE_STATE_BIT;
> > +
> > + if ((!enable) == (!(*old_state & BD70528_WAKE_STATE_BIT)))
> > + return 0;
>
> I think
> if (enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
> would be much better readable. Even if not, there are way too many ()
> in the above conditional.
>
The substitution is not equivalent to original. I think you mean:
if (!!enable == !!(*old_state & BD70528_WAKE_STATE_BIT))
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [PATCHv7] x86/kdump: bugfix, make the behavior of crashkernel=X consistent with kaslr
On Tue, Jan 15, 2019 at 04:07:03PM +0800, Pingfan Liu wrote:
> People reported a bug on a high end server with many pcie devices, where
> kernel bootup with crashkernel=384M, and kaslr is enabled. Even
> though we still see much memory under 896 MB, the finding still failed
> intermittently. Because currently we can only find region under 896 MB,
> if without ',high' specified. Then KASLR breaks 896 MB into several parts
> randomly, and crashkernel reservation need be aligned to 128 MB, that's
> why failure is found. It raises confusion to the end user that sometimes
> crashkernel=X works while sometimes fails.
> If want to make it succeed, customer can change kernel option to
> "crashkernel=384M,high". Just this give "crashkernel=xx@yy" a very
> limited space to behave even though its grammar looks more generic.
> And we can't answer questions raised from customer that confidently:
> 1) why it doesn't succeed to reserve 896 MB;
> 2) what's wrong with memory region under 4G;
> 3) why I have to add ',high', I only require 384 MB, not 3840 MB.
> This patch tries to get memory region from 896 MB firstly, then [896MB,4G],
> finally above 4G.
While allocating crashkernel from below 4G seems fine, won't we have
problems if the crash kernel gets allocated above 4G because of the SWIOTLB?
thanks
> Dave Young sent the original post, and I just re-post it with commit log
> improvement as his requirement.
> http://lists.infradead.org/pipermail/kexec/2017-October/019571.html
> There was an old discussion below (previously posted by Chao Wang):
> https://lkml.org/lkml/2013/10/15/601
>
> Signed-off-by: Pingfan Liu
> Cc: Dave Young
> Cc: Baoquan He
> Cc: Andrew Morton
> Cc: Mike Rapoport
> Cc: ying...@kernel.org,
> Cc: vgo...@redhat.com
> Cc: Randy Dunlap
> ---
> v6 -> v7: fix spelling mistake pointed out by Randy
> arch/x86/kernel/setup.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 3d872a5..fa62c81 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -551,6 +551,22 @@ static void __init reserve_crashkernel(void)
> high ? CRASH_ADDR_HIGH_MAX
>: CRASH_ADDR_LOW_MAX,
> crash_size, CRASH_ALIGN);
> +#ifdef CONFIG_X86_64
> + /*
> + * crashkernel=X reserve below 896M fails? Try below 4G
> + */
> + if (!high && !crash_base)
> + crash_base = memblock_find_in_range(CRASH_ALIGN,
> + (1ULL << 32),
> + crash_size, CRASH_ALIGN);
> + /*
> + * crashkernel=X reserve below 4G fails? Try MAXMEM
> + */
> + if (!high && !crash_base)
> + crash_base = memblock_find_in_range(CRASH_ALIGN,
> + CRASH_ADDR_HIGH_MAX,
> + crash_size, CRASH_ALIGN);
> +#endif
> if (!crash_base) {
> pr_info("crashkernel reservation failed - No suitable
> area found.\n");
> return;
> --
> 2.7.4
>
>
> ___
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [PATCHv2] watchdog: qcom: Add suspend/resume support
On Thu, Jan 17, 2019 at 11:09:31AM -0800, Guenter Roeck wrote:
> On Thu, Jan 17, 2019 at 10:37 AM Stephen Boyd wrote:
> >
> > Quoting Sai Prakash Ranjan (2019-01-17 07:19:42)
> > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > > index 780971318810..5dfd604477a4 100644
> > > --- a/drivers/watchdog/qcom-wdt.c
> > > +++ b/drivers/watchdog/qcom-wdt.c
> > > @@ -245,6 +245,28 @@ static int qcom_wdt_remove(struct platform_device
> > > *pdev)
> > > return 0;
> > > }
> > >
> > > +static int __maybe_unused qcom_wdt_suspend(struct device *dev)
> > > +{
> > > + struct qcom_wdt *wdt = dev_get_drvdata(dev);
> > > +
> > > + if (watchdog_active(&wdt->wdd))
> > > + qcom_wdt_stop(&wdt->wdd);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int __maybe_unused qcom_wdt_resume(struct device *dev)
> > > +{
> > > + struct qcom_wdt *wdt = dev_get_drvdata(dev);
> > > +
> > > + if (watchdog_active(&wdt->wdd))
> > > + qcom_wdt_start(&wdt->wdd);
> > > +
> > > + return 0;
> > > +}
> >
> > This looks fairly generic. For example, the Mediatek driver also stops
> > and starts (but also pings after starting). Grepping for 'active' in
> > drivers/watchdog/ finds more examples. Could there be some functions in
> > watchdog core that do the common things like watchdog_stop() and
> > watchdog_start() and watchdog_start_and_ping()? Or maybe a bit can be
> > set during registration so that the 'struct class watchdog_class' can
> > get PM ops to stop and start on suspend/resume of the watchdog character
> > device?
> >
> > Nothing is wrong with the patch, I'm just bemoaning the amount of code
> > duplication here.
> >
>
> Patch(es) to add the functionality to the watchdog core are welcome;
> it should be possible to move the functionality into the core (maybe
> to be enabled with a new watchdog API call). Doing it using the class
> device sounds like an excellent idea. This should be straightforward
> to implement, though the question of "should we ping on resume or not"
> might be an endless source for bike shedding fun.
>
> Guenter
It could be controlled by a currently unused bit in the
watchdog_info.options. Then people can agree to disagree.
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [RFC PATCH 1/4] watchdog: hpwdt: Don't disable watchdog on NMI
On Mon, Jan 14, 2019 at 07:36:14AM +0500, Ivan Mironov wrote: > Existing code disables watchdog on NMI right before completely hanging > the system. > > There are two problems here: > > * First, watchdog is expected to reset the system in a case of such >failure, no matter what. Documentation/watchdog/watchdog-api.txt explicitly allows for pretimeout NMI and generation of kernel crash dumps. By removing hpwdt_stop the system will likely fail to crash dump as there is only 9 seconds between receipt of a NMI and the iLO resetting the system. Unfortunately, kdump is not without issues and can also be difficult to properly configure either of which can result in failure to dump and reset. Customers who value availability over kdump collection, the pretimeout NMI can be disabled and hardware will not issue the pretimeout NMI and will only do reset. A middle ground for those who want tombstones but not kdump, would be to leave the pretimeout NMI enabled and add "panic=N" to the Linux command line. That way after the panic, the tombstone is printed and the system resets after N seconds. > * Second, this code has no effect if there are more than one watchdog. That is correct. Hpwdt will not turn off any other WDT. I don't see a current method of notifying other watchdogs that a given watchdog is going to take the system down. The closest I hook see is watchdog_notify_pretimeout, but I don't see that notifying other WDT. Its not clear to me that it should. (e.g. the second WDT could be of longer duration and protect against kdump hanging. This would need to be thought through.) > > Signed-off-by: Ivan Mironov > --- > drivers/watchdog/hpwdt.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > index ef30c7e9728d..2467e6bc25c2 100644 > --- a/drivers/watchdog/hpwdt.c > +++ b/drivers/watchdog/hpwdt.c > @@ -170,8 +170,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct > pt_regs *regs) > if (ilo5 && !pretimeout && !mynmi) > return NMI_DONE; > > - hpwdt_stop(); > - > hex_byte_pack(panic_msg, mynmi); > nmi_panic(regs, panic_msg); > > -- > 2.20.1 -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [RFC PATCH 0/4] watchdog: hpwdt: Fix NMI-related behaviour when CONFIG_HPWDT_NMI_DECODING is enabled
On Mon, Jan 14, 2019 at 07:36:13AM +0500, Ivan Mironov wrote: > Hi, > > I found out that hpwdt alters NMI behaviour unexpectedly if compiled > with enabled CONFIG_HPWDT_NMI_DECODING: > > * System starts to panic on any NMI with misleading message. hpwdt doesn't start to panic on any NMI. It starts to panic on: 1) NMI_SERR associated with NMI 2) NMI_IO_CHECK associated with IO errors 3) NMI_UNKNOWN NMI unclaimed by all local handlers. On Gen10 going forward we plan to restrict to just iLO generated NMIs. There is a long history on hp/hpe proliant systems where hpwdt was handler of general IO errors (at least ones that would cause NMI to be generated) and we chose to panic in these situation as the errors were generally quite serious. Yes, this has caused some problems in the past as Linux has overloaded NMI and some subsystems didn't claim the NMIs that they generated (think profiling.) But, I haven't seen these types of problems for several years now. The more modern platforms have more robust error handling built into them and to linux so going forward we'll restrict hpwdt to a more traditional WDT role. But we're retaining the more conservative approach for legacy platforms. How would you suggest that the message be enhanced? > * Watchdog provided by hpwdt is not working after such panic. > > Here are the patches that should fix this. > > This is an RFC patch series because I am not sure that patches are > correct. Questions: > > * Are "mynmi" flags always set on all supported iLO versions when iLO >is the source of NMI? Unfortunately no. hpwdt is a dual purpose driver. It handles the iLO watchdog timer and the "Generate NMI to System" button. These are closely related hardware wise. However, some platforms generate NMI for "Generate NMI to System" button but aren't signaled via iLO registers. These will show up as NMI_UNKNOWN, hence while hpwdt still claims these. There are also some systems that do not set the nmistat bits correctly. So as to not break legacy platforms, the use the nmistat bits for control will be for Gen10 going forward. > * Is it safe to reset "mynmi" flags to zero if code decides to not panic? The reading of the registers is itself destructive (sets to zero) but the real issue is that some proliant systems lack the ability to acknowledge the NMI so only one can ever be received. So returning is not advisable as no further NMI will be generated via this path. A reset through firmware is required to restore the feature. > > Ivan Mironov (4): > watchdog: hpwdt: Don't disable watchdog on NMI > watchdog: hpwdt: Don't panic on foreign NMI > watchdog: hpwdt: Add more information into message > watchdog: hpwdt: Make panic behaviour configurable > > drivers/watchdog/hpwdt.c | 45 ++-- > 1 file changed, 25 insertions(+), 20 deletions(-) > > -- > 2.20.1 -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [RFC PATCH 4/4] watchdog: hpwdt: Make panic behaviour configurable
On Mon, Jan 14, 2019 at 07:36:17AM +0500, Ivan Mironov wrote:
> This adds an option to not panic on NMI even if it was caused by iLO.
>
> Signed-off-by: Ivan Mironov
> ---
> drivers/watchdog/hpwdt.c | 19 ---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 95d002b5b120..b12858491189 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -37,6 +37,10 @@ static unsigned int soft_margin = DEFAULT_MARGIN; /* in
> seconds */
> static bool nowayout = WATCHDOG_NOWAYOUT;
> static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);
>
> +#ifdef CONFIG_HPWDT_NMI_DECODING
> +static bool panic_on_nmi = true;
> +#endif /* CONFIG_HPWDT_NMI_DECODING */
> +
> static void __iomem *pci_mem_addr; /* the PCI-memory address */
> static unsigned long __iomem *hpwdt_nmistat;
> static unsigned long __iomem *hpwdt_timer_reg;
> @@ -146,7 +150,10 @@ static int hpwdt_set_pretimeout(struct watchdog_device
> *wdd, unsigned int req)
>
> static int hpwdt_my_nmi(void)
> {
> - return ioread8(hpwdt_nmistat) & 0x6;
> + int nmistat = ioread8(hpwdt_nmistat);
> +
> + iowrite8(nmistat & ~0x6, hpwdt_nmistat);
> + return nmistat & 0x6;
This is a read only register.
> }
>
> /*
> @@ -168,7 +175,10 @@ static int hpwdt_pretimeout(unsigned int ulReason,
> struct pt_regs *regs)
>"4. iLO Event Log\n",
>mynmi, ulReason, smp_processor_id());
>
> - nmi_panic(regs, "hpwdt: NMI: Not continuing");
> + if (panic_on_nmi)
> + nmi_panic(regs, "hpwdt: NMI: Not continuing");
> +
> + pr_emerg("Dazed and confused, but trying to continue\n");
>
The watchdog core provides a way to enable/disable the NMI pretimeout
dynamically
via ioctl. The pretimeout NMI can also be disabled via module parameter to
hpwdt.
This adds complexity without really adding functionality.
BTW, don't reuse error messages. Makes it difficult to tell where the error
originated from.
> return NMI_HANDLED;
> }
> @@ -376,6 +386,9 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped
> once started (default="
> #ifdef CONFIG_HPWDT_NMI_DECODING
> module_param(pretimeout, bool, 0);
> MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
> -#endif
> +
> +module_param(panic_on_nmi, bool, 0);
> +MODULE_PARM_DESC(panic_on_nmi, "Cause panic on NMI induced by iLO
> (default=1)");
> +#endif /* CONFIG_HPWDT_NMI_DECODING */
>
> module_pci_driver(hpwdt_driver);
> --
> 2.20.1
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
[PATCH v2 2/3] watchdog/hpwdt: Do not claim unsupported hardware
Do not claim when SSID 0x0289 as the watchdog features
are not enabled/validated by the firmware.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index eecd014..c8e8055 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -52,6 +52,7 @@
static const struct pci_device_id hpwdt_blacklist[] = {
{ PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3306, PCI_VENDOR_ID_HP, 0x1979) },
/* auxilary iLO */
+ { PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3306, PCI_VENDOR_ID_HP_3PAR,
0x0289) }, /* CL */
{0},/* terminate list */
};
--
1.8.3.1
[PATCH v2 0/3] watchdog/hpwdt: Do not claim on unsupported hardware
hpwdt driver isn't supported on all iLO hardware. Changes in version 2: 1) Instead of having explicit if statement to check device IDs, provide a pci_device_id table of devices to blacklist. 2) Convert the current instance where SSID 0x1979 was being skipped. 3) Add new patch to add SSID 0x0289 to the blacklist table. 4) Print via dev_dbg instead of dev_info. Jerry Hoemann (3): watchdog/hpwdt: Exclude via blacklist watchdog/hpwdt: Do not claim unsupported hardware watchdog/hpwdt: Update driver version. drivers/watchdog/hpwdt.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) -- 1.8.3.1
[PATCH v2 3/3] watchdog/hpwdt: Update driver version.
Bump version number to reflect recent minor changes. Signed-off-by: Jerry Hoemann --- 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 c8e8055..ef30c7e 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -26,7 +26,7 @@ #include #include -#define HPWDT_VERSION "2.0.1" +#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_TIMERTICKS_TO_SECS(65535) -- 1.8.3.1
[PATCH v2 1/3] watchdog/hpwdt: Exclude via blacklist
Instead of having explicit if statments excluding devices,
use a pci_device_id table of devices to blacklist.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 9356230..eecd014 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -50,6 +50,10 @@
};
MODULE_DEVICE_TABLE(pci, hpwdt_devices);
+static const struct pci_device_id hpwdt_blacklist[] = {
+ { PCI_DEVICE_SUB(PCI_VENDOR_ID_HP, 0x3306, PCI_VENDOR_ID_HP, 0x1979) },
/* auxilary iLO */
+ {0},/* terminate list */
+};
/*
* Watchdog operations
@@ -274,12 +278,10 @@ static int hpwdt_init_one(struct pci_dev *dev,
return -ENODEV;
}
- /*
-* Ignore all auxilary iLO devices with the following PCI ID
-*/
- if (dev->subsystem_vendor == PCI_VENDOR_ID_HP &&
- dev->subsystem_device == 0x1979)
+ if (pci_match_id(hpwdt_blacklist, dev)) {
+ dev_dbg(&dev->dev, "Not supported on this device\n");
return -ENODEV;
+ }
if (pci_enable_device(dev)) {
dev_warn(&dev->dev,
--
1.8.3.1
[PATCH 1/1] watchdog/hpwdt: Do not claim unsupported hardware
Do not claim when SSID 0x0289 as the watchdog features
are not enabled/validated by the firmware.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 9356230..b99ef0a4 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -281,6 +281,17 @@ static int hpwdt_init_one(struct pci_dev *dev,
dev->subsystem_device == 0x1979)
return -ENODEV;
+ /*
+* Ignore unsupported hardware
+*/
+ if (dev->vendor == PCI_VENDOR_ID_HP &&
+ dev->device == 0x3306 &&
+ dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR &&
+ dev->subsystem_device == 0x0289) {
+ dev_info(&dev->dev, "Not supported on this platform\n");
+ return -ENODEV;
+ }
+
if (pci_enable_device(dev)) {
dev_warn(&dev->dev,
"Not possible to enable PCI Device: 0x%x:0x%x.\n",
--
1.8.3.1
[PATCH 1/1] selftests: watchdog: Add gettimeleft command line arg
Add command line argument to call and display the results
of ioctl WDIOC_GETTIMELEFT.
Signed-off-by: Jerry Hoemann
---
tools/testing/selftests/watchdog/watchdog-test.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/watchdog/watchdog-test.c
b/tools/testing/selftests/watchdog/watchdog-test.c
index c6bd9a6..dac907a 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -19,7 +19,7 @@
int fd;
const char v = 'V';
-static const char sopts[] = "bdehp:t:Tn:N";
+static const char sopts[] = "bdehp:t:Tn:NL";
static const struct option lopts[] = {
{"bootstatus", no_argument, NULL, 'b'},
{"disable", no_argument, NULL, 'd'},
@@ -30,6 +30,7 @@
{"gettimeout", no_argument, NULL, 'T'},
{"pretimeout",required_argument, NULL, 'n'},
{"getpretimeout", no_argument, NULL, 'N'},
+ {"gettimeleft", no_argument, NULL, 'L'},
{NULL, no_argument, NULL, 0x0}
};
@@ -77,6 +78,7 @@ static void usage(char *progname)
printf(" -T, --gettimeoutGet the timeout\n");
printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");
printf(" -N, --getpretimeout Get the pretimeout\n");
+ printf(" -L, --gettimeleft Get the time left until timer experies\n");
printf("\n");
printf("Parameters are parsed left-to-right in real-time.\n");
printf("Example: %s -d -t 10 -p 5 -e\n", progname);
@@ -180,6 +182,15 @@ int main(int argc, char *argv[])
else
printf("WDIOC_GETPRETIMEOUT error '%s'\n",
strerror(errno));
break;
+ case 'L':
+ oneshot = 1;
+ ret = ioctl(fd, WDIOC_GETTIMELEFT, &flags);
+ if (!ret)
+ printf("WDIOC_GETTIMELEFT returns %u
seconds.\n", flags);
+ else
+ printf("WDIOC_GETTIMELEFT error '%s'\n",
strerror(errno));
+ break;
+
default:
usage(argv[0]);
goto end;
--
1.8.3.1
Re: [PATCH] watchdog: core: suppress "watchdog did not stop" message
On Fri, Nov 16, 2018 at 12:37:28AM +, Tao Ren wrote: > On 11/15/18 4:19 PM, Guenter Roeck wrote: > > NACK. This message is displayed if/when the watchdog application > > exits without stopping the watchdog and/or without closing properly. > > This _is_ critical since it will reboot the system after the next > > timeout period. > > > > If userspace triggers this message on purpose (eg by the mentioned > > script, which does not exit properly), userspace is at fault, > > not the kernel. > > > > Guenter > > Thank you for the quick response, Guenter. I see the log each time when I > reboot my system, and when I searched the message in google, I also found > posts asking why the message is printed at reboot, and that's why I feel it's > confusing. > > Anyways, please ignore the patch since it's necessary. Tao, If you're on a system running systemd, the default behavior is to enable the watchdog during shutdown. This guards against shutdown hanging. So this message will be routinely printed out during orderly shutdown. See file: /etc/systemd/system.conf -- ... # Entries in this file show the compile time defaults. ... #ShutdownWatchdogSec=10min -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
[V3 PATCH 2/2] selftests: watchdog: Add gettimeout and get|set pretimeout
Add command line arguments to call ioctl WDIOC_GETTIMEOUT,
WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT.
Signed-off-by: Jerry Hoemann
---
tools/testing/selftests/watchdog/watchdog-test.c | 33 +++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/watchdog/watchdog-test.c
b/tools/testing/selftests/watchdog/watchdog-test.c
index 8429186..e29bc71 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -19,7 +19,7 @@
int fd;
const char v = 'V';
-static const char sopts[] = "bdehp:t:";
+static const char sopts[] = "bdehp:t:Tn:N";
static const struct option lopts[] = {
{"bootstatus", no_argument, NULL, 'b'},
{"disable", no_argument, NULL, 'd'},
@@ -27,6 +27,9 @@
{"help",no_argument, NULL, 'h'},
{"pingrate", required_argument, NULL, 'p'},
{"timeout", required_argument, NULL, 't'},
+ {"gettimeout", no_argument, NULL, 'T'},
+ {"pretimeout",required_argument, NULL, 'n'},
+ {"getpretimeout", no_argument, NULL, 'N'},
{NULL, no_argument, NULL, 0x0}
};
@@ -71,9 +74,13 @@ static void usage(char *progname)
printf(" -h, --help Print the help message\n");
printf(" -p, --pingrate=PSet ping rate to P seconds (default
%d)\n", DEFAULT_PING_RATE);
printf(" -t, --timeout=T Set timeout to T seconds\n");
+ printf(" -T, --gettimeoutGet the timeout\n");
+ printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");
+ printf(" -N, --getpretimeout Get the pretimeout\n");
printf("\n");
printf("Parameters are parsed left-to-right in real-time.\n");
printf("Example: %s -d -t 10 -p 5 -e\n", progname);
+ printf("Example: %s -t 12 -T -n 7 -N\n", progname);
}
int main(int argc, char *argv[])
@@ -135,6 +142,30 @@ int main(int argc, char *argv[])
else
printf("WDIOC_SETTIMEOUT error '%s'\n",
strerror(errno));
break;
+ case 'T':
+ oneshot = 1;
+ ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
+ if (!ret)
+ printf("WDIOC_GETTIMEOUT returns %u
seconds.\n", flags);
+ else
+ printf("WDIOC_GETTIMEOUT error '%s'\n",
strerror(errno));
+ break;
+ case 'n':
+ flags = strtoul(optarg, NULL, 0);
+ ret = ioctl(fd, WDIOC_SETPRETIMEOUT, &flags);
+ if (!ret)
+ printf("Watchdog pretimeout set to %u
seconds.\n", flags);
+ else
+ printf("WDIOC_SETPRETIMEOUT error '%s'\n",
strerror(errno));
+ break;
+ case 'N':
+ oneshot = 1;
+ ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
+ if (!ret)
+ printf("WDIOC_GETPRETIMEOUT returns %u
seconds.\n", flags);
+ else
+ printf("WDIOC_GETPRETIMEOUT error '%s'\n",
strerror(errno));
+ break;
default:
usage(argv[0]);
goto end;
--
1.8.3.1
[V3 PATCH 0/2] selftests: watchdog: Add get/set/pre timeout
Add command line arguments to call ioctl WDIOC_GETTIMEOUT, WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT. Changes v2 1) Update usage to include argument 2) Update usage to give example. 3) Made printf of WDIOC_GETTIMEOUT distinct from WDIOC_SETTIMEOUT 4) Made WDIOC_GETTIMEOUT a "one shot" 5) Made printf of WDIOC_GETPRETIMEOUT disnct from WDIOC_SETPRETIMEOUT 6) Made WDIOC_GETPRETIMEOUT a "one shot" Change v3 1) Printf says errno, but prints the string version of the error. Make the printf consistent. 2) As above error was cut/paste from prior printf in application add new patch 1 to fix the existing printf first. Jerry Hoemann (2): selftests: watchdog: Fix error message. selftests: watchdog: Add gettimeout and get|set pretimeout tools/testing/selftests/watchdog/watchdog-test.c | 41 +--- 1 file changed, 36 insertions(+), 5 deletions(-) -- 1.8.3.1
[V3 PATCH 1/2] selftests: watchdog: Fix error message.
Printf's say errno but print the string version of error.
Make consistent.
Signed-off-by: Jerry Hoemann
---
tools/testing/selftests/watchdog/watchdog-test.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/watchdog/watchdog-test.c
b/tools/testing/selftests/watchdog/watchdog-test.c
index 6e29087..8429186 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -103,7 +103,7 @@ int main(int argc, char *argv[])
printf("Last boot is caused by: %s.\n", (flags
!= 0) ?
"Watchdog" : "Power-On-Reset");
else
- printf("WDIOC_GETBOOTSTATUS errno '%s'\n",
strerror(errno));
+ printf("WDIOC_GETBOOTSTATUS error '%s'\n",
strerror(errno));
break;
case 'd':
flags = WDIOS_DISABLECARD;
@@ -111,7 +111,7 @@ int main(int argc, char *argv[])
if (!ret)
printf("Watchdog card disabled.\n");
else
- printf("WDIOS_DISABLECARD errno '%s'\n",
strerror(errno));
+ printf("WDIOS_DISABLECARD error '%s'\n",
strerror(errno));
break;
case 'e':
flags = WDIOS_ENABLECARD;
@@ -119,7 +119,7 @@ int main(int argc, char *argv[])
if (!ret)
printf("Watchdog card enabled.\n");
else
- printf("WDIOS_ENABLECARD errno '%s'\n",
strerror(errno));
+ printf("WDIOS_ENABLECARD error '%s'\n",
strerror(errno));
break;
case 'p':
ping_rate = strtoul(optarg, NULL, 0);
@@ -133,7 +133,7 @@ int main(int argc, char *argv[])
if (!ret)
printf("Watchdog timeout set to %u seconds.\n",
flags);
else
- printf("WDIOC_SETTIMEOUT errno '%s'\n",
strerror(errno));
+ printf("WDIOC_SETTIMEOUT error '%s'\n",
strerror(errno));
break;
default:
usage(argv[0]);
--
1.8.3.1
Re: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout
On Wed, Sep 26, 2018 at 01:47:25PM -0600, Shuah Khan wrote:
> On 09/26/2018 10:29 AM, Jerry Hoemann wrote:
> > On Tue, Sep 25, 2018 at 02:51:15PM -0600, Shuah Khan wrote:
> >> Hi Jerry,
> >>
> >>
> >> The rest looks good to me.
>
> I spoke too soon. I ran your patch on softdog and error messages in
> unsupported
> cases could you refinement. Please see below:
>
> Sorry for not catching this earlier.
>
> >>
> >>> }
> >>>
> >>> int main(int argc, char *argv[])
> >>> @@ -135,6 +142,30 @@ int main(int argc, char *argv[])
> >>> else
> >>> printf("WDIOC_SETTIMEOUT errno '%s'\n",
> >>> strerror(errno));
> >>> break;
> >>> + case 'T':
> >>> + oneshot = 1;
> >>> + ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
> >>> + if (!ret)
> >>> + printf("WDIOC_GETTIMEOUT returns %u
> >>> seconds.\n", flags);
> >>> + else
> >>> + printf("WDIOC_GETTIMEOUT errno '%s'\n",
> >>> strerror(errno));
>
> Either remove "errno" or change it to "error '%s'"
Oh, I see. I did a cut/paste from prior printf in file which have same issue.
I'll fix those while I'm at it.
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout
On Tue, Sep 25, 2018 at 02:51:15PM -0600, Shuah Khan wrote:
> Hi Jerry,
>
> On 09/24/2018 01:36 PM, Jerry Hoemann wrote:
> > {"disable", no_argument, NULL, 'd'},
> > @@ -27,6 +27,9 @@
> > {"help",no_argument, NULL, 'h'},
> > {"pingrate", required_argument, NULL, 'p'},
> > {"timeout", required_argument, NULL, 't'},
> > + {"gettimeout", no_argument, NULL, 'T'},
> > + {"pretimeout",required_argument, NULL, 'n'},
> > + {"getpretimeout", no_argument, NULL, 'N'},
> > {NULL, no_argument, NULL, 0x0}
> > };
> >
> > @@ -71,9 +74,13 @@ static void usage(char *progname)
> > printf(" -h, --help Print the help message\n");
> > printf(" -p, --pingrate=PSet ping rate to P seconds (default
> > %d)\n", DEFAULT_PING_RATE);
> > printf(" -t, --timeout=T Set timeout to T seconds\n");
> > + printf(" -T, --gettimeoutGet the timeout\n");
> > + printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");
> > + printf(" -N, --getpretimeout Get the pretimeout\n");
> > printf("\n");
> > printf("Parameters are parsed left-to-right in real-time.\n");
> > printf("Example: %s -d -t 10 -p 5 -e\n", progname);
> > + printf("Example: %s -t 12 -T -n 7 -N\n", progname);
>
> Would this work the way you want it though, since -N now is oneshot?
> Should this be just "printf("Example: %s -t 12 -T -n 7\n", progname); ??
Example shows how to set/query the timers to make sure value set was what
was intended.
Note: "oneshot" is a bit of a misnomer as it causes clean exit before
going into the keep alive loop, but one can still specify multiple
options.
>
> The rest looks good to me.
>
> > }
> >
> > int main(int argc, char *argv[])
> > @@ -135,6 +142,30 @@ int main(int argc, char *argv[])
> > else
> > printf("WDIOC_SETTIMEOUT errno '%s'\n",
> > strerror(errno));
> > break;
> > + case 'T':
> > + oneshot = 1;
> > + ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
> > + if (!ret)
> > + printf("WDIOC_GETTIMEOUT returns %u
> > seconds.\n", flags);
> > + else
> > + printf("WDIOC_GETTIMEOUT errno '%s'\n",
> > strerror(errno));
> > + break;
> > + case 'n':
> > + flags = strtoul(optarg, NULL, 0);
> > + ret = ioctl(fd, WDIOC_SETPRETIMEOUT, &flags);
> > + if (!ret)
> > + printf("Watchdog pretimeout set to %u
> > seconds.\n", flags);
> > + else
> > + printf("WDIOC_SETPRETIMEOUT errno '%s'\n",
> > strerror(errno));
> > + break;
> > + case 'N':
> > + oneshot = 1;
> > + ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
> > + if (!ret)
> > + printf("WDIOC_GETPRETIMEOUT returns %u
> > seconds.\n", flags);
> > + else
> > + printf("WDIOC_GETPRETIMEOUT errno '%s'\n",
> > strerror(errno));
> > + break;
> > default:
> > usage(argv[0]);
> > goto end;
> >
>
> thanks,
> -- Shuah
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout
Shuah,
Wrote this yesterday, and wanted to proof it before sending. I got
your other email earlier and replied to specific point on permission
of /dev/watchdog, so some of this is now redundant.
-
With the potential exception of error path, I think my v2 of the patch
addresses the issues you raised below. Additional comments inline.
On Mon, Sep 24, 2018 at 02:42:33PM -0600, Shuah Khan wrote:
> On 09/23/2018 07:47 PM, Jerry Hoemann wrote:
> > On Fri, Sep 21, 2018 at 05:42:00PM -0600, Shuah Khan wrote:
> >>>
> >>> @@ -135,6 +141,28 @@ int main(int argc, char *argv[])
> >>> else
> >>> printf("WDIOC_SETTIMEOUT errno '%s'\n",
> >>> strerror(errno));
> >>> break;
> >>> + case 'T':
> >>> + ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
> >>> + if (!ret)
> >>> + printf("Watchdog timeout set to %u seconds.\n",
> >>> flags);
> >>
> >> It would good to make this message different from the WDIOC_SETTIMEOUT
> >> message.
> >> Please update it to reflect that this is the result of a WDIOC_GETTIMEOUT.
> >
> > Will update message to make distinct.
> >
> >>
> >> What would user intend to do with this GETTIMEOUT? Shouldn't this be the
> >> case that it
> >> prints the current value and exits instead of the same logic as SETTIMEOUT
> >> option?
> >
> > Are you suggesting setting the "oneshot" flag so the test app doesn't
> > actually
> > go into the while(1) keep_alive loop?
> >
> > Watchdog drivers may adjust the requested value to match hardware
> > constraints.
> > Callers of set timeout (and set pretimeout) should call get timeout to see
> > what
> > value was actually set.
> >
> > B/c of above, I just got into the habit of specifying both flags: first set,
> > then get to make sure value set was what I intended.
> >
> > But I can make the "Get" a one shot. Just let me know if this is your
> > preference.
>
> I prefer that both GETs be oneshot. GETs should just print the current value
> and go
> follow oneshot path. It doesn't make sense for them to do more.
> >
> >
> >>
> >>> + else
> >>> + printf("WDIOC_GETTIMEOUT errno '%s'\n",
> >>> strerror(errno))
> >>
> >> Shouldn't this error be an exit condition?
> >
> > Hmmm, I don't see this error path much different than the error path for the
> > other failing ioctl. Am I missing something?
>
> Yeah that is what I don't understand with the new code as well as the
> existing.
> Shouldn't error path be handled differently. What is the point in doing more
> other than gracefully exit closing the file? I don't think existing error
> paths
> are doing this, probably they should.
Watchdog timers have a long and varied history in Linux. Traditionally, not all
watchdog have implemented all the ioctl interfaces. So, an ioctl returning
error
doesn't necessarily mean that an error has occurred, it might just mean
that the particular watchdog didn't implement that particular feature.
E.g., yes, we could error out if user tries to set a PRETIMEOUT on a system
that doesn't support that feature, or we could just continue.
> >
> >
> > But, If we make the "GET" a one shot, then we wouldn't really need to
> > special case the failure case as we wouldn't go into the keep_alive
> > loop in either case.
> >
> >
>
> Right.
>
> >
> >>
> >>> + break;
> >>> + case 'n':
> >>> + flags = strtoul(optarg, NULL, 0);
> >>> + ret = ioctl(fd, WDIOC_SETPRETIMEOUT, &flags);
> >>> + if (!ret)
> >>> + printf("Watchdog pretimeout set to %u
> >>> seconds.\n", flags);
> >>> + else
> >>> + printf("WDIOC_SETPRETIMEOUT errno '%s'\n",
> >>> strerror(errno));
> >>> + break;
> >>> + case 'N':
> >>> + ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
> >>> + if (!ret)
> >>> +
Re: [PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout
On Tue, Sep 25, 2018 at 09:50:18AM -0600, Shuah Khan wrote:
> >>> Also can you run this test as normal user?
> >>
> >> No. Must be run as root to open /dev/watchdog. When /dev/watchdog is
> >> opened, the
> >> WD is started and if not updated properly, the system will crash.
> >
> > Hmm. I don't understand why the system would panic if non-root user can't
> > open the
> > device, at least in the context of this test.
> >
> > fd = open("/dev/watchdog", O_WRONLY);
> >
> > if (fd == -1) {
> > printf("Watchdog device not enabled.\n");
> > exit(-1);
> > }
> >
> >
> > Shouldn't it just exit based on the code above?
> >
> >>
> >
> >> "cat /dev/watchdog" is one of my favorite ways to crash a system. :) :)
> >
> > That doesn't sound great, if a non-root user can bring the system down!!
> >
>
> This got me concerned enough that I tried this with softdog. It behaved just
> the way I expected it.
>
> cat /dev/watchdog
> cat: /dev/watchdog: Permission denied
>
> Running the test as non-root does the following as per the current logic.
>
> watchdog-test -b
> Watchdog device not enabled.
>
> I think this logic could be improved to detect that a non-root user is running
> the test and print appropriate message.
>
> However, I am not seeing the behavior you are describing that "cat
> /dev/watchdog"
> panics the syste. Did you mean running a root which is expected unless you
> terminate
> before the timeout? If you are seeing this as non-root user on you system, the
> watchdog driver could be suspect.
>
> thanks,
> -- Shuah
Sorry, for misunderstanding. Let me rephrase:
When you asked if the test can be run as a normal user::
No. The test must be run as root to open /dev/watchdog as the permission
on /dev/watchdog allow only root to open it. The reason that we only
allow root to open /dev/watchdog is that it is trivial to crash
the system. Just open /dev/watchdog and don't update the watchdog.
One of my favorite ways to crash the system is to
as root "cat /dev/watchdog."
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
[V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout
Add command line arguments to call ioctl WDIOC_GETTIMEOUT,
WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT.
Signed-off-by: Jerry Hoemann
---
tools/testing/selftests/watchdog/watchdog-test.c | 33 +++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/watchdog/watchdog-test.c
b/tools/testing/selftests/watchdog/watchdog-test.c
index 6e29087..3e8e393 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -19,7 +19,7 @@
int fd;
const char v = 'V';
-static const char sopts[] = "bdehp:t:";
+static const char sopts[] = "bdehp:t:Tn:N";
static const struct option lopts[] = {
{"bootstatus", no_argument, NULL, 'b'},
{"disable", no_argument, NULL, 'd'},
@@ -27,6 +27,9 @@
{"help",no_argument, NULL, 'h'},
{"pingrate", required_argument, NULL, 'p'},
{"timeout", required_argument, NULL, 't'},
+ {"gettimeout", no_argument, NULL, 'T'},
+ {"pretimeout",required_argument, NULL, 'n'},
+ {"getpretimeout", no_argument, NULL, 'N'},
{NULL, no_argument, NULL, 0x0}
};
@@ -71,9 +74,13 @@ static void usage(char *progname)
printf(" -h, --help Print the help message\n");
printf(" -p, --pingrate=PSet ping rate to P seconds (default
%d)\n", DEFAULT_PING_RATE);
printf(" -t, --timeout=T Set timeout to T seconds\n");
+ printf(" -T, --gettimeoutGet the timeout\n");
+ printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");
+ printf(" -N, --getpretimeout Get the pretimeout\n");
printf("\n");
printf("Parameters are parsed left-to-right in real-time.\n");
printf("Example: %s -d -t 10 -p 5 -e\n", progname);
+ printf("Example: %s -t 12 -T -n 7 -N\n", progname);
}
int main(int argc, char *argv[])
@@ -135,6 +142,30 @@ int main(int argc, char *argv[])
else
printf("WDIOC_SETTIMEOUT errno '%s'\n",
strerror(errno));
break;
+ case 'T':
+ oneshot = 1;
+ ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
+ if (!ret)
+ printf("WDIOC_GETTIMEOUT returns %u
seconds.\n", flags);
+ else
+ printf("WDIOC_GETTIMEOUT errno '%s'\n",
strerror(errno));
+ break;
+ case 'n':
+ flags = strtoul(optarg, NULL, 0);
+ ret = ioctl(fd, WDIOC_SETPRETIMEOUT, &flags);
+ if (!ret)
+ printf("Watchdog pretimeout set to %u
seconds.\n", flags);
+ else
+ printf("WDIOC_SETPRETIMEOUT errno '%s'\n",
strerror(errno));
+ break;
+ case 'N':
+ oneshot = 1;
+ ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
+ if (!ret)
+ printf("WDIOC_GETPRETIMEOUT returns %u
seconds.\n", flags);
+ else
+ printf("WDIOC_GETPRETIMEOUT errno '%s'\n",
strerror(errno));
+ break;
default:
usage(argv[0]);
goto end;
--
1.8.3.1
[V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout
Add command line arguments to call ioctl WDIOC_GETTIMEOUT, WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT. Changes v2 1) Update usage to include argument 2) Update usage to give example. 3) Made printf of WDIOC_GETTIMEOUT distinct from WDIOC_SETTIMEOUT 4) Made WDIOC_GETTIMEOUT a "one shot" 5) Made printf of WDIOC_GETPRETIMEOUT disnct from WDIOC_SETPRETIMEOUT 6) Made WDIOC_GETPRETIMEOUT a "one shot" Jerry Hoemann (1): selftests: watchdog: Add gettimeout and get|set pretimeout tools/testing/selftests/watchdog/watchdog-test.c | 33 +++- 1 file changed, 32 insertions(+), 1 deletion(-) -- 1.8.3.1
Re: [PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout
On Fri, Sep 21, 2018 at 05:42:00PM -0600, Shuah Khan wrote:
> Hi Jerry,
>
> Thanks for the patch. A few comments below:
Replies inline.
>
> On 09/21/2018 04:55 PM, Jerry Hoemann wrote:
> > Add command line arguments to call ioctl WDIOC_GETTIMEOUT,
> > WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT.
> >
> > Signed-off-by: Jerry Hoemann
> > ---
> > tools/testing/selftests/watchdog/watchdog-test.c | 30
> > +++-
> > 1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/watchdog/watchdog-test.c
> > b/tools/testing/selftests/watchdog/watchdog-test.c
> > index 6e29087..4861e2c 100644
> > --- a/tools/testing/selftests/watchdog/watchdog-test.c
> > +++ b/tools/testing/selftests/watchdog/watchdog-test.c
> > @@ -19,7 +19,7 @@
> >
> > int fd;
> > const char v = 'V';
> > -static const char sopts[] = "bdehp:t:";
> > +static const char sopts[] = "bdehp:t:Tn:N";
> > static const struct option lopts[] = {
> > {"bootstatus", no_argument, NULL, 'b'},
> > {"disable", no_argument, NULL, 'd'},
> > @@ -27,6 +27,9 @@
> > {"help",no_argument, NULL, 'h'},
> > {"pingrate", required_argument, NULL, 'p'},
> > {"timeout", required_argument, NULL, 't'},
> > + {"gettimeout", no_argument, NULL, 'T'},
> > + {"pretimeout",required_argument, NULL, 'n'},
> > + {"getpretimeout", no_argument, NULL, 'N'},
> > {NULL, no_argument, NULL, 0x0}
> > };
> >
> > @@ -71,6 +74,9 @@ static void usage(char *progname)
> > printf(" -h, --help Print the help message\n");
> > printf(" -p, --pingrate=PSet ping rate to P seconds (default
> > %d)\n", DEFAULT_PING_RATE);
> > printf(" -t, --timeout=T Set timeout to T seconds\n");
> > + printf(" -T, --gettimeoutGet the timeout\n");
> > + printf(" -n, --pretimeoutSet the pretimeout to T seconds\n");
> > + printf(" -N, --getpretimeout Get the pretimeout\n");
>
> How are the new arguments used?
I forgot the param. Should be:
+ printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");
I'll update in v2.
Is this what you mean? Or did I misunderstand?
>
> > printf("\n");
> > printf("Parameters are parsed left-to-right in real-time.\n");
> > printf("Example: %s -d -t 10 -p 5 -e\n", progname);
>
> Please add an example usage for each of these new arguments.
Will do.
>
> > @@ -135,6 +141,28 @@ int main(int argc, char *argv[])
> > else
> > printf("WDIOC_SETTIMEOUT errno '%s'\n",
> > strerror(errno));
> > break;
> > + case 'T':
> > + ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
> > + if (!ret)
> > + printf("Watchdog timeout set to %u seconds.\n",
> > flags);
>
> It would good to make this message different from the WDIOC_SETTIMEOUT
> message.
> Please update it to reflect that this is the result of a WDIOC_GETTIMEOUT.
Will update message to make distinct.
>
> What would user intend to do with this GETTIMEOUT? Shouldn't this be the case
> that it
> prints the current value and exits instead of the same logic as SETTIMEOUT
> option?
Are you suggesting setting the "oneshot" flag so the test app doesn't actually
go into the while(1) keep_alive loop?
Watchdog drivers may adjust the requested value to match hardware constraints.
Callers of set timeout (and set pretimeout) should call get timeout to see what
value was actually set.
B/c of above, I just got into the habit of specifying both flags: first set,
then get to make sure value set was what I intended.
But I can make the "Get" a one shot. Just let me know if this is your
preference.
>
> > + else
> > + printf("WDIOC_GETTIMEOUT errno '%s'\n",
> > strerror(errno))
>
> Shouldn't this error be an exit condition?
Hmmm, I don't see this error path much different than the error path for the
other failing ioctl. Am I missing something?
But, If we make the "GET" a one shot, then we wouldn't really need to
[PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout
Add command line arguments to call ioctl WDIOC_GETTIMEOUT,
WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT.
Signed-off-by: Jerry Hoemann
---
tools/testing/selftests/watchdog/watchdog-test.c | 30 +++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/watchdog/watchdog-test.c
b/tools/testing/selftests/watchdog/watchdog-test.c
index 6e29087..4861e2c 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -19,7 +19,7 @@
int fd;
const char v = 'V';
-static const char sopts[] = "bdehp:t:";
+static const char sopts[] = "bdehp:t:Tn:N";
static const struct option lopts[] = {
{"bootstatus", no_argument, NULL, 'b'},
{"disable", no_argument, NULL, 'd'},
@@ -27,6 +27,9 @@
{"help",no_argument, NULL, 'h'},
{"pingrate", required_argument, NULL, 'p'},
{"timeout", required_argument, NULL, 't'},
+ {"gettimeout", no_argument, NULL, 'T'},
+ {"pretimeout",required_argument, NULL, 'n'},
+ {"getpretimeout", no_argument, NULL, 'N'},
{NULL, no_argument, NULL, 0x0}
};
@@ -71,6 +74,9 @@ static void usage(char *progname)
printf(" -h, --help Print the help message\n");
printf(" -p, --pingrate=PSet ping rate to P seconds (default
%d)\n", DEFAULT_PING_RATE);
printf(" -t, --timeout=T Set timeout to T seconds\n");
+ printf(" -T, --gettimeoutGet the timeout\n");
+ printf(" -n, --pretimeoutSet the pretimeout to T seconds\n");
+ printf(" -N, --getpretimeout Get the pretimeout\n");
printf("\n");
printf("Parameters are parsed left-to-right in real-time.\n");
printf("Example: %s -d -t 10 -p 5 -e\n", progname);
@@ -135,6 +141,28 @@ int main(int argc, char *argv[])
else
printf("WDIOC_SETTIMEOUT errno '%s'\n",
strerror(errno));
break;
+ case 'T':
+ ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
+ if (!ret)
+ printf("Watchdog timeout set to %u seconds.\n",
flags);
+ else
+ printf("WDIOC_GETTIMEOUT errno '%s'\n",
strerror(errno));
+ break;
+ case 'n':
+ flags = strtoul(optarg, NULL, 0);
+ ret = ioctl(fd, WDIOC_SETPRETIMEOUT, &flags);
+ if (!ret)
+ printf("Watchdog pretimeout set to %u
seconds.\n", flags);
+ else
+ printf("WDIOC_SETPRETIMEOUT errno '%s'\n",
strerror(errno));
+ break;
+ case 'N':
+ ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
+ if (!ret)
+ printf("Watchdog pretimeout set to %u
seconds.\n", flags);
+ else
+ printf("WDIOC_GETPRETIMEOUT errno '%s'\n",
strerror(errno));
+ break;
default:
usage(argv[0]);
goto end;
--
1.8.3.1
[PATCH] watchdog/hpwdt: Disable PreTimeout when Timeout is smaller
During module install, disable pretimeout if the requested timeout
value is not greater than the minimal pretimeout value that is
supported by hardware.
This makes the module load handling of pretimeout consistent
with the ioctl handling of pretimeout.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 7af358b3e278..93562304f7aa 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -311,6 +311,10 @@ 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);
+ if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) {
+ dev_warn(&dev->dev, "timeout <= pretimeout. Setting pretimeout
to zero\n");
+ pretimeout = 0;
+ }
hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
hpwdt_dev.parent = &dev->dev;
--
2.13.6
Re: [PATCH v2 0/5] watchdog: hpwdt: Bug Fixes/Enhancement
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 -
[PATCH] watchdog: hpwdt: Update Driver Documentation.
Remove references to deprecated features like NMI sourcing and obsoleted module parameters. Add details concerning new module parameter pretimeout and tips to programming it. Signed-off-by: Jerry Hoemann --- Documentation/watchdog/hpwdt.txt | 93 ++-- 1 file changed, 31 insertions(+), 62 deletions(-) diff --git a/Documentation/watchdog/hpwdt.txt b/Documentation/watchdog/hpwdt.txt index 6d866c537127..55df692c5595 100644 --- a/Documentation/watchdog/hpwdt.txt +++ b/Documentation/watchdog/hpwdt.txt @@ -1,15 +1,12 @@ -Last reviewed: 05/20/2016 +Last reviewed: 08/20/2018 HPE iLO NMI Watchdog Driver - NMI sourcing for iLO based ProLiant Servers - Documentation and Driver by - Thomas Mingarelli + for iLO based ProLiant Servers The HPE iLO NMI Watchdog driver is a kernel module that provides basic - watchdog functionality and the added benefit of NMI sourcing. Both the - watchdog functionality and the NMI sourcing capability need to be enabled - by the user. Remember that the two modes are not dependent on one another. - A user can have the NMI sourcing without the watchdog timer and vice-versa. + watchdog functionality and handler for the iLO "Generate NMI to System" + virtual button. + All references to iLO in this document imply it also works on iLO2 and all subsequent generations. @@ -21,12 +18,16 @@ Last reviewed: 05/20/2016 not be updated in a timely fashion and a hardware system reset (also known as an Automatic Server Recovery (ASR)) event will occur. - The hpwdt driver also has three (3) module parameters. They are the following: + The hpwdt driver also has the following module parameters: soft_margin - allows the user to set the watchdog timer value. Default value is 30 seconds. - allow_kdump - allows the user to save off a kernel dump image after an NMI. - Default value is 1/ON + timeout - an alias of soft_margin. + pretimeout - allows the user to set the watchdog pretimeout value. + This is the number of seconds before timeout when an + NMI is delivered to the system. Setting the value to + zero disables the pretimeout NMI. + Default value is 9 seconds. nowayout- basic watchdog parameter that does not allow the timer to be restarted or an impending ASR to be escaped. Default value is set when compiling the kernel. If it is set @@ -37,61 +38,29 @@ Last reviewed: 05/20/2016 interface to /dev/watchdog can be found in Documentation/watchdog/watchdog-api.txt and Documentation/IPMI.txt. - The NMI sourcing capability is disabled by default due to the inability to - distinguish between "NMI Watchdog Ticks" and "HW generated NMI events" in the - Linux kernel. What this means is that the hpwdt nmi handler code is called - each time the NMI signal fires off. This could amount to several thousands of - NMIs in a matter of seconds. If a user sees the Linux kernel's "dazed and - confused" message in the logs or if the system gets into a hung state, then - the hpwdt driver can be reloaded. - - 1. If the kernel has not been booted with nmi_watchdog turned off then -edit and place the nmi_watchdog=0 at the end of the currently booting -kernel line. Depending on your Linux distribution and platform setup: -For non-UEFI systems - /boot/grub/grub.conf or - /boot/grub/menu.lst -For UEFI systems - /boot/efi/EFI/distroname/grub.conf or - /boot/efi/efi/distroname/elilo.conf - 2. reboot the sever - 3. Once the system comes up perform a modprobe -r hpwdt - 4. modprobe /lib/modules/`uname -r`/kernel/drivers/watchdog/hpwdt.ko - - Now, the hpwdt can successfully receive and source the NMI and provide a log - message that details the reason for the NMI (as determined by the HPE BIOS). - - Below is a list of NMIs the HPE BIOS understands along with the associated - code (reason): - - No source found00h - - Uncorrectable Memory Error 01h - - ASR NMI1Bh - - PCI Parity Error 20h - - NMI Button Press 27h - - SB_BUS_NMI 28h - - ILO Doorbell NMI 29h - - ILO IOP NMI2Ah - - ILO Watchdog NMI 2Bh - - Proc Throt NMI 2Ch + Due to limitations in the iLO hardware, the NMI pretimeout if enabled, + can only be set to 9 seconds. Attempts to set pretimeout to other + non-zero values will be rounded, possibly to zero. Users should verify + the pretimeout value after attempting to set pretimeout or timeout. - Front Side Bus NMI 2Dh + Upon receipt of an NMI from the iLO, the hpwdt driver will initiate a + panic. This is to a
[PATCH v2 2/5] 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. 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 --- 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
[PATCH v2 1/5] 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.
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
---
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
[PATCH v2 3/5] watchdog: hpwdt: Display module parameters.
Print module parameters when the driver is loaded. Signed-off-by: Jerry Hoemann --- 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
[PATCH v2 5/5] watchdog: hpwdt: Update version number.
Bump version number to reflect recent bug fixes. Signed-off-by: Jerry Hoemann --- 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 #include -#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_TIMERTICKS_TO_SECS(65535) -- 1.8.3.1
[PATCH v2 4/5] watchdog: hpwdt: Module paramerter alias.
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 --- 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
[PATCH v2 0/5] watchdog: hpwdt: Bug Fixes/Enhancement
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
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 > > Guess you know what you are doing here. > > Reviewed-by: Guenter Roeck 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 -
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 > > --- > > 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 -
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 > > --- > > 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 -
[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
[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
---
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
[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 --- 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
[PATCH 4/4] watchdog: hpwdt: Update version number.
Bump version number to reflect recent bug fixes. Signed-off-by: Jerry Hoemann --- 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 #include -#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_TIMERTICKS_TO_SECS(65535) -- 1.8.3.1
[PATCH 3/4] watchdog: hpwdt: Display module parameters.
Print module parameters when the driver is loaded. Signed-off-by: Jerry Hoemann --- 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
[PATCH 1/1] watchdog: hpwdt: Claim NMIs generated by iLO5
On iLO5 going forward we want to return and not claim the NMI, if the NMI was NOT gnerated by the iLO as a result of the watchdog timing out or an explicit generate NMI. The sense of the test in is inverted and prevents hpwdt_pretimeout from claiming NMIs when it should. Signed-off-by: Jerry Hoemann --- 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 a43ab2cecca2..9dc62a461451 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -159,7 +159,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) "3. OA Forward Progress Log\n" "4. iLO Event Log"; - if (ilo5 && ulReason == NMI_UNKNOWN && mynmi) + if (ilo5 && ulReason == NMI_UNKNOWN && !mynmi) return NMI_DONE; if (ilo5 && !pretimeout) -- 2.13.6
[PATCH 0/1] watchdog/hpwdt: Claim NMIs generated by iLO5
Guenter, In v4 of my Febuary patch series: https://lkml.org/lkml/2018/2/25/201 I had a merge error in hpwdt.c that causes hpwdt to not claim NMIs that it should on iLO5 (i.e. Gen10) systems. The Feb patches were recently picked up for 4.17-rc2. Not sure the appropriate action here. The merge error ocurred on: https://lkml.org/lkml/2018/2/25/203 And was carried forward through out the remaining patches in the series. We could ask for patches to be reverted and reworked. Or, we can apply a 1-line fix. If the latter, here is the 1-line fix. Jerry Hoemann (1): watchdog: hpwdt: Claim NMIs generated by iLO5 drivers/watchdog/hpwdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.13.6
Re: [PATCH 4.4 29/63] watchdog: hpwdt: fix unused variable warning
Greg,
Sorry, if I'm missing something, but I see 3 patches for
hpwdt queued up for 4.4:
queue-4.4/watchdog-hpwdt-fix-unused-variable-warning.patch
queue-4.4/watchdog-hpwdt-smbios-check.patch
queue-4.4/watchdog-hpwdt-check-source-of-nmi.patch
Shouldn't there also be a 4.4 patch for
commit 2b3d89b402b085b08498e896c65267a145bed486
watchdog: hpwdt: Remove legacy NMI sourcing.
As there was for 4.15, 4.14, and 4.9?
commit 2b3d89b40 is the Spectre related patch.
thanks
Jerry Hoemann
On Fri, Mar 16, 2018 at 04:23:01PM +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch. If anyone has any objections, please let me know.
>
> --
>
> From: Arnd Bergmann
>
> commit aeebc6ba88ba3758ad95467ff6191fabf2074c13 upstream.
>
> The new hpwdt_my_nmi() function is used conditionally, which produces
> a harmless warning in some configurations:
>
> drivers/watchdog/hpwdt.c:478:12: error: 'hpwdt_my_nmi' defined but not used
> [-Werror=unused-function]
>
> This moves it inside of the #ifdef that protects its caller, to silence
> the warning.
>
> Fixes: 621174a92851 ("watchdog: hpwdt: Check source of NMI")
> Signed-off-by: Arnd Bergmann
> Reviewed-by: Jerry Hoemann
> Reviewed-by: Guenter Roeck
> Signed-off-by: Guenter Roeck
> Signed-off-by: Wim Van Sebroeck
> Signed-off-by: Greg Kroah-Hartman
>
> ---
> drivers/watchdog/hpwdt.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -474,12 +474,12 @@ static int hpwdt_time_left(void)
> return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
> }
>
> +#ifdef CONFIG_HPWDT_NMI_DECODING
> static int hpwdt_my_nmi(void)
> {
> return ioread8(hpwdt_nmistat) & 0x6;
> }
>
> -#ifdef CONFIG_HPWDT_NMI_DECODING
> /*
> * NMI Handler
> */
>
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [PATCH v5 2/9] watchdog/hpwdt: Remove legacy NMI sourcing.
On Mon, Feb 26, 2018 at 05:29:55PM -0800, Guenter Roeck wrote: > On 02/26/2018 05:02 PM, Jerry Hoemann wrote: > > On Mon, Feb 26, 2018 at 06:32:30AM -0800, Guenter Roeck wrote: > > > On 02/26/2018 06:11 AM, Arnd Bergmann wrote: > > > > On Mon, Feb 26, 2018 at 4:22 AM, Jerry Hoemann > > > > wrote: > > > > > Gen8 and prior Proliant systems supported the "CRU" interface > > > > > to firmware. This interfaces allows linux to "call back" into > > > > > firmware > > > > > to source the cause of an NMI. This feature isn't fully utilized > > > > > as the actual source of the NMI isn't printed, the driver only > > > > > indicates that the source couldn't be determined when the call > > > > > fails. > > > > > > > > > > With the advent of Gen9, iCRU replaces the CRU. The call back > > > > > feature is no longer available in firmware. To be compatible and > > > > > not attempt to call back into firmware on system not supporting CRU, > > > > > the SMBIOS table is consulted to determine if it is safe to > > > > > make the call back or not. > > > > > > > > > > This results in about half of the driver code being devoted > > > > > to either making CRU calls or determing if it is safe to make > > > > > CRU calls. As noted, the driver isn't really using the results of > > > > > the CRU calls. > > > > > > > > > > Furthermore, as a consequence of the Spectre security issue, the > > > > > BIOS/EFI calls are being wrapped into Spectre-disabling section. > > > > > Removing the call back in hpwdt_pretimeout assists in this effort. > > > > > > > > > > As the CRU sourcing of the NMI isn't required for handling the > > > > > NMI and there are security concerns with making the call back, remove > > > > > the legacy (pre Gen9) NMI sourcing and the DMI code to determine if > > > > > the system had the CRU interface. > > > > > > > > > > Signed-off-by: Jerry Hoemann > > > > > > > > This avoids a warning in mainline kernels, so that's great: > > > > > > > > drivers/watchdog/hpwdt.o: warning: objtool: .text+0x24: indirect call > > > > found in RETPOLINE build > > > > > > > > I wonder what we do about stable kernels. Are both this patch and the > > > > patch > > > > that added the objtool warning message candidates for backports to > > > > stable kernels? > > > > > > > > > > Makes sense to me, but it is really a bit more than a bug fix, so I'll > > > leave it up to Jerry/HPE to make the call in respect to hpwdt. > > > > > > > Generally speaking, HPE customers who run linux do so through a distro > > vendor and pick up patches from them. But I'm sure there are some > > customers who do things differently. > > > > The distro vendor's have their own repos and we'll work with them > > to back port patches to their code base. So, I typically don't do a lot > > of kernel.org stable branch work. > > > > Looks like objtool has been enhanced to find Spectre vulnerable code. > > Are the other kernel patches related to Spectre being back ported > > to stable release lines? If yes, it probably make sense to do > > the hpwdt change as well. > > > > Spectre has been backported to v4.4 and later. I don't know about earlier > kernels. > > > Is just the patch removing the firmware call back wanted/needed? Or the > > whole driver rewrite? (The older baseline don't have all the watchdog > > features that the patch set uses.) > > > > We would only want to backport this patch. The rest is really out of scope. > > > Which stable baseline(s) would need to be patched? Priority? > > > > Who does it? (i.e. do you want me to submit patches to the stable > > baseline?) > > > We would tag the patch for stable (and submit it into v4.16-rc). Greg would > take care of the rest unless there are conflicts, in which case we get a note > telling us that a backport is needed. > Guenter, Are you waiting for anything more from me on this patch, or are we good for now until the back ports to v.15 etc.,? Thanks Jerry -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [PATCH v5 2/9] watchdog/hpwdt: Remove legacy NMI sourcing.
On Mon, Feb 26, 2018 at 06:32:30AM -0800, Guenter Roeck wrote: > On 02/26/2018 06:11 AM, Arnd Bergmann wrote: > > On Mon, Feb 26, 2018 at 4:22 AM, Jerry Hoemann > > wrote: > > > Gen8 and prior Proliant systems supported the "CRU" interface > > > to firmware. This interfaces allows linux to "call back" into firmware > > > to source the cause of an NMI. This feature isn't fully utilized > > > as the actual source of the NMI isn't printed, the driver only > > > indicates that the source couldn't be determined when the call > > > fails. > > > > > > With the advent of Gen9, iCRU replaces the CRU. The call back > > > feature is no longer available in firmware. To be compatible and > > > not attempt to call back into firmware on system not supporting CRU, > > > the SMBIOS table is consulted to determine if it is safe to > > > make the call back or not. > > > > > > This results in about half of the driver code being devoted > > > to either making CRU calls or determing if it is safe to make > > > CRU calls. As noted, the driver isn't really using the results of > > > the CRU calls. > > > > > > Furthermore, as a consequence of the Spectre security issue, the > > > BIOS/EFI calls are being wrapped into Spectre-disabling section. > > > Removing the call back in hpwdt_pretimeout assists in this effort. > > > > > > As the CRU sourcing of the NMI isn't required for handling the > > > NMI and there are security concerns with making the call back, remove > > > the legacy (pre Gen9) NMI sourcing and the DMI code to determine if > > > the system had the CRU interface. > > > > > > Signed-off-by: Jerry Hoemann > > > > This avoids a warning in mainline kernels, so that's great: > > > > drivers/watchdog/hpwdt.o: warning: objtool: .text+0x24: indirect call > > found in RETPOLINE build > > > > I wonder what we do about stable kernels. Are both this patch and the patch > > that added the objtool warning message candidates for backports to > > stable kernels? > > > > Makes sense to me, but it is really a bit more than a bug fix, so I'll > leave it up to Jerry/HPE to make the call in respect to hpwdt. > Generally speaking, HPE customers who run linux do so through a distro vendor and pick up patches from them. But I'm sure there are some customers who do things differently. The distro vendor's have their own repos and we'll work with them to back port patches to their code base. So, I typically don't do a lot of kernel.org stable branch work. Looks like objtool has been enhanced to find Spectre vulnerable code. Are the other kernel patches related to Spectre being back ported to stable release lines? If yes, it probably make sense to do the hpwdt change as well. Is just the patch removing the firmware call back wanted/needed? Or the whole driver rewrite? (The older baseline don't have all the watchdog features that the patch set uses.) Which stable baseline(s) would need to be patched? Priority? Who does it? (i.e. do you want me to submit patches to the stable baseline?) Thanks -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
[PATCH v5 5/9] watchdog/hpwdt: condition early return of NMI handler on iLO5
Modify prior change to not claim an NMI unless originated from iLO to apply only to iLO5 and later going forward. This restores hpwdt traditional behavior of calling panic if the NMI is NMI_IO_CHECK, NMI_SERR, or NMI_UNKNOWN for legacy hardware. Signed-off-by: Jerry Hoemann --- drivers/watchdog/hpwdt.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index 0e35bb735d8e..171d5033d7b5 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -32,6 +32,7 @@ #define HPWDT_MAX_TIMERTICKS_TO_SECS(65535) #define DEFAULT_MARGIN 30 +static bool ilo5; static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */ static unsigned int reload;/* the computed soft_margin */ static bool nowayout = WATCHDOG_NOWAYOUT; @@ -120,7 +121,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) "3. OA Forward Progress Log\n" "4. iLO Event Log"; - if ((ulReason == NMI_UNKNOWN) && mynmi) + if (ilo5 && ulReason == NMI_UNKNOWN && mynmi) return NMI_DONE; if (allow_kdump) @@ -277,6 +278,9 @@ static int hpwdt_init_one(struct pci_dev *dev, ", timer margin: %d seconds (nowayout=%d).\n", HPWDT_VERSION, hpwdt_dev.timeout, nowayout); + if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR) + ilo5 = true; + return 0; error_wd_register: -- 2.13.6
[PATCH v5 0/9] watchdog/hpwdt: Update driver to use watchdog core.
== v5 == Patch numbers changing due to squash and reorder. Using Prior (v4) Patch number noting *new* patch numbers as bullet item. Patch 0001 & 0004 Merged into: Update-Module-info-and-copyright 1) New Patch 0001 Patch 0002 Remove legacy NMI sourcing 1) Unmodified Patch 0003 Update nmi_panic message. 1) Unmodified Patch 0004 Update Module info. 1) Merged into 0001 above. Patch 0005 Modify to use watchdog core 1) syntatic sugar. 2) Now Patch 0004 Patch 0006 condition-early-return-of-NMI-handler 1) Now Patch 0005 2) Casing on iLO5 3) true for boolean Patch 0007 remove allow_kdump module param 1) Now patch 0006 Patch 0008 Programable Pretimeout NMI 1) Now patch 7 Patch 0009 Add dynamic debug 1) Now patch 0008 Patch 0010 Update driver version. 1) Now Patch 0009 == v4 == Modifications from prior version: Manual merging and patch reordering has cause minor white space and diff changes. Such change shouldn't have effected the logic and are omitted below: Patch 001 Remove legacy NMI sourcing. 1) Updated patch documentation to reflect Spectre concerns. 2) Restored comment at #endif >>>>>>>>> 3) New Patch 0002 <<<<<<<< Patch 002 watchdog/hpwdt: remove include files no longer 1) Squashed onto Patch001. >>>>>>>>> 3) Still Patch 0002 <<<<<<<< Patch 0003: watchdog/hpwdt: Update nmi_panic message. 1) Unchanged Patch 0004: watchdog/hpwdt: white space changes 1) Redacted. Patch 0005: watchdog/hpwdt: Update Module info. 1) Now Patch 0004 2) Update only module description to reflect branding changes. 3) Rest of patch redacted. Patch 0006 Select CORE 1) Merged with 0007 Patch 0007 Modify to use watchdog core 1) squashed on Patch 0006. 2) Now Patch 0005 3) Requested white space changes. 4) Use "wdd" for watchdog_device like other watchdog drivers do Patch 0008 Programable-Pretimeout 1) hpwdt_settimeout re-worked to take into account timeout <= pretimeout. Patterned after watchdog_set_timeout. 2) hpwdt_set_pretimeout re-worked to take into account requested pretimeout <= timeout. 3) Clarify patch doc to reflect above. Patch 0009 condition-early-return-of-NMI 1) Requested syntatic surgar. 2) Now Patch 0006 Patch 0010 remove-allow_kdump 1) Now Patch 0007 Patch 0011 Add-dynamic-debug 1) Now Patch 0009 2) Typo fix 3) New dev_dbg resulting from changes to set timeout and pretimeout. Patch 0012 Update driver version. 1) Now Patch 0010 New Patch 1) Patch 0001 Update Copyright == v3 == Incorperating code review feedback. 1) Patch 0003: Use existing hex_byte_pack instead of creating new function. 2) Patch 0005: Redacted change in module_param permission. 3) Patch 0006: switch from pr_debug etc., to dev_dbg where possible. 4) Patch 0006: No longer updating soft_margin post module load. 5) Patch 0006: Initialize hpwdt_dev.parent before registering watchdog. 6) Patch 0006: Redacted change to dev_info message w.r.t. allow_kdump 7) Patch 0006 & 0007: Reorder patches to maintain bisectability. 8) Patch 0008: Change pr_debug to dev_dbg 9) Patch 0010: Change dev_info message w.r.t. allow_kdump where feature is removed. Note, I am explicitly ignoring the checkpatch error on Patch 0008 for specifying permisson of "0" instead of "". == v2 == 1) Fix compiler error when CONFIG_HPWDT_NMI_DECODING is not defined. 2) Break out driver version change to its own patch (0011). == v1 == The primary purposes of this patch set are to 1) Update the hpwdt driver to use the watchdog core. 2) Reduce complexity by removing unnecessary features. 3) Add customer requested features like optional pretimeout. 4) Enhance readability/maintainability of the driver. The size of the resultant driver is reduced from over 900 lines to 350 lines. Patch 1& 2 remove legacy NMI sourcing. Patch 3 adds useful indication of NMI cause to panic message Patch 4 & 5 are general cleanup Patch 6 & 7 updates the driver to user the watchdog core. Patch 8 makes the pretimeout NMI programmable. Patch 9 modifies whether the NMI handler claims the NMI. Patch 10 retracts the allow_kdump module parameter. Jerry Hoemann (9): watchdog/hpwdt: Update Module info and copyright. watchdog/hpwdt: Remove legacy NMI sourcing. watchdog/hpwdt: Update nmi_panic message. watchdog/hpwdt: Modify to use watchdog core. watchdog/hpwdt: condition early return of NMI handler on iLO5 watchdog/hpwdt: remove allow_kdump module parameter. watchdog/hpwdt: Programable Pretimeout NMI watchdog/hpwdt: Add dynamic debug watchdog/hpwdt: Update driver version. drivers/watchdog/Kconfig | 1 + drivers/watchdog/hpwdt.c | 791 +++ 2 files changed, 121 insertions(+), 671 deletions(-) -- 2.13.6
[PATCH v5 2/9] watchdog/hpwdt: Remove legacy NMI sourcing.
Gen8 and prior Proliant systems supported the "CRU" interface
to firmware. This interfaces allows linux to "call back" into firmware
to source the cause of an NMI. This feature isn't fully utilized
as the actual source of the NMI isn't printed, the driver only
indicates that the source couldn't be determined when the call
fails.
With the advent of Gen9, iCRU replaces the CRU. The call back
feature is no longer available in firmware. To be compatible and
not attempt to call back into firmware on system not supporting CRU,
the SMBIOS table is consulted to determine if it is safe to
make the call back or not.
This results in about half of the driver code being devoted
to either making CRU calls or determing if it is safe to make
CRU calls. As noted, the driver isn't really using the results of
the CRU calls.
Furthermore, as a consequence of the Spectre security issue, the
BIOS/EFI calls are being wrapped into Spectre-disabling section.
Removing the call back in hpwdt_pretimeout assists in this effort.
As the CRU sourcing of the NMI isn't required for handling the
NMI and there are security concerns with making the call back, remove
the legacy (pre Gen9) NMI sourcing and the DMI code to determine if
the system had the CRU interface.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 501 +--
1 file changed, 9 insertions(+), 492 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 542dfc598716..1d20bc6d2c44 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -28,16 +28,7 @@
#include
#include
#include
-#ifdef CONFIG_HPWDT_NMI_DECODING
-#include
-#include
-#include
-#include
-#include
-#include
-#endif /* CONFIG_HPWDT_NMI_DECODING */
#include
-#include
#define HPWDT_VERSION "1.4.0"
#define SECS_TO_TICKS(secs)((secs) * 1000 / 128)
@@ -48,6 +39,9 @@
static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */
static unsigned int reload;/* the computed soft_margin */
static bool nowayout = WATCHDOG_NOWAYOUT;
+#ifdef CONFIG_HPWDT_NMI_DECODING
+static unsigned int allow_kdump = 1;
+#endif
static char expect_release;
static unsigned long hpwdt_is_open;
@@ -63,373 +57,6 @@ static const struct pci_device_id hpwdt_devices[] = {
};
MODULE_DEVICE_TABLE(pci, hpwdt_devices);
-#ifdef CONFIG_HPWDT_NMI_DECODING
-#define PCI_BIOS32_SD_VALUE0x5F32335F /* "_32_" */
-#define CRU_BIOS_SIGNATURE_VALUE 0x55524324
-#define PCI_BIOS32_PARAGRAPH_LEN 16
-#define PCI_ROM_BASE1 0x000F
-#define ROM_SIZE 0x1
-
-struct bios32_service_dir {
- u32 signature;
- u32 entry_point;
- u8 revision;
- u8 length;
- u8 checksum;
- u8 reserved[5];
-};
-
-/* type 212 */
-struct smbios_cru64_info {
- u8 type;
- u8 byte_length;
- u16 handle;
- u32 signature;
- u64 physical_address;
- u32 double_length;
- u32 double_offset;
-};
-#define SMBIOS_CRU64_INFORMATION 212
-
-/* type 219 */
-struct smbios_proliant_info {
- u8 type;
- u8 byte_length;
- u16 handle;
- u32 power_features;
- u32 omega_features;
- u32 reserved;
- u32 misc_features;
-};
-#define SMBIOS_ICRU_INFORMATION219
-
-
-struct cmn_registers {
- union {
- struct {
- u8 ral;
- u8 rah;
- u16 rea2;
- };
- u32 reax;
- } u1;
- union {
- struct {
- u8 rbl;
- u8 rbh;
- u8 reb2l;
- u8 reb2h;
- };
- u32 rebx;
- } u2;
- union {
- struct {
- u8 rcl;
- u8 rch;
- u16 rec2;
- };
- u32 recx;
- } u3;
- union {
- struct {
- u8 rdl;
- u8 rdh;
- u16 red2;
- };
- u32 redx;
- } u4;
-
- u32 resi;
- u32 redi;
- u16 rds;
- u16 res;
- u32 reflags;
-} __attribute__((packed));
-
-static unsigned int hpwdt_nmi_decoding;
-static unsigned int allow_kdump = 1;
-static unsigned int is_icru;
-static unsigned int is_uefi;
-static DEFINE_SPINLOCK(rom_lock);
-static void *cru_rom_addr;
-static struct cmn_registers cmn_regs;
-
-extern asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
- unsigned long *pRomEntry);
-
-#ifdef CONFIG_X86_32
-/* --32 Bit Bios */
-
-#define HPWDT_ARCH 32
-
-asm(".text
[PATCH v5 8/9] watchdog/hpwdt: Add dynamic debug
Add a few dynamic debug messages to aid in module level debug.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index b8205c6e61c1..b82bbeed0e43 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -59,6 +59,7 @@ static int hpwdt_start(struct watchdog_device *wdd)
int control = 0x81 | (pretimeout ? 0x4 : 0);
int reload = SECS_TO_TICKS(wdd->timeout);
+ dev_dbg(wdd->parent, "start watchdog 0x%08x:0x%02x\n", reload, control);
iowrite16(reload, hpwdt_timer_reg);
iowrite8(control, hpwdt_timer_con);
@@ -69,6 +70,8 @@ static void hpwdt_stop(void)
{
unsigned long data;
+ pr_debug("stop watchdog\n");
+
data = ioread8(hpwdt_timer_con);
data &= 0xFE;
iowrite8(data, hpwdt_timer_con);
@@ -85,6 +88,7 @@ static int hpwdt_ping(struct watchdog_device *wdd)
{
int reload = SECS_TO_TICKS(wdd->timeout);
+ dev_dbg(wdd->parent, "ping watchdog 0x%08x\n", reload);
iowrite16(reload, hpwdt_timer_reg);
return 0;
@@ -97,8 +101,11 @@ static unsigned int hpwdt_gettimeleft(struct
watchdog_device *wdd)
static int hpwdt_settimeout(struct watchdog_device *wdd, unsigned int val)
{
+ dev_dbg(wdd->parent, "set_timeout = %d\n", val);
+
wdd->timeout = val;
if (val <= wdd->pretimeout) {
+ dev_dbg(wdd->parent, "pretimeout < timeout. Setting to zero\n");
wdd->pretimeout = 0;
pretimeout = 0;
if (watchdog_active(wdd))
@@ -114,12 +121,16 @@ static int hpwdt_set_pretimeout(struct watchdog_device
*wdd, unsigned int req)
{
unsigned int val = 0;
+ dev_dbg(wdd->parent, "set_pretimeout = %d\n", req);
if (req) {
val = PRETIMEOUT_SEC;
if (val >= wdd->timeout)
return -EINVAL;
}
+ if (val != req)
+ dev_dbg(wdd->parent, "Rounding pretimeout to: %d\n", val);
+
wdd->pretimeout = val;
pretimeout = !!val;
--
2.13.6
[PATCH v5 6/9] watchdog/hpwdt: remove allow_kdump module parameter.
The intent of this parameter is unclear and it sets up a race between the reset of the system by ASR and crashdump. The length of time between receipt of the pretimeout NMI and the ASR reset of the system is fixed by hardware. Turning the parameter off doesn't necessairly prevent a crash dump. Also, having the ASR reset occur while the system is crash dumping doesn't imply that the dump was hung given the short duration between the NMI and the reset. This parameter is not a substitute for having a architected watchdog crashdump hang detection paridigm. Signed-off-by: Jerry Hoemann --- drivers/watchdog/hpwdt.c | 16 +++- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index 171d5033d7b5..68e84a212d00 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -36,9 +36,6 @@ static bool ilo5; static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */ static unsigned int reload;/* the computed soft_margin */ static bool nowayout = WATCHDOG_NOWAYOUT; -#ifdef CONFIG_HPWDT_NMI_DECODING -static unsigned int allow_kdump = 1; -#endif static void __iomem *pci_mem_addr; /* the PCI-memory address */ static unsigned long __iomem *hpwdt_nmistat; @@ -124,8 +121,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) if (ilo5 && ulReason == NMI_UNKNOWN && mynmi) return NMI_DONE; - if (allow_kdump) - hpwdt_stop(); + hpwdt_stop(); hex_byte_pack(panic_msg, mynmi); nmi_panic(regs, panic_msg); @@ -186,9 +182,8 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev) goto error2; dev_info(&dev->dev, - "HPE Watchdog Timer Driver: NMI decoding initialized" - ", allow kernel dump: %s (default = 1/ON)\n", - (allow_kdump == 0) ? "OFF" : "ON"); + "HPE Watchdog Timer Driver: NMI decoding initialized\n"); + return 0; error2: @@ -322,9 +317,4 @@ module_param(nowayout, bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); -#ifdef CONFIG_HPWDT_NMI_DECODING -module_param(allow_kdump, int, 0); -MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs"); -#endif /* CONFIG_HPWDT_NMI_DECODING */ - module_pci_driver(hpwdt_driver); -- 2.13.6
[PATCH v5 3/9] watchdog/hpwdt: Update nmi_panic message.
Include the nmistat in the nmi_panic message to give support
an indication why the NMI was called (e.g. a timeout or generate
nmi button.)
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 1d20bc6d2c44..44c3038cc531 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -113,19 +113,23 @@ static int hpwdt_my_nmi(void)
*/
static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
{
- if ((ulReason == NMI_UNKNOWN) && !hpwdt_my_nmi())
+ unsigned int mynmi = hpwdt_my_nmi();
+ static char panic_msg[] =
+ "00: An NMI occurred. Depending on your system the reason "
+ "for the NMI is logged in any one of the following resources:\n"
+ "1. Integrated Management Log (IML)\n"
+ "2. OA Syslog\n"
+ "3. OA Forward Progress Log\n"
+ "4. iLO Event Log";
+
+ if ((ulReason == NMI_UNKNOWN) && mynmi)
return NMI_DONE;
if (allow_kdump)
hpwdt_stop();
- nmi_panic(regs, "An NMI occurred. Depending on your system the reason "
- "for the NMI is logged in any one of the following "
- "resources:\n"
- "1. Integrated Management Log (IML)\n"
- "2. OA Syslog\n"
- "3. OA Forward Progress Log\n"
- "4. iLO Event Log");
+ hex_byte_pack(panic_msg, mynmi);
+ nmi_panic(regs, panic_msg);
return NMI_HANDLED;
}
--
2.13.6
[PATCH v5 1/9] watchdog/hpwdt: Update Module info and copyright.
Update Copyright and Module description to reflect branding changes.
Signed-off-by: Jerry Hoemann
---
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 f1f00dfc0e68..542dfc598716 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -4,7 +4,7 @@
*
* SoftDog 0.05: A Software Watchdog Device
*
- * (c) Copyright 2015 Hewlett Packard Enterprise Development LP
+ * (c) Copyright 2018 Hewlett Packard Enterprise Development LP
* Thomas Mingarelli
*
* This program is free software; you can redistribute it and/or
@@ -908,7 +908,7 @@ static struct pci_driver hpwdt_driver = {
};
MODULE_AUTHOR("Tom Mingarelli");
-MODULE_DESCRIPTION("hp watchdog driver");
+MODULE_DESCRIPTION("hpe watchdog driver");
MODULE_LICENSE("GPL");
MODULE_VERSION(HPWDT_VERSION);
--
2.13.6
[PATCH v5 9/9] watchdog/hpwdt: Update driver version.
Update driver version number to reflect changes. Signed-off-by: Jerry Hoemann --- 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 b82bbeed0e43..a43ab2cecca2 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -26,7 +26,7 @@ #include #include -#define HPWDT_VERSION "1.4.0" +#define HPWDT_VERSION "2.0.0" #define SECS_TO_TICKS(secs)((secs) * 1000 / 128) #define TICKS_TO_SECS(ticks) ((ticks) * 128 / 1000) #define HPWDT_MAX_TIMERTICKS_TO_SECS(65535) -- 2.13.6
[PATCH v5 7/9] watchdog/hpwdt: Programable Pretimeout NMI
Make whether or not the hpwdt watchdog delivers a pretimeout NMI
programable by the user.
The underlying iLO hardware is programmable as to whether or not
a pre-timeout NMI is delivered to the system before the iLO resets
the system. However, the iLO does not allow for programming the
length of time that NMI is delivered before the system is reset.
By watchdog API, in hpwdt_set_pretimeout a val == 0 disables the NMI.
When val != 0, hpwdt_set_pretimeout will enable the pretimeout NMI
provided the current timeout is greator than the HW specified
pretimeout length. Otherwise an error is returned.
In set_timeout, if the new timeout is <= an already established pretimeout,
the pretimeout is canceled. This matches the action watchdog_set_timeout
in the watchdog core would do if an hpwdt specific set_timeout
function wasn't specified.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 53
1 file changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 68e84a212d00..b8205c6e61c1 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -31,11 +31,12 @@
#define TICKS_TO_SECS(ticks) ((ticks) * 128 / 1000)
#define HPWDT_MAX_TIMERTICKS_TO_SECS(65535)
#define DEFAULT_MARGIN 30
+#define PRETIMEOUT_SEC 9
static bool ilo5;
static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */
-static unsigned int reload;/* the computed soft_margin */
static bool nowayout = WATCHDOG_NOWAYOUT;
+static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);
static void __iomem *pci_mem_addr; /* the PCI-memory address */
static unsigned long __iomem *hpwdt_nmistat;
@@ -55,10 +56,11 @@ MODULE_DEVICE_TABLE(pci, hpwdt_devices);
*/
static int hpwdt_start(struct watchdog_device *wdd)
{
- reload = SECS_TO_TICKS(wdd->timeout);
+ int control = 0x81 | (pretimeout ? 0x4 : 0);
+ int reload = SECS_TO_TICKS(wdd->timeout);
iowrite16(reload, hpwdt_timer_reg);
- iowrite8(0x85, hpwdt_timer_con);
+ iowrite8(control, hpwdt_timer_con);
return 0;
}
@@ -81,7 +83,10 @@ static int hpwdt_stop_core(struct watchdog_device *wdd)
static int hpwdt_ping(struct watchdog_device *wdd)
{
+ int reload = SECS_TO_TICKS(wdd->timeout);
+
iowrite16(reload, hpwdt_timer_reg);
+
return 0;
}
@@ -93,12 +98,37 @@ static unsigned int hpwdt_gettimeleft(struct
watchdog_device *wdd)
static int hpwdt_settimeout(struct watchdog_device *wdd, unsigned int val)
{
wdd->timeout = val;
+ if (val <= wdd->pretimeout) {
+ wdd->pretimeout = 0;
+ pretimeout = 0;
+ if (watchdog_active(wdd))
+ hpwdt_start(wdd);
+ }
hpwdt_ping(wdd);
return 0;
}
#ifdef CONFIG_HPWDT_NMI_DECODING
+static int hpwdt_set_pretimeout(struct watchdog_device *wdd, unsigned int req)
+{
+ unsigned int val = 0;
+
+ if (req) {
+ val = PRETIMEOUT_SEC;
+ if (val >= wdd->timeout)
+ return -EINVAL;
+ }
+
+ wdd->pretimeout = val;
+ pretimeout = !!val;
+
+ if (watchdog_active(wdd))
+ hpwdt_start(wdd);
+
+ return 0;
+}
+
static int hpwdt_my_nmi(void)
{
return ioread8(hpwdt_nmistat) & 0x6;
@@ -121,6 +151,9 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct
pt_regs *regs)
if (ilo5 && ulReason == NMI_UNKNOWN && mynmi)
return NMI_DONE;
+ if (ilo5 && !pretimeout)
+ return NMI_DONE;
+
hpwdt_stop();
hex_byte_pack(panic_msg, mynmi);
@@ -132,7 +165,8 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct
pt_regs *regs)
static const struct watchdog_info ident = {
- .options = WDIOF_SETTIMEOUT |
+ .options = WDIOF_PRETIMEOUT|
+ WDIOF_SETTIMEOUT|
WDIOF_KEEPALIVEPING |
WDIOF_MAGICCLOSE,
.identity = "HPE iLO2+ HW Watchdog Timer",
@@ -149,6 +183,9 @@ static const struct watchdog_ops hpwdt_ops = {
.ping = hpwdt_ping,
.set_timeout= hpwdt_settimeout,
.get_timeleft = hpwdt_gettimeleft,
+#ifdef CONFIG_HPWDT_NMI_DECODING
+ .set_pretimeout = hpwdt_set_pretimeout,
+#endif
};
static struct watchdog_device hpwdt_dev = {
@@ -157,6 +194,9 @@ static struct watchdog_device hpwdt_dev = {
.min_timeout= 1,
.max_timeout= HPWDT_MAX_TIMER,
.timeout= DEFAULT_MARGIN,
+#ifdef CONFIG_HPWDT_NMI_DECODING
+ .pretimeout = PRETIMEOUT_SEC,
+#endif
};
@@ -317,4 +357,9 @@ module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once star
[PATCH v5 4/9] watchdog/hpwdt: Modify to use watchdog core.
Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
convert hpwdt from legacy watchdog driver to use the watchdog core.
Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
Added functions: hpwdt_settimeout
Added structures: watchdog_device
Update Kconfig file to show that hpwdt now selects WATCHDOG_CORE.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/hpwdt.c | 216 +++
2 files changed, 48 insertions(+), 169 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 37460cd6cabb..e873522fca2d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1118,6 +1118,7 @@ config IT87_WDT
config HP_WATCHDOG
tristate "HP ProLiant iLO2+ Hardware Watchdog Timer"
+ select WATCHDOG_CORE
depends on X86 && PCI
help
A software monitoring watchdog and NMI sourcing driver. This driver
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 44c3038cc531..0e35bb735d8e 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -16,17 +16,13 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include
-#include
#include
-#include
#include
-#include
#include
#include
#include
#include
#include
-#include
#include
#include
@@ -42,8 +38,6 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
#ifdef CONFIG_HPWDT_NMI_DECODING
static unsigned int allow_kdump = 1;
#endif
-static char expect_release;
-static unsigned long hpwdt_is_open;
static void __iomem *pci_mem_addr; /* the PCI-memory address */
static unsigned long __iomem *hpwdt_nmistat;
@@ -61,11 +55,14 @@ MODULE_DEVICE_TABLE(pci, hpwdt_devices);
/*
* Watchdog operations
*/
-static void hpwdt_start(void)
+static int hpwdt_start(struct watchdog_device *wdd)
{
- reload = SECS_TO_TICKS(soft_margin);
+ reload = SECS_TO_TICKS(wdd->timeout);
+
iowrite16(reload, hpwdt_timer_reg);
iowrite8(0x85, hpwdt_timer_con);
+
+ return 0;
}
static void hpwdt_stop(void)
@@ -77,31 +74,32 @@ static void hpwdt_stop(void)
iowrite8(data, hpwdt_timer_con);
}
-static void hpwdt_ping(void)
+static int hpwdt_stop_core(struct watchdog_device *wdd)
{
- iowrite16(reload, hpwdt_timer_reg);
+ hpwdt_stop();
+
+ return 0;
}
-static int hpwdt_change_timer(int new_margin)
+static int hpwdt_ping(struct watchdog_device *wdd)
{
- if (new_margin < 1 || new_margin > HPWDT_MAX_TIMER) {
- pr_warn("New value passed in is invalid: %d seconds\n",
- new_margin);
- return -EINVAL;
- }
-
- soft_margin = new_margin;
- pr_debug("New timer passed in is %d seconds\n", new_margin);
- reload = SECS_TO_TICKS(soft_margin);
-
+ iowrite16(reload, hpwdt_timer_reg);
return 0;
}
-static int hpwdt_time_left(void)
+static unsigned int hpwdt_gettimeleft(struct watchdog_device *wdd)
{
return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
}
+static int hpwdt_settimeout(struct watchdog_device *wdd, unsigned int val)
+{
+ wdd->timeout = val;
+ hpwdt_ping(wdd);
+
+ return 0;
+}
+
#ifdef CONFIG_HPWDT_NMI_DECODING
static int hpwdt_my_nmi(void)
{
@@ -135,68 +133,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct
pt_regs *regs)
}
#endif /* CONFIG_HPWDT_NMI_DECODING */
-/*
- * /dev/watchdog handling
- */
-static int hpwdt_open(struct inode *inode, struct file *file)
-{
- /* /dev/watchdog can only be opened once */
- if (test_and_set_bit(0, &hpwdt_is_open))
- return -EBUSY;
-
- /* Start the watchdog */
- hpwdt_start();
- hpwdt_ping();
-
- return nonseekable_open(inode, file);
-}
-
-static int hpwdt_release(struct inode *inode, struct file *file)
-{
- /* Stop the watchdog */
- if (expect_release == 42) {
- hpwdt_stop();
- } else {
- pr_crit("Unexpected close, not stopping watchdog!\n");
- hpwdt_ping();
- }
-
- expect_release = 0;
-
- /* /dev/watchdog is being closed, make sure it can be re-opened */
- clear_bit(0, &hpwdt_is_open);
-
- return 0;
-}
-
-static ssize_t hpwdt_write(struct file *file, const char __user *data,
- size_t len, loff_t *ppos)
-{
- /* See if we got the magic character 'V' and reload the timer */
- if (len) {
- if (!nowayout) {
- size_t i;
-
- /* note: just in case someone wrote the magic character
-* five months ago... */
- expect_release = 0;
-
-
[PATCH v4 04/10] watchdog/hpwdt: Update Module info.
Update Module description to reflect branding changes.
Signed-off-by: Jerry Hoemann
---
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 2a6f3be22c98..44c3038cc531 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -429,7 +429,7 @@ static struct pci_driver hpwdt_driver = {
};
MODULE_AUTHOR("Tom Mingarelli");
-MODULE_DESCRIPTION("hp watchdog driver");
+MODULE_DESCRIPTION("hpe watchdog driver");
MODULE_LICENSE("GPL");
MODULE_VERSION(HPWDT_VERSION);
--
2.13.6
[PATCH v4 03/10] watchdog/hpwdt: Update nmi_panic message.
Include the nmistat in the nmi_panic message to give support
an indication why the NMI was called (e.g. a timeout or generate
nmi button.)
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 8ea8e7466dd9..2a6f3be22c98 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -113,19 +113,23 @@ static int hpwdt_my_nmi(void)
*/
static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
{
- if ((ulReason == NMI_UNKNOWN) && !hpwdt_my_nmi())
+ unsigned int mynmi = hpwdt_my_nmi();
+ static char panic_msg[] =
+ "00: An NMI occurred. Depending on your system the reason "
+ "for the NMI is logged in any one of the following resources:\n"
+ "1. Integrated Management Log (IML)\n"
+ "2. OA Syslog\n"
+ "3. OA Forward Progress Log\n"
+ "4. iLO Event Log";
+
+ if ((ulReason == NMI_UNKNOWN) && mynmi)
return NMI_DONE;
if (allow_kdump)
hpwdt_stop();
- nmi_panic(regs, "An NMI occurred. Depending on your system the reason "
- "for the NMI is logged in any one of the following "
- "resources:\n"
- "1. Integrated Management Log (IML)\n"
- "2. OA Syslog\n"
- "3. OA Forward Progress Log\n"
- "4. iLO Event Log");
+ hex_byte_pack(panic_msg, mynmi);
+ nmi_panic(regs, panic_msg);
return NMI_HANDLED;
}
--
2.13.6
[PATCH v4 00/10] watchdog/hpwdt: Update driver to use watchdog core.
== v4 == Modifications from prior version: Manual merging and patch reordering has caused minor white space and diff changes. Such change shouldn't have effected the logic and are omitted below: Patch 001 Remove legacy NMI sourcing. 1) Updated patch documentation to reflect Spectre concerns. 2) Restored comment at #endif Patch 002 watchdog/hpwdt: remove include files no longer 1) Squashed onto Patch001. Patch 0003: watchdog/hpwdt: Update nmi_panic message. 1) Unchanged Patch 0004: watchdog/hpwdt: white space changes 1) Redacted. Patch 0005: watchdog/hpwdt: Update Module info. 1) Now Patch 0004 2) Update only module description to reflect branding changes. 3) Rest of patch redacted. Patch 0006 Select CORE 1) Merged with 0007 Patch 0007 Modify to use watchdog core 1) squashed on Patch 0006. 2) Now Patch 0005 3) Requested white space changes. 4) Use "wdd" for watchdog_device like other watchdog drivers do Patch 0008 Programable-Pretimeout 1) hpwdt_settimeout re-worked to take into account timeout <= pretimeout. Patterned after watchdog_set_timeout. 2) hpwdt_set_pretimeout re-worked to take into account requested pretimeout <= timeout. 3) Clarify patch doc to reflect above. Patch 0009 condition-early-return-of-NMI 1) Requested syntatic surgar. 2) Now Patch 0006 Patch 0010 remove-allow_kdump 1) Now Patch 0007 Patch 0011 Add-dynamic-debug 1) Now Patch 0009 2) Typo fix 3) New dev_dbg resulting from changes to set timeout and pretimeout. Patch 0012 Update driver version. 1) Now Patch 0010 New Patch 1) Patch 0001 Update Copyright == v3 == Incorperating code review feedback. 1) Patch 0003: Use existing hex_byte_pack instead of creating new function. 2) Patch 0005: Redacted change in module_param permission. 3) Patch 0006: switch from pr_debug etc., to dev_dbg where possible. 4) Patch 0006: No longer updating soft_margin post module load. 5) Patch 0006: Initialize hpwdt_dev.parent before registering watchdog. 6) Patch 0006: Redacted change to dev_info message w.r.t. allow_kdump 7) Patch 0006 & 0007: Reorder patches to maintain bisectability. 8) Patch 0008: Change pr_debug to dev_dbg 9) Patch 0010: Change dev_info message w.r.t. allow_kdump where feature is removed. Note, I am explicitly ignoring the checkpatch error on Patch 0008 for specifying permisson of "0" instead of "". == v2 == 1) Fix compiler error when CONFIG_HPWDT_NMI_DECODING is not defined. 2) Break out driver version change to its own patch (0011). == v1 == The primary purposes of this patch set are to 1) Update the hpwdt driver to use the watchdog core. 2) Reduce complexity by removing unnecessary features. 3) Add customer requested features like optional pretimeout. 4) Enhance readability/maintainability of the driver. The size of the resultant driver is reduced from over 900 lines to 350 lines. Patch 1& 2 remove legacy NMI sourcing. Patch 3 adds useful indication of NMI cause to panic message Patch 4 & 5 are general cleanup Patch 6 & 7 updates the driver to user the watchdog core. Patch 8 makes the pretimeout NMI programmable. Patch 9 modifies whether the NMI handler claims the NMI. Patch 10 retracts the allow_kdump module parameter. Jerry Hoemann (10): watchdog/hpwdt: Update copyright. watchdog/hpwdt: Remove legacy NMI sourcing. watchdog/hpwdt: Update nmi_panic message. watchdog/hpwdt: Update Module info. watchdog/hpwdt: Modify to use watchdog core. watchdog/hpwdt: condition early return of NMI handler on iLO5 watchdog/hpwdt: remove allow_kdump module parameter. watchdog/hpwdt: Programable Pretimeout NMI watchdog/hpwdt: Add dynamic debug watchdog/hpwdt: Update driver version. drivers/watchdog/Kconfig | 1 + drivers/watchdog/hpwdt.c | 789 +++ 2 files changed, 121 insertions(+), 669 deletions(-) -- 2.13.6
[PATCH v4 02/10] watchdog/hpwdt: Remove legacy NMI sourcing.
Gen8 and prior Proliant systems supported the "CRU" interface
to firmware. This interfaces allows linux to "call back" into firmware
to source the cause of an NMI. This feature isn't fully utilized
as the actual source of the NMI isn't printed, the driver only
indicates that the source couldn't be determined when the call
fails.
With the advent of Gen9, iCRU replaces the CRU. The call back
feature is no longer available in firmware. To be compatible and
not attempt to call back into firmware on system not supporting CRU,
the SMBIOS table is consulted to determine if it is safe to
make the call back or not.
This results in about half of the driver code being devoted
to either making CRU calls or determing if it is safe to make
CRU calls. As noted, the driver isn't really using the results of
the CRU calls.
Furthermore, as a consequence of the Spectre security issue, the
BIOS/EFI calls are being wrapped into Spectre-disabling section.
Removing the call back in hpwdt_pretimeout assists in this effort.
As the CRU sourcing of the NMI isn't required for handling the
NMI and there are security concerns with making the call back, remove
the legacy (pre Gen9) NMI sourcing and the DMI code to determine if
the system had the CRU interface.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 501 +--
1 file changed, 9 insertions(+), 492 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 3d435d6d3226..8ea8e7466dd9 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -28,16 +28,7 @@
#include
#include
#include
-#ifdef CONFIG_HPWDT_NMI_DECODING
-#include
-#include
-#include
-#include
-#include
-#include
-#endif /* CONFIG_HPWDT_NMI_DECODING */
#include
-#include
#define HPWDT_VERSION "1.4.0"
#define SECS_TO_TICKS(secs)((secs) * 1000 / 128)
@@ -48,6 +39,9 @@
static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */
static unsigned int reload;/* the computed soft_margin */
static bool nowayout = WATCHDOG_NOWAYOUT;
+#ifdef CONFIG_HPWDT_NMI_DECODING
+static unsigned int allow_kdump = 1;
+#endif
static char expect_release;
static unsigned long hpwdt_is_open;
@@ -63,373 +57,6 @@ static const struct pci_device_id hpwdt_devices[] = {
};
MODULE_DEVICE_TABLE(pci, hpwdt_devices);
-#ifdef CONFIG_HPWDT_NMI_DECODING
-#define PCI_BIOS32_SD_VALUE0x5F32335F /* "_32_" */
-#define CRU_BIOS_SIGNATURE_VALUE 0x55524324
-#define PCI_BIOS32_PARAGRAPH_LEN 16
-#define PCI_ROM_BASE1 0x000F
-#define ROM_SIZE 0x1
-
-struct bios32_service_dir {
- u32 signature;
- u32 entry_point;
- u8 revision;
- u8 length;
- u8 checksum;
- u8 reserved[5];
-};
-
-/* type 212 */
-struct smbios_cru64_info {
- u8 type;
- u8 byte_length;
- u16 handle;
- u32 signature;
- u64 physical_address;
- u32 double_length;
- u32 double_offset;
-};
-#define SMBIOS_CRU64_INFORMATION 212
-
-/* type 219 */
-struct smbios_proliant_info {
- u8 type;
- u8 byte_length;
- u16 handle;
- u32 power_features;
- u32 omega_features;
- u32 reserved;
- u32 misc_features;
-};
-#define SMBIOS_ICRU_INFORMATION219
-
-
-struct cmn_registers {
- union {
- struct {
- u8 ral;
- u8 rah;
- u16 rea2;
- };
- u32 reax;
- } u1;
- union {
- struct {
- u8 rbl;
- u8 rbh;
- u8 reb2l;
- u8 reb2h;
- };
- u32 rebx;
- } u2;
- union {
- struct {
- u8 rcl;
- u8 rch;
- u16 rec2;
- };
- u32 recx;
- } u3;
- union {
- struct {
- u8 rdl;
- u8 rdh;
- u16 red2;
- };
- u32 redx;
- } u4;
-
- u32 resi;
- u32 redi;
- u16 rds;
- u16 res;
- u32 reflags;
-} __attribute__((packed));
-
-static unsigned int hpwdt_nmi_decoding;
-static unsigned int allow_kdump = 1;
-static unsigned int is_icru;
-static unsigned int is_uefi;
-static DEFINE_SPINLOCK(rom_lock);
-static void *cru_rom_addr;
-static struct cmn_registers cmn_regs;
-
-extern asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
- unsigned long *pRomEntry);
-
-#ifdef CONFIG_X86_32
-/* --32 Bit Bios */
-
-#define HPWDT_ARCH 32
-
-asm(".text
[PATCH v4 09/10] watchdog/hpwdt: Add dynamic debug
Add a few dynamic debug messages to aid in module level debug.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index be7b3edac56c..b5fb2663c2a1 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -59,6 +59,7 @@ static int hpwdt_start(struct watchdog_device *wdd)
int control = 0x81 | (pretimeout ? 0x4 : 0);
int reload = SECS_TO_TICKS(wdd->timeout);
+ dev_dbg(wdd->parent, "start watchdog 0x%08x:0x%02x\n", reload, control);
iowrite16(reload, hpwdt_timer_reg);
iowrite8(control, hpwdt_timer_con);
@@ -69,6 +70,8 @@ static void hpwdt_stop(void)
{
unsigned long data;
+ pr_debug("stop watchdog\n");
+
data = ioread8(hpwdt_timer_con);
data &= 0xFE;
iowrite8(data, hpwdt_timer_con);
@@ -85,6 +88,7 @@ static int hpwdt_ping(struct watchdog_device *wdd)
{
int reload = SECS_TO_TICKS(wdd->timeout);
+ dev_dbg(wdd->parent, "ping watchdog 0x%08x\n", reload);
iowrite16(reload, hpwdt_timer_reg);
return 0;
@@ -98,8 +102,11 @@ static unsigned int hpwdt_gettimeleft(struct
watchdog_device *wdd)
static int hpwdt_settimeout(struct watchdog_device *wdd, unsigned int val)
{
+ dev_dbg(wdd->parent, "set_timeout = %d\n", val);
+
wdd->timeout = val;
if (val <= wdd->pretimeout) {
+ dev_dbg(wdd->parent, "pretimeout < timeout. Setting to zero\n");
wdd->pretimeout = 0;
pretimeout = 0;
if (watchdog_active(wdd))
@@ -115,12 +122,16 @@ static int hpwdt_set_pretimeout(struct watchdog_device
*wdd, unsigned int req)
{
unsigned int val = 0;
+ dev_dbg(wdd->parent, "set_pretimeout = %d\n", req);
if (req) {
val = PRETIMEOUT_SEC;
if (val >= wdd->timeout)
return -EINVAL;
}
+ if (val != req)
+ dev_dbg(wdd->parent, "Rounding pretimeout to: %d\n", val);
+
wdd->pretimeout = val;
pretimeout = !!val;
--
2.13.6
[PATCH v4 05/10] watchdog/hpwdt: Modify to use watchdog core.
Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
convert hpwdt from legacy watchdog driver to use the watchdog core.
Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
Added functions: hpwdt_settimeout
Added structures: watchdog_device
Update Kconfig file to show that hpwdt now selects WATCHDOG_CORE.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/hpwdt.c | 216 +++
2 files changed, 49 insertions(+), 168 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 37460cd6cabb..e873522fca2d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1118,6 +1118,7 @@ config IT87_WDT
config HP_WATCHDOG
tristate "HP ProLiant iLO2+ Hardware Watchdog Timer"
+ select WATCHDOG_CORE
depends on X86 && PCI
help
A software monitoring watchdog and NMI sourcing driver. This driver
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 44c3038cc531..1a18326bc99d 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -16,17 +16,13 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include
-#include
#include
-#include
#include
-#include
#include
#include
#include
#include
#include
-#include
#include
#include
@@ -42,8 +38,6 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
#ifdef CONFIG_HPWDT_NMI_DECODING
static unsigned int allow_kdump = 1;
#endif
-static char expect_release;
-static unsigned long hpwdt_is_open;
static void __iomem *pci_mem_addr; /* the PCI-memory address */
static unsigned long __iomem *hpwdt_nmistat;
@@ -61,11 +55,14 @@ MODULE_DEVICE_TABLE(pci, hpwdt_devices);
/*
* Watchdog operations
*/
-static void hpwdt_start(void)
+static int hpwdt_start(struct watchdog_device *wdd)
{
- reload = SECS_TO_TICKS(soft_margin);
+ reload = SECS_TO_TICKS(wdd->timeout);
+
iowrite16(reload, hpwdt_timer_reg);
iowrite8(0x85, hpwdt_timer_con);
+
+ return 0;
}
static void hpwdt_stop(void)
@@ -77,31 +74,33 @@ static void hpwdt_stop(void)
iowrite8(data, hpwdt_timer_con);
}
-static void hpwdt_ping(void)
+static int hpwdt_stop_core(struct watchdog_device *wdd)
{
- iowrite16(reload, hpwdt_timer_reg);
+ hpwdt_stop();
+
+ return 0;
}
-static int hpwdt_change_timer(int new_margin)
+static int hpwdt_ping(struct watchdog_device *wdd)
{
- if (new_margin < 1 || new_margin > HPWDT_MAX_TIMER) {
- pr_warn("New value passed in is invalid: %d seconds\n",
- new_margin);
- return -EINVAL;
- }
-
- soft_margin = new_margin;
- pr_debug("New timer passed in is %d seconds\n", new_margin);
- reload = SECS_TO_TICKS(soft_margin);
-
+ iowrite16(reload, hpwdt_timer_reg);
return 0;
}
-static int hpwdt_time_left(void)
+
+static unsigned int hpwdt_gettimeleft(struct watchdog_device *wdd)
{
return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
}
+static int hpwdt_settimeout(struct watchdog_device *wdd, unsigned int val)
+{
+ wdd->timeout = val;
+ hpwdt_ping(wdd);
+
+ return 0;
+}
+
#ifdef CONFIG_HPWDT_NMI_DECODING
static int hpwdt_my_nmi(void)
{
@@ -135,68 +134,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct
pt_regs *regs)
}
#endif /* CONFIG_HPWDT_NMI_DECODING */
-/*
- * /dev/watchdog handling
- */
-static int hpwdt_open(struct inode *inode, struct file *file)
-{
- /* /dev/watchdog can only be opened once */
- if (test_and_set_bit(0, &hpwdt_is_open))
- return -EBUSY;
-
- /* Start the watchdog */
- hpwdt_start();
- hpwdt_ping();
-
- return nonseekable_open(inode, file);
-}
-
-static int hpwdt_release(struct inode *inode, struct file *file)
-{
- /* Stop the watchdog */
- if (expect_release == 42) {
- hpwdt_stop();
- } else {
- pr_crit("Unexpected close, not stopping watchdog!\n");
- hpwdt_ping();
- }
-
- expect_release = 0;
-
- /* /dev/watchdog is being closed, make sure it can be re-opened */
- clear_bit(0, &hpwdt_is_open);
-
- return 0;
-}
-
-static ssize_t hpwdt_write(struct file *file, const char __user *data,
- size_t len, loff_t *ppos)
-{
- /* See if we got the magic character 'V' and reload the timer */
- if (len) {
- if (!nowayout) {
- size_t i;
-
- /* note: just in case someone wrote the magic character
-* five months ago... */
- expect_release = 0;
-
-
[PATCH v4 08/10] watchdog/hpwdt: Programable Pretimeout NMI
Make whether or not the hpwdt watchdog delivers a pretimeout NMI
programable by the user.
The underlying iLO hardware is programmable as to whether or not
a pre-timeout NMI is delivered to the system before the iLO resets
the system. However, the iLO does not allow for programming the
length of time that NMI is delivered before the system is reset.
By watchdog API, in hpwdt_set_pretimeout a val == 0 disables the NMI.
When val != 0, hpwdt_set_pretimeout will enable the pretimeout NMI
provided the current timeout is greator than the HW specified
pretimeout length. Otherwise an error is returned.
In set_timeout, if the new timeout is <= an already established pretimeout,
the pretimeout is canceled. This matches the action watchdog_set_timeout
in the watchdog core would do if an hpwdt specific set_timeout
function wasn't specified.
Signed-off-by: Jerry Hoemann
---
drivers/watchdog/hpwdt.c | 53
1 file changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index a584ccf05202..be7b3edac56c 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -31,11 +31,12 @@
#define TICKS_TO_SECS(ticks) ((ticks) * 128 / 1000)
#define HPWDT_MAX_TIMERTICKS_TO_SECS(65535)
#define DEFAULT_MARGIN 30
+#define PRETIMEOUT_SEC 9
static bool iLO5;
static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */
-static unsigned int reload;/* the computed soft_margin */
static bool nowayout = WATCHDOG_NOWAYOUT;
+static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);
static void __iomem *pci_mem_addr; /* the PCI-memory address */
static unsigned long __iomem *hpwdt_nmistat;
@@ -55,10 +56,11 @@ MODULE_DEVICE_TABLE(pci, hpwdt_devices);
*/
static int hpwdt_start(struct watchdog_device *wdd)
{
- reload = SECS_TO_TICKS(wdd->timeout);
+ int control = 0x81 | (pretimeout ? 0x4 : 0);
+ int reload = SECS_TO_TICKS(wdd->timeout);
iowrite16(reload, hpwdt_timer_reg);
- iowrite8(0x85, hpwdt_timer_con);
+ iowrite8(control, hpwdt_timer_con);
return 0;
}
@@ -81,7 +83,10 @@ static int hpwdt_stop_core(struct watchdog_device *wdd)
static int hpwdt_ping(struct watchdog_device *wdd)
{
+ int reload = SECS_TO_TICKS(wdd->timeout);
+
iowrite16(reload, hpwdt_timer_reg);
+
return 0;
}
@@ -94,12 +99,37 @@ static unsigned int hpwdt_gettimeleft(struct
watchdog_device *wdd)
static int hpwdt_settimeout(struct watchdog_device *wdd, unsigned int val)
{
wdd->timeout = val;
+ if (val <= wdd->pretimeout) {
+ wdd->pretimeout = 0;
+ pretimeout = 0;
+ if (watchdog_active(wdd))
+ hpwdt_start(wdd);
+ }
hpwdt_ping(wdd);
return 0;
}
#ifdef CONFIG_HPWDT_NMI_DECODING
+static int hpwdt_set_pretimeout(struct watchdog_device *wdd, unsigned int req)
+{
+ unsigned int val = 0;
+
+ if (req) {
+ val = PRETIMEOUT_SEC;
+ if (val >= wdd->timeout)
+ return -EINVAL;
+ }
+
+ wdd->pretimeout = val;
+ pretimeout = !!val;
+
+ if (watchdog_active(wdd))
+ hpwdt_start(wdd);
+
+ return 0;
+}
+
static int hpwdt_my_nmi(void)
{
return ioread8(hpwdt_nmistat) & 0x6;
@@ -122,6 +152,9 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct
pt_regs *regs)
if (iLO5 && ulReason == NMI_UNKNOWN && mynmi)
return NMI_DONE;
+ if (iLO5 && !pretimeout)
+ return NMI_DONE;
+
hpwdt_stop();
hex_byte_pack(panic_msg, mynmi);
@@ -133,7 +166,8 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct
pt_regs *regs)
static const struct watchdog_info ident = {
- .options = WDIOF_SETTIMEOUT |
+ .options = WDIOF_PRETIMEOUT|
+ WDIOF_SETTIMEOUT|
WDIOF_KEEPALIVEPING |
WDIOF_MAGICCLOSE,
.identity = "HPE iLO2+ HW Watchdog Timer",
@@ -150,6 +184,9 @@ static const struct watchdog_ops hpwdt_ops = {
.ping = hpwdt_ping,
.set_timeout= hpwdt_settimeout,
.get_timeleft = hpwdt_gettimeleft,
+#ifdef CONFIG_HPWDT_NMI_DECODING
+ .set_pretimeout = hpwdt_set_pretimeout,
+#endif
};
static struct watchdog_device hpwdt_dev = {
@@ -158,6 +195,9 @@ static struct watchdog_device hpwdt_dev = {
.min_timeout= 1,
.max_timeout= HPWDT_MAX_TIMER,
.timeout= DEFAULT_MARGIN,
+#ifdef CONFIG_HPWDT_NMI_DECODING
+ .pretimeout = PRETIMEOUT_SEC,
+#endif
};
@@ -319,4 +359,9 @@ module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once star
[PATCH v4 10/10] watchdog/hpwdt: Update driver version.
Update driver version number to reflect changes. Signed-off-by: Jerry Hoemann --- 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 b5fb2663c2a1..3432f58dd99d 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -26,7 +26,7 @@ #include #include -#define HPWDT_VERSION "1.4.0" +#define HPWDT_VERSION "2.0.0" #define SECS_TO_TICKS(secs)((secs) * 1000 / 128) #define TICKS_TO_SECS(ticks) ((ticks) * 128 / 1000) #define HPWDT_MAX_TIMERTICKS_TO_SECS(65535) -- 2.13.6
[PATCH v4 07/10] watchdog/hpwdt: remove allow_kdump module parameter.
The intent of this parameter is unclear and it sets up a race between the reset of the system by ASR and crashdump. The length of time between receipt of the pretimeout NMI and the ASR reset of the system is fixed by hardware. Turning the parameter off doesn't necessairly prevent a crash dump. Also, having the ASR reset occur while the system is crash dumping doesn't imply that the dump was hung given the short duration between the NMI and the reset. This parameter is not a substitute for having a architected watchdog crashdump hang detection paridigm. Signed-off-by: Jerry Hoemann --- drivers/watchdog/hpwdt.c | 16 +++- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index e60c689ebdb2..a584ccf05202 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -36,9 +36,6 @@ static bool iLO5; static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */ static unsigned int reload;/* the computed soft_margin */ static bool nowayout = WATCHDOG_NOWAYOUT; -#ifdef CONFIG_HPWDT_NMI_DECODING -static unsigned int allow_kdump = 1; -#endif static void __iomem *pci_mem_addr; /* the PCI-memory address */ static unsigned long __iomem *hpwdt_nmistat; @@ -125,8 +122,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) if (iLO5 && ulReason == NMI_UNKNOWN && mynmi) return NMI_DONE; - if (allow_kdump) - hpwdt_stop(); + hpwdt_stop(); hex_byte_pack(panic_msg, mynmi); nmi_panic(regs, panic_msg); @@ -188,9 +184,8 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev) goto error2; dev_info(&dev->dev, - "HPE Watchdog Timer Driver: NMI decoding initialized" - ", allow kernel dump: %s (default = 1/ON)\n", - (allow_kdump == 0) ? "OFF" : "ON"); + "HPE Watchdog Timer Driver: NMI decoding initialized\n"); + return 0; error2: @@ -324,9 +319,4 @@ module_param(nowayout, bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); -#ifdef CONFIG_HPWDT_NMI_DECODING -module_param(allow_kdump, int, 0); -MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs"); -#endif /* CONFIG_HPWDT_NMI_DECODING */ - module_pci_driver(hpwdt_driver); -- 2.13.6
[PATCH v4 06/10] watchdog/hpwdt: condition early return of NMI handler on iLO5
Modify prior change to not claim an NMI unless originated from iLO to apply only to iLO5 and later going forward. This restores hpwdt traditional behavior of calling panic if the NMI is NMI_IO_CHECK, NMI_SERR, or NMI_UNKNOWN for legacy hardware. Signed-off-by: Jerry Hoemann --- drivers/watchdog/hpwdt.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index 1a18326bc99d..e60c689ebdb2 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -32,6 +32,7 @@ #define HPWDT_MAX_TIMERTICKS_TO_SECS(65535) #define DEFAULT_MARGIN 30 +static bool iLO5; static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */ static unsigned int reload;/* the computed soft_margin */ static bool nowayout = WATCHDOG_NOWAYOUT; @@ -121,7 +122,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) "3. OA Forward Progress Log\n" "4. iLO Event Log"; - if ((ulReason == NMI_UNKNOWN) && mynmi) + if (iLO5 && ulReason == NMI_UNKNOWN && mynmi) return NMI_DONE; if (allow_kdump) @@ -279,6 +280,9 @@ static int hpwdt_init_one(struct pci_dev *dev, ", timer margin: %d seconds (nowayout=%d).\n", HPWDT_VERSION, hpwdt_dev.timeout, nowayout); + if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR) + iLO5 = 1; + return 0; error_wd_register: -- 2.13.6
[PATCH v4 01/10] watchdog/hpwdt: Update copyright.
Signed-off-by: Jerry Hoemann --- 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 f1f00dfc0e68..3d435d6d3226 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -4,7 +4,7 @@ * * SoftDog 0.05: A Software Watchdog Device * - * (c) Copyright 2015 Hewlett Packard Enterprise Development LP + * (c) Copyright 2018 Hewlett Packard Enterprise Development LP * Thomas Mingarelli * * This program is free software; you can redistribute it and/or -- 2.13.6
[PATCH] watchdog/hpwdt: update maintainer information
Signed-off-by: Jerry Hoemann --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 9a7f76eadae9..98b2ef91487d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6267,7 +6267,7 @@ S:Odd Fixes F: drivers/media/usb/hdpvr/ HEWLETT PACKARD ENTERPRISE ILO NMI WATCHDOG DRIVER -M: Jimmy Vance +M: Jerry Hoemann S: Supported F: Documentation/watchdog/hpwdt.txt F: drivers/watchdog/hpwdt.c -- 2.13.6
Re: [v3,07/11] watchdog/hpwdt: Modify to use watchdog core.
On Sat, Feb 17, 2018 at 08:49:07AM -0800, Guenter Roeck wrote:
> On Thu, Feb 15, 2018 at 04:43:56PM -0700, Jerry Hoemann wrote:
> > Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
> > convert hpwdt from legacy watchdog driver to use the watchdog core.
> >
> > Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
> > Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
> > Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
> > Added functions: hpwdt_settimeout
> > Added structures: watchdog_device
> >
> > Signed-off-by: Jerry Hoemann
> > ---
> > drivers/watchdog/hpwdt.c | 249
> > ---
> > 1 file changed, 63 insertions(+), 186 deletions(-)
> >
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index 71e49d0ab789..da9a04101814 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -14,18 +14,13 @@
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > #include
> > -#include
> > -#include
>
> The driver still performs direct IO. Why do you remove this
> include file ?
I was just removing file includes no longer needed to compile
the module. While there are sential ifdef protecting against
compile errors for multiply including the same file, the extra
open and parsing of the file does add a little bit of overhead
to the build time. Probably not as important today as it used
to be.
I will redact the change.
>
> > -#include
> > #include
> > -#include
> > #include
> > #include
> > #include
> > -#include
>
> The PCI vendor IDs used by the driver are declared in this file.
> Is there a direction somewhere suggesting that this file
> should not be directly included ?
See above.
>
> > #include
> > -#include
> > #include
> > +
> > #include
> >
> > #define HPWDT_VERSION "1.4.0"
> > @@ -40,8 +35,6 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
> > #ifdef CONFIG_HPWDT_NMI_DECODING
> > static unsigned int allow_kdump = 1;
> > #endif
> > -static char expect_release;
> > -static unsigned long hpwdt_is_open;
> >
> > static void __iomem *pci_mem_addr; /* the PCI-memory address */
> > static unsigned long __iomem *hpwdt_nmistat;
> > @@ -55,53 +48,58 @@ static const struct pci_device_id hpwdt_devices[] = {
> > };
> > MODULE_DEVICE_TABLE(pci, hpwdt_devices);
> >
> > +static struct watchdog_device hpwdt_dev;
>
> Please no double declarations. This is only needed for the NMI
> code to pass to hpwdt_stop where it isn't used. It would be
> easy to introduce _hpwdt_stop() with no parameter and call that
> function frm hpwdt_stop().
I'll redact this change.
I'll add a new function hpwdt_stop_new (or something like that) which
takes the watchdog_devices as input and it will call current upstream
hpwdt_stop.
>
> >
> > - retval = misc_register(&hpwdt_miscdev);
> > + hpwdt_dev.parent = &dev->dev;
> > + retval = watchdog_register_device(&hpwdt_dev);
> > if (retval < 0) {
> > - dev_warn(&dev->dev,
> > - "Unable to register miscdev on minor=%d (err=%d).\n",
> > - WATCHDOG_MINOR, retval);
> > - goto error_misc_register;
> > + dev_warn(&dev->dev, "Unable to register hpe watchdog
> > (err=%d).\n", retval);
>
> checkpatch warning. Also, this should be dev_err() since it is fatal.
i'll change to dev_err. i'll shorten the message to something similar.
>
> > + goto error_wd_register;
> > }
> >
> > + /* Make sure that timer is disabled until /dev/watchdog is opened */
> > + hpwdt_stop(&hpwdt_dev);
>
> The watchdog is already registered and the device can already have been
> opened at this point. The watchdog would have to be stopped before
> registering the watchdog.
>
> A better approach would be to detect if the watchdog is running and inform
> the watchdog core about it. The code in the stop function suggests that
> this would be possible. This would ensure that the watchdog protection
> during boot is retained.
I will redact this change as it will no longer be necessary. See above.
>
> > +
> > + watchdog_set_nowayout(&hpwdt_dev, nowayout);
> > + if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
> > + dev_warn(&dev->dev, "Invalid soft_margin: %d. Using default\n",
> > soft_margin);
> > +
> Those two functions should be called before registering the watchdog.
> Also, there is a checkpatch warning.
>
Will change.
Please update the Documentation/watchdog/watchdog-kernel-api.txt.
It is not clear from the documentation which interfaces are to be
called before watchdog_register_device and which ones are to be
called after.
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [v3,05/11] watchdog/hpwdt: Update Module info.
On Sat, Feb 17, 2018 at 08:19:23AM -0800, Guenter Roeck wrote:
> On Thu, Feb 15, 2018 at 04:43:54PM -0700, Jerry Hoemann wrote:
> > Update Module Author and description to reflect changes in driver.
> >
> > Signed-off-by: Jerry Hoemann
> > ---
> > 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 d5989acf3e37..71e49d0ab789 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -425,8 +425,8 @@ static struct pci_driver hpwdt_driver = {
> > .remove = hpwdt_exit,
> > };
> >
> > -MODULE_AUTHOR("Tom Mingarelli");
> > -MODULE_DESCRIPTION("hp watchdog driver");
> > +MODULE_AUTHOR("Jerry Hoemann");
> > +MODULE_DESCRIPTION("hpe watchdog driver");
>
> It is quite unusual to replace an author name. The original author is
> still the original author and should be retained. Adding another author
> is fine, but I disagree with the removal.
Wasn't intending to usurp Tom's credit (I left him in as Author in
comment block at top of file.) But as Tom has retired, I didn't want
anyone having problems with the rewrite of the driver to bother him
about it.
I'll redact this change.
>
> Guenter
>
> > MODULE_LICENSE("GPL");
> > MODULE_VERSION(HPWDT_VERSION);
> >
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [v3,04/11] watchdog/hpwdt: white space changes
On Sat, Feb 17, 2018 at 09:27:53PM +0100, Marcus Folkesson wrote:
> On Sat, Feb 17, 2018 at 12:32:08PM -0700, Jerry Hoemann wrote:
> > On Sat, Feb 17, 2018 at 08:17:34AM -0800, Guenter Roeck wrote:
> > > On Thu, Feb 15, 2018 at 04:43:53PM -0700, Jerry Hoemann wrote:
> > > > Minor white space changes and some name clean up.
> > > >
> > > > Signed-off-by: Jerry Hoemann
> > > > ---
> > > > MODULE_DEVICE_TABLE(pci, hpwdt_devices);
> > > >
> > > > @@ -102,7 +100,7 @@ static int hpwdt_time_left(void)
> > > > return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
> > > > }
> > > >
> > > > -#ifdef CONFIG_HPWDT_NMI_DECODING
> > > > +#ifdef CONFIG_HPWDT_NMI_DECODING /* { */
> > > > static int hpwdt_my_nmi(void)
> > > > {
> > > > return ioread8(hpwdt_nmistat) & 0x6;
> > > > @@ -133,7 +131,7 @@ static int hpwdt_pretimeout(unsigned int ulReason,
> > > > struct pt_regs *regs)
> > > >
> > > > return NMI_HANDLED;
> > > > }
> > > > -#endif /* CONFIG_HPWDT_NMI_DECODING */
> > > > +#endif /* } */
> > >
> > > I disagree with those changes. While I don't object to adding the '{'
> > > per se, I find it very useful to have the 'CONFIG_HPWDT_NMI_DECODING'
> > > with an endif to be able to associate it with the matching #ifdef.
>
> Well, it does not follow our coding style.
>
> Documentation/process/coding-style.rst:
>
> At the end of any non-trivial #if or #ifdef block (more than a few
> lines),
> place a comment after the #endif on the same line, noting the
> conditional
> expression used. For instance:
>
> .. code-block:: c
>
> #ifdef CONFIG_SOMETHING
> ...
> #endif /* CONFIG_SOMETHING */
>
> >
> > The matching /* { */ and /* } */ allow for quickly the finding of the
> > matching ifdef/endif.
> >
> > In the "vim" editor, the command '%' will take one from one curly paren to
> > its
> > matching curly paren...
>
> '%' in vim let you jump between matching ifdef/endif as well.
Interesting. Didn't know that. In that light, I'll redact the white space
changes.
Thanks
>
> >
> > There is a similar sequence for emacs.
> >
> > >
> > > Deferring to Wim.
> > >
> > > Guenter
> > >
> >
> > --
> >
> > -
> > Jerry Hoemann Software Engineer Hewlett Packard
> > Enterprise
> > -
>
> Thanks,
> Marcus Folkesson
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [v3,06/11] watchdog/hpwdt: Select WATCHDOG_CORE
On Sat, Feb 17, 2018 at 08:21:30AM -0800, Guenter Roeck wrote: > On Thu, Feb 15, 2018 at 04:43:55PM -0700, Jerry Hoemann wrote: > > Update Kconfig file to show that hpwdt now selects WATCHDOG_CORE. > > > > Signed-off-by: Jerry Hoemann > > At this point, WATCHDOG_CORE is not required. This patch should > be merged with the next patch, which adds the requirement. > > Since it appears that you insist on having it as separate patch, > deferring to Wim. > I have no objection to squashing these two commits once we have agreed upon the actual changes being made. I re-ordered to address the bisectability question, but kept them separate as it is a little bit easier for me to rework patches if the patch contains only a single file. -- ----- Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [v3,04/11] watchdog/hpwdt: white space changes
On Sat, Feb 17, 2018 at 08:17:34AM -0800, Guenter Roeck wrote:
> On Thu, Feb 15, 2018 at 04:43:53PM -0700, Jerry Hoemann wrote:
> > Minor white space changes and some name clean up.
> >
> > Signed-off-by: Jerry Hoemann
> > ---
> > MODULE_DEVICE_TABLE(pci, hpwdt_devices);
> >
> > @@ -102,7 +100,7 @@ static int hpwdt_time_left(void)
> > return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
> > }
> >
> > -#ifdef CONFIG_HPWDT_NMI_DECODING
> > +#ifdef CONFIG_HPWDT_NMI_DECODING /* { */
> > static int hpwdt_my_nmi(void)
> > {
> > return ioread8(hpwdt_nmistat) & 0x6;
> > @@ -133,7 +131,7 @@ static int hpwdt_pretimeout(unsigned int ulReason,
> > struct pt_regs *regs)
> >
> > return NMI_HANDLED;
> > }
> > -#endif /* CONFIG_HPWDT_NMI_DECODING */
> > +#endif /* } */
>
> I disagree with those changes. While I don't object to adding the '{'
> per se, I find it very useful to have the 'CONFIG_HPWDT_NMI_DECODING'
> with an endif to be able to associate it with the matching #ifdef.
The matching /* { */ and /* } */ allow for quickly the finding of the
matching ifdef/endif.
In the "vim" editor, the command '%' will take one from one curly paren to its
matching curly paren...
There is a similar sequence for emacs.
>
> Deferring to Wim.
>
> Guenter
>
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
Re: [PATCH v3 08/11] watchdog/hpwdt: Programable Pretimeout NMI
On Fri, Feb 16, 2018 at 03:55:06PM -0800, Guenter Roeck wrote:
> On Fri, Feb 16, 2018 at 04:46:17PM -0700, Jerry Hoemann wrote:
> > On Fri, Feb 16, 2018 at 12:34:40PM -0800, Guenter Roeck wrote:
> > > On Thu, Feb 15, 2018 at 04:43:57PM -0700, Jerry Hoemann wrote:
...
> > > > @@ -98,12 +106,21 @@ static int hpwdt_settimeout(struct watchdog_device
> > > > *dev, unsigned int val)
> > > > }
> > > >
> > > > #ifdef CONFIG_HPWDT_NMI_DECODING /* { */
> > > > +static int hpwdt_set_pretimeout(struct watchdog_device *dev, unsigned
> > > > int val)
> > > > +{
> > > > + if (val && (val != PRETIMEOUT_SEC)) {
> > >
> > > Unnecessary ( )
> >
> >
> > There are several things going on here. I'm not sure which one the above
> > comment is intended.
> >
> The "Unnecessary" refers to the ( ) around the second part of the expression
> above. While there may be valid reasons to include extra ( ), I think we
> can trust the C compiler to get it right here.
Okay, wasn't sure what you were getting at here.
I trust the C compiler, I don't trust humans. In compound conditionals,
I'll add parens so that the meaning is clear.
> > While a pretimeout NMI isn't required by the HW to be enabled, if enabled
> > the
> > length of pretimeout is fixed by HW.
> >
> > I didn't see anything in the API that would allow us to communicate to
> > the user this "feature." timeout at leasst has both min_timeout and
> > max_timeout, but
> > I didn't see similar for pretimeout. I also don't think its reasonable to
> > fail
> > here if the requested value is not 9 as the user really has no way of
> > knowing what
> > the valid range of pretimeout values are. So I accept, any non-zero value
> > for pretimeout, but then set pretimeout to be 9.
> >
> > But at the same time, I don't like to silently change a human request
> > w/o at least warning.
> >
> Sorry, I lost you here.
I wasn't sure to what you were objecting to. I thought you might
not have understood why I was converting non-zero values of
"pretimeout" to 9. Was trying to explain the reasoning.
A problem I see with the watchdog API is that users don't know
what is an acceptable range of values for pretimeout.
For HPE proliant systems, one cannot just choose an arbitrary
value for pretimeout.
I don't see a reasonable way that a user can determine the valid range
for pretimeout for HPE systems given our hardware restrictions.
>
> > >
> > > The actual timeout can be a value smaller than 9 seconds.
> > > Minimum is 1 second. What happens if the user configures
> > > a timeout of less than 9 seconds as well as a pretimeout ?
> > > Will it fire immediately ?
> >
> > The architecture is silent on this issue. My experience with
> > this is that if timeout < 9 seconds, the NMI is not issued.
> > System resets when the timeout expires. This could be implementation
> > dependent.
> >
> > Note, this is not a new issue.
> >
> Bad argument.
Not sure exactly to what your objections are. I'll point out that:
1) hpwdt has been using pretimeout NMI for watchdog for > 10 years.
2) For 8 years, its been possible to have a timeout < 9 seconds.
3) AFAIK this hasn't proven to be a big issue.
4) I have real questions as to how (or if) to address the issue.
I am perfectly willing to discuss the problem, but I don't think
it is a requirement for this patch set.
>
> > I thought about setting the min timeout to ten seconds to avoid this
> > situation.
> >
> You could reject reject request to set the pretimeout to a value <= the
> timeout.
I think you mis-communicated here.
It is perfectly fine to have a timeout of 30 seconds with the pretimeout of 9
seconds.
>
> > I haven't dug into the various user level clients of watchdog so I'm not
> > sure
> > what the impact of making this change would be to them.
> >
> >
> > >
> > > > + dev_info(dev->parent, "Setting pretimeout to %d\n",
> > > > PRETIMEOUT_SEC);
> > >
> > > Please no ongoing logging noise. This can easily be abused to clog
> > > the kernel log.
> >
> > Good point. I will look at WARN_ONCE or something similar.
> >
> A traceback if someone sets a bad timeout ? That would be even worse.
I am thinking something more in line with setting a static variable if
the message had already been printed and not reprinting it.
--
-
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-
