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

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

[Jerry Hoemann]

== v2 ==

1) Fix compiler error when CONFIG_HPWDT_NMI_DECODING is not defined.
Small non-white space changes to patches: 0001, 0006, 0008, 0009.

2) Break out driver version change to its own patch.
Small non-white space changes to patch 0008 and added 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: Modify to use watchdog core.
  watchdog/hpwdt: Select 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 | 847 ++++++++---------------------------------------
 2 files changed, 139 insertions(+), 709 deletions(-)

-- 
2.13.6

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

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

[PATCH v2 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 | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 20a13c5d0285..6f7949ef9a23 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -108,24 +108,35 @@ static int hpwdt_my_nmi(void)
 	return ioread8(hpwdt_nmistat) & 0x6;
 }
 
+static inline int hexdigit(int v)
+{
+	return (v > 9) ? (v-9+'A') : (v+'0');
+}
+
 /*
  *	NMI Handler
  */
 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");
+	panic_msg[0] = hexdigit((mynmi>>4)&0xf);
+	panic_msg[1] = hexdigit(mynmi&0xf);
+
+	nmi_panic(regs, panic_msg);
 
 	return NMI_HANDLED;
 }
-- 
2.13.6

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

[Guenter Roeck]

On Sun, Feb 11, 2018 at 10:21:03PM -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 | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 20a13c5d0285..6f7949ef9a23 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -108,24 +108,35 @@ static int hpwdt_my_nmi(void)
>  	return ioread8(hpwdt_nmistat) & 0x6;
>  }
>  
> +static inline int hexdigit(int v)
> +{
> +	return (v > 9) ? (v-9+'A') : (v+'0');
> +}
> +
>  /*
>   *	NMI Handler
>   */
>  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");
> +	panic_msg[0] = hexdigit((mynmi>>4)&0xf);
> +	panic_msg[1] = hexdigit(mynmi&0xf);

No need to reinvent the wheel.

	panic_msg[0] = hex_asc_hi(mynmi);
	panic_msg[1] = hex_asc_lo(mynmi);

or even better
	hex_byte_pack(panic_msg, mynmi);

There are matching _upper functions if you prefer A..F instead of a..f.

Guenter

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

[Jerry Hoemann]

On Tue, Feb 13, 2018 at 09:41:35AM -0800, Guenter Roeck wrote:
> On Sun, Feb 11, 2018 at 10:21:03PM -0700, Jerry Hoemann wrote:
> > +	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");
> > +	panic_msg[0] = hexdigit((mynmi>>4)&0xf);
> > +	panic_msg[1] = hexdigit(mynmi&0xf);
> 
> No need to reinvent the wheel.
> 
> 	panic_msg[0] = hex_asc_hi(mynmi);
> 	panic_msg[1] = hex_asc_lo(mynmi);
> 
> or even better
> 	hex_byte_pack(panic_msg, mynmi);
> 
> There are matching _upper functions if you prefer A..F instead of a..f.
> 
> Guenter

Will do. thanks

-- 

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

[PATCH v2 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 6f7949ef9a23..ee9a92220ece 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;
@@ -140,7 +138,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 
 	return NMI_HANDLED;
 }
-#endif /* CONFIG_HPWDT_NMI_DECODING */
+#endif					/* } */
 
 /*
  *	/dev/watchdog handling
@@ -205,11 +203,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,
@@ -223,7 +221,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;
 
@@ -320,7 +318,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:
@@ -334,15 +332,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;
 
@@ -429,10 +426,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");
@@ -447,9 +444,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: [PATCH v2 04/11] watchdog/hpwdt: white space changes

[Philippe Ombredanne]

Jerry,

On Mon, Feb 12, 2018 at 6:21 AM, Jerry Hoemann <jerry.hoemann@hpe.com> 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 6f7949ef9a23..ee9a92220ece 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

Since you are updating the copyright, why not updating the licensing
to use the new concise SPDX ids instead? [1]
And you get a big good karma bonus if you can also do the same for all
HPE-copyrighted files, tree-wide ;) and/or spread the word inside your
team.

Thank you for your kind consideration!

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

Cordially
Philippe Ombredanne

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

[Guenter Roeck]

On Mon, Feb 12, 2018 at 10:35:50AM +0100, Philippe Ombredanne wrote:
> Jerry,
> 
> On Mon, Feb 12, 2018 at 6:21 AM, Jerry Hoemann <jerry.hoemann@hpe.com> 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 6f7949ef9a23..ee9a92220ece 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
> 
> Since you are updating the copyright, why not updating the licensing
> to use the new concise SPDX ids instead? [1]
> And you get a big good karma bonus if you can also do the same for all
> HPE-copyrighted files, tree-wide ;) and/or spread the word inside your
> team.
> 

Different logical change, separate patch, please. Which, on a side note,
you might consider submitting yourself.

Thanks,
Guenter

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

[Jerry Hoemann]

Update Module Author and permission on parameters so that the
parameters show up in sysfs.

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

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index ee9a92220ece..dead59f9ca80 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -432,20 +432,20 @@ 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);
 
-module_param(soft_margin, int, 0);
+module_param(soft_margin, int, 0444);
 MODULE_PARM_DESC(soft_margin, "Watchdog timeout in seconds");
 
-module_param(nowayout, bool, 0);
+module_param(nowayout, bool, 0444);
 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_param(allow_kdump, int, 0444);
 MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
 #endif					/* } */
 
