Re: [DRIVER][RFC] SC1200 Watchdog driver

Christer Weinigel
2002-02-21T12:58:08Z

[-- Attachment #1: Type: text/plain, Size: 2673 bytes --]

Jeff Garzik wrote:
> MODULE_PARM_DESC would be nice

Done.

> > static void scx200_watchdog_update_margin(void)
> > {
> > printk(KERN_INFO "%s: timer margin %d seconds\n", name, margin);
> > wdto_restart = 32768 / 1024 * margin;
> > scx200_watchdog_ping();
> > }
>
> if you can turn multiplication and division of powers-of-2 into left and
> right shifts, other simplications sometimes follow. Certainly you want
> to avoid division especially and multiplication also if possible.

Since this is only called on initialization I'm not overly concerned
with performance here, I prefer code clarity. This ought to be
optimized by gcc anyways.

> now, a policy question -- do you want to fail or simply put to sleep
> multiple openers? if you want to fail, this should be ok I think. if
> you want to sleep, you can look at sound/oss/* in 2.5.x or
> drivers/sound/* in 2.4.x for some examples of semaphore use on
> open(2).

I'm not even sure if single-open sematics are neccesary at all, but I
copied most of the interface from wdt285.c so I copied this too. The
watchdog API seems to be a rather ad hoc thing. For example I just
noticed that the WDIOC_SETTIMEOUT call probably takes a parameter
which seems to be minutes, not seconds. "Someone (tm)" ought to write
a more formal API specification.

> I wonder why 'name' is not simply a macro defining a string constant?
> Oh yeah, it matters very little. You might want to make 'name' const,
> though.

Because "%s: " is less text than "scx200_watchdog" and I'm not sure if
gcc is able to merge duplicate strings. Not much of a difference.

> > static struct notifier_block scx200_watchdog_notifier =
> > {
> > scx200_watchdog_notify_sys,
> > NULL,
> > 0
> > };
>
> use name:value style of struct initialization, and omit any struct
> members which are 0/NULL (that's implicit).

Done. I also changed the notifier codes that cause the watchdog to
shut down to something that seems more useful.

> > static int __init scx200_watchdog_init(void)
> > {
> > int r;
>
> Here's a big one, I still don't like this lack of probing in the
> driver. Sure we have "probed elsewhere", but IMO each driver like this
> one needs to check -something- to ensure that SC1200 hardware is
> present. Otherwise, a random user from a distro-that-builds-all-drivers
> might "modprobe sc1200_watchdog" and things go boom.

You're right, I just assumed that nobody would load this driver unless
they are on a SCx200 system. Done. I'll update all the other drivers
too.

/Christer (off to lunch)

--
Blatant plug: I'm a freelance consultant looking for interesting work.

[-- Attachment #2: d.diff --]
[-- Type: application/octet-stream, Size: 2483 bytes --]

diff -urw nano/drivers/char/scx200_watchdog.c.orig nano/drivers/char/scx200_watchdog.c
--- nano/drivers/char/scx200_watchdog.c.orig Thu Feb 21 13:49:53 2002
+++ nano/drivers/char/scx200_watchdog.c Thu Feb 21 13:55:30 2002
@@ -38,13 +38,15 @@
#define CONFIG_WATCHDOG_NOWAYOUT 0
#endif

-static char name[] = "scx200_watchdog";
+static const char name[] = "scx200_watchdog";

static int margin = 60; /* in seconds */
MODULE_PARM(margin, "i");
+MODULE_PARM_DESC(margin, "Watchdog margin in seconds");

static int nowayout = CONFIG_WATCHDOG_NOWAYOUT;
MODULE_PARM(nowayout, "i");
+MODULE_PARM_DESC(nowayout, "If true the watchdog can't be disabled\n");

static u16 wdto_restart;
static struct semaphore open_sem;
@@ -57,6 +59,8 @@
#define WDSTS 0x04 /* Status Register */
#define WDOVF (1<<0) /* Overflow */

+#define W_SCALE (32768/1024) /* This depends on the value of W_ENABLE */
+
static void scx200_watchdog_ping(void)
{
outw(wdto_restart, scx200_config_block + WDTO);
@@ -65,7 +69,7 @@
static void scx200_watchdog_update_margin(void)
{
printk(KERN_INFO "%s: timer margin %d seconds\n", name, margin);
- wdto_restart = 32768 / 1024 * margin;
+ wdto_restart = margin * W_SCALE;
scx200_watchdog_ping();
}

@@ -117,7 +121,7 @@
static int scx200_watchdog_notify_sys(struct notifier_block *this,
unsigned long code, void *unused)
{
- if (code == SYS_DOWN || code == SYS_HALT)
+ if (code == SYS_HALT || code == SYS_POWER_OFF)
scx200_watchdog_disable();

return NOTIFY_DONE;
@@ -125,9 +129,7 @@

static struct notifier_block scx200_watchdog_notifier =
{
- scx200_watchdog_notify_sys,
- NULL,
- 0
+ notifier_call: scx200_watchdog_notify_sys
};

static ssize_t scx200_watchdog_write(struct file *file, const char *data,
@@ -182,13 +184,12 @@
case WDIOC_SETTIMEOUT:
if (get_user(new_margin, (int *)arg))
return -EFAULT;
- margin = new_margin;
+ margin = new_margin * 60; /* convert minutes to seconds */
scx200_watchdog_update_margin();
return 0;
-
#ifdef WDIOC_GETTIMEOUT
case WDIOC_GETTIMEOUT:
- return put_user(margin, (int *)arg);
+ return put_user((margin + 59) / 60, (int *)arg);
#endif
}
}
@@ -210,6 +211,11 @@
static int __init scx200_watchdog_init(void)
{
int r;
+
+ if (scx200_f0_pdev == NULL) {
+ printk(KERN_ERR "%s: Not a SCx200 CPU\n", name);
+ return -ENODEV;
+ }

scx200_watchdog_update_margin();
sema_init(&open_sem, 1);