linux-kernel - Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

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: <20110803100716.GS4227@infomag.iguana.be>
Date:	Wed, 3 Aug 2011 12:07:16 +0200
From:	Wim Van Sebroeck <wim@...ana.be>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
Cc:	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Watchdog Mailing List <linux-watchdog@...r.kernel.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>
Subject: Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver
	Core.

Hi H Hartley,

> > [...]
> >> @@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
> >>  {
> >>  	int err;
> >>  
> >> -	err = misc_register(&ep93xx_wdt_miscdev);
> >> +	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
> >> +	ep93xx_wdd.timeout = timeout;
> >> +
> >> +	err = watchdog_register_device(&ep93xx_wdd);
> >> +	if (err)
> >> +		return err;
> >>  
> >> -	boot_status = __raw_readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
> >> +	setup_timer(&timer, ep93xx_timer_ping, 1);
> >
> > Shouldn't the bootstatus setting be:
> > ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG) & 0x01 ? 1 : 0;
> > (or something similar with the WDIOF_OVERHEAT, ... flags).
> 
> The ep93xx watchdog status register doesn't line up nicely with the standard
> WDIOF_* flags.  The register has these bits defined:
> 
> PLSDN		6	Pulse Disable Not
> OVRID		5	Software Override of HWDIS
> SWDIS		4	Software Watchdog Disable
> HWDIS		3	Hardware Watchdog Disable
> URST		2	User Reset Detected
> 3KRST		1	Three-Key Reset Detected
> WD		0	Watchdog Reset Detected

Hmm, it would be nice to have this info also in the driver. Certainly at least a define for the WD bit.
Secondly: what it is doing now is allready incorrect: The WD bit is returned as bit 0 of the bootstatus value.
This is actually: WDIOF_OVERHEAT (Reset due to CPU overheat) instead of
WDIOF_CARDRESET (0x0020 Card previously reset the CPU)...

> The original bootstatus setting just checked for the WD bit.  I would like
> to pass the full status register so that userspace can figure out what caused
> the reset.  Is this an inappropriate abuse of WDIOC_GETBOOTSTATUS?

It's indeed an inappropriate abuse of WDIOC_GETBOOTSTATUS.
The WDIOC_GETBOOTSTATUS should return a result for all watchdog drivers.
The result is based on the WDIOF_* flags
This however does not mean that we can add flags. The 'User Reset detected' looks to me as something that can be usefull in embedded environments. We need to think about this and see what would be usefull in general.

Kind regards,
Wim.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/