-- 
2.13.6

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

[Guenter Roeck]

On Sun, Feb 11, 2018 at 10:21:05PM -0700, Jerry Hoemann wrote:
> Update Module Author and permission on parameters so that the
> parameters show up in sysfs.
> 

Does it really add value to see the module parameters in sysfs ?
You would get both in standardized form by enabling WATCHDOG_SYSFS.

> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  drivers/watchdog/hpwdt.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index ee9a92220ece..dead59f9ca80 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -432,20 +432,20 @@ 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);
>  
> -module_param(soft_margin, int, 0);
> +module_param(soft_margin, int, 0444);
>  MODULE_PARM_DESC(soft_margin, "Watchdog timeout in seconds");
>  
> -module_param(nowayout, bool, 0);
> +module_param(nowayout, bool, 0444);
>  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_param(allow_kdump, int, 0444);
>  MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
>  #endif					/* } */
>  
> -- 
> 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 v2 05/11] watchdog/hpwdt: Update Module info.

[Jerry Hoemann]

On Tue, Feb 13, 2018 at 09:49:50AM -0800, Guenter Roeck wrote:
> On Sun, Feb 11, 2018 at 10:21:05PM -0700, Jerry Hoemann wrote:
> > Update Module Author and permission on parameters so that the
> > parameters show up in sysfs.
> > 
> 
> Does it really add value to see the module parameters in sysfs ?
> You would get both in standardized form by enabling WATCHDOG_SYSFS.


Not as much value as it did before I knew about WATCHDOG_SYSFS.  :)

It might still offer some unit level debug insight.
The majority of users of module_param are specifying read permissions,
so there is a uniformity aspect to having it.

But, not a big deal.  I'll remove read permission change if you want me to.




-- 

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

[PATCH v2 06/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 | 251 ++++++++++++-----------------------------------
 1 file changed, 63 insertions(+), 188 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index dead59f9ca80..740d0c633204 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)
+{
+	pr_debug("settimeout = %d\n", val);
+
+	soft_margin = 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;
 }
@@ -128,8 +126,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);
 
 	panic_msg[0] = hexdigit((mynmi>>4)&0xf);
 	panic_msg[1] = hexdigit(mynmi&0xf);
@@ -140,68 +140,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    |
@@ -210,90 +148,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	/* { */
@@ -311,10 +169,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");
 	return 0;
 
 error2:
@@ -381,31 +236,32 @@ static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 	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);
+	hpwdt_stop(&hpwdt_dev);
 
 	/* Initialize NMI Decoding functionality */
 	retval = hpwdt_init_nmi_decoding(dev);
 	if (retval != 0)
 		goto error_init_nmi_decoding;
 
-	retval = misc_register(&hpwdt_miscdev);
+	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;
+	}
+
+	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);
+		soft_margin = DEFAULT_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);
@@ -417,9 +273,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);
@@ -432,6 +288,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: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

[Marcus Folkesson]

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

Hi Jerry,

On Sun, Feb 11, 2018 at 10:21:06PM -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>

I think it would be better to use
    dev_emerg()
    dev_crit()
    dev_alert()
    dev_err()
    dev_warn()
    dev_notice()

instead of pr_* functions now when we have a device to use.

