linux-kernel - Re: [PATCH] watchdog: add f71862fg support

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: <20100907131837.GH6114@salidar.me.mortis.eu>
Date:	Tue, 7 Sep 2010 15:18:37 +0200
From:	Giel van Schijndel <me@...tis.eu>
To:	Lutz Ballaschke <vegan.grindcore@...glemail.com>
Cc:	wim@...ana.be, linux-watchdog@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] watchdog: add f71862fg support

On Mon, Sep 06, 2010 at 03:24:34PM +0200, Lutz Ballaschke wrote:
> Fintek Super-I/O f71862fg watchdog support added to f71808e_wdt driver.
> Furthermore the WDIOC_GETTIMELEFT ioctl is added to the driver.

You're patch makes three changes:
 * Add f71862 support
 * Add the WDIOC_GETTIMELEFT ioctl
 * Change some config-register (magic) numbers into constants

Please split these things into separate patches.

> diff -Nurp linux-2.6.36-rc1-orig/drivers/watchdog/f71808e_wdt.c linux-2.6.36-rc1/drivers/watchdog/f71808e_wdt.c
> --- linux-2.6.36-rc1-orig/drivers/watchdog/f71808e_wdt.c	2010-08-16 02:41:37.000000000 +0200
> +++ linux-2.6.36-rc1/drivers/watchdog/f71808e_wdt.c	2010-09-04 18:52:14.000000000 +0200
> @@ -252,6 +255,24 @@ exit_unlock:
>  	return err;
>  }
>  
> +static int watchdog_time_left(void)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&watchdog.lock);
> +	ret = superio_enter(watchdog.sioaddr);
> +	if (ret)
> +		goto exit_unlock;
> +
> +	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
> +	ret = (int) superio_inb(watchdog.sioaddr, F71808FG_REG_WD_TIME);
> +	superio_exit(watchdog.sioaddr);
> +
> +exit_unlock:
> +	mutex_unlock(&watchdog.lock);
> +	return ret;
> +}
> +
>  static int watchdog_keepalive(void)
>  {
>  	int err = 0;

This function doesn't perform any unit-conversion.  The ioctl expects to
receive seconds as the result, while this function can either return
minutes or seconds depending on the value of WD_UNIT (see datasheet
and/or uses of minutes_mode in the driver).

Further, why are you casting superio_inb()'s return value to (int) while
that's already the return type of that function?

> @@ -308,4 +329,10 @@ static int watchdog_start(void)
>  		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
>  		break;
>  
> +	case f71862fg:
> +		/* Set pin 63 to WDTRST# (SPI must be disabled first!) */
> +		superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
> +		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
> +		break;
> +
>  	default:

Given that the F71862 has *two* pins assignable to the WDTRST# signal,
please document that fact, and the reason for choosing this one in the
code.

Perhaps it might even be desirable to insert a printk there.  This to
make the user aware of the fact that this driver might not work if the
pin configuration of their hardware doesn't match the assumption you
make about it in this code.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)