Subject: Re: [PATCH] Added MIPS RM9K watchdog driver

From: Wim Van Sebroeck <[email-masked]>
Date: Wed, 1 Nov 2006 19:46:33 +0100

Hi All,

Can you all review the following watchdog driver? I want to include
it in my linux-2.6-watchdog-mm tree, but would appreciate it if you
could have all a look at it again.

Thomas: I moved start and stop code into seperate functions. I also
deleted the #include <rm9k_wdt.h> because the file doesn't exist.
Note that the shutdown function of the platform_device_driver is
similar to a reboot notifier.

Thanks in advance,
Wim.

--------------------------------------------------------------------------------
/*
 *  Watchdog implementation for GPI h/w found on PMC-Sierra RM9xxx
 *  chips.
 *
 *  Copyright (C) 2004 by Basler Vision Technologies AG
 *  Author: Thomas Koeller <[email-masked]>
 *
 *  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 Free Software Foundation; either version 2 of the License, or
 *  (at your option) any later version.
 *
 *  This program is distributed in the hope that it will be useful,
 *  but WITHOUT ANY WARRANTY; without even the implied warranty of
 *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *  GNU General Public License for more details.
 *
 *  You should have received a copy of the GNU General Public License
 *  along with this program; if not, write to the Free Software
 *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 */

#include <linux/platform_device.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/interrupt.h>
#include <linux/fs.h>
#include <linux/reboot.h>
#include <linux/notifier.h>
#include <linux/miscdevice.h>
#include <linux/watchdog.h>
#include <asm/io.h>
#include <asm/atomic.h>
#include <asm/processor.h>
#include <asm/uaccess.h>
#include <asm/system.h>
#include <asm/rm9k-ocd.h>

#define CLOCK                  125000000
#define MAX_TIMEOUT_SECONDS    32
#define CPCCR                  0x0080
#define CPGIG1SR               0x0044
#define CPGIG1ER               0x0054

/* Function prototypes */
static int __init wdt_gpi_probe(struct device *);
static int __exit wdt_gpi_remove(struct device *);
static void wdt_gpi_start(void);
static void wdt_gpi_stop(void);
static void wdt_gpi_set_timeout(unsigned int);
static int wdt_gpi_open(struct inode *, struct file *);
static int wdt_gpi_release(struct inode *, struct file *);
static ssize_t wdt_gpi_write(struct file *, const char __user *, size_t, loff_t *);
static long wdt_gpi_ioctl(struct file *, unsigned int, unsigned long);
static const struct resource *wdt_gpi_get_resource(struct platform_device *, const char *, unsigned int);
static int wdt_gpi_notify(struct notifier_block *, unsigned long, void *);
static irqreturn_t wdt_gpi_irqhdl(int, void *, struct pt_regs *);

static const char wdt_gpi_name[] = "wdt_gpi";
static atomic_t opencnt;
static int expect_close;
static int locked = 0;

/* These are set from device resources */
static void __iomem * wd_regs;
static unsigned int wd_irq, wd_ctr;

/* Module arguments */
static int timeout = MAX_TIMEOUT_SECONDS;
module_param(timeout, int, 0444);
static unsigned long resetaddr = 0xbffdc200;
module_param(resetaddr, ulong, 0444);
static unsigned long flagaddr = 0xbffdc104;
module_param(flagaddr, ulong, 0444);
static int powercycle = 0;
module_param(powercycle, bool, 0444);

static int nowayout = WATCHDOG_NOWAYOUT;
module_param(nowayout, bool, 0444);

static struct file_operations fops = {
	.owner		= THIS_MODULE,
	.open		= wdt_gpi_open,
	.release	= wdt_gpi_release,
	.write		= wdt_gpi_write,
	.unlocked_ioctl	= wdt_gpi_ioctl,
};

static struct miscdevice miscdev = {
	.minor		= WATCHDOG_MINOR,
	.name		= wdt_gpi_name,
	.fops		= &fops,
};

static struct device_driver wdt_gpi_driver = {
	.name		= (char *) wdt_gpi_name,
	.bus		= &platform_bus_type,
	.owner		= THIS_MODULE,
	.probe		= wdt_gpi_probe,
	.remove		= __exit_p(wdt_gpi_remove),
	.shutdown	= NULL,
	.suspend	= NULL,
	.resume		= NULL,
};

static struct notifier_block wdt_gpi_shutdown = {
	.notifier_call	= wdt_gpi_notify,
};

