[PATCH v3 00/11] watchdog/hpwdt: Update driver to use watchdog core.

[PATCH v3 00/11] watchdog/hpwdt: Update driver to use watchdog core.

[Jerry Hoemann]

== v3 ==

Incorperating code review feedback. Following modifications were made:

1) Patch 0003: Use existing hex_byte_pack instead of creating new function.
2) Patch 0005: Redacted change in module_param permission.
3) Patch 0006: switch from pr_debug etc., to dev_dbg where possible.
4) Patch 0006: No longer updating soft_margin post module load.
5) Patch 0006: Initialize hpwdt_dev.parent before registering watchdog.
6) Patch 0006: Redacted change to dev_info message w.r.t. allow_kdump
7) Patch 0006 & 0007: Reorder patches to maintain bisectability.
8) Patch 0008: Change pr_debug to dev_dbg
9) Patch 0010: Change dev_info message w.r.t. allow_kdump where feature
		is removed.

Note, I am explicitly ignoring the checkpatch error on Patch 0008
for specifying permisson of "0" instead of "0000".


== v2 ==

1) Fix compiler error when CONFIG_HPWDT_NMI_DECODING is not defined.

2) Break out driver version change to its own patch (0011).


== v1 ==

The primary purposes of this patch set are to

1) Update the hpwdt driver to use the watchdog core.
2) Reduce complexity by removing unnecessary features.
3) Add customer requested features like optional pretimeout.
4) Enhance readability/maintainability of the driver.

The size of the resultant driver is reduced from over 900
lines to 350 lines.

Patch 1& 2 remove legacy NMI sourcing.
Patch 3	adds useful indication of NMI cause to panic message
Patch 4 & 5 are general cleanup
Patch 6 & 7 updates the driver to user the watchdog core.
Patch 8 makes the pretimeout NMI programmable.
Patch 9 modifies whether the NMI handler claims the NMI.
Patch 10 retracts the allow_kdump module parameter.



Jerry Hoemann (11):
  watchdog/hpwdt: Remove legacy NMI sourcing.
  watchdog/hpwdt: remove include files no longer needed.
  watchdog/hpwdt: Update nmi_panic message.
  watchdog/hpwdt: white space changes
  watchdog/hpwdt: Update Module info.
  watchdog/hpwdt: Select WATCHDOG_CORE
  watchdog/hpwdt: Modify to use watchdog core.
  watchdog/hpwdt: Programable Pretimeout NMI
  watchdog/hpwdt: condition early return of NMI handler on iLO5
  watchdog/hpwdt: remove allow_kdump module parameter.
  watchdog/hpwdt: Update driver version.

 drivers/watchdog/Kconfig |   1 +
 drivers/watchdog/hpwdt.c | 845 ++++++++---------------------------------------
 2 files changed, 134 insertions(+), 712 deletions(-)

-- 
2.13.6

[PATCH v3 01/11] watchdog/hpwdt: Remove legacy NMI sourcing.

[Jerry Hoemann]

Gen8 and prior Proliant systems supported the "CRU" interface
to firmware.  This interfaces allows linux to "call back" into firmware
to source the cause of an NMI.  This feature isn't fully utilized
as the actual source of the NMI isn't printed, the driver only
indicates that the source couldn't be determined when the call
fails.

With the advent of Gen9, iCRU replaces the CRU. The call back
feature is no longer available in firmware.  To be compatible and
not attempt to call back into firmware on system not supporting CRU,
the SMBIOS table is consulted to determine if it is safe to
make the call back or not.

This results in about half of the driver code being devoted
to either making CRU calls or determing if it is safe to make
CRU calls.  As noted, the driver isn't really using the results of
the CRU calls.

As the CRU sourcing of the NMI isn't required for handling the
NMI, remove the legacy (pre Gen9) NMI sourcing and the DMI code to
determine if the system had the CRU interface.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 490 +----------------------------------------------
 1 file changed, 8 insertions(+), 482 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index f1f00dfc0e68..113058644fc3 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -48,6 +48,9 @@
 static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
 static unsigned int reload;			/* the computed soft_margin */
 static bool nowayout = WATCHDOG_NOWAYOUT;
+#ifdef CONFIG_HPWDT_NMI_DECODING
+static unsigned int allow_kdump = 1;
+#endif
 static char expect_release;
 static unsigned long hpwdt_is_open;
 
@@ -63,373 +66,6 @@ static const struct pci_device_id hpwdt_devices[] = {
 };
 MODULE_DEVICE_TABLE(pci, hpwdt_devices);
 
-#ifdef CONFIG_HPWDT_NMI_DECODING
-#define PCI_BIOS32_SD_VALUE		0x5F32335F	/* "_32_" */
-#define CRU_BIOS_SIGNATURE_VALUE	0x55524324
-#define PCI_BIOS32_PARAGRAPH_LEN	16
-#define PCI_ROM_BASE1			0x000F0000
-#define ROM_SIZE			0x10000
-
-struct bios32_service_dir {
-	u32 signature;
-	u32 entry_point;
-	u8 revision;
-	u8 length;
-	u8 checksum;
-	u8 reserved[5];
-};
-
-/* type 212 */
-struct smbios_cru64_info {
-	u8 type;
-	u8 byte_length;
-	u16 handle;
-	u32 signature;
-	u64 physical_address;
-	u32 double_length;
-	u32 double_offset;
-};
-#define SMBIOS_CRU64_INFORMATION	212
-
-/* type 219 */
-struct smbios_proliant_info {
-	u8 type;
-	u8 byte_length;
-	u16 handle;
-	u32 power_features;
-	u32 omega_features;
-	u32 reserved;
-	u32 misc_features;
-};
-#define SMBIOS_ICRU_INFORMATION		219
-
-
-struct cmn_registers {
-	union {
-		struct {
-			u8 ral;
-			u8 rah;
-			u16 rea2;
-		};
-		u32 reax;
-	} u1;
-	union {
-		struct {
-			u8 rbl;
-			u8 rbh;
-			u8 reb2l;
-			u8 reb2h;
-		};
-		u32 rebx;
-	} u2;
-	union {
-		struct {
-			u8 rcl;
-			u8 rch;
-			u16 rec2;
-		};
-		u32 recx;
-	} u3;
-	union {
-		struct {
-			u8 rdl;
-			u8 rdh;
-			u16 red2;
-		};
-		u32 redx;
-	} u4;
-
-	u32 resi;
-	u32 redi;
-	u16 rds;
-	u16 res;
-	u32 reflags;
-}  __attribute__((packed));
-
-static unsigned int hpwdt_nmi_decoding;
-static unsigned int allow_kdump = 1;
-static unsigned int is_icru;
-static unsigned int is_uefi;
-static DEFINE_SPINLOCK(rom_lock);
-static void *cru_rom_addr;
-static struct cmn_registers cmn_regs;
-
-extern asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
-						unsigned long *pRomEntry);
-
-#ifdef CONFIG_X86_32
-/* --32 Bit Bios------------------------------------------------------------ */
-
-#define HPWDT_ARCH	32
-
-asm(".text                          \n\t"
-    ".align 4                       \n\t"
-    ".globl asminline_call	    \n"
-    "asminline_call:                \n\t"
-    "pushl       %ebp               \n\t"
-    "movl        %esp, %ebp         \n\t"
-    "pusha                          \n\t"
-    "pushf                          \n\t"
-    "push        %es                \n\t"
-    "push        %ds                \n\t"
-    "pop         %es                \n\t"
-    "movl        8(%ebp),%eax       \n\t"
-    "movl        4(%eax),%ebx       \n\t"
-    "movl        8(%eax),%ecx       \n\t"
-    "movl        12(%eax),%edx      \n\t"
-    "movl        16(%eax),%esi      \n\t"
-    "movl        20(%eax),%edi      \n\t"
-    "movl        (%eax),%eax        \n\t"
-    "push        %cs                \n\t"
-    "call        *12(%ebp)          \n\t"
-    "pushf                          \n\t"
-    "pushl       %eax               \n\t"
-    "movl        8(%ebp),%eax       \n\t"
-    "movl        %ebx,4(%eax)       \n\t"
-    "movl        %ecx,8(%eax)       \n\t"
-    "movl        %edx,12(%eax)      \n\t"
-    "movl        %esi,16(%eax)      \n\t"
-    "movl        %edi,20(%eax)      \n\t"
-    "movw        %ds,24(%eax)       \n\t"
-    "movw        %es,26(%eax)       \n\t"
-    "popl        %ebx               \n\t"
-    "movl        %ebx,(%eax)        \n\t"
-    "popl        %ebx               \n\t"
-    "movl        %ebx,28(%eax)      \n\t"
-    "pop         %es                \n\t"
-    "popf                           \n\t"
-    "popa                           \n\t"
-    "leave                          \n\t"
-    "ret                            \n\t"
-    ".previous");
-
-
-/*
- *	cru_detect
- *
- *	Routine Description:
- *	This function uses the 32-bit BIOS Service Directory record to
- *	search for a $CRU record.
- *
- *	Return Value:
- *	0        :  SUCCESS
- *	<0       :  FAILURE
- */
-static int cru_detect(unsigned long map_entry,
-	unsigned long map_offset)
-{
-	void *bios32_map;
-	unsigned long *bios32_entrypoint;
-	unsigned long cru_physical_address;
-	unsigned long cru_length;
-	unsigned long physical_bios_base = 0;
-	unsigned long physical_bios_offset = 0;
-	int retval = -ENODEV;
-
-	bios32_map = ioremap(map_entry, (2 * PAGE_SIZE));
-
-	if (bios32_map == NULL)
-		return -ENODEV;
-
-	bios32_entrypoint = bios32_map + map_offset;
-
-	cmn_regs.u1.reax = CRU_BIOS_SIGNATURE_VALUE;
-
-	set_memory_x((unsigned long)bios32_map, 2);
-	asminline_call(&cmn_regs, bios32_entrypoint);
-
-	if (cmn_regs.u1.ral != 0) {
-		pr_warn("Call succeeded but with an error: 0x%x\n",
-			cmn_regs.u1.ral);
-	} else {
-		physical_bios_base = cmn_regs.u2.rebx;
-		physical_bios_offset = cmn_regs.u4.redx;
-		cru_length = cmn_regs.u3.recx;
-		cru_physical_address =
-			physical_bios_base + physical_bios_offset;
-
-		/* If the values look OK, then map it in. */
-		if ((physical_bios_base + physical_bios_offset)) {
-			cru_rom_addr =
-				ioremap(cru_physical_address, cru_length);
-			if (cru_rom_addr) {
-				set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK,
-					(cru_length + PAGE_SIZE - 1) >> PAGE_SHIFT);
-				retval = 0;
-			}
-		}
-
-		pr_debug("CRU Base Address:   0x%lx\n", physical_bios_base);
-		pr_debug("CRU Offset Address: 0x%lx\n", physical_bios_offset);
-		pr_debug("CRU Length:         0x%lx\n", cru_length);
-		pr_debug("CRU Mapped Address: %p\n", &cru_rom_addr);
-	}
-	iounmap(bios32_map);
-	return retval;
-}
-
-/*
- *	bios_checksum
- */
-static int bios_checksum(const char __iomem *ptr, int len)
-{
-	char sum = 0;
-	int i;
-
-	/*
-	 * calculate checksum of size bytes. This should add up
-	 * to zero if we have a valid header.
-	 */
-	for (i = 0; i < len; i++)
-		sum += ptr[i];
-
-	return ((sum == 0) && (len > 0));
-}
-
-/*
- *	bios32_present
- *
- *	Routine Description:
- *	This function finds the 32-bit BIOS Service Directory
- *
- *	Return Value:
- *	0        :  SUCCESS
- *	<0       :  FAILURE
- */
-static int bios32_present(const char __iomem *p)
-{
-	struct bios32_service_dir *bios_32_ptr;
-	int length;
-	unsigned long map_entry, map_offset;
-
-	bios_32_ptr = (struct bios32_service_dir *) p;
-
-	/*
-	 * Search for signature by checking equal to the swizzled value
-	 * instead of calling another routine to perform a strcmp.
-	 */
-	if (bios_32_ptr->signature == PCI_BIOS32_SD_VALUE) {
-		length = bios_32_ptr->length * PCI_BIOS32_PARAGRAPH_LEN;
-		if (bios_checksum(p, length)) {
-			/*
-			 * According to the spec, we're looking for the
-			 * first 4KB-aligned address below the entrypoint
-			 * listed in the header. The Service Directory code
-			 * is guaranteed to occupy no more than 2 4KB pages.
-			 */
-			map_entry = bios_32_ptr->entry_point & ~(PAGE_SIZE - 1);
-			map_offset = bios_32_ptr->entry_point - map_entry;
-
-			return cru_detect(map_entry, map_offset);
-		}
-	}
-	return -ENODEV;
-}
-
-static int detect_cru_service(void)
-{
-	char __iomem *p, *q;
-	int rc = -1;
-
-	/*
-	 * Search from 0x0f0000 through 0x0fffff, inclusive.
-	 */
-	p = ioremap(PCI_ROM_BASE1, ROM_SIZE);
-	if (p == NULL)
-		return -ENOMEM;
-
-	for (q = p; q < p + ROM_SIZE; q += 16) {
-		rc = bios32_present(q);
-		if (!rc)
-			break;
-	}
-	iounmap(p);
-	return rc;
-}
-/* ------------------------------------------------------------------------- */
-#endif /* CONFIG_X86_32 */
-#ifdef CONFIG_X86_64
-/* --64 Bit Bios------------------------------------------------------------ */
-
-#define HPWDT_ARCH	64
-
-asm(".text                      \n\t"
-    ".align 4                   \n\t"
-    ".globl asminline_call	\n\t"
-    ".type asminline_call, @function \n\t"
-    "asminline_call:            \n\t"
-    FRAME_BEGIN
-    "pushq      %rax            \n\t"
-    "pushq      %rbx            \n\t"
-    "pushq      %rdx            \n\t"
-    "pushq      %r12            \n\t"
-    "pushq      %r9             \n\t"
-    "movq       %rsi, %r12      \n\t"
-    "movq       %rdi, %r9       \n\t"
-    "movl       4(%r9),%ebx     \n\t"
-    "movl       8(%r9),%ecx     \n\t"
-    "movl       12(%r9),%edx    \n\t"
-    "movl       16(%r9),%esi    \n\t"
-    "movl       20(%r9),%edi    \n\t"
-    "movl       (%r9),%eax      \n\t"
-    "call       *%r12           \n\t"
-    "pushfq                     \n\t"
-    "popq        %r12           \n\t"
-    "movl       %eax, (%r9)     \n\t"
-    "movl       %ebx, 4(%r9)    \n\t"
-    "movl       %ecx, 8(%r9)    \n\t"
-    "movl       %edx, 12(%r9)   \n\t"
-    "movl       %esi, 16(%r9)   \n\t"
-    "movl       %edi, 20(%r9)   \n\t"
-    "movq       %r12, %rax      \n\t"
-    "movl       %eax, 28(%r9)   \n\t"
-    "popq       %r9             \n\t"
-    "popq       %r12            \n\t"
-    "popq       %rdx            \n\t"
-    "popq       %rbx            \n\t"
-    "popq       %rax            \n\t"
-    FRAME_END
-    "ret                        \n\t"
-    ".previous");
-
-/*
- *	dmi_find_cru
- *
- *	Routine Description:
- *	This function checks whether or not a SMBIOS/DMI record is
- *	the 64bit CRU info or not
- */
-static void dmi_find_cru(const struct dmi_header *dm, void *dummy)
-{
-	struct smbios_cru64_info *smbios_cru64_ptr;
-	unsigned long cru_physical_address;
-
-	if (dm->type == SMBIOS_CRU64_INFORMATION) {
-		smbios_cru64_ptr = (struct smbios_cru64_info *) dm;
-		if (smbios_cru64_ptr->signature == CRU_BIOS_SIGNATURE_VALUE) {
-			cru_physical_address =
-				smbios_cru64_ptr->physical_address +
-				smbios_cru64_ptr->double_offset;
-			cru_rom_addr = ioremap(cru_physical_address,
-				smbios_cru64_ptr->double_length);
-			set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK,
-				smbios_cru64_ptr->double_length >> PAGE_SHIFT);
-		}
-	}
-}
-
-static int detect_cru_service(void)
-{
-	cru_rom_addr = NULL;
-
-	dmi_walk(dmi_find_cru, NULL);
-
-	/* if cru_rom_addr has been set then we found a CRU service */
-	return ((cru_rom_addr != NULL) ? 0 : -ENODEV);
-}
-/* ------------------------------------------------------------------------- */
-#endif /* CONFIG_X86_64 */
-#endif /* CONFIG_HPWDT_NMI_DECODING */
 
 /*
  *	Watchdog operations
@@ -486,30 +122,12 @@ static int hpwdt_my_nmi(void)
  */
 static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 {
-	unsigned long rom_pl;
-	static int die_nmi_called;
-
-	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);
-	die_nmi_called = 1;
-	spin_unlock_irqrestore(&rom_lock, rom_pl);
-
 	if (allow_kdump)
 		hpwdt_stop();
 
