(no subject)

[Chen Gong]

this patch only makes a few fixes for latest kernel git tree

[PATCH] Watchdog on MPC85xx SMP system

[Chen Gong]

On Book-E SMP systems each core has its own private watchdog.
If only one watchdog is enabled, when the core that doesn't
enable the watchdog is hung, system can't reset because no
watchdog is running on it. That's bad. It means we must
enable watchdogs on both cores.

We can use smp_call_function() to send appropriate messages to
all the other cores to enable and update the watchdog.

Signed-off-by: Chen Gong <g.chen@freescale.com>
---
Now Tested on MPC8572DS platform.

 drivers/watchdog/booke_wdt.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c
index d362f5b..8a4e7f0 100644
--- a/drivers/watchdog/booke_wdt.c
+++ b/drivers/watchdog/booke_wdt.c
@@ -1,12 +1,12 @@
 /*
- * drivers/char/watchdog/booke_wdt.c
+ * drivers/watchdog/booke_wdt.c
  *
  * Watchdog timer for PowerPC Book-E systems
  *
  * Author: Matthew McClintock
  * Maintainer: Kumar Gala <galak@kernel.crashing.org>
  *
- * Copyright 2005 Freescale Semiconductor Inc.
+ * Copyright 2005, 2008 Freescale Semiconductor Inc.
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -16,6 +16,7 @@
 
 #include <linux/module.h>
 #include <linux/fs.h>
+#include <linux/smp.h>
 #include <linux/miscdevice.h>
 #include <linux/notifier.h>
 #include <linux/watchdog.h>
@@ -47,23 +48,31 @@ u32 booke_wdt_period = WDT_PERIOD_DEFAULT;
 #define WDTP(x)		(TCR_WP(x))
 #endif
 
+static DEFINE_SPINLOCK(booke_wdt_lock);
+
+static void __booke_wdt_ping(void *data)
+{
+	mtspr(SPRN_TSR, TSR_ENW|TSR_WIS);
+}
+
 /*
  * booke_wdt_ping:
  */
 static __inline__ void booke_wdt_ping(void)
 {
-	mtspr(SPRN_TSR, TSR_ENW|TSR_WIS);
+	smp_call_function(__booke_wdt_ping, NULL, 0, 0);
+	__booke_wdt_ping(NULL);
 }
 
 /*
- * booke_wdt_enable:
+ * __booke_wdt_enable:
  */
-static __inline__ void booke_wdt_enable(void)
+static inline void __booke_wdt_enable(void *data)
 {
 	u32 val;
 
 	/* clear status before enabling watchdog */
-	booke_wdt_ping();
+	__booke_wdt_ping(NULL);
 	val = mfspr(SPRN_TCR);
 	val |= (TCR_WIE|TCR_WRC(WRC_CHIP)|WDTP(booke_wdt_period));
 
@@ -137,12 +146,15 @@ static int booke_wdt_ioctl (struct inode *inode, struct file *file,
  */
 static int booke_wdt_open (struct inode *inode, struct file *file)
 {
+	spin_lock(&booke_wdt_lock);
 	if (booke_wdt_enabled == 0) {
 		booke_wdt_enabled = 1;
-		booke_wdt_enable();
+		__booke_wdt_enable(NULL);
+		smp_call_function(__booke_wdt_enable, NULL, 0, 0);
 		printk (KERN_INFO "PowerPC Book-E Watchdog Timer Enabled (wdt_period=%d)\n",
 				booke_wdt_period);
 	}
+	spin_unlock(&booke_wdt_lock);
 
 	return nonseekable_open(inode, file);
 }
@@ -183,11 +195,14 @@ static int __init booke_wdt_init(void)
 		return ret;
 	}
 
+	spin_lock(&booke_wdt_lock);
 	if (booke_wdt_enabled == 1) {
 		printk (KERN_INFO "PowerPC Book-E Watchdog Timer Enabled (wdt_period=%d)\n",
 				booke_wdt_period);
-		booke_wdt_enable();
+		__booke_wdt_enable(NULL);
+		smp_call_function(__booke_wdt_enable, NULL, 0, 0);
 	}
+	spin_unlock(&booke_wdt_lock);
 
 	return ret;
 }
-- 
1.5.4

Re: [PATCH] Watchdog on MPC85xx SMP system

[Andrew Morton]

On Tue, 29 Apr 2008 16:42:05 +0800
Chen Gong <g.chen@freescale.com> wrote:

