All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
@ 2022-06-02 17:53 ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux, tzungbi, brking, nathanl, aik, npiggin, vaishnavi, wvoigt,
	linuxppc-dev

PAPR v2.12 defines a new hypercall, H_WATCHDOG.  This patch series
adds support for this hypercall to powerpc/pseries kernels and
introduces a new watchdog driver, "pseries-wdt", for the virtual
timers exposed by the hypercall.

This series is preceded by the following:

RFC v1: https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-cheloha@linux.ibm.com/
RFC v2: https://lore.kernel.org/linux-watchdog/20220509174357.5448-1-cheloha@linux.ibm.com/
PATCH v1: https://lore.kernel.org/linux-watchdog/20220520183552.33426-1-cheloha@linux.ibm.com/

Changes of note from PATCH v1:

- Trim down the large comment documenting the H_WATCHDOG hypercall.
  The comment is likely to rot, so remove anything we aren't using
  and anything overly obvious.

- Remove any preprocessor definitions not actually used in the module
  right now.  If we want to use other features offered by the hypercall
  we can add them in later.  They're just clutter until then.

- Simplify the "action" module parameter.  The value is now an index
  into an array of possible timeoutAction values.  This design removes
  the need for the custom get/set methods used in PATCH v1.

  Now we merely need to check that the "action" value is a valid
  index during pseries_wdt_probe().  Easy.

- Make the timeoutAction a member of pseries_wdt, "action".  This
  eliminates the use of a global variable during pseries_wdt_start().

- Use watchdog_init_timeout() idiomatically.  Check its return value
  and error out of pseries_wdt_probe() if it fails.



^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 0/4] pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
@ 2022-06-02 17:53 ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: nathanl, wvoigt, aik, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux

PAPR v2.12 defines a new hypercall, H_WATCHDOG.  This patch series
adds support for this hypercall to powerpc/pseries kernels and
introduces a new watchdog driver, "pseries-wdt", for the virtual
timers exposed by the hypercall.

This series is preceded by the following:

RFC v1: https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-cheloha@linux.ibm.com/
RFC v2: https://lore.kernel.org/linux-watchdog/20220509174357.5448-1-cheloha@linux.ibm.com/
PATCH v1: https://lore.kernel.org/linux-watchdog/20220520183552.33426-1-cheloha@linux.ibm.com/

Changes of note from PATCH v1:

- Trim down the large comment documenting the H_WATCHDOG hypercall.
  The comment is likely to rot, so remove anything we aren't using
  and anything overly obvious.

- Remove any preprocessor definitions not actually used in the module
  right now.  If we want to use other features offered by the hypercall
  we can add them in later.  They're just clutter until then.

- Simplify the "action" module parameter.  The value is now an index
  into an array of possible timeoutAction values.  This design removes
  the need for the custom get/set methods used in PATCH v1.

  Now we merely need to check that the "action" value is a valid
  index during pseries_wdt_probe().  Easy.

- Make the timeoutAction a member of pseries_wdt, "action".  This
  eliminates the use of a global variable during pseries_wdt_start().

- Use watchdog_init_timeout() idiomatically.  Check its return value
  and error out of pseries_wdt_probe() if it fails.



^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 1/4] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code
  2022-06-02 17:53 ` Scott Cheloha
@ 2022-06-02 17:53   ` Scott Cheloha
  -1 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux, tzungbi, brking, nathanl, aik, npiggin, vaishnavi, wvoigt,
	linuxppc-dev, Scott Cheloha

PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
guest control of one or more virtual watchdog timers.

Add the opcode for the H_WATCHDOG hypercall to hvcall.h.  While here,
add a definition for H_NOOP, a possible return code for H_WATCHDOG.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
 arch/powerpc/include/asm/hvcall.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index d92a20a85395..4b4f69c35b4f 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -87,6 +87,7 @@
 #define H_P7		-60
 #define H_P8		-61
 #define H_P9		-62
+#define H_NOOP		-63
 #define H_TOO_BIG	-64
 #define H_UNSUPPORTED	-67
 #define H_OVERLAP	-68
@@ -324,7 +325,8 @@
 #define H_RPT_INVALIDATE	0x448
 #define H_SCM_FLUSH		0x44C
 #define H_GET_ENERGY_SCALE_INFO	0x450
-#define MAX_HCALL_OPCODE	H_GET_ENERGY_SCALE_INFO
+#define H_WATCHDOG		0x45C
+#define MAX_HCALL_OPCODE	H_WATCHDOG
 
 /* Scope args for H_SCM_UNBIND_ALL */
 #define H_UNBIND_SCOPE_ALL (0x1)
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 1/4] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code
@ 2022-06-02 17:53   ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: nathanl, wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi,
	brking, linuxppc-dev, linux

PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
guest control of one or more virtual watchdog timers.

Add the opcode for the H_WATCHDOG hypercall to hvcall.h.  While here,
add a definition for H_NOOP, a possible return code for H_WATCHDOG.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
 arch/powerpc/include/asm/hvcall.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index d92a20a85395..4b4f69c35b4f 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -87,6 +87,7 @@
 #define H_P7		-60
 #define H_P8		-61
 #define H_P9		-62
+#define H_NOOP		-63
 #define H_TOO_BIG	-64
 #define H_UNSUPPORTED	-67
 #define H_OVERLAP	-68
@@ -324,7 +325,8 @@
 #define H_RPT_INVALIDATE	0x448
 #define H_SCM_FLUSH		0x44C
 #define H_GET_ENERGY_SCALE_INFO	0x450
-#define MAX_HCALL_OPCODE	H_GET_ENERGY_SCALE_INFO
+#define H_WATCHDOG		0x45C
+#define MAX_HCALL_OPCODE	H_WATCHDOG
 
 /* Scope args for H_SCM_UNBIND_ALL */
 #define H_UNBIND_SCOPE_ALL (0x1)
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 2/4] powerpc/pseries: add FW_FEATURE_WATCHDOG flag
  2022-06-02 17:53 ` Scott Cheloha
@ 2022-06-02 17:53   ` Scott Cheloha
  -1 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux, tzungbi, brking, nathanl, aik, npiggin, vaishnavi, wvoigt,
	linuxppc-dev, Scott Cheloha

PAPR v2.12 specifies a new optional function set, "hcall-watchdog",
for the /rtas/ibm,hypertas-functions property.  The presence of this
function set indicates support for the H_WATCHDOG hypercall.

Check for this function set and, if present, set the new
FW_FEATURE_WATCHDOG flag.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
 arch/powerpc/include/asm/firmware.h       | 3 ++-
 arch/powerpc/platforms/pseries/firmware.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h
index 8dddd34b8ecf..398e0b5e485f 100644
--- a/arch/powerpc/include/asm/firmware.h
+++ b/arch/powerpc/include/asm/firmware.h
@@ -55,6 +55,7 @@
 #define FW_FEATURE_RPT_INVALIDATE ASM_CONST(0x0000010000000000)
 #define FW_FEATURE_FORM2_AFFINITY ASM_CONST(0x0000020000000000)
 #define FW_FEATURE_ENERGY_SCALE_INFO ASM_CONST(0x0000040000000000)
+#define FW_FEATURE_WATCHDOG	ASM_CONST(0x0000080000000000)
 
 #ifndef __ASSEMBLY__
 
@@ -76,7 +77,7 @@ enum {
 		FW_FEATURE_DRC_INFO | FW_FEATURE_BLOCK_REMOVE |
 		FW_FEATURE_PAPR_SCM | FW_FEATURE_ULTRAVISOR |
 		FW_FEATURE_RPT_INVALIDATE | FW_FEATURE_FORM2_AFFINITY |
-		FW_FEATURE_ENERGY_SCALE_INFO,
+		FW_FEATURE_ENERGY_SCALE_INFO | FW_FEATURE_WATCHDOG,
 	FW_FEATURE_PSERIES_ALWAYS = 0,
 	FW_FEATURE_POWERNV_POSSIBLE = FW_FEATURE_OPAL | FW_FEATURE_ULTRAVISOR,
 	FW_FEATURE_POWERNV_ALWAYS = 0,
diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c
index 09c119b2f623..080108d129ed 100644
--- a/arch/powerpc/platforms/pseries/firmware.c
+++ b/arch/powerpc/platforms/pseries/firmware.c
@@ -67,6 +67,7 @@ hypertas_fw_features_table[] = {
 	{FW_FEATURE_PAPR_SCM,		"hcall-scm"},
 	{FW_FEATURE_RPT_INVALIDATE,	"hcall-rpt-invalidate"},
 	{FW_FEATURE_ENERGY_SCALE_INFO,	"hcall-energy-scale-info"},
+	{FW_FEATURE_WATCHDOG,		"hcall-watchdog"},
 };
 
 /* Build up the firmware features bitmask using the contents of
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 2/4] powerpc/pseries: add FW_FEATURE_WATCHDOG flag
@ 2022-06-02 17:53   ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: nathanl, wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi,
	brking, linuxppc-dev, linux

PAPR v2.12 specifies a new optional function set, "hcall-watchdog",
for the /rtas/ibm,hypertas-functions property.  The presence of this
function set indicates support for the H_WATCHDOG hypercall.

Check for this function set and, if present, set the new
FW_FEATURE_WATCHDOG flag.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
 arch/powerpc/include/asm/firmware.h       | 3 ++-
 arch/powerpc/platforms/pseries/firmware.c | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h
index 8dddd34b8ecf..398e0b5e485f 100644
--- a/arch/powerpc/include/asm/firmware.h
+++ b/arch/powerpc/include/asm/firmware.h
@@ -55,6 +55,7 @@
 #define FW_FEATURE_RPT_INVALIDATE ASM_CONST(0x0000010000000000)
 #define FW_FEATURE_FORM2_AFFINITY ASM_CONST(0x0000020000000000)
 #define FW_FEATURE_ENERGY_SCALE_INFO ASM_CONST(0x0000040000000000)
+#define FW_FEATURE_WATCHDOG	ASM_CONST(0x0000080000000000)
 
 #ifndef __ASSEMBLY__
 
@@ -76,7 +77,7 @@ enum {
 		FW_FEATURE_DRC_INFO | FW_FEATURE_BLOCK_REMOVE |
 		FW_FEATURE_PAPR_SCM | FW_FEATURE_ULTRAVISOR |
 		FW_FEATURE_RPT_INVALIDATE | FW_FEATURE_FORM2_AFFINITY |
-		FW_FEATURE_ENERGY_SCALE_INFO,
+		FW_FEATURE_ENERGY_SCALE_INFO | FW_FEATURE_WATCHDOG,
 	FW_FEATURE_PSERIES_ALWAYS = 0,
 	FW_FEATURE_POWERNV_POSSIBLE = FW_FEATURE_OPAL | FW_FEATURE_ULTRAVISOR,
 	FW_FEATURE_POWERNV_ALWAYS = 0,
diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c
index 09c119b2f623..080108d129ed 100644
--- a/arch/powerpc/platforms/pseries/firmware.c
+++ b/arch/powerpc/platforms/pseries/firmware.c
@@ -67,6 +67,7 @@ hypertas_fw_features_table[] = {
 	{FW_FEATURE_PAPR_SCM,		"hcall-scm"},
 	{FW_FEATURE_RPT_INVALIDATE,	"hcall-rpt-invalidate"},
 	{FW_FEATURE_ENERGY_SCALE_INFO,	"hcall-energy-scale-info"},
+	{FW_FEATURE_WATCHDOG,		"hcall-watchdog"},
 };
 
 /* Build up the firmware features bitmask using the contents of
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 3/4] powerpc/pseries: register pseries-wdt device with platform bus
  2022-06-02 17:53 ` Scott Cheloha
@ 2022-06-02 17:53   ` Scott Cheloha
  -1 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux, tzungbi, brking, nathanl, aik, npiggin, vaishnavi, wvoigt,
	linuxppc-dev, Scott Cheloha

PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
guest control of one or more virtual watchdog timers.

These timers do not conform to PowerPC device conventions.  They are
not affixed to any extant bus, nor do they have full representation in
the device tree.

As a workaround we represent them as platform devices.

This patch registers a single platform device, "pseries-wdt", with the
platform bus if the FW_FEATURE_WATCHDOG flag is set.

A driver for this device, "pseries-wdt", will be introduced in a
subsequent patch.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/setup.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index afb074269b42..233c64f59815 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -14,6 +14,7 @@
 
 #include <linux/cpu.h>
 #include <linux/errno.h>
+#include <linux/platform_device.h>
 #include <linux/sched.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
@@ -169,6 +170,22 @@ static void __init fwnmi_init(void)
 #endif
 }
 
+/*
+ * Affix a device for the first timer to the platform bus if
+ * we have firmware support for the H_WATCHDOG hypercall.
+ */
+static struct platform_device *pseries_wdt_pdev;
+
+static __init int pseries_wdt_init(void)
+{
+	if (!firmware_has_feature(FW_FEATURE_WATCHDOG))
+		return 0;
+	pseries_wdt_pdev = platform_device_register_simple("pseries-wdt",
+							   0, NULL, 0);
+	return 0;
+}
+machine_subsys_initcall(pseries, pseries_wdt_init);
+
 static void pseries_8259_cascade(struct irq_desc *desc)
 {
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 3/4] powerpc/pseries: register pseries-wdt device with platform bus
@ 2022-06-02 17:53   ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: nathanl, wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi,
	brking, linuxppc-dev, linux

PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
guest control of one or more virtual watchdog timers.

These timers do not conform to PowerPC device conventions.  They are
not affixed to any extant bus, nor do they have full representation in
the device tree.

As a workaround we represent them as platform devices.

This patch registers a single platform device, "pseries-wdt", with the
platform bus if the FW_FEATURE_WATCHDOG flag is set.

A driver for this device, "pseries-wdt", will be introduced in a
subsequent patch.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/setup.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index afb074269b42..233c64f59815 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -14,6 +14,7 @@
 
 #include <linux/cpu.h>
 #include <linux/errno.h>
+#include <linux/platform_device.h>
 #include <linux/sched.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
@@ -169,6 +170,22 @@ static void __init fwnmi_init(void)
 #endif
 }
 