-	if (!is_icru && !is_uefi) {
-		if (cmn_regs.u1.ral == 0) {
-			nmi_panic(regs, "An NMI occurred, but unable to determine source.\n");
-			return NMI_HANDLED;
-		}
-	}
 	nmi_panic(regs, "An NMI occurred. Depending on your system the reason "
 		"for the NMI is logged in any one of the following "
 		"resources:\n"
@@ -675,84 +293,11 @@ static struct miscdevice hpwdt_miscdev = {
  *	Init & Exit
  */
 
-#ifdef CONFIG_HPWDT_NMI_DECODING
-#ifdef CONFIG_X86_LOCAL_APIC
-static void hpwdt_check_nmi_decoding(struct pci_dev *dev)
-{
-	/*
-	 * If nmi_watchdog is turned off then we can turn on
-	 * our nmi decoding capability.
-	 */
-	hpwdt_nmi_decoding = 1;
-}
-#else
-static void hpwdt_check_nmi_decoding(struct pci_dev *dev)
-{
-	dev_warn(&dev->dev, "NMI decoding is disabled. "
-		"Your kernel does not support a NMI Watchdog.\n");
-}
-#endif /* CONFIG_X86_LOCAL_APIC */
-
-/*
- *	dmi_find_icru
- *
- *	Routine Description:
- *	This function checks whether or not we are on an iCRU-based server.
- *	This check is independent of architecture and needs to be made for
- *	any ProLiant system.
- */
-static void dmi_find_icru(const struct dmi_header *dm, void *dummy)
-{
-	struct smbios_proliant_info *smbios_proliant_ptr;
-
-	if (dm->type == SMBIOS_ICRU_INFORMATION) {
-		smbios_proliant_ptr = (struct smbios_proliant_info *) dm;
-		if (smbios_proliant_ptr->misc_features & 0x01)
-			is_icru = 1;
-		if (smbios_proliant_ptr->misc_features & 0x1400)
-			is_uefi = 1;
-	}
-}
 
 static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
 {
+#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
 	int retval;
-
-	/*
-	 * On typical CRU-based systems we need to map that service in
-	 * the BIOS. For 32 bit Operating Systems we need to go through
-	 * the 32 Bit BIOS Service Directory. For 64 bit Operating
-	 * Systems we get that service through SMBIOS.
-	 *
-	 * On systems that support the new iCRU service all we need to
-	 * do is call dmi_walk to get the supported flag value and skip
-	 * the old cru detect code.
-	 */
-	dmi_walk(dmi_find_icru, NULL);
-	if (!is_icru && !is_uefi) {
-
-		/*
-		* We need to map the ROM to get the CRU service.
-		* For 32 bit Operating Systems we need to go through the 32 Bit
-		* BIOS Service Directory
-		* For 64 bit Operating Systems we get that service through SMBIOS.
-		*/
-		retval = detect_cru_service();
-		if (retval < 0) {
-			dev_warn(&dev->dev,
-				"Unable to detect the %d Bit CRU Service.\n",
-				HPWDT_ARCH);
-			return retval;
-		}
-
-		/*
-		* We know this is the only CRU call we need to make so lets keep as
-		* few instructions as possible once the NMI comes in.
-		*/
-		cmn_regs.u1.rah = 0x0D;
-		cmn_regs.u1.ral = 0x02;
-	}
-
 	/*
 	 * Only one function can register for NMI_UNKNOWN
 	 */
@@ -780,33 +325,19 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
 	dev_warn(&dev->dev,
 		"Unable to register a die notifier (err=%d).\n",
 		retval);
-	if (cru_rom_addr)
-		iounmap(cru_rom_addr);
 	return retval;
+#endif					/* } */
+	return 0;
 }
 
 static void hpwdt_exit_nmi_decoding(void)
 {
+#ifdef CONFIG_HPWDT_NMI_DECODING
 	unregister_nmi_handler(NMI_UNKNOWN, "hpwdt");
 	unregister_nmi_handler(NMI_SERR, "hpwdt");
 	unregister_nmi_handler(NMI_IO_CHECK, "hpwdt");
-	if (cru_rom_addr)
-		iounmap(cru_rom_addr);
+#endif
 }
-#else /* !CONFIG_HPWDT_NMI_DECODING */
-static void hpwdt_check_nmi_decoding(struct pci_dev *dev)
-{
-}
-
-static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
-{
-	return 0;
-}
-
-static void hpwdt_exit_nmi_decoding(void)
-{
-}
-#endif /* CONFIG_HPWDT_NMI_DECODING */
 
 static int hpwdt_init_one(struct pci_dev *dev,
 					const struct pci_device_id *ent)
@@ -814,11 +345,6 @@ static int hpwdt_init_one(struct pci_dev *dev,
 	int retval;
 
 	/*
-	 * Check if we can do NMI decoding or not
-	 */
-	hpwdt_check_nmi_decoding(dev);
-
-	/*
 	 * First let's find out if we are on an iLO2+ server. We will
 	 * not run on a legacy ASM box.
 	 * So we only support the G5 ProLiant servers and higher.
-- 
2.13.6

Re: [PATCH v3 01/11] watchdog/hpwdt: Remove legacy NMI sourcing.

[Ingo Molnar]

* Jerry Hoemann <jerry.hoemann@hpe.com> wrote:

> Gen8 and prior Proliant systems supported the "CRU" interface
> to firmware.  This interfaces allows linux to "call back" into firmware
> to source the cause of an NMI.  This feature isn't fully utilized
> as the actual source of the NMI isn't printed, the driver only
> indicates that the source couldn't be determined when the call
> fails.
> 
> With the advent of Gen9, iCRU replaces the CRU. The call back
> feature is no longer available in firmware.  To be compatible and
> not attempt to call back into firmware on system not supporting CRU,
> the SMBIOS table is consulted to determine if it is safe to
> make the call back or not.
> 
> This results in about half of the driver code being devoted
> to either making CRU calls or determing if it is safe to make
> CRU calls.  As noted, the driver isn't really using the results of
> the CRU calls.
> 
> As the CRU sourcing of the NMI isn't required for handling the
> NMI, remove the legacy (pre Gen9) NMI sourcing and the DMI code to
> determine if the system had the CRU interface.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/watchdog/hpwdt.c | 490 +----------------------------------------------
>  1 file changed, 8 insertions(+), 482 deletions(-)

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [v3,01/11] watchdog/hpwdt: Remove legacy NMI sourcing.

[Guenter Roeck]

On Thu, Feb 15, 2018 at 04:43:50PM -0700, Jerry Hoemann wrote:
> Gen8 and prior Proliant systems supported the "CRU" interface
> to firmware.  This interfaces allows linux to "call back" into firmware
> to source the cause of an NMI.  This feature isn't fully utilized
> as the actual source of the NMI isn't printed, the driver only
> indicates that the source couldn't be determined when the call
> fails.
> 
> With the advent of Gen9, iCRU replaces the CRU. The call back
> feature is no longer available in firmware.  To be compatible and
> not attempt to call back into firmware on system not supporting CRU,
> the SMBIOS table is consulted to determine if it is safe to
> make the call back or not.
> 
> This results in about half of the driver code being devoted
> to either making CRU calls or determing if it is safe to make
> CRU calls.  As noted, the driver isn't really using the results of
> the CRU calls.
> 
> As the CRU sourcing of the NMI isn't required for handling the
> NMI, remove the legacy (pre Gen9) NMI sourcing and the DMI code to
> determine if the system had the CRU interface.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> Acked-by: Ingo Molnar <mingo@kernel.org>

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

> ---
>  drivers/watchdog/hpwdt.c | 490 +----------------------------------------------
>  1 file changed, 8 insertions(+), 482 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index f1f00dfc0e68..113058644fc3 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -48,6 +48,9 @@
>  static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
>  static unsigned int reload;			/* the computed soft_margin */
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> +#ifdef CONFIG_HPWDT_NMI_DECODING
> +static unsigned int allow_kdump = 1;
> +#endif
>  static char expect_release;
>  static unsigned long hpwdt_is_open;
>  
> @@ -63,373 +66,6 @@ static const struct pci_device_id hpwdt_devices[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
>  
> -#ifdef CONFIG_HPWDT_NMI_DECODING
> -#define PCI_BIOS32_SD_VALUE		0x5F32335F	/* "_32_" */
> -#define CRU_BIOS_SIGNATURE_VALUE	0x55524324
> -#define PCI_BIOS32_PARAGRAPH_LEN	16
> -#define PCI_ROM_BASE1			0x000F0000
> -#define ROM_SIZE			0x10000
> -
> -struct bios32_service_dir {
> -	u32 signature;
> -	u32 entry_point;
> -	u8 revision;
> -	u8 length;
> -	u8 checksum;
> -	u8 reserved[5];
> -};
> -
> -/* type 212 */
> -struct smbios_cru64_info {
> -	u8 type;
> -	u8 byte_length;
> -	u16 handle;
> -	u32 signature;
> -	u64 physical_address;
> -	u32 double_length;
> -	u32 double_offset;
> -};
> -#define SMBIOS_CRU64_INFORMATION	212
> -
> -/* type 219 */
> -struct smbios_proliant_info {
> -	u8 type;
> -	u8 byte_length;
> -	u16 handle;
> -	u32 power_features;
> -	u32 omega_features;
> -	u32 reserved;
> -	u32 misc_features;
> -};
> -#define SMBIOS_ICRU_INFORMATION		219
> -
> -
> -struct cmn_registers {
> -	union {
> -		struct {
> -			u8 ral;
> -			u8 rah;
> -			u16 rea2;
> -		};
> -		u32 reax;
> -	} u1;
> -	union {
> -		struct {
> -			u8 rbl;
> -			u8 rbh;
> -			u8 reb2l;
> -			u8 reb2h;
> -		};
> -		u32 rebx;
> -	} u2;
> -	union {
> -		struct {
> -			u8 rcl;
> -			u8 rch;
> -			u16 rec2;
> -		};
> -		u32 recx;
> -	} u3;
> -	union {
> -		struct {
> -			u8 rdl;
> -			u8 rdh;
> -			u16 red2;
> -		};
> -		u32 redx;
> -	} u4;
> -
> -	u32 resi;
> -	u32 redi;
> -	u16 rds;
> -	u16 res;
> -	u32 reflags;
> -}  __attribute__((packed));
> -
> -static unsigned int hpwdt_nmi_decoding;
> -static unsigned int allow_kdump = 1;
> -static unsigned int is_icru;
> -static unsigned int is_uefi;
> -static DEFINE_SPINLOCK(rom_lock);
> -static void *cru_rom_addr;
> -static struct cmn_registers cmn_regs;
> -
> -extern asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
> -						unsigned long *pRomEntry);
> -
> -#ifdef CONFIG_X86_32
> -/* --32 Bit Bios------------------------------------------------------------ */
> -
> -#define HPWDT_ARCH	32
> -
> -asm(".text                          \n\t"
> -    ".align 4                       \n\t"
> -    ".globl asminline_call	    \n"
> -    "asminline_call:                \n\t"
> -    "pushl       %ebp               \n\t"
> -    "movl        %esp, %ebp         \n\t"
> -    "pusha                          \n\t"
> -    "pushf                          \n\t"
> -    "push        %es                \n\t"
> -    "push        %ds                \n\t"
> -    "pop         %es                \n\t"
> -    "movl        8(%ebp),%eax       \n\t"
> -    "movl        4(%eax),%ebx       \n\t"
> -    "movl        8(%eax),%ecx       \n\t"
> -    "movl        12(%eax),%edx      \n\t"
> -    "movl        16(%eax),%esi      \n\t"
> -    "movl        20(%eax),%edi      \n\t"
> -    "movl        (%eax),%eax        \n\t"
> -    "push        %cs                \n\t"
> -    "call        *12(%ebp)          \n\t"
> -    "pushf                          \n\t"
> -    "pushl       %eax               \n\t"
> -    "movl        8(%ebp),%eax       \n\t"
> -    "movl        %ebx,4(%eax)       \n\t"
> -    "movl        %ecx,8(%eax)       \n\t"
> -    "movl        %edx,12(%eax)      \n\t"
> -    "movl        %esi,16(%eax)      \n\t"
> -    "movl        %edi,20(%eax)      \n\t"
> -    "movw        %ds,24(%eax)       \n\t"
> -    "movw        %es,26(%eax)       \n\t"
> -    "popl        %ebx               \n\t"
> -    "movl        %ebx,(%eax)        \n\t"
> -    "popl        %ebx               \n\t"
> -    "movl        %ebx,28(%eax)      \n\t"
> -    "pop         %es                \n\t"
> -    "popf                           \n\t"
> -    "popa                           \n\t"
> -    "leave                          \n\t"
> -    "ret                            \n\t"
> -    ".previous");
> -
> -
> -/*
> - *	cru_detect
> - *
> - *	Routine Description:
> - *	This function uses the 32-bit BIOS Service Directory record to
> - *	search for a $CRU record.
> - *
> - *	Return Value:
> - *	0        :  SUCCESS
> - *	<0       :  FAILURE
> - */
> -static int cru_detect(unsigned long map_entry,
> -	unsigned long map_offset)
> -{
> -	void *bios32_map;
> -	unsigned long *bios32_entrypoint;
> -	unsigned long cru_physical_address;
> -	unsigned long cru_length;
> -	unsigned long physical_bios_base = 0;
> -	unsigned long physical_bios_offset = 0;
> -	int retval = -ENODEV;
> -
> -	bios32_map = ioremap(map_entry, (2 * PAGE_SIZE));
> -
> -	if (bios32_map == NULL)
> -		return -ENODEV;
> -
> -	bios32_entrypoint = bios32_map + map_offset;
> -
> -	cmn_regs.u1.reax = CRU_BIOS_SIGNATURE_VALUE;
> -
> -	set_memory_x((unsigned long)bios32_map, 2);
> -	asminline_call(&cmn_regs, bios32_entrypoint);
> -
> -	if (cmn_regs.u1.ral != 0) {
> -		pr_warn("Call succeeded but with an error: 0x%x\n",
> -			cmn_regs.u1.ral);
> -	} else {
> -		physical_bios_base = cmn_regs.u2.rebx;
> -		physical_bios_offset = cmn_regs.u4.redx;
> -		cru_length = cmn_regs.u3.recx;
> -		cru_physical_address =
> -			physical_bios_base + physical_bios_offset;
> -
> -		/* If the values look OK, then map it in. */
> -		if ((physical_bios_base + physical_bios_offset)) {
> -			cru_rom_addr =
> -				ioremap(cru_physical_address, cru_length);
> -			if (cru_rom_addr) {
> -				set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK,
> -					(cru_length + PAGE_SIZE - 1) >> PAGE_SHIFT);
> -				retval = 0;
> -			}
> -		}
> -
> -		pr_debug("CRU Base Address:   0x%lx\n", physical_bios_base);
> -		pr_debug("CRU Offset Address: 0x%lx\n", physical_bios_offset);
> -		pr_debug("CRU Length:         0x%lx\n", cru_length);
> -		pr_debug("CRU Mapped Address: %p\n", &cru_rom_addr);
> -	}
> -	iounmap(bios32_map);
> -	return retval;
> -}
> -
> -/*
> - *	bios_checksum
> - */
> -static int bios_checksum(const char __iomem *ptr, int len)
> -{
> -	char sum = 0;
> -	int i;
> -
> -	/*
> -	 * calculate checksum of size bytes. This should add up
> -	 * to zero if we have a valid header.
> -	 */
> -	for (i = 0; i < len; i++)
> -		sum += ptr[i];
> -
> -	return ((sum == 0) && (len > 0));
> -}
> -
> -/*
> - *	bios32_present
> - *
> - *	Routine Description:
> - *	This function finds the 32-bit BIOS Service Directory
> - *
> - *	Return Value:
> - *	0        :  SUCCESS
> - *	<0       :  FAILURE
> - */
> -static int bios32_present(const char __iomem *p)
> -{
> -	struct bios32_service_dir *bios_32_ptr;
> -	int length;
> -	unsigned long map_entry, map_offset;
> -
> -	bios_32_ptr = (struct bios32_service_dir *) p;
> -
> -	/*
> -	 * Search for signature by checking equal to the swizzled value
> -	 * instead of calling another routine to perform a strcmp.
> -	 */
> -	if (bios_32_ptr->signature == PCI_BIOS32_SD_VALUE) {
> -		length = bios_32_ptr->length * PCI_BIOS32_PARAGRAPH_LEN;
> -		if (bios_checksum(p, length)) {
> -			/*
> -			 * According to the spec, we're looking for the
> -			 * first 4KB-aligned address below the entrypoint
> -			 * listed in the header. The Service Directory code
> -			 * is guaranteed to occupy no more than 2 4KB pages.
> -			 */
> -			map_entry = bios_32_ptr->entry_point & ~(PAGE_SIZE - 1);
> -			map_offset = bios_32_ptr->entry_point - map_entry;
> -
> -			return cru_detect(map_entry, map_offset);
> -		}
> -	}
> -	return -ENODEV;
> -}
> -
> -static int detect_cru_service(void)
> -{
> -	char __iomem *p, *q;
> -	int rc = -1;
> -
> -	/*
> -	 * Search from 0x0f0000 through 0x0fffff, inclusive.
> -	 */
> -	p = ioremap(PCI_ROM_BASE1, ROM_SIZE);
> -	if (p == NULL)
> -		return -ENOMEM;
> -
> -	for (q = p; q < p + ROM_SIZE; q += 16) {
> -		rc = bios32_present(q);
> -		if (!rc)
> -			break;
> -	}
> -	iounmap(p);
> -	return rc;
> -}
> -/* ------------------------------------------------------------------------- */
> -#endif /* CONFIG_X86_32 */
> -#ifdef CONFIG_X86_64
> -/* --64 Bit Bios------------------------------------------------------------ */
> -
> -#define HPWDT_ARCH	64
> -
> -asm(".text                      \n\t"
> -    ".align 4                   \n\t"
> -    ".globl asminline_call	\n\t"
> -    ".type asminline_call, @function \n\t"
> -    "asminline_call:            \n\t"
> -    FRAME_BEGIN
> -    "pushq      %rax            \n\t"
> -    "pushq      %rbx            \n\t"
> -    "pushq      %rdx            \n\t"
> -    "pushq      %r12            \n\t"
> -    "pushq      %r9             \n\t"
> -    "movq       %rsi, %r12      \n\t"
> -    "movq       %rdi, %r9       \n\t"
> -    "movl       4(%r9),%ebx     \n\t"
> -    "movl       8(%r9),%ecx     \n\t"
> -    "movl       12(%r9),%edx    \n\t"
> -    "movl       16(%r9),%esi    \n\t"
> -    "movl       20(%r9),%edi    \n\t"
> -    "movl       (%r9),%eax      \n\t"
> -    "call       *%r12           \n\t"
> -    "pushfq                     \n\t"
> -    "popq        %r12           \n\t"
> -    "movl       %eax, (%r9)     \n\t"
> -    "movl       %ebx, 4(%r9)    \n\t"
> -    "movl       %ecx, 8(%r9)    \n\t"
> -    "movl       %edx, 12(%r9)   \n\t"
> -    "movl       %esi, 16(%r9)   \n\t"
> -    "movl       %edi, 20(%r9)   \n\t"
> -    "movq       %r12, %rax      \n\t"
> -    "movl       %eax, 28(%r9)   \n\t"
> -    "popq       %r9             \n\t"
> -    "popq       %r12            \n\t"
> -    "popq       %rdx            \n\t"
> -    "popq       %rbx            \n\t"
> -    "popq       %rax            \n\t"
> -    FRAME_END
> -    "ret                        \n\t"
> -    ".previous");
> -
> -/*
> - *	dmi_find_cru
> - *
> - *	Routine Description:
> - *	This function checks whether or not a SMBIOS/DMI record is
> - *	the 64bit CRU info or not
> - */
> -static void dmi_find_cru(const struct dmi_header *dm, void *dummy)
> -{
> -	struct smbios_cru64_info *smbios_cru64_ptr;
> -	unsigned long cru_physical_address;
> -
> -	if (dm->type == SMBIOS_CRU64_INFORMATION) {
> -		smbios_cru64_ptr = (struct smbios_cru64_info *) dm;
> -		if (smbios_cru64_ptr->signature == CRU_BIOS_SIGNATURE_VALUE) {
> -			cru_physical_address =
> -				smbios_cru64_ptr->physical_address +
> -				smbios_cru64_ptr->double_offset;
> -			cru_rom_addr = ioremap(cru_physical_address,
> -				smbios_cru64_ptr->double_length);
> -			set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK,
> -				smbios_cru64_ptr->double_length >> PAGE_SHIFT);
> -		}
> -	}
> -}
> -
> -static int detect_cru_service(void)
> -{
> -	cru_rom_addr = NULL;
> -
> -	dmi_walk(dmi_find_cru, NULL);
> -
> -	/* if cru_rom_addr has been set then we found a CRU service */
> -	return ((cru_rom_addr != NULL) ? 0 : -ENODEV);
> -}
> -/* ------------------------------------------------------------------------- */
> -#endif /* CONFIG_X86_64 */
> -#endif /* CONFIG_HPWDT_NMI_DECODING */
>  
>  /*
>   *	Watchdog operations
> @@ -486,30 +122,12 @@ static int hpwdt_my_nmi(void)
>   */
>  static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>  {
> -	unsigned long rom_pl;
> -	static int die_nmi_called;
> -
> -	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);
> -	die_nmi_called = 1;
> -	spin_unlock_irqrestore(&rom_lock, rom_pl);
> -
>  	if (allow_kdump)
>  		hpwdt_stop();
>  
> -	if (!is_icru && !is_uefi) {
> -		if (cmn_regs.u1.ral == 0) {
> -			nmi_panic(regs, "An NMI occurred, but unable to determine source.\n");
> -			return NMI_HANDLED;
> -		}
> -	}
>  	nmi_panic(regs, "An NMI occurred. Depending on your system the reason "
>  		"for the NMI is logged in any one of the following "
>  		"resources:\n"
> @@ -675,84 +293,11 @@ static struct miscdevice hpwdt_miscdev = {
>   *	Init & Exit
>   */
>  
> -#ifdef CONFIG_HPWDT_NMI_DECODING
> -#ifdef CONFIG_X86_LOCAL_APIC
> -static void hpwdt_check_nmi_decoding(struct pci_dev *dev)
> -{
> -	/*
> -	 * If nmi_watchdog is turned off then we can turn on
> -	 * our nmi decoding capability.
> -	 */
> -	hpwdt_nmi_decoding = 1;
> -}
> -#else
> -static void hpwdt_check_nmi_decoding(struct pci_dev *dev)
> -{
> -	dev_warn(&dev->dev, "NMI decoding is disabled. "
> -		"Your kernel does not support a NMI Watchdog.\n");
> -}
> -#endif /* CONFIG_X86_LOCAL_APIC */
> -
> -/*
> - *	dmi_find_icru
> - *
> - *	Routine Description:
> - *	This function checks whether or not we are on an iCRU-based server.
> - *	This check is independent of architecture and needs to be made for
> - *	any ProLiant system.
> - */
> -static void dmi_find_icru(const struct dmi_header *dm, void *dummy)
> -{
> -	struct smbios_proliant_info *smbios_proliant_ptr;
> -
> -	if (dm->type == SMBIOS_ICRU_INFORMATION) {
> -		smbios_proliant_ptr = (struct smbios_proliant_info *) dm;
> -		if (smbios_proliant_ptr->misc_features & 0x01)
> -			is_icru = 1;
> -		if (smbios_proliant_ptr->misc_features & 0x1400)
> -			is_uefi = 1;
> -	}
> -}
>  
>  static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  {
> +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
>  	int retval;
> -
> -	/*
> -	 * On typical CRU-based systems we need to map that service in
> -	 * the BIOS. For 32 bit Operating Systems we need to go through
> -	 * the 32 Bit BIOS Service Directory. For 64 bit Operating
> -	 * Systems we get that service through SMBIOS.
> -	 *
> -	 * On systems that support the new iCRU service all we need to
> -	 * do is call dmi_walk to get the supported flag value and skip
> -	 * the old cru detect code.
> -	 */
> -	dmi_walk(dmi_find_icru, NULL);
> -	if (!is_icru && !is_uefi) {
> -
> -		/*
> -		* We need to map the ROM to get the CRU service.
> -		* For 32 bit Operating Systems we need to go through the 32 Bit
> -		* BIOS Service Directory
> -		* For 64 bit Operating Systems we get that service through SMBIOS.
> -		*/
> -		retval = detect_cru_service();
> -		if (retval < 0) {
> -			dev_warn(&dev->dev,
> -				"Unable to detect the %d Bit CRU Service.\n",
> -				HPWDT_ARCH);
> -			return retval;
> -		}
> -
> -		/*
> -		* We know this is the only CRU call we need to make so lets keep as
> -		* few instructions as possible once the NMI comes in.
> -		*/
> -		cmn_regs.u1.rah = 0x0D;
> -		cmn_regs.u1.ral = 0x02;
> -	}
> -
>  	/*
>  	 * Only one function can register for NMI_UNKNOWN
>  	 */
> @@ -780,33 +325,19 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  	dev_warn(&dev->dev,
>  		"Unable to register a die notifier (err=%d).\n",
>  		retval);
> -	if (cru_rom_addr)
> -		iounmap(cru_rom_addr);
>  	return retval;
> +#endif					/* } */
> +	return 0;
>  }
>  
>  static void hpwdt_exit_nmi_decoding(void)
>  {
> +#ifdef CONFIG_HPWDT_NMI_DECODING
>  	unregister_nmi_handler(NMI_UNKNOWN, "hpwdt");
>  	unregister_nmi_handler(NMI_SERR, "hpwdt");
>  	unregister_nmi_handler(NMI_IO_CHECK, "hpwdt");
> -	if (cru_rom_addr)
> -		iounmap(cru_rom_addr);
> +#endif
>  }
> -#else /* !CONFIG_HPWDT_NMI_DECODING */
> -static void hpwdt_check_nmi_decoding(struct pci_dev *dev)
> -{
> -}
> -
> -static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
> -{
> -	return 0;
> -}
> -
> -static void hpwdt_exit_nmi_decoding(void)
> -{
> -}
> -#endif /* CONFIG_HPWDT_NMI_DECODING */
>  
>  static int hpwdt_init_one(struct pci_dev *dev,
>  					const struct pci_device_id *ent)
> @@ -814,11 +345,6 @@ static int hpwdt_init_one(struct pci_dev *dev,
>  	int retval;
>  
>  	/*
> -	 * Check if we can do NMI decoding or not
> -	 */
> -	hpwdt_check_nmi_decoding(dev);
> -
> -	/*
>  	 * First let's find out if we are on an iLO2+ server. We will
>  	 * not run on a legacy ASM box.
>  	 * So we only support the G5 ProLiant servers and higher.

[PATCH v3 02/11] watchdog/hpwdt: remove include files no longer needed.

[Jerry Hoemann]

Remove header files used by NMI sourcing and DMI decoding.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 113058644fc3..20a13c5d0285 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -28,16 +28,7 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <linux/watchdog.h>
-#ifdef CONFIG_HPWDT_NMI_DECODING
-#include <linux/dmi.h>
-#include <linux/spinlock.h>
-#include <linux/nmi.h>
-#include <linux/kdebug.h>
-#include <linux/notifier.h>
-#include <asm/set_memory.h>
-#endif /* CONFIG_HPWDT_NMI_DECODING */
 #include <asm/nmi.h>
-#include <asm/frame.h>
 
 #define HPWDT_VERSION			"1.4.0"
 #define SECS_TO_TICKS(secs)		((secs) * 1000 / 128)
-- 
2.13.6

Re: [v3,02/11] watchdog/hpwdt: remove include files no longer needed.

[Guenter Roeck]

On Thu, Feb 15, 2018 at 04:43:51PM -0700, Jerry Hoemann wrote:
> Remove header files used by NMI sourcing and DMI decoding.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

This patch should have been merged with the 1st patch of the series,
since it is logically the same change. However, that does not raise
to the level of objection.

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

> ---
>  drivers/watchdog/hpwdt.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 113058644fc3..20a13c5d0285 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -28,16 +28,7 @@
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <linux/watchdog.h>
> -#ifdef CONFIG_HPWDT_NMI_DECODING
> -#include <linux/dmi.h>
> -#include <linux/spinlock.h>
> -#include <linux/nmi.h>
> -#include <linux/kdebug.h>
> -#include <linux/notifier.h>
> -#include <asm/set_memory.h>
> -#endif /* CONFIG_HPWDT_NMI_DECODING */
>  #include <asm/nmi.h>
> -#include <asm/frame.h>
>  
>  #define HPWDT_VERSION			"1.4.0"
>  #define SECS_TO_TICKS(secs)		((secs) * 1000 / 128)

[PATCH v3 03/11] watchdog/hpwdt: Update nmi_panic message.

[Jerry Hoemann]

Include the nmistat in the nmi_panic message to give support
an indication why the NMI was called (e.g. a timeout or generate
nmi button.)

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 20a13c5d0285..07810caabf74 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -113,19 +113,23 @@ static int hpwdt_my_nmi(void)
  */
 static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 {
-	if ((ulReason == NMI_UNKNOWN) && !hpwdt_my_nmi())
+	unsigned int mynmi = hpwdt_my_nmi();
+	static char panic_msg[] =
+		"00: An NMI occurred. Depending on your system the reason "
+		"for the NMI is logged in any one of the following resources:\n"
+		"1. Integrated Management Log (IML)\n"
+		"2. OA Syslog\n"
+		"3. OA Forward Progress Log\n"
+		"4. iLO Event Log";
+
+	if ((ulReason == NMI_UNKNOWN) && !mynmi)
 		return NMI_DONE;
 
 	if (allow_kdump)
 		hpwdt_stop();
 
-	nmi_panic(regs, "An NMI occurred. Depending on your system the reason "
-		"for the NMI is logged in any one of the following "
-		"resources:\n"
-		"1. Integrated Management Log (IML)\n"
-		"2. OA Syslog\n"
-		"3. OA Forward Progress Log\n"
-		"4. iLO Event Log");
+	hex_byte_pack(panic_msg, mynmi);
+	nmi_panic(regs, panic_msg);
 
 	return NMI_HANDLED;
 }
-- 
2.13.6

Re: [v3,03/11] watchdog/hpwdt: Update nmi_panic message.

[Guenter Roeck]

On Thu, Feb 15, 2018 at 04:43:52PM -0700, Jerry Hoemann wrote:
> Include the nmistat in the nmi_panic message to give support
> an indication why the NMI was called (e.g. a timeout or generate
> nmi button.)
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/watchdog/hpwdt.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 20a13c5d0285..07810caabf74 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -113,19 +113,23 @@ static int hpwdt_my_nmi(void)
>   */
>  static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>  {
> -	if ((ulReason == NMI_UNKNOWN) && !hpwdt_my_nmi())
> +	unsigned int mynmi = hpwdt_my_nmi();
> +	static char panic_msg[] =
> +		"00: An NMI occurred. Depending on your system the reason "
> +		"for the NMI is logged in any one of the following resources:\n"
> +		"1. Integrated Management Log (IML)\n"
> +		"2. OA Syslog\n"
> +		"3. OA Forward Progress Log\n"
> +		"4. iLO Event Log";
> +
> +	if ((ulReason == NMI_UNKNOWN) && !mynmi)

As mentioned before, I won't accept patches with unnecessary ( ).
Deferring to Wim.

Guenter

>  		return NMI_DONE;
>  
>  	if (allow_kdump)
>  		hpwdt_stop();
>  
> -	nmi_panic(regs, "An NMI occurred. Depending on your system the reason "
> -		"for the NMI is logged in any one of the following "
> -		"resources:\n"
> -		"1. Integrated Management Log (IML)\n"
> -		"2. OA Syslog\n"
> -		"3. OA Forward Progress Log\n"
> -		"4. iLO Event Log");
> +	hex_byte_pack(panic_msg, mynmi);
> +	nmi_panic(regs, panic_msg);
>  
>  	return NMI_HANDLED;
>  }

[PATCH v3 04/11] watchdog/hpwdt: white space changes

[Jerry Hoemann]

Minor white space changes and some name clean up.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 49 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 07810caabf74..d5989acf3e37 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -1,11 +1,9 @@
 /*
  *	HPE WatchDog Driver
- *	based on
  *
- *	SoftDog	0.05:	A Software Watchdog Device
- *
- *	(c) Copyright 2015 Hewlett Packard Enterprise Development LP
+ *	(c) Copyright 2018 Hewlett Packard Enterprise Development LP
  *	Thomas Mingarelli <thomas.mingarelli@hpe.com>
+ *	Jerry Hoemann <jerry.hoemann@hpe.com>
  *
  *	This program is free software; you can redistribute it and/or
  *	modify it under the terms of the GNU General Public License
@@ -53,7 +51,7 @@ static unsigned long __iomem *hpwdt_timer_con;
 static const struct pci_device_id hpwdt_devices[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_COMPAQ, 0xB203) },	/* iLO2 */
 	{ PCI_DEVICE(PCI_VENDOR_ID_HP, 0x3306) },	/* iLO3 */
-	{0},			/* terminate list */
+	{0},						/* terminate list */
 };
 MODULE_DEVICE_TABLE(pci, hpwdt_devices);
 
@@ -102,7 +100,7 @@ static int hpwdt_time_left(void)
 	return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
 }
 
-#ifdef CONFIG_HPWDT_NMI_DECODING
+#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
 static int hpwdt_my_nmi(void)
 {
 	return ioread8(hpwdt_nmistat) & 0x6;
@@ -133,7 +131,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 
 	return NMI_HANDLED;
 }
-#endif /* CONFIG_HPWDT_NMI_DECODING */
+#endif					/* } */
 
 /*
  *	/dev/watchdog handling
@@ -198,11 +196,11 @@ static ssize_t hpwdt_write(struct file *file, const char __user *data,
 	return len;
 }
 
-static const struct watchdog_info ident = {
-	.options = WDIOF_SETTIMEOUT |
-		   WDIOF_KEEPALIVEPING |
-		   WDIOF_MAGICCLOSE,
-	.identity = "HPE iLO2+ HW Watchdog Timer",
+static const struct watchdog_info hpwdt_info = {
+	.options	= WDIOF_SETTIMEOUT    |
+			  WDIOF_KEEPALIVEPING |
+			  WDIOF_MAGICCLOSE,
+	.identity	= "HPE iLO2+ HW Watchdog Timer",
 };
 
 static long hpwdt_ioctl(struct file *file, unsigned int cmd,
@@ -216,7 +214,7 @@ static long hpwdt_ioctl(struct file *file, unsigned int cmd,
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
 		ret = 0;
-		if (copy_to_user(argp, &ident, sizeof(ident)))
+		if (copy_to_user(argp, &hpwdt_info, sizeof(hpwdt_info)))
 			ret = -EFAULT;
 		break;
 
@@ -313,7 +311,7 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
 	return 0;
 
 error2:
-	unregister_nmi_handler(NMI_SERR, "hpwdt");
+	unregister_nmi_handler(NMI_SERR,    "hpwdt");
 error1:
 	unregister_nmi_handler(NMI_UNKNOWN, "hpwdt");
 error:
@@ -327,15 +325,14 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
 
 static void hpwdt_exit_nmi_decoding(void)
 {
-#ifdef CONFIG_HPWDT_NMI_DECODING
-	unregister_nmi_handler(NMI_UNKNOWN, "hpwdt");
-	unregister_nmi_handler(NMI_SERR, "hpwdt");
+#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
+	unregister_nmi_handler(NMI_UNKNOWN,  "hpwdt");
+	unregister_nmi_handler(NMI_SERR,     "hpwdt");
 	unregister_nmi_handler(NMI_IO_CHECK, "hpwdt");
-#endif
+#endif					/* } */
 }
 
-static int hpwdt_init_one(struct pci_dev *dev,
-					const struct pci_device_id *ent)
+static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 {
 	int retval;
 
@@ -422,10 +419,10 @@ static void hpwdt_exit(struct pci_dev *dev)
 }
 
 static struct pci_driver hpwdt_driver = {
-	.name = "hpwdt",
-	.id_table = hpwdt_devices,
-	.probe = hpwdt_init_one,
-	.remove = hpwdt_exit,
+	.name		= "hpwdt",
+	.id_table	= hpwdt_devices,
+	.probe		= hpwdt_probe,
+	.remove		= hpwdt_exit,
 };
 
 MODULE_AUTHOR("Tom Mingarelli");
@@ -440,9 +437,9 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 		__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-#ifdef CONFIG_HPWDT_NMI_DECODING
+#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
 module_param(allow_kdump, int, 0);
 MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
-#endif /* !CONFIG_HPWDT_NMI_DECODING */
+#endif					/* } */
 
 module_pci_driver(hpwdt_driver);
-- 
2.13.6

Re: [v3,04/11] watchdog/hpwdt: white space changes

[Guenter Roeck]

On Thu, Feb 15, 2018 at 04:43:53PM -0700, Jerry Hoemann wrote:
> Minor white space changes and some name clean up.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/watchdog/hpwdt.c | 49 +++++++++++++++++++++++-------------------------
>  1 file changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 07810caabf74..d5989acf3e37 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -1,11 +1,9 @@
>  /*
>   *	HPE WatchDog Driver
> - *	based on
>   *
> - *	SoftDog	0.05:	A Software Watchdog Device
> - *
> - *	(c) Copyright 2015 Hewlett Packard Enterprise Development LP
> + *	(c) Copyright 2018 Hewlett Packard Enterprise Development LP
>   *	Thomas Mingarelli <thomas.mingarelli@hpe.com>
> + *	Jerry Hoemann <jerry.hoemann@hpe.com>
>   *
>   *	This program is free software; you can redistribute it and/or
>   *	modify it under the terms of the GNU General Public License
> @@ -53,7 +51,7 @@ static unsigned long __iomem *hpwdt_timer_con;
>  static const struct pci_device_id hpwdt_devices[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_COMPAQ, 0xB203) },	/* iLO2 */
>  	{ PCI_DEVICE(PCI_VENDOR_ID_HP, 0x3306) },	/* iLO3 */
> -	{0},			/* terminate list */
> +	{0},						/* terminate list */
>  };
>  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
>  
> @@ -102,7 +100,7 @@ static int hpwdt_time_left(void)
>  	return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
>  }
>  
> -#ifdef CONFIG_HPWDT_NMI_DECODING
> +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
>  static int hpwdt_my_nmi(void)
>  {
>  	return ioread8(hpwdt_nmistat) & 0x6;
> @@ -133,7 +131,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>  
>  	return NMI_HANDLED;
>  }
> -#endif /* CONFIG_HPWDT_NMI_DECODING */
> +#endif					/* } */

