On May 11 2004, at 18:37, Jeff Garzik was caught saying:
> Deepak Saxena wrote:
> >On May 11 2004, at 14:33, Andrew Morton was caught saying:
> >+#ifdef CONFIG_WATCHDOG_NOWAYOUT
> >+static int nowayout = 1;
> >+#else
> >+static int nowayout = 0;
> >+#endif
> >+static int heartbeat = 60;	/* (secs) Default is 1 minute */
> >+static unsigned long wdt_status = 0;	
> >+static unsigned long boot_status = 0;
> 
> Wastes bss(?) space to explicitly zero statics.

My mother always told me to initialize my variables but this
is easily fixed. 

> >+static void 
> >+wdt_disable(void)
> >+{
> >+	*IXP4XX_OSWK = IXP4XX_WDT_KEY;
> >+	*IXP4XX_OSWE = 0;
> >+	*IXP4XX_OSWK = 0;
> >+}
> 
> Are these magic CPU addresses you're writing to?  Normally one uses bus 
> macros to read/write to devices.  But SoC and embedded stuff is often 
> special...

Yep, these are mapped in in platform setup code in arch/arm/mach-ixp4xx.

> >+ixp4xx_wdt_open(struct inode *inode, struct file *file)
> >+{
> >+	if (test_and_set_bit(WDT_IN_USE, &wdt_status))
> >+		return -EBUSY;
> >+
> >+	clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
> >+
> >+	wdt_enable();
> >+
> >+	return 0;
> >+}
> 
> You typically want semaphores rather than bit tests.
> 
> And what about blocking on multiple openers?

I'm just copying other wdt driver in the tree do, so I'm assuming
it's "standard practice" to just -EBUSY on multiple users. 
Documentation/watchdog/watchdog-api.txt doesn't really specify the
behavior, so this may just be from historical reasons. In practice
you will only have one application managing the watchdog.

> >+static ssize_t 
> >+ixp4xx_wdt_write(struct file *file, const char *data, size_t len, loff_t 
> >*ppos)
> >+{
> >+	/* Can't seek (pwrite) on this device  */
> >+	if (ppos != &file->f_pos)
> >+		return -ESPIPE;
> >+
> >+	if (len) {
> >+		if (!nowayout) {
> >+			size_t i;
> >+
> >+			clear_bit(WDT_OK_TO_CLOSE, &wdt_status);
> >+
> >+			for (i = 0; i != len; i++) {
> >+				char c;
> >+
> >+				if (get_user(c, data + i))
> >+					return -EFAULT;
> >+				if (c == 'V')
> >+					set_bit(WDT_OK_TO_CLOSE, 
> >&wdt_status);
> >+			}
> >+		}
> >+		wdt_enable();
> >+	}
> 
> do other watchdog drivers really twiddle a bit like this on each write?


They use a separate variable but it amounts to the same behavior.
Clear it then reset it after we read the user command.  (see i8xx_tco.c)
I decided to twiddle a bit instead of using another static. 

Updated patch attached with variable init removed.

~Deepak

-- 
Deepak Saxena - dsaxena at plexity dot net - http://www.plexity.net/

"Unlike me, many of you have accepted the situation of your imprisonment and
 will die here like rotten cabbages." - Number 6