Re: [PATCH v1 0/2] Watchdog Core Global Parameters
rs_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(_dev, soft_margin, NULL);
>
> if (is_kdump_kernel()) {
> - pretimeout = 0;
> + pretimeout = false;
> kdumptimeout = 0;
> }
>
> if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) {
> dev_warn(>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
t; 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.
[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(_dev);
watchdog_set_nowayout(_dev, nowayout);
if (watchdog_init_timeout(_dev, soft_margin, NULL))
dev_warn(>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(_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, "timer is running\n");
+ set_bit(WDOG_HW_RUNNING, _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 = ,
.ops= _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;
retval = watchdog_register_device(_dev);
@@ -342,6 +359,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
hpwdt_dev.timeout, nowayout);
dev_info(>dev, "pretimeout: %s.\n",
pretimeout ? "on" : "off");
+ dev_info(>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,
> > > > _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, _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(>wdd))
> > > + qcom_wdt_stop(>wdd);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int __maybe_unused qcom_wdt_resume(struct device *dev)
> > > +{
> > > + struct qcom_wdt *wdt = dev_get_drvdata(dev);
> > > +
> > > + if (watchdog_active(>wdd))
> > > + qcom_wdt_start(>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 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, "Not supported on this device\n");
return -ENODEV;
+ }
if (pci_enable_device(dev)) {
dev_warn(>dev,
--
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, "Not supported on this device\n");
return -ENODEV;
+ }
if (pci_enable_device(dev)) {
dev_warn(>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, "Not supported on this platform\n");
+ return -ENODEV;
+ }
+
if (pci_enable_device(dev)) {
dev_warn(>dev,
"Not possible to enable PCI Device: 0x%x:0x%x.\n",
--
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, "Not supported on this platform\n");
+ return -ENODEV;
+ }
+
if (pci_enable_device(dev)) {
dev_warn(>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, );
+ 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
[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, );
+ 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 -
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, );
+ 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, );
+ 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, );
+ 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 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, );
+ 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, );
+ 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, );
+ 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
[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, );
> >>> + 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 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, );
> >>> + 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, );
> > + 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, );
> > + 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, );
> > + 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: [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, );
> > + 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, );
> > + 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, );
> > + 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, );
> >>> + 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, );
> >>> + 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, );
> >>> + if (!ret)
> >>> + printf("Watchdog pretimeout set to %u
> >>> seconds.\n", flags);
> >>
> >> It would good to make this m
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, );
> >>> + 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, );
> >>> + 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, );
> >>> + if (!ret)
> >>> + printf("Watchdog pretimeout set to %u
> >>> seconds.\n", flags);
> >>
> >> It would good to make this m
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
-
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, );
+ 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, );
+ 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, );
+ 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.
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, );
+ 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, );
+ 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, );
+ 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
[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, );
> > + 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
special case the failure case as we wouldn't go into the keep_alive
loop in either case.
>
> > + break;
> > + cas
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, );
> > + 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
special case the failure case as we wouldn't go into the keep_alive
loop in either case.
>
> > + break;
> > + cas
[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, );
+ 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, );
+ 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, );
+ 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] 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, );
+ 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, );
+ 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, );
+ 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(_dev, soft_margin, NULL))
dev_warn(>dev, "Invalid soft_margin: %d.\n", soft_margin);
+ if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) {
+ dev_warn(>dev, "timeout <= pretimeout. Setting pretimeout
to zero\n");
+ pretimeout = 0;
+ }
hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
hpwdt_dev.parent = >dev;
--
2.13.6
[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(_dev, soft_margin, NULL))
dev_warn(>dev, "Invalid soft_margin: %d.\n", soft_margin);
+ if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) {
+ dev_warn(>dev, "timeout <= pretimeout. Setting pretimeout
to zero\n");
+ pretimeout = 0;
+ }
hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
hpwdt_dev.parent = >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 -
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 t
[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 t
[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(_dev, soft_margin, NULL))
dev_warn(>dev, "Invalid soft_margin: %d.\n", soft_margin);
+ hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
+
hpwdt_dev.parent = >dev;
retval = watchdog_register_device(_dev);
if (retval < 0) {
--
1.8.3.1
[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(_dev, soft_margin, NULL))
dev_warn(>dev, "Invalid soft_margin: %d.\n", soft_margin);
+ hpwdt_dev.pretimeout = pretimeout ? PRETIMEOUT_SEC : 0;
+
hpwdt_dev.parent = >dev;
retval = watchdog_register_device(_dev);
if (retval < 0) {
--
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 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, "HPE Watchdog Timer Driver: %s" - ", timer margin: %d seconds (nowayout=%d).\n", - HPWDT_VERSION, hpwdt_dev.timeout, nowayout); + dev_info(>dev, "HPE Watchdog Timer Driver: Version: %s\n", + HPWDT_VERSION); + dev_info(>dev, "timeout: %d seconds (nowayout=%d)\n", + hpwdt_dev.timeout, nowayout); + dev_info(>dev, "pretimeout: %s.\n", + pretimeout ? "on" : "off"); if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR) ilo5 = true; -- 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, "HPE Watchdog Timer Driver: %s" - ", timer margin: %d seconds (nowayout=%d).\n", - HPWDT_VERSION, hpwdt_dev.timeout, nowayout); + dev_info(>dev, "HPE Watchdog Timer Driver: Version: %s\n", + HPWDT_VERSION); + dev_info(>dev, "timeout: %d seconds (nowayout=%d)\n", + hpwdt_dev.timeout, nowayout); + dev_info(>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
[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 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(_dev, soft_margin, NULL)) > > dev_warn(>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 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(_dev, soft_margin, NULL)) > > dev_warn(>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, "HPE Watchdog Timer Driver: %s" > > - ", timer margin: %d seconds (nowayout=%d).\n", > > - HPWDT_VERSION, hpwdt_dev.timeout, nowayout); > > + ", timeout : %d seconds (nowayout=%d) pretimeout=%s.\n", > > + HPWDT_VERSION, hpwdt_dev.timeout, nowayout, > > + pretimeout ? "on" : "off"); > When touching that, you might as well address > > WARNING: quoted string split across lines k. Think I'll split into two dev_info calls as line is too long to fit into 80 chars w/o splitting. > > Why did you add a space before ':' ? Personal preference ? I think you're referring to "timeout : %d seconds". Bad editting when going from "timer margin:" to "timeout :". I'll fix. If you referring to the spaces around the ternary operator, that is in coding-style although checkpatch accepts w/o spaces around the operators. > > Guenter > > > if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR) > > ilo5 = true; > > -- - Jerry Hoemann Software Engineer Hewlett Packard Enterprise -
Re: [PATCH 3/4] watchdog: hpwdt: Display module parameters.
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, "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(_dev, soft_margin, NULL))
dev_warn(>dev, "Invalid soft_margin: %d.\n", soft_margin);
+#ifdef CONFIG_HPWDT_NMI_DECODING
+ if (!pretimeout)
+ hpwdt_dev.pretimeout = 0;
+#endif
+
hpwdt_dev.parent = >dev;
retval = watchdog_register_device(_dev);
if (retval < 0) {
--
1.8.3.1
[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(_dev, soft_margin, NULL))
dev_warn(>dev, "Invalid soft_margin: %d.\n", soft_margin);
+#ifdef CONFIG_HPWDT_NMI_DECODING
+ if (!pretimeout)
+ hpwdt_dev.pretimeout = 0;
+#endif
+
hpwdt_dev.parent = >dev;
retval = watchdog_register_device(_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 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, "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