I disagree with those changes. While I don't object to adding the '{'
per se, I find it very useful to have the 'CONFIG_HPWDT_NMI_DECODING'
with an endif to be able to associate it with the matching #ifdef.

Deferring to Wim.

Guenter

>  
>  /*
>   *	/dev/watchdog handling
> @@ -198,11 +196,11 @@ static ssize_t hpwdt_write(struct file *file, const char __user *data,
>  	return len;
>  }
>  
> -static const struct watchdog_info ident = {
> -	.options = WDIOF_SETTIMEOUT |
> -		   WDIOF_KEEPALIVEPING |
> -		   WDIOF_MAGICCLOSE,
> -	.identity = "HPE iLO2+ HW Watchdog Timer",
> +static const struct watchdog_info hpwdt_info = {
> +	.options	= WDIOF_SETTIMEOUT    |
> +			  WDIOF_KEEPALIVEPING |
> +			  WDIOF_MAGICCLOSE,
> +	.identity	= "HPE iLO2+ HW Watchdog Timer",
>  };
>  
>  static long hpwdt_ioctl(struct file *file, unsigned int cmd,
> @@ -216,7 +214,7 @@ static long hpwdt_ioctl(struct file *file, unsigned int cmd,
>  	switch (cmd) {
>  	case WDIOC_GETSUPPORT:
>  		ret = 0;
> -		if (copy_to_user(argp, &ident, sizeof(ident)))
> +		if (copy_to_user(argp, &hpwdt_info, sizeof(hpwdt_info)))
>  			ret = -EFAULT;
>  		break;
>  
> @@ -313,7 +311,7 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  	return 0;
>  
>  error2:
> -	unregister_nmi_handler(NMI_SERR, "hpwdt");
> +	unregister_nmi_handler(NMI_SERR,    "hpwdt");
>  error1:
>  	unregister_nmi_handler(NMI_UNKNOWN, "hpwdt");
>  error:
> @@ -327,15 +325,14 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  
>  static void hpwdt_exit_nmi_decoding(void)
>  {
> -#ifdef CONFIG_HPWDT_NMI_DECODING
> -	unregister_nmi_handler(NMI_UNKNOWN, "hpwdt");
> -	unregister_nmi_handler(NMI_SERR, "hpwdt");
> +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> +	unregister_nmi_handler(NMI_UNKNOWN,  "hpwdt");
> +	unregister_nmi_handler(NMI_SERR,     "hpwdt");
>  	unregister_nmi_handler(NMI_IO_CHECK, "hpwdt");
> -#endif
> +#endif					/* } */
>  }
>  
> -static int hpwdt_init_one(struct pci_dev *dev,
> -					const struct pci_device_id *ent)
> +static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
>  {
>  	int retval;
>  
> @@ -422,10 +419,10 @@ static void hpwdt_exit(struct pci_dev *dev)
>  }
>  
>  static struct pci_driver hpwdt_driver = {
> -	.name = "hpwdt",
> -	.id_table = hpwdt_devices,
> -	.probe = hpwdt_init_one,
> -	.remove = hpwdt_exit,
> +	.name		= "hpwdt",
> +	.id_table	= hpwdt_devices,
> +	.probe		= hpwdt_probe,
> +	.remove		= hpwdt_exit,
>  };
>  
>  MODULE_AUTHOR("Tom Mingarelli");
> @@ -440,9 +437,9 @@ module_param(nowayout, bool, 0);
>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>  		__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> -#ifdef CONFIG_HPWDT_NMI_DECODING
> +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
>  module_param(allow_kdump, int, 0);
>  MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
> -#endif /* !CONFIG_HPWDT_NMI_DECODING */
> +#endif					/* } */
>  
>  module_pci_driver(hpwdt_driver);

