[PATCH 0/3] watchdog: hpwdt: driver update

[PATCH 0/3] watchdog: hpwdt: driver update

[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

[PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT

[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

[PATCH 2/3] watchdog: hpwdt: SMBIOS check

[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

[PATCH 3/3] watchdog: hpwdt: Check source of NMI

[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

Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT

[Guenter Roeck]

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

Re: [PATCH 2/3] watchdog: hpwdt: SMBIOS check

[Guenter Roeck]

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

Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT

[Jerry Hoemann]

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
-----------------------------------------------------------------------------

Re: [PATCH 2/3] watchdog: hpwdt: SMBIOS check

[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
-----------------------------------------------------------------------------

Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT

[Guenter Roeck]

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
> 

Re: [PATCH 2/3] watchdog: hpwdt: SMBIOS check

[Guenter Roeck]

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
> 

Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT

[Jerry Hoemann]

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
-----------------------------------------------------------------------------

Re: [PATCH 1/3] watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT

[Guenter Roeck]

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