> ---
>  drivers/watchdog/hpwdt.c | 251 ++++++++++++-----------------------------------
>  1 file changed, 63 insertions(+), 188 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index dead59f9ca80..740d0c633204 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)
> +{
> +	pr_debug("settimeout = %d\n", val);

dev_dbg()


> +
> +	soft_margin = dev->timeout = val;

No need to update soft_margin

> +	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;
>  }
> @@ -128,8 +126,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);

dev_dbg()

> +
>  	if (allow_kdump)
> -		hpwdt_stop();
> +		hpwdt_stop(&hpwdt_dev);
>  
>  	panic_msg[0] = hexdigit((mynmi>>4)&0xf);
>  	panic_msg[1] = hexdigit(mynmi&0xf);
> @@ -140,68 +140,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    |
> @@ -210,90 +148,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	/* { */
> @@ -311,10 +169,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");
>  	return 0;
>  
>  error2:
> @@ -381,31 +236,32 @@ static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
>  	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);
> +	hpwdt_stop(&hpwdt_dev);
>  
>  	/* Initialize NMI Decoding functionality */
>  	retval = hpwdt_init_nmi_decoding(dev);
>  	if (retval != 0)
>  		goto error_init_nmi_decoding;
>  
> -	retval = misc_register(&hpwdt_miscdev);
> +	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;
> +	}
> +
> +	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);
> +		soft_margin = DEFAULT_MARGIN;
>  	}

In this case watchdog_init_timout() will:
1) Check if soft_margin is valid
2) if not, keep hpwdt_dev.timout intact (which is already set in
declaration of hpwdt_dev)

So we could remove the condition and only keep
watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);


Also, we need to set an invalid initial value for soft_margin to make
the logic for watchdog_init_timeout work. 

i.e
- static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
+ static unsigned int soft_margin;

>  
>  	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
>  			", timer margin: %d seconds (nowayout=%d).\n",
> -			HPWDT_VERSION, soft_margin, nowayout);

print hpwdt_dev.timeout instead of soft_margin

> +			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);
> @@ -417,9 +273,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);
> @@ -432,6 +288,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
> 

Best regards
Marcus Folkesson

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

Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

[Guenter Roeck]

On Mon, Feb 12, 2018 at 10:06:21AM +0100, Marcus Folkesson wrote:
> Hi Jerry,
> 
> On Sun, Feb 11, 2018 at 10:21:06PM -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>
> 
I second Marcus' feedback.

Thanks,
Guenter

> I think it would be better to use
>     dev_emerg()
>     dev_crit()
>     dev_alert()
>     dev_err()
>     dev_warn()
>     dev_notice()
> 
> instead of pr_* functions now when we have a device to use.
> 
> > ---
> >  drivers/watchdog/hpwdt.c | 251 ++++++++++++-----------------------------------
> >  1 file changed, 63 insertions(+), 188 deletions(-)
> > 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index dead59f9ca80..740d0c633204 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)
> > +{
> > +	pr_debug("settimeout = %d\n", val);
> 
> dev_dbg()
> 
> 
> > +
> > +	soft_margin = dev->timeout = val;
> 
> No need to update soft_margin
> 
> > +	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;
> >  }
> > @@ -128,8 +126,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);
> 
> dev_dbg()
> 
> > +
> >  	if (allow_kdump)
> > -		hpwdt_stop();
> > +		hpwdt_stop(&hpwdt_dev);
> >  
> >  	panic_msg[0] = hexdigit((mynmi>>4)&0xf);
> >  	panic_msg[1] = hexdigit(mynmi&0xf);
> > @@ -140,68 +140,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    |
> > @@ -210,90 +148,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	/* { */
> > @@ -311,10 +169,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");
> >  	return 0;
> >  
> >  error2:
> > @@ -381,31 +236,32 @@ static int hpwdt_probe(struct pci_dev *dev, const struct pci_device_id *ent)
> >  	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);
> > +	hpwdt_stop(&hpwdt_dev);
> >  
> >  	/* Initialize NMI Decoding functionality */
> >  	retval = hpwdt_init_nmi_decoding(dev);
> >  	if (retval != 0)
> >  		goto error_init_nmi_decoding;
> >  
> > -	retval = misc_register(&hpwdt_miscdev);
> > +	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;
> > +	}
> > +
> > +	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);
> > +		soft_margin = DEFAULT_MARGIN;
> >  	}
> 
> In this case watchdog_init_timout() will:
> 1) Check if soft_margin is valid
> 2) if not, keep hpwdt_dev.timout intact (which is already set in
> declaration of hpwdt_dev)
> 
> So we could remove the condition and only keep
> watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);
> 
> 
> Also, we need to set an invalid initial value for soft_margin to make
> the logic for watchdog_init_timeout work. 
> 
> i.e
> - static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
> + static unsigned int soft_margin;
> 
> >  
> >  	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
> >  			", timer margin: %d seconds (nowayout=%d).\n",
> > -			HPWDT_VERSION, soft_margin, nowayout);
> 
> print hpwdt_dev.timeout instead of soft_margin
> 
> > +			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);
> > @@ -417,9 +273,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);
> > @@ -432,6 +288,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
> > 
> 
> Best regards
> Marcus Folkesson

Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