Re: [v3,04/11] watchdog/hpwdt: white space changes

[Jerry Hoemann]

On Sat, Feb 17, 2018 at 08:17:34AM -0800, Guenter Roeck wrote:
> On Thu, Feb 15, 2018 at 04:43:53PM -0700, Jerry Hoemann wrote:
> > Minor white space changes and some name clean up.
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
> >  
> > @@ -102,7 +100,7 @@ static int hpwdt_time_left(void)
> >  	return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
> >  }
> >  
> > -#ifdef CONFIG_HPWDT_NMI_DECODING
> > +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> >  static int hpwdt_my_nmi(void)
> >  {
> >  	return ioread8(hpwdt_nmistat) & 0x6;
> > @@ -133,7 +131,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
> >  
> >  	return NMI_HANDLED;
> >  }
> > -#endif /* CONFIG_HPWDT_NMI_DECODING */
> > +#endif					/* } */
> 
> I disagree with those changes. While I don't object to adding the '{'
> per se, I find it very useful to have the 'CONFIG_HPWDT_NMI_DECODING'
> with an endif to be able to associate it with the matching #ifdef.

The matching /* { */ and /* } */ allow for quickly the finding of the
matching ifdef/endif.

In the "vim" editor, the command '%' will take one from one curly paren to its
matching curly paren...

There is a similar sequence for emacs.

> 
> Deferring to Wim.
> 
> Guenter
> 

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [v3,04/11] watchdog/hpwdt: white space changes

[Marcus Folkesson]

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

On Sat, Feb 17, 2018 at 12:32:08PM -0700, Jerry Hoemann wrote:
> On Sat, Feb 17, 2018 at 08:17:34AM -0800, Guenter Roeck wrote:
> > On Thu, Feb 15, 2018 at 04:43:53PM -0700, Jerry Hoemann wrote:
> > > Minor white space changes and some name clean up.
> > > 
> > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > > ---
> > >  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
> > >  
> > > @@ -102,7 +100,7 @@ static int hpwdt_time_left(void)
> > >  	return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
> > >  }
> > >  
> > > -#ifdef CONFIG_HPWDT_NMI_DECODING
> > > +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> > >  static int hpwdt_my_nmi(void)
> > >  {
> > >  	return ioread8(hpwdt_nmistat) & 0x6;
> > > @@ -133,7 +131,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
> > >  
> > >  	return NMI_HANDLED;
> > >  }
> > > -#endif /* CONFIG_HPWDT_NMI_DECODING */
> > > +#endif					/* } */
> > 
> > I disagree with those changes. While I don't object to adding the '{'
> > per se, I find it very useful to have the 'CONFIG_HPWDT_NMI_DECODING'
> > with an endif to be able to associate it with the matching #ifdef.

Well, it does not follow our coding style.

Documentation/process/coding-style.rst:

	At the end of any non-trivial #if or #ifdef block (more than a few lines),
	place a comment after the #endif on the same line, noting the conditional
	expression used.  For instance:

	.. code-block:: c

		#ifdef CONFIG_SOMETHING
		...
		#endif /* CONFIG_SOMETHING */

> 
> The matching /* { */ and /* } */ allow for quickly the finding of the
> matching ifdef/endif.
> 
> In the "vim" editor, the command '%' will take one from one curly paren to its
> matching curly paren...

'%' in vim let you jump between matching ifdef/endif as well.

> 
> There is a similar sequence for emacs.
> 
> > 
> > Deferring to Wim.
> > 
> > Guenter
> > 
> 
> -- 
> 
> -----------------------------------------------------------------------------
> Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
> -----------------------------------------------------------------------------

Thanks,
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [v3,04/11] watchdog/hpwdt: white space changes

[Jerry Hoemann]

On Sat, Feb 17, 2018 at 09:27:53PM +0100, Marcus Folkesson wrote:
> On Sat, Feb 17, 2018 at 12:32:08PM -0700, Jerry Hoemann wrote:
> > On Sat, Feb 17, 2018 at 08:17:34AM -0800, Guenter Roeck wrote:
> > > On Thu, Feb 15, 2018 at 04:43:53PM -0700, Jerry Hoemann wrote:
> > > > Minor white space changes and some name clean up.
> > > > 
> > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > > > ---
> > > >  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
> > > >  
> > > > @@ -102,7 +100,7 @@ static int hpwdt_time_left(void)
> > > >  	return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
> > > >  }
> > > >  
> > > > -#ifdef CONFIG_HPWDT_NMI_DECODING
> > > > +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> > > >  static int hpwdt_my_nmi(void)
> > > >  {
> > > >  	return ioread8(hpwdt_nmistat) & 0x6;
> > > > @@ -133,7 +131,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
> > > >  
> > > >  	return NMI_HANDLED;
> > > >  }
> > > > -#endif /* CONFIG_HPWDT_NMI_DECODING */
> > > > +#endif					/* } */
> > > 
> > > I disagree with those changes. While I don't object to adding the '{'
> > > per se, I find it very useful to have the 'CONFIG_HPWDT_NMI_DECODING'
> > > with an endif to be able to associate it with the matching #ifdef.
> 
> Well, it does not follow our coding style.
> 
> Documentation/process/coding-style.rst:
> 
> 	At the end of any non-trivial #if or #ifdef block (more than a few lines),
> 	place a comment after the #endif on the same line, noting the conditional
> 	expression used.  For instance:
> 
> 	.. code-block:: c
> 
> 		#ifdef CONFIG_SOMETHING
> 		...
> 		#endif /* CONFIG_SOMETHING */
> 
> > 
> > The matching /* { */ and /* } */ allow for quickly the finding of the
> > matching ifdef/endif.
> > 
> > In the "vim" editor, the command '%' will take one from one curly paren to its
> > matching curly paren...
> 
> '%' in vim let you jump between matching ifdef/endif as well.

Interesting.  Didn't know that.  In that light, I'll redact the white space
changes.

Thanks 

> 
> > 
> > There is a similar sequence for emacs.
> > 
> > > 
> > > Deferring to Wim.
> > > 
> > > Guenter
> > > 
> > 
> > -- 
> > 
> > -----------------------------------------------------------------------------
> > Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
> > -----------------------------------------------------------------------------
> 
> Thanks,
> Marcus Folkesson



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [v3,04/11] watchdog/hpwdt: white space changes

[Guenter Roeck]

On 02/17/2018 11:32 AM, Jerry Hoemann wrote:
> On Sat, Feb 17, 2018 at 08:17:34AM -0800, Guenter Roeck wrote:
>> On Thu, Feb 15, 2018 at 04:43:53PM -0700, Jerry Hoemann wrote:
>>> Minor white space changes and some name clean up.
>>>
>>> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
>>> ---
>>>   MODULE_DEVICE_TABLE(pci, hpwdt_devices);
>>>   
>>> @@ -102,7 +100,7 @@ static int hpwdt_time_left(void)
>>>   	return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
>>>   }
>>>   
>>> -#ifdef CONFIG_HPWDT_NMI_DECODING
>>> +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
>>>   static int hpwdt_my_nmi(void)
>>>   {
>>>   	return ioread8(hpwdt_nmistat) & 0x6;
>>> @@ -133,7 +131,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>>>   
>>>   	return NMI_HANDLED;
>>>   }
>>> -#endif /* CONFIG_HPWDT_NMI_DECODING */
>>> +#endif					/* } */
>>
>> I disagree with those changes. While I don't object to adding the '{'
>> per se, I find it very useful to have the 'CONFIG_HPWDT_NMI_DECODING'
>> with an endif to be able to associate it with the matching #ifdef.
> 
> The matching /* { */ and /* } */ allow for quickly the finding of the
> matching ifdef/endif.
> 
> In the "vim" editor, the command '%' will take one from one curly paren to its
> matching curly paren...
> 
> There is a similar sequence for emacs.
> 
This isn't about you, it is about the community.

Looking again into this patch, the only acceptable change is the copyright update.
The driver is still based on softdog. The other changes are your personal preference
only and don't fix any checkpatch issues (and even those would be arguable for
some maintainers). This means that, long term, we might get another patch from
someone else changing indentations and variable names again ... and again ...
and again. This adds no value.

Guenter

Re: [v3,04/11] watchdog/hpwdt: white space changes

[Philippe Ombredanne]

Jerry,

