by Samuel Tardieu

[permalink] [raw]

Subject: Re: [PATCH] watchdog: add support for w83697hg chip

On 9/09, Samuel Tardieu wrote:

| | fd = open("/dev/watchdog", O_RDWR);
| | switch(fork())
| | {
| |
| | .. one open, two users, two processes, two CPUs
|
| Right. Thanks for the review, will fix.

Updated patch follows.

Sam

Winbond W83697HF/W83697HGHG watchdog timer

The Winbond SuperIO W83697HF/HG includes a watchdog that can count from
1 to 255 seconds (or minutes). This drivers allows the seconds mode to
be used. It exposes a standard /dev/watchdog interface. This chip is
currently being used on some motherboards designed by VIA.

By default, the module looks for a chip at I/O port 0x4e. The chip can
be configured to be at 0x2e on some motherboards, the address can be
chosen using the wdt_io module parameter. Using 0 will try to autodetect
the address.

Signed-off-by: Samuel Tardieu <[email protected]>

diff -r 92038c30d67b drivers/char/watchdog/Kconfig
--- a/drivers/char/watchdog/Kconfig Sat Sep 09 17:30:15 2006 +0200
+++ b/drivers/char/watchdog/Kconfig Sat Sep 09 17:31:47 2006 +0200
@@ -371,6 +371,21 @@ config W83627HF_WDT

Most people will say N.

+config W83697HF_WDT
+ tristate "W83697HF/W83697HG Watchdog Timer"
+ depends on WATCHDOG && X86
+ ---help---
+ This is the driver for the hardware watchdog on the W83697HF/HG
+ chipset as used in Dedibox/VIA motherboards (and likely others).
+ This watchdog simply watches your kernel to make sure it doesn't
+ freeze, and if it does, it reboots your computer after a certain
+ amount of time.
+
+ To compile this driver as a module, choose M here: the
+ module will be called w83697hf_wdt.
+
+ Most people will say N.
+
config W83877F_WDT
tristate "W83877F (EMACS) Watchdog Timer"
depends on WATCHDOG && X86
diff -r 92038c30d67b drivers/char/watchdog/Makefile
--- a/drivers/char/watchdog/Makefile Sat Sep 09 17:30:15 2006 +0200
+++ b/drivers/char/watchdog/Makefile Sat Sep 09 17:31:47 2006 +0200
@@ -51,6 +51,7 @@ obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
+obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o
obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o
obj-$(CONFIG_MACHZ_WDT) += machzwd.o
diff -r 92038c30d67b drivers/char/watchdog/w83697hf_wdt.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/char/watchdog/w83697hf_wdt.c Sat Sep 09 18:23:56 2006 +0200
@@ -0,0 +1,429 @@
+/*
+ * w83697hf/hg WDT driver
+ *
+ * (c) Copyright 2006 Samuel Tardieu <[email protected]>
+ * (c) Copyright 2006 Marcus Junker <[email protected]>
+ *
+ * Based on w83627hf_wdt which is based on wadvantechwdt.c
+ * which is based on wdt.c.
+ * Original copyright messages:
+ *
+ * (c) Copyright 2003 P?draig Brady <[email protected]>
+ *
+ * (c) Copyright 2000-2001 Marek Michalkiewicz <[email protected]>
+ *
+ * (c) Copyright 1996 Alan Cox <[email protected]>, All Rights Reserved.
+ * http://www.redhat.com
+ *
+ * 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.
+ *
+ * Neither Alan Cox nor CymruNet Ltd. admit liability nor provide
+ * warranty for any of this software. This material is provided
+ * "AS-IS" and at no charge.
+ *
+ * (c) Copyright 1995 Alan Cox <[email protected]>
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/ioport.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+#include <linux/init.h>
+
+#include <asm/io.h>
+#include <asm/semaphore.h>
+#include <asm/uaccess.h>
+#include <asm/system.h>
+
+#define WATCHDOG_NAME "w83697hf/hg WDT"
+#define PFX WATCHDOG_NAME ": "
+#define WATCHDOG_TIMEOUT 60 /* 60 sec default timeout */
+
+static unsigned long wdt_is_open;
+static char expect_close;
+static DECLARE_MUTEX(dev_lock); /* Protect sequential operations */
+
+/* You must set this - there is no sane way to probe for this board. */
+static int wdt_io = 0x2e;
+module_param(wdt_io, int, 0);
+MODULE_PARM_DESC(wdt_io, "w83697hf/hg WDT io port (default 0x2e, 0 = autodetect)");
+
+static int timeout = WATCHDOG_TIMEOUT; /* in seconds */
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. 1<= timeout <=255, default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=CONFIG_WATCHDOG_NOWAYOUT)");
+
+/*
+ * Kernel methods.
+ */
+
+#define W83697HF_EFER (wdt_io+0) /* Extended function enable register */
+#define W83697HF_EFIR (wdt_io+0) /* Extended function index register */
+#define W83697HF_EFDR (wdt_io+1) /* Extended function data register */
+
+static inline void
+w83697hf_unlock(void)
+{
+ outb_p(0x87, W83697HF_EFER);
+ outb_p(0x87, W83697HF_EFER);
+}
+
+static inline void
+w83697hf_lock(void)
+{
+ outb_p(0xAA, W83697HF_EFER);
+}
+
+/*
+ * The three functions w83697hf_get_reg(), w83697_set_reg() and
+ * wdt_ctrl() must be called with the device unlocked.
+ */
+
+static unsigned char
+w83697hf_get_reg(unsigned char reg)
+{
+ outb_p(reg, W83697HF_EFIR);
+ return inb_p(W83697HF_EFDR);
+}
+
+static void
+w83697hf_set_reg(unsigned char reg, unsigned char data)
+{
+ outb_p(reg, W83697HF_EFIR);
+ outb_p(data, W83697HF_EFDR);
+}
+
+static void
+wdt_ctrl(int timeout)
+{
+ w83697hf_set_reg(0xF4, timeout);
+}
+
+static void
+w83697hf_select_wdt(void)
+{
+ down(&dev_lock);
+ w83697hf_unlock();
+ w83697hf_set_reg(0x07, 0x08); /* Switch to logic device 8 */
+}
+
+static inline void
+w83697hf_deselect_wdt(void)
+{
+ w83697hf_lock();
+ up(&dev_lock);
+}
+
+static void
+wdt_ping(void)
+{
+ w83697hf_select_wdt();
+ wdt_ctrl(timeout);
+ w83697hf_deselect_wdt();
+}
+
+static void
+wdt_enable(void)
+{
+ unsigned char bbuf;
+
+ w83697hf_select_wdt();
+
+ bbuf = w83697hf_get_reg(0x29);
+ bbuf &= ~0x60;
+ bbuf |= 0x20;
+ w83697hf_set_reg(0x29, bbuf); /* Set pin 119 to WDTO# mode */
+
+ bbuf = w83697hf_get_reg(0xF3);
+ bbuf &= ~0x04;
+ w83697hf_set_reg(0xF3, bbuf); /* Count mode is seconds */
+ w83697hf_set_reg(0x30, 1); /* Enable timer */
+
+ w83697hf_deselect_wdt();
+}
+
+static void
+wdt_disable(void)
+{
+ w83697hf_select_wdt();
+ w83697hf_set_reg(0x30, 0); /* Disable timer */
+ wdt_ctrl(0);
+ w83697hf_deselect_wdt();
+}
+
+static int
+wdt_set_heartbeat(int t)
+{
+ if ((t < 1) || (t > 255))
+ return -EINVAL;
+
+ timeout = t;
+ return 0;
+}
+
+static ssize_t
+wdt_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+ if (count) {
+ if (!nowayout) {
+ size_t i;
+
+ expect_close = 0;
+
+ for (i = 0; i != count; i++) {
+ char c;
+ if (get_user(c, buf+i))
+ return -EFAULT;
+ if (c == 'V')
+ expect_close = 42;
+ }
+ }
+ wdt_ping();
+ }
+ return count;
+}
+
+static int
+wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ void __user *argp = (void __user *)arg;
+ int __user *p = argp;
+ int new_timeout;
+ static struct watchdog_info ident = {
+ .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
+ .firmware_version = 1,
+ .identity = "W83697HF WDT",
+ };
+
+ switch (cmd) {
+ case WDIOC_GETSUPPORT:
+ if (copy_to_user(argp, &ident, sizeof(ident)))
+ return -EFAULT;
+ break;
+
+ case WDIOC_GETSTATUS:
+ case WDIOC_GETBOOTSTATUS:
+ return put_user(0, p);
+
+ case WDIOC_KEEPALIVE:
+ wdt_ping();
+ break;
+
+ case WDIOC_SETTIMEOUT:
+ if (get_user(new_timeout, p))
+ return -EFAULT;
+ if (wdt_set_heartbeat(new_timeout))
+ return -EINVAL;
+ wdt_ping();
+ /* Fall */
+
+ case WDIOC_GETTIMEOUT:
+ return put_user(timeout, p);
+
+ case WDIOC_SETOPTIONS:
+ {
+ int options, retval = -EINVAL;
+
+ if (get_user(options, p))
+ return -EFAULT;
+
+ if (options & WDIOS_DISABLECARD) {
+ wdt_disable();
+ retval = 0;
+ }
+
+ if (options & WDIOS_ENABLECARD) {
+ wdt_ping();
+ retval = 0;
+ }
+
+ return retval;
+ }
+
+ default:
+ return -ENOTTY;
+ }
+ return 0;
+}
+
+static int
+wdt_open(struct inode *inode, struct file *file)
+{
+ if (test_and_set_bit(0, &wdt_is_open))
+ return -EBUSY;
+ /*
+ * Activate
+ */
+
+ wdt_enable();
+ return nonseekable_open(inode, file);
+}
+
+static int
+wdt_close(struct inode *inode, struct file *file)
+{
+ if (expect_close == 42) {
+ wdt_disable();
+ } else {
+ printk(KERN_CRIT PFX "Unexpected close, not stopping watchdog!\n");
+ wdt_ping();
+ }
+ expect_close = 0;
+ clear_bit(0, &wdt_is_open);
+ return 0;
+}
+
+/*
+ * Notifier for system down
+ */
+
+static int
+wdt_notify_sys(struct notifier_block *this, unsigned long code,
+ void *unused)
+{
+ if (code == SYS_DOWN || code == SYS_HALT) {
+ /* Turn the WDT off */
+ wdt_disable();
+ }
+ return NOTIFY_DONE;
+}
+
+/*
+ * Kernel Interfaces
+ */
+
+static struct file_operations wdt_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .write = wdt_write,
+ .ioctl = wdt_ioctl,
+ .open = wdt_open,
+ .release = wdt_close,
+};
+
+static struct miscdevice wdt_miscdev = {
+ .minor = WATCHDOG_MINOR,
+ .name = "watchdog",
+ .fops = &wdt_fops,
+};
+
+/*
+ * The WDT needs to learn about soft shutdowns in order to
+ * turn the timebomb registers off.
+ */
+
+static struct notifier_block wdt_notifier = {
+ .notifier_call = wdt_notify_sys,
+};
+
+static int
+w83697hf_init(void)
+{
+ if (!request_region(wdt_io, 2, WATCHDOG_NAME)) {
+ printk(KERN_ERR PFX "I/O address 0x%x already in use\n", wdt_io);
+ return -EIO;
+ }
+
+ printk(KERN_DEBUG PFX "Looking for watchdog at address 0x%x\n", wdt_io);
+ w83697hf_unlock();
+ if (w83697hf_get_reg(0x20) == 0x60) {
+ printk(KERN_INFO PFX "watchdog found at address 0x%x\n", wdt_io);
+ w83697hf_lock();
+ return 0;
+ }
+ w83697hf_lock(); /* Reprotect in case it was a compatible device */
+
+ printk(KERN_INFO PFX "watchdog not found at address 0x%x\n", wdt_io);
+ release_region(wdt_io, 2);
+ return -EIO;
+}
+
+static int __init
+wdt_init(void)
+{
+ int ret, autodetect;
+
+ printk(KERN_INFO PFX "WDT driver for W83697HF/HG initializing\n");
+
+ autodetect = wdt_io == 0;
+ if (autodetect)
+ wdt_io = 0x2e;
+
+ if (!w83697hf_init())
+ goto found;
+
+ if (autodetect) {
+ wdt_io = 0x4e;
+ if (!w83697hf_init())
+ goto found;
+ }
+
+ printk(KERN_ERR PFX "No W83697HF/HG could be found\n");
+ ret = -EIO;
+ goto out;
+
+ found:
+
+ w83697hf_select_wdt();
+ wdt_ctrl(0); /* Disable watchdog until first use */
+ w83697hf_deselect_wdt();
+
+ if (wdt_set_heartbeat(timeout)) {
+ wdt_set_heartbeat(WATCHDOG_TIMEOUT);
+ printk(KERN_INFO PFX "timeout value must be 1<=timeout<=255, using %d\n",
+ WATCHDOG_TIMEOUT);
+ }
+
+ ret = register_reboot_notifier(&wdt_notifier);
+ if (ret != 0) {
+ printk (KERN_ERR PFX "cannot register reboot notifier (err=%d)\n",
+ ret);
+ goto unreg_regions;
+ }
+
+ ret = misc_register(&wdt_miscdev);
+ if (ret != 0) {
+ printk (KERN_ERR PFX "cannot register miscdev on minor=%d (err=%d)\n",
+ WATCHDOG_MINOR, ret);
+ goto unreg_reboot;
+ }
+
+ printk (KERN_INFO PFX "initialized. timeout=%d sec (nowayout=%d)\n",
+ timeout, nowayout);
+
+ out:
+ return ret;
+ unreg_reboot:
+ unregister_reboot_notifier(&wdt_notifier);
+ unreg_regions:
+ release_region(wdt_io, 2);
+ goto out;
+}
+
+static void __exit
+wdt_exit(void)
+{
+ misc_deregister(&wdt_miscdev);
+ unregister_reboot_notifier(&wdt_notifier);
+ release_region(wdt_io, 2);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Samuel Tardieu <[email protected]>");
+MODULE_DESCRIPTION("w83697hf WDT driver");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

2006-09-09 18:03:29

On Thu, 14 Sep 2006 01:13:03 -0600 Jim Cromie wrote:

> diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/isa/Kconfig 6locks-2/drivers/isa/Kconfig
> --- 6locks-1/drivers/isa/Kconfig 1969-12-31 17:00:00.000000000 -0700
> +++ 6locks-2/drivers/isa/Kconfig 2006-09-13 09:54:18.000000000 -0600
> @@ -0,0 +1,7 @@
> +
> +config SUPERIO_LOCKS
> + tristate "Super-IO port sharing"
> + help
> + this module provides locks for use by drivers which need to

This

> + share access to a multi-function device via its superio port,
> + and which register that port.

> diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/isa/superio_locks.c 6locks-2/drivers/isa/superio_locks.c
> --- 6locks-1/drivers/isa/superio_locks.c 1969-12-31 17:00:00.000000000 -0700
> +++ 6locks-2/drivers/isa/superio_locks.c 2006-09-13 14:56:32.000000000 -0600
> @@ -0,0 +1,169 @@
> +
> +/**
> + module provides a means for modules to register their use of a
> + Super-IO port, and provides an access-lock for the registering
> + modules to use to coordinate with each other. Consider it a
> + parking-attendant's key-board. Design is perhaps ISA centric,
> + maybe formalize this, with (platform|isa)_driver.
> +*/

Two comments about that comment:

a. Kernel long-comment style is:

/*
* This module provides a means for modules to register
* their use of a Super-IO port, ...
*/

b. Don't use "/**" to begin a comment block unless the comment block
contains kernel-doc formatted comments. (This one does not.)

> +
> +/* superio_get() checks whether the expected SuperIO device is
> + present at a specific cmd-addr. Use in loop to scan.
> +*/

For functions that are not static (and when you want this merged,
not just RFC), please include kernel-doc comments describing the
function and its parameters. See Documentation/kernel-doc-nano-HOWTO.txt
for more info.

> +struct superio* superio_get(u16 cmd_addr, u8 dev_id_addr,
> + u8 want_devid)
> +{
> + int slot, rc, mydevid;
> +
> + mutex_lock(&reservation_lock);
> +
> + /* share any already allocated lock for this cmd_addr, device-id */
> + for (slot = 0; slot < max_locks; slot++) {
> + if (sio_locks[slot].users
> + && cmd_addr == sio_locks[slot].sioaddr
> + && want_devid == sio_locks[slot].devid) {
> +
> + if (sio_locks[slot].users == 255) {
> + dprintk("too many drivers sharing port %x\n", cmd_addr);
> + mutex_unlock(&reservation_lock);
> + return 0;
> + }
> + sio_locks[slot].users++;
> + dprintk("sharing port:%x dev:%x users:%d\n",
> + cmd_addr, want_devid, sio_locks[slot].users);
> + mutex_unlock(&reservation_lock);
> + return &sio_locks[slot];
> + }
> + }
> + /* read the device-id-address */
> + outb(dev_id_addr, cmd_addr);
> + mydevid = inb(cmd_addr+1);
> +
> + /* but 1st, check that the cmd register remembers the val just written */
> + rc = inb(cmd_addr);
> + if (rc != dev_id_addr) {
> + dprintk("superio_cmdaddr %x absent %d\n", cmd_addr, rc);
> + mutex_unlock(&reservation_lock);
> + return NULL;
> + }
> + /* test for the desired device id value */
> + if (mydevid != want_devid) {
> + mutex_unlock(&reservation_lock);
> + return NULL;
> + }
> + /* find 1st unused slot */
> + for (slot = 0; slot < max_locks; slot++)
> + if (!sio_locks[slot].users)
> + break;
> +
> + if (slot >= max_locks) {
> + printk(KERN_ERR "No superio-locks left. increase max_locks\n");
> + mutex_unlock(&reservation_lock);
> + return NULL;
> + }
> + dprintk("allocating slot %d, addr %x for device %x\n",
> + slot, cmd_addr, want_devid);
> +
> + sio_locks[slot].sioaddr = cmd_addr;
> + sio_locks[slot].devid = want_devid;
> + sio_locks[slot].users = 1;
> + num_locks++;
> +
> + mutex_unlock(&reservation_lock);
> + return &sio_locks[slot];
> +}
> +EXPORT_SYMBOL_GPL(superio_get);
> +
> +/* array args must be null terminated */

Also needs kernel-doc function comment header.

> +struct superio* superio_find(u16 cmd_addrs[], u8 devid_addr,
> + u8 want_devids[])
> +{
> + int i, j;
> + struct superio* gate;
> +
> + for (i = 0; cmd_addrs[i]; i++) {
> + for (j = 0; want_devids[j]; j++) {
> + gate = superio_get(cmd_addrs[i], devid_addr,
> + want_devids[j]);
> + if (gate) {
> + dprintk("found devid:%x port:%x\n",
> + want_devids[j], cmd_addrs[i]);
> + return gate;
> + } else
> + dprintk("no devid:%x at port:%x\n",
> + want_devids[j], cmd_addrs[i]);
> + }
> + }
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(superio_find);
> +
> +void superio_release(struct superio* const gate)

Ditto.

> +{
> + if (gate < &sio_locks[0] || gate >= &sio_locks[max_locks]) {
> + printk(KERN_ERR
> + " superio: attempt to release corrupted superio-lock"
> + " %p vs %p\n", gate, &sio_locks);
> + return;
> + }
> + if (!(--gate->users))
> + dprintk("releasing last user of superio-port %x\n", gate->sioaddr);
> + return;
> +}
> +EXPORT_SYMBOL_GPL(superio_release);
> +

> diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/include/linux/superio-locks.h 6locks-2/include/linux/superio-locks.h
> --- 6locks-1/include/linux/superio-locks.h 1969-12-31 17:00:00.000000000 -0700
> +++ 6locks-2/include/linux/superio-locks.h 2006-09-13 14:21:08.000000000 -0600
> @@ -0,0 +1,55 @@
> +#include <linux/mutex.h>
> +#include <asm/io.h>
> +
> +/* Super-IO ports are found in low-pin-count hardware (typically ISA,
> + any others ?). They usually provide access to many functional
> + units, so many drivers must share the superio port. This struct
> + provides a lock that allows the drivers to coordinate access to that
> + port.
> +*/

Use long-comment style, please.

> +struct superio {
> + struct mutex lock; /* lock shared amongst user drivers */
> + u16 sioaddr; /* port's tested cmd-address */
> + u8 devid; /* devid found by the registering driver */
> + u8 users; /* I cant imagine >256 user drivers */
> +};
> +
> +/* array args must be null terminated */
> +struct superio* superio_find(u16 sioaddrs[], u8 devid_addr, u8 devid_vals[]);
> +struct superio* superio_get(u16 sioaddr, u8 devid_addr, u8 devid_val);
> +void superio_release(struct superio* const gate);
> +
> +/* these locking ops do not address the idling & activation of some
> + superio devices, which will, once 'locked', ignore accesses until
> + the 'unlock' sequence is done 1st. Unfortunately these sequences
> + vary by device, and in any case don't protect 2 drivers from
> + stepping on each other's operations.
> +
> + Callbacks are a possible approach, but every driver using a device
> + would have to provide them, and only the 1st loaded module would
> + actually succeed in registering them. Furthermore, if any driver
> + accessing a port uses the idle/activate sequences, they all must.
> + On the whole, this is complexity w/o benefit.
> +*/

long-comment style, please.

---
~Randy