[PATCH 0/5] diag288 watchdog fixes and improvements

[PATCH 0/5] diag288 watchdog fixes and improvements

[Alexander Egorenkov]

Minor code refactoring to improve readability of the driver,
reduce code duplication and remove dead code.

Alexander Egorenkov (5):
  watchdog: diag288_wdt: get rid of register asm
  watchdog: diag288_wdt: remove power management
  watchdog: diag288_wdt: unify command buffer handling for diag288 zvm
  watchdog: diag288_wdt: de-duplicate diag_stat_inc() calls
  watchdog: diag288_wdt: unify lpar and zvm diag288 helpers

 drivers/watchdog/diag288_wdt.c | 162 ++++++++-------------------------
 1 file changed, 37 insertions(+), 125 deletions(-)

-- 
2.37.2

[PATCH 1/5] watchdog: diag288_wdt: get rid of register asm

[Alexander Egorenkov]

Using register asm statements has been proven to be very error prone,
especially when using code instrumentation where gcc may add function
calls, which clobbers register contents in an unexpected way.

Therefore, get rid of register asm statements in watchdog code, and make
sure this bug class cannot happen.

Moreover, remove the register r1 from the clobber list because this
register is not changed by DIAG 288.

Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
---
 drivers/watchdog/diag288_wdt.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
index 6ca5d9515d85..07ebbb709af4 100644
--- a/drivers/watchdog/diag288_wdt.c
+++ b/drivers/watchdog/diag288_wdt.c
@@ -73,20 +73,19 @@ MODULE_ALIAS("vmwatchdog");
 static int __diag288(unsigned int func, unsigned int timeout,
 		     unsigned long action, unsigned int len)
 {
-	register unsigned long __func asm("2") = func;
-	register unsigned long __timeout asm("3") = timeout;
-	register unsigned long __action asm("4") = action;
-	register unsigned long __len asm("5") = len;
+	union register_pair r1 = { .even = func, .odd = timeout, };
+	union register_pair r3 = { .even = action, .odd = len, };
 	int err;
 
 	err = -EINVAL;
 	asm volatile(
-		"	diag	%1, %3, 0x288\n"
-		"0:	la	%0, 0\n"
+		"	diag	%[r1],%[r3],0x288\n"
+		"0:	la	%[err],0\n"
 		"1:\n"
 		EX_TABLE(0b, 1b)
-		: "+d" (err) : "d"(__func), "d"(__timeout),
-		  "d"(__action), "d"(__len) : "1", "cc", "memory");
+		: [err] "+d" (err)
+		: [r1] "d" (r1.pair), [r3] "d" (r3.pair)
+		: "cc", "memory");
 	return err;
 }
 
-- 
2.37.2

Re: [PATCH 1/5] watchdog: diag288_wdt: get rid of register asm

[Guenter Roeck]

On Fri, Feb 03, 2023 at 08:39:54AM +0100, Alexander Egorenkov wrote:
> Using register asm statements has been proven to be very error prone,
> especially when using code instrumentation where gcc may add function
> calls, which clobbers register contents in an unexpected way.
> 
> Therefore, get rid of register asm statements in watchdog code, and make
> sure this bug class cannot happen.
> 
> Moreover, remove the register r1 from the clobber list because this
> register is not changed by DIAG 288.
> 
> Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>

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

> ---
>  drivers/watchdog/diag288_wdt.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
> index 6ca5d9515d85..07ebbb709af4 100644
> --- a/drivers/watchdog/diag288_wdt.c
> +++ b/drivers/watchdog/diag288_wdt.c
> @@ -73,20 +73,19 @@ MODULE_ALIAS("vmwatchdog");
>  static int __diag288(unsigned int func, unsigned int timeout,
>  		     unsigned long action, unsigned int len)
>  {
> -	register unsigned long __func asm("2") = func;
> -	register unsigned long __timeout asm("3") = timeout;
> -	register unsigned long __action asm("4") = action;
> -	register unsigned long __len asm("5") = len;
> +	union register_pair r1 = { .even = func, .odd = timeout, };
> +	union register_pair r3 = { .even = action, .odd = len, };
>  	int err;
>  
>  	err = -EINVAL;
>  	asm volatile(
> -		"	diag	%1, %3, 0x288\n"
> -		"0:	la	%0, 0\n"
> +		"	diag	%[r1],%[r3],0x288\n"
> +		"0:	la	%[err],0\n"
>  		"1:\n"
>  		EX_TABLE(0b, 1b)
> -		: "+d" (err) : "d"(__func), "d"(__timeout),
> -		  "d"(__action), "d"(__len) : "1", "cc", "memory");
> +		: [err] "+d" (err)
> +		: [r1] "d" (r1.pair), [r3] "d" (r3.pair)
> +		: "cc", "memory");
>  	return err;
>  }
>  

[PATCH 2/5] watchdog: diag288_wdt: remove power management

[Alexander Egorenkov]

