linux-kernel - Re: [PATCH] add watchdog driver IT8716 IT8718 IT8726 IT8712J/K

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47D72865.1010304@aol.com>
Date:	Wed, 12 Mar 2008 01:48:37 +0100
From:	Oliver Schuster <olivers137@....com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	Oliver Schuster <olivers137@....com>, linux-kernel@...r.kernel.org,
	wim@...ana.be
Subject: Re: [PATCH] add watchdog driver IT8716 IT8718 IT8726 IT8712J/K

Hi Andrew,

thank you for your recommendations and corrections.
I've considered all and changed the code.

But first: Add support for it8718, which made the driver slightly more
complex.
Now the driver supports it8716, it8718, it8726, it8712-j and it8712-k,
which are all 16-bit watchdog timer on ITE lpc-super-io-chips.

Andrew Morton wrote:
> On Wed, 27 Feb 2008 15:55:54 +0100
> Oliver Schuster <olivers137@....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?

exclusive -- sorry 8)

>> +	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?

You are right, changed.
About the spinlock, i use it primary to ensure that at the same time 
only one task can have access to the superio chip configuration register.

> The driver ..., but it uses this
> open-coded test when reading bits. ...

test_bit() is now used.

> Is this comment about `buf' true?

Comment about wdt_write is changed.

>> +				if (c == 'V')
>> +					expect_close = WD_MAGIC;
> 
> So we support a write of "VVVVVVVVVVV"?  Seems odd.

I prefer "echo V>/dev/watchdog" (in the case that nowayout is NOT set)
to stop the watchdog. But any write which contain the magic can stop the 
watchdog on next close of the device.
This code originates from softdog.c, now slightly changed.

>> +/*
>> + *      wdt_ioctl:
>> + *      @inode: inode ...
 >> ...
> 
> The comments in this file are _almost_ in kerneldoc format, but they
> actually aren't.  Please convert them fully (and test it!)

Most of comments are from the code, that i used as template. These
commentaries seems to be kerneldoc, but aren't. I changed this and add 
all needful.

To extract the kerneldoc, i like this colorful way:
scripts/kernel-doc -html ... | lynx -stdin

>> +config IT87_WDT
>> +	tristate "IT87 Watchdog Timer"
>> +	depends on EXPERIMENTAL
>> +	---help---
>> +	  This is the ...
> 
> 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. ...

The driver hwmon/it87.c use the same way to detect the ITE superio chip 
by poking at IO ports. It dependence on HAS_IOMEM, but i decided to be 
more strictly and use X86, because the low pin count interface is 
created for this architecture and the IO base addresses used by these 
chips are X86 specific.

Regards,
Oliver

View attachment "2.6.24.3-it87_wdt-1.12.patch" of type "text/x-patch" (20226 bytes)