> On Book-E SMP systems each core has its own private watchdog.
> If only one watchdog is enabled, when the core that doesn't
> enable the watchdog is hung, system can't reset because no
> watchdog is running on it. That's bad. It means we must
> enable watchdogs on both cores.
> 
> We can use smp_call_function() to send appropriate messages to
> all the other cores to enable and update the watchdog.
> 
> Signed-off-by: Chen Gong <g.chen@freescale.com>
> ---
> Now Tested on MPC8572DS platform.
> 
>  drivers/watchdog/booke_wdt.c |   31 +++++++++++++++++++++++--------
>  1 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/booke_wdt.c b/drivers/watchdog/booke_wdt.c
> index d362f5b..8a4e7f0 100644
> --- a/drivers/watchdog/booke_wdt.c
> +++ b/drivers/watchdog/booke_wdt.c
> @@ -1,12 +1,12 @@
>  /*
> - * drivers/char/watchdog/booke_wdt.c
> + * drivers/watchdog/booke_wdt.c
>   *
>   * Watchdog timer for PowerPC Book-E systems
>   *
>   * Author: Matthew McClintock
>   * Maintainer: Kumar Gala <galak@kernel.crashing.org>
>   *
> - * Copyright 2005 Freescale Semiconductor Inc.
> + * Copyright 2005, 2008 Freescale Semiconductor Inc.
>   *
>   * This program is free software; you can redistribute  it and/or modify it
>   * under  the terms of  the GNU General  Public License as published by the
> @@ -16,6 +16,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/fs.h>
> +#include <linux/smp.h>
>  #include <linux/miscdevice.h>
>  #include <linux/notifier.h>
>  #include <linux/watchdog.h>
> @@ -47,23 +48,31 @@ u32 booke_wdt_period = WDT_PERIOD_DEFAULT;
>  #define WDTP(x)		(TCR_WP(x))
>  #endif
>  
> +static DEFINE_SPINLOCK(booke_wdt_lock);
> +
> +static void __booke_wdt_ping(void *data)
> +{
> +	mtspr(SPRN_TSR, TSR_ENW|TSR_WIS);
> +}
> +
>  /*
>   * booke_wdt_ping:
>   */
>  static __inline__ void booke_wdt_ping(void)
>  {
> -	mtspr(SPRN_TSR, TSR_ENW|TSR_WIS);
> +	smp_call_function(__booke_wdt_ping, NULL, 0, 0);
> +	__booke_wdt_ping(NULL);
>  }
>  
>  /*
> - * booke_wdt_enable:
> + * __booke_wdt_enable:
>   */
> -static __inline__ void booke_wdt_enable(void)
> +static inline void __booke_wdt_enable(void *data)
>  {
>  	u32 val;
>  
>  	/* clear status before enabling watchdog */
> -	booke_wdt_ping();
> +	__booke_wdt_ping(NULL);
>  	val = mfspr(SPRN_TCR);
>  	val |= (TCR_WIE|TCR_WRC(WRC_CHIP)|WDTP(booke_wdt_period));
>  
> @@ -137,12 +146,15 @@ static int booke_wdt_ioctl (struct inode *inode, struct file *file,
>   */
>  static int booke_wdt_open (struct inode *inode, struct file *file)
>  {
> +	spin_lock(&booke_wdt_lock);
>  	if (booke_wdt_enabled == 0) {
>  		booke_wdt_enabled = 1;
> -		booke_wdt_enable();
> +		__booke_wdt_enable(NULL);
> +		smp_call_function(__booke_wdt_enable, NULL, 0, 0);

It's nonsensical to call an inlined function via smp_call_function().  What
we're asking the compiler to do here is to generate both an out-of-line
copy and an inlined copy.

Also, the above is an open-coded version of on_each_cpu().

so...  something like the below should tidy that up, along with numerous
other things.  Can you please check it over?



diff -puN drivers/watchdog/booke_wdt.c~watchdog-fix-booke_wdtc-on-mpc85xx-smp-system-fix drivers/watchdog/booke_wdt.c
--- a/drivers/watchdog/booke_wdt.c~watchdog-fix-booke_wdtc-on-mpc85xx-smp-system-fix
+++ a/drivers/watchdog/booke_wdt.c
@@ -1,6 +1,4 @@
 /*
- * drivers/watchdog/booke_wdt.c
- *
  * Watchdog timer for PowerPC Book-E systems
  *
  * Author: Matthew McClintock
@@ -39,7 +37,7 @@
 #define WDT_PERIOD_DEFAULT 3	/* Refer to the PPC40x and PPC4xx manuals */
 #endif				/* for timing information */
 
-u32 booke_wdt_enabled = 0;
+u32 booke_wdt_enabled;
 u32 booke_wdt_period = WDT_PERIOD_DEFAULT;
 
 #ifdef	CONFIG_FSL_BOOKE
@@ -55,19 +53,12 @@ static void __booke_wdt_ping(void *data)
 	mtspr(SPRN_TSR, TSR_ENW|TSR_WIS);
 }
 
-/*
- * booke_wdt_ping:
- */
-static __inline__ void booke_wdt_ping(void)
+static void booke_wdt_ping(void)
 {
-	smp_call_function(__booke_wdt_ping, NULL, 0, 0);
-	__booke_wdt_ping(NULL);
+	on_each_cpu(__booke_wdt_ping, NULL, 0, 0);
 }
 
-/*
- * __booke_wdt_enable:
- */
-static inline void __booke_wdt_enable(void *data)
+static void __booke_wdt_enable(void *data)
 {
 	u32 val;
 
@@ -79,10 +70,7 @@ static inline void __booke_wdt_enable(vo
 	mtspr(SPRN_TCR, val);
 }
 
-/*
- * booke_wdt_write:
- */
-static ssize_t booke_wdt_write (struct file *file, const char __user *buf,
+static ssize_t booke_wdt_write(struct file *file, const char __user *buf,
 				size_t count, loff_t *ppos)
 {
 	booke_wdt_ping();
@@ -90,15 +78,11 @@ static ssize_t booke_wdt_write (struct f
 }
 
 static struct watchdog_info ident = {
-  .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
-  .firmware_version = 0,
-  .identity = "PowerPC Book-E Watchdog",
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "PowerPC Book-E Watchdog",
 };
 
-/*
- * booke_wdt_ioctl:
- */
-static int booke_wdt_ioctl (struct inode *inode, struct file *file,
+static int booke_wdt_ioctl(struct inode *inode, struct file *file,
 			    unsigned int cmd, unsigned long arg)
 {
 	u32 tmp = 0;
@@ -106,7 +90,7 @@ static int booke_wdt_ioctl (struct inode
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
-		if (copy_to_user ((struct watchdog_info __user *) arg, &ident,
+		if (copy_to_user((struct watchdog_info __user *)arg, &ident,
 				sizeof(struct watchdog_info)))
 			return -EFAULT;
 	case WDIOC_GETSTATUS:
@@ -141,18 +125,15 @@ static int booke_wdt_ioctl (struct inode
 
 	return 0;
 }
-/*
- * booke_wdt_open:
- */
-static int booke_wdt_open (struct inode *inode, struct file *file)
+
+static int booke_wdt_open(struct inode *inode, struct file *file)
 {
 	spin_lock(&booke_wdt_lock);
 	if (booke_wdt_enabled == 0) {
 		booke_wdt_enabled = 1;
-		__booke_wdt_enable(NULL);
-		smp_call_function(__booke_wdt_enable, NULL, 0, 0);
-		printk (KERN_INFO "PowerPC Book-E Watchdog Timer Enabled (wdt_period=%d)\n",
-				booke_wdt_period);
+		on_each_cpu(__booke_wdt_enable, NULL, 0, 0);
+		printk(KERN_INFO "PowerPC Book-E Watchdog Timer Enabled "
+				"(wdt_period=%d)\n", booke_wdt_period);
 	}
 	spin_unlock(&booke_wdt_lock);
 
@@ -160,17 +141,17 @@ static int booke_wdt_open (struct inode 
 }
 
 static const struct file_operations booke_wdt_fops = {
-  .owner = THIS_MODULE,
-  .llseek = no_llseek,
-  .write = booke_wdt_write,
-  .ioctl = booke_wdt_ioctl,
-  .open = booke_wdt_open,
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.write = booke_wdt_write,
+	.ioctl = booke_wdt_ioctl,
+	.open = booke_wdt_open,
 };
 
 static struct miscdevice booke_wdt_miscdev = {
-  .minor = WATCHDOG_MINOR,
-  .name = "watchdog",
-  .fops = &booke_wdt_fops,
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &booke_wdt_fops,
 };
 
 static void __exit booke_wdt_exit(void)
@@ -178,29 +159,25 @@ static void __exit booke_wdt_exit(void)
 	misc_deregister(&booke_wdt_miscdev);
 }
 
-/*
- * booke_wdt_init:
- */
 static int __init booke_wdt_init(void)
 {
 	int ret = 0;
 
-	printk (KERN_INFO "PowerPC Book-E Watchdog Timer Loaded\n");
+	printk(KERN_INFO "PowerPC Book-E Watchdog Timer Loaded\n");
 	ident.firmware_version = cur_cpu_spec->pvr_value;
 
 	ret = misc_register(&booke_wdt_miscdev);
 	if (ret) {
-		printk (KERN_CRIT "Cannot register miscdev on minor=%d (err=%d)\n",
+		printk(KERN_CRIT "Cannot register miscdev on minor=%d: %d\n",
 				WATCHDOG_MINOR, ret);
 		return ret;
 	}
 
 	spin_lock(&booke_wdt_lock);
 	if (booke_wdt_enabled == 1) {
-		printk (KERN_INFO "PowerPC Book-E Watchdog Timer Enabled (wdt_period=%d)\n",
-				booke_wdt_period);
-		__booke_wdt_enable(NULL);
-		smp_call_function(__booke_wdt_enable, NULL, 0, 0);
+		printk (KERN_INFO "PowerPC Book-E Watchdog Timer Enabled "
+				"(wdt_period=%d)\n", booke_wdt_period);
+		on_each_cpu(__booke_wdt_enable, NULL, 0, 0);
 	}
 	spin_unlock(&booke_wdt_lock);
 
_