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

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

[Jerry Hoemann]

Changes v2
----------
1. Re-order patches

2. Provide additional details for SMBIOS check patch.


Three short patches to the hpwdt driver.

1. An hpwdt SMBIOS check is incorrect due to a change in
   the specification and firmware.

2. Do not claim NMI unless generated from iLO.

3. Add WDIOC_GETPRETIMEOUT so that user space can determine
   when to expect the NMI arrivial.


Jerry Hoemann (3):
  watchdog: hpwdt: SMBIOS check
  watchdog: hpwdt: Check source of NMI
  watchdog: hpwdt: add ioctl WDIOC_GETPRETIMEOUT

 drivers/watchdog/hpwdt.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH v2 1/3] watchdog: hpwdt: SMBIOS check

[Jerry Hoemann]

This corrects:
commit cce78da76601 ("watchdog: hpwdt: Add check for UEFI bits")

The test on HPE SMBIOS extension type 219 record "Misc Features"
bits for UEFI support is incorrect.  The definition of the Misc Features
bits in the HPE SMBIOS OEM Extensions specification (and related
firmware) was changed to use a different pair of bits to
represent UEFI supported.  Howerver, a corresponding change
to Linux was missed.

Current code/platform work because the iCRU test is working.
But purpose of cce78da766 is to ensure correct functionality
on future systems where iCRU isn't 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 67fbe35..9fd869f 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -700,7 +700,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

Re: [PATCH v2 1/3] watchdog: hpwdt: SMBIOS check

[Guenter Roeck]

On 10/23/2017 03:46 PM, Jerry Hoemann wrote:
> This corrects:
> commit cce78da76601 ("watchdog: hpwdt: Add check for UEFI bits")
> 
> The test on HPE SMBIOS extension type 219 record "Misc Features"
> bits for UEFI support is incorrect.  The definition of the Misc Features
> bits in the HPE SMBIOS OEM Extensions specification (and related
> firmware) was changed to use a different pair of bits to
> represent UEFI supported.  Howerver, a corresponding change
> to Linux was missed.
> 
> Current code/platform work because the iCRU test is working.
> But purpose of cce78da766 is to ensure correct functionality
> on future systems where iCRU isn't supported.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   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 67fbe35..9fd869f 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -700,7 +700,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;
>   	}
>   }
> 

[PATCH v2 2/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 9fd869f..e616583 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -52,6 +52,7 @@
 static unsigned long hpwdt_is_open;
 
 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;
 
@@ -474,6 +475,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
@@ -486,6 +492,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);
@@ -842,6 +851,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 v2 2/3] watchdog: hpwdt: Check source of NMI

[Guenter Roeck]

On 10/23/2017 03:46 PM, Jerry Hoemann wrote:
> 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>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/hpwdt.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 9fd869f..e616583 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -52,6 +52,7 @@
>   static unsigned long hpwdt_is_open;
>   
>   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;
>   
> @@ -474,6 +475,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
> @@ -486,6 +492,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);
> @@ -842,6 +851,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;
>   
> 

[PATCH v2 3/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 e616583..b64ce43 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_nmistat;
@@ -631,6 +632,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

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

[Guenter Roeck]

On 10/23/2017 03:46 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>

I'll leave this for Wim to decide. My take is that we should not add functionality
to old-style drivers and convert those drivers to use the watchdog core instead.

Thanks,
Guenter

> ---
>   drivers/watchdog/hpwdt.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index e616583..b64ce43 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_nmistat;
> @@ -631,6 +632,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)
>