+/*
+ * Affix a device for the first timer to the platform bus if
+ * we have firmware support for the H_WATCHDOG hypercall.
+ */
+static struct platform_device *pseries_wdt_pdev;
+
+static __init int pseries_wdt_init(void)
+{
+	if (!firmware_has_feature(FW_FEATURE_WATCHDOG))
+		return 0;
+	pseries_wdt_pdev = platform_device_register_simple("pseries-wdt",
+							   0, NULL, 0);
+	return 0;
+}
+machine_subsys_initcall(pseries, pseries_wdt_init);
+
 static void pseries_8259_cascade(struct irq_desc *desc)
 {
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-02 17:53 ` Scott Cheloha
@ 2022-06-02 17:53   ` Scott Cheloha
  -1 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: linux, tzungbi, brking, nathanl, aik, npiggin, vaishnavi, wvoigt,
	linuxppc-dev, Scott Cheloha

PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
guest control of one or more virtual watchdog timers.  The timers have
millisecond granularity.  The guest is terminated when a timer
expires.

This patch adds a watchdog driver for these timers, "pseries-wdt".

pseries_wdt_probe() currently assumes the existence of only one
platform device and always assigns it watchdogNumber 1.  If we ever
expose more than one timer to userspace we will need to devise a way
to assign a distinct watchdogNumber to each platform device at device
registration time.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
 .../watchdog/watchdog-parameters.rst          |  12 +
 drivers/watchdog/Kconfig                      |   8 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/pseries-wdt.c                | 264 ++++++++++++++++++
 4 files changed, 285 insertions(+)
 create mode 100644 drivers/watchdog/pseries-wdt.c

diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
index 223c99361a30..29153eed6689 100644
--- a/Documentation/watchdog/watchdog-parameters.rst
+++ b/Documentation/watchdog/watchdog-parameters.rst
@@ -425,6 +425,18 @@ pnx833x_wdt:
 
 -------------------------------------------------
 
+pseries-wdt:
+    action:
+	Action taken when watchdog expires: 0 (power off), 1 (restart),
+	2 (dump and restart). (default=1)
+    timeout:
+	Initial watchdog timeout in seconds. (default=60)
+    nowayout:
+	Watchdog cannot be stopped once started.
+	(default=kernel config parameter)
+
+-------------------------------------------------
+
 rc32434_wdt:
     timeout:
 	Watchdog timeout value, in seconds (default=20)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c4e82a8d863f..06b412603f3e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1932,6 +1932,14 @@ config MEN_A21_WDT
 
 # PPC64 Architecture
 
+config PSERIES_WDT
+	tristate "POWER Architecture Platform Watchdog Timer"
+	depends on PPC_PSERIES
+	select WATCHDOG_CORE
+	help
+	  Driver for virtual watchdog timers provided by PAPR
+	  hypervisors (e.g. PowerVM, KVM).
+
 config WATCHDOG_RTAS
 	tristate "RTAS watchdog"
 	depends on PPC_RTAS
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f7da867e8782..f35660409f17 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
 obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
 
 # PPC64 Architecture
+obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
 obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
 
 # S390 Architecture
diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
new file mode 100644
index 000000000000..cfe53587457d
--- /dev/null
+++ b/drivers/watchdog/pseries-wdt.c
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 International Business Machines, Inc.
+ */
+
+#include <linux/bitops.h>
+#include <linux/kernel.h>
+#include <linux/limits.h>
+#include <linux/math.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#define DRV_NAME "pseries-wdt"
+
+/*
+ * The PAPR's MSB->LSB bit ordering is 0->63.  These macros simplify
+ * defining bitfields as described in the PAPR without needing to
+ * transpose values to the more C-like 63->0 ordering.
+ */
+#define SETFIELD(_v, _b, _e)	\
+	(((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
+#define GETFIELD(_v, _b, _e)	\
+	(((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
+
+/*
+ * The H_WATCHDOG hypercall first appears in PAPR v2.12 and is
+ * described fully in sections 14.5 and 14.15.6.
+ *
+ *
+ * H_WATCHDOG Input
+ *
+ * R4: "flags":
+ *
+ *         Bits 48-55: "operation"
+ *
+ *             0x01  Start Watchdog
+ *             0x02  Stop Watchdog
+ *             0x03  Query Watchdog Capabilities
+ */
+#define PSERIES_WDTF_OP(op)		SETFIELD((op), 48, 55)
+#define PSERIES_WDTF_OP_START		PSERIES_WDTF_OP(0x1)
+#define PSERIES_WDTF_OP_STOP		PSERIES_WDTF_OP(0x2)
+#define PSERIES_WDTF_OP_QUERY		PSERIES_WDTF_OP(0x3)
+
+/*
+ *         Bits 56-63: "timeoutAction" (for "Start Watchdog" only)
+ *
+ *             0x01  Hard poweroff
+ *             0x02  Hard restart
+ *             0x03  Dump restart
+ */
+#define PSERIES_WDTF_ACTION(ac)			SETFIELD(ac, 56, 63)
+#define PSERIES_WDTF_ACTION_HARD_POWEROFF	PSERIES_WDTF_ACTION(0x1)
+#define PSERIES_WDTF_ACTION_HARD_RESTART	PSERIES_WDTF_ACTION(0x2)
+#define PSERIES_WDTF_ACTION_DUMP_RESTART	PSERIES_WDTF_ACTION(0x3)
+
+/*
+ * H_WATCHDOG Output
+ *
+ * R3: Return code
+ *
+ *     H_SUCCESS    The operation completed.
+ *
+ *     H_BUSY	    The hypervisor is too busy; retry the operation.
+ *
+ *     H_PARAMETER  The given "flags" are somehow invalid.  Either the
+ *                  "operation" or "timeoutAction" is invalid, or a
+ *                  reserved bit is set.
+ *
+ *     H_P2         The given "watchdogNumber" is zero or exceeds the
+ *                  supported maximum value.
+ *
+ *     H_P3         The given "timeoutInMs" is below the supported
+ *                  minimum value.
+ *
+ *     H_NOOP       The given "watchdogNumber" is already stopped.
+ *
+ *     H_HARDWARE   The operation failed for ineffable reasons.
+ *
+ *     H_FUNCTION   The H_WATCHDOG hypercall is not supported by this
+ *                  hypervisor.
+ *
+ * R4:
+ *
+ * - For the "Query Watchdog Capabilities" operation, a 64-bit
+ *   value structured as follows:
+ *
+ *       Bits  0-15: The minimum supported timeout in milliseconds.
+ *       Bits 16-31: The number of watchdogs supported.
+ *       Bits 32-63: Reserved.
+ */
+#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	GETFIELD((cap), 0, 15)
+#define PSERIES_WDTQ_MAX_NUMBER(cap)	GETFIELD((cap), 16, 31)
+
+static const unsigned long pseries_wdt_action[] = {
+	[0] = PSERIES_WDTF_ACTION_HARD_POWEROFF,
+	[1] = PSERIES_WDTF_ACTION_HARD_RESTART,
+	[2] = PSERIES_WDTF_ACTION_DUMP_RESTART,
+};
+
+#define WATCHDOG_ACTION 1
+static unsigned int action = WATCHDOG_ACTION;
+module_param(action, uint, 0444);
+MODULE_PARM_DESC(action, "Action taken when watchdog expires (default="
+		 __MODULE_STRING(WATCHDOG_ACTION) ")");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0444);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+#define WATCHDOG_TIMEOUT 60
+static unsigned int timeout = WATCHDOG_TIMEOUT;
+module_param(timeout, uint, 0444);
+MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default="
+		 __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
+
+struct pseries_wdt {
+	struct watchdog_device wd;
+	unsigned long action;
+	unsigned long num;		/* Watchdog numbers are 1-based */
+};
+
+static int pseries_wdt_start(struct watchdog_device *wdd)
+{
+	struct device *dev = wdd->parent;
+	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
+	unsigned long flags, msecs;
+	long rc;
+
+	flags = pw->action | PSERIES_WDTF_OP_START;
+	msecs = wdd->timeout * 1000UL;
+	rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
+	if (rc != H_SUCCESS) {
+		dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
+			 rc, pw->num);
+		return -EIO;
+	}
+	return 0;
+}
+
+static int pseries_wdt_stop(struct watchdog_device *wdd)
+{
+	struct device *dev = wdd->parent;
+	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
+	long rc;
+
+	rc = plpar_hcall_norets(H_WATCHDOG, PSERIES_WDTF_OP_STOP, pw->num);
+	if (rc != H_SUCCESS && rc != H_NOOP) {
+		dev_crit(dev, "H_WATCHDOG: %ld: failed to stop timer %lu",
+			 rc, pw->num);
+		return -EIO;
+	}
+	return 0;
+}
+
+static struct watchdog_info pseries_wdt_info = {
+	.identity = DRV_NAME,
+	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT
+	    | WDIOF_PRETIMEOUT,
+};
+
+static const struct watchdog_ops pseries_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = pseries_wdt_start,
+	.stop = pseries_wdt_stop,
+};
+
+static int pseries_wdt_probe(struct platform_device *pdev)
+{
+	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
+	unsigned long cap;
+	long rc;
+	struct pseries_wdt *pw;
+	int err;
+
+	rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
+	if (rc == H_FUNCTION)
+		return -ENODEV;
+	if (rc != H_SUCCESS)
+		return -EIO;
+	cap = ret[0];
+
+	pw = devm_kzalloc(&pdev->dev, sizeof(*pw), GFP_KERNEL);
+	if (!pw)
+		return -ENOMEM;
+
+	/*
+	 * Assume watchdogNumber 1 for now.  If we ever support
+	 * multiple timers we will need to devise a way to choose a
+	 * distinct watchdogNumber for each platform device at device
+	 * registration time.
+	 */
+	pw->num = 1;
+
+	if (action >= ARRAY_SIZE(pseries_wdt_action))
+		return -EINVAL;
+	pw->action = pseries_wdt_action[action];
+
+	pw->wd.parent = &pdev->dev;
+	pw->wd.info = &pseries_wdt_info;
+	pw->wd.ops = &pseries_wdt_ops;
+	pw->wd.min_timeout = DIV_ROUND_UP(PSERIES_WDTQ_MIN_TIMEOUT(cap), 1000);
+	pw->wd.max_timeout = UINT_MAX / 1000;
+	pw->wd.timeout = timeout;
+	if (watchdog_init_timeout(&pw->wd, 0, NULL))
+		return -EINVAL;
+	watchdog_set_nowayout(&pw->wd, nowayout);
+	watchdog_stop_on_reboot(&pw->wd);
+	watchdog_stop_on_unregister(&pw->wd);
+	watchdog_set_drvdata(&pw->wd, pw);
+
+	err = devm_watchdog_register_device(&pdev->dev, &pw->wd);
+	if (err)
+		return err;
+
+	platform_set_drvdata(pdev, &pw->wd);
+
+	return 0;
+}
+
+static int pseries_wdt_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct watchdog_device *wd = platform_get_drvdata(pdev);
+
+	if (watchdog_active(wd))
+		return pseries_wdt_stop(wd);
+	return 0;
+}
+
+static int pseries_wdt_resume(struct platform_device *pdev)
+{
+	struct watchdog_device *wd = platform_get_drvdata(pdev);
+
+	if (watchdog_active(wd))
+		return pseries_wdt_start(wd);
+	return 0;
+}
+
+static const struct platform_device_id pseries_wdt_id[] = {
+	{ .name = "pseries-wdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, pseries_wdt_id);
+
+static struct platform_driver pseries_wdt_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+	},
+	.id_table = pseries_wdt_id,
+	.probe = pseries_wdt_probe,
+	.resume = pseries_wdt_resume,
+	.suspend = pseries_wdt_suspend,
+};
+module_platform_driver(pseries_wdt_driver);
+
+MODULE_AUTHOR("Alexey Kardashevskiy <aik@ozlabs.ru>");
+MODULE_AUTHOR("Scott Cheloha <cheloha@linux.ibm.com>");
+MODULE_DESCRIPTION("POWER Architecture Platform Watchdog Driver");
+MODULE_LICENSE("GPL");
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
@ 2022-06-02 17:53   ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-06-02 17:53 UTC (permalink / raw)
  To: linux-watchdog
  Cc: nathanl, wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi,
	brking, linuxppc-dev, linux

PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
guest control of one or more virtual watchdog timers.  The timers have
millisecond granularity.  The guest is terminated when a timer
expires.

This patch adds a watchdog driver for these timers, "pseries-wdt".

pseries_wdt_probe() currently assumes the existence of only one
platform device and always assigns it watchdogNumber 1.  If we ever
expose more than one timer to userspace we will need to devise a way
to assign a distinct watchdogNumber to each platform device at device
registration time.

Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
---
 .../watchdog/watchdog-parameters.rst          |  12 +
 drivers/watchdog/Kconfig                      |   8 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/pseries-wdt.c                | 264 ++++++++++++++++++
 4 files changed, 285 insertions(+)
 create mode 100644 drivers/watchdog/pseries-wdt.c

diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
index 223c99361a30..29153eed6689 100644
--- a/Documentation/watchdog/watchdog-parameters.rst
+++ b/Documentation/watchdog/watchdog-parameters.rst
@@ -425,6 +425,18 @@ pnx833x_wdt:
 
 -------------------------------------------------
 
+pseries-wdt:
+    action:
+	Action taken when watchdog expires: 0 (power off), 1 (restart),
+	2 (dump and restart). (default=1)
+    timeout:
+	Initial watchdog timeout in seconds. (default=60)
+    nowayout:
+	Watchdog cannot be stopped once started.
+	(default=kernel config parameter)
+
+-------------------------------------------------
+
 rc32434_wdt:
     timeout:
 	Watchdog timeout value, in seconds (default=20)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c4e82a8d863f..06b412603f3e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1932,6 +1932,14 @@ config MEN_A21_WDT
 
 # PPC64 Architecture
 
+config PSERIES_WDT
+	tristate "POWER Architecture Platform Watchdog Timer"
+	depends on PPC_PSERIES
+	select WATCHDOG_CORE
+	help
+	  Driver for virtual watchdog timers provided by PAPR
+	  hypervisors (e.g. PowerVM, KVM).
+
 config WATCHDOG_RTAS
 	tristate "RTAS watchdog"
 	depends on PPC_RTAS
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f7da867e8782..f35660409f17 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
 obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
 
 # PPC64 Architecture
+obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
 obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
 
 # S390 Architecture
diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
new file mode 100644
index 000000000000..cfe53587457d
--- /dev/null
+++ b/drivers/watchdog/pseries-wdt.c
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 International Business Machines, Inc.
+ */
+
+#include <linux/bitops.h>
+#include <linux/kernel.h>
+#include <linux/limits.h>
+#include <linux/math.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#define DRV_NAME "pseries-wdt"
+
+/*
+ * The PAPR's MSB->LSB bit ordering is 0->63.  These macros simplify
+ * defining bitfields as described in the PAPR without needing to
+ * transpose values to the more C-like 63->0 ordering.
+ */
+#define SETFIELD(_v, _b, _e)	\
+	(((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
+#define GETFIELD(_v, _b, _e)	\
+	(((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
+
+/*
+ * The H_WATCHDOG hypercall first appears in PAPR v2.12 and is
+ * described fully in sections 14.5 and 14.15.6.
+ *
+ *
+ * H_WATCHDOG Input
+ *
+ * R4: "flags":
+ *
+ *         Bits 48-55: "operation"
+ *
+ *             0x01  Start Watchdog
+ *             0x02  Stop Watchdog
+ *             0x03  Query Watchdog Capabilities
+ */
+#define PSERIES_WDTF_OP(op)		SETFIELD((op), 48, 55)
+#define PSERIES_WDTF_OP_START		PSERIES_WDTF_OP(0x1)
+#define PSERIES_WDTF_OP_STOP		PSERIES_WDTF_OP(0x2)
+#define PSERIES_WDTF_OP_QUERY		PSERIES_WDTF_OP(0x3)
+
+/*
+ *         Bits 56-63: "timeoutAction" (for "Start Watchdog" only)
+ *
+ *             0x01  Hard poweroff
+ *             0x02  Hard restart
+ *             0x03  Dump restart
+ */
+#define PSERIES_WDTF_ACTION(ac)			SETFIELD(ac, 56, 63)
+#define PSERIES_WDTF_ACTION_HARD_POWEROFF	PSERIES_WDTF_ACTION(0x1)
+#define PSERIES_WDTF_ACTION_HARD_RESTART	PSERIES_WDTF_ACTION(0x2)
+#define PSERIES_WDTF_ACTION_DUMP_RESTART	PSERIES_WDTF_ACTION(0x3)
+
+/*
+ * H_WATCHDOG Output
+ *
+ * R3: Return code
+ *
+ *     H_SUCCESS    The operation completed.
+ *
+ *     H_BUSY	    The hypervisor is too busy; retry the operation.
+ *
+ *     H_PARAMETER  The given "flags" are somehow invalid.  Either the
+ *                  "operation" or "timeoutAction" is invalid, or a
+ *                  reserved bit is set.
+ *
+ *     H_P2         The given "watchdogNumber" is zero or exceeds the
+ *                  supported maximum value.
+ *
+ *     H_P3         The given "timeoutInMs" is below the supported
+ *                  minimum value.
+ *
+ *     H_NOOP       The given "watchdogNumber" is already stopped.
+ *
+ *     H_HARDWARE   The operation failed for ineffable reasons.
+ *
+ *     H_FUNCTION   The H_WATCHDOG hypercall is not supported by this
+ *                  hypervisor.
+ *
+ * R4:
+ *
+ * - For the "Query Watchdog Capabilities" operation, a 64-bit
+ *   value structured as follows:
+ *
+ *       Bits  0-15: The minimum supported timeout in milliseconds.
+ *       Bits 16-31: The number of watchdogs supported.
+ *       Bits 32-63: Reserved.
+ */
+#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	GETFIELD((cap), 0, 15)
+#define PSERIES_WDTQ_MAX_NUMBER(cap)	GETFIELD((cap), 16, 31)
+
+static const unsigned long pseries_wdt_action[] = {
+	[0] = PSERIES_WDTF_ACTION_HARD_POWEROFF,
+	[1] = PSERIES_WDTF_ACTION_HARD_RESTART,
+	[2] = PSERIES_WDTF_ACTION_DUMP_RESTART,
+};
+
+#define WATCHDOG_ACTION 1
+static unsigned int action = WATCHDOG_ACTION;
+module_param(action, uint, 0444);
+MODULE_PARM_DESC(action, "Action taken when watchdog expires (default="
+		 __MODULE_STRING(WATCHDOG_ACTION) ")");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0444);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+#define WATCHDOG_TIMEOUT 60
+static unsigned int timeout = WATCHDOG_TIMEOUT;
+module_param(timeout, uint, 0444);
+MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default="
+		 __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
+
+struct pseries_wdt {
+	struct watchdog_device wd;
+	unsigned long action;
+	unsigned long num;		/* Watchdog numbers are 1-based */
+};
+
+static int pseries_wdt_start(struct watchdog_device *wdd)
+{
+	struct device *dev = wdd->parent;
+	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
+	unsigned long flags, msecs;
+	long rc;
+
+	flags = pw->action | PSERIES_WDTF_OP_START;
+	msecs = wdd->timeout * 1000UL;
+	rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
+	if (rc != H_SUCCESS) {
+		dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
+			 rc, pw->num);
+		return -EIO;
+	}
+	return 0;
+}
+
+static int pseries_wdt_stop(struct watchdog_device *wdd)
+{
+	struct device *dev = wdd->parent;
+	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
+	long rc;
+
+	rc = plpar_hcall_norets(H_WATCHDOG, PSERIES_WDTF_OP_STOP, pw->num);
+	if (rc != H_SUCCESS && rc != H_NOOP) {
+		dev_crit(dev, "H_WATCHDOG: %ld: failed to stop timer %lu",
+			 rc, pw->num);
+		return -EIO;
+	}
+	return 0;
+}
+
+static struct watchdog_info pseries_wdt_info = {
+	.identity = DRV_NAME,
+	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT
+	    | WDIOF_PRETIMEOUT,
+};
+
+static const struct watchdog_ops pseries_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = pseries_wdt_start,
+	.stop = pseries_wdt_stop,
+};
+
+static int pseries_wdt_probe(struct platform_device *pdev)
+{
+	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
+	unsigned long cap;
+	long rc;
+	struct pseries_wdt *pw;
+	int err;
+
+	rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
+	if (rc == H_FUNCTION)
+		return -ENODEV;
+	if (rc != H_SUCCESS)
+		return -EIO;
+	cap = ret[0];
+
+	pw = devm_kzalloc(&pdev->dev, sizeof(*pw), GFP_KERNEL);
+	if (!pw)
+		return -ENOMEM;
+
+	/*
+	 * Assume watchdogNumber 1 for now.  If we ever support
+	 * multiple timers we will need to devise a way to choose a
+	 * distinct watchdogNumber for each platform device at device
+	 * registration time.
+	 */
+	pw->num = 1;
+
+	if (action >= ARRAY_SIZE(pseries_wdt_action))
+		return -EINVAL;
+	pw->action = pseries_wdt_action[action];
+
+	pw->wd.parent = &pdev->dev;
+	pw->wd.info = &pseries_wdt_info;
+	pw->wd.ops = &pseries_wdt_ops;
+	pw->wd.min_timeout = DIV_ROUND_UP(PSERIES_WDTQ_MIN_TIMEOUT(cap), 1000);
+	pw->wd.max_timeout = UINT_MAX / 1000;
+	pw->wd.timeout = timeout;
+	if (watchdog_init_timeout(&pw->wd, 0, NULL))
+		return -EINVAL;
+	watchdog_set_nowayout(&pw->wd, nowayout);
+	watchdog_stop_on_reboot(&pw->wd);
+	watchdog_stop_on_unregister(&pw->wd);
+	watchdog_set_drvdata(&pw->wd, pw);
+
+	err = devm_watchdog_register_device(&pdev->dev, &pw->wd);
+	if (err)
+		return err;
+
+	platform_set_drvdata(pdev, &pw->wd);
+
+	return 0;
+}
+
+static int pseries_wdt_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct watchdog_device *wd = platform_get_drvdata(pdev);
+
+	if (watchdog_active(wd))
+		return pseries_wdt_stop(wd);
+	return 0;
+}
+
+static int pseries_wdt_resume(struct platform_device *pdev)
+{
+	struct watchdog_device *wd = platform_get_drvdata(pdev);
+
+	if (watchdog_active(wd))
+		return pseries_wdt_start(wd);
+	return 0;
+}
+
+static const struct platform_device_id pseries_wdt_id[] = {
+	{ .name = "pseries-wdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, pseries_wdt_id);
+
+static struct platform_driver pseries_wdt_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+	},
+	.id_table = pseries_wdt_id,
+	.probe = pseries_wdt_probe,
+	.resume = pseries_wdt_resume,
+	.suspend = pseries_wdt_suspend,
+};
+module_platform_driver(pseries_wdt_driver);
+
+MODULE_AUTHOR("Alexey Kardashevskiy <aik@ozlabs.ru>");
+MODULE_AUTHOR("Scott Cheloha <cheloha@linux.ibm.com>");
+MODULE_DESCRIPTION("POWER Architecture Platform Watchdog Driver");
+MODULE_LICENSE("GPL");
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 0/4] pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-02 17:53 ` Scott Cheloha
                   ` (4 preceding siblings ...)
  (?)
@ 2022-06-16  1:43 ` Daniel Henrique Barboza
  2022-06-16 16:44   ` Tyrel Datwyler
  2022-06-16 18:33   ` Daniel Henrique Barboza
  -1 siblings, 2 replies; 39+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-16  1:43 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: nathanl, wvoigt, aik, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux

Hi,

I tried this series out with mainline QEMU built with Alexey's patch [1]
and I wasn't able to get it to work. I'm using a simple QEMU command line
booting a fedora36 guest in a Power9 boston host:

sudo  ./qemu-system-ppc64 \
-M pseries,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off,ic-mode=dual \
-m 4G -accel kvm -cpu POWER9 -smp 1,maxcpus=1,threads=1,cores=1,sockets=1 \
-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x2 \
-drive file=/home/danielhb/fedora36.qcow2,if=none,id=drive-scsi0-0-0-0,format=qcow2,cache=none \
-device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=2 \
-device qemu-xhci,id=usb,bus=pci.0,addr=0x4 -nographic -display none


Guest is running v5.19-rc2 with this series applied. Kernel config consists of
'pseries_le_defconfig' plus the following 'watchdog' related changes:

[root@fedora ~]# cat linux/.config | grep PSERIES_WDT
CONFIG_PSERIES_WDT=y

[root@fedora ~]# cat linux/.config | grep -i watchdog
CONFIG_PPC_WATCHDOG=y
CONFIG_HAVE_NMI_WATCHDOG=y
CONFIG_WATCHDOG=y
CONFIG_WATCHDOG_CORE=y
CONFIG_WATCHDOG_NOWAYOUT=y
CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED=y
CONFIG_WATCHDOG_OPEN_TIMEOUT=0
# CONFIG_WATCHDOG_SYSFS is not set
# CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT is not set
# Watchdog Pretimeout Governors
# CONFIG_WATCHDOG_PRETIMEOUT_GOV is not set
# Watchdog Device Drivers
# CONFIG_SOFT_WATCHDOG is not set
# CONFIG_XILINX_WATCHDOG is not set
# CONFIG_ZIIRAVE_WATCHDOG is not set
# CONFIG_CADENCE_WATCHDOG is not set
# CONFIG_DW_WATCHDOG is not set
# CONFIG_MAX63XX_WATCHDOG is not set
CONFIG_WATCHDOG_RTAS=y
# PCI-based Watchdog Cards
# CONFIG_PCIPCWATCHDOG is not set
# USB-based Watchdog Cards
# CONFIG_USBPCWATCHDOG is not set
# CONFIG_WQ_WATCHDOG is not set
[root@fedora ~]#



Kernel command line:

[root@fedora ~]# cat /proc/cmdline
BOOT_IMAGE=(ieee1275/disk,msdos2)/vmlinuz-5.19.0-rc2-00054-g12ede8ffb103 \
root=/dev/mapper/fedora_fedora-root ro rd.lvm.lv=fedora_fedora/root \
pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2


With all that, executing

echo V > /dev/watchdog0

Does nothing. dmesg is clean and the guest doesn't reboot after the 60 sec
timeout.  I also tried with PSERIES_WDT being compiled as a module instead
of built-in. Same results.


What am I missing?


[1] https://patchwork.ozlabs.org/project/qemu-ppc/patch/20220608030153.1862335-1-aik@ozlabs.ru/



Thanks,


Daniel




On 6/2/22 14:53, Scott Cheloha wrote:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  This patch series
> adds support for this hypercall to powerpc/pseries kernels and
> introduces a new watchdog driver, "pseries-wdt", for the virtual
> timers exposed by the hypercall.
> 
> This series is preceded by the following:
> 
> RFC v1: https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-cheloha@linux.ibm.com/
> RFC v2: https://lore.kernel.org/linux-watchdog/20220509174357.5448-1-cheloha@linux.ibm.com/
> PATCH v1: https://lore.kernel.org/linux-watchdog/20220520183552.33426-1-cheloha@linux.ibm.com/
> 
> Changes of note from PATCH v1:
> 
> - Trim down the large comment documenting the H_WATCHDOG hypercall.
>    The comment is likely to rot, so remove anything we aren't using
>    and anything overly obvious.
> 
> - Remove any preprocessor definitions not actually used in the module
>    right now.  If we want to use other features offered by the hypercall
>    we can add them in later.  They're just clutter until then.
> 
> - Simplify the "action" module parameter.  The value is now an index
>    into an array of possible timeoutAction values.  This design removes
>    the need for the custom get/set methods used in PATCH v1.
> 
>    Now we merely need to check that the "action" value is a valid
>    index during pseries_wdt_probe().  Easy.
> 
> - Make the timeoutAction a member of pseries_wdt, "action".  This
>    eliminates the use of a global variable during pseries_wdt_start().
> 
> - Use watchdog_init_timeout() idiomatically.  Check its return value
>    and error out of pseries_wdt_probe() if it fails.
> 
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 0/4] pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-16  1:43 ` [PATCH v2 0/4] pseries-wdt: " Daniel Henrique Barboza
@ 2022-06-16 16:44   ` Tyrel Datwyler
  2022-06-16 18:16     ` Daniel Henrique Barboza
  2022-06-16 18:33   ` Daniel Henrique Barboza
  1 sibling, 1 reply; 39+ messages in thread
From: Tyrel Datwyler @ 2022-06-16 16:44 UTC (permalink / raw)
  To: Daniel Henrique Barboza, Scott Cheloha, linux-watchdog
  Cc: nathanl, wvoigt, aik, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux

On 6/15/22 18:43, Daniel Henrique Barboza wrote:
> Hi,
> 
> I tried this series out with mainline QEMU built with Alexey's patch [1]
> and I wasn't able to get it to work. I'm using a simple QEMU command line
> booting a fedora36 guest in a Power9 boston host:

I would assume the H_WATCHDOG hypercall is not implemented by the qemu pseries
machine type. It is a very new hypercall.

-Tyrel

> 
> sudo  ./qemu-system-ppc64 \
> -M
> pseries,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off,ic-mode=dual
> \
> -m 4G -accel kvm -cpu POWER9 -smp 1,maxcpus=1,threads=1,cores=1,sockets=1 \
> -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x2 \
> -drive
> file=/home/danielhb/fedora36.qcow2,if=none,id=drive-scsi0-0-0-0,format=qcow2,cache=none
> \
> -device
> scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=2
> \
> -device qemu-xhci,id=usb,bus=pci.0,addr=0x4 -nographic -display none
> 
> 
> Guest is running v5.19-rc2 with this series applied. Kernel config consists of
> 'pseries_le_defconfig' plus the following 'watchdog' related changes:
> 
> [root@fedora ~]# cat linux/.config | grep PSERIES_WDT
> CONFIG_PSERIES_WDT=y
> 
> [root@fedora ~]# cat linux/.config | grep -i watchdog
> CONFIG_PPC_WATCHDOG=y
> CONFIG_HAVE_NMI_WATCHDOG=y
> CONFIG_WATCHDOG=y
> CONFIG_WATCHDOG_CORE=y
> CONFIG_WATCHDOG_NOWAYOUT=y
> CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED=y
> CONFIG_WATCHDOG_OPEN_TIMEOUT=0
> # CONFIG_WATCHDOG_SYSFS is not set
> # CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT is not set
> # Watchdog Pretimeout Governors
> # CONFIG_WATCHDOG_PRETIMEOUT_GOV is not set
> # Watchdog Device Drivers
> # CONFIG_SOFT_WATCHDOG is not set
> # CONFIG_XILINX_WATCHDOG is not set
> # CONFIG_ZIIRAVE_WATCHDOG is not set
> # CONFIG_CADENCE_WATCHDOG is not set
> # CONFIG_DW_WATCHDOG is not set
> # CONFIG_MAX63XX_WATCHDOG is not set
> CONFIG_WATCHDOG_RTAS=y
> # PCI-based Watchdog Cards
> # CONFIG_PCIPCWATCHDOG is not set
> # USB-based Watchdog Cards
> # CONFIG_USBPCWATCHDOG is not set
> # CONFIG_WQ_WATCHDOG is not set
> [root@fedora ~]#
> 
> 
> 
> Kernel command line:
> 
> [root@fedora ~]# cat /proc/cmdline
> BOOT_IMAGE=(ieee1275/disk,msdos2)/vmlinuz-5.19.0-rc2-00054-g12ede8ffb103 \
> root=/dev/mapper/fedora_fedora-root ro rd.lvm.lv=fedora_fedora/root \
> pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2
> 
> 
> With all that, executing
> 
> echo V > /dev/watchdog0
> 
> Does nothing. dmesg is clean and the guest doesn't reboot after the 60 sec
> timeout.  I also tried with PSERIES_WDT being compiled as a module instead
> of built-in. Same results.
> 
> 
> What am I missing?
> 
> 
> [1]
> https://patchwork.ozlabs.org/project/qemu-ppc/patch/20220608030153.1862335-1-aik@ozlabs.ru/
> 
> 
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> 
> On 6/2/22 14:53, Scott Cheloha wrote:
>> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  This patch series
>> adds support for this hypercall to powerpc/pseries kernels and
>> introduces a new watchdog driver, "pseries-wdt", for the virtual
>> timers exposed by the hypercall.
>>
>> This series is preceded by the following:
>>
>> RFC v1:
>> https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-cheloha@linux.ibm.com/
>>
>> RFC v2:
>> https://lore.kernel.org/linux-watchdog/20220509174357.5448-1-cheloha@linux.ibm.com/
>>
>> PATCH v1:
>> https://lore.kernel.org/linux-watchdog/20220520183552.33426-1-cheloha@linux.ibm.com/
>>
>>
>> Changes of note from PATCH v1:
>>
>> - Trim down the large comment documenting the H_WATCHDOG hypercall.
>>    The comment is likely to rot, so remove anything we aren't using
>>    and anything overly obvious.
>>
>> - Remove any preprocessor definitions not actually used in the module
>>    right now.  If we want to use other features offered by the hypercall
>>    we can add them in later.  They're just clutter until then.
>>
>> - Simplify the "action" module parameter.  The value is now an index
>>    into an array of possible timeoutAction values.  This design removes
>>    the need for the custom get/set methods used in PATCH v1.
>>
>>    Now we merely need to check that the "action" value is a valid
>>    index during pseries_wdt_probe().  Easy.
>>
>> - Make the timeoutAction a member of pseries_wdt, "action".  This
>>    eliminates the use of a global variable during pseries_wdt_start().
>>
>> - Use watchdog_init_timeout() idiomatically.  Check its return value
>>    and error out of pseries_wdt_probe() if it fails.
>>
>>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 0/4] pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-16 16:44   ` Tyrel Datwyler
@ 2022-06-16 18:16     ` Daniel Henrique Barboza
  0 siblings, 0 replies; 39+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-16 18:16 UTC (permalink / raw)
  To: Tyrel Datwyler, Scott Cheloha, linux-watchdog
  Cc: nathanl, wvoigt, aik, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux



On 6/16/22 13:44, Tyrel Datwyler wrote:
> On 6/15/22 18:43, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> I tried this series out with mainline QEMU built with Alexey's patch [1]
>> and I wasn't able to get it to work. I'm using a simple QEMU command line
>> booting a fedora36 guest in a Power9 boston host:
> 
> I would assume the H_WATCHDOG hypercall is not implemented by the qemu pseries
> machine type. It is a very new hypercall.

Alexey implemented QEMU support for this hypercall here:

"[qemu] ppc/spapr: Implement H_WATCHDOG"
https://patchwork.ozlabs.org/project/qemu-ppc/patch/20220608030153.1862335-1-aik@ozlabs.ru/

It is under review in the QEMU mailing list. I tried it out with this series
and wasn't able to get it to work.


Thanks,

Daniel


> 
> -Tyrel
> 
>>
>> sudo  ./qemu-system-ppc64 \
>> -M
>> pseries,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off,ic-mode=dual
>> \
>> -m 4G -accel kvm -cpu POWER9 -smp 1,maxcpus=1,threads=1,cores=1,sockets=1 \
>> -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x2 \
>> -drive
>> file=/home/danielhb/fedora36.qcow2,if=none,id=drive-scsi0-0-0-0,format=qcow2,cache=none
>> \
>> -device
>> scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=2
>> \
>> -device qemu-xhci,id=usb,bus=pci.0,addr=0x4 -nographic -display none
>>
>>
>> Guest is running v5.19-rc2 with this series applied. Kernel config consists of
>> 'pseries_le_defconfig' plus the following 'watchdog' related changes:
>>
>> [root@fedora ~]# cat linux/.config | grep PSERIES_WDT
>> CONFIG_PSERIES_WDT=y
>>
>> [root@fedora ~]# cat linux/.config | grep -i watchdog
>> CONFIG_PPC_WATCHDOG=y
>> CONFIG_HAVE_NMI_WATCHDOG=y
>> CONFIG_WATCHDOG=y
>> CONFIG_WATCHDOG_CORE=y
>> CONFIG_WATCHDOG_NOWAYOUT=y
>> CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED=y
>> CONFIG_WATCHDOG_OPEN_TIMEOUT=0
>> # CONFIG_WATCHDOG_SYSFS is not set
>> # CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT is not set
>> # Watchdog Pretimeout Governors
>> # CONFIG_WATCHDOG_PRETIMEOUT_GOV is not set
>> # Watchdog Device Drivers
>> # CONFIG_SOFT_WATCHDOG is not set
>> # CONFIG_XILINX_WATCHDOG is not set
>> # CONFIG_ZIIRAVE_WATCHDOG is not set
>> # CONFIG_CADENCE_WATCHDOG is not set
>> # CONFIG_DW_WATCHDOG is not set
>> # CONFIG_MAX63XX_WATCHDOG is not set
>> CONFIG_WATCHDOG_RTAS=y
>> # PCI-based Watchdog Cards
>> # CONFIG_PCIPCWATCHDOG is not set
>> # USB-based Watchdog Cards
>> # CONFIG_USBPCWATCHDOG is not set
>> # CONFIG_WQ_WATCHDOG is not set
>> [root@fedora ~]#
>>
>>
>>
>> Kernel command line:
>>
>> [root@fedora ~]# cat /proc/cmdline
>> BOOT_IMAGE=(ieee1275/disk,msdos2)/vmlinuz-5.19.0-rc2-00054-g12ede8ffb103 \
>> root=/dev/mapper/fedora_fedora-root ro rd.lvm.lv=fedora_fedora/root \
>> pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2
>>
>>
>> With all that, executing
>>
>> echo V > /dev/watchdog0
>>
>> Does nothing. dmesg is clean and the guest doesn't reboot after the 60 sec
>> timeout.  I also tried with PSERIES_WDT being compiled as a module instead
>> of built-in. Same results.
>>
>>
>> What am I missing?
>>
>>
>> [1]
>> https://patchwork.ozlabs.org/project/qemu-ppc/patch/20220608030153.1862335-1-aik@ozlabs.ru/
>>
>>
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>
>>
>> On 6/2/22 14:53, Scott Cheloha wrote:
>>> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  This patch series
>>> adds support for this hypercall to powerpc/pseries kernels and
>>> introduces a new watchdog driver, "pseries-wdt", for the virtual
>>> timers exposed by the hypercall.
>>>
>>> This series is preceded by the following:
>>>
>>> RFC v1:
>>> https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-cheloha@linux.ibm.com/
>>>
>>> RFC v2:
>>> https://lore.kernel.org/linux-watchdog/20220509174357.5448-1-cheloha@linux.ibm.com/
>>>
>>> PATCH v1:
>>> https://lore.kernel.org/linux-watchdog/20220520183552.33426-1-cheloha@linux.ibm.com/
>>>
>>>
>>> Changes of note from PATCH v1:
>>>
>>> - Trim down the large comment documenting the H_WATCHDOG hypercall.
>>>     The comment is likely to rot, so remove anything we aren't using
>>>     and anything overly obvious.
>>>
>>> - Remove any preprocessor definitions not actually used in the module
>>>     right now.  If we want to use other features offered by the hypercall
>>>     we can add them in later.  They're just clutter until then.
>>>
>>> - Simplify the "action" module parameter.  The value is now an index
>>>     into an array of possible timeoutAction values.  This design removes
>>>     the need for the custom get/set methods used in PATCH v1.
>>>
>>>     Now we merely need to check that the "action" value is a valid
>>>     index during pseries_wdt_probe().  Easy.
>>>
>>> - Make the timeoutAction a member of pseries_wdt, "action".  This
>>>     eliminates the use of a global variable during pseries_wdt_start().
>>>
>>> - Use watchdog_init_timeout() idiomatically.  Check its return value
>>>     and error out of pseries_wdt_probe() if it fails.
>>>
>>>
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 0/4] pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-16  1:43 ` [PATCH v2 0/4] pseries-wdt: " Daniel Henrique Barboza
  2022-06-16 16:44   ` Tyrel Datwyler
@ 2022-06-16 18:33   ` Daniel Henrique Barboza
  1 sibling, 0 replies; 39+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-16 18:33 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: nathanl, wvoigt, aik, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux

Hi,

Update: checking out 'dmesg' more carefully I found out that the module probe
is failing with the following message:

[  186.298424][  T811] pseries-wdt: probe of pseries-wdt.0 failed with error -5

This fail is consistent. If I remove the module and modprobe it again the same
error happens.

The message is being throw by pseries_wdt_probe() (patch 4/4). Back in QEMU, in
Alexey's H_WATCHDOG implementation [1], I see that h_watchdog is returning H_PARAMETER
because the retrieved 'watchdogNumber' is zero.

Also, the pseries_wdt module still appears in 'lsmod' despite this probe error. Not sure
if this is a bug:

[root@fedora ~]# rmmod pseries_wdt
[root@fedora ~]# modprobe pseries_wdt
[ 1792.846769][  T865] pseries-wdt: probe of pseries-wdt.0 failed with error -5
[root@fedora ~]# lsmod | grep pseries
pseries_wdt           262144  0
[root@fedora ~]#


For reference this is all the output of 'lsmod' in the guest:

[root@fedora ~]# lsmod
Module                  Size  Used by
pseries_wdt           262144  0
nfnetlink             262144  1
evdev                 327680  1
input_leds            262144  0
led_class             262144  1 input_leds
fuse                  458752  1
xfs                  1835008  2
libcrc32c             262144  1 xfs
virtio_scsi           327680  2
virtio_pci            262144  0
virtio                327680  2 virtio_scsi,virtio_pci
vmx_crypto            262144  0
gf128mul              262144  1 vmx_crypto
crc32c_vpmsum         327680  1
virtio_ring           327680  3 virtio_scsi,virtio_pci,virtio
virtio_pci_legacy_dev   262144  1 virtio_pci
virtio_pci_modern_dev   262144  1 virtio_pci
autofs4               327680  2


Given that the error is being thrown by Alexey's QEMU code, I'll bring it up in the QEMU
mailing list in [1] and follow it up from there.


[1] https://patchwork.ozlabs.org/project/qemu-ppc/patch/20220608030153.1862335-1-aik@ozlabs.ru/


Thanks,


Daniel


On 6/15/22 22:43, Daniel Henrique Barboza wrote:
> Hi,
> 
> I tried this series out with mainline QEMU built with Alexey's patch [1]
> and I wasn't able to get it to work. I'm using a simple QEMU command line
> booting a fedora36 guest in a Power9 boston host:
> 
> sudo  ./qemu-system-ppc64 \
> -M pseries,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off,ic-mode=dual \
> -m 4G -accel kvm -cpu POWER9 -smp 1,maxcpus=1,threads=1,cores=1,sockets=1 \
> -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x2 \
> -drive file=/home/danielhb/fedora36.qcow2,if=none,id=drive-scsi0-0-0-0,format=qcow2,cache=none \
> -device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=2 \
> -device qemu-xhci,id=usb,bus=pci.0,addr=0x4 -nographic -display none
> 
> 
> Guest is running v5.19-rc2 with this series applied. Kernel config consists of
> 'pseries_le_defconfig' plus the following 'watchdog' related changes:
> 
> [root@fedora ~]# cat linux/.config | grep PSERIES_WDT
> CONFIG_PSERIES_WDT=y
> 
> [root@fedora ~]# cat linux/.config | grep -i watchdog
> CONFIG_PPC_WATCHDOG=y
> CONFIG_HAVE_NMI_WATCHDOG=y
> CONFIG_WATCHDOG=y
> CONFIG_WATCHDOG_CORE=y
> CONFIG_WATCHDOG_NOWAYOUT=y
> CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED=y
> CONFIG_WATCHDOG_OPEN_TIMEOUT=0
> # CONFIG_WATCHDOG_SYSFS is not set
> # CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT is not set
> # Watchdog Pretimeout Governors
> # CONFIG_WATCHDOG_PRETIMEOUT_GOV is not set
> # Watchdog Device Drivers
> # CONFIG_SOFT_WATCHDOG is not set
> # CONFIG_XILINX_WATCHDOG is not set
> # CONFIG_ZIIRAVE_WATCHDOG is not set
> # CONFIG_CADENCE_WATCHDOG is not set
> # CONFIG_DW_WATCHDOG is not set
> # CONFIG_MAX63XX_WATCHDOG is not set
> CONFIG_WATCHDOG_RTAS=y
> # PCI-based Watchdog Cards
> # CONFIG_PCIPCWATCHDOG is not set
> # USB-based Watchdog Cards
> # CONFIG_USBPCWATCHDOG is not set
> # CONFIG_WQ_WATCHDOG is not set
> [root@fedora ~]#
> 
> 
> 
> Kernel command line:
> 
> [root@fedora ~]# cat /proc/cmdline
> BOOT_IMAGE=(ieee1275/disk,msdos2)/vmlinuz-5.19.0-rc2-00054-g12ede8ffb103 \
> root=/dev/mapper/fedora_fedora-root ro rd.lvm.lv=fedora_fedora/root \
> pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2
> 
> 
> With all that, executing
> 
> echo V > /dev/watchdog0
> 
> Does nothing. dmesg is clean and the guest doesn't reboot after the 60 sec
> timeout.  I also tried with PSERIES_WDT being compiled as a module instead
> of built-in. Same results.
> 
> 
> What am I missing?
> 
> 
> [1] https://patchwork.ozlabs.org/project/qemu-ppc/patch/20220608030153.1862335-1-aik@ozlabs.ru/
> 
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> 
> On 6/2/22 14:53, Scott Cheloha wrote:
>> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  This patch series
>> adds support for this hypercall to powerpc/pseries kernels and
>> introduces a new watchdog driver, "pseries-wdt", for the virtual
>> timers exposed by the hypercall.
>>
>> This series is preceded by the following:
>>
>> RFC v1: https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-cheloha@linux.ibm.com/
>> RFC v2: https://lore.kernel.org/linux-watchdog/20220509174357.5448-1-cheloha@linux.ibm.com/
>> PATCH v1: https://lore.kernel.org/linux-watchdog/20220520183552.33426-1-cheloha@linux.ibm.com/
>>
>> Changes of note from PATCH v1:
>>
>> - Trim down the large comment documenting the H_WATCHDOG hypercall.
>>    The comment is likely to rot, so remove anything we aren't using
>>    and anything overly obvious.
>>
>> - Remove any preprocessor definitions not actually used in the module
>>    right now.  If we want to use other features offered by the hypercall
>>    we can add them in later.  They're just clutter until then.
>>
>> - Simplify the "action" module parameter.  The value is now an index
>>    into an array of possible timeoutAction values.  This design removes
>>    the need for the custom get/set methods used in PATCH v1.
>>
>>    Now we merely need to check that the "action" value is a valid
>>    index during pseries_wdt_probe().  Easy.
>>
>> - Make the timeoutAction a member of pseries_wdt, "action".  This
>>    eliminates the use of a global variable during pseries_wdt_start().
>>
>> - Use watchdog_init_timeout() idiomatically.  Check its return value
>>    and error out of pseries_wdt_probe() if it fails.
>>
>>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 0/4] pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-02 17:53 ` Scott Cheloha
                   ` (5 preceding siblings ...)
  (?)
@ 2022-06-17 12:54 ` Daniel Henrique Barboza
  -1 siblings, 0 replies; 39+ messages in thread
From: Daniel Henrique Barboza @ 2022-06-17 12:54 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: nathanl, wvoigt, aik, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux



On 6/2/22 14:53, Scott Cheloha wrote:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  This patch series
> adds support for this hypercall to powerpc/pseries kernels and
> introduces a new watchdog driver, "pseries-wdt", for the virtual
> timers exposed by the hypercall.
> 
> This series is preceded by the following:
> 
> RFC v1: https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-cheloha@linux.ibm.com/
> RFC v2: https://lore.kernel.org/linux-watchdog/20220509174357.5448-1-cheloha@linux.ibm.com/
> PATCH v1: https://lore.kernel.org/linux-watchdog/20220520183552.33426-1-cheloha@linux.ibm.com/

Tested successfully with mainline QEMU plus Alexey's new h_watchdog patches in
https://patchwork.ozlabs.org/project/qemu-ppc/list/?series=305226.


All patches of this series:

Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>


Thanks,


Daniel

> 
> Changes of note from PATCH v1:
> 
> - Trim down the large comment documenting the H_WATCHDOG hypercall.
>    The comment is likely to rot, so remove anything we aren't using
>    and anything overly obvious.
> 
> - Remove any preprocessor definitions not actually used in the module
>    right now.  If we want to use other features offered by the hypercall
>    we can add them in later.  They're just clutter until then.
> 
> - Simplify the "action" module parameter.  The value is now an index
>    into an array of possible timeoutAction values.  This design removes
>    the need for the custom get/set methods used in PATCH v1.
> 
>    Now we merely need to check that the "action" value is a valid
>    index during pseries_wdt_probe().  Easy.
> 
> - Make the timeoutAction a member of pseries_wdt, "action".  This
>    eliminates the use of a global variable during pseries_wdt_start().
> 
> - Use watchdog_init_timeout() idiomatically.  Check its return value
>    and error out of pseries_wdt_probe() if it fails.
> 
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-02 17:53   ` Scott Cheloha
@ 2022-06-20  6:09     ` Alexey Kardashevskiy
  -1 siblings, 0 replies; 39+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-20  6:09 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: linux, tzungbi, brking, nathanl, npiggin, vaishnavi, wvoigt,
	linuxppc-dev



On 6/3/22 03:53, Scott Cheloha wrote:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> guest control of one or more virtual watchdog timers.  The timers have
> millisecond granularity.  The guest is terminated when a timer
> expires.
> 
> This patch adds a watchdog driver for these timers, "pseries-wdt".
> 
> pseries_wdt_probe() currently assumes the existence of only one
> platform device and always assigns it watchdogNumber 1.  If we ever
> expose more than one timer to userspace we will need to devise a way
> to assign a distinct watchdogNumber to each platform device at device
> registration time.
> 
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>


Besides the patch ordering and 0444 vs. 0644 (which is up to the PPC 
maintainer to decide anyway :) ), looks good to me.


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



> ---
>   .../watchdog/watchdog-parameters.rst          |  12 +
>   drivers/watchdog/Kconfig                      |   8 +
>   drivers/watchdog/Makefile                     |   1 +
>   drivers/watchdog/pseries-wdt.c                | 264 ++++++++++++++++++
>   4 files changed, 285 insertions(+)
>   create mode 100644 drivers/watchdog/pseries-wdt.c
> 
> diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
> index 223c99361a30..29153eed6689 100644
> --- a/Documentation/watchdog/watchdog-parameters.rst
> +++ b/Documentation/watchdog/watchdog-parameters.rst
> @@ -425,6 +425,18 @@ pnx833x_wdt:
>   
>   -------------------------------------------------
>   
> +pseries-wdt:
> +    action:
> +	Action taken when watchdog expires: 0 (power off), 1 (restart),
> +	2 (dump and restart). (default=1)
> +    timeout:
> +	Initial watchdog timeout in seconds. (default=60)
> +    nowayout:
> +	Watchdog cannot be stopped once started.
> +	(default=kernel config parameter)
> +
> +-------------------------------------------------
> +
>   rc32434_wdt:
>       timeout:
>   	Watchdog timeout value, in seconds (default=20)
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c4e82a8d863f..06b412603f3e 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1932,6 +1932,14 @@ config MEN_A21_WDT
>   
>   # PPC64 Architecture
>   
> +config PSERIES_WDT
> +	tristate "POWER Architecture Platform Watchdog Timer"
> +	depends on PPC_PSERIES
> +	select WATCHDOG_CORE
> +	help
> +	  Driver for virtual watchdog timers provided by PAPR
> +	  hypervisors (e.g. PowerVM, KVM).
> +
>   config WATCHDOG_RTAS
>   	tristate "RTAS watchdog"
>   	depends on PPC_RTAS
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f7da867e8782..f35660409f17 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
>   obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
>   
>   # PPC64 Architecture
> +obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
>   obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
>   
>   # S390 Architecture
> diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
> new file mode 100644
> index 000000000000..cfe53587457d
> --- /dev/null
> +++ b/drivers/watchdog/pseries-wdt.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 International Business Machines, Inc.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/math.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define DRV_NAME "pseries-wdt"
> +
> +/*
> + * The PAPR's MSB->LSB bit ordering is 0->63.  These macros simplify
> + * defining bitfields as described in the PAPR without needing to
> + * transpose values to the more C-like 63->0 ordering.
> + */
> +#define SETFIELD(_v, _b, _e)	\
> +	(((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
> +#define GETFIELD(_v, _b, _e)	\
> +	(((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
> +
> +/*
> + * The H_WATCHDOG hypercall first appears in PAPR v2.12 and is
> + * described fully in sections 14.5 and 14.15.6.
> + *
> + *
> + * H_WATCHDOG Input
> + *
> + * R4: "flags":
> + *
> + *         Bits 48-55: "operation"
> + *
> + *             0x01  Start Watchdog
> + *             0x02  Stop Watchdog
> + *             0x03  Query Watchdog Capabilities
> + */
> +#define PSERIES_WDTF_OP(op)		SETFIELD((op), 48, 55)
> +#define PSERIES_WDTF_OP_START		PSERIES_WDTF_OP(0x1)
> +#define PSERIES_WDTF_OP_STOP		PSERIES_WDTF_OP(0x2)
> +#define PSERIES_WDTF_OP_QUERY		PSERIES_WDTF_OP(0x3)
> +
> +/*
> + *         Bits 56-63: "timeoutAction" (for "Start Watchdog" only)
> + *
> + *             0x01  Hard poweroff
> + *             0x02  Hard restart
> + *             0x03  Dump restart
> + */
> +#define PSERIES_WDTF_ACTION(ac)			SETFIELD(ac, 56, 63)
> +#define PSERIES_WDTF_ACTION_HARD_POWEROFF	PSERIES_WDTF_ACTION(0x1)
> +#define PSERIES_WDTF_ACTION_HARD_RESTART	PSERIES_WDTF_ACTION(0x2)
> +#define PSERIES_WDTF_ACTION_DUMP_RESTART	PSERIES_WDTF_ACTION(0x3)
> +
> +/*
> + * H_WATCHDOG Output
> + *
> + * R3: Return code
> + *
> + *     H_SUCCESS    The operation completed.
> + *
> + *     H_BUSY	    The hypervisor is too busy; retry the operation.
> + *
> + *     H_PARAMETER  The given "flags" are somehow invalid.  Either the
> + *                  "operation" or "timeoutAction" is invalid, or a
> + *                  reserved bit is set.
> + *
> + *     H_P2         The given "watchdogNumber" is zero or exceeds the
> + *                  supported maximum value.
> + *
> + *     H_P3         The given "timeoutInMs" is below the supported
> + *                  minimum value.
> + *
> + *     H_NOOP       The given "watchdogNumber" is already stopped.
> + *
> + *     H_HARDWARE   The operation failed for ineffable reasons.
> + *
> + *     H_FUNCTION   The H_WATCHDOG hypercall is not supported by this
> + *                  hypervisor.
> + *
> + * R4:
> + *
> + * - For the "Query Watchdog Capabilities" operation, a 64-bit
> + *   value structured as follows:
> + *
> + *       Bits  0-15: The minimum supported timeout in milliseconds.
> + *       Bits 16-31: The number of watchdogs supported.
> + *       Bits 32-63: Reserved.
> + */
> +#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	GETFIELD((cap), 0, 15)
> +#define PSERIES_WDTQ_MAX_NUMBER(cap)	GETFIELD((cap), 16, 31)
> +
> +static const unsigned long pseries_wdt_action[] = {
> +	[0] = PSERIES_WDTF_ACTION_HARD_POWEROFF,
> +	[1] = PSERIES_WDTF_ACTION_HARD_RESTART,
> +	[2] = PSERIES_WDTF_ACTION_DUMP_RESTART,
> +};
> +
> +#define WATCHDOG_ACTION 1
> +static unsigned int action = WATCHDOG_ACTION;
> +module_param(action, uint, 0444);
> +MODULE_PARM_DESC(action, "Action taken when watchdog expires (default="
> +		 __MODULE_STRING(WATCHDOG_ACTION) ")");
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0444);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +#define WATCHDOG_TIMEOUT 60
> +static unsigned int timeout = WATCHDOG_TIMEOUT;
> +module_param(timeout, uint, 0444);
> +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default="
> +		 __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
> +
> +struct pseries_wdt {
> +	struct watchdog_device wd;
> +	unsigned long action;
> +	unsigned long num;		/* Watchdog numbers are 1-based */
> +};
> +
> +static int pseries_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct device *dev = wdd->parent;
> +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> +	unsigned long flags, msecs;
> +	long rc;
> +
> +	flags = pw->action | PSERIES_WDTF_OP_START;
> +	msecs = wdd->timeout * 1000UL;
> +	rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
> +	if (rc != H_SUCCESS) {
> +		dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
> +			 rc, pw->num);
> +		return -EIO;
> +	}
> +	return 0;
> +}
> +
> +static int pseries_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct device *dev = wdd->parent;
> +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> +	long rc;
> +
> +	rc = plpar_hcall_norets(H_WATCHDOG, PSERIES_WDTF_OP_STOP, pw->num);
> +	if (rc != H_SUCCESS && rc != H_NOOP) {
> +		dev_crit(dev, "H_WATCHDOG: %ld: failed to stop timer %lu",
> +			 rc, pw->num);
> +		return -EIO;
> +	}
> +	return 0;
> +}
> +
> +static struct watchdog_info pseries_wdt_info = {
> +	.identity = DRV_NAME,
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT
> +	    | WDIOF_PRETIMEOUT,
> +};
> +
> +static const struct watchdog_ops pseries_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pseries_wdt_start,
> +	.stop = pseries_wdt_stop,
> +};
> +
> +static int pseries_wdt_probe(struct platform_device *pdev)
> +{
> +	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
> +	unsigned long cap;
> +	long rc;
> +	struct pseries_wdt *pw;
> +	int err;
> +
> +	rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
> +	if (rc == H_FUNCTION)
> +		return -ENODEV;
> +	if (rc != H_SUCCESS)
> +		return -EIO;
> +	cap = ret[0];
> +
> +	pw = devm_kzalloc(&pdev->dev, sizeof(*pw), GFP_KERNEL);
> +	if (!pw)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Assume watchdogNumber 1 for now.  If we ever support
> +	 * multiple timers we will need to devise a way to choose a
> +	 * distinct watchdogNumber for each platform device at device
> +	 * registration time.
> +	 */
> +	pw->num = 1;
> +
> +	if (action >= ARRAY_SIZE(pseries_wdt_action))
> +		return -EINVAL;
> +	pw->action = pseries_wdt_action[action];
> +
> +	pw->wd.parent = &pdev->dev;
> +	pw->wd.info = &pseries_wdt_info;
> +	pw->wd.ops = &pseries_wdt_ops;
> +	pw->wd.min_timeout = DIV_ROUND_UP(PSERIES_WDTQ_MIN_TIMEOUT(cap), 1000);
> +	pw->wd.max_timeout = UINT_MAX / 1000;
> +	pw->wd.timeout = timeout;
> +	if (watchdog_init_timeout(&pw->wd, 0, NULL))
> +		return -EINVAL;
> +	watchdog_set_nowayout(&pw->wd, nowayout);
> +	watchdog_stop_on_reboot(&pw->wd);
> +	watchdog_stop_on_unregister(&pw->wd);
> +	watchdog_set_drvdata(&pw->wd, pw);
> +
> +	err = devm_watchdog_register_device(&pdev->dev, &pw->wd);
> +	if (err)
> +		return err;
> +
> +	platform_set_drvdata(pdev, &pw->wd);
> +
> +	return 0;
> +}
> +
> +static int pseries_wdt_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> +
> +	if (watchdog_active(wd))
> +		return pseries_wdt_stop(wd);
> +	return 0;
> +}
> +
> +static int pseries_wdt_resume(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> +
> +	if (watchdog_active(wd))
> +		return pseries_wdt_start(wd);
> +	return 0;
> +}
> +
> +static const struct platform_device_id pseries_wdt_id[] = {
> +	{ .name = "pseries-wdt" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(platform, pseries_wdt_id);
> +
> +static struct platform_driver pseries_wdt_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +	.id_table = pseries_wdt_id,
> +	.probe = pseries_wdt_probe,
> +	.resume = pseries_wdt_resume,
> +	.suspend = pseries_wdt_suspend,
> +};
> +module_platform_driver(pseries_wdt_driver);
> +
> +MODULE_AUTHOR("Alexey Kardashevskiy <aik@ozlabs.ru>");
> +MODULE_AUTHOR("Scott Cheloha <cheloha@linux.ibm.com>");
> +MODULE_DESCRIPTION("POWER Architecture Platform Watchdog Driver");
> +MODULE_LICENSE("GPL");

-- 
Alexey

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
@ 2022-06-20  6:09     ` Alexey Kardashevskiy
  0 siblings, 0 replies; 39+ messages in thread
From: Alexey Kardashevskiy @ 2022-06-20  6:09 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: nathanl, wvoigt, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux



On 6/3/22 03:53, Scott Cheloha wrote:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> guest control of one or more virtual watchdog timers.  The timers have
> millisecond granularity.  The guest is terminated when a timer
> expires.
> 
> This patch adds a watchdog driver for these timers, "pseries-wdt".
> 
> pseries_wdt_probe() currently assumes the existence of only one
> platform device and always assigns it watchdogNumber 1.  If we ever
> expose more than one timer to userspace we will need to devise a way
> to assign a distinct watchdogNumber to each platform device at device
> registration time.
> 
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>


Besides the patch ordering and 0444 vs. 0644 (which is up to the PPC 
maintainer to decide anyway :) ), looks good to me.


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



> ---
>   .../watchdog/watchdog-parameters.rst          |  12 +
>   drivers/watchdog/Kconfig                      |   8 +
>   drivers/watchdog/Makefile                     |   1 +
>   drivers/watchdog/pseries-wdt.c                | 264 ++++++++++++++++++
>   4 files changed, 285 insertions(+)
>   create mode 100644 drivers/watchdog/pseries-wdt.c
> 
> diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
> index 223c99361a30..29153eed6689 100644
> --- a/Documentation/watchdog/watchdog-parameters.rst
> +++ b/Documentation/watchdog/watchdog-parameters.rst
> @@ -425,6 +425,18 @@ pnx833x_wdt:
>   
>   -------------------------------------------------
>   
> +pseries-wdt:
> +    action:
> +	Action taken when watchdog expires: 0 (power off), 1 (restart),
> +	2 (dump and restart). (default=1)
> +    timeout:
> +	Initial watchdog timeout in seconds. (default=60)
> +    nowayout:
> +	Watchdog cannot be stopped once started.
> +	(default=kernel config parameter)
> +
> +-------------------------------------------------
> +
>   rc32434_wdt:
>       timeout:
>   	Watchdog timeout value, in seconds (default=20)
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c4e82a8d863f..06b412603f3e 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1932,6 +1932,14 @@ config MEN_A21_WDT
>   
>   # PPC64 Architecture
>   
> +config PSERIES_WDT
> +	tristate "POWER Architecture Platform Watchdog Timer"
> +	depends on PPC_PSERIES
> +	select WATCHDOG_CORE
> +	help
> +	  Driver for virtual watchdog timers provided by PAPR
> +	  hypervisors (e.g. PowerVM, KVM).
> +
>   config WATCHDOG_RTAS
>   	tristate "RTAS watchdog"
>   	depends on PPC_RTAS
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f7da867e8782..f35660409f17 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
>   obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
>   
>   # PPC64 Architecture
> +obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
>   obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
>   
>   # S390 Architecture
> diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
> new file mode 100644
> index 000000000000..cfe53587457d
> --- /dev/null
> +++ b/drivers/watchdog/pseries-wdt.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 International Business Machines, Inc.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/math.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define DRV_NAME "pseries-wdt"
> +
> +/*
> + * The PAPR's MSB->LSB bit ordering is 0->63.  These macros simplify
> + * defining bitfields as described in the PAPR without needing to
> + * transpose values to the more C-like 63->0 ordering.
> + */
> +#define SETFIELD(_v, _b, _e)	\
> +	(((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
> +#define GETFIELD(_v, _b, _e)	\
> +	(((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
> +
> +/*
> + * The H_WATCHDOG hypercall first appears in PAPR v2.12 and is
> + * described fully in sections 14.5 and 14.15.6.
> + *
> + *
> + * H_WATCHDOG Input
> + *
> + * R4: "flags":
> + *
> + *         Bits 48-55: "operation"
> + *
> + *             0x01  Start Watchdog
> + *             0x02  Stop Watchdog
> + *             0x03  Query Watchdog Capabilities
> + */
> +#define PSERIES_WDTF_OP(op)		SETFIELD((op), 48, 55)
> +#define PSERIES_WDTF_OP_START		PSERIES_WDTF_OP(0x1)
> +#define PSERIES_WDTF_OP_STOP		PSERIES_WDTF_OP(0x2)
> +#define PSERIES_WDTF_OP_QUERY		PSERIES_WDTF_OP(0x3)
> +
> +/*
> + *         Bits 56-63: "timeoutAction" (for "Start Watchdog" only)
> + *
> + *             0x01  Hard poweroff
> + *             0x02  Hard restart
> + *             0x03  Dump restart
> + */
> +#define PSERIES_WDTF_ACTION(ac)			SETFIELD(ac, 56, 63)
> +#define PSERIES_WDTF_ACTION_HARD_POWEROFF	PSERIES_WDTF_ACTION(0x1)
> +#define PSERIES_WDTF_ACTION_HARD_RESTART	PSERIES_WDTF_ACTION(0x2)
> +#define PSERIES_WDTF_ACTION_DUMP_RESTART	PSERIES_WDTF_ACTION(0x3)
> +
> +/*
> + * H_WATCHDOG Output
> + *
> + * R3: Return code
> + *
> + *     H_SUCCESS    The operation completed.
> + *
> + *     H_BUSY	    The hypervisor is too busy; retry the operation.
> + *
> + *     H_PARAMETER  The given "flags" are somehow invalid.  Either the
> + *                  "operation" or "timeoutAction" is invalid, or a
> + *                  reserved bit is set.
> + *
> + *     H_P2         The given "watchdogNumber" is zero or exceeds the
> + *                  supported maximum value.
> + *
> + *     H_P3         The given "timeoutInMs" is below the supported
> + *                  minimum value.
> + *
> + *     H_NOOP       The given "watchdogNumber" is already stopped.
> + *
> + *     H_HARDWARE   The operation failed for ineffable reasons.
> + *
> + *     H_FUNCTION   The H_WATCHDOG hypercall is not supported by this
> + *                  hypervisor.
> + *
> + * R4:
> + *
> + * - For the "Query Watchdog Capabilities" operation, a 64-bit
> + *   value structured as follows:
> + *
> + *       Bits  0-15: The minimum supported timeout in milliseconds.
> + *       Bits 16-31: The number of watchdogs supported.
> + *       Bits 32-63: Reserved.
> + */
> +#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	GETFIELD((cap), 0, 15)
> +#define PSERIES_WDTQ_MAX_NUMBER(cap)	GETFIELD((cap), 16, 31)
> +
> +static const unsigned long pseries_wdt_action[] = {
> +	[0] = PSERIES_WDTF_ACTION_HARD_POWEROFF,
> +	[1] = PSERIES_WDTF_ACTION_HARD_RESTART,
> +	[2] = PSERIES_WDTF_ACTION_DUMP_RESTART,
> +};
> +
> +#define WATCHDOG_ACTION 1
> +static unsigned int action = WATCHDOG_ACTION;
> +module_param(action, uint, 0444);
> +MODULE_PARM_DESC(action, "Action taken when watchdog expires (default="
> +		 __MODULE_STRING(WATCHDOG_ACTION) ")");
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0444);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +#define WATCHDOG_TIMEOUT 60
> +static unsigned int timeout = WATCHDOG_TIMEOUT;
> +module_param(timeout, uint, 0444);
> +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default="
> +		 __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
> +
> +struct pseries_wdt {
> +	struct watchdog_device wd;
> +	unsigned long action;
> +	unsigned long num;		/* Watchdog numbers are 1-based */
> +};
> +
> +static int pseries_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct device *dev = wdd->parent;
> +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> +	unsigned long flags, msecs;
> +	long rc;
> +
> +	flags = pw->action | PSERIES_WDTF_OP_START;
> +	msecs = wdd->timeout * 1000UL;
> +	rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
> +	if (rc != H_SUCCESS) {
> +		dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
> +			 rc, pw->num);
> +		return -EIO;
> +	}
> +	return 0;
> +}
> +
> +static int pseries_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct device *dev = wdd->parent;
> +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> +	long rc;
> +
> +	rc = plpar_hcall_norets(H_WATCHDOG, PSERIES_WDTF_OP_STOP, pw->num);
> +	if (rc != H_SUCCESS && rc != H_NOOP) {
> +		dev_crit(dev, "H_WATCHDOG: %ld: failed to stop timer %lu",
> +			 rc, pw->num);
> +		return -EIO;
> +	}
> +	return 0;
> +}
> +
> +static struct watchdog_info pseries_wdt_info = {
> +	.identity = DRV_NAME,
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT
> +	    | WDIOF_PRETIMEOUT,
> +};
> +
> +static const struct watchdog_ops pseries_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pseries_wdt_start,
> +	.stop = pseries_wdt_stop,
> +};
> +
> +static int pseries_wdt_probe(struct platform_device *pdev)
> +{
> +	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
> +	unsigned long cap;
> +	long rc;
> +	struct pseries_wdt *pw;
> +	int err;
> +
> +	rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
> +	if (rc == H_FUNCTION)
> +		return -ENODEV;
> +	if (rc != H_SUCCESS)
> +		return -EIO;
> +	cap = ret[0];
> +
> +	pw = devm_kzalloc(&pdev->dev, sizeof(*pw), GFP_KERNEL);
> +	if (!pw)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Assume watchdogNumber 1 for now.  If we ever support
> +	 * multiple timers we will need to devise a way to choose a
> +	 * distinct watchdogNumber for each platform device at device
> +	 * registration time.
> +	 */
> +	pw->num = 1;
> +
> +	if (action >= ARRAY_SIZE(pseries_wdt_action))
> +		return -EINVAL;
> +	pw->action = pseries_wdt_action[action];
> +
> +	pw->wd.parent = &pdev->dev;
> +	pw->wd.info = &pseries_wdt_info;
> +	pw->wd.ops = &pseries_wdt_ops;
> +	pw->wd.min_timeout = DIV_ROUND_UP(PSERIES_WDTQ_MIN_TIMEOUT(cap), 1000);
> +	pw->wd.max_timeout = UINT_MAX / 1000;
> +	pw->wd.timeout = timeout;
> +	if (watchdog_init_timeout(&pw->wd, 0, NULL))
> +		return -EINVAL;
> +	watchdog_set_nowayout(&pw->wd, nowayout);
> +	watchdog_stop_on_reboot(&pw->wd);
> +	watchdog_stop_on_unregister(&pw->wd);
> +	watchdog_set_drvdata(&pw->wd, pw);
> +
> +	err = devm_watchdog_register_device(&pdev->dev, &pw->wd);
> +	if (err)
> +		return err;
> +
> +	platform_set_drvdata(pdev, &pw->wd);
> +
> +	return 0;
> +}
> +
> +static int pseries_wdt_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> +
> +	if (watchdog_active(wd))
> +		return pseries_wdt_stop(wd);
> +	return 0;
> +}
> +
> +static int pseries_wdt_resume(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> +
> +	if (watchdog_active(wd))
> +		return pseries_wdt_start(wd);
> +	return 0;
> +}
> +
> +static const struct platform_device_id pseries_wdt_id[] = {
> +	{ .name = "pseries-wdt" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(platform, pseries_wdt_id);
> +
> +static struct platform_driver pseries_wdt_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +	.id_table = pseries_wdt_id,
> +	.probe = pseries_wdt_probe,
> +	.resume = pseries_wdt_resume,
> +	.suspend = pseries_wdt_suspend,
> +};
> +module_platform_driver(pseries_wdt_driver);
> +
> +MODULE_AUTHOR("Alexey Kardashevskiy <aik@ozlabs.ru>");
> +MODULE_AUTHOR("Scott Cheloha <cheloha@linux.ibm.com>");
> +MODULE_DESCRIPTION("POWER Architecture Platform Watchdog Driver");
> +MODULE_LICENSE("GPL");

-- 
Alexey

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/4] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code
  2022-06-02 17:53   ` Scott Cheloha
@ 2022-06-21 14:44     ` Nathan Lynch
  -1 siblings, 0 replies; 39+ messages in thread
From: Nathan Lynch @ 2022-06-21 14:44 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: linux, tzungbi, brking, aik, npiggin, vaishnavi, wvoigt,
	linuxppc-dev, Scott Cheloha

Scott Cheloha <cheloha@linux.ibm.com> writes:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> guest control of one or more virtual watchdog timers.
>
> Add the opcode for the H_WATCHDOG hypercall to hvcall.h.  While here,
> add a definition for H_NOOP, a possible return code for H_WATCHDOG.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/hvcall.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index d92a20a85395..4b4f69c35b4f 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -87,6 +87,7 @@
>  #define H_P7		-60
>  #define H_P8		-61
>  #define H_P9		-62
> +#define H_NOOP		-63
>  #define H_TOO_BIG	-64
>  #define H_UNSUPPORTED	-67
>  #define H_OVERLAP	-68

Not a problem to fix in your series, but I guess these should be
parenthesized i.e.

#define H_P7		(-60)
#define H_P8		(-61)
#define H_P9		(-62)
#define H_NOOP		(-63)

> @@ -324,7 +325,8 @@
>  #define H_RPT_INVALIDATE	0x448
>  #define H_SCM_FLUSH		0x44C
>  #define H_GET_ENERGY_SCALE_INFO	0x450
> -#define MAX_HCALL_OPCODE	H_GET_ENERGY_SCALE_INFO
> +#define H_WATCHDOG		0x45C
> +#define MAX_HCALL_OPCODE	H_WATCHDOG
>

Looks fine.

Reviewed-by: Nathan Lynch <nathanl@linux.ibm.com>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/4] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code
@ 2022-06-21 14:44     ` Nathan Lynch
  0 siblings, 0 replies; 39+ messages in thread
From: Nathan Lynch @ 2022-06-21 14:44 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux

Scott Cheloha <cheloha@linux.ibm.com> writes:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> guest control of one or more virtual watchdog timers.
>
> Add the opcode for the H_WATCHDOG hypercall to hvcall.h.  While here,
> add a definition for H_NOOP, a possible return code for H_WATCHDOG.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/hvcall.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index d92a20a85395..4b4f69c35b4f 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -87,6 +87,7 @@
>  #define H_P7		-60
>  #define H_P8		-61
>  #define H_P9		-62
> +#define H_NOOP		-63
>  #define H_TOO_BIG	-64
>  #define H_UNSUPPORTED	-67
>  #define H_OVERLAP	-68

Not a problem to fix in your series, but I guess these should be
parenthesized i.e.

#define H_P7		(-60)
#define H_P8		(-61)
#define H_P9		(-62)
#define H_NOOP		(-63)

> @@ -324,7 +325,8 @@
>  #define H_RPT_INVALIDATE	0x448
>  #define H_SCM_FLUSH		0x44C
>  #define H_GET_ENERGY_SCALE_INFO	0x450
> -#define MAX_HCALL_OPCODE	H_GET_ENERGY_SCALE_INFO
> +#define H_WATCHDOG		0x45C
> +#define MAX_HCALL_OPCODE	H_WATCHDOG
>

Looks fine.

Reviewed-by: Nathan Lynch <nathanl@linux.ibm.com>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 2/4] powerpc/pseries: add FW_FEATURE_WATCHDOG flag
  2022-06-02 17:53   ` Scott Cheloha
@ 2022-06-21 15:03     ` Nathan Lynch
  -1 siblings, 0 replies; 39+ messages in thread
From: Nathan Lynch @ 2022-06-21 15:03 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: linux, tzungbi, brking, aik, npiggin, vaishnavi, wvoigt,
	linuxppc-dev, Scott Cheloha

Scott Cheloha <cheloha@linux.ibm.com> writes:
> PAPR v2.12 specifies a new optional function set, "hcall-watchdog",
> for the /rtas/ibm,hypertas-functions property.  The presence of this
> function set indicates support for the H_WATCHDOG hypercall.
>
> Check for this function set and, if present, set the new
> FW_FEATURE_WATCHDOG flag.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>

...

> diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h
> index 8dddd34b8ecf..398e0b5e485f 100644
> --- a/arch/powerpc/include/asm/firmware.h
> +++ b/arch/powerpc/include/asm/firmware.h
> @@ -55,6 +55,7 @@
>  #define FW_FEATURE_RPT_INVALIDATE ASM_CONST(0x0000010000000000)
>  #define FW_FEATURE_FORM2_AFFINITY ASM_CONST(0x0000020000000000)
>  #define FW_FEATURE_ENERGY_SCALE_INFO ASM_CONST(0x0000040000000000)
> +#define FW_FEATURE_WATCHDOG	ASM_CONST(0x0000080000000000)
>  
>  #ifndef __ASSEMBLY__
>  
> @@ -76,7 +77,7 @@ enum {
>  		FW_FEATURE_DRC_INFO | FW_FEATURE_BLOCK_REMOVE |
>  		FW_FEATURE_PAPR_SCM | FW_FEATURE_ULTRAVISOR |
>  		FW_FEATURE_RPT_INVALIDATE | FW_FEATURE_FORM2_AFFINITY |
> -		FW_FEATURE_ENERGY_SCALE_INFO,
> +		FW_FEATURE_ENERGY_SCALE_INFO | FW_FEATURE_WATCHDOG,
>  	FW_FEATURE_PSERIES_ALWAYS = 0,
>  	FW_FEATURE_POWERNV_POSSIBLE = FW_FEATURE_OPAL | FW_FEATURE_ULTRAVISOR,
>  	FW_FEATURE_POWERNV_ALWAYS = 0,
> diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c
> index 09c119b2f623..080108d129ed 100644
> --- a/arch/powerpc/platforms/pseries/firmware.c
> +++ b/arch/powerpc/platforms/pseries/firmware.c
> @@ -67,6 +67,7 @@ hypertas_fw_features_table[] = {
>  	{FW_FEATURE_PAPR_SCM,		"hcall-scm"},
>  	{FW_FEATURE_RPT_INVALIDATE,	"hcall-rpt-invalidate"},
>  	{FW_FEATURE_ENERGY_SCALE_INFO,	"hcall-energy-scale-info"},
> +	{FW_FEATURE_WATCHDOG,		"hcall-watchdog"},
>  };

All looks correct.

Reviewed-by: Nathan Lynch <nathanl@linux.ibm.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 2/4] powerpc/pseries: add FW_FEATURE_WATCHDOG flag
@ 2022-06-21 15:03     ` Nathan Lynch
  0 siblings, 0 replies; 39+ messages in thread
From: Nathan Lynch @ 2022-06-21 15:03 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux

Scott Cheloha <cheloha@linux.ibm.com> writes:
> PAPR v2.12 specifies a new optional function set, "hcall-watchdog",
> for the /rtas/ibm,hypertas-functions property.  The presence of this
> function set indicates support for the H_WATCHDOG hypercall.
>
> Check for this function set and, if present, set the new
> FW_FEATURE_WATCHDOG flag.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>

...

> diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h
> index 8dddd34b8ecf..398e0b5e485f 100644
> --- a/arch/powerpc/include/asm/firmware.h
> +++ b/arch/powerpc/include/asm/firmware.h
> @@ -55,6 +55,7 @@
>  #define FW_FEATURE_RPT_INVALIDATE ASM_CONST(0x0000010000000000)
>  #define FW_FEATURE_FORM2_AFFINITY ASM_CONST(0x0000020000000000)
>  #define FW_FEATURE_ENERGY_SCALE_INFO ASM_CONST(0x0000040000000000)
> +#define FW_FEATURE_WATCHDOG	ASM_CONST(0x0000080000000000)
>  
>  #ifndef __ASSEMBLY__
>  
> @@ -76,7 +77,7 @@ enum {
>  		FW_FEATURE_DRC_INFO | FW_FEATURE_BLOCK_REMOVE |
>  		FW_FEATURE_PAPR_SCM | FW_FEATURE_ULTRAVISOR |
>  		FW_FEATURE_RPT_INVALIDATE | FW_FEATURE_FORM2_AFFINITY |
> -		FW_FEATURE_ENERGY_SCALE_INFO,
> +		FW_FEATURE_ENERGY_SCALE_INFO | FW_FEATURE_WATCHDOG,
>  	FW_FEATURE_PSERIES_ALWAYS = 0,
>  	FW_FEATURE_POWERNV_POSSIBLE = FW_FEATURE_OPAL | FW_FEATURE_ULTRAVISOR,
>  	FW_FEATURE_POWERNV_ALWAYS = 0,
> diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c
> index 09c119b2f623..080108d129ed 100644
> --- a/arch/powerpc/platforms/pseries/firmware.c
> +++ b/arch/powerpc/platforms/pseries/firmware.c
> @@ -67,6 +67,7 @@ hypertas_fw_features_table[] = {
>  	{FW_FEATURE_PAPR_SCM,		"hcall-scm"},
>  	{FW_FEATURE_RPT_INVALIDATE,	"hcall-rpt-invalidate"},
>  	{FW_FEATURE_ENERGY_SCALE_INFO,	"hcall-energy-scale-info"},
> +	{FW_FEATURE_WATCHDOG,		"hcall-watchdog"},
>  };

All looks correct.

Reviewed-by: Nathan Lynch <nathanl@linux.ibm.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 3/4] powerpc/pseries: register pseries-wdt device with platform bus
  2022-06-02 17:53   ` Scott Cheloha
  (?)
@ 2022-06-21 15:30   ` Nathan Lynch
  2022-06-24 13:27     ` Michael Ellerman
  -1 siblings, 1 reply; 39+ messages in thread
From: Nathan Lynch @ 2022-06-21 15:30 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux

Scott Cheloha <cheloha@linux.ibm.com> writes:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> guest control of one or more virtual watchdog timers.
>
> These timers do not conform to PowerPC device conventions.  They are
> not affixed to any extant bus, nor do they have full representation in
> the device tree.
>
> As a workaround we represent them as platform devices.
>
> This patch registers a single platform device, "pseries-wdt", with the
> platform bus if the FW_FEATURE_WATCHDOG flag is set.
>
> A driver for this device, "pseries-wdt", will be introduced in a
> subsequent patch.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/setup.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index afb074269b42..233c64f59815 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -14,6 +14,7 @@
>  
>  #include <linux/cpu.h>
>  #include <linux/errno.h>
> +#include <linux/platform_device.h>
>  #include <linux/sched.h>
>  #include <linux/kernel.h>
>  #include <linux/mm.h>
> @@ -169,6 +170,22 @@ static void __init fwnmi_init(void)
>  #endif
>  }
>  
> +/*
> + * Affix a device for the first timer to the platform bus if
> + * we have firmware support for the H_WATCHDOG hypercall.
> + */
> +static struct platform_device *pseries_wdt_pdev;
> +
> +static __init int pseries_wdt_init(void)
> +{
> +	if (!firmware_has_feature(FW_FEATURE_WATCHDOG))
> +		return 0;
> +	pseries_wdt_pdev = platform_device_register_simple("pseries-wdt",
> +							   0, NULL, 0);
> +	return 0;
> +}
> +machine_subsys_initcall(pseries, pseries_wdt_init);
> +

Seems like we don't need pseries_wdt_pdev as it's unused elsewhere? But
that's quite minor.

Reviewed-by: Nathan Lynch <nathanl@linux.ibm.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-02 17:53   ` Scott Cheloha
@ 2022-06-21 15:45     ` Nathan Lynch
  -1 siblings, 0 replies; 39+ messages in thread
From: Nathan Lynch @ 2022-06-21 15:45 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: linux, tzungbi, brking, aik, npiggin, vaishnavi, wvoigt,
	linuxppc-dev, Scott Cheloha

Scott Cheloha <cheloha@linux.ibm.com> writes:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> guest control of one or more virtual watchdog timers.  The timers have
> millisecond granularity.  The guest is terminated when a timer
> expires.
>
> This patch adds a watchdog driver for these timers, "pseries-wdt".
>
> pseries_wdt_probe() currently assumes the existence of only one
> platform device and always assigns it watchdogNumber 1.  If we ever
> expose more than one timer to userspace we will need to devise a way
> to assign a distinct watchdogNumber to each platform device at device
> registration time.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>

Reviewed-by: Nathan Lynch <nathanl@linux.ibm.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
@ 2022-06-21 15:45     ` Nathan Lynch
  0 siblings, 0 replies; 39+ messages in thread
From: Nathan Lynch @ 2022-06-21 15:45 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux

Scott Cheloha <cheloha@linux.ibm.com> writes:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> guest control of one or more virtual watchdog timers.  The timers have
> millisecond granularity.  The guest is terminated when a timer
> expires.
>
> This patch adds a watchdog driver for these timers, "pseries-wdt".
>
> pseries_wdt_probe() currently assumes the existence of only one
> platform device and always assigns it watchdogNumber 1.  If we ever
> expose more than one timer to userspace we will need to devise a way
> to assign a distinct watchdogNumber to each platform device at device
> registration time.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>

Reviewed-by: Nathan Lynch <nathanl@linux.ibm.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/4] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code
  2022-06-21 14:44     ` Nathan Lynch
@ 2022-06-21 17:31       ` Segher Boessenkool
  -1 siblings, 0 replies; 39+ messages in thread