[Jerry Hoemann]

Thanks for the review. Comments inline.

On Mon, Feb 12, 2018 at 10:06:21AM +0100, Marcus Folkesson wrote:
> Hi Jerry,
> 
> On Sun, Feb 11, 2018 at 10:21:06PM -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>
> 
> I think it would be better to use
>     dev_emerg()
>     dev_crit()
>     dev_alert()
>     dev_err()
>     dev_warn()
>     dev_notice()
> 
> instead of pr_* functions now when we have a device to use.

In general, is there something bad with using pr_debug, etc.,?

When converting the driver over, I had many more debug messages being
printed from locations where I didn't have a valid device handle and
those needed to be done w/ the pr_.* functions.  So, I just uniformally
used pr_.*.


> > -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)
> > +{
> > +	pr_debug("settimeout = %d\n", val);
> 
> dev_dbg()

I think you are proposing I do:

	dev_dbg(dev->parent, "settimeout = %d\n", val);

This raises the issue that I didn't set parent and I believe I should have.


> 
> 
> > +
> > +	soft_margin = dev->timeout = val;
> 
> No need to update soft_margin

I'd made the permission on the module parameters 0444 so that they
would show up in sysfs.  I updated this value so that I could see
current value of timeout in sysfs.  But, as Guenter points out in a
later review, these values are available in under CONFIG_WATCHDOG_SYSFS.
So, I'll remove.


> > +	pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
> 
> dev_dbg()

See above.

> 
> >  	retval = hpwdt_init_nmi_decoding(dev);
> >  	if (retval != 0)
> >  		goto error_init_nmi_decoding;
> >  
> > -	retval = misc_register(&hpwdt_miscdev);
> > +	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;
> > +	}
> > +
> > +	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);
> > +		soft_margin = DEFAULT_MARGIN;
> >  	}
> 
> In this case watchdog_init_timout() will:
> 1) Check if soft_margin is valid
> 2) if not, keep hpwdt_dev.timout intact (which is already set in
> declaration of hpwdt_dev)
> 
> So we could remove the condition and only keep
> watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);
> 
> 
> Also, we need to set an invalid initial value for soft_margin to make
> the logic for watchdog_init_timeout work. 
> 
> i.e
> - static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
> + static unsigned int soft_margin;


I don't see the core printing a warning message if an invalid value
is specified for soft_margin when loading the module.

I don't mind the hpwdt module correcting an out of range parameter, but I
don't think it should do so siliently.


> 
> >  
> >  	dev_info(&dev->dev, "HPE Watchdog Timer Driver: %s"
> >  			", timer margin: %d seconds (nowayout=%d).\n",
> > -			HPWDT_VERSION, soft_margin, nowayout);
> 
> print hpwdt_dev.timeout instead of soft_margin
> 


Will do.



-- 

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

Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

[Guenter Roeck]

On Tue, Feb 13, 2018 at 02:36:48PM -0700, Jerry Hoemann wrote:
> 
> 
> Thanks for the review. Comments inline.
> 
> On Mon, Feb 12, 2018 at 10:06:21AM +0100, Marcus Folkesson wrote:
> > Hi Jerry,
> > 
> > On Sun, Feb 11, 2018 at 10:21:06PM -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>
> > 
> > I think it would be better to use
> >     dev_emerg()
> >     dev_crit()
> >     dev_alert()
> >     dev_err()
> >     dev_warn()
> >     dev_notice()
> > 
> > instead of pr_* functions now when we have a device to use.
> 
> In general, is there something bad with using pr_debug, etc.,?
> 

