watchdog: hpwdt: driver update
cover watchdog: hpwdt: driver update
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(-)
Signature · 1 line
1.8.3.1
PATCH 1/3 watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT
Add support for WDIOC_GETPRETIMEOUT ioctl so that user applications can determine when the NMI should arrive.
--- drivers/watchdog/hpwdt.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/watchdog/hpwdt.c +7 -0
@@ -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)
Signature · 1 line
1.8.3.1
Guenter Roeck
maintainerRe: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT
Quoted 24 lines · Jerry Hoemann
Add support for
WDIOC_GETPRETIMEOUTioctl so that user applications can determine when the NMI should arrive.Signed-off-by Jerry Hoemann <[email protected]>--- drivers/watchdog/hpwdt.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/watchdog/hpwdt.c +1
-0@@ -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 ;
drivers/watchdog/hpwdt.c static long hpwdt_ioctl(struct file *file, unsigned int cmd, · line 634
Quoted 10 lines
drivers/watchdog/hpwdt.c +6
-0+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
Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT
Quoted 30 lines · Guenter Roeck
Quoted 29 lines · Jerry Hoemann
Add support for
WDIOC_GETPRETIMEOUTioctl so that user applications can determine when the NMI should arrive.Signed-off-by Jerry Hoemann <[email protected]>--- drivers/watchdog/hpwdt.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/watchdog/hpwdt.c +7
-0@@ -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 :
Quoted 8 lines
drivers/watchdog/hpwdt.c static long hpwdt_ioctl(struct file *file, unsigned int cmd,
Quoted 3 lines
drivers/watchdog/hpwdt.c +0
-0ret =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
Signature · 3 lines
----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise -----------------------------------------------------------------------------
Guenter Roeck
maintainerRe: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT
Quoted 40 lines · Jerry Hoemann
Quoted 30 lines · Guenter Roeck
Quoted 29 lines · Jerry Hoemann
Add support for
WDIOC_GETPRETIMEOUTioctl so that user applications can determine when the NMI should arrive.Signed-off-by Jerry Hoemann <[email protected]>--- drivers/watchdog/hpwdt.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/watchdog/hpwdt.c +7
-0@@ -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 :Quoted 8 lines
drivers/watchdog/hpwdt.c static long hpwdt_ioctl(struct file *file, unsigned int cmd,
Quoted 3 lines
drivers/watchdog/hpwdt.c +0
-0ret =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.
Quoted 3 lines
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.
Quoted 9 lines
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
Quoted 9 lines
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
Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT
Quoted 53 lines · Guenter Roeck
Quoted 40 lines · Jerry Hoemann
Quoted 30 lines · Guenter Roeck
Quoted 29 lines · Jerry Hoemann
Add support for
WDIOC_GETPRETIMEOUTioctl so that user applications can determine when the NMI should arrive.Signed-off-by Jerry Hoemann <[email protected]>--- drivers/watchdog/hpwdt.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/watchdog/hpwdt.c +7
-0@@ -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 :Quoted 8 lines
drivers/watchdog/hpwdt.c static long hpwdt_ioctl(struct file *file, unsigned int cmd,
Quoted 3 lines
drivers/watchdog/hpwdt.c +0
-0ret =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.
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 7 lines
Quoted 4 lines
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
Quoted 10 lines
Quoted 10 lines
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
Signature · 3 lines
----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise -----------------------------------------------------------------------------
Guenter Roeck
maintainerRe: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT
Quoted 74 lines · Jerry Hoemann
Quoted 53 lines · Guenter Roeck
Quoted 40 lines · Jerry Hoemann
Quoted 30 lines · Guenter Roeck
Quoted 29 lines · Jerry Hoemann
Add support for
WDIOC_GETPRETIMEOUTioctl so that user applications can determine when the NMI should arrive.Signed-off-by Jerry Hoemann <[email protected]>--- drivers/watchdog/hpwdt.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/watchdog/hpwdt.c +7
-0@@ -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 :Quoted 8 lines
drivers/watchdog/hpwdt.c static long hpwdt_ioctl(struct file *file, unsigned int cmd,
Quoted 3 lines
drivers/watchdog/hpwdt.c +0
-0ret =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.
Quoted 3 lines
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.
Quoted 5 lines
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.Quoted 7 lines
Quoted 4 lines
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
Quoted 7 lines
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
PATCH 2/3 watchdog: hpwdt: SMBIOS check
In reply to Jerry HoemannCorrect test on SMBIOS table 219 Misc Features bits for UEFI supported.
--- drivers/watchdog/hpwdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/hpwdt.c +1 -1
@@ -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 ; } }
Signature · 1 line
1.8.3.1
Guenter Roeck
maintainerRe: [PATCH 2/3] watchdog: hpwdt: SMBIOS check
Quoted 2 lines · Jerry Hoemann
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.
Quoted 11 lines
Signed-off-by Jerry Hoemann <[email protected]>--- drivers/watchdog/hpwdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/watchdog/hpwdt.c +0
-0
drivers/watchdog/hpwdt.c static void dmi_find_icru(const struct dmi_header *dm, void *dummy) · line 713
Quoted 8 lines
drivers/watchdog/hpwdt.c +1
-1@@ -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
Re: [PATCH 2/3] watchdog: hpwdt: SMBIOS check
Quoted 4 lines · Guenter Roeck
Quoted 2 lines · Jerry Hoemann
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.
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
Signed-off-by Jerry Hoemann <[email protected]>--- drivers/watchdog/hpwdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 17 lines
Quoted 6 lines
drivers/watchdog/hpwdt.c +0
-0drivers/watchdog/hpwdt.c static void dmi_find_icru(const struct dmi_header *dm, void *dummy) · line 713
Quoted 8 lines
drivers/watchdog/hpwdt.c +1
-1@@ -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
Signature · 3 lines
----------------------------------------------------------------------------- Jerry Hoemann Software Engineer Hewlett Packard Enterprise -----------------------------------------------------------------------------
Guenter Roeck
maintainerRe: [PATCH 2/3] watchdog: hpwdt: SMBIOS check
Quoted 32 lines · Jerry Hoemann
Quoted 4 lines · Guenter Roeck
Quoted 2 lines · Jerry Hoemann
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.
Quoted 4 lines
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.
Quoted 5 lines
The code is currently working today as the iCRU bit is correctly being checked. But as the purpose of
cce78da766is 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
Quoted 31 lines
Quoted 7 lines
Quoted 7 lines
Signed-off-by Jerry Hoemann <[email protected]>--- drivers/watchdog/hpwdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Quoted 17 lines
Quoted 6 lines
drivers/watchdog/hpwdt.c +0
-0drivers/watchdog/hpwdt.c static void dmi_find_icru(const struct dmi_header *dm, void *dummy) · line 713
Quoted 8 lines
drivers/watchdog/hpwdt.c +1
-1@@ -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
PATCH 3/3 watchdog: hpwdt: Check source of NMI
In reply to Jerry HoemannDo not claim the NMI (i.e. return NMI_DONE) if the source of the NMI isn't the iLO watchdog or debug.
--- drivers/watchdog/hpwdt.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/watchdog/hpwdt.c +10 -0
@@ -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 ;
Signature · 1 line
1.8.3.1