On Sat, Feb 17, 2018 at 5:17 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Feb 15, 2018 at 04:43:53PM -0700, Jerry Hoemann wrote:
>> Minor white space changes and some name clean up.
>>
>> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
>> ---
>>  drivers/watchdog/hpwdt.c | 49 +++++++++++++++++++++++-------------------------
>>  1 file changed, 23 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
>> index 07810caabf74..d5989acf3e37 100644
>> --- a/drivers/watchdog/hpwdt.c
>> +++ b/drivers/watchdog/hpwdt.c
>> @@ -1,11 +1,9 @@
>>  /*
>>   *   HPE WatchDog Driver
>> - *   based on
>>   *
>> - *   SoftDog 0.05:   A Software Watchdog Device
>> - *
>> - *   (c) Copyright 2015 Hewlett Packard Enterprise Development LP
>> + *   (c) Copyright 2018 Hewlett Packard Enterprise Development LP
>>   *   Thomas Mingarelli <thomas.mingarelli@hpe.com>
>> + *   Jerry Hoemann <jerry.hoemann@hpe.com>
>>   *
>>   *   This program is free software; you can redistribute it and/or
>>   *   modify it under the terms of the GNU General Public License

It would be awesome if you could adopt SPDX ids here and in all HPE
existing and future contributions [1] rather than this fine legalese.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
-- 
Cordially
Philippe Ombredanne

[PATCH v3 05/11] watchdog/hpwdt: Update Module info.

[Jerry Hoemann]

Update Module Author and description to reflect changes in driver.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 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 d5989acf3e37..71e49d0ab789 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -425,8 +425,8 @@ static struct pci_driver hpwdt_driver = {
 	.remove		= hpwdt_exit,
 };
 
-MODULE_AUTHOR("Tom Mingarelli");
-MODULE_DESCRIPTION("hp watchdog driver");
+MODULE_AUTHOR("Jerry Hoemann");
+MODULE_DESCRIPTION("hpe watchdog driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(HPWDT_VERSION);
 
-- 
2.13.6

Re: [v3,05/11] watchdog/hpwdt: Update Module info.

[Guenter Roeck]

On Thu, Feb 15, 2018 at 04:43:54PM -0700, Jerry Hoemann wrote:
> Update Module Author and description to reflect changes in driver.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  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 d5989acf3e37..71e49d0ab789 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -425,8 +425,8 @@ static struct pci_driver hpwdt_driver = {
>  	.remove		= hpwdt_exit,
>  };
>  
> -MODULE_AUTHOR("Tom Mingarelli");
> -MODULE_DESCRIPTION("hp watchdog driver");
> +MODULE_AUTHOR("Jerry Hoemann");
> +MODULE_DESCRIPTION("hpe watchdog driver");

It is quite unusual to replace an author name. The original author is
still the original author and should be retained. Adding another author
is fine, but I disagree with the removal.

Guenter

>  MODULE_LICENSE("GPL");
>  MODULE_VERSION(HPWDT_VERSION);
>  

Re: [v3,05/11] watchdog/hpwdt: Update Module info.

[Jerry Hoemann]

On Sat, Feb 17, 2018 at 08:19:23AM -0800, Guenter Roeck wrote:
> On Thu, Feb 15, 2018 at 04:43:54PM -0700, Jerry Hoemann wrote:
> > Update Module Author and description to reflect changes in driver.
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  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 d5989acf3e37..71e49d0ab789 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -425,8 +425,8 @@ static struct pci_driver hpwdt_driver = {
> >  	.remove		= hpwdt_exit,
> >  };
> >  
> > -MODULE_AUTHOR("Tom Mingarelli");
> > -MODULE_DESCRIPTION("hp watchdog driver");
> > +MODULE_AUTHOR("Jerry Hoemann");
> > +MODULE_DESCRIPTION("hpe watchdog driver");
> 
> It is quite unusual to replace an author name. The original author is
> still the original author and should be retained. Adding another author
> is fine, but I disagree with the removal.

Wasn't intending to usurp Tom's credit (I left him in as Author in
comment block at top of file.)  But as Tom has retired,  I didn't want
anyone having problems with the rewrite of the driver to bother him
about it.

I'll redact this change.

> 
> Guenter
> 
> >  MODULE_LICENSE("GPL");
> >  MODULE_VERSION(HPWDT_VERSION);
> >  

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

[PATCH v3 06/11] watchdog/hpwdt: Select WATCHDOG_CORE

[Jerry Hoemann]

Update Kconfig file to show that hpwdt now selects WATCHDOG_CORE.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 6a602f70aaa4..4d219c3fa8b4 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1108,6 +1108,7 @@ config IT87_WDT
 
 config HP_WATCHDOG
 	tristate "HP ProLiant iLO2+ Hardware Watchdog Timer"
+	select WATCHDOG_CORE
 	depends on X86 && PCI
 	help
 	  A software monitoring watchdog and NMI sourcing driver. This driver
-- 
2.13.6

Re: [v3,06/11] watchdog/hpwdt: Select WATCHDOG_CORE

[Guenter Roeck]

On Thu, Feb 15, 2018 at 04:43:55PM -0700, Jerry Hoemann wrote:
> Update Kconfig file to show that hpwdt now selects WATCHDOG_CORE.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

At this point, WATCHDOG_CORE is not required. This patch should
be merged with the next patch, which adds the requirement.

Since it appears that you insist on having it as separate patch,
deferring to Wim.

Guenter

> ---
>  drivers/watchdog/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 6a602f70aaa4..4d219c3fa8b4 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1108,6 +1108,7 @@ config IT87_WDT
>  
>  config HP_WATCHDOG
>  	tristate "HP ProLiant iLO2+ Hardware Watchdog Timer"
> +	select WATCHDOG_CORE
>  	depends on X86 && PCI
>  	help
>  	  A software monitoring watchdog and NMI sourcing driver. This driver

Re: [v3,06/11] watchdog/hpwdt: Select WATCHDOG_CORE

[Jerry Hoemann]

On Sat, Feb 17, 2018 at 08:21:30AM -0800, Guenter Roeck wrote:
> On Thu, Feb 15, 2018 at 04:43:55PM -0700, Jerry Hoemann wrote:
> > Update Kconfig file to show that hpwdt now selects WATCHDOG_CORE.
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> 
> At this point, WATCHDOG_CORE is not required. This patch should
> be merged with the next patch, which adds the requirement.
> 
> Since it appears that you insist on having it as separate patch,
> deferring to Wim.
> 

I have no objection to squashing these two commits once we have
agreed upon the actual changes being made.

I re-ordered to address the bisectability question, but kept them
separate as it is a little bit easier for me to rework patches if
the patch contains only a single file.


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

[PATCH v3 07/11] watchdog/hpwdt: Modify to use watchdog core.

[Jerry Hoemann]

Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
convert hpwdt from legacy watchdog driver to use the watchdog core.

Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
Added functions: hpwdt_settimeout
Added structures: watchdog_device

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 249 ++++++++++++-----------------------------------
 1 file changed, 63 insertions(+), 186 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 71e49d0ab789..da9a04101814 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -14,18 +14,13 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/device.h>
-#include <linux/fs.h>
-#include <linux/io.h>
-#include <linux/bitops.h>
 #include <linux/kernel.h>
-#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/pci.h>
-#include <linux/pci_ids.h>
 #include <linux/types.h>
-#include <linux/uaccess.h>
 #include <linux/watchdog.h>
+
 #include <asm/nmi.h>
 
 #define HPWDT_VERSION			"1.4.0"
@@ -40,8 +35,6 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
 #ifdef CONFIG_HPWDT_NMI_DECODING
 static unsigned int allow_kdump = 1;
 #endif
-static char expect_release;
-static unsigned long hpwdt_is_open;
 
 static void __iomem *pci_mem_addr;		/* the PCI-memory address */
 static unsigned long __iomem *hpwdt_nmistat;
@@ -55,53 +48,58 @@ static const struct pci_device_id hpwdt_devices[] = {
 };
 MODULE_DEVICE_TABLE(pci, hpwdt_devices);
 
+static struct watchdog_device hpwdt_dev;
 
 /*
  *	Watchdog operations
  */
-static void hpwdt_start(void)
+static int hpwdt_start(struct watchdog_device *dev)
 {
-	reload = SECS_TO_TICKS(soft_margin);
+	reload = SECS_TO_TICKS(dev->timeout);
+
 	iowrite16(reload, hpwdt_timer_reg);
 	iowrite8(0x85, hpwdt_timer_con);
+
+	return 0;
 }
 
-static void hpwdt_stop(void)
+static int hpwdt_stop(struct watchdog_device *dev)
 {
 	unsigned long data;
 
 	data = ioread8(hpwdt_timer_con);
 	data &= 0xFE;
 	iowrite8(data, hpwdt_timer_con);
+	return 0;
 }
 
-static void hpwdt_ping(void)
-{
-	iowrite16(reload, hpwdt_timer_reg);
-}
-
-static int hpwdt_change_timer(int new_margin)
+static int hpwdt_ping(struct watchdog_device *dev)
 {
-	if (new_margin < 1 || new_margin > HPWDT_MAX_TIMER) {
-		pr_warn("New value passed in is invalid: %d seconds\n",
-			new_margin);
-		return -EINVAL;
-	}
+	reload = SECS_TO_TICKS(dev->timeout);
 
-	soft_margin = new_margin;
-	pr_debug("New timer passed in is %d seconds\n", new_margin);
-	reload = SECS_TO_TICKS(soft_margin);
+	iowrite16(reload, hpwdt_timer_reg);
 
 	return 0;
 }
 
-static int hpwdt_time_left(void)
+static unsigned int hpwdt_gettimeleft(struct watchdog_device *dev)
 {
 	return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
 }
 
+static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
+{
+	dev_dbg(dev->parent, "settimeout = %d\n", val);
+
+	dev->timeout = val;
+	hpwdt_ping(dev);
+
+	return 0;
+}
+
 #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
-static int hpwdt_my_nmi(void)
+
+static unsigned int hpwdt_my_nmi(void)
 {
 	return ioread8(hpwdt_nmistat) & 0x6;
 }
@@ -123,8 +121,10 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 	if ((ulReason == NMI_UNKNOWN) && !mynmi)
 		return NMI_DONE;
 
+	pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
+
 	if (allow_kdump)
-		hpwdt_stop();
+		hpwdt_stop(&hpwdt_dev);
 
 	hex_byte_pack(panic_msg, mynmi);
 	nmi_panic(regs, panic_msg);
@@ -133,68 +133,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 }
 #endif					/* } */
 
-/*
- *	/dev/watchdog handling
- */
-static int hpwdt_open(struct inode *inode, struct file *file)
-{
-	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &hpwdt_is_open))
-		return -EBUSY;
-
-	/* Start the watchdog */
-	hpwdt_start();
-	hpwdt_ping();
-
-	return nonseekable_open(inode, file);
-}
-
-static int hpwdt_release(struct inode *inode, struct file *file)
-{
-	/* Stop the watchdog */
-	if (expect_release == 42) {
-		hpwdt_stop();
-	} else {
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-		hpwdt_ping();
-	}
-
-	expect_release = 0;
-
-	/* /dev/watchdog is being closed, make sure it can be re-opened */
-	clear_bit(0, &hpwdt_is_open);
-
-	return 0;
-}
-
-static ssize_t hpwdt_write(struct file *file, const char __user *data,
-	size_t len, loff_t *ppos)
-{
-	/* See if we got the magic character 'V' and reload the timer */
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* note: just in case someone wrote the magic character
-			 * five months ago... */
-			expect_release = 0;
-
-			/* scan to see whether or not we got the magic char. */
-			for (i = 0; i != len; i++) {
-				char c;
-				if (get_user(c, data + i))
-					return -EFAULT;
-				if (c == 'V')
-					expect_release = 42;
-			}
-		}
-
-		/* someone wrote to us, we should reload the timer */
-		hpwdt_ping();
-	}
-
-	return len;
-}
 
 static const struct watchdog_info hpwdt_info = {
 	.options	= WDIOF_SETTIMEOUT    |
@@ -203,90 +141,10 @@ static const struct watchdog_info hpwdt_info = {
 	.identity	= "HPE iLO2+ HW Watchdog Timer",
 };
 
-static long hpwdt_ioctl(struct file *file, unsigned int cmd,
-	unsigned long arg)
-{
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	int new_margin, options;
-	int ret = -ENOTTY;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		ret = 0;
-		if (copy_to_user(argp, &hpwdt_info, sizeof(hpwdt_info)))
-			ret = -EFAULT;
-		break;
-
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		ret = put_user(0, p);
-		break;
-
-	case WDIOC_KEEPALIVE:
-		hpwdt_ping();
-		ret = 0;
-		break;
-
-	case WDIOC_SETOPTIONS:
-		ret = get_user(options, p);
-		if (ret)
-			break;
-
-		if (options & WDIOS_DISABLECARD)
-			hpwdt_stop();
-
-		if (options & WDIOS_ENABLECARD) {
-			hpwdt_start();
-			hpwdt_ping();
-		}
-		break;
-
-	case WDIOC_SETTIMEOUT:
-		ret = get_user(new_margin, p);
-		if (ret)
-			break;
-
-		ret = hpwdt_change_timer(new_margin);
-		if (ret)
-			break;
-
-		hpwdt_ping();
-		/* Fall */
-	case WDIOC_GETTIMEOUT:
-		ret = put_user(soft_margin, p);
-		break;
-
-	case WDIOC_GETTIMELEFT:
-		ret = put_user(hpwdt_time_left(), p);
-		break;
-	}
-	return ret;
-}
-
-/*
- *	Kernel interfaces
- */
-static const struct file_operations hpwdt_fops = {
-	.owner = THIS_MODULE,
-	.llseek = no_llseek,
-	.write = hpwdt_write,
-	.unlocked_ioctl = hpwdt_ioctl,
-	.open = hpwdt_open,
-	.release = hpwdt_release,
-};
-
-static struct miscdevice hpwdt_miscdev = {
-	.minor = WATCHDOG_MINOR,
-	.name = "watchdog",
-	.fops = &hpwdt_fops,
-};
-
 /*
  *	Init & Exit
  */
 
-
 static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
 {
 #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
@@ -373,32 +231,32 @@ static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 	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();
-
-	/* Make sure that we have a valid soft_margin */
-	if (hpwdt_change_timer(soft_margin))
-		hpwdt_change_timer(DEFAULT_MARGIN);
-
 	/* Initialize NMI Decoding functionality */
 	retval = hpwdt_init_nmi_decoding(dev);
 	if (retval != 0)
 		goto error_init_nmi_decoding;
 
-	retval = misc_register(&hpwdt_miscdev);
+	hpwdt_dev.parent = &dev->dev;
+	retval = watchdog_register_device(&hpwdt_dev);
 	if (retval < 0) {
-		dev_warn(&dev->dev,
-			"Unable to register miscdev on minor=%d (err=%d).\n",
-			WATCHDOG_MINOR, retval);
-		goto error_misc_register;
+		dev_warn(&dev->dev, "Unable to register hpe watchdog (err=%d).\n", retval);
+		goto error_wd_register;
 	}
 
+	/* Make sure that timer is disabled until /dev/watchdog is opened */
+	hpwdt_stop(&hpwdt_dev);
+
+	watchdog_set_nowayout(&hpwdt_dev, nowayout);
+	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
+		dev_warn(&dev->dev, "Invalid soft_margin: %d. Using default\n", soft_margin);
+
 	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
 			", timer margin: %d seconds (nowayout=%d).\n",
-			HPWDT_VERSION, soft_margin, nowayout);
+			HPWDT_VERSION, hpwdt_dev.timeout, nowayout);
+
 	return 0;
 
-error_misc_register:
+error_wd_register:
 	hpwdt_exit_nmi_decoding();
 error_init_nmi_decoding:
 	pci_iounmap(dev, pci_mem_addr);
@@ -410,9 +268,9 @@ static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 static void hpwdt_exit(struct pci_dev *dev)
 {
 	if (!nowayout)
-		hpwdt_stop();
+		hpwdt_stop(&hpwdt_dev);
 
-	misc_deregister(&hpwdt_miscdev);
+	watchdog_unregister_device(&hpwdt_dev);
 	hpwdt_exit_nmi_decoding();
 	pci_iounmap(dev, pci_mem_addr);
 	pci_disable_device(dev);
@@ -425,6 +283,25 @@ static struct pci_driver hpwdt_driver = {
 	.remove		= hpwdt_exit,
 };
 
+
+static const struct watchdog_ops hpwdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= hpwdt_start,
+	.stop		= hpwdt_stop,
+	.ping		= hpwdt_ping,
+	.set_timeout	= hpwdt_settimeout,
+	.get_timeleft	= hpwdt_gettimeleft,
+};
+
+static struct watchdog_device hpwdt_dev = {
+	.info		= &hpwdt_info,
+	.ops		= &hpwdt_ops,
+	.min_timeout	= 1,
+	.max_timeout	= HPWDT_MAX_TIMER,
+	.timeout	= DEFAULT_MARGIN,
+};
+
+
 MODULE_AUTHOR("Jerry Hoemann");
 MODULE_DESCRIPTION("hpe watchdog driver");
 MODULE_LICENSE("GPL");
-- 
2.13.6

Re: [v3,07/11] watchdog/hpwdt: Modify to use watchdog core.

[Guenter Roeck]

On Thu, Feb 15, 2018 at 04:43:56PM -0700, Jerry Hoemann wrote:
> Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
> convert hpwdt from legacy watchdog driver to use the watchdog core.
> 
> Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
> Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
> Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
> Added functions: hpwdt_settimeout
> Added structures: watchdog_device
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/watchdog/hpwdt.c | 249 ++++++++++++-----------------------------------
>  1 file changed, 63 insertions(+), 186 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 71e49d0ab789..da9a04101814 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -14,18 +14,13 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/device.h>
> -#include <linux/fs.h>
> -#include <linux/io.h>

The driver still performs direct IO. Why do you remove this
include file ?

> -#include <linux/bitops.h>
>  #include <linux/kernel.h>
> -#include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/pci.h>
> -#include <linux/pci_ids.h>

The PCI vendor IDs used by the driver are declared in this file.
Is there a direction somewhere suggesting that this file
should not be directly included ?