No, but it is desirable to use dev_ functions where possible.

> When converting the driver over, I had many more debug messages being
> printed from locations where I didn't have a valid device handle and
> those needed to be done w/ the pr_.* functions.  So, I just uniformally
> used pr_.*.
> 
> 
> > > -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)
> > > +{
> > > +	pr_debug("settimeout = %d\n", val);
> > 
> > dev_dbg()
> 
> I think you are proposing I do:
> 
> 	dev_dbg(dev->parent, "settimeout = %d\n", val);
> 
> This raises the issue that I didn't set parent and I believe I should have.
> 
Correct.

> 
> > 
> > 
> > > +
> > > +	soft_margin = dev->timeout = val;
> > 
> > No need to update soft_margin
> 
> I'd made the permission on the module parameters 0444 so that they
> would show up in sysfs.  I updated this value so that I could see
> current value of timeout in sysfs.  But, as Guenter points out in a
> later review, these values are available in under CONFIG_WATCHDOG_SYSFS.
> So, I'll remove.
> 
> 
> > > +	pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
> > 
> > dev_dbg()
> 
> See above.
> 
> > 
> > >  	retval = hpwdt_init_nmi_decoding(dev);
> > >  	if (retval != 0)
> > >  		goto error_init_nmi_decoding;
> > >  
> > > -	retval = misc_register(&hpwdt_miscdev);
> > > +	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;
> > > +	}
> > > +
> > > +	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);
> > > +		soft_margin = DEFAULT_MARGIN;
> > >  	}
> > 
> > In this case watchdog_init_timout() will:
> > 1) Check if soft_margin is valid
> > 2) if not, keep hpwdt_dev.timout intact (which is already set in
> > declaration of hpwdt_dev)
> > 
> > So we could remove the condition and only keep
> > watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);
> > 
> > 
> > Also, we need to set an invalid initial value for soft_margin to make
> > the logic for watchdog_init_timeout work. 
> > 
> > i.e
> > - static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
> > + static unsigned int soft_margin;
> 
> 
> I don't see the core printing a warning message if an invalid value
> is specified for soft_margin when loading the module.
> 
> I don't mind the hpwdt module correcting an out of range parameter, but I
> don't think it should do so siliently.
> 

The point here is that setting soft_margin to 0 and setting the
timeout in the watchdog_device structure to DEFAULT_MARGIN
means that it is not necessary to override it on error.
If you want to be verbose, you can still log a warning message
if the timeout provided by the module parameter is wrong.

Anyway, this driver will presumably never support devicetree,
so the watchdog core will never read the timeout from there,
and this is not a showstopper.

Guenter

Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

[Jerry Hoemann]

On Tue, Feb 13, 2018 at 01:55:49PM -0800, Guenter Roeck wrote:
> On Tue, Feb 13, 2018 at 02:36:48PM -0700, Jerry Hoemann wrote:
> > 
> > 
> > > 
> > > dev_dbg()
> > 
> > I think you are proposing I do:
> > 
> > 	dev_dbg(dev->parent, "settimeout = %d\n", val);
> > 
> > This raises the issue that I didn't set parent and I believe I should have.
> > 
> Correct.

Will fix.


> > > >  	retval = hpwdt_init_nmi_decoding(dev);
> > > >  	if (retval != 0)
> > > >  		goto error_init_nmi_decoding;
> > > >  
> > > > -	retval = misc_register(&hpwdt_miscdev);
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	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);
> > > > +		soft_margin = DEFAULT_MARGIN;
> > > >  	}
> > > 
> > > In this case watchdog_init_timout() will:
> > > 1) Check if soft_margin is valid
> > > 2) if not, keep hpwdt_dev.timout intact (which is already set in
> > > declaration of hpwdt_dev)
> > > 
> > > So we could remove the condition and only keep
> > > watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);
> > > 
> > > 
> > > Also, we need to set an invalid initial value for soft_margin to make
> > > the logic for watchdog_init_timeout work. 
> > > 
> > > i.e
> > > - static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
> > > + static unsigned int soft_margin;
> > 
> > 
> > I don't see the core printing a warning message if an invalid value
> > is specified for soft_margin when loading the module.
> > 
> > I don't mind the hpwdt module correcting an out of range parameter, but I
> > don't think it should do so siliently.
> > 
> 
> The point here is that setting soft_margin to 0 and setting the
> timeout in the watchdog_device structure to DEFAULT_MARGIN
> means that it is not necessary to override it on error.