From: Segher Boessenkool @ 2022-06-21 17:31 UTC (permalink / raw)
  To: Nathan Lynch
  Cc: wvoigt, linux-watchdog, aik, Scott Cheloha, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

Hi!

On Tue, Jun 21, 2022 at 09:44:42AM -0500, Nathan Lynch wrote:
> Not a problem to fix in your series, but I guess these should be
> parenthesized i.e.
> 
> #define H_P7		(-60)
> #define H_P8		(-61)
> #define H_P9		(-62)
> #define H_NOOP		(-63)

Why?  It does not change the semantics of any correct code.  For what
incorrect code will it make the diagnostics clearer?


Segher

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/4] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code
@ 2022-06-21 17:31       ` Segher Boessenkool
  0 siblings, 0 replies; 39+ messages in thread
From: Segher Boessenkool @ 2022-06-21 17:31 UTC (permalink / raw)
  To: Nathan Lynch
  Cc: Scott Cheloha, linux-watchdog, wvoigt, aik, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

Hi!

On Tue, Jun 21, 2022 at 09:44:42AM -0500, Nathan Lynch wrote:
> Not a problem to fix in your series, but I guess these should be
> parenthesized i.e.
> 
> #define H_P7		(-60)
> #define H_P8		(-61)
> #define H_P9		(-62)
> #define H_NOOP		(-63)

Why?  It does not change the semantics of any correct code.  For what
incorrect code will it make the diagnostics clearer?


Segher

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/4] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code
  2022-06-21 17:31       ` Segher Boessenkool