static const struct resource *
wdt_gpi_get_resource(struct platform_device *pdv, const char *name,
		      unsigned int type)
{
	char buf[80];
	if (snprintf(buf, sizeof buf, "%s_0", name) >= sizeof buf)
		return NULL;
	return platform_get_resource_byname(pdv, type, buf);
}

/* No hotplugging on the platform bus - use __init */
static int __init wdt_gpi_probe(struct device *dev)
{
	int res;
	struct platform_device * const pdv = to_platform_device(dev);
	const struct resource
		* const rr = wdt_gpi_get_resource(pdv, WDT_RESOURCE_REGS,
						  IORESOURCE_MEM),
		* const ri = wdt_gpi_get_resource(pdv, WDT_RESOURCE_IRQ,
						  IORESOURCE_IRQ),
		* const rc = wdt_gpi_get_resource(pdv, WDT_RESOURCE_COUNTER,
						  0);

	if (unlikely(!rr || !ri || !rc))
		return -ENXIO;

	wd_regs = ioremap_nocache(rr->start, rr->end + 1 - rr->start);
	if (unlikely(!wd_regs))
		return -ENOMEM;
	wd_irq = ri->start;
	wd_ctr = rc->start;
	res = misc_register(&miscdev);
	if (res)
		iounmap(wd_regs);
	else
		register_reboot_notifier(&wdt_gpi_shutdown);
	return res;
}

static int __exit wdt_gpi_remove(struct device *dev)
{
	int res;

	unregister_reboot_notifier(&wdt_gpi_shutdown);
	res = misc_deregister(&miscdev);
	iounmap(wd_regs);
	wd_regs = NULL;
	return res;
}

static void wdt_gpi_start(void)
{
	u32 reg;

	lock_titan_regs();
	reg = titan_readl(CPGIG1ER);
	titan_writel(reg | (0x100 << wd_ctr), CPGIG1ER);
	iob();
	unlock_titan_regs();
}

static void wdt_gpi_stop(void)
{
	u32 reg;

	lock_titan_regs();
	reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
	titan_writel(reg, CPCCR);
	reg = titan_readl(CPGIG1ER);
	titan_writel(reg & ~(0x100 << wd_ctr), CPGIG1ER);
	iob();
	unlock_titan_regs();
}

static void wdt_gpi_set_timeout(unsigned int to)
{
	u32 reg;
	const u32 wdval = (to * CLOCK) & ~0x0000000f;

	lock_titan_regs();
	reg = titan_readl(CPCCR) & ~(0xf << (wd_ctr * 4));
	titan_writel(reg, CPCCR);
	wmb();
	__raw_writel(wdval, wd_regs + 0x0000);
	wmb();
	titan_writel(reg | (0x2 << (wd_ctr * 4)), CPCCR);
	wmb();
	titan_writel(reg | (0x5 << (wd_ctr * 4)), CPCCR);
	iob();
	unlock_titan_regs();
}

static int wdt_gpi_open(struct inode *inode, struct file *file)
{
	int res;

	if (unlikely(0 > atomic_dec_if_positive(&opencnt)))
		return -EBUSY;

	expect_close = 0;
	if (locked) {
		module_put(THIS_MODULE);
		free_irq(wd_irq, &miscdev);
		locked = 0;
	}

	res = request_irq(wd_irq, wdt_gpi_irqhdl, SA_SHIRQ | SA_INTERRUPT,
			  wdt_gpi_name, &miscdev);
	if (unlikely(res))
		return res;

	wdt_gpi_set_timeout(timeout);
	wdt_gpi_start();

	printk(KERN_INFO "%s: watchdog started, timeout = %u seconds\n",
		wdt_gpi_name, timeout);
	return nonseekable_open(inode, file);
}

static int wdt_gpi_release(struct inode *i, struct file *f)
{
	if (nowayout) {
		printk(KERN_NOTICE "%s: no way out - watchdog left running\n",
			wdt_gpi_name);
		__module_get(THIS_MODULE);
		locked = 1;
	} else {
		if (expect_close) {
			wdt_gpi_stop();
			free_irq(wd_irq, &miscdev);
			printk(KERN_INFO "%s: watchdog stopped\n", wdt_gpi_name);
		} else {
			printk(KERN_NOTICE "%s: unexpected close() -"
				" watchdog left running\n",
				wdt_gpi_name);
			wdt_gpi_set_timeout(timeout);
			__module_get(THIS_MODULE);
			locked = 1;
		}
	}

	atomic_inc(&opencnt);
	return 0;
}