Remove power management because s390 no longer supports hibernation since
commit 394216275c7d ("s390: remove broken hibernate / power management
support").

Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
---
 drivers/watchdog/diag288_wdt.c | 65 ++--------------------------------
 1 file changed, 2 insertions(+), 63 deletions(-)

diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
index 07ebbb709af4..c8d516ced6d2 100644
--- a/drivers/watchdog/diag288_wdt.c
+++ b/drivers/watchdog/diag288_wdt.c
@@ -27,7 +27,6 @@
 #include <linux/moduleparam.h>
 #include <linux/slab.h>
 #include <linux/watchdog.h>
-#include <linux/suspend.h>
 #include <asm/ebcdic.h>
 #include <asm/diag.h>
 #include <linux/io.h>
@@ -103,10 +102,6 @@ static int __diag288_lpar(unsigned int func, unsigned int timeout,
 	return __diag288(func, timeout, action, 0);
 }
 
-static unsigned long wdt_status;
-
-#define DIAG_WDOG_BUSY	0
-
 static int wdt_start(struct watchdog_device *dev)
 {
 	char *ebc_cmd;
@@ -114,15 +109,10 @@ static int wdt_start(struct watchdog_device *dev)
 	int ret;
 	unsigned int func;
 
-	if (test_and_set_bit(DIAG_WDOG_BUSY, &wdt_status))
-		return -EBUSY;
-
 	if (MACHINE_IS_VM) {
 		ebc_cmd = kmalloc(MAX_CMDLEN, GFP_KERNEL);
-		if (!ebc_cmd) {
-			clear_bit(DIAG_WDOG_BUSY, &wdt_status);
+		if (!ebc_cmd)
 			return -ENOMEM;
-		}
 		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
 		ASCEBC(ebc_cmd, MAX_CMDLEN);
 		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
@@ -139,7 +129,6 @@ static int wdt_start(struct watchdog_device *dev)
 
 	if (ret) {
 		pr_err("The watchdog cannot be activated\n");
-		clear_bit(DIAG_WDOG_BUSY, &wdt_status);
 		return ret;
 	}
 	return 0;
@@ -152,8 +141,6 @@ static int wdt_stop(struct watchdog_device *dev)
 	diag_stat_inc(DIAG_STAT_X288);
 	ret = __diag288(WDT_FUNC_CANCEL, 0, 0, 0);
 
-	clear_bit(DIAG_WDOG_BUSY, &wdt_status);
-
 	return ret;
 }
 
@@ -222,45 +209,6 @@ static struct watchdog_device wdt_dev = {
 	.max_timeout = MAX_INTERVAL,
 };
 
-/*
- * It makes no sense to go into suspend while the watchdog is running.
- * Depending on the memory size, the watchdog might trigger, while we
- * are still saving the memory.
- */
-static int wdt_suspend(void)
-{
-	if (test_and_set_bit(DIAG_WDOG_BUSY, &wdt_status)) {
-		pr_err("Linux cannot be suspended while the watchdog is in use\n");
-		return notifier_from_errno(-EBUSY);
-	}
-	return NOTIFY_DONE;
-}
-
-static int wdt_resume(void)
-{
-	clear_bit(DIAG_WDOG_BUSY, &wdt_status);
-	return NOTIFY_DONE;
-}
-
-static int wdt_power_event(struct notifier_block *this, unsigned long event,
-			   void *ptr)
-{
-	switch (event) {
-	case PM_POST_HIBERNATION:
-	case PM_POST_SUSPEND:
-		return wdt_resume();
-	case PM_HIBERNATION_PREPARE:
-	case PM_SUSPEND_PREPARE:
-		return wdt_suspend();
-	default:
-		return NOTIFY_DONE;
-	}
-}
-
-static struct notifier_block wdt_power_notifier = {
-	.notifier_call = wdt_power_event,
-};
-
 static int __init diag288_init(void)
 {
 	int ret;
@@ -297,21 +245,12 @@ static int __init diag288_init(void)
 		return -EINVAL;
 	}
 
-	ret = register_pm_notifier(&wdt_power_notifier);
-	if (ret)
-		return ret;
-
-	ret = watchdog_register_device(&wdt_dev);
-	if (ret)
-		unregister_pm_notifier(&wdt_power_notifier);
-
-	return ret;
+	return watchdog_register_device(&wdt_dev);
 }
 
 static void __exit diag288_exit(void)
 {
 	watchdog_unregister_device(&wdt_dev);
-	unregister_pm_notifier(&wdt_power_notifier);
 }
 
 module_init(diag288_init);
-- 
2.37.2

Re: [PATCH 2/5] watchdog: diag288_wdt: remove power management

[Guenter Roeck]

On 2/2/23 23:39, Alexander Egorenkov wrote:
> Remove power management because s390 no longer supports hibernation since
> commit 394216275c7d ("s390: remove broken hibernate / power management
> support").
> 
> Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>

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

> ---
>   drivers/watchdog/diag288_wdt.c | 65 ++--------------------------------
>   1 file changed, 2 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
> index 07ebbb709af4..c8d516ced6d2 100644
> --- a/drivers/watchdog/diag288_wdt.c
> +++ b/drivers/watchdog/diag288_wdt.c
> @@ -27,7 +27,6 @@
>   #include <linux/moduleparam.h>
>   #include <linux/slab.h>
>   #include <linux/watchdog.h>
> -#include <linux/suspend.h>
>   #include <asm/ebcdic.h>
>   #include <asm/diag.h>
>   #include <linux/io.h>
> @@ -103,10 +102,6 @@ static int __diag288_lpar(unsigned int func, unsigned int timeout,
>   	return __diag288(func, timeout, action, 0);
>   }
>   
> -static unsigned long wdt_status;
> -
> -#define DIAG_WDOG_BUSY	0
> -
>   static int wdt_start(struct watchdog_device *dev)
>   {
>   	char *ebc_cmd;
> @@ -114,15 +109,10 @@ static int wdt_start(struct watchdog_device *dev)
>   	int ret;
>   	unsigned int func;
>   
> -	if (test_and_set_bit(DIAG_WDOG_BUSY, &wdt_status))
> -		return -EBUSY;
> -
>   	if (MACHINE_IS_VM) {
>   		ebc_cmd = kmalloc(MAX_CMDLEN, GFP_KERNEL);
> -		if (!ebc_cmd) {
> -			clear_bit(DIAG_WDOG_BUSY, &wdt_status);
> +		if (!ebc_cmd)
>   			return -ENOMEM;
> -		}
>   		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
>   		ASCEBC(ebc_cmd, MAX_CMDLEN);
>   		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
> @@ -139,7 +129,6 @@ static int wdt_start(struct watchdog_device *dev)
>   
>   	if (ret) {
>   		pr_err("The watchdog cannot be activated\n");
> -		clear_bit(DIAG_WDOG_BUSY, &wdt_status);
>   		return ret;
>   	}
>   	return 0;
> @@ -152,8 +141,6 @@ static int wdt_stop(struct watchdog_device *dev)
>   	diag_stat_inc(DIAG_STAT_X288);
>   	ret = __diag288(WDT_FUNC_CANCEL, 0, 0, 0);
>   
> -	clear_bit(DIAG_WDOG_BUSY, &wdt_status);
> -
>   	return ret;
>   }
>   
> @@ -222,45 +209,6 @@ static struct watchdog_device wdt_dev = {
>   	.max_timeout = MAX_INTERVAL,
>   };
>   
> -/*
> - * It makes no sense to go into suspend while the watchdog is running.
> - * Depending on the memory size, the watchdog might trigger, while we
> - * are still saving the memory.
> - */
> -static int wdt_suspend(void)
> -{
> -	if (test_and_set_bit(DIAG_WDOG_BUSY, &wdt_status)) {
> -		pr_err("Linux cannot be suspended while the watchdog is in use\n");
> -		return notifier_from_errno(-EBUSY);
> -	}
> -	return NOTIFY_DONE;
> -}
> -
> -static int wdt_resume(void)
> -{
> -	clear_bit(DIAG_WDOG_BUSY, &wdt_status);
> -	return NOTIFY_DONE;
> -}
> -
> -static int wdt_power_event(struct notifier_block *this, unsigned long event,
> -			   void *ptr)
> -{
> -	switch (event) {
> -	case PM_POST_HIBERNATION:
> -	case PM_POST_SUSPEND:
> -		return wdt_resume();
> -	case PM_HIBERNATION_PREPARE:
> -	case PM_SUSPEND_PREPARE:
> -		return wdt_suspend();
> -	default:
> -		return NOTIFY_DONE;
> -	}
> -}
> -
> -static struct notifier_block wdt_power_notifier = {
> -	.notifier_call = wdt_power_event,
> -};
> -
>   static int __init diag288_init(void)
>   {
>   	int ret;
> @@ -297,21 +245,12 @@ static int __init diag288_init(void)
>   		return -EINVAL;
>   	}
>   
> -	ret = register_pm_notifier(&wdt_power_notifier);
> -	if (ret)
> -		return ret;
> -
> -	ret = watchdog_register_device(&wdt_dev);
> -	if (ret)
> -		unregister_pm_notifier(&wdt_power_notifier);
> -
> -	return ret;
> +	return watchdog_register_device(&wdt_dev);
>   }
>   
>   static void __exit diag288_exit(void)
>   {
>   	watchdog_unregister_device(&wdt_dev);
> -	unregister_pm_notifier(&wdt_power_notifier);
>   }
>   
>   module_init(diag288_init);

[PATCH 3/5] watchdog: diag288_wdt: unify command buffer handling for diag288 zvm

[Alexander Egorenkov]

Simplify and de-duplicate code by introducing a common single command
buffer allocated once at initialization. Moreover, simplify the interface
of __diag288_vm() by accepting ASCII strings as the command parameter
and converting it to the EBCDIC format within the function itself.

Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
---
 drivers/watchdog/diag288_wdt.c | 55 +++++++++++++---------------------
 1 file changed, 20 insertions(+), 35 deletions(-)

diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
index c8d516ced6d2..c717f47dd4c3 100644
--- a/drivers/watchdog/diag288_wdt.c
+++ b/drivers/watchdog/diag288_wdt.c
@@ -69,6 +69,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default = C
 
 MODULE_ALIAS("vmwatchdog");
 
+static char *cmd_buf;
+
 static int __diag288(unsigned int func, unsigned int timeout,
 		     unsigned long action, unsigned int len)
 {
@@ -88,11 +90,18 @@ static int __diag288(unsigned int func, unsigned int timeout,
 	return err;
 }
 
-static int __diag288_vm(unsigned int  func, unsigned int timeout,
-			char *cmd, size_t len)
+static int __diag288_vm(unsigned int  func, unsigned int timeout, char *cmd)
 {
+	ssize_t len;
+
+	len = strscpy(cmd_buf, cmd, MAX_CMDLEN);
+	if (len < 0)
+		return len;
+	ASCEBC(cmd_buf, MAX_CMDLEN);
+	EBC_TOUPPER(cmd_buf, MAX_CMDLEN);
+
 	diag_stat_inc(DIAG_STAT_X288);
-	return __diag288(func, timeout, virt_to_phys(cmd), len);
+	return __diag288(func, timeout, virt_to_phys(cmd_buf), len);
 }
 
 static int __diag288_lpar(unsigned int func, unsigned int timeout,
@@ -104,24 +113,14 @@ static int __diag288_lpar(unsigned int func, unsigned int timeout,
 
 static int wdt_start(struct watchdog_device *dev)
 {
-	char *ebc_cmd;
-	size_t len;
 	int ret;
 	unsigned int func;
 
 	if (MACHINE_IS_VM) {
-		ebc_cmd = kmalloc(MAX_CMDLEN, GFP_KERNEL);
-		if (!ebc_cmd)
-			return -ENOMEM;
-		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
-		ASCEBC(ebc_cmd, MAX_CMDLEN);
-		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
-
 		func = conceal_on ? (WDT_FUNC_INIT | WDT_FUNC_CONCEAL)
 			: WDT_FUNC_INIT;
-		ret = __diag288_vm(func, dev->timeout, ebc_cmd, len);
+		ret = __diag288_vm(func, dev->timeout, wdt_cmd);
 		WARN_ON(ret != 0);
-		kfree(ebc_cmd);
 	} else {
 		ret = __diag288_lpar(WDT_FUNC_INIT,
 				     dev->timeout, LPARWDT_RESTART);
@@ -146,19 +145,10 @@ static int wdt_stop(struct watchdog_device *dev)
 
 static int wdt_ping(struct watchdog_device *dev)
 {
-	char *ebc_cmd;
-	size_t len;
 	int ret;
 	unsigned int func;
 
 	if (MACHINE_IS_VM) {
-		ebc_cmd = kmalloc(MAX_CMDLEN, GFP_KERNEL);
-		if (!ebc_cmd)
-			return -ENOMEM;
-		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
-		ASCEBC(ebc_cmd, MAX_CMDLEN);
-		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
-
 		/*
 		 * It seems to be ok to z/VM to use the init function to
 		 * retrigger the watchdog. On LPAR WDT_FUNC_CHANGE must
@@ -167,9 +157,8 @@ static int wdt_ping(struct watchdog_device *dev)
 		func = conceal_on ? (WDT_FUNC_INIT | WDT_FUNC_CONCEAL)
 			: WDT_FUNC_INIT;
 
-		ret = __diag288_vm(func, dev->timeout, ebc_cmd, len);
+		ret = __diag288_vm(func, dev->timeout, wdt_cmd);
 		WARN_ON(ret != 0);
-		kfree(ebc_cmd);
 	} else {
 		ret = __diag288_lpar(WDT_FUNC_CHANGE, dev->timeout, 0);
 	}
@@ -212,25 +201,20 @@ static struct watchdog_device wdt_dev = {
 static int __init diag288_init(void)
 {
 	int ret;
-	char ebc_begin[] = {
-		194, 197, 199, 201, 213
-	};
-	char *ebc_cmd;
 
 	watchdog_set_nowayout(&wdt_dev, nowayout_info);
 
 	if (MACHINE_IS_VM) {
-		ebc_cmd = kmalloc(sizeof(ebc_begin), GFP_KERNEL);
-		if (!ebc_cmd) {
+		cmd_buf = kmalloc(MAX_CMDLEN, GFP_KERNEL);
+		if (!cmd_buf) {
 			pr_err("The watchdog cannot be initialized\n");
 			return -ENOMEM;
 		}
-		memcpy(ebc_cmd, ebc_begin, sizeof(ebc_begin));
-		ret = __diag288_vm(WDT_FUNC_INIT, 15,
-				   ebc_cmd, sizeof(ebc_begin));
-		kfree(ebc_cmd);
+
+		ret = __diag288_vm(WDT_FUNC_INIT, MIN_INTERVAL, "BEGIN");
 		if (ret != 0) {
 			pr_err("The watchdog cannot be initialized\n");
+			kfree(cmd_buf);
 			return -EINVAL;
 		}
 	} else {
@@ -251,6 +235,7 @@ static int __init diag288_init(void)
 static void __exit diag288_exit(void)
 {
 	watchdog_unregister_device(&wdt_dev);
+	kfree(cmd_buf);
 }
 
 module_init(diag288_init);
-- 
2.37.2

Re: [PATCH 3/5] watchdog: diag288_wdt: unify command buffer handling for diag288 zvm

[Guenter Roeck]

On 2/2/23 23:39, Alexander Egorenkov wrote:
> Simplify and de-duplicate code by introducing a common single command
> buffer allocated once at initialization. Moreover, simplify the interface
> of __diag288_vm() by accepting ASCII strings as the command parameter
> and converting it to the EBCDIC format within the function itself.
> 
> Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
> ---
>   drivers/watchdog/diag288_wdt.c | 55 +++++++++++++---------------------
>   1 file changed, 20 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
> index c8d516ced6d2..c717f47dd4c3 100644
> --- a/drivers/watchdog/diag288_wdt.c
> +++ b/drivers/watchdog/diag288_wdt.c
> @@ -69,6 +69,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default = C
>   
>   MODULE_ALIAS("vmwatchdog");
>   
> +static char *cmd_buf;
> +

Personally I wound not even bother allocating this, but that is just my personal
opinion. And I guess one more static variable doesn't make much of a difference
either.

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

>   static int __diag288(unsigned int func, unsigned int timeout,
>   		     unsigned long action, unsigned int len)
>   {
> @@ -88,11 +90,18 @@ static int __diag288(unsigned int func, unsigned int timeout,
>   	return err;
>   }
>   
> -static int __diag288_vm(unsigned int  func, unsigned int timeout,
> -			char *cmd, size_t len)
> +static int __diag288_vm(unsigned int  func, unsigned int timeout, char *cmd)
>   {
> +	ssize_t len;
> +
> +	len = strscpy(cmd_buf, cmd, MAX_CMDLEN);
> +	if (len < 0)
> +		return len;
> +	ASCEBC(cmd_buf, MAX_CMDLEN);
> +	EBC_TOUPPER(cmd_buf, MAX_CMDLEN);
> +
>   	diag_stat_inc(DIAG_STAT_X288);
> -	return __diag288(func, timeout, virt_to_phys(cmd), len);
> +	return __diag288(func, timeout, virt_to_phys(cmd_buf), len);
>   }
>   
>   static int __diag288_lpar(unsigned int func, unsigned int timeout,
> @@ -104,24 +113,14 @@ static int __diag288_lpar(unsigned int func, unsigned int timeout,
>   
>   static int wdt_start(struct watchdog_device *dev)
>   {
> -	char *ebc_cmd;
> -	size_t len;
>   	int ret;
>   	unsigned int func;
>   
>   	if (MACHINE_IS_VM) {
> -		ebc_cmd = kmalloc(MAX_CMDLEN, GFP_KERNEL);
> -		if (!ebc_cmd)
> -			return -ENOMEM;
> -		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
> -		ASCEBC(ebc_cmd, MAX_CMDLEN);
> -		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
> -
>   		func = conceal_on ? (WDT_FUNC_INIT | WDT_FUNC_CONCEAL)
>   			: WDT_FUNC_INIT;
> -		ret = __diag288_vm(func, dev->timeout, ebc_cmd, len);
> +		ret = __diag288_vm(func, dev->timeout, wdt_cmd);
>   		WARN_ON(ret != 0);
> -		kfree(ebc_cmd);
>   	} else {
>   		ret = __diag288_lpar(WDT_FUNC_INIT,
>   				     dev->timeout, LPARWDT_RESTART);
> @@ -146,19 +145,10 @@ static int wdt_stop(struct watchdog_device *dev)
>   
>   static int wdt_ping(struct watchdog_device *dev)
>   {
> -	char *ebc_cmd;
> -	size_t len;
>   	int ret;
>   	unsigned int func;
>   
>   	if (MACHINE_IS_VM) {
> -		ebc_cmd = kmalloc(MAX_CMDLEN, GFP_KERNEL);
> -		if (!ebc_cmd)
> -			return -ENOMEM;
> -		len = strlcpy(ebc_cmd, wdt_cmd, MAX_CMDLEN);
> -		ASCEBC(ebc_cmd, MAX_CMDLEN);
> -		EBC_TOUPPER(ebc_cmd, MAX_CMDLEN);
> -
>   		/*
>   		 * It seems to be ok to z/VM to use the init function to
>   		 * retrigger the watchdog. On LPAR WDT_FUNC_CHANGE must
> @@ -167,9 +157,8 @@ static int wdt_ping(struct watchdog_device *dev)
>   		func = conceal_on ? (WDT_FUNC_INIT | WDT_FUNC_CONCEAL)
>   			: WDT_FUNC_INIT;
>   
> -		ret = __diag288_vm(func, dev->timeout, ebc_cmd, len);
> +		ret = __diag288_vm(func, dev->timeout, wdt_cmd);
>   		WARN_ON(ret != 0);
> -		kfree(ebc_cmd);
>   	} else {
>   		ret = __diag288_lpar(WDT_FUNC_CHANGE, dev->timeout, 0);
>   	}
> @@ -212,25 +201,20 @@ static struct watchdog_device wdt_dev = {
>   static int __init diag288_init(void)
>   {
>   	int ret;
> -	char ebc_begin[] = {
> -		194, 197, 199, 201, 213
> -	};
> -	char *ebc_cmd;
>   
>   	watchdog_set_nowayout(&wdt_dev, nowayout_info);
>   
>   	if (MACHINE_IS_VM) {
> -		ebc_cmd = kmalloc(sizeof(ebc_begin), GFP_KERNEL);
> -		if (!ebc_cmd) {
> +		cmd_buf = kmalloc(MAX_CMDLEN, GFP_KERNEL);
> +		if (!cmd_buf) {
>   			pr_err("The watchdog cannot be initialized\n");
>   			return -ENOMEM;
>   		}
> -		memcpy(ebc_cmd, ebc_begin, sizeof(ebc_begin));
> -		ret = __diag288_vm(WDT_FUNC_INIT, 15,
> -				   ebc_cmd, sizeof(ebc_begin));
> -		kfree(ebc_cmd);
> +
> +		ret = __diag288_vm(WDT_FUNC_INIT, MIN_INTERVAL, "BEGIN");
>   		if (ret != 0) {
>   			pr_err("The watchdog cannot be initialized\n");
> +			kfree(cmd_buf);
>   			return -EINVAL;
>   		}
>   	} else {
> @@ -251,6 +235,7 @@ static int __init diag288_init(void)
>   static void __exit diag288_exit(void)
>   {
>   	watchdog_unregister_device(&wdt_dev);
> +	kfree(cmd_buf);
>   }
>   
>   module_init(diag288_init);

[PATCH 4/5] watchdog: diag288_wdt: de-duplicate diag_stat_inc() calls

[Alexander Egorenkov]

Call diag_stat_inc() from __diag288() to reduce code duplication.

Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
---
 drivers/watchdog/diag288_wdt.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
index c717f47dd4c3..a29ad164b27a 100644
--- a/drivers/watchdog/diag288_wdt.c
+++ b/drivers/watchdog/diag288_wdt.c
@@ -78,6 +78,8 @@ static int __diag288(unsigned int func, unsigned int timeout,
 	union register_pair r3 = { .even = action, .odd = len, };
 	int err;
 
+	diag_stat_inc(DIAG_STAT_X288);
+
 	err = -EINVAL;
 	asm volatile(
 		"	diag	%[r1],%[r3],0x288\n"
@@ -100,14 +102,12 @@ static int __diag288_vm(unsigned int  func, unsigned int timeout, char *cmd)
 	ASCEBC(cmd_buf, MAX_CMDLEN);
 	EBC_TOUPPER(cmd_buf, MAX_CMDLEN);
 
-	diag_stat_inc(DIAG_STAT_X288);
 	return __diag288(func, timeout, virt_to_phys(cmd_buf), len);
 }
 
 static int __diag288_lpar(unsigned int func, unsigned int timeout,
 			  unsigned long action)
 {
-	diag_stat_inc(DIAG_STAT_X288);
 	return __diag288(func, timeout, action, 0);
 }
 
@@ -135,12 +135,7 @@ static int wdt_start(struct watchdog_device *dev)
 
 static int wdt_stop(struct watchdog_device *dev)
 {
-	int ret;
-
-	diag_stat_inc(DIAG_STAT_X288);
-	ret = __diag288(WDT_FUNC_CANCEL, 0, 0, 0);
-
-	return ret;
+	return __diag288(WDT_FUNC_CANCEL, 0, 0, 0);
 }
 
 static int wdt_ping(struct watchdog_device *dev)
-- 
2.37.2

Re: [PATCH 4/5] watchdog: diag288_wdt: de-duplicate diag_stat_inc() calls

[Guenter Roeck]

On 2/2/23 23:39, Alexander Egorenkov wrote:
> Call diag_stat_inc() from __diag288() to reduce code duplication.
> 
> Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>

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

> ---
>   drivers/watchdog/diag288_wdt.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
> index c717f47dd4c3..a29ad164b27a 100644
> --- a/drivers/watchdog/diag288_wdt.c
> +++ b/drivers/watchdog/diag288_wdt.c
> @@ -78,6 +78,8 @@ static int __diag288(unsigned int func, unsigned int timeout,
>   	union register_pair r3 = { .even = action, .odd = len, };
>   	int err;
>   
> +	diag_stat_inc(DIAG_STAT_X288);
> +
>   	err = -EINVAL;
>   	asm volatile(
>   		"	diag	%[r1],%[r3],0x288\n"
> @@ -100,14 +102,12 @@ static int __diag288_vm(unsigned int  func, unsigned int timeout, char *cmd)
>   	ASCEBC(cmd_buf, MAX_CMDLEN);
>   	EBC_TOUPPER(cmd_buf, MAX_CMDLEN);
>   
> -	diag_stat_inc(DIAG_STAT_X288);
>   	return __diag288(func, timeout, virt_to_phys(cmd_buf), len);
>   }
>   
>   static int __diag288_lpar(unsigned int func, unsigned int timeout,
>   			  unsigned long action)
>   {
> -	diag_stat_inc(DIAG_STAT_X288);
>   	return __diag288(func, timeout, action, 0);
>   }
>   
> @@ -135,12 +135,7 @@ static int wdt_start(struct watchdog_device *dev)
>   
>   static int wdt_stop(struct watchdog_device *dev)
>   {
> -	int ret;
> -
> -	diag_stat_inc(DIAG_STAT_X288);
> -	ret = __diag288(WDT_FUNC_CANCEL, 0, 0, 0);
> -
> -	return ret;
> +	return __diag288(WDT_FUNC_CANCEL, 0, 0, 0);
>   }
>   
>   static int wdt_ping(struct watchdog_device *dev)

[PATCH 5/5] watchdog: diag288_wdt: unify lpar and zvm diag288 helpers

[Alexander Egorenkov]

Change naming of the internal diag288 helper functions
to improve overall readability and reduce confusion:
* Rename __diag288() to diag288().
* Get rid of the misnamed helper __diag288_lpar() that was used not only
  on LPARs but also zVM and KVM systems.
* Rename __diag288_vm() to diag288_str().

Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
---
 drivers/watchdog/diag288_wdt.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
index a29ad164b27a..4631d0a3866a 100644
--- a/drivers/watchdog/diag288_wdt.c
+++ b/drivers/watchdog/diag288_wdt.c
@@ -71,8 +71,8 @@ MODULE_ALIAS("vmwatchdog");
 
 static char *cmd_buf;
 
-static int __diag288(unsigned int func, unsigned int timeout,
-		     unsigned long action, unsigned int len)
+static int diag288(unsigned int func, unsigned int timeout,
+		   unsigned long action, unsigned int len)
 {
 	union register_pair r1 = { .even = func, .odd = timeout, };
 	union register_pair r3 = { .even = action, .odd = len, };
@@ -92,7 +92,7 @@ static int __diag288(unsigned int func, unsigned int timeout,
 	return err;
 }
 
-static int __diag288_vm(unsigned int  func, unsigned int timeout, char *cmd)
+static int diag288_str(unsigned int func, unsigned int timeout, char *cmd)
 {
 	ssize_t len;
 
@@ -102,13 +102,7 @@ static int __diag288_vm(unsigned int  func, unsigned int timeout, char *cmd)
 	ASCEBC(cmd_buf, MAX_CMDLEN);
 	EBC_TOUPPER(cmd_buf, MAX_CMDLEN);
 
-	return __diag288(func, timeout, virt_to_phys(cmd_buf), len);
-}
-
-static int __diag288_lpar(unsigned int func, unsigned int timeout,
-			  unsigned long action)
-{
-	return __diag288(func, timeout, action, 0);
+	return diag288(func, timeout, virt_to_phys(cmd_buf), len);
 }
 
 static int wdt_start(struct watchdog_device *dev)
@@ -119,11 +113,10 @@ static int wdt_start(struct watchdog_device *dev)
 	if (MACHINE_IS_VM) {
 		func = conceal_on ? (WDT_FUNC_INIT | WDT_FUNC_CONCEAL)
 			: WDT_FUNC_INIT;
-		ret = __diag288_vm(func, dev->timeout, wdt_cmd);
+		ret = diag288_str(func, dev->timeout, wdt_cmd);
 		WARN_ON(ret != 0);
 	} else {
-		ret = __diag288_lpar(WDT_FUNC_INIT,
-				     dev->timeout, LPARWDT_RESTART);
+		ret = diag288(WDT_FUNC_INIT, dev->timeout, LPARWDT_RESTART, 0);
 	}
 
 	if (ret) {
@@ -135,7 +128,7 @@ static int wdt_start(struct watchdog_device *dev)
 
 static int wdt_stop(struct watchdog_device *dev)
 {
-	return __diag288(WDT_FUNC_CANCEL, 0, 0, 0);
+	return diag288(WDT_FUNC_CANCEL, 0, 0, 0);
 }
 
 static int wdt_ping(struct watchdog_device *dev)
@@ -152,10 +145,10 @@ static int wdt_ping(struct watchdog_device *dev)
 		func = conceal_on ? (WDT_FUNC_INIT | WDT_FUNC_CONCEAL)
 			: WDT_FUNC_INIT;
 
-		ret = __diag288_vm(func, dev->timeout, wdt_cmd);
+		ret = diag288_str(func, dev->timeout, wdt_cmd);
 		WARN_ON(ret != 0);
 	} else {
-		ret = __diag288_lpar(WDT_FUNC_CHANGE, dev->timeout, 0);
+		ret = diag288(WDT_FUNC_CHANGE, dev->timeout, 0, 0);
 	}
 
 	if (ret)
@@ -206,20 +199,21 @@ static int __init diag288_init(void)
 			return -ENOMEM;
 		}
 
-		ret = __diag288_vm(WDT_FUNC_INIT, MIN_INTERVAL, "BEGIN");
+		ret = diag288_str(WDT_FUNC_INIT, MIN_INTERVAL, "BEGIN");
 		if (ret != 0) {
 			pr_err("The watchdog cannot be initialized\n");
 			kfree(cmd_buf);
 			return -EINVAL;
 		}
 	} else {
-		if (__diag288_lpar(WDT_FUNC_INIT, 30, LPARWDT_RESTART)) {
+		if (diag288(WDT_FUNC_INIT, WDT_DEFAULT_TIMEOUT,
+			    LPARWDT_RESTART, 0)) {
 			pr_err("The watchdog cannot be initialized\n");
 			return -EINVAL;
 		}
 	}
 
-	if (__diag288_lpar(WDT_FUNC_CANCEL, 0, 0)) {
+	if (diag288(WDT_FUNC_CANCEL, 0, 0, 0)) {
 		pr_err("The watchdog cannot be deactivated\n");
 		return -EINVAL;
 	}
-- 
2.37.2

Re: [PATCH 5/5] watchdog: diag288_wdt: unify lpar and zvm diag288 helpers

[Guenter Roeck]

On 2/2/23 23:39, Alexander Egorenkov wrote:
> Change naming of the internal diag288 helper functions
> to improve overall readability and reduce confusion:
> * Rename __diag288() to diag288().
> * Get rid of the misnamed helper __diag288_lpar() that was used not only
>    on LPARs but also zVM and KVM systems.
> * Rename __diag288_vm() to diag288_str().
> 
> Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
> Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>

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

> ---
>   drivers/watchdog/diag288_wdt.c | 32 +++++++++++++-------------------
>   1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/watchdog/diag288_wdt.c b/drivers/watchdog/diag288_wdt.c
> index a29ad164b27a..4631d0a3866a 100644
> --- a/drivers/watchdog/diag288_wdt.c
> +++ b/drivers/watchdog/diag288_wdt.c
> @@ -71,8 +71,8 @@ MODULE_ALIAS("vmwatchdog");
>   
>   static char *cmd_buf;
>   
> -static int __diag288(unsigned int func, unsigned int timeout,
> -		     unsigned long action, unsigned int len)
> +static int diag288(unsigned int func, unsigned int timeout,
> +		   unsigned long action, unsigned int len)
>   {
>   	union register_pair r1 = { .even = func, .odd = timeout, };
>   	union register_pair r3 = { .even = action, .odd = len, };
> @@ -92,7 +92,7 @@ static int __diag288(unsigned int func, unsigned int timeout,
>   	return err;
>   }
>   
> -static int __diag288_vm(unsigned int  func, unsigned int timeout, char *cmd)
> +static int diag288_str(unsigned int func, unsigned int timeout, char *cmd)
>   {
>   	ssize_t len;
>   
> @@ -102,13 +102,7 @@ static int __diag288_vm(unsigned int  func, unsigned int timeout, char *cmd)
>   	ASCEBC(cmd_buf, MAX_CMDLEN);
>   	EBC_TOUPPER(cmd_buf, MAX_CMDLEN);
>   
> -	return __diag288(func, timeout, virt_to_phys(cmd_buf), len);
> -}
> -
> -static int __diag288_lpar(unsigned int func, unsigned int timeout,
> -			  unsigned long action)
> -{
> -	return __diag288(func, timeout, action, 0);
> +	return diag288(func, timeout, virt_to_phys(cmd_buf), len);
>   }
>   
>   static int wdt_start(struct watchdog_device *dev)
> @@ -119,11 +113,10 @@ static int wdt_start(struct watchdog_device *dev)
>   	if (MACHINE_IS_VM) {
>   		func = conceal_on ? (WDT_FUNC_INIT | WDT_FUNC_CONCEAL)
>   			: WDT_FUNC_INIT;
> -		ret = __diag288_vm(func, dev->timeout, wdt_cmd);
> +		ret = diag288_str(func, dev->timeout, wdt_cmd);
>   		WARN_ON(ret != 0);
>   	} else {
> -		ret = __diag288_lpar(WDT_FUNC_INIT,
> -				     dev->timeout, LPARWDT_RESTART);
> +		ret = diag288(WDT_FUNC_INIT, dev->timeout, LPARWDT_RESTART, 0);
>   	}
>   
>   	if (ret) {
> @@ -135,7 +128,7 @@ static int wdt_start(struct watchdog_device *dev)
>   
>   static int wdt_stop(struct watchdog_device *dev)
>   {
> -	return __diag288(WDT_FUNC_CANCEL, 0, 0, 0);
> +	return diag288(WDT_FUNC_CANCEL, 0, 0, 0);
>   }
>   
>   static int wdt_ping(struct watchdog_device *dev)
> @@ -152,10 +145,10 @@ static int wdt_ping(struct watchdog_device *dev)
>   		func = conceal_on ? (WDT_FUNC_INIT | WDT_FUNC_CONCEAL)
>   			: WDT_FUNC_INIT;
>   
> -		ret = __diag288_vm(func, dev->timeout, wdt_cmd);
> +		ret = diag288_str(func, dev->timeout, wdt_cmd);
>   		WARN_ON(ret != 0);
>   	} else {
> -		ret = __diag288_lpar(WDT_FUNC_CHANGE, dev->timeout, 0);
> +		ret = diag288(WDT_FUNC_CHANGE, dev->timeout, 0, 0);
>   	}
>   
>   	if (ret)
> @@ -206,20 +199,21 @@ static int __init diag288_init(void)
>   			return -ENOMEM;
>   		}
>   
> -		ret = __diag288_vm(WDT_FUNC_INIT, MIN_INTERVAL, "BEGIN");
> +		ret = diag288_str(WDT_FUNC_INIT, MIN_INTERVAL, "BEGIN");
>   		if (ret != 0) {
>   			pr_err("The watchdog cannot be initialized\n");
>   			kfree(cmd_buf);
>   			return -EINVAL;
>   		}
>   	} else {
> -		if (__diag288_lpar(WDT_FUNC_INIT, 30, LPARWDT_RESTART)) {
> +		if (diag288(WDT_FUNC_INIT, WDT_DEFAULT_TIMEOUT,
> +			    LPARWDT_RESTART, 0)) {
>   			pr_err("The watchdog cannot be initialized\n");
>   			return -EINVAL;
>   		}
>   	}
>   
> -	if (__diag288_lpar(WDT_FUNC_CANCEL, 0, 0)) {
> +	if (diag288(WDT_FUNC_CANCEL, 0, 0, 0)) {
>   		pr_err("The watchdog cannot be deactivated\n");
>   		return -EINVAL;
>   	}

Re: [PATCH 0/5] diag288 watchdog fixes and improvements

[Heiko Carstens]

On Fri, Feb 03, 2023 at 08:39:53AM +0100, Alexander Egorenkov wrote:
> Minor code refactoring to improve readability of the driver,
> reduce code duplication and remove dead code.
> 
> Alexander Egorenkov (5):
>   watchdog: diag288_wdt: get rid of register asm
>   watchdog: diag288_wdt: remove power management
>   watchdog: diag288_wdt: unify command buffer handling for diag288 zvm
>   watchdog: diag288_wdt: de-duplicate diag_stat_inc() calls
>   watchdog: diag288_wdt: unify lpar and zvm diag288 helpers
> 
>  drivers/watchdog/diag288_wdt.c | 162 ++++++++-------------------------
>  1 file changed, 37 insertions(+), 125 deletions(-)

Guenter, Wim, how should this go upstream?

I can easily pick this up via the s390 tree for the next merge window.
Please let me know.

Thanks,
Heiko

Re: [PATCH 0/5] diag288 watchdog fixes and improvements

[Guenter Roeck]

On 2/6/23 01:59, Heiko Carstens wrote:
> On Fri, Feb 03, 2023 at 08:39:53AM +0100, Alexander Egorenkov wrote:
>> Minor code refactoring to improve readability of the driver,
>> reduce code duplication and remove dead code.
>>
>> Alexander Egorenkov (5):
>>    watchdog: diag288_wdt: get rid of register asm
>>    watchdog: diag288_wdt: remove power management
>>    watchdog: diag288_wdt: unify command buffer handling for diag288 zvm
>>    watchdog: diag288_wdt: de-duplicate diag_stat_inc() calls
>>    watchdog: diag288_wdt: unify lpar and zvm diag288 helpers
>>
>>   drivers/watchdog/diag288_wdt.c | 162 ++++++++-------------------------
>>   1 file changed, 37 insertions(+), 125 deletions(-)
> 
> Guenter, Wim, how should this go upstream?
> 
> I can easily pick this up via the s390 tree for the next merge window.
> Please let me know.
> 

I have it currently in my watchdog-next tree, but that is not in linux-next.
Fine with me to go through the s390 tree.

Guenter

Re: [PATCH 0/5] diag288 watchdog fixes and improvements

[Heiko Carstens]

On Mon, Feb 06, 2023 at 05:55:40AM -0800, Guenter Roeck wrote:
> On 2/6/23 01:59, Heiko Carstens wrote:
> > On Fri, Feb 03, 2023 at 08:39:53AM +0100, Alexander Egorenkov wrote:
> > > Minor code refactoring to improve readability of the driver,
> > > reduce code duplication and remove dead code.
> > > 
> > > Alexander Egorenkov (5):
> > >    watchdog: diag288_wdt: get rid of register asm
> > >    watchdog: diag288_wdt: remove power management
> > >    watchdog: diag288_wdt: unify command buffer handling for diag288 zvm
> > >    watchdog: diag288_wdt: de-duplicate diag_stat_inc() calls
> > >    watchdog: diag288_wdt: unify lpar and zvm diag288 helpers
> > > 
> > >   drivers/watchdog/diag288_wdt.c | 162 ++++++++-------------------------
> > >   1 file changed, 37 insertions(+), 125 deletions(-)
> > 
> > Guenter, Wim, how should this go upstream?
> > 
> > I can easily pick this up via the s390 tree for the next merge window.
> > Please let me know.
> 
> I have it currently in my watchdog-next tree, but that is not in linux-next.
> Fine with me to go through the s390 tree.

Applied to s390 tree - should be in next linux-next release.

Thank you!