@ 2022-06-21 22:22         ` Nathan Lynch
  -1 siblings, 0 replies; 39+ messages in thread
From: Nathan Lynch @ 2022-06-21 22:22 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: Scott Cheloha, linux-watchdog, wvoigt, aik, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Tue, Jun 21, 2022 at 09:44:42AM -0500, Nathan Lynch wrote:
>> Not a problem to fix in your series, but I guess these should be
>> parenthesized i.e.
>> 
>> #define H_P7		(-60)
>> #define H_P8		(-61)
>> #define H_P9		(-62)
>> #define H_NOOP		(-63)
>
> Why?  It does not change the semantics of any correct code.  For what
> incorrect code will it make the diagnostics clearer?

Yeah never mind, I was confused.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 1/4] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code
@ 2022-06-21 22:22         ` Nathan Lynch
  0 siblings, 0 replies; 39+ messages in thread
From: Nathan Lynch @ 2022-06-21 22:22 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: wvoigt, linux-watchdog, aik, Scott Cheloha, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Tue, Jun 21, 2022 at 09:44:42AM -0500, Nathan Lynch wrote:
>> Not a problem to fix in your series, but I guess these should be
>> parenthesized i.e.
>> 
>> #define H_P7		(-60)
>> #define H_P8		(-61)
>> #define H_P9		(-62)
>> #define H_NOOP		(-63)
>
> Why?  It does not change the semantics of any correct code.  For what
> incorrect code will it make the diagnostics clearer?