>  #include <linux/types.h>
> -#include <linux/uaccess.h>
>  #include <linux/watchdog.h>
> +
>  #include <asm/nmi.h>
>  
>  #define HPWDT_VERSION			"1.4.0"
> @@ -40,8 +35,6 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
>  #ifdef CONFIG_HPWDT_NMI_DECODING
>  static unsigned int allow_kdump = 1;
>  #endif
> -static char expect_release;
> -static unsigned long hpwdt_is_open;
>  
>  static void __iomem *pci_mem_addr;		/* the PCI-memory address */
>  static unsigned long __iomem *hpwdt_nmistat;
> @@ -55,53 +48,58 @@ static const struct pci_device_id hpwdt_devices[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
>  
> +static struct watchdog_device hpwdt_dev;

Please no double declarations. This is only needed for the NMI
code to pass to hpwdt_stop where it isn't used. It would be
easy to introduce _hpwdt_stop() with no parameter and call that
function frm hpwdt_stop().

>  
>  /*
>   *	Watchdog operations
>   */
> -static void hpwdt_start(void)
> +static int hpwdt_start(struct watchdog_device *dev)
>  {
> -	reload = SECS_TO_TICKS(soft_margin);
> +	reload = SECS_TO_TICKS(dev->timeout);
> +
>  	iowrite16(reload, hpwdt_timer_reg);
>  	iowrite8(0x85, hpwdt_timer_con);
> +
> +	return 0;
>  }
>  
> -static void hpwdt_stop(void)
> +static int hpwdt_stop(struct watchdog_device *dev)
>  {
>  	unsigned long data;
>  
>  	data = ioread8(hpwdt_timer_con);
>  	data &= 0xFE;
>  	iowrite8(data, hpwdt_timer_con);
> +	return 0;
>  }
>  
> -static void hpwdt_ping(void)
> -{
> -	iowrite16(reload, hpwdt_timer_reg);
> -}
> -
> -static int hpwdt_change_timer(int new_margin)
> +static int hpwdt_ping(struct watchdog_device *dev)
>  {
> -	if (new_margin < 1 || new_margin > HPWDT_MAX_TIMER) {
> -		pr_warn("New value passed in is invalid: %d seconds\n",
> -			new_margin);
> -		return -EINVAL;
> -	}
> +	reload = SECS_TO_TICKS(dev->timeout);
>  
> -	soft_margin = new_margin;
> -	pr_debug("New timer passed in is %d seconds\n", new_margin);
> -	reload = SECS_TO_TICKS(soft_margin);
> +	iowrite16(reload, hpwdt_timer_reg);
>  
>  	return 0;
>  }
>  
> -static int hpwdt_time_left(void)
> +static unsigned int hpwdt_gettimeleft(struct watchdog_device *dev)
>  {
>  	return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
>  }
>  
> +static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
> +{
> +	dev_dbg(dev->parent, "settimeout = %d\n", val);
> +
> +	dev->timeout = val;
> +	hpwdt_ping(dev);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> -static int hpwdt_my_nmi(void)
> +
> +static unsigned int hpwdt_my_nmi(void)
>  {
>  	return ioread8(hpwdt_nmistat) & 0x6;
>  }
> @@ -123,8 +121,10 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>  	if ((ulReason == NMI_UNKNOWN) && !mynmi)
>  		return NMI_DONE;
>  
> +	pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
> +
>  	if (allow_kdump)
> -		hpwdt_stop();
> +		hpwdt_stop(&hpwdt_dev);
>  
>  	hex_byte_pack(panic_msg, mynmi);
>  	nmi_panic(regs, panic_msg);
> @@ -133,68 +133,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>  }
>  #endif					/* } */
>  
> -/*
> - *	/dev/watchdog handling
> - */
> -static int hpwdt_open(struct inode *inode, struct file *file)
> -{
> -	/* /dev/watchdog can only be opened once */
> -	if (test_and_set_bit(0, &hpwdt_is_open))
> -		return -EBUSY;
> -
> -	/* Start the watchdog */
> -	hpwdt_start();
> -	hpwdt_ping();
> -
> -	return nonseekable_open(inode, file);
> -}
> -
> -static int hpwdt_release(struct inode *inode, struct file *file)
> -{
> -	/* Stop the watchdog */
> -	if (expect_release == 42) {
> -		hpwdt_stop();
> -	} else {
> -		pr_crit("Unexpected close, not stopping watchdog!\n");
> -		hpwdt_ping();
> -	}
> -
> -	expect_release = 0;
> -
> -	/* /dev/watchdog is being closed, make sure it can be re-opened */
> -	clear_bit(0, &hpwdt_is_open);
> -
> -	return 0;
> -}
> -
> -static ssize_t hpwdt_write(struct file *file, const char __user *data,
> -	size_t len, loff_t *ppos)
> -{
> -	/* See if we got the magic character 'V' and reload the timer */
> -	if (len) {
> -		if (!nowayout) {
> -			size_t i;
> -
> -			/* note: just in case someone wrote the magic character
> -			 * five months ago... */
> -			expect_release = 0;
> -
> -			/* scan to see whether or not we got the magic char. */
> -			for (i = 0; i != len; i++) {
> -				char c;
> -				if (get_user(c, data + i))
> -					return -EFAULT;
> -				if (c == 'V')
> -					expect_release = 42;
> -			}
> -		}
> -
> -		/* someone wrote to us, we should reload the timer */
> -		hpwdt_ping();
> -	}
> -
> -	return len;
> -}
>  
>  static const struct watchdog_info hpwdt_info = {
>  	.options	= WDIOF_SETTIMEOUT    |
> @@ -203,90 +141,10 @@ static const struct watchdog_info hpwdt_info = {
>  	.identity	= "HPE iLO2+ HW Watchdog Timer",
>  };
>  
> -static long hpwdt_ioctl(struct file *file, unsigned int cmd,
> -	unsigned long arg)
> -{
> -	void __user *argp = (void __user *)arg;
> -	int __user *p = argp;
> -	int new_margin, options;
> -	int ret = -ENOTTY;
> -
> -	switch (cmd) {
> -	case WDIOC_GETSUPPORT:
> -		ret = 0;
> -		if (copy_to_user(argp, &hpwdt_info, sizeof(hpwdt_info)))
> -			ret = -EFAULT;
> -		break;
> -
> -	case WDIOC_GETSTATUS:
> -	case WDIOC_GETBOOTSTATUS:
> -		ret = put_user(0, p);
> -		break;
> -
> -	case WDIOC_KEEPALIVE:
> -		hpwdt_ping();
> -		ret = 0;
> -		break;
> -
> -	case WDIOC_SETOPTIONS:
> -		ret = get_user(options, p);
> -		if (ret)
> -			break;
> -
> -		if (options & WDIOS_DISABLECARD)
> -			hpwdt_stop();
> -
> -		if (options & WDIOS_ENABLECARD) {
> -			hpwdt_start();
> -			hpwdt_ping();
> -		}
> -		break;
> -
> -	case WDIOC_SETTIMEOUT:
> -		ret = get_user(new_margin, p);
> -		if (ret)
> -			break;
> -
> -		ret = hpwdt_change_timer(new_margin);
> -		if (ret)
> -			break;
> -
> -		hpwdt_ping();
> -		/* Fall */
> -	case WDIOC_GETTIMEOUT:
> -		ret = put_user(soft_margin, p);
> -		break;
> -
> -	case WDIOC_GETTIMELEFT:
> -		ret = put_user(hpwdt_time_left(), p);
> -		break;
> -	}
> -	return ret;
> -}
> -
> -/*
> - *	Kernel interfaces
> - */
> -static const struct file_operations hpwdt_fops = {
> -	.owner = THIS_MODULE,
> -	.llseek = no_llseek,
> -	.write = hpwdt_write,
> -	.unlocked_ioctl = hpwdt_ioctl,
> -	.open = hpwdt_open,
> -	.release = hpwdt_release,
> -};
> -
> -static struct miscdevice hpwdt_miscdev = {
> -	.minor = WATCHDOG_MINOR,
> -	.name = "watchdog",
> -	.fops = &hpwdt_fops,
> -};
> -
>  /*
>   *	Init & Exit
>   */
>  
> -
>  static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  {
>  #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> @@ -373,32 +231,32 @@ static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
>  	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();
> -
> -	/* Make sure that we have a valid soft_margin */
> -	if (hpwdt_change_timer(soft_margin))
> -		hpwdt_change_timer(DEFAULT_MARGIN);
> -
>  	/* Initialize NMI Decoding functionality */
>  	retval = hpwdt_init_nmi_decoding(dev);
>  	if (retval != 0)
>  		goto error_init_nmi_decoding;
>  
> -	retval = misc_register(&hpwdt_miscdev);
> +	hpwdt_dev.parent = &dev->dev;
> +	retval = watchdog_register_device(&hpwdt_dev);
>  	if (retval < 0) {
> -		dev_warn(&dev->dev,
> -			"Unable to register miscdev on minor=%d (err=%d).\n",
> -			WATCHDOG_MINOR, retval);
> -		goto error_misc_register;
> +		dev_warn(&dev->dev, "Unable to register hpe watchdog (err=%d).\n", retval);

checkpatch warning. Also, this should be dev_err() since it is fatal.

> +		goto error_wd_register;
>  	}
>  
> +	/* Make sure that timer is disabled until /dev/watchdog is opened */
> +	hpwdt_stop(&hpwdt_dev);

The watchdog is already registered and the device can already have been
opened at this point. The watchdog would have to be stopped before
registering the watchdog.

A better approach would be to detect if the watchdog is running and inform
the watchdog core about it. The code in the stop function suggests that
this would be possible. This would ensure that the watchdog protection
during boot is retained.

> +
> +	watchdog_set_nowayout(&hpwdt_dev, nowayout);
> +	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
> +		dev_warn(&dev->dev, "Invalid soft_margin: %d. Using default\n", soft_margin);
> +
Those two functions should be called before registering the watchdog.
Also, there is a checkpatch warning.

>  	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
>  			", timer margin: %d seconds (nowayout=%d).\n",
> -			HPWDT_VERSION, soft_margin, nowayout);
> +			HPWDT_VERSION, hpwdt_dev.timeout, nowayout);
> +
>  	return 0;
>  
> -error_misc_register:
> +error_wd_register:
>  	hpwdt_exit_nmi_decoding();
>  error_init_nmi_decoding:
>  	pci_iounmap(dev, pci_mem_addr);
> @@ -410,9 +268,9 @@ static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
>  static void hpwdt_exit(struct pci_dev *dev)
>  {
>  	if (!nowayout)
> -		hpwdt_stop();
> +		hpwdt_stop(&hpwdt_dev);
>  
> -	misc_deregister(&hpwdt_miscdev);
> +	watchdog_unregister_device(&hpwdt_dev);
>  	hpwdt_exit_nmi_decoding();
>  	pci_iounmap(dev, pci_mem_addr);
>  	pci_disable_device(dev);
> @@ -425,6 +283,25 @@ static struct pci_driver hpwdt_driver = {
>  	.remove		= hpwdt_exit,
>  };
>  
> +
> +static const struct watchdog_ops hpwdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= hpwdt_start,
> +	.stop		= hpwdt_stop,
> +	.ping		= hpwdt_ping,
> +	.set_timeout	= hpwdt_settimeout,
> +	.get_timeleft	= hpwdt_gettimeleft,
> +};
> +
> +static struct watchdog_device hpwdt_dev = {
> +	.info		= &hpwdt_info,
> +	.ops		= &hpwdt_ops,
> +	.min_timeout	= 1,
> +	.max_timeout	= HPWDT_MAX_TIMER,
> +	.timeout	= DEFAULT_MARGIN,
> +};
> +
> +
>  MODULE_AUTHOR("Jerry Hoemann");
>  MODULE_DESCRIPTION("hpe watchdog driver");
>  MODULE_LICENSE("GPL");

Re: [v3,07/11] watchdog/hpwdt: Modify to use watchdog core.

[Jerry Hoemann]

On Sat, Feb 17, 2018 at 08:49:07AM -0800, Guenter Roeck wrote:
> On Thu, Feb 15, 2018 at 04:43:56PM -0700, Jerry Hoemann wrote:
> > Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
> > convert hpwdt from legacy watchdog driver to use the watchdog core.
> > 
> > Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
> > Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
> > Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
> > Added functions: hpwdt_settimeout
> > Added structures: watchdog_device
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  drivers/watchdog/hpwdt.c | 249 ++++++++++++-----------------------------------
> >  1 file changed, 63 insertions(+), 186 deletions(-)
> > 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index 71e49d0ab789..da9a04101814 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -14,18 +14,13 @@
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >  
> >  #include <linux/device.h>
> > -#include <linux/fs.h>
> > -#include <linux/io.h>
> 
> The driver still performs direct IO. Why do you remove this
> include file ?

I was just removing file includes no longer needed to compile
the module.  While there are sential ifdef protecting against
compile errors for multiply including the same file, the extra
open and parsing of the file does add a little bit of overhead
to the build time.  Probably not as important today as it used
to be.

I will redact the change.

> 
> > -#include <linux/bitops.h>
> >  #include <linux/kernel.h>
> > -#include <linux/miscdevice.h>
> >  #include <linux/module.h>
> >  #include <linux/moduleparam.h>
> >  #include <linux/pci.h>
> > -#include <linux/pci_ids.h>
> 
> The PCI vendor IDs used by the driver are declared in this file.
> Is there a direction somewhere suggesting that this file
> should not be directly included ?

See above.

> 
> >  #include <linux/types.h>
> > -#include <linux/uaccess.h>
> >  #include <linux/watchdog.h>
> > +
> >  #include <asm/nmi.h>
> >  
> >  #define HPWDT_VERSION			"1.4.0"
> > @@ -40,8 +35,6 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
> >  #ifdef CONFIG_HPWDT_NMI_DECODING
> >  static unsigned int allow_kdump = 1;
> >  #endif
> > -static char expect_release;
> > -static unsigned long hpwdt_is_open;
> >  
> >  static void __iomem *pci_mem_addr;		/* the PCI-memory address */
> >  static unsigned long __iomem *hpwdt_nmistat;
> > @@ -55,53 +48,58 @@ static const struct pci_device_id hpwdt_devices[] = {
> >  };
> >  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
> >  
> > +static struct watchdog_device hpwdt_dev;
> 
> Please no double declarations. This is only needed for the NMI
> code to pass to hpwdt_stop where it isn't used. It would be
> easy to introduce _hpwdt_stop() with no parameter and call that
> function frm hpwdt_stop().

I'll redact this change.

I'll add a new function hpwdt_stop_new (or something like that) which
takes the watchdog_devices as input and it will call current upstream
hpwdt_stop.

> 
> >  
> > -	retval = misc_register(&hpwdt_miscdev);
> > +	hpwdt_dev.parent = &dev->dev;
> > +	retval = watchdog_register_device(&hpwdt_dev);
> >  	if (retval < 0) {
> > -		dev_warn(&dev->dev,
> > -			"Unable to register miscdev on minor=%d (err=%d).\n",
> > -			WATCHDOG_MINOR, retval);
> > -		goto error_misc_register;
> > +		dev_warn(&dev->dev, "Unable to register hpe watchdog (err=%d).\n", retval);
> 
> checkpatch warning. Also, this should be dev_err() since it is fatal.

i'll change to dev_err.  i'll shorten the message to something similar.

> 
> > +		goto error_wd_register;
> >  	}
> >  
> > +	/* Make sure that timer is disabled until /dev/watchdog is opened */
> > +	hpwdt_stop(&hpwdt_dev);
> 
> The watchdog is already registered and the device can already have been
> opened at this point. The watchdog would have to be stopped before
> registering the watchdog.
> 
> A better approach would be to detect if the watchdog is running and inform
> the watchdog core about it. The code in the stop function suggests that
> this would be possible. This would ensure that the watchdog protection
> during boot is retained.

I will redact this change as it will no longer be necessary. See above.

> 
> > +
> > +	watchdog_set_nowayout(&hpwdt_dev, nowayout);
> > +	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
> > +		dev_warn(&dev->dev, "Invalid soft_margin: %d. Using default\n", soft_margin);
> > +
> Those two functions should be called before registering the watchdog.
> Also, there is a checkpatch warning.
> 

Will change.

Please update the Documentation/watchdog/watchdog-kernel-api.txt.

It is not clear from the documentation which interfaces are to be
called before watchdog_register_device and which ones are to be
called after.




-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

[PATCH v3 08/11] watchdog/hpwdt: Programable Pretimeout NMI

[Jerry Hoemann]

Make whether or not the hpwdt watchdog delivers a pretimeout NMI
programable by the user.

The underlying iLO hardware is programmable as to whether or not
a pre-timeout NMI is delivered to the system before the iLO resets
the system.  However, the iLO does not allow for programming the
length of time that NMI is delivered before the system is reset.

Hence, in hpwdt_set_pretimeout, val == 0 disables the NMI. Any
non-zero value sets the pretimeout length to what the hardware
supports.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index da9a04101814..dc0ad20738ed 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -28,12 +28,15 @@
 #define TICKS_TO_SECS(ticks)		((ticks) * 128 / 1000)
 #define HPWDT_MAX_TIMER			TICKS_TO_SECS(65535)
 #define DEFAULT_MARGIN			30
+#define PRETIMEOUT_SEC			9
 
 static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
-static unsigned int reload;			/* the computed soft_margin */
 static bool nowayout = WATCHDOG_NOWAYOUT;
 #ifdef CONFIG_HPWDT_NMI_DECODING
 static unsigned int allow_kdump = 1;
+static bool pretimeout = 1;
+#else
+static bool pretimeout;
 #endif
 
 static void __iomem *pci_mem_addr;		/* the PCI-memory address */
@@ -55,10 +58,12 @@ static struct watchdog_device hpwdt_dev;
  */
 static int hpwdt_start(struct watchdog_device *dev)
 {
-	reload = SECS_TO_TICKS(dev->timeout);
+	int control = 0x81 | (pretimeout ? 0x4 : 0);
+	int reload = SECS_TO_TICKS(dev->timeout);
 
+	dev_dbg(dev->parent, "start watchdog 0x%08x:0x%02x\n", reload, control);
 	iowrite16(reload, hpwdt_timer_reg);
-	iowrite8(0x85, hpwdt_timer_con);
+	iowrite8(control, hpwdt_timer_con);
 
 	return 0;
 }
@@ -67,6 +72,8 @@ static int hpwdt_stop(struct watchdog_device *dev)
 {
 	unsigned long data;
 
+	dev_dbg(dev->parent, "stop  watchdog\n");
+
 	data = ioread8(hpwdt_timer_con);
 	data &= 0xFE;
 	iowrite8(data, hpwdt_timer_con);
@@ -75,8 +82,9 @@ static int hpwdt_stop(struct watchdog_device *dev)
 
 static int hpwdt_ping(struct watchdog_device *dev)
 {
-	reload = SECS_TO_TICKS(dev->timeout);
+	int reload = SECS_TO_TICKS(dev->timeout);
 
+	dev_dbg(dev->parent, "ping  watchdog 0x%08x\n", reload);
 	iowrite16(reload, hpwdt_timer_reg);
 
 	return 0;
@@ -98,12 +106,21 @@ static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
 }
 
 #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
+static int hpwdt_set_pretimeout(struct watchdog_device *dev, unsigned int val)
+{
+	if (val && (val != PRETIMEOUT_SEC)) {
+		dev_info(dev->parent, "Setting pretimeout to %d\n", PRETIMEOUT_SEC);
+		val = PRETIMEOUT_SEC;
+	}
+	dev->pretimeout = val;
+	pretimeout = val ? 1 : 0;
+	return 0;
+}
 
 static unsigned int hpwdt_my_nmi(void)
 {
 	return ioread8(hpwdt_nmistat) & 0x6;
 }
-
 /*
  *	NMI Handler
  */
@@ -123,6 +140,9 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 
 	pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
 
+	if (!pretimeout)
+		return NMI_DONE;
+
 	if (allow_kdump)
 		hpwdt_stop(&hpwdt_dev);
 
@@ -137,7 +157,8 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 static const struct watchdog_info hpwdt_info = {
 	.options	= WDIOF_SETTIMEOUT    |
 			  WDIOF_KEEPALIVEPING |
-			  WDIOF_MAGICCLOSE,
+			  WDIOF_MAGICCLOSE    |
+			  WDIOF_PRETIMEOUT,
 	.identity	= "HPE iLO2+ HW Watchdog Timer",
 };
 
@@ -291,6 +312,9 @@ static const struct watchdog_ops hpwdt_ops = {
 	.ping		= hpwdt_ping,
 	.set_timeout	= hpwdt_settimeout,
 	.get_timeleft	= hpwdt_gettimeleft,
+#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
+	.set_pretimeout	= hpwdt_set_pretimeout,
+#endif					/* } */
 };
 
 static struct watchdog_device hpwdt_dev = {
@@ -299,6 +323,9 @@ static struct watchdog_device hpwdt_dev = {
 	.min_timeout	= 1,
 	.max_timeout	= HPWDT_MAX_TIMER,
 	.timeout	= DEFAULT_MARGIN,
+#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
+	.pretimeout	= PRETIMEOUT_SEC,
+#endif					/* } */
 };
 
 
@@ -317,6 +344,9 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
 module_param(allow_kdump, int, 0);
 MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
+
+module_param(pretimeout, bool, 0);
+MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
 #endif					/* } */
 
 module_pci_driver(hpwdt_driver);
-- 
2.13.6

Re: [PATCH v3 08/11] watchdog/hpwdt: Programable Pretimeout NMI

[Guenter Roeck]

On Thu, Feb 15, 2018 at 04:43:57PM -0700, Jerry Hoemann wrote:
> Make whether or not the hpwdt watchdog delivers a pretimeout NMI
> programable by the user.
> 
> The underlying iLO hardware is programmable as to whether or not
> a pre-timeout NMI is delivered to the system before the iLO resets
> the system.  However, the iLO does not allow for programming the
> length of time that NMI is delivered before the system is reset.
> 
> Hence, in hpwdt_set_pretimeout, val == 0 disables the NMI. Any
> non-zero value sets the pretimeout length to what the hardware
> supports.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/watchdog/hpwdt.c | 42 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index da9a04101814..dc0ad20738ed 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -28,12 +28,15 @@
>  #define TICKS_TO_SECS(ticks)		((ticks) * 128 / 1000)
>  #define HPWDT_MAX_TIMER			TICKS_TO_SECS(65535)
>  #define DEFAULT_MARGIN			30
> +#define PRETIMEOUT_SEC			9
>  
>  static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
> -static unsigned int reload;			/* the computed soft_margin */
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  #ifdef CONFIG_HPWDT_NMI_DECODING
>  static unsigned int allow_kdump = 1;
> +static bool pretimeout = 1;
> +#else
> +static bool pretimeout;
>  #endif
>  
static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);

>  static void __iomem *pci_mem_addr;		/* the PCI-memory address */
> @@ -55,10 +58,12 @@ static struct watchdog_device hpwdt_dev;
>   */
>  static int hpwdt_start(struct watchdog_device *dev)
>  {
> -	reload = SECS_TO_TICKS(dev->timeout);
> +	int control = 0x81 | (pretimeout ? 0x4 : 0);
> +	int reload = SECS_TO_TICKS(dev->timeout);
>  
> +	dev_dbg(dev->parent, "start watchdog 0x%08x:0x%02x\n", reload, control);
>  	iowrite16(reload, hpwdt_timer_reg);
> -	iowrite8(0x85, hpwdt_timer_con);
> +	iowrite8(control, hpwdt_timer_con);
>  
>  	return 0;
>  }
> @@ -67,6 +72,8 @@ static int hpwdt_stop(struct watchdog_device *dev)
>  {
>  	unsigned long data;
>  
> +	dev_dbg(dev->parent, "stop  watchdog\n");
> +
Unrelated.

>  	data = ioread8(hpwdt_timer_con);
>  	data &= 0xFE;
>  	iowrite8(data, hpwdt_timer_con);
> @@ -75,8 +82,9 @@ static int hpwdt_stop(struct watchdog_device *dev)
>  
>  static int hpwdt_ping(struct watchdog_device *dev)
>  {
> -	reload = SECS_TO_TICKS(dev->timeout);
> +	int reload = SECS_TO_TICKS(dev->timeout);
>  
> +	dev_dbg(dev->parent, "ping  watchdog 0x%08x\n", reload);

Unrelated. If you want to add debug messages, please do it
in a separate patch.

>  	iowrite16(reload, hpwdt_timer_reg);
>  
>  	return 0;
> @@ -98,12 +106,21 @@ static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
>  }
>  
>  #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> +static int hpwdt_set_pretimeout(struct watchdog_device *dev, unsigned int val)
> +{
> +	if (val && (val != PRETIMEOUT_SEC)) {

Unnecessary ( )

The actual timeout can be a value smaller than 9 seconds.
Minimum is 1 second. What happens if the user configures
a timeout of less than 9 seconds as well as a pretimeout ?
Will it fire immediately ? 

> +		dev_info(dev->parent, "Setting pretimeout to %d\n", PRETIMEOUT_SEC);

Please no ongoing logging noise. This can easily be abused to clog
the kernel log.

> +		val = PRETIMEOUT_SEC;
> +	}
> +	dev->pretimeout = val;
> +	pretimeout = val ? 1 : 0;

	pretimeout = !!val;

> +	return 0;
> +}
>  
>  static unsigned int hpwdt_my_nmi(void)
>  {
>  	return ioread8(hpwdt_nmistat) & 0x6;
>  }
> -
>  /*
>   *	NMI Handler
>   */
> @@ -123,6 +140,9 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>  
>  	pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
>  
> +	if (!pretimeout)
> +		return NMI_DONE;
> +
>  	if (allow_kdump)
>  		hpwdt_stop(&hpwdt_dev);
>  
> @@ -137,7 +157,8 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>  static const struct watchdog_info hpwdt_info = {
>  	.options	= WDIOF_SETTIMEOUT    |
>  			  WDIOF_KEEPALIVEPING |
> -			  WDIOF_MAGICCLOSE,
> +			  WDIOF_MAGICCLOSE    |
> +			  WDIOF_PRETIMEOUT,
>  	.identity	= "HPE iLO2+ HW Watchdog Timer",
>  };
>  
> @@ -291,6 +312,9 @@ static const struct watchdog_ops hpwdt_ops = {
>  	.ping		= hpwdt_ping,
>  	.set_timeout	= hpwdt_settimeout,
>  	.get_timeleft	= hpwdt_gettimeleft,
> +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> +	.set_pretimeout	= hpwdt_set_pretimeout,
> +#endif					/* } */
>  };
>  
>  static struct watchdog_device hpwdt_dev = {
> @@ -299,6 +323,9 @@ static struct watchdog_device hpwdt_dev = {
>  	.min_timeout	= 1,
>  	.max_timeout	= HPWDT_MAX_TIMER,
>  	.timeout	= DEFAULT_MARGIN,
> +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> +	.pretimeout	= PRETIMEOUT_SEC,
> +#endif					/* } */
>  };
>  
>  
> @@ -317,6 +344,9 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>  #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
>  module_param(allow_kdump, int, 0);
>  MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
> +
> +module_param(pretimeout, bool, 0);
> +MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
>  #endif					/* } */
>  
>  module_pci_driver(hpwdt_driver);
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 08/11] watchdog/hpwdt: Programable Pretimeout NMI

[Jerry Hoemann]

On Fri, Feb 16, 2018 at 12:34:40PM -0800, Guenter Roeck wrote:
> On Thu, Feb 15, 2018 at 04:43:57PM -0700, Jerry Hoemann wrote:
> > Make whether or not the hpwdt watchdog delivers a pretimeout NMI
> > programable by the user.
> > 
> > The underlying iLO hardware is programmable as to whether or not
> > a pre-timeout NMI is delivered to the system before the iLO resets
> > the system.  However, the iLO does not allow for programming the
> > length of time that NMI is delivered before the system is reset.
> > 
> > Hence, in hpwdt_set_pretimeout, val == 0 disables the NMI. Any
> > non-zero value sets the pretimeout length to what the hardware
> > supports.
> > 
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  drivers/watchdog/hpwdt.c | 42 ++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index da9a04101814..dc0ad20738ed 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -28,12 +28,15 @@
> >  #define TICKS_TO_SECS(ticks)		((ticks) * 128 / 1000)
> >  #define HPWDT_MAX_TIMER			TICKS_TO_SECS(65535)
> >  #define DEFAULT_MARGIN			30
> > +#define PRETIMEOUT_SEC			9
> >  
> >  static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
> > -static unsigned int reload;			/* the computed soft_margin */
> >  static bool nowayout = WATCHDOG_NOWAYOUT;
> >  #ifdef CONFIG_HPWDT_NMI_DECODING
> >  static unsigned int allow_kdump = 1;
> > +static bool pretimeout = 1;
> > +#else
> > +static bool pretimeout;
> >  #endif
> >  
> static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);

ack. will do.

> 
> >  static void __iomem *pci_mem_addr;		/* the PCI-memory address */
> > @@ -55,10 +58,12 @@ static struct watchdog_device hpwdt_dev;
> >   */
> >  static int hpwdt_start(struct watchdog_device *dev)
> >  {
> > -	reload = SECS_TO_TICKS(dev->timeout);
> > +	int control = 0x81 | (pretimeout ? 0x4 : 0);
> > +	int reload = SECS_TO_TICKS(dev->timeout);
> >  
> > +	dev_dbg(dev->parent, "start watchdog 0x%08x:0x%02x\n", reload, control);
> >  	iowrite16(reload, hpwdt_timer_reg);
> > -	iowrite8(0x85, hpwdt_timer_con);
> > +	iowrite8(control, hpwdt_timer_con);
> >  
> >  	return 0;
> >  }
> > @@ -67,6 +72,8 @@ static int hpwdt_stop(struct watchdog_device *dev)
> >  {
> >  	unsigned long data;
> >  
> > +	dev_dbg(dev->parent, "stop  watchdog\n");
> > +
> Unrelated.
> 
> >  	data = ioread8(hpwdt_timer_con);
> >  	data &= 0xFE;
> >  	iowrite8(data, hpwdt_timer_con);
> > @@ -75,8 +82,9 @@ static int hpwdt_stop(struct watchdog_device *dev)
> >  
> >  static int hpwdt_ping(struct watchdog_device *dev)
> >  {
> > -	reload = SECS_TO_TICKS(dev->timeout);
> > +	int reload = SECS_TO_TICKS(dev->timeout);
> >  
> > +	dev_dbg(dev->parent, "ping  watchdog 0x%08x\n", reload);
> 
> Unrelated. If you want to add debug messages, please do it
> in a separate patch.


Different patch, but same set?  I'll move these (and ones from earlier
patch to a new separate patch later in set.)

> 
> >  	iowrite16(reload, hpwdt_timer_reg);
> >  
> >  	return 0;
> > @@ -98,12 +106,21 @@ static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
> >  }
> >  
> >  #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> > +static int hpwdt_set_pretimeout(struct watchdog_device *dev, unsigned int val)
> > +{
> > +	if (val && (val != PRETIMEOUT_SEC)) {
> 
> Unnecessary ( )


There are several things going on here. I'm not sure which one the above
comment is intended.

While a pretimeout NMI isn't required by the HW to be enabled, if enabled the
length of pretimeout is fixed by HW.

I didn't see anything in the API that would allow us to communicate to
the user this "feature."  timeout at leasst has both min_timeout and max_timeout, but
I didn't see similar for pretimeout.  I also don't think its reasonable to fail
here if the requested value is not 9 as the user really has no way of knowing what
the valid range of pretimeout values are.  So I accept, any non-zero value
for pretimeout, but then set pretimeout to be 9.

But at the same time, I don't like to siliently change a human request
w/o at least warning.



> 
> The actual timeout can be a value smaller than 9 seconds.
> Minimum is 1 second. What happens if the user configures
> a timeout of less than 9 seconds as well as a pretimeout ?
> Will it fire immediately ? 

The architecture is silient on this issue.  My experience with
this is that if timeout < 9 seconds, the NMI is not issued.
System resets when the timeout expires.  This could be implementation
dependent.

Note, this is not a new issue.

I thought about setting the min timeout to ten seconds to avoid this situation.

I haven't dug into the various user level clients of watchdog so I'm not sure 
what the impact of making this change would be to them.


> 
> > +		dev_info(dev->parent, "Setting pretimeout to %d\n", PRETIMEOUT_SEC);
> 
> Please no ongoing logging noise. This can easily be abused to clog
> the kernel log.

Good point.  I will look at WARN_ONCE or something similar.

> 
> > +		val = PRETIMEOUT_SEC;
> > +	}
> > +	dev->pretimeout = val;
> > +	pretimeout = val ? 1 : 0;
> 
> 	pretimeout = !!val;
> 

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v3 08/11] watchdog/hpwdt: Programable Pretimeout NMI

[Guenter Roeck]

On Fri, Feb 16, 2018 at 04:46:17PM -0700, Jerry Hoemann wrote:
> On Fri, Feb 16, 2018 at 12:34:40PM -0800, Guenter Roeck wrote:
> > On Thu, Feb 15, 2018 at 04:43:57PM -0700, Jerry Hoemann wrote:
> > > Make whether or not the hpwdt watchdog delivers a pretimeout NMI
> > > programable by the user.
> > > 
> > > The underlying iLO hardware is programmable as to whether or not
> > > a pre-timeout NMI is delivered to the system before the iLO resets
> > > the system.  However, the iLO does not allow for programming the
> > > length of time that NMI is delivered before the system is reset.
> > > 
> > > Hence, in hpwdt_set_pretimeout, val == 0 disables the NMI. Any
> > > non-zero value sets the pretimeout length to what the hardware
> > > supports.
> > > 
> > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > > ---
> > >  drivers/watchdog/hpwdt.c | 42 ++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 36 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > > index da9a04101814..dc0ad20738ed 100644
> > > --- a/drivers/watchdog/hpwdt.c
> > > +++ b/drivers/watchdog/hpwdt.c
> > > @@ -28,12 +28,15 @@
> > >  #define TICKS_TO_SECS(ticks)		((ticks) * 128 / 1000)
> > >  #define HPWDT_MAX_TIMER			TICKS_TO_SECS(65535)
> > >  #define DEFAULT_MARGIN			30
> > > +#define PRETIMEOUT_SEC			9
> > >  
> > >  static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
> > > -static unsigned int reload;			/* the computed soft_margin */
> > >  static bool nowayout = WATCHDOG_NOWAYOUT;
> > >  #ifdef CONFIG_HPWDT_NMI_DECODING
> > >  static unsigned int allow_kdump = 1;
> > > +static bool pretimeout = 1;
> > > +#else
> > > +static bool pretimeout;
> > >  #endif
> > >  
> > static bool pretimeout = IS_ENABLED(CONFIG_HPWDT_NMI_DECODING);
> 
> ack. will do.
> 
> > 
> > >  static void __iomem *pci_mem_addr;		/* the PCI-memory address */
> > > @@ -55,10 +58,12 @@ static struct watchdog_device hpwdt_dev;
> > >   */
> > >  static int hpwdt_start(struct watchdog_device *dev)
> > >  {
> > > -	reload = SECS_TO_TICKS(dev->timeout);
> > > +	int control = 0x81 | (pretimeout ? 0x4 : 0);
> > > +	int reload = SECS_TO_TICKS(dev->timeout);
> > >  
> > > +	dev_dbg(dev->parent, "start watchdog 0x%08x:0x%02x\n", reload, control);
> > >  	iowrite16(reload, hpwdt_timer_reg);
> > > -	iowrite8(0x85, hpwdt_timer_con);
> > > +	iowrite8(control, hpwdt_timer_con);
> > >  
> > >  	return 0;
> > >  }
> > > @@ -67,6 +72,8 @@ static int hpwdt_stop(struct watchdog_device *dev)
> > >  {
> > >  	unsigned long data;
> > >  
> > > +	dev_dbg(dev->parent, "stop  watchdog\n");
> > > +
> > Unrelated.
> > 
> > >  	data = ioread8(hpwdt_timer_con);
> > >  	data &= 0xFE;
> > >  	iowrite8(data, hpwdt_timer_con);
> > > @@ -75,8 +82,9 @@ static int hpwdt_stop(struct watchdog_device *dev)
> > >  
> > >  static int hpwdt_ping(struct watchdog_device *dev)
> > >  {
> > > -	reload = SECS_TO_TICKS(dev->timeout);
> > > +	int reload = SECS_TO_TICKS(dev->timeout);
> > >  
> > > +	dev_dbg(dev->parent, "ping  watchdog 0x%08x\n", reload);
> > 
> > Unrelated. If you want to add debug messages, please do it
> > in a separate patch.
> 
> 
> Different patch, but same set?  I'll move these (and ones from earlier
> patch to a new separate patch later in set.)
> 
> > 
> > >  	iowrite16(reload, hpwdt_timer_reg);
> > >  
> > >  	return 0;
> > > @@ -98,12 +106,21 @@ static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
> > >  }
> > >  
> > >  #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> > > +static int hpwdt_set_pretimeout(struct watchdog_device *dev, unsigned int val)
> > > +{
> > > +	if (val && (val != PRETIMEOUT_SEC)) {
> > 
> > Unnecessary ( )
> 
> 
> There are several things going on here. I'm not sure which one the above
> comment is intended.
> 
The "Unnecessary" refers to the ( ) around the second part of the expression
above. While there may be valid reasons to include extra ( ), I think we
can trust the C compiler to get it right here.

> While a pretimeout NMI isn't required by the HW to be enabled, if enabled the
> length of pretimeout is fixed by HW.
> 
> I didn't see anything in the API that would allow us to communicate to
> the user this "feature."  timeout at leasst has both min_timeout and max_timeout, but
> I didn't see similar for pretimeout.  I also don't think its reasonable to fail
> here if the requested value is not 9 as the user really has no way of knowing what
> the valid range of pretimeout values are.  So I accept, any non-zero value
> for pretimeout, but then set pretimeout to be 9.
> 
> But at the same time, I don't like to siliently change a human request
> w/o at least warning.
> 
Sorry, I lost you here.

> > 
> > The actual timeout can be a value smaller than 9 seconds.
> > Minimum is 1 second. What happens if the user configures
> > a timeout of less than 9 seconds as well as a pretimeout ?
> > Will it fire immediately ? 
> 
> The architecture is silient on this issue.  My experience with
> this is that if timeout < 9 seconds, the NMI is not issued.
> System resets when the timeout expires.  This could be implementation
> dependent.
> 
> Note, this is not a new issue.
> 
Bad argument.

> I thought about setting the min timeout to ten seconds to avoid this situation.
> 
You could reject reject request to set the pretimeout to a value <= the
timeout.

> I haven't dug into the various user level clients of watchdog so I'm not sure 
> what the impact of making this change would be to them.
> 
> 
> > 
> > > +		dev_info(dev->parent, "Setting pretimeout to %d\n", PRETIMEOUT_SEC);
> > 
> > Please no ongoing logging noise. This can easily be abused to clog
> > the kernel log.
> 
> Good point.  I will look at WARN_ONCE or something similar.
> 
A traceback if someone sets a bad timeout ? That would be even worse.

Guenter

> > 
> > > +		val = PRETIMEOUT_SEC;
> > > +	}
> > > +	dev->pretimeout = val;
> > > +	pretimeout = val ? 1 : 0;
> > 
> > 	pretimeout = !!val;
> > 
> 
> -- 
> 
> -----------------------------------------------------------------------------
> Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
> -----------------------------------------------------------------------------

Re: [PATCH v3 08/11] watchdog/hpwdt: Programable Pretimeout NMI

[Jerry Hoemann]

On Fri, Feb 16, 2018 at 03:55:06PM -0800, Guenter Roeck wrote:
> On Fri, Feb 16, 2018 at 04:46:17PM -0700, Jerry Hoemann wrote:
> > On Fri, Feb 16, 2018 at 12:34:40PM -0800, Guenter Roeck wrote:
> > > On Thu, Feb 15, 2018 at 04:43:57PM -0700, Jerry Hoemann wrote:

...

> > > > @@ -98,12 +106,21 @@ static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
> > > >  }
> > > >  
> > > >  #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
> > > > +static int hpwdt_set_pretimeout(struct watchdog_device *dev, unsigned int val)
> > > > +{
> > > > +	if (val && (val != PRETIMEOUT_SEC)) {
> > > 
> > > Unnecessary ( )
> > 
> > 
> > There are several things going on here. I'm not sure which one the above
> > comment is intended.
> > 
> The "Unnecessary" refers to the ( ) around the second part of the expression
> above. While there may be valid reasons to include extra ( ), I think we
> can trust the C compiler to get it right here.

Okay, wasn't sure what you were getting at here.

I trust the C compiler, I don't trust humans.  In compound conditionals,
I'll add parens so that the meaning is clear.


> > While a pretimeout NMI isn't required by the HW to be enabled, if enabled the
> > length of pretimeout is fixed by HW.
> > 
> > I didn't see anything in the API that would allow us to communicate to
> > the user this "feature."  timeout at leasst has both min_timeout and max_timeout, but
> > I didn't see similar for pretimeout.  I also don't think its reasonable to fail
> > here if the requested value is not 9 as the user really has no way of knowing what
> > the valid range of pretimeout values are.  So I accept, any non-zero value
> > for pretimeout, but then set pretimeout to be 9.
> > 
> > But at the same time, I don't like to silently change a human request
> > w/o at least warning.
> > 
> Sorry, I lost you here.

I wasn't sure to what you were objecting to.  I thought you might
not have understood why I was converting non-zero values of
"pretimeout" to 9.  Was trying to explain the reasoning.

A problem I see with the watchdog API is that users don't know
what is an acceptable range of values for pretimeout.

For HPE proliant systems, one cannot just choose an arbitrary
value for pretimeout.

I don't see a reasonable way that a user can determine the valid range
for pretimeout for HPE systems given our hardware restrictions.


> 
> > > 
> > > The actual timeout can be a value smaller than 9 seconds.
> > > Minimum is 1 second. What happens if the user configures
> > > a timeout of less than 9 seconds as well as a pretimeout ?
> > > Will it fire immediately ? 
> > 
> > The architecture is silent on this issue.  My experience with
> > this is that if timeout < 9 seconds, the NMI is not issued.
> > System resets when the timeout expires.  This could be implementation
> > dependent.
> > 
> > Note, this is not a new issue.
> > 
> Bad argument.

Not sure exactly to what your objections are.  I'll point out that:

1) hpwdt has been using pretimeout NMI for watchdog for > 10 years.
2) For 8 years, its been possible to have a timeout < 9 seconds.
3) AFAIK this hasn't proven to be a big issue.
4) I have real questions as to how (or if) to address the issue.

