* [PATCH 0/3] watchdog: hpwdt: driver update
@ 2017-10-20 22:54 Jerry Hoemann
2017-10-20 22:54 ` [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT Jerry Hoemann
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Jerry Hoemann @ 2017-10-20 22:54 UTC (permalink / raw)
To: wim, linux; +Cc: linux-watchdog, linux-kernel, Jerry Hoemann
Three short patches to the hpwdt driver.
1. Add WDIOC_GETPRETIMEOUT so that user space can determine
when to expect the NMI arrivial.
2. The SMBIOS check was incorrect.
3. Do not claim NMI unless generated from iLO.
Jerry Hoemann (3):
watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT
watchdog: hpwdt: SMBIOS check
watchdog: hpwdt: Check source of NMI
drivers/watchdog/hpwdt.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT 2017-10-20 22:54 [PATCH 0/3] watchdog: hpwdt: driver update Jerry Hoemann @ 2017-10-20 22:54 ` Jerry Hoemann 2017-10-21 2:25 ` Guenter Roeck 2017-10-20 22:54 ` [PATCH 2/3] watchdog: hpwdt: SMBIOS check Jerry Hoemann 2017-10-20 22:54 ` [PATCH 3/3] watchdog: hpwdt: Check source of NMI Jerry Hoemann 2 siblings, 1 reply; 12+ messages in thread From: Jerry Hoemann @ 2017-10-20 22:54 UTC (permalink / raw) To: wim, linux; +Cc: linux-watchdog, linux-kernel, Jerry Hoemann Add support for WDIOC_GETPRETIMEOUT ioctl so that user applications can determine when the NMI should arrive. Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> --- drivers/watchdog/hpwdt.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index 67fbe35..ef54b03 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -50,6 +50,7 @@ static bool nowayout = WATCHDOG_NOWAYOUT; static char expect_release; static unsigned long hpwdt_is_open; +static const int pretimeout = 9; static void __iomem *pci_mem_addr; /* the PCI-memory address */ static unsigned long __iomem *hpwdt_timer_reg; @@ -622,6 +623,12 @@ static long hpwdt_ioctl(struct file *file, unsigned int cmd, } break; + case WDIOC_GETPRETIMEOUT: + ret = copy_to_user(argp, &pretimeout, sizeof(pretimeout)); + if (ret) + ret = -EFAULT; + break; + case WDIOC_SETTIMEOUT: ret = get_user(new_margin, p); if (ret) -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT 2017-10-20 22:54 ` [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT Jerry Hoemann @ 2017-10-21 2:25 ` Guenter Roeck 2017-10-22 1:41 ` Jerry Hoemann 0 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2017-10-21 2:25 UTC (permalink / raw) To: Jerry Hoemann, wim; +Cc: linux-watchdog, linux-kernel On 10/20/2017 03:54 PM, Jerry Hoemann wrote: > Add support for WDIOC_GETPRETIMEOUT ioctl so that user applications > can determine when the NMI should arrive. > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > --- > drivers/watchdog/hpwdt.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > index 67fbe35..ef54b03 100644 > --- a/drivers/watchdog/hpwdt.c > +++ b/drivers/watchdog/hpwdt.c > @@ -50,6 +50,7 @@ > static bool nowayout = WATCHDOG_NOWAYOUT; > static char expect_release; > static unsigned long hpwdt_is_open; > +static const int pretimeout = 9; > > static void __iomem *pci_mem_addr; /* the PCI-memory address */ > static unsigned long __iomem *hpwdt_timer_reg; > @@ -622,6 +623,12 @@ static long hpwdt_ioctl(struct file *file, unsigned int cmd, > } > break; > > + case WDIOC_GETPRETIMEOUT: > + ret = copy_to_user(argp, &pretimeout, sizeof(pretimeout)); > + if (ret) > + ret = -EFAULT; > + break; > + > case WDIOC_SETTIMEOUT: > ret = get_user(new_margin, p); > if (ret) > Can you please convert the driver to use the watchdog subsystem instead ? If there are still improvements needed afterwards, they can still be implemented, but we really should not make improvements which are already supported by the watchdog core. Thanks, Guenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT 2017-10-21 2:25 ` Guenter Roeck @ 2017-10-22 1:41 ` Jerry Hoemann 2017-10-22 14:55 ` Guenter Roeck 0 siblings, 1 reply; 12+ messages in thread From: Jerry Hoemann @ 2017-10-22 1:41 UTC (permalink / raw) To: Guenter Roeck; +Cc: wim, linux-watchdog, linux-kernel On Fri, Oct 20, 2017 at 07:25:20PM -0700, Guenter Roeck wrote: > On 10/20/2017 03:54 PM, Jerry Hoemann wrote: > > Add support for WDIOC_GETPRETIMEOUT ioctl so that user applications > > can determine when the NMI should arrive. > > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > > --- > > drivers/watchdog/hpwdt.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > > index 67fbe35..ef54b03 100644 > > --- a/drivers/watchdog/hpwdt.c > > +++ b/drivers/watchdog/hpwdt.c > > @@ -50,6 +50,7 @@ > > static bool nowayout = WATCHDOG_NOWAYOUT; > > static char expect_release; > > static unsigned long hpwdt_is_open; > > +static const int pretimeout = 9; > > static void __iomem *pci_mem_addr; /* the PCI-memory address */ > > static unsigned long __iomem *hpwdt_timer_reg; > > @@ -622,6 +623,12 @@ static long hpwdt_ioctl(struct file *file, unsigned int cmd, > > } > > break; > > + case WDIOC_GETPRETIMEOUT: > > + ret = copy_to_user(argp, &pretimeout, sizeof(pretimeout)); > > + if (ret) > > + ret = -EFAULT; > > + break; > > + > > case WDIOC_SETTIMEOUT: > > ret = get_user(new_margin, p); > > if (ret) > > > > Can you please convert the driver to use the watchdog subsystem instead ? > If there are still improvements needed afterwards, they can still be > implemented, but we really should not make improvements which are > already supported by the watchdog core. I will look into converting the driver, but would like to get this fix in independently. SuSE brought https://bugzilla.novell.com/show_bug.cgi?id=1042933 to my attention earlier this summer. The submitter was trying to develop a watchdog test where the ping rate was set to be the Timeout/2. The test worked fine until (Timeout/2) < PreTimeout. At this point an NMI would be delivered to the system before the test could refresh the timer. I came to the view that a watchdog that implements a pre-timeout NMI where the value of the pre-timeout is not known programmatically as having a defect. This problem has been around a long time and we could live with it, but figured while I was in fixing other problems, I'd fix this one as well. Thanks -- ----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise ----------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT 2017-10-22 1:41 ` Jerry Hoemann @ 2017-10-22 14:55 ` Guenter Roeck 2017-10-24 16:13 ` Jerry Hoemann 0 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2017-10-22 14:55 UTC (permalink / raw) To: Jerry.Hoemann; +Cc: wim, linux-watchdog, linux-kernel On 10/21/2017 06:41 PM, Jerry Hoemann wrote: > On Fri, Oct 20, 2017 at 07:25:20PM -0700, Guenter Roeck wrote: >> On 10/20/2017 03:54 PM, Jerry Hoemann wrote: >>> Add support for WDIOC_GETPRETIMEOUT ioctl so that user applications >>> can determine when the NMI should arrive. >>> >>> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> >>> --- >>> drivers/watchdog/hpwdt.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c >>> index 67fbe35..ef54b03 100644 >>> --- a/drivers/watchdog/hpwdt.c >>> +++ b/drivers/watchdog/hpwdt.c >>> @@ -50,6 +50,7 @@ >>> static bool nowayout = WATCHDOG_NOWAYOUT; >>> static char expect_release; >>> static unsigned long hpwdt_is_open; >>> +static const int pretimeout = 9; >>> static void __iomem *pci_mem_addr; /* the PCI-memory address */ >>> static unsigned long __iomem *hpwdt_timer_reg; >>> @@ -622,6 +623,12 @@ static long hpwdt_ioctl(struct file *file, unsigned int cmd, >>> } >>> break; >>> + case WDIOC_GETPRETIMEOUT: >>> + ret = copy_to_user(argp, &pretimeout, sizeof(pretimeout)); >>> + if (ret) >>> + ret = -EFAULT; >>> + break; >>> + >>> case WDIOC_SETTIMEOUT: >>> ret = get_user(new_margin, p); >>> if (ret) >>> >> >> Can you please convert the driver to use the watchdog subsystem instead ? >> If there are still improvements needed afterwards, they can still be >> implemented, but we really should not make improvements which are >> already supported by the watchdog core. > > I will look into converting the driver, but would like to get this > fix in independently. > I don't see this patch as a fix. It adds functionality. the other two patches are fixes and should come first. > SuSE brought https://bugzilla.novell.com/show_bug.cgi?id=1042933 > to my attention earlier this summer. The submitter was trying to > develop a watchdog test where the ping rate was set to be the > Timeout/2. > > The test worked fine until (Timeout/2) < PreTimeout. At this point > an NMI would be delivered to the system before the test could refresh > the timer. > Yes, but I don't think that is fixed with this patch. That would be patch 3/3, no ? Guenter > I came to the view that a watchdog that implements a pre-timeout NMI > where the value of the pre-timeout is not known programmatically as having > a defect. > > This problem has been around a long time and we could live with it, but > figured while I was in fixing other problems, I'd fix this one as well. > > Thanks > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT 2017-10-22 14:55 ` Guenter Roeck @ 2017-10-24 16:13 ` Jerry Hoemann 2017-10-24 18:42 ` Guenter Roeck 0 siblings, 1 reply; 12+ messages in thread From: Jerry Hoemann @ 2017-10-24 16:13 UTC (permalink / raw) To: Guenter Roeck; +Cc: wim, linux-watchdog, linux-kernel On Sun, Oct 22, 2017 at 07:55:19AM -0700, Guenter Roeck wrote: > On 10/21/2017 06:41 PM, Jerry Hoemann wrote: > > On Fri, Oct 20, 2017 at 07:25:20PM -0700, Guenter Roeck wrote: > > > On 10/20/2017 03:54 PM, Jerry Hoemann wrote: > > > > Add support for WDIOC_GETPRETIMEOUT ioctl so that user applications > > > > can determine when the NMI should arrive. > > > > > > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > > > > --- > > > > drivers/watchdog/hpwdt.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > > > > index 67fbe35..ef54b03 100644 > > > > --- a/drivers/watchdog/hpwdt.c > > > > +++ b/drivers/watchdog/hpwdt.c > > > > @@ -50,6 +50,7 @@ > > > > static bool nowayout = WATCHDOG_NOWAYOUT; > > > > static char expect_release; > > > > static unsigned long hpwdt_is_open; > > > > +static const int pretimeout = 9; > > > > static void __iomem *pci_mem_addr; /* the PCI-memory address */ > > > > static unsigned long __iomem *hpwdt_timer_reg; > > > > @@ -622,6 +623,12 @@ static long hpwdt_ioctl(struct file *file, unsigned int cmd, > > > > } > > > > break; > > > > + case WDIOC_GETPRETIMEOUT: > > > > + ret = copy_to_user(argp, &pretimeout, sizeof(pretimeout)); > > > > + if (ret) > > > > + ret = -EFAULT; > > > > + break; > > > > + > > > > case WDIOC_SETTIMEOUT: > > > > ret = get_user(new_margin, p); > > > > if (ret) > > > > > > > > > > Can you please convert the driver to use the watchdog subsystem instead ? > > > If there are still improvements needed afterwards, they can still be > > > implemented, but we really should not make improvements which are > > > already supported by the watchdog core. > > > > I will look into converting the driver, but would like to get this > > fix in independently. > > > > I don't see this patch as a fix. It adds functionality. the other two patches > are fixes and should come first. > > > SuSE brought https://bugzilla.novell.com/show_bug.cgi?id=1042933 > > to my attention earlier this summer. The submitter was trying to > > develop a watchdog test where the ping rate was set to be the > > Timeout/2. > > > > The test worked fine until (Timeout/2) < PreTimeout. At this point > > an NMI would be delivered to the system before the test could refresh > > the timer. > > > Yes, but I don't think that is fixed with this patch. That would be patch 3/3, no ? > > Guenter Oh, the test program had problems too. While a pretimeout doesn't have to crash the system they can. hpwdt most certainly does. So, my read of how to safely handle a wdt is this: Let Pretimeout == 0 for wdt that don't support a pre timeout. A user space applications needs to ensure that: 1. Timeout > Pretimeout 2. Ping < Timeout - Pretimeout The test application did neither. But since hpwdt wasn't supplying the pretimeout, even the corrected test application would have still failed. Am I missing something? How should user space daemons take into account pretimeouts? thanks Jerry > > > I came to the view that a watchdog that implements a pre-timeout NMI > > where the value of the pre-timeout is not known programmatically as having > > a defect. > > > > This problem has been around a long time and we could live with it, but > > figured while I was in fixing other problems, I'd fix this one as well. > > > > Thanks > > -- ----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise ----------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT 2017-10-24 16:13 ` Jerry Hoemann @ 2017-10-24 18:42 ` Guenter Roeck 0 siblings, 0 replies; 12+ messages in thread From: Guenter Roeck @ 2017-10-24 18:42 UTC (permalink / raw) To: Jerry Hoemann; +Cc: wim, linux-watchdog, linux-kernel On Tue, Oct 24, 2017 at 10:13:22AM -0600, Jerry Hoemann wrote: > On Sun, Oct 22, 2017 at 07:55:19AM -0700, Guenter Roeck wrote: > > On 10/21/2017 06:41 PM, Jerry Hoemann wrote: > > > On Fri, Oct 20, 2017 at 07:25:20PM -0700, Guenter Roeck wrote: > > > > On 10/20/2017 03:54 PM, Jerry Hoemann wrote: > > > > > Add support for WDIOC_GETPRETIMEOUT ioctl so that user applications > > > > > can determine when the NMI should arrive. > > > > > > > > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > > > > > --- > > > > > drivers/watchdog/hpwdt.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > > > > > index 67fbe35..ef54b03 100644 > > > > > --- a/drivers/watchdog/hpwdt.c > > > > > +++ b/drivers/watchdog/hpwdt.c > > > > > @@ -50,6 +50,7 @@ > > > > > static bool nowayout = WATCHDOG_NOWAYOUT; > > > > > static char expect_release; > > > > > static unsigned long hpwdt_is_open; > > > > > +static const int pretimeout = 9; > > > > > static void __iomem *pci_mem_addr; /* the PCI-memory address */ > > > > > static unsigned long __iomem *hpwdt_timer_reg; > > > > > @@ -622,6 +623,12 @@ static long hpwdt_ioctl(struct file *file, unsigned int cmd, > > > > > } > > > > > break; > > > > > + case WDIOC_GETPRETIMEOUT: > > > > > + ret = copy_to_user(argp, &pretimeout, sizeof(pretimeout)); > > > > > + if (ret) > > > > > + ret = -EFAULT; > > > > > + break; > > > > > + > > > > > case WDIOC_SETTIMEOUT: > > > > > ret = get_user(new_margin, p); > > > > > if (ret) > > > > > > > > > > > > > Can you please convert the driver to use the watchdog subsystem instead ? > > > > If there are still improvements needed afterwards, they can still be > > > > implemented, but we really should not make improvements which are > > > > already supported by the watchdog core. > > > > > > I will look into converting the driver, but would like to get this > > > fix in independently. > > > > > > > I don't see this patch as a fix. It adds functionality. the other two patches > > are fixes and should come first. > > > > > SuSE brought https://bugzilla.novell.com/show_bug.cgi?id=1042933 > > > to my attention earlier this summer. The submitter was trying to > > > develop a watchdog test where the ping rate was set to be the > > > Timeout/2. > > > > > > The test worked fine until (Timeout/2) < PreTimeout. At this point > > > an NMI would be delivered to the system before the test could refresh > > > the timer. > > > > > Yes, but I don't think that is fixed with this patch. That would be patch 3/3, no ? > > > > Guenter > > Oh, the test program had problems too. > > While a pretimeout doesn't have to crash the system they can. hpwdt most > certainly does. > > So, my read of how to safely handle a wdt is this: > Let Pretimeout == 0 for wdt that don't support a pre timeout. > A user space applications needs to ensure that: > > 1. Timeout > Pretimeout > 2. Ping < Timeout - Pretimeout > > The test application did neither. But since hpwdt wasn't supplying > the pretimeout, even the corrected test application would have still > failed. > > Am I missing something? How should user space daemons take into > account pretimeouts? > The watchdog core should reject bad timout (and pretimeout) values. Of course, the hpwdt driver may not do this, since it doesn't use the watchdog core. My take is that the driver should be converted to use the watchdog core instead of trying to fix it. Now, if you insist to improve the driver's pretimeout handling instead of converting it to use the watchdog core, that is fine with me. Please note though that I'll defer the review of such patches to Wim since I strongly believe that it is the wrong approach. Thanks, Guenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] watchdog: hpwdt: SMBIOS check 2017-10-20 22:54 [PATCH 0/3] watchdog: hpwdt: driver update Jerry Hoemann 2017-10-20 22:54 ` [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT Jerry Hoemann @ 2017-10-20 22:54 ` Jerry Hoemann 2017-10-21 2:37 ` Guenter Roeck 2017-10-20 22:54 ` [PATCH 3/3] watchdog: hpwdt: Check source of NMI Jerry Hoemann 2 siblings, 1 reply; 12+ messages in thread From: Jerry Hoemann @ 2017-10-20 22:54 UTC (permalink / raw) To: wim, linux; +Cc: linux-watchdog, linux-kernel, Jerry Hoemann Correct test on SMBIOS table 219 Misc Features bits for UEFI supported. Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> --- 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 ef54b03..4c011e8 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -707,7 +707,7 @@ static void dmi_find_icru(const struct dmi_header *dm, void *dummy) smbios_proliant_ptr = (struct smbios_proliant_info *) dm; if (smbios_proliant_ptr->misc_features & 0x01) is_icru = 1; - if (smbios_proliant_ptr->misc_features & 0x408) + if (smbios_proliant_ptr->misc_features & 0x1400) is_uefi = 1; } } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] watchdog: hpwdt: SMBIOS check 2017-10-20 22:54 ` [PATCH 2/3] watchdog: hpwdt: SMBIOS check Jerry Hoemann @ 2017-10-21 2:37 ` Guenter Roeck 2017-10-22 1:56 ` Jerry Hoemann 0 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2017-10-21 2:37 UTC (permalink / raw) To: Jerry Hoemann, wim; +Cc: linux-watchdog, linux-kernel On 10/20/2017 03:54 PM, Jerry Hoemann wrote: > Correct test on SMBIOS table 219 Misc Features bits for UEFI supported. > Please explain in more detail. There is no table 219 in the SMBIOS specification. There is table 9, BIOS Characteristics Extension Byte 2, which specifies bit 3 as "UEFI Specification is supported.", but nothing that really maps to the other byte, and no "misc features". Maybe this is HP specific, but then we'll need to have much better explanation. > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > --- > 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 ef54b03..4c011e8 100644 > --- a/drivers/watchdog/hpwdt.c > +++ b/drivers/watchdog/hpwdt.c > @@ -707,7 +707,7 @@ static void dmi_find_icru(const struct dmi_header *dm, void *dummy) > smbios_proliant_ptr = (struct smbios_proliant_info *) dm; > if (smbios_proliant_ptr->misc_features & 0x01) > is_icru = 1; > - if (smbios_proliant_ptr->misc_features & 0x408) > + if (smbios_proliant_ptr->misc_features & 0x1400) > is_uefi = 1; > } > } > Presumably patch 2/3 and 3/3 are bug fixs and should come first so they can be applied to stable releases. Thanks, Guenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] watchdog: hpwdt: SMBIOS check 2017-10-21 2:37 ` Guenter Roeck @ 2017-10-22 1:56 ` Jerry Hoemann 2017-10-22 14:56 ` Guenter Roeck 0 siblings, 1 reply; 12+ messages in thread From: Jerry Hoemann @ 2017-10-22 1:56 UTC (permalink / raw) To: Guenter Roeck; +Cc: wim, linux-watchdog, linux-kernel, Jerry Hoemann On Fri, Oct 20, 2017 at 07:37:21PM -0700, Guenter Roeck wrote: > On 10/20/2017 03:54 PM, Jerry Hoemann wrote: > > Correct test on SMBIOS table 219 Misc Features bits for UEFI supported. > > > Please explain in more detail. There is no table 219 in the SMBIOS specification. Sorry, my patch documentation was imprecise, I should have stated Type 219 record. Type 219 is an HPE OEM SMBIOS extension whose contents are considered confidential, so I'm not at liberty to go into details. I will say that Type 219 describes features of the iLO which hpwdt is implemented against. > There is table 9, BIOS Characteristics Extension Byte 2, which specifies bit 3 > as "UEFI Specification is supported.", but nothing that really maps to the > other byte, and no "misc features". Maybe this is HP specific, but then we'll > need to have much better explanation. This patch is to correct commit cce78da766. Our current servers do not support the CRU BIOS interfaces and we need to avoid calling it. Tom initially only checked that iCRU which replaced CRU was supported. But, later added code to extend to also test whether UEFI was supported to anticipate a time when iCRU wasn't supported either but where we still don't want to call back into CRU. Tom's original change was implemented to an older definition of Type 219. Unfortunately, the specification (and firmware) were modified to use a different pair of bits to represent UEFI. However, a corresponding change to update Linux was missed. The code is currently working today as the iCRU bit is correctly being checked. But as the purpose of cce78da766 is to protect the code for a time when iCRU isn't true, we want to correct the checking of the UEFI bit. > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > > --- > > 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 ef54b03..4c011e8 100644 > > --- a/drivers/watchdog/hpwdt.c > > +++ b/drivers/watchdog/hpwdt.c > > @@ -707,7 +707,7 @@ static void dmi_find_icru(const struct dmi_header *dm, void *dummy) > > smbios_proliant_ptr = (struct smbios_proliant_info *) dm; > > if (smbios_proliant_ptr->misc_features & 0x01) > > is_icru = 1; > > - if (smbios_proliant_ptr->misc_features & 0x408) > > + if (smbios_proliant_ptr->misc_features & 0x1400) > > is_uefi = 1; > > } > > } > > > Presumably patch 2/3 and 3/3 are bug fixs and should come first > so they can be applied to stable releases. > I can re-order the patches or delay the first patch if necessary. Thanks Jerry -- ----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise ----------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] watchdog: hpwdt: SMBIOS check 2017-10-22 1:56 ` Jerry Hoemann @ 2017-10-22 14:56 ` Guenter Roeck 0 siblings, 0 replies; 12+ messages in thread From: Guenter Roeck @ 2017-10-22 14:56 UTC (permalink / raw) To: Jerry.Hoemann; +Cc: wim, linux-watchdog, linux-kernel On 10/21/2017 06:56 PM, Jerry Hoemann wrote: > On Fri, Oct 20, 2017 at 07:37:21PM -0700, Guenter Roeck wrote: >> On 10/20/2017 03:54 PM, Jerry Hoemann wrote: >>> Correct test on SMBIOS table 219 Misc Features bits for UEFI supported. >>> >> Please explain in more detail. There is no table 219 in the SMBIOS specification. > > Sorry, my patch documentation was imprecise, I should have stated Type 219 record. > > Type 219 is an HPE OEM SMBIOS extension whose contents are considered > confidential, so I'm not at liberty to go into details. I will say > that Type 219 describes features of the iLO which hpwdt is implemented > against. > > >> There is table 9, BIOS Characteristics Extension Byte 2, which specifies bit 3 >> as "UEFI Specification is supported.", but nothing that really maps to the >> other byte, and no "misc features". Maybe this is HP specific, but then we'll >> need to have much better explanation. > > This patch is to correct commit cce78da766. > > Our current servers do not support the CRU BIOS interfaces and we > need to avoid calling it. Tom initially only checked that iCRU which > replaced CRU was supported. But, later added code to extend to also > test whether UEFI was supported to anticipate a time when iCRU wasn't > supported either but where we still don't want to call back into CRU. > > Tom's original change was implemented to an older definition of Type 219. > Unfortunately, the specification (and firmware) were modified to use a > different pair of bits to represent UEFI. However, a corresponding change > to update Linux was missed. > > The code is currently working today as the iCRU bit is correctly being > checked. But as the purpose of cce78da766 is to protect the > code for a time when iCRU isn't true, we want to correct the > checking of the UEFI bit. > Please add at least some of that explanation to the description and reorder the patches such that the fixes come first. Thanks, Guenter > >> >>> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> >>> --- >>> 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 ef54b03..4c011e8 100644 >>> --- a/drivers/watchdog/hpwdt.c >>> +++ b/drivers/watchdog/hpwdt.c >>> @@ -707,7 +707,7 @@ static void dmi_find_icru(const struct dmi_header *dm, void *dummy) >>> smbios_proliant_ptr = (struct smbios_proliant_info *) dm; >>> if (smbios_proliant_ptr->misc_features & 0x01) >>> is_icru = 1; >>> - if (smbios_proliant_ptr->misc_features & 0x408) >>> + if (smbios_proliant_ptr->misc_features & 0x1400) >>> is_uefi = 1; >>> } >>> } >>> >> Presumably patch 2/3 and 3/3 are bug fixs and should come first >> so they can be applied to stable releases. >> > > I can re-order the patches or delay the first patch if necessary. > > > Thanks > Jerry > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] watchdog: hpwdt: Check source of NMI 2017-10-20 22:54 [PATCH 0/3] watchdog: hpwdt: driver update Jerry Hoemann 2017-10-20 22:54 ` [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT Jerry Hoemann 2017-10-20 22:54 ` [PATCH 2/3] watchdog: hpwdt: SMBIOS check Jerry Hoemann @ 2017-10-20 22:54 ` Jerry Hoemann 2 siblings, 0 replies; 12+ messages in thread From: Jerry Hoemann @ 2017-10-20 22:54 UTC (permalink / raw) To: wim, linux; +Cc: linux-watchdog, linux-kernel, Jerry Hoemann Do not claim the NMI (i.e. return NMI_DONE) if the source of the NMI isn't the iLO watchdog or debug. Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> --- drivers/watchdog/hpwdt.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index 4c011e8..b64ce43 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -53,6 +53,7 @@ static const int pretimeout = 9; static void __iomem *pci_mem_addr; /* the PCI-memory address */ +static unsigned long __iomem *hpwdt_nmistat; static unsigned long __iomem *hpwdt_timer_reg; static unsigned long __iomem *hpwdt_timer_con; @@ -475,6 +476,11 @@ static int hpwdt_time_left(void) return TICKS_TO_SECS(ioread16(hpwdt_timer_reg)); } +static int hpwdt_my_nmi(void) +{ + return ioread8(hpwdt_nmistat) & 0x6; +} + #ifdef CONFIG_HPWDT_NMI_DECODING /* * NMI Handler @@ -487,6 +493,9 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) if (!hpwdt_nmi_decoding) return NMI_DONE; + if ((ulReason == NMI_UNKNOWN) && !hpwdt_my_nmi()) + return NMI_DONE; + spin_lock_irqsave(&rom_lock, rom_pl); if (!die_nmi_called && !is_icru && !is_uefi) asminline_call(&cmn_regs, cru_rom_addr); @@ -849,6 +858,7 @@ static int hpwdt_init_one(struct pci_dev *dev, retval = -ENOMEM; goto error_pci_iomap; } + hpwdt_nmistat = pci_mem_addr + 0x6e; hpwdt_timer_reg = pci_mem_addr + 0x70; hpwdt_timer_con = pci_mem_addr + 0x72; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-10-24 18:42 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-20 22:54 [PATCH 0/3] watchdog: hpwdt: driver update Jerry Hoemann 2017-10-20 22:54 ` [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT Jerry Hoemann 2017-10-21 2:25 ` Guenter Roeck 2017-10-22 1:41 ` Jerry Hoemann 2017-10-22 14:55 ` Guenter Roeck 2017-10-24 16:13 ` Jerry Hoemann 2017-10-24 18:42 ` Guenter Roeck 2017-10-20 22:54 ` [PATCH 2/3] watchdog: hpwdt: SMBIOS check Jerry Hoemann 2017-10-21 2:37 ` Guenter Roeck 2017-10-22 1:56 ` Jerry Hoemann 2017-10-22 14:56 ` Guenter Roeck 2017-10-20 22:54 ` [PATCH 3/3] watchdog: hpwdt: Check source of NMI Jerry Hoemann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.