LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Oliver Schuster <olivers137@aol.com>
Cc: linux-kernel@vger.kernel.org, wim@iguana.be
Subject: Re: [PATCH] add watchdog driver IT8716 IT8726 IT8712J/K
Date: Fri, 29 Feb 2008 15:51:36 -0800	[thread overview]
Message-ID: <20080229155136.49c34bed.akpm@linux-foundation.org> (raw)
In-Reply-To: <47C579FA.7060403@aol.com>

On Wed, 27 Feb 2008 15:55:54 +0100
Oliver Schuster <olivers137@aol.com> wrote:

> Hi,
> this patch add a hardware watchdog device driver. It supports the
> 16 bit watchdog timer on superio chip it8712f-j, it 8712f-k, it8716f
> and it8726f. Today it was tested under vanilla 2.6.24.3, vanilla
> 2.6.24.2 and Mandriva with 2.6.24.2 and 2.6.24 kernel.
> 
> ...
>

> static  int exclusiv = DEFAULT_EXCLUSIV;

Could we put the "e" back into "exclusive" please?

> +static int wdt_set_timeout(int t)
> +{
> +	unsigned long flags;
> +
> +	if (t < 1 || t > 65535)
> +		return -EINVAL;
> +
> +	timeout = t;
> +
> +	if (wdt_status & BIT_MASK(WDTS_TIMER_RUN)) {
> +		spin_lock_irqsave(&spinlock, flags);

Are you sure we are OK testing WDTS_TIMER_RUN prior to taking the lock? 
Should that test happen inside the lock?


> +		superio_enter();
> +
> +		superio_select(GPIO);
> +		superio_outb(t>>8, WDTVALMSB);
> +		superio_outb(t, WDTVALLSB);
> +
> +		superio_exit();
> +		spin_unlock_irqrestore(&spinlock, flags);
> +	}
> +	return 0;
> +}
> +
> +/*
> + * The status bit does not allow to distinguish between a regular
> + * system reset and a watchdog forced reset. But, in testmode it
> + * is useful, so it is supported through WDIOC_GETSTATUS
> + */
> +
> +static int wdt_get_status(int *status)
> +{
> +	int new_status = 0;
> +	unsigned long flags;
> +
> +	if (testmode) {
> +		spin_lock_irqsave(&spinlock, flags);
> +		superio_enter();
> +
> +		superio_select(GPIO);
> +		new_status = superio_inb(WDTCTRL);
> +		superio_outb(new_status & 0xfe, WDTCTRL);
> +		new_status &= 0x01;
> +
> +		superio_exit();
> +		spin_unlock_irqrestore(&spinlock, flags);
> +	}
> +	*status = 0;
> +	if (new_status)
> +		*status |= WDIOF_CARDRESET;
> +	if (wdt_status & BIT_MASK(WDTS_KEEPALIVE))

The driver uses set_bit(&wdt_status, ...) to modify bits, but it uses this
open-coded test when reading bits.  Please switch all instances of this to
use plain old test_bit().

> +		*status |= WDIOF_KEEPALIVEPING;
> +	clear_bit(WDTS_KEEPALIVE, &wdt_status);
> +	return 0;
> +}
>
> ...
>
> +/*
> + *      wdt_write:
> + *      @file: file handle to the watchdog
> + *      @buf: buffer to write (unused as data does not matter here)

Is this comment about `buf' true?

> + *      @count: count of bytes
> + *      @ppos: pointer to the position to write. No seeks allowed
> + *
> + *      A write to a watchdog device is defined as a keepalive signal. Any
> + *      write of data will do, as we we don't define content meaning.
> + */
> +
> +static ssize_t wdt_write(struct file *file, const char __user *buf,
> +			    size_t count, loff_t *ppos)
> +{
> +	if (count) {
> +		if (!nowayout) {
> +			size_t ofs;
> +
> +	/* note: just in case someone wrote the magic character long ago */
> +			expect_close = 0;
> +			for (ofs = 0; ofs != count; ofs++) {
> +				char c;
> +				if (get_user(c, buf + ofs))
> +					return -EFAULT;
> +				if (c == 'V')
> +					expect_close = WD_MAGIC;

So we support a write of "VVVVVVVVVVV"?  Seems odd.

> +			}
> +		}
> +		wdt_keepalive();
> +	}
> +	return count;
> +}
> +
> +/*
> + *      wdt_ioctl:
> + *      @inode: inode of the device
> + *      @file: file handle to the device
> + *      @cmd: watchdog command
> + *      @arg: argument pointer
> + *
> + *      The watchdog API defines a common set of functions for all watchdogs
> + *      according to their available features.
> + */

The comments in this file are _almost_ in kerneldoc format, but they
actually aren't.  Please convert them fully (and test it!)

> +static struct watchdog_info ident = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
> +	.firmware_version =	1,
> +	.identity = WATCHDOG_NAME,
> +};
> +
>
> ...
>
> +config IT87_WDT
> +	tristate "IT87 Watchdog Timer"
> +	depends on EXPERIMENTAL
> +	---help---
> +	  This is the driver for the hardware watchdog on the ITE IT8716F,
> +	  IT8726F, IT8712F(Version J,K) Super I/O chips.  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 it87_wdt.

Surely this needs more dependencies.  The driver is x86(?)-specific and
blindly pokes at IO ports prior to determining whether the device is
actually present.  We don't want people who try loading this driver on
their ia64 servers to crash the machine, scribble on their disk drives, etc.



  reply	other threads:[~2008-02-29 23:52 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-27 14:55 Oliver Schuster
2008-02-29 23:51 ` Andrew Morton [this message]
2008-03-12  0:48   ` [PATCH] add watchdog driver IT8716 IT8718 " Oliver Schuster
2008-09-18  9:08     ` Wim Van Sebroeck
2008-03-05 15:48 [PATCH] add watchdog driver IT8716 " Oliver Schuster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080229155136.49c34bed.akpm@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=olivers137@aol.com \
    --cc=wim@iguana.be \
    --subject='Re: [PATCH] add watchdog driver IT8716 IT8726 IT8712J/K' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).