Yes, I don't need to set soft_margin = DEFAULT_MARGIN inside the
conditional on watchdog_init_timeout.  I was doing that to keep
the value of soft_margin consistent with the value of hpwdt_dev.timeout
so that it would show the current timeout value in sysfs.  But, as
you pointed out, the watchdog sysfs displays timeout.  So, i don't
need that asignment anymore.

But, I want to keep the initialization:
static unsigned int soft_margin = DEFAULT_MARGIN;


Reasoning: I want soft_margin to be an optional parameter that
defaults to DEFAULT_MARGIN without warning.  If soft_margin is initialized
to a value outside of the valid range, say N, the code can't tell the
difference between the static value of N or if the module was loaded with the
soft_margin=N.  I only want to warn on the latter.


> If you want to be verbose, you can still log a warning message
> if the timeout provided by the module parameter is wrong.
> 
> Anyway, this driver will presumably never support devicetree,
> so the watchdog core will never read the timeout from there,
> and this is not a showstopper.
> 



-- 

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

Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

[Jerry Hoemann]

On Mon, Feb 12, 2018 at 10:06:21AM +0100, Marcus Folkesson wrote:
> Hi Jerry,
> 
> On Sun, Feb 11, 2018 at 10:21:06PM -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>
> 
> I think it would be better to use
>     dev_emerg()
>     dev_crit()
>     dev_alert()
>     dev_err()
>     dev_warn()
>     dev_notice()
> 
> instead of pr_* functions now when we have a device to use.



> >  }
> > @@ -128,8 +126,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);
> 
> dev_dbg()
> 

Sorry, wasn't clear on my earlier response.  As hpwdt_pretiemout isn't
being passed a device, this instance will still be a pr_debug.  I have
converted the others to dev_.* in the next version of the series.




-- 

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

[PATCH v2 07/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: [PATCH v2 07/11] watchdog/hpwdt: Select WATCHDOG_CORE

[Guenter Roeck]

On Sun, Feb 11, 2018 at 10:21:07PM -0700, Jerry Hoemann wrote:
> Update Kconfig file to show that hpwdt now selects WATCHDOG_CORE.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>

Please merge with previous patch to preserve bisectability.

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

[PATCH v2 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 740d0c633204..e7355f72d883 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);
 
+	pr_debug("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;
 
+	pr_debug("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);
 
+	pr_debug("ping  watchdog 0x%08x\n", reload);
 	iowrite16(reload, hpwdt_timer_reg);
 
 	return 0;
@@ -98,6 +106,16 @@ 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)) {
+		pr_info("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)
 {
@@ -108,7 +126,6 @@ static inline int hexdigit(int v)
 {
 	return (v > 9) ? (v-9+'A') : (v+'0');
 }
-
 /*
  *	NMI Handler
  */
@@ -128,6 +145,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);
 
@@ -144,7 +164,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",
 };
 
@@ -296,6 +317,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 = {
@@ -304,6 +328,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					/* } */
 };
 
 
@@ -322,6 +349,9 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
 module_param(allow_kdump, int, 0444);
 MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
+
+module_param(pretimeout, bool, 0444);
+MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
 #endif					/* } */
 
 module_pci_driver(hpwdt_driver);
-- 
2.13.6

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

[Marcus Folkesson]

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

Hi Jerry,

On Sun, Feb 11, 2018 at 10:21:08PM -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 740d0c633204..e7355f72d883 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);
>  
> +	pr_debug("start watchdog 0x%08x:0x%02x\n", reload, control);

dev_dbg()

Even here, I think we should use 
    dev_emerg()
    dev_crit()
    dev_alert()
    dev_err()
    dev_warn()
    dev_notice()

instead of pr_* functions now when we have a device to use.


>  	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;
>  
> +	pr_debug("stop  watchdog\n");
dev_dbg()

> +
>  	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);
>  
> +	pr_debug("ping  watchdog 0x%08x\n", reload);
>  	iowrite16(reload, hpwdt_timer_reg);
>  
>  	return 0;
> @@ -98,6 +106,16 @@ 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)) {
> +		pr_info("Setting pretimeout to %d\n", PRETIMEOUT_SEC);

dev_info()

> +		val = PRETIMEOUT_SEC;
> +	}
> +	dev->pretimeout = val;
> +	pretimeout = val ? 1 : 0;
> +	return 0;
> +}
>  
>  static unsigned int hpwdt_my_nmi(void)
>  {
> @@ -108,7 +126,6 @@ static inline int hexdigit(int v)
>  {
>  	return (v > 9) ? (v-9+'A') : (v+'0');
>  }
> -
>  /*
>   *	NMI Handler
>   */
> @@ -128,6 +145,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);
>  
> @@ -144,7 +164,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",
>  };
>  
> @@ -296,6 +317,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 = {
> @@ -304,6 +328,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					/* } */
>  };
>  
>  
> @@ -322,6 +349,9 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>  #ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
>  module_param(allow_kdump, int, 0444);
>  MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
> +
> +module_param(pretimeout, bool, 0444);
> +MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
>  #endif					/* } */
>  
>  module_pci_driver(hpwdt_driver);
> -- 
> 2.13.6