I am perfectly willing to discuss the problem, but I don't think
it is a requirement for this patch set.


> 
> > I thought about setting the min timeout to ten seconds to avoid this situation.
> > 
> You could reject reject request to set the pretimeout to a value <= the
> timeout.

I think you mis-communicated here.

It is perfectly fine to have a timeout of 30 seconds with the pretimeout of 9 seconds.

> 
> > I haven't dug into the various user level clients of watchdog so I'm not sure 
> > what the impact of making this change would be to them.
> > 
> > 
> > > 
> > > > +		dev_info(dev->parent, "Setting pretimeout to %d\n", PRETIMEOUT_SEC);
> > > 
> > > Please no ongoing logging noise. This can easily be abused to clog
> > > the kernel log.
> > 
> > Good point.  I will look at WARN_ONCE or something similar.
> > 
> A traceback if someone sets a bad timeout ? That would be even worse.


I am thinking something more in line with setting a static variable if
the message had already been printed and not reprinting it.




-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v3 08/11] watchdog/hpwdt: Programable Pretimeout NMI

[Guenter Roeck]

On 02/16/2018 05:56 PM, Jerry Hoemann wrote:
> On Fri, Feb 16, 2018 at 03:55:06PM -0800, Guenter Roeck wrote:
>> On Fri, Feb 16, 2018 at 04:46:17PM -0700, Jerry Hoemann wrote:
>>> On Fri, Feb 16, 2018 at 12:34:40PM -0800, Guenter Roeck wrote:
>>>> On Thu, Feb 15, 2018 at 04:43:57PM -0700, Jerry Hoemann wrote:
> 
> ...
> 
>>>>> @@ -98,12 +106,21 @@ static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
>>>>>   }
>>>>>   
>>>>>   #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
>>>>> +static int hpwdt_set_pretimeout(struct watchdog_device *dev, unsigned int val)
>>>>> +{
>>>>> +	if (val && (val != PRETIMEOUT_SEC)) {
>>>>
>>>> Unnecessary ( )
>>>
>>>
>>> There are several things going on here. I'm not sure which one the above
>>> comment is intended.
>>>
>> The "Unnecessary" refers to the ( ) around the second part of the expression
>> above. While there may be valid reasons to include extra ( ), I think we
>> can trust the C compiler to get it right here.
> 
> Okay, wasn't sure what you were getting at here.
> 
> I trust the C compiler, I don't trust humans.  In compound conditionals,
> I'll add parens so that the meaning is clear.
> 

... and I don't accept patches with excessive ( ) if I catch them, because
it confuses me since I start looking for a meaning that isn't there.

> 
>>> While a pretimeout NMI isn't required by the HW to be enabled, if enabled the
>>> length of pretimeout is fixed by HW.
>>>
>>> I didn't see anything in the API that would allow us to communicate to
>>> the user this "feature."  timeout at leasst has both min_timeout and max_timeout, but
>>> I didn't see similar for pretimeout.  I also don't think its reasonable to fail
>>> here if the requested value is not 9 as the user really has no way of knowing what
>>> the valid range of pretimeout values are.  So I accept, any non-zero value
>>> for pretimeout, but then set pretimeout to be 9.
>>>
>>> But at the same time, I don't like to silently change a human request
>>> w/o at least warning.
>>>
>> Sorry, I lost you here.
> 
> I wasn't sure to what you were objecting to.  I thought you might
> not have understood why I was converting non-zero values of
> "pretimeout" to 9.  Was trying to explain the reasoning.
>  > A problem I see with the watchdog API is that users don't know
> what is an acceptable range of values for pretimeout.
> 
> For HPE proliant systems, one cannot just choose an arbitrary
> value for pretimeout.
> 
> I don't see a reasonable way that a user can determine the valid range
> for pretimeout for HPE systems given our hardware restrictions.
> 

I fully understand this, and I did not object to it. Watchdog drivers may and
are expected to adjust timeouts and pretimeouts to match hardware constraints,
and user space programs are expected to check the actual values after setting
timeouts. That is nothing unexpected or special.

> 
>>
>>>>
>>>> The actual timeout can be a value smaller than 9 seconds.
>>>> Minimum is 1 second. What happens if the user configures
>>>> a timeout of less than 9 seconds as well as a pretimeout ?
>>>> Will it fire immediately ?
>>>
>>> The architecture is silent on this issue.  My experience with
>>> this is that if timeout < 9 seconds, the NMI is not issued.
>>> System resets when the timeout expires.  This could be implementation
>>> dependent.
>>>
>>> Note, this is not a new issue.
>>>
>> Bad argument.
> 
> Not sure exactly to what your objections are.  I'll point out that:
> 
> 1) hpwdt has been using pretimeout NMI for watchdog for > 10 years.
> 2) For 8 years, its been possible to have a timeout < 9 seconds.
> 3) AFAIK this hasn't proven to be a big issue.
> 4) I have real questions as to how (or if) to address the issue.
> 