Yeah never mind, I was confused.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-02 17:53   ` Scott Cheloha
                     ` (2 preceding siblings ...)
  (?)
@ 2022-06-24 13:27   ` Michael Ellerman
  2022-06-24 15:31       ` Segher Boessenkool
  2022-07-08  5:51       ` Scott Cheloha
  -1 siblings, 2 replies; 39+ messages in thread
From: Michael Ellerman @ 2022-06-24 13:27 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: nathanl, wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi,
	brking, linuxppc-dev, linux

Hi Scott,

A few comments below ...

Scott Cheloha <cheloha@linux.ibm.com> writes:
> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> guest control of one or more virtual watchdog timers.  The timers have
> millisecond granularity.  The guest is terminated when a timer
> expires.
>
> This patch adds a watchdog driver for these timers, "pseries-wdt".
>
> pseries_wdt_probe() currently assumes the existence of only one
> platform device and always assigns it watchdogNumber 1.  If we ever
> expose more than one timer to userspace we will need to devise a way
> to assign a distinct watchdogNumber to each platform device at device
> registration time.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
> ---
>  .../watchdog/watchdog-parameters.rst          |  12 +
>  drivers/watchdog/Kconfig                      |   8 +
>  drivers/watchdog/Makefile                     |   1 +
>  drivers/watchdog/pseries-wdt.c                | 264 ++++++++++++++++++
>  4 files changed, 285 insertions(+)
>  create mode 100644 drivers/watchdog/pseries-wdt.c
>
> diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
> index 223c99361a30..29153eed6689 100644
> --- a/Documentation/watchdog/watchdog-parameters.rst
> +++ b/Documentation/watchdog/watchdog-parameters.rst
> @@ -425,6 +425,18 @@ pnx833x_wdt:
>  
>  -------------------------------------------------
>  
> +pseries-wdt:
> +    action:
> +	Action taken when watchdog expires: 0 (power off), 1 (restart),
> +	2 (dump and restart). (default=1)

I doesn't look like these values match what other drivers use to any
great extent.

So why not use the values from PAPR directly?

ie. 1 = power off, 2 = hard reset, 3 = dump & restart.

It seems like it would be easier to follow if the values map directly.

It's possible in future PAPR adds 247 to mean something, in which case
maybe we'd want to map that to a less silly value, but at least for now
the PAPR values are sensible enough.

> +    timeout:
> +	Initial watchdog timeout in seconds. (default=60)

That seems like a pretty common value, I don't see any guidance in PAPR.
Do we have any input from PowerVM on whether that's a good value?

> diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
> new file mode 100644
> index 000000000000..cfe53587457d
> --- /dev/null
> +++ b/drivers/watchdog/pseries-wdt.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 International Business Machines, Inc.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/math.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define DRV_NAME "pseries-wdt"
> +
> +/*
> + * The PAPR's MSB->LSB bit ordering is 0->63.  These macros simplify
> + * defining bitfields as described in the PAPR without needing to
> + * transpose values to the more C-like 63->0 ordering.
> + */
> +#define SETFIELD(_v, _b, _e)	\
> +	(((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
> +#define GETFIELD(_v, _b, _e)	\
> +	(((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))

This will probably sound like a cranky maintainer rant, but ...,
I really dislike these GETFIELD/SETFIELD macros.

I know you didn't invent them, but I would be much happier if you didn't
use them.

I know they (slightly) simplify things when you're transcribing values
from PAPR into the source, but that happens only once.

And then for the rest of eternity the source is harder to read because
there's this ridiculous level of indirection through insane macros just
to define some constants.

Anyone trying to use a debugger against this code will see a value in
memory like 0x200 and have to sit down and work out which SETFIELD()
macro it corresponds to.

> +/*
> + * The H_WATCHDOG hypercall first appears in PAPR v2.12 and is
> + * described fully in sections 14.5 and 14.15.6.
> + *
> + *
> + * H_WATCHDOG Input
> + *
> + * R4: "flags":
> + *
> + *         Bits 48-55: "operation"
> + *
> + *             0x01  Start Watchdog
> + *             0x02  Stop Watchdog
> + *             0x03  Query Watchdog Capabilities
> + */
> +#define PSERIES_WDTF_OP(op)		SETFIELD((op), 48, 55)
> +#define PSERIES_WDTF_OP_START		PSERIES_WDTF_OP(0x1)
> +#define PSERIES_WDTF_OP_STOP		PSERIES_WDTF_OP(0x2)
> +#define PSERIES_WDTF_OP_QUERY		PSERIES_WDTF_OP(0x3)
 
eg, IMHO these are much more reader friendly:

#define PSERIES_WDTF_OP_START		(1 << 8)
#define PSERIES_WDTF_OP_STOP		(2 << 8)
#define PSERIES_WDTF_OP_QUERY		(3 << 8)

> +/*
> + *         Bits 56-63: "timeoutAction" (for "Start Watchdog" only)
> + *
> + *             0x01  Hard poweroff
> + *             0x02  Hard restart
> + *             0x03  Dump restart
> + */
> +#define PSERIES_WDTF_ACTION(ac)			SETFIELD(ac, 56, 63)
> +#define PSERIES_WDTF_ACTION_HARD_POWEROFF	PSERIES_WDTF_ACTION(0x1)
> +#define PSERIES_WDTF_ACTION_HARD_RESTART	PSERIES_WDTF_ACTION(0x2)
> +#define PSERIES_WDTF_ACTION_DUMP_RESTART	PSERIES_WDTF_ACTION(0x3)

These are a slam dunk:

#define PSERIES_WDTF_ACTION_HARD_POWEROFF	1
#define PSERIES_WDTF_ACTION_HARD_RESTART	2
#define PSERIES_WDTF_ACTION_DUMP_RESTART	3

> +
> +/*
> + * H_WATCHDOG Output
> + *
> + * R3: Return code
> + *
> + *     H_SUCCESS    The operation completed.
> + *
> + *     H_BUSY	    The hypervisor is too busy; retry the operation.
> + *
> + *     H_PARAMETER  The given "flags" are somehow invalid.  Either the
> + *                  "operation" or "timeoutAction" is invalid, or a
> + *                  reserved bit is set.
> + *
> + *     H_P2         The given "watchdogNumber" is zero or exceeds the
> + *                  supported maximum value.
> + *
> + *     H_P3         The given "timeoutInMs" is below the supported
> + *                  minimum value.
> + *
> + *     H_NOOP       The given "watchdogNumber" is already stopped.
> + *
> + *     H_HARDWARE   The operation failed for ineffable reasons.
> + *
> + *     H_FUNCTION   The H_WATCHDOG hypercall is not supported by this
> + *                  hypervisor.
> + *
> + * R4:
> + *
> + * - For the "Query Watchdog Capabilities" operation, a 64-bit
> + *   value structured as follows:
> + *
> + *       Bits  0-15: The minimum supported timeout in milliseconds.
> + *       Bits 16-31: The number of watchdogs supported.
> + *       Bits 32-63: Reserved.
> + */
> +#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	GETFIELD((cap), 0, 15)

This one is less obviously better, but I still think it's clearer as all
the logic is there in front of you, rather than hidden in the macro. It
is clearer that we're only returning a 16-bit value.

#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	(((cap) >> 48) & 0xffff)

> +#define PSERIES_WDTQ_MAX_NUMBER(cap)	GETFIELD((cap), 16, 31)

That's unused.

I guess we're assuming at least one timer is always supported? Seems
reasonable.

> +
> +static const unsigned long pseries_wdt_action[] = {
> +	[0] = PSERIES_WDTF_ACTION_HARD_POWEROFF,
> +	[1] = PSERIES_WDTF_ACTION_HARD_RESTART,
> +	[2] = PSERIES_WDTF_ACTION_DUMP_RESTART,
> +};

If we used the PAPR values we wouldn't need that ^

> +#define WATCHDOG_ACTION 1

DEFAULT_ACTION ?

> +static unsigned int action = WATCHDOG_ACTION;
> +module_param(action, uint, 0444);
> +MODULE_PARM_DESC(action, "Action taken when watchdog expires (default="
> +		 __MODULE_STRING(WATCHDOG_ACTION) ")");
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0444);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +#define WATCHDOG_TIMEOUT 60

DEFAULT_TIMEOUT ?

> +static unsigned int timeout = WATCHDOG_TIMEOUT;
> +module_param(timeout, uint, 0444);
> +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default="
> +		 __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
> +
> +struct pseries_wdt {
> +	struct watchdog_device wd;
> +	unsigned long action;
> +	unsigned long num;		/* Watchdog numbers are 1-based */

num can just be an int.

But do we even need it, do we anticipate supporting multiple timers?
Should we just hard code '1' ?

> +};
> +
> +static int pseries_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct device *dev = wdd->parent;
> +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> +	unsigned long flags, msecs;
> +	long rc;
> +
> +	flags = pw->action | PSERIES_WDTF_OP_START;

We set pw->action at probe time based on the module param action, but
this is the only place we use it.

If we use the PAPR values, this could just be:

      flags = (pw->action << 8) | PSERIES_WDTF_OP_START;

And is there any benefit in storing action in pseries_wdt, we could just
use the module param value here.

> +	msecs = wdd->timeout * 1000UL;
 
Using MSEC_PER_SEC makes it clearer what that conversion is doing.

> +	rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
> +	if (rc != H_SUCCESS) {
> +		dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
> +			 rc, pw->num);
> +		return -EIO;
> +	}
> +	return 0;
> +}
> +
> +static int pseries_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct device *dev = wdd->parent;
> +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> +	long rc;
> +
> +	rc = plpar_hcall_norets(H_WATCHDOG, PSERIES_WDTF_OP_STOP, pw->num);
> +	if (rc != H_SUCCESS && rc != H_NOOP) {
> +		dev_crit(dev, "H_WATCHDOG: %ld: failed to stop timer %lu",
> +			 rc, pw->num);
> +		return -EIO;
> +	}
> +	return 0;
> +}
> +
> +static struct watchdog_info pseries_wdt_info = {
> +	.identity = DRV_NAME,
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT
> +	    | WDIOF_PRETIMEOUT,

I don't know the watchdog code to know if those make sense.

> +};
> +
> +static const struct watchdog_ops pseries_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pseries_wdt_start,
> +	.stop = pseries_wdt_stop,
> +};
> +
> +static int pseries_wdt_probe(struct platform_device *pdev)
> +{
> +	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
> +	unsigned long cap;
> +	long rc;
> +	struct pseries_wdt *pw;
> +	int err;

Try to use reverse xmas tree for new code please.

> +	rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
> +	if (rc == H_FUNCTION)
> +		return -ENODEV;
> +	if (rc != H_SUCCESS)
> +		return -EIO;
> +	cap = ret[0];
> +
> +	pw = devm_kzalloc(&pdev->dev, sizeof(*pw), GFP_KERNEL);
> +	if (!pw)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Assume watchdogNumber 1 for now.  If we ever support
> +	 * multiple timers we will need to devise a way to choose a
> +	 * distinct watchdogNumber for each platform device at device
> +	 * registration time.
> +	 */
> +	pw->num = 1;
> +
> +	if (action >= ARRAY_SIZE(pseries_wdt_action))
> +		return -EINVAL;
> +	pw->action = pseries_wdt_action[action];
> +
> +	pw->wd.parent = &pdev->dev;
> +	pw->wd.info = &pseries_wdt_info;
> +	pw->wd.ops = &pseries_wdt_ops;
> +	pw->wd.min_timeout = DIV_ROUND_UP(PSERIES_WDTQ_MIN_TIMEOUT(cap), 1000);

MSEC_TO_SEC again?

> +	pw->wd.max_timeout = UINT_MAX / 1000;
 
Where does that value come from?

> +	pw->wd.timeout = timeout;
> +	if (watchdog_init_timeout(&pw->wd, 0, NULL))
> +		return -EINVAL;

It's late so maybe I'm misreading it, but does watchdog_init_timeout()
actually clamp the values if we don't pass a timeout?

It looks like basically a nop when we pass timeout_param=0 and dev=NULL.

Which makes me think we aren't checking anywhere that the timeout we are
using >= what firmware will accept.

> +	watchdog_set_nowayout(&pw->wd, nowayout);
> +	watchdog_stop_on_reboot(&pw->wd);
> +	watchdog_stop_on_unregister(&pw->wd);
> +	watchdog_set_drvdata(&pw->wd, pw);
> +
> +	err = devm_watchdog_register_device(&pdev->dev, &pw->wd);
> +	if (err)
> +		return err;
> +
> +	platform_set_drvdata(pdev, &pw->wd);
> +
> +	return 0;
> +}
> +
> +static int pseries_wdt_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> +
> +	if (watchdog_active(wd))
> +		return pseries_wdt_stop(wd);
> +	return 0;
> +}
> +
> +static int pseries_wdt_resume(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> +
> +	if (watchdog_active(wd))
> +		return pseries_wdt_start(wd);
> +	return 0;
> +}
> +
> +static const struct platform_device_id pseries_wdt_id[] = {
> +	{ .name = "pseries-wdt" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(platform, pseries_wdt_id);
> +
> +static struct platform_driver pseries_wdt_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +	.id_table = pseries_wdt_id,
> +	.probe = pseries_wdt_probe,
> +	.resume = pseries_wdt_resume,
> +	.suspend = pseries_wdt_suspend,

I don't see any handling of the possible requirement to suspend timers
across LPM. I don't think just wiring these up is enough?

> +};
> +module_platform_driver(pseries_wdt_driver);
> +
> +MODULE_AUTHOR("Alexey Kardashevskiy <aik@ozlabs.ru>");
> +MODULE_AUTHOR("Scott Cheloha <cheloha@linux.ibm.com>");

I'd prefer the module authors were just the names, email addresses
inevitably bitrot.

Your email address is in the change log.

cheers

> +MODULE_DESCRIPTION("POWER Architecture Platform Watchdog Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.27.0

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 3/4] powerpc/pseries: register pseries-wdt device with platform bus
  2022-06-21 15:30   ` Nathan Lynch