static ssize_t
wdt_gpi_write(struct file *f, const char __user *d, size_t s, loff_t *o)
{
	char val;

	wdt_gpi_set_timeout(timeout);
	expect_close = (s > 0) && !get_user(val, d) && (val == 'V');
	return s ? 1 : 0;
}

static long
wdt_gpi_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
{
	long res = -ENOTTY;
	const long size = _IOC_SIZE(cmd);
	int stat;
	static struct watchdog_info wdinfo = {
		.identity               = "RM9xxx/GPI watchdog",
		.firmware_version       = 0,
		.options                = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING
	};

	if (unlikely(_IOC_TYPE(cmd) != WATCHDOG_IOCTL_BASE))
		return -ENOTTY;

	if ((_IOC_DIR(cmd) & _IOC_READ)
	    && !access_ok(VERIFY_WRITE, arg, size))
		return -EFAULT;

	if ((_IOC_DIR(cmd) & _IOC_WRITE)
	    && !access_ok(VERIFY_READ, arg, size))
		return -EFAULT;

	expect_close = 0;

	switch (cmd) {
	case WDIOC_GETSUPPORT:
		wdinfo.options = nowayout ?
			WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING :
			WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE;
		res = __copy_to_user((void __user *)arg, &wdinfo, size) ?
			-EFAULT : size;
		break;

	case WDIOC_GETSTATUS:
		break;

	case WDIOC_GETBOOTSTATUS:
		stat = (*(volatile char *) flagaddr & 0x01)
			? WDIOF_CARDRESET : 0;
		res = __copy_to_user((void __user *)arg, &stat, size) ?
			-EFAULT : size;
		break;

	case WDIOC_SETOPTIONS:
		break;

	case WDIOC_KEEPALIVE:
		wdt_gpi_set_timeout(timeout);
		res = size;
		break;

	case WDIOC_SETTIMEOUT:
		{
			int val;
			if (unlikely(__copy_from_user(&val, (const void __user *) arg,
					size))) {
				res = -EFAULT;
				break;
			}

			if (val > 32)
				val = 32;
			timeout = val;
			wdt_gpi_set_timeout(val);
			res = size;
			printk("%s: timeout set to %u seconds\n",
				wdt_gpi_name, timeout);
		}
		break;

	case WDIOC_GETTIMEOUT:
		res = __copy_to_user((void __user *) arg, &timeout, size) ?
			-EFAULT : size;
		break;
	}

	return res;
}

static irqreturn_t wdt_gpi_irqhdl(int irq, void *ctxt, struct pt_regs *regs)
{
	if (!unlikely(__raw_readl(wd_regs + 0x0008) & 0x1))
		return IRQ_NONE;
	__raw_writel(0x1, wd_regs + 0x0008);

	printk(KERN_WARNING "%s: watchdog expired - resetting system\n",
		wdt_gpi_name);

	*(volatile char *) flagaddr |= 0x01;
	*(volatile char *) resetaddr = powercycle ? 0x01 : 0x2;
	iob();
	while (1)
		cpu_relax();
}

static int
wdt_gpi_notify(struct notifier_block *this, unsigned long code, void *unused)
{
	if(code == SYS_DOWN || code == SYS_HALT) {
		wdt_gpi_stop();
	}
	return NOTIFY_DONE;
}

static int __init wdt_gpi_init_module(void)
{
	atomic_set(&opencnt, 1);
	if (timeout > MAX_TIMEOUT_SECONDS)
		timeout = MAX_TIMEOUT_SECONDS;
	return driver_register(&wdt_gpi_driver);
}

static void __exit wdt_gpi_cleanup_module(void)
{
	driver_unregister(&wdt_gpi_driver);
}

module_init(wdt_gpi_init_module);
module_exit(wdt_gpi_cleanup_module);

MODULE_AUTHOR("Thomas Koeller <[email-masked]>");
MODULE_DESCRIPTION("Basler eXcite watchdog driver for gpi devices");
MODULE_VERSION("0.1");
MODULE_LICENSE("GPL");
MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
MODULE_PARM_DESC(resetaddr, "Address to write to to force a reset");
MODULE_PARM_DESC(flagaddr, "Address to write to boot flags to");
MODULE_PARM_DESC(nowayout, "Watchdog cannot be disabled once started");
MODULE_PARM_DESC(powercycle, "Cycle power if watchdog expires");

---

From: Ralf Baechle <[email-masked]>
Date: Wed, 1 Nov 2006 18:50:36 +0000

> deleted the #include <rm9k_wdt.h> because the file doesn't exist.

You just didn't see it, include/asm-mips/mach-excite/rm9k_wdt.h.

  Ralf