and I don't see the problem with returning EINVAL, ie rejecting the
pretimeout, if the selected timeout value is <= 9 seconds.

So the review found a problem (or maybe inconsistency), and you
refuse to fix it because you don't see the fix as part of this
patch set, even though it would literally require one or two lines
of code. Hmm.

> I am perfectly willing to discuss the problem, but I don't think
> it is a requirement for this patch set.
> 
Well, I think it is. But see below.

> 
>>
>>> I thought about setting the min timeout to ten seconds to avoid this situation.
>>>
>> You could reject reject request to set the pretimeout to a value <= the
>> timeout.
> 
> I think you mis-communicated here.
> 
I think you understand what I mean: reject setting a a pretimeout
if the timeout is set to be <= 9 seconds.

> It is perfectly fine to have a timeout of 30 seconds with the pretimeout of 9 seconds.
> 
>>
>>> I haven't dug into the various user level clients of watchdog so I'm not sure
>>> what the impact of making this change would be to them.
>>>
>>>
>>>>
>>>>> +		dev_info(dev->parent, "Setting pretimeout to %d\n", PRETIMEOUT_SEC);
>>>>
>>>> Please no ongoing logging noise. This can easily be abused to clog
>>>> the kernel log.
>>>
>>> Good point.  I will look at WARN_ONCE or something similar.
>>>
>> A traceback if someone sets a bad timeout ? That would be even worse.
> 
> 
> I am thinking something more in line with setting a static variable if
> the message had already been printed and not reprinting it.
> 

That is ok, and different to WARN_ONCE.

I think I'll take a step back at this point. I had also asked you to combine
a couple of patches (the include file removal and selecting WATCHDOG_CORE),
which you have not done. We are in severe bike-shedding territory, and that
affects my ability to look at the patches with a clear head.
I think I may ask Wim to step in and finalize the review.

Thanks,
Guenter

[PATCH v3 09/11] watchdog/hpwdt: condition early return of NMI handler on iLO5

[Jerry Hoemann]

Modify prior change to not claim an NMI unless originated
from iLO to apply only to iLO5 and later going forward.
This restores hpwdt traditional behavior of calling panic
if the NMI is NMI_IO_CHECK, NMI_SERR, or NMI_UNKNOWN for
legacy hardware.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index dc0ad20738ed..654e22b84c80 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -38,6 +38,7 @@ static bool pretimeout = 1;
 #else
 static bool pretimeout;
 #endif
+static bool iLO5;
 
 static void __iomem *pci_mem_addr;		/* the PCI-memory address */
 static unsigned long __iomem *hpwdt_nmistat;
@@ -135,14 +136,14 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 		"3. OA Forward Progress Log\n"
 		"4. iLO Event Log";
 
-	if ((ulReason == NMI_UNKNOWN) && !mynmi)
-		return NMI_DONE;
-
 	pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
 
 	if (!pretimeout)
 		return NMI_DONE;
 
+	if (iLO5 && (ulReason == NMI_UNKNOWN) && !mynmi)
+		return NMI_DONE;
+
 	if (allow_kdump)
 		hpwdt_stop(&hpwdt_dev);
 
@@ -275,6 +276,9 @@ static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 			", timer margin: %d seconds (nowayout=%d).\n",
 			HPWDT_VERSION, hpwdt_dev.timeout, nowayout);
 
+	if (dev->subsystem_vendor == PCI_VENDOR_ID_HP_3PAR)
+		iLO5 = 1;
+
 	return 0;
 
 error_wd_register:
-- 
2.13.6

[PATCH v3 10/11] watchdog/hpwdt: remove allow_kdump module parameter.

[Jerry Hoemann]

The intent of this parameter is unclear and it sets up a
race between the reset of the system by ASR and crashdump.

The length of time between receipt of the pretimeout NMI
and the ASR reset of the system is fixed by hardware.

Turning the parameter off doesn't necessairly prevent a crash dump.
Also, having the ASR reset occur while the system is crash dumping
doesn't imply that the dump was hung given the short duration
between the NMI and the reset.

This parameter is not a substitute for having a architected watchdog
crashdump hang detection paridigm.

Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
---
 drivers/watchdog/hpwdt.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 654e22b84c80..6e71106f4e4c 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -33,7 +33,6 @@
 static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
 static bool nowayout = WATCHDOG_NOWAYOUT;
 #ifdef CONFIG_HPWDT_NMI_DECODING
-static unsigned int allow_kdump = 1;
 static bool pretimeout = 1;
 #else
 static bool pretimeout;
@@ -144,8 +143,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 	if (iLO5 && (ulReason == NMI_UNKNOWN) && !mynmi)
 		return NMI_DONE;
 
-	if (allow_kdump)
-		hpwdt_stop(&hpwdt_dev);
+	hpwdt_stop(&hpwdt_dev);
 
 	hex_byte_pack(panic_msg, mynmi);
 	nmi_panic(regs, panic_msg);
@@ -184,10 +182,7 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
 	if (retval)
 		goto error2;
 
-	dev_info(&dev->dev,
-			"HPE Watchdog Timer Driver: NMI decoding initialized"
-			", allow kernel dump: %s (default = 1/ON)\n",
-			(allow_kdump == 0) ? "OFF" : "ON");
+	dev_info(&dev->dev, "HPE Watchdog Timer Driver: NMI decoding initialized\n");
 	return 0;
 
 error2:
@@ -346,9 +341,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 		__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
 #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
-module_param(allow_kdump, int, 0);
-MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
-
 module_param(pretimeout, bool, 0);
 MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
 #endif					/* } */
-- 
2.13.6

[PATCH v3 11/11] watchdog/hpwdt: Update driver version.

[Jerry Hoemann]

Update driver version number to reflect changes.

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 6e71106f4e4c..2c12e7a4ebc0 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -23,7 +23,7 @@
 
 #include <asm/nmi.h>
 
-#define HPWDT_VERSION			"1.4.0"
+#define HPWDT_VERSION			"2.0.0"
 #define SECS_TO_TICKS(secs)		((secs) * 1000 / 128)
 #define TICKS_TO_SECS(ticks)		((ticks) * 128 / 1000)
 #define HPWDT_MAX_TIMER			TICKS_TO_SECS(65535)
-- 
2.13.6