@ 2022-06-24 13:27     ` Michael Ellerman
  2022-07-07 15:53         ` Scott Cheloha
  0 siblings, 1 reply; 39+ messages in thread
From: Michael Ellerman @ 2022-06-24 13:27 UTC (permalink / raw)
  To: Nathan Lynch, Scott Cheloha, linux-watchdog
  Cc: wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi, brking,
	linuxppc-dev, linux

Nathan Lynch <nathanl@linux.ibm.com> writes:
> Scott Cheloha <cheloha@linux.ibm.com> writes:
>> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
>> guest control of one or more virtual watchdog timers.
...
>
> Seems like we don't need pseries_wdt_pdev as it's unused elsewhere? But
> that's quite minor.

It's minor but please drop it in the next version.

cheers

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-02 17:53   ` Scott Cheloha
                     ` (3 preceding siblings ...)
  (?)
@ 2022-06-24 13:51   ` Michael Ellerman
  2022-07-07 15:53       ` Scott Cheloha
  -1 siblings, 1 reply; 39+ messages in thread
From: Michael Ellerman @ 2022-06-24 13:51 UTC (permalink / raw)
  To: Scott Cheloha, linux-watchdog
  Cc: nathanl, wvoigt, aik, Scott Cheloha, vaishnavi, npiggin, tzungbi,
	brking, linuxppc-dev, linux

Scott Cheloha <cheloha@linux.ibm.com> writes:
...
> +
> +static struct platform_driver pseries_wdt_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,

That owner assignment is not required.

It's set for you by platform_driver_register() via
module_platform_driver().

cheers

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-24 13:27   ` Michael Ellerman
@ 2022-06-24 15:31       ` Segher Boessenkool
  2022-07-08  5:51       ` Scott Cheloha
  1 sibling, 0 replies; 39+ messages in thread
From: Segher Boessenkool @ 2022-06-24 15:31 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: nathanl, wvoigt, linux-watchdog, aik, Scott Cheloha, vaishnavi,
	npiggin, tzungbi, brking, linuxppc-dev, linux

On Fri, Jun 24, 2022 at 11:27:24PM +1000, Michael Ellerman wrote:
> Scott Cheloha <cheloha@linux.ibm.com> writes:
> > + * - For the "Query Watchdog Capabilities" operation, a 64-bit
> > + *   value structured as follows:
> > + *
> > + *       Bits  0-15: The minimum supported timeout in milliseconds.
> > + *       Bits 16-31: The number of watchdogs supported.
> > + *       Bits 32-63: Reserved.
> > + */
> > +#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	GETFIELD((cap), 0, 15)
> 
> This one is less obviously better, but I still think it's clearer as all
> the logic is there in front of you, rather than hidden in the macro. It
> is clearer that we're only returning a 16-bit value.
> 
> #define PSERIES_WDTQ_MIN_TIMEOUT(cap)	(((cap) >> 48) & 0xffff)

Or even
  ((cap) >> 48)
since it is a 64-bit value.  If you want better defences you should not
use macros here at all, anyway (but inline functions, instead).

I could rant about the 1000UL being meaningless and/or misleading, or
that 0x1 is just silly, but it is a sunny day :-)


Segher

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
@ 2022-06-24 15:31       ` Segher Boessenkool
  0 siblings, 0 replies; 39+ messages in thread
From: Segher Boessenkool @ 2022-06-24 15:31 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Scott Cheloha, linux-watchdog, nathanl, wvoigt, aik, vaishnavi,
	npiggin, tzungbi, brking, linuxppc-dev, linux

On Fri, Jun 24, 2022 at 11:27:24PM +1000, Michael Ellerman wrote:
> Scott Cheloha <cheloha@linux.ibm.com> writes:
> > + * - For the "Query Watchdog Capabilities" operation, a 64-bit
> > + *   value structured as follows:
> > + *
> > + *       Bits  0-15: The minimum supported timeout in milliseconds.
> > + *       Bits 16-31: The number of watchdogs supported.
> > + *       Bits 32-63: Reserved.
> > + */
> > +#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	GETFIELD((cap), 0, 15)
> 
> This one is less obviously better, but I still think it's clearer as all
> the logic is there in front of you, rather than hidden in the macro. It
> is clearer that we're only returning a 16-bit value.
> 
> #define PSERIES_WDTQ_MIN_TIMEOUT(cap)	(((cap) >> 48) & 0xffff)