Best regards
Marcus Folkesson
> 

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

[PATCH v2 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 e7355f72d883..e9e54fe20804 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;
@@ -140,14 +141,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);
 
@@ -280,6 +281,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 v2 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 | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index e9e54fe20804..bb0dcc8709b8 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;
@@ -149,8 +148,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);
 
 	panic_msg[0] = hexdigit((mynmi>>4)&0xf);
 	panic_msg[1] = hexdigit(mynmi&0xf);
@@ -351,9 +349,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, 0444);
-MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
-
 module_param(pretimeout, bool, 0444);
 MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
 #endif					/* } */
-- 
2.13.6

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

[Guenter Roeck]

On Sun, Feb 11, 2018 at 10:21:10PM -0700, Jerry Hoemann wrote:
> 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 | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index e9e54fe20804..bb0dcc8709b8 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;

At the end of hpwdt_init_nmi_decoding(), there used to be a log message
showing the value of allow_kdump. Maybe that was removed in another patch,
but it really belongs to this patch, or am I missing something ?

Guenter

>  static bool pretimeout = 1;
>  #else
>  static bool pretimeout;
> @@ -149,8 +148,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);
>  
>  	panic_msg[0] = hexdigit((mynmi>>4)&0xf);
>  	panic_msg[1] = hexdigit(mynmi&0xf);
> @@ -351,9 +349,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, 0444);
> -MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
> -
>  module_param(pretimeout, bool, 0444);
>  MODULE_PARM_DESC(pretimeout, "Watchdog pretimeout enabled");
>  #endif					/* } */
> -- 
> 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 v2 10/11] watchdog/hpwdt: remove allow_kdump module parameter.

[Jerry Hoemann]

On Tue, Feb 13, 2018 at 10:20:42AM -0800, Guenter Roeck wrote:
> On Sun, Feb 11, 2018 at 10:21:10PM -0700, Jerry Hoemann wrote:
> > 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 | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index e9e54fe20804..bb0dcc8709b8 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;
> 
> At the end of hpwdt_init_nmi_decoding(), there used to be a log message
> showing the value of allow_kdump. Maybe that was removed in another patch,
> but it really belongs to this patch, or am I missing something ?
> 
> Guenter

That got pulled into patch 6.  I'll refactor the patches to include the
message change here.



-- 

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

[PATCH v2 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 bb0dcc8709b8..78168e2f9b4e 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

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

[Guenter Roeck]

On Sun, Feb 11, 2018 at 10:21:11PM -0700, Jerry Hoemann wrote:
> 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 bb0dcc8709b8..78168e2f9b4e 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"

I would suggest to drop the version entirely. History and experience shows
that it is not really useful.

Guenter

>  #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
> 
> --
> 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 v2 11/11] watchdog/hpwdt: Update driver version.

[Jerry Hoemann]

On Tue, Feb 13, 2018 at 09:07:25AM -0800, Guenter Roeck wrote:
> On Sun, Feb 11, 2018 at 10:21:11PM -0700, Jerry Hoemann wrote:
> > 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 bb0dcc8709b8..78168e2f9b4e 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"
> 
> I would suggest to drop the version entirely. History and experience shows
> that it is not really useful.
> 
> Guenter
> 

HPE can and does replace drivers w/o updating the underlying kernel version.

So having driver print its version has been very useful to me in debugging
issues in other drivers.  I want to maintain that in here.


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

-- 

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