linux-kernel - Re: add bcm47xx watchdog driver

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: <4A29805D.60205@free.fr>
Date:	Fri, 05 Jun 2009 22:30:21 +0200
From:	matthieu castet <castet.matthieu@...e.fr>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	wim@...ana.be, linux-kernel@...r.kernel.org,
	linux-mips@...ux-mips.org, biblbroks@...ampro.rs
Subject: Re: add bcm47xx watchdog driver

Andrew Morton wrote:
> On Thu, 04 Jun 2009 22:24:56 +0200
> matthieu castet <castet.matthieu@...e.fr> wrote:
> 
>>
>> This add watchdog driver for broadcom 47xx device.
>> It uses the ssb subsytem to access embeded watchdog device.
>>
>> Because the watchdog timeout is very short (about 2s), a soft timer is used
>> to increase the watchdog period.
>>
>> Note : A patch for exporting the ssb_watchdog_timer_set will
>> be submitted on next linux-mips merge. Without this patch it can't 
>> be build as a module.
>>
>>
>> ...
>>
>> --- linux-2.6.orig/drivers/watchdog/Kconfig	2009-05-25 22:22:02.000000000 +0200
>> +++ linux-2.6/drivers/watchdog/Kconfig	2009-05-25 22:26:06.000000000 +0200
>> @@ -764,6 +764,12 @@
>>  	help
>>  	  Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
>>  
>> +config BCM47XX_WDT
>> +    tristate "Broadcom BCM47xx Watchdog Timer"
>> +    depends on BCM47XX
>> +    help
>> +      Hardware driver for the Broadcom BCM47xx Watchog Timer.
>> +
> 
> Please indent the kconfig body with a single tab character.
> 
Done

>> ...
>>
>> +#define DRV_NAME		"bcm47xx_wdt"
>> +
>> +#define WDT_DEFAULT_TIME	30	/* seconds */
>> +#define WDT_MAX_TIME		256	/* seconds */
>> +
>> +static int wdt_time = WDT_DEFAULT_TIME;
>> +static int nowayout = WATCHDOG_NOWAYOUT;
>> +
>> +module_param(wdt_time, int, 0);
>> +MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
>> +				__MODULE_STRING(WDT_DEFAULT_TIME) ")");
>> +
>> +#ifdef CONFIG_WATCHDOG_NOWAYOUT
>> +module_param(nowayout, int, 0);
>> +MODULE_PARM_DESC(nowayout,
>> +		"Watchdog cannot be stopped once started (default="
>> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>> +#endif
> 
> hm, now what's happening with the third arg to module_param()?
I don't understand what you mean.
This thing is common in watchdog drivers. For example 
drivers/watchdog/at91rm9200_wdt.c does the same thing.

> 
>> +static struct platform_device *bcm47xx_wdt_platform_device;
>> +
>> +static unsigned long bcm47xx_wdt_busy;
>> +static char expect_release;
>> +static struct timer_list wdt_timer;
>> +static atomic_t ticks;
>> +
>> +static inline void bcm47xx_wdt_hw_start(void)
>> +{
>> +	/* this is 2,5s on 100Mhz clock  and 2s on 133 Mhz */
>> +	ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
>> +}
>> +
>> +static inline int bcm47xx_wdt_hw_stop(void)
>> +{
>> +	return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
>> +}
>> +
>> +static void bcm47xx_timer_tick(unsigned long unused)
>> +{
>> +	if(!atomic_dec_and_test(&ticks)) {
> 
> Please pass this patch (and all others) through scripts/checkpatch.pl
> and review the resulting output.
Done, everything is ok, expect a printk line over 80 characters.

> 
>> +		bcm47xx_wdt_hw_start();
>> +		mod_timer(&wdt_timer, jiffies + HZ);
>> +	}
>> +	else {
>> +		printk(KERN_CRIT PFX "Watchdog will fire soon!!!.\n");
>> +	}
>> +}
>> +
>> +static inline void bcm47xx_wdt_pet(void)
>> +{
>> +	atomic_set(&ticks, wdt_time);
>> +}
> 
> What does "pet" stand for?
> 
A watchdog timer is a computer hardware timing device that triggers a 
system reset if the main program, due to some fault condition, such as a 
hang, neglects to regularly service the watchdog (writing a “service 
pulse” to it, also referred to as “petting the dog” [1]

[1] http://en.wikipedia.org/wiki/Watchdog_timer

But I can change the name if you want. Note that pet appear in 
drivers/watchdog/sb_wdog.c and drivers/watchdog/sbc_epx_c3.c

>> +static void bcm47xx_wdt_start(void)
>> +{
>> +	bcm47xx_wdt_pet();
>> +	bcm47xx_timer_tick(0);
>> +}
>> +
>> +static void bcm47xx_wdt_pause(void)
>> +{
>> +	del_timer(&wdt_timer);
> 
> Should this be del_timer_sync()?  The timer callback can still be
> executing after del_timer() returns.
Yes, changed to del_timer_sync()

>> +static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
>> +{
>> +	if (expect_release == 42) {
>> +		bcm47xx_wdt_stop();
>> +	} else {
>> +		printk(KERN_CRIT DRV_NAME ": Unexpected close, not stopping watchdog!\n");
> 
> Can this happen?
yes : this is a common pattern in watchdog driver (check for example 
softdog) :
- expect_release is in bss (set to 0)
- we set expect_release to this magic value, only if we get a write with 
a special character and we are not in nowayout.
- So for example doing a "cat /dev/watchdog" should go in this path.

> 
>> +		bcm47xx_wdt_start();
>> +	}
>> +
>> +	clear_bit(0, &bcm47xx_wdt_busy);
>> +	expect_release = 0;
>> +	return 0;
>> +}
>> +

Thanks for the review.

I attach a new version.

View attachment "bcm47xx_watchdog_v3.diff" of type "text/x-diff" (8353 bytes)