Or even
  ((cap) >> 48)
since it is a 64-bit value.  If you want better defences you should not
use macros here at all, anyway (but inline functions, instead).

I could rant about the 1000UL being meaningless and/or misleading, or
that 0x1 is just silly, but it is a sunny day :-)


Segher

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-24 13:51   ` Michael Ellerman
@ 2022-07-07 15:53       ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-07-07 15:53 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: linux-watchdog, nathanl, wvoigt, aik, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

On Fri, Jun 24, 2022 at 11:51:01PM +1000, Michael Ellerman wrote:
> Scott Cheloha <cheloha@linux.ibm.com> writes:
> ...
> > +
> > +static struct platform_driver pseries_wdt_driver = {
> > +	.driver = {
> > +		.name = DRV_NAME,
> > +		.owner = THIS_MODULE,
> 
> That owner assignment is not required.
> 
> It's set for you by platform_driver_register() via
> module_platform_driver().

Great, removed.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
@ 2022-07-07 15:53       ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-07-07 15:53 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: nathanl, wvoigt, linux-watchdog, aik, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

On Fri, Jun 24, 2022 at 11:51:01PM +1000, Michael Ellerman wrote:
> Scott Cheloha <cheloha@linux.ibm.com> writes:
> ...
> > +
> > +static struct platform_driver pseries_wdt_driver = {
> > +	.driver = {
> > +		.name = DRV_NAME,
> > +		.owner = THIS_MODULE,
> 
> That owner assignment is not required.
> 
> It's set for you by platform_driver_register() via
> module_platform_driver().

Great, removed.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 3/4] powerpc/pseries: register pseries-wdt device with platform bus
  2022-06-24 13:27     ` Michael Ellerman
@ 2022-07-07 15:53         ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-07-07 15:53 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nathan Lynch, linux-watchdog, wvoigt, aik, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

On Fri, Jun 24, 2022 at 11:27:36PM +1000, Michael Ellerman wrote:
> Nathan Lynch <nathanl@linux.ibm.com> writes:
> > Scott Cheloha <cheloha@linux.ibm.com> writes:
> >> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> >> guest control of one or more virtual watchdog timers.
> ...
> >
> > Seems like we don't need pseries_wdt_pdev as it's unused elsewhere? But
> > that's quite minor.
> 
> It's minor but please drop it in the next version.

Dropped.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 3/4] powerpc/pseries: register pseries-wdt device with platform bus
@ 2022-07-07 15:53         ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-07-07 15:53 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nathan Lynch, wvoigt, linux-watchdog, aik, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

On Fri, Jun 24, 2022 at 11:27:36PM +1000, Michael Ellerman wrote:
> Nathan Lynch <nathanl@linux.ibm.com> writes:
> > Scott Cheloha <cheloha@linux.ibm.com> writes:
> >> PAPR v2.12 defines a new hypercall, H_WATCHDOG.  The hypercall permits
> >> guest control of one or more virtual watchdog timers.
> ...
> >
> > Seems like we don't need pseries_wdt_pdev as it's unused elsewhere? But
> > that's quite minor.
> 
> It's minor but please drop it in the next version.

Dropped.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
  2022-06-24 13:27   ` Michael Ellerman
@ 2022-07-08  5:51       ` Scott Cheloha
  2022-07-08  5:51       ` Scott Cheloha
  1 sibling, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-07-08  5:51 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: linux-watchdog, nathanl, wvoigt, aik, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

On Fri, Jun 24, 2022 at 11:27:24PM +1000, Michael Ellerman wrote:
> Hi Scott,
> 
> A few comments below ...
> 
> Scott Cheloha <cheloha@linux.ibm.com> writes:
> > 
> > [...]
> > 
> > diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
> > index 223c99361a30..29153eed6689 100644
> > --- a/Documentation/watchdog/watchdog-parameters.rst
> > +++ b/Documentation/watchdog/watchdog-parameters.rst
> > @@ -425,6 +425,18 @@ pnx833x_wdt:
> >  
> >  -------------------------------------------------
> >  
> > +pseries-wdt:
> > +    action:
> > +	Action taken when watchdog expires: 0 (power off), 1 (restart),
> > +	2 (dump and restart). (default=1)
> 
> I doesn't look like these values match what other drivers use to any
> great extent.
> 
> So why not use the values from PAPR directly?
> 
> ie. 1 = power off, 2 = hard reset, 3 = dump & restart.
> 
> It seems like it would be easier to follow if the values map directly.
> 
> It's possible in future PAPR adds 247 to mean something, in which case
> maybe we'd want to map that to a less silly value, but at least for now
> the PAPR values are sensible enough.

I tried using 1-2-3 in Patch v1 but Guenter objected and we switched:

https://lore.kernel.org/linux-watchdog/a6090ef3-f597-e10b-010b-cc32bff08c93@roeck-us.net/

I think the code is fine to read as-is.  We're not expecting the
administrator to read the PAPR, right?  So 1-2-3 is not any more
intuitive for the user than 0-1-2.

Given that it's all arbitrary and there aren't any hard rules for
module parameters outside of general programmer "that seems
fine"-ness, I would really like to leave the numbers as-is.

> > +    timeout:
> > +	Initial watchdog timeout in seconds. (default=60)
> 
> That seems like a pretty common value, I don't see any guidance in PAPR.
> Do we have any input from PowerVM on whether that's a good value?

Currently the minimum timeout is 500ms on all the builds I've tried.
I doubt the minimum will ever be anywhere near as large as 60s on a
practical H_WATCHDOG implementation, so I don't think there is any
risk of the driver failing to probe.

Real software using the watchdog API will set a timeout to a smaller
value if it needs to.

60 seconds gives userland ample time to reconfigure the watchdog
without risk of it expiring in the midst of a bunch of ioctl(2) calls
before they reach the main loop.

> > diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
> > new file mode 100644
> > index 000000000000..cfe53587457d
> > --- /dev/null
> > +++ b/drivers/watchdog/pseries-wdt.c
> > @@ -0,0 +1,264 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022 International Business Machines, Inc.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/kernel.h>
> > +#include <linux/limits.h>
> > +#include <linux/math.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define DRV_NAME "pseries-wdt"
> > +
> > +/*
> > + * The PAPR's MSB->LSB bit ordering is 0->63.  These macros simplify
> > + * defining bitfields as described in the PAPR without needing to
> > + * transpose values to the more C-like 63->0 ordering.
> > + */
> > +#define SETFIELD(_v, _b, _e)	\
> > +	(((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
> > +#define GETFIELD(_v, _b, _e)	\
> > +	(((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
> 
> This will probably sound like a cranky maintainer rant, but ...,
> I really dislike these GETFIELD/SETFIELD macros.
> 
> I know you didn't invent them, but I would be much happier if you didn't
> use them.
> 
> I know they (slightly) simplify things when you're transcribing values
> from PAPR into the source, but that happens only once.
> 
> And then for the rest of eternity the source is harder to read because
> there's this ridiculous level of indirection through insane macros just
> to define some constants.
> 
> Anyone trying to use a debugger against this code will see a value in
> memory like 0x200 and have to sit down and work out which SETFIELD()
> macro it corresponds to.

Don't look at me, I never would have come up with them.  I got them
from Alexey :)

I will drop them.

> > +/*
> > + * The H_WATCHDOG hypercall first appears in PAPR v2.12 and is
> > + * described fully in sections 14.5 and 14.15.6.
> > + *
> > + *
> > + * H_WATCHDOG Input
> > + *
> > + * R4: "flags":
> > + *
> > + *         Bits 48-55: "operation"
> > + *
> > + *             0x01  Start Watchdog
> > + *             0x02  Stop Watchdog
> > + *             0x03  Query Watchdog Capabilities
> > + */
> > +#define PSERIES_WDTF_OP(op)		SETFIELD((op), 48, 55)
> > +#define PSERIES_WDTF_OP_START		PSERIES_WDTF_OP(0x1)
> > +#define PSERIES_WDTF_OP_STOP		PSERIES_WDTF_OP(0x2)
> > +#define PSERIES_WDTF_OP_QUERY		PSERIES_WDTF_OP(0x3)
>  
> eg, IMHO these are much more reader friendly:
> 
> #define PSERIES_WDTF_OP_START		(1 << 8)
> #define PSERIES_WDTF_OP_STOP		(2 << 8)
> #define PSERIES_WDTF_OP_QUERY		(3 << 8)
> 
> > +/*
> > + *         Bits 56-63: "timeoutAction" (for "Start Watchdog" only)
> > + *
> > + *             0x01  Hard poweroff
> > + *             0x02  Hard restart
> > + *             0x03  Dump restart
> > + */
> > +#define PSERIES_WDTF_ACTION(ac)			SETFIELD(ac, 56, 63)
> > +#define PSERIES_WDTF_ACTION_HARD_POWEROFF	PSERIES_WDTF_ACTION(0x1)
> > +#define PSERIES_WDTF_ACTION_HARD_RESTART	PSERIES_WDTF_ACTION(0x2)
> > +#define PSERIES_WDTF_ACTION_DUMP_RESTART	PSERIES_WDTF_ACTION(0x3)
> 
> These are a slam dunk:
> 
> #define PSERIES_WDTF_ACTION_HARD_POWEROFF	1
> #define PSERIES_WDTF_ACTION_HARD_RESTART	2
> #define PSERIES_WDTF_ACTION_DUMP_RESTART	3

Yes, yes they are.

> > +
> > +/*
> > + * H_WATCHDOG Output
> > + *
> > + * R3: Return code
> > + *
> > + *     H_SUCCESS    The operation completed.
> > + *
> > + *     H_BUSY	    The hypervisor is too busy; retry the operation.
> > + *
> > + *     H_PARAMETER  The given "flags" are somehow invalid.  Either the
> > + *                  "operation" or "timeoutAction" is invalid, or a
> > + *                  reserved bit is set.
> > + *
> > + *     H_P2         The given "watchdogNumber" is zero or exceeds the
> > + *                  supported maximum value.
> > + *
> > + *     H_P3         The given "timeoutInMs" is below the supported
> > + *                  minimum value.
> > + *
> > + *     H_NOOP       The given "watchdogNumber" is already stopped.
> > + *
> > + *     H_HARDWARE   The operation failed for ineffable reasons.
> > + *
> > + *     H_FUNCTION   The H_WATCHDOG hypercall is not supported by this
> > + *                  hypervisor.
> > + *
> > + * R4:
> > + *
> > + * - For the "Query Watchdog Capabilities" operation, a 64-bit
> > + *   value structured as follows:
> > + *
> > + *       Bits  0-15: The minimum supported timeout in milliseconds.
> > + *       Bits 16-31: The number of watchdogs supported.
> > + *       Bits 32-63: Reserved.
> > + */
> > +#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	GETFIELD((cap), 0, 15)
> 
> This one is less obviously better, but I still think it's clearer as all
> the logic is there in front of you, rather than hidden in the macro. It
> is clearer that we're only returning a 16-bit value.
> 
> #define PSERIES_WDTQ_MIN_TIMEOUT(cap)	(((cap) >> 48) & 0xffff)
> 
> > +#define PSERIES_WDTQ_MAX_NUMBER(cap)	GETFIELD((cap), 16, 31)
> 
> That's unused.
> 
> I guess we're assuming at least one timer is always supported? Seems
> reasonable.

There is a distinction between "we have support for this hypercall"
and "you have a timer available to you".  We should double-check.

I can't imagine it ever being an issue on a practical, working
implementation, but it might save us some debugging if there is ever a
hypervisor bug where somehow they allocate us zero timers to work
with.

> > +
> > +static const unsigned long pseries_wdt_action[] = {
> > +	[0] = PSERIES_WDTF_ACTION_HARD_POWEROFF,
> > +	[1] = PSERIES_WDTF_ACTION_HARD_RESTART,
> > +	[2] = PSERIES_WDTF_ACTION_DUMP_RESTART,
> > +};
> 
> If we used the PAPR values we wouldn't need that ^
> 
> > +#define WATCHDOG_ACTION 1
> 
> DEFAULT_ACTION ?

The idiom for the default timeout is "WATCHDOG_TIMEOUT" so I went with
"WATCHDOG_ACTION".

> > +static unsigned int action = WATCHDOG_ACTION;
> > +module_param(action, uint, 0444);
> > +MODULE_PARM_DESC(action, "Action taken when watchdog expires (default="
> > +		 __MODULE_STRING(WATCHDOG_ACTION) ")");
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> > +module_param(nowayout, bool, 0444);
> > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> > +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +#define WATCHDOG_TIMEOUT 60
> 
> DEFAULT_TIMEOUT ?

"WATCHDOG_TIMEOUT" is the idiomatic name for the default timeout in
drivers/watchdog/.

> > +static unsigned int timeout = WATCHDOG_TIMEOUT;
> > +module_param(timeout, uint, 0444);
> > +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default="
> > +		 __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
> > +
> > +struct pseries_wdt {
> > +	struct watchdog_device wd;
> > +	unsigned long action;
> > +	unsigned long num;		/* Watchdog numbers are 1-based */
> 
> num can just be an int.

It's an argument to the hypercall, which takes an unsigned long.  Do
we need to save 4 bytes?

I guess if we wanted to be precise it should be a 16-bit value.

> But do we even need it, do we anticipate supporting multiple timers?
> Should we just hard code '1' ?

We have not had a serious discussion about whether more timers in
userspace make sense.  This code let's us experiment with it, though.

> > +};
> > +
> > +static int pseries_wdt_start(struct watchdog_device *wdd)
> > +{
> > +	struct device *dev = wdd->parent;
> > +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> > +	unsigned long flags, msecs;
> > +	long rc;
> > +
> > +	flags = pw->action | PSERIES_WDTF_OP_START;
> 
> We set pw->action at probe time based on the module param action, but
> this is the only place we use it.
> 
> If we use the PAPR values, this could just be:
> 
>       flags = (pw->action << 8) | PSERIES_WDTF_OP_START;
> 
> And is there any benefit in storing action in pseries_wdt, we could just
> use the module param value here.

That was Guenter's idea and I went with it.

> > +	msecs = wdd->timeout * 1000UL;
>  
> Using MSEC_PER_SEC makes it clearer what that conversion is doing.

Done.

> > +	rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
> > +	if (rc != H_SUCCESS) {
> > +		dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
> > +			 rc, pw->num);
> > +		return -EIO;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int pseries_wdt_stop(struct watchdog_device *wdd)
> > +{
> > +	struct device *dev = wdd->parent;
> > +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> > +	long rc;
> > +
> > +	rc = plpar_hcall_norets(H_WATCHDOG, PSERIES_WDTF_OP_STOP, pw->num);
> > +	if (rc != H_SUCCESS && rc != H_NOOP) {
> > +		dev_crit(dev, "H_WATCHDOG: %ld: failed to stop timer %lu",
> > +			 rc, pw->num);
> > +		return -EIO;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static struct watchdog_info pseries_wdt_info = {
> > +	.identity = DRV_NAME,
> > +	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT
> > +	    | WDIOF_PRETIMEOUT,
> 
> I don't know the watchdog code to know if those make sense.

It makes sense.

> > +};
> > +
> > +static const struct watchdog_ops pseries_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = pseries_wdt_start,
> > +	.stop = pseries_wdt_stop,
> > +};
> > +
> > +static int pseries_wdt_probe(struct platform_device *pdev)
> > +{
> > +	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
> > +	unsigned long cap;
> > +	long rc;
> > +	struct pseries_wdt *pw;
> > +	int err;
> 
> Try to use reverse xmas tree for new code please.

Is it not good practice to keep declarations of a particular type
adjacent?

It feels... correct-ish to keep the longs together.

In this case there is no downside to doing "reverse xmas tree"
because sizeof(long) is the same as sizeof(void *), but this looks
odd to me:

	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
	struct pseries_wdt *pw;
	unsigned long cap;
	long rc;
	int err;

> > +	rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
> > +	if (rc == H_FUNCTION)
> > +		return -ENODEV;
> > +	if (rc != H_SUCCESS)
> > +		return -EIO;
> > +	cap = ret[0];
> > +
> > +	pw = devm_kzalloc(&pdev->dev, sizeof(*pw), GFP_KERNEL);
> > +	if (!pw)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * Assume watchdogNumber 1 for now.  If we ever support
> > +	 * multiple timers we will need to devise a way to choose a
> > +	 * distinct watchdogNumber for each platform device at device
> > +	 * registration time.
> > +	 */
> > +	pw->num = 1;
> > +
> > +	if (action >= ARRAY_SIZE(pseries_wdt_action))
> > +		return -EINVAL;
> > +	pw->action = pseries_wdt_action[action];
> > +
> > +	pw->wd.parent = &pdev->dev;
> > +	pw->wd.info = &pseries_wdt_info;
> > +	pw->wd.ops = &pseries_wdt_ops;
> > +	pw->wd.min_timeout = DIV_ROUND_UP(PSERIES_WDTQ_MIN_TIMEOUT(cap), 1000);
> 
> MSEC_TO_SEC again?

MSEC_PER_SEC, sure.

> > +	pw->wd.max_timeout = UINT_MAX / 1000;
>  
> Where does that value come from?

It's just the maximum value the watchdog framework will accept.  It's
in one of the watchdog headers.

> > +	pw->wd.timeout = timeout;
> > +	if (watchdog_init_timeout(&pw->wd, 0, NULL))
> > +		return -EINVAL;
> 
> It's late so maybe I'm misreading it, but does watchdog_init_timeout()
> actually clamp the values if we don't pass a timeout?
> 
> It looks like basically a nop when we pass timeout_param=0 and dev=NULL.
> 
> Which makes me think we aren't checking anywhere that the timeout we are
> using >= what firmware will accept.

No, watchdog_init_timeout() checks that

	min_timeout <= timeout <= max_timeout

and returns an error if not.  If somehow the minimum timeout exceeds
the default 60 seconds we will catch it here and fail the probe.

> > +	watchdog_set_nowayout(&pw->wd, nowayout);
> > +	watchdog_stop_on_reboot(&pw->wd);
> > +	watchdog_stop_on_unregister(&pw->wd);
> > +	watchdog_set_drvdata(&pw->wd, pw);
> > +
> > +	err = devm_watchdog_register_device(&pdev->dev, &pw->wd);
> > +	if (err)
> > +		return err;
> > +
> > +	platform_set_drvdata(pdev, &pw->wd);
> > +
> > +	return 0;
> > +}
> > +
> > +static int pseries_wdt_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> > +
> > +	if (watchdog_active(wd))
> > +		return pseries_wdt_stop(wd);
> > +	return 0;
> > +}
> > +
> > +static int pseries_wdt_resume(struct platform_device *pdev)
> > +{
> > +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> > +
> > +	if (watchdog_active(wd))
> > +		return pseries_wdt_start(wd);
> > +	return 0;
> > +}
> > +
> > +static const struct platform_device_id pseries_wdt_id[] = {
> > +	{ .name = "pseries-wdt" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(platform, pseries_wdt_id);
> > +
> > +static struct platform_driver pseries_wdt_driver = {
> > +	.driver = {
> > +		.name = DRV_NAME,
> > +		.owner = THIS_MODULE,
> > +	},
> > +	.id_table = pseries_wdt_id,
> > +	.probe = pseries_wdt_probe,
> > +	.resume = pseries_wdt_resume,
> > +	.suspend = pseries_wdt_suspend,
> 
> I don't see any handling of the possible requirement to suspend timers
> across LPM. I don't think just wiring these up is enough?

I talked to Brian King about this and we decided that leaving the
watchdog running across an LPM might lead to some potentially
confusing behavior.

For example, if the watchdog expires while we're suspeded and the
machine is hard reset the instant we come out of it on the other side.

Unless there is an ask by downstream software to actually leave the
timer running over an LPM I think it is safest to err on the side of
caution and unconditionally stop running timers before suspend.

> > +};
> > +module_platform_driver(pseries_wdt_driver);
> > +
> > +MODULE_AUTHOR("Alexey Kardashevskiy <aik@ozlabs.ru>");
> > +MODULE_AUTHOR("Scott Cheloha <cheloha@linux.ibm.com>");
> 
> I'd prefer the module authors were just the names, email addresses
> inevitably bitrot.
> 
> Your email address is in the change log.

Dropped.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
@ 2022-07-08  5:51       ` Scott Cheloha
  0 siblings, 0 replies; 39+ messages in thread