---

From: Wim Van Sebroeck <[email-masked]>
Date: Wed, 1 Nov 2006 20:11:12 +0100

Hi Ralf,

> You just didn't see it, include/asm-mips/mach-excite/rm9k_wdt.h.

Thanks, didn't see it indeed :-). I'll include that again.
Any other remarks still?

Greetings,
Wim.

---

From: Sam Ravnborg <[email-masked]>
Date: Wed, 1 Nov 2006 21:43:53 +0100

Hi Wim.

Not much of my area so only some nitpicking stuff.

>  *  This program is free software; you can redistribute it and/or modify
We have COPYING in top level directory. No need to include GPL here.

> /* Function prototypes */

Too many empty lines. Get it down to 2. Seen on many places.

> static void wdt_gpi_set_timeout(unsigned int);
3 prototpyes not needed

> static const struct resource *wdt_gpi_get_resource(struct platform_device *, const char *, unsigned int);
80 char coloum
Prototype not needed.

> static void __iomem * wd_regs;
Good to see code annotated.
I assume sparse did not give any warnings on this code?

> module_param(nowayout, bool, 0444);

Locate parameter descriptions clode to parameter definition - not in
 bottom of file.

> 	.name		= (char *) wdt_gpi_name,
If this cast is really needed then struct device_driver ought to be updated?

> 		* const rc = wdt_gpi_get_resource(pdv, WDT_RESOURCE_COUNTER,

Separate variable definition and assignment => nicer code.

> wdt_gpi_ioctl(struct file *f, unsigned int cmd, unsigned long arg)

arg is a __user pointer - why hide this fact?

> 		res = __copy_to_user((void __user *)arg, &wdinfo, size) ?
then you get rid of this cast.
And code this as if () else
Using ?: is just ugly here.

> 		stat = (*(volatile char *) flagaddr & 0x01)

Use of volatile almost always indicate a bug.
Please explain why volatile is needed and consider the other options.

> 			printk("%s: timeout set to %u seconds\n",

I just noticed this pringk miss a <KERNEL_DEBUG> or similar specifier.
I guess this is all of them.

> 	__raw_writel(0x1, wd_regs + 0x0008);
Magics suchs as '0x0008' deserve a comment.

> 	*(volatile char *) resetaddr = powercycle ? 0x01 : 0x2;

volatile again...

> MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
These five tags belongs in the start of the file.

> MODULE_PARM_DESC(powercycle, "Cycle power if watchdog expires");
Same here - but already noted.

	Sam

---

From: Randy Dunlap <[email-masked]>
Date: Wed, 1 Nov 2006 22:11:25 -0800

> static const struct resource *wdt_gpi_get_resource(struct platform_device *, const char *, unsigned int);

Some lines too long (stay <= 80 columns).

> static const char wdt_gpi_name[] = "wdt_gpi";

Don't need to init to 0.

> static int powercycle = 0;

no need to init to 0.

> 	wd_regs = ioremap_nocache(rr->start, rr->end + 1 - rr->start);

There's no way to return the resources on failure?

> 	if (unlikely(0 > atomic_dec_if_positive(&opencnt)))

Style: Instead of
		if (constant op function_or_variable)
we prefer
		if (function_or_variable op constant)

> 			if (unlikely(__copy_from_user(&val, (const void __user *) arg,

There's a #defined constant for that "32".
Please use it.

> 	printk(KERN_WARNING "%s: watchdog expired - resetting system\n",

I would expect a KERN_ALERT or KERN_EMERG or KERN_CRIT there...

> 	if(code == SYS_DOWN || code == SYS_HALT) {

Space between if and (.

> 		wdt_gpi_stop();

No braces around one-statement blocks.

> 	return NOTIFY_DONE;

---
~Randy

---

From: Ralf Baechle <[email-masked]>
Date: Thu, 2 Nov 2006 14:04:51 +0000

> There's no way to return the resources on failure?

MIPS drivers (and this one is specific to a particular MIPS SOC) are
generally a bit sloopy about checking of return values of ioremap because
ioremap is only doing some address arithmetic but no allocations that
actually could fail.  So for 64-bit kernels or addresses below 0x20000000
on a 32-bit system ioremap cannot fail.  In the same cases ioremap happens
to be a no-op because where nothing was allocated nothing needs to be
freed.

> > 			if (unlikely(__copy_from_user(&val, (const void __user *) arg,

Note to self, __copy_from_user and gang are generally assume to not
return an error so it might be a good idea to move that unlikely() into
the macro definitions.

  Ralf