From: Scott Cheloha @ 2022-07-08  5:51 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: nathanl, wvoigt, linux-watchdog, aik, vaishnavi, npiggin,
	tzungbi, brking, linuxppc-dev, linux

On Fri, Jun 24, 2022 at 11:27:24PM +1000, Michael Ellerman wrote:
> Hi Scott,
> 
> A few comments below ...
> 
> Scott Cheloha <cheloha@linux.ibm.com> writes:
> > 
> > [...]
> > 
> > diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
> > index 223c99361a30..29153eed6689 100644
> > --- a/Documentation/watchdog/watchdog-parameters.rst
> > +++ b/Documentation/watchdog/watchdog-parameters.rst
> > @@ -425,6 +425,18 @@ pnx833x_wdt:
> >  
> >  -------------------------------------------------
> >  
> > +pseries-wdt:
> > +    action:
> > +	Action taken when watchdog expires: 0 (power off), 1 (restart),
> > +	2 (dump and restart). (default=1)
> 
> I doesn't look like these values match what other drivers use to any
> great extent.
> 
> So why not use the values from PAPR directly?
> 
> ie. 1 = power off, 2 = hard reset, 3 = dump & restart.
> 
> It seems like it would be easier to follow if the values map directly.
> 
> It's possible in future PAPR adds 247 to mean something, in which case
> maybe we'd want to map that to a less silly value, but at least for now
> the PAPR values are sensible enough.

I tried using 1-2-3 in Patch v1 but Guenter objected and we switched:

https://lore.kernel.org/linux-watchdog/a6090ef3-f597-e10b-010b-cc32bff08c93@roeck-us.net/

I think the code is fine to read as-is.  We're not expecting the
administrator to read the PAPR, right?  So 1-2-3 is not any more
intuitive for the user than 0-1-2.

Given that it's all arbitrary and there aren't any hard rules for
module parameters outside of general programmer "that seems
fine"-ness, I would really like to leave the numbers as-is.

> > +    timeout:
> > +	Initial watchdog timeout in seconds. (default=60)
> 
> That seems like a pretty common value, I don't see any guidance in PAPR.
> Do we have any input from PowerVM on whether that's a good value?

Currently the minimum timeout is 500ms on all the builds I've tried.
I doubt the minimum will ever be anywhere near as large as 60s on a
practical H_WATCHDOG implementation, so I don't think there is any
risk of the driver failing to probe.

Real software using the watchdog API will set a timeout to a smaller
value if it needs to.

60 seconds gives userland ample time to reconfigure the watchdog
without risk of it expiring in the midst of a bunch of ioctl(2) calls
before they reach the main loop.

> > diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
> > new file mode 100644
> > index 000000000000..cfe53587457d
> > --- /dev/null
> > +++ b/drivers/watchdog/pseries-wdt.c
> > @@ -0,0 +1,264 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022 International Business Machines, Inc.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/kernel.h>
> > +#include <linux/limits.h>
> > +#include <linux/math.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define DRV_NAME "pseries-wdt"
> > +
> > +/*
> > + * The PAPR's MSB->LSB bit ordering is 0->63.  These macros simplify
> > + * defining bitfields as described in the PAPR without needing to
> > + * transpose values to the more C-like 63->0 ordering.
> > + */
> > +#define SETFIELD(_v, _b, _e)	\
> > +	(((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
> > +#define GETFIELD(_v, _b, _e)	\
> > +	(((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
> 
> This will probably sound like a cranky maintainer rant, but ...,
> I really dislike these GETFIELD/SETFIELD macros.
> 
> I know you didn't invent them, but I would be much happier if you didn't
> use them.
> 
> I know they (slightly) simplify things when you're transcribing values
> from PAPR into the source, but that happens only once.
> 
> And then for the rest of eternity the source is harder to read because
> there's this ridiculous level of indirection through insane macros just
> to define some constants.
> 
> Anyone trying to use a debugger against this code will see a value in
> memory like 0x200 and have to sit down and work out which SETFIELD()
> macro it corresponds to.

Don't look at me, I never would have come up with them.  I got them
from Alexey :)

I will drop them.

> > +/*
> > + * The H_WATCHDOG hypercall first appears in PAPR v2.12 and is
> > + * described fully in sections 14.5 and 14.15.6.
> > + *
> > + *
> > + * H_WATCHDOG Input
> > + *
> > + * R4: "flags":
> > + *
> > + *         Bits 48-55: "operation"
> > + *
> > + *             0x01  Start Watchdog
> > + *             0x02  Stop Watchdog
> > + *             0x03  Query Watchdog Capabilities
> > + */
> > +#define PSERIES_WDTF_OP(op)		SETFIELD((op), 48, 55)
> > +#define PSERIES_WDTF_OP_START		PSERIES_WDTF_OP(0x1)
> > +#define PSERIES_WDTF_OP_STOP		PSERIES_WDTF_OP(0x2)
> > +#define PSERIES_WDTF_OP_QUERY		PSERIES_WDTF_OP(0x3)
>  
> eg, IMHO these are much more reader friendly:
> 
> #define PSERIES_WDTF_OP_START		(1 << 8)
> #define PSERIES_WDTF_OP_STOP		(2 << 8)
> #define PSERIES_WDTF_OP_QUERY		(3 << 8)
> 
> > +/*
> > + *         Bits 56-63: "timeoutAction" (for "Start Watchdog" only)
> > + *
> > + *             0x01  Hard poweroff
> > + *             0x02  Hard restart
> > + *             0x03  Dump restart
> > + */
> > +#define PSERIES_WDTF_ACTION(ac)			SETFIELD(ac, 56, 63)
> > +#define PSERIES_WDTF_ACTION_HARD_POWEROFF	PSERIES_WDTF_ACTION(0x1)
> > +#define PSERIES_WDTF_ACTION_HARD_RESTART	PSERIES_WDTF_ACTION(0x2)
> > +#define PSERIES_WDTF_ACTION_DUMP_RESTART	PSERIES_WDTF_ACTION(0x3)
> 
> These are a slam dunk:
> 
> #define PSERIES_WDTF_ACTION_HARD_POWEROFF	1
> #define PSERIES_WDTF_ACTION_HARD_RESTART	2
> #define PSERIES_WDTF_ACTION_DUMP_RESTART	3

Yes, yes they are.

> > +
> > +/*
> > + * H_WATCHDOG Output
> > + *
> > + * R3: Return code
> > + *
> > + *     H_SUCCESS    The operation completed.
> > + *
> > + *     H_BUSY	    The hypervisor is too busy; retry the operation.
> > + *
> > + *     H_PARAMETER  The given "flags" are somehow invalid.  Either the
> > + *                  "operation" or "timeoutAction" is invalid, or a
> > + *                  reserved bit is set.
> > + *
> > + *     H_P2         The given "watchdogNumber" is zero or exceeds the
> > + *                  supported maximum value.
> > + *
> > + *     H_P3         The given "timeoutInMs" is below the supported
> > + *                  minimum value.
> > + *
> > + *     H_NOOP       The given "watchdogNumber" is already stopped.
> > + *
> > + *     H_HARDWARE   The operation failed for ineffable reasons.
> > + *
> > + *     H_FUNCTION   The H_WATCHDOG hypercall is not supported by this
> > + *                  hypervisor.
> > + *
> > + * R4:
> > + *
> > + * - For the "Query Watchdog Capabilities" operation, a 64-bit
> > + *   value structured as follows:
> > + *
> > + *       Bits  0-15: The minimum supported timeout in milliseconds.
> > + *       Bits 16-31: The number of watchdogs supported.
> > + *       Bits 32-63: Reserved.
> > + */
> > +#define PSERIES_WDTQ_MIN_TIMEOUT(cap)	GETFIELD((cap), 0, 15)
> 
> This one is less obviously better, but I still think it's clearer as all
> the logic is there in front of you, rather than hidden in the macro. It
> is clearer that we're only returning a 16-bit value.
> 
> #define PSERIES_WDTQ_MIN_TIMEOUT(cap)	(((cap) >> 48) & 0xffff)
> 
> > +#define PSERIES_WDTQ_MAX_NUMBER(cap)	GETFIELD((cap), 16, 31)
> 
> That's unused.
> 
> I guess we're assuming at least one timer is always supported? Seems
> reasonable.

There is a distinction between "we have support for this hypercall"
and "you have a timer available to you".  We should double-check.

I can't imagine it ever being an issue on a practical, working
implementation, but it might save us some debugging if there is ever a
hypervisor bug where somehow they allocate us zero timers to work
with.

> > +
> > +static const unsigned long pseries_wdt_action[] = {
> > +	[0] = PSERIES_WDTF_ACTION_HARD_POWEROFF,
> > +	[1] = PSERIES_WDTF_ACTION_HARD_RESTART,
> > +	[2] = PSERIES_WDTF_ACTION_DUMP_RESTART,
> > +};
> 
> If we used the PAPR values we wouldn't need that ^
> 
> > +#define WATCHDOG_ACTION 1
> 
> DEFAULT_ACTION ?

The idiom for the default timeout is "WATCHDOG_TIMEOUT" so I went with
"WATCHDOG_ACTION".

> > +static unsigned int action = WATCHDOG_ACTION;
> > +module_param(action, uint, 0444);
> > +MODULE_PARM_DESC(action, "Action taken when watchdog expires (default="
> > +		 __MODULE_STRING(WATCHDOG_ACTION) ")");
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> > +module_param(nowayout, bool, 0444);
> > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> > +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +#define WATCHDOG_TIMEOUT 60
> 
> DEFAULT_TIMEOUT ?

"WATCHDOG_TIMEOUT" is the idiomatic name for the default timeout in
drivers/watchdog/.

> > +static unsigned int timeout = WATCHDOG_TIMEOUT;
> > +module_param(timeout, uint, 0444);
> > +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default="
> > +		 __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
> > +
> > +struct pseries_wdt {
> > +	struct watchdog_device wd;
> > +	unsigned long action;
> > +	unsigned long num;		/* Watchdog numbers are 1-based */
> 
> num can just be an int.

It's an argument to the hypercall, which takes an unsigned long.  Do
we need to save 4 bytes?

I guess if we wanted to be precise it should be a 16-bit value.

> But do we even need it, do we anticipate supporting multiple timers?
> Should we just hard code '1' ?

We have not had a serious discussion about whether more timers in
userspace make sense.  This code let's us experiment with it, though.

> > +};
> > +
> > +static int pseries_wdt_start(struct watchdog_device *wdd)
> > +{
> > +	struct device *dev = wdd->parent;
> > +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> > +	unsigned long flags, msecs;
> > +	long rc;
> > +
> > +	flags = pw->action | PSERIES_WDTF_OP_START;
> 
> We set pw->action at probe time based on the module param action, but
> this is the only place we use it.
> 
> If we use the PAPR values, this could just be:
> 
>       flags = (pw->action << 8) | PSERIES_WDTF_OP_START;
> 
> And is there any benefit in storing action in pseries_wdt, we could just
> use the module param value here.

That was Guenter's idea and I went with it.

> > +	msecs = wdd->timeout * 1000UL;
>  
> Using MSEC_PER_SEC makes it clearer what that conversion is doing.

Done.

> > +	rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
> > +	if (rc != H_SUCCESS) {
> > +		dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
> > +			 rc, pw->num);
> > +		return -EIO;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int pseries_wdt_stop(struct watchdog_device *wdd)
> > +{
> > +	struct device *dev = wdd->parent;
> > +	struct pseries_wdt *pw = watchdog_get_drvdata(wdd);
> > +	long rc;
> > +
> > +	rc = plpar_hcall_norets(H_WATCHDOG, PSERIES_WDTF_OP_STOP, pw->num);
> > +	if (rc != H_SUCCESS && rc != H_NOOP) {
> > +		dev_crit(dev, "H_WATCHDOG: %ld: failed to stop timer %lu",
> > +			 rc, pw->num);
> > +		return -EIO;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static struct watchdog_info pseries_wdt_info = {
> > +	.identity = DRV_NAME,
> > +	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT
> > +	    | WDIOF_PRETIMEOUT,
> 
> I don't know the watchdog code to know if those make sense.

It makes sense.

> > +};
> > +
> > +static const struct watchdog_ops pseries_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = pseries_wdt_start,
> > +	.stop = pseries_wdt_stop,
> > +};
> > +
> > +static int pseries_wdt_probe(struct platform_device *pdev)
> > +{
> > +	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
> > +	unsigned long cap;
> > +	long rc;
> > +	struct pseries_wdt *pw;
> > +	int err;
> 
> Try to use reverse xmas tree for new code please.

Is it not good practice to keep declarations of a particular type
adjacent?

It feels... correct-ish to keep the longs together.

In this case there is no downside to doing "reverse xmas tree"
because sizeof(long) is the same as sizeof(void *), but this looks
odd to me:

	unsigned long ret[PLPAR_HCALL_BUFSIZE] = { 0 };
	struct pseries_wdt *pw;
	unsigned long cap;
	long rc;
	int err;

> > +	rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
> > +	if (rc == H_FUNCTION)
> > +		return -ENODEV;
> > +	if (rc != H_SUCCESS)
> > +		return -EIO;
> > +	cap = ret[0];
> > +
> > +	pw = devm_kzalloc(&pdev->dev, sizeof(*pw), GFP_KERNEL);
> > +	if (!pw)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * Assume watchdogNumber 1 for now.  If we ever support
> > +	 * multiple timers we will need to devise a way to choose a
> > +	 * distinct watchdogNumber for each platform device at device
> > +	 * registration time.
> > +	 */
> > +	pw->num = 1;
> > +
> > +	if (action >= ARRAY_SIZE(pseries_wdt_action))
> > +		return -EINVAL;
> > +	pw->action = pseries_wdt_action[action];
> > +
> > +	pw->wd.parent = &pdev->dev;
> > +	pw->wd.info = &pseries_wdt_info;
> > +	pw->wd.ops = &pseries_wdt_ops;
> > +	pw->wd.min_timeout = DIV_ROUND_UP(PSERIES_WDTQ_MIN_TIMEOUT(cap), 1000);
> 
> MSEC_TO_SEC again?

MSEC_PER_SEC, sure.

> > +	pw->wd.max_timeout = UINT_MAX / 1000;
>  
> Where does that value come from?

It's just the maximum value the watchdog framework will accept.  It's
in one of the watchdog headers.

> > +	pw->wd.timeout = timeout;
> > +	if (watchdog_init_timeout(&pw->wd, 0, NULL))
> > +		return -EINVAL;
> 
> It's late so maybe I'm misreading it, but does watchdog_init_timeout()
> actually clamp the values if we don't pass a timeout?
> 
> It looks like basically a nop when we pass timeout_param=0 and dev=NULL.
> 
> Which makes me think we aren't checking anywhere that the timeout we are
> using >= what firmware will accept.

No, watchdog_init_timeout() checks that

	min_timeout <= timeout <= max_timeout

and returns an error if not.  If somehow the minimum timeout exceeds
the default 60 seconds we will catch it here and fail the probe.

> > +	watchdog_set_nowayout(&pw->wd, nowayout);
> > +	watchdog_stop_on_reboot(&pw->wd);
> > +	watchdog_stop_on_unregister(&pw->wd);
> > +	watchdog_set_drvdata(&pw->wd, pw);
> > +
> > +	err = devm_watchdog_register_device(&pdev->dev, &pw->wd);
> > +	if (err)
> > +		return err;
> > +
> > +	platform_set_drvdata(pdev, &pw->wd);
> > +
> > +	return 0;
> > +}
> > +
> > +static int pseries_wdt_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> > +
> > +	if (watchdog_active(wd))
> > +		return pseries_wdt_stop(wd);
> > +	return 0;
> > +}
> > +
> > +static int pseries_wdt_resume(struct platform_device *pdev)
> > +{
> > +	struct watchdog_device *wd = platform_get_drvdata(pdev);
> > +
> > +	if (watchdog_active(wd))
> > +		return pseries_wdt_start(wd);
> > +	return 0;
> > +}
> > +
> > +static const struct platform_device_id pseries_wdt_id[] = {
> > +	{ .name = "pseries-wdt" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(platform, pseries_wdt_id);
> > +
> > +static struct platform_driver pseries_wdt_driver = {
> > +	.driver = {
> > +		.name = DRV_NAME,
> > +		.owner = THIS_MODULE,
> > +	},
> > +	.id_table = pseries_wdt_id,
> > +	.probe = pseries_wdt_probe,
> > +	.resume = pseries_wdt_resume,
> > +	.suspend = pseries_wdt_suspend,
> 
> I don't see any handling of the possible requirement to suspend timers
> across LPM. I don't think just wiring these up is enough?

I talked to Brian King about this and we decided that leaving the
watchdog running across an LPM might lead to some potentially
confusing behavior.

For example, if the watchdog expires while we're suspeded and the
machine is hard reset the instant we come out of it on the other side.

Unless there is an ask by downstream software to actually leave the
timer running over an LPM I think it is safest to err on the side of
caution and unconditionally stop running timers before suspend.

> > +};
> > +module_platform_driver(pseries_wdt_driver);
> > +
> > +MODULE_AUTHOR("Alexey Kardashevskiy <aik@ozlabs.ru>");
> > +MODULE_AUTHOR("Scott Cheloha <cheloha@linux.ibm.com>");
> 
> I'd prefer the module authors were just the names, email addresses
> inevitably bitrot.
> 
> Your email address is in the change log.

Dropped.

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2022-07-08  5:52 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-02 17:53 [PATCH v2 0/4] pseries-wdt: initial support for H_WATCHDOG-based watchdog timers Scott Cheloha
2022-06-02 17:53 ` Scott Cheloha
2022-06-02 17:53 ` [PATCH v2 1/4] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code Scott Cheloha
2022-06-02 17:53   ` Scott Cheloha
2022-06-21 14:44   ` Nathan Lynch
2022-06-21 14:44     ` Nathan Lynch
2022-06-21 17:31     ` Segher Boessenkool
2022-06-21 17:31       ` Segher Boessenkool
2022-06-21 22:22       ` Nathan Lynch
2022-06-21 22:22         ` Nathan Lynch
2022-06-02 17:53 ` [PATCH v2 2/4] powerpc/pseries: add FW_FEATURE_WATCHDOG flag Scott Cheloha
2022-06-02 17:53   ` Scott Cheloha
2022-06-21 15:03   ` Nathan Lynch
2022-06-21 15:03     ` Nathan Lynch
2022-06-02 17:53 ` [PATCH v2 3/4] powerpc/pseries: register pseries-wdt device with platform bus Scott Cheloha
2022-06-02 17:53   ` Scott Cheloha
2022-06-21 15:30   ` Nathan Lynch
2022-06-24 13:27     ` Michael Ellerman
2022-07-07 15:53       ` Scott Cheloha
2022-07-07 15:53         ` Scott Cheloha
2022-06-02 17:53 ` [PATCH v2 4/4] watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers Scott Cheloha
2022-06-02 17:53   ` Scott Cheloha
2022-06-20  6:09   ` Alexey Kardashevskiy
2022-06-20  6:09     ` Alexey Kardashevskiy
2022-06-21 15:45   ` Nathan Lynch
2022-06-21 15:45     ` Nathan Lynch
2022-06-24 13:27   ` Michael Ellerman
2022-06-24 15:31     ` Segher Boessenkool
2022-06-24 15:31       ` Segher Boessenkool
2022-07-08  5:51     ` Scott Cheloha
2022-07-08  5:51       ` Scott Cheloha
2022-06-24 13:51   ` Michael Ellerman
2022-07-07 15:53     ` Scott Cheloha
2022-07-07 15:53       ` Scott Cheloha
2022-06-16  1:43 ` [PATCH v2 0/4] pseries-wdt: " Daniel Henrique Barboza
2022-06-16 16:44   ` Tyrel Datwyler
2022-06-16 18:16     ` Daniel Henrique Barboza
2022-06-16 18:33   ` Daniel Henrique Barboza
2022-06-17 12:54 ` Daniel Henrique Barboza

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.