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]

Date:	Wed, 3 Aug 2011 11:56:06 +0200
From:	Wim Van Sebroeck <wim@...ana.be>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
Cc:	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Watchdog Mailing List <linux-watchdog@...r.kernel.org>
Subject: Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver
	Core.

Hi All,

> >> @@ -24,11 +24,9 @@
> >>   */
> >>  
> >>  #include <linux/module.h>
> >> -#include <linux/fs.h>
> >>  #include <linux/miscdevice.h>
> >
> > Is the above header still needed?
> 
> I think so due to the MODULE_ALIAS_MISCDEV() at the end of the file.  But,
> I'm not sure if that is really needed... Wim?

Yes it's till needed. Both MODULE_ALIAS_MISCDEV() and WATCHDOG_MINOR are defined in miscdevice.h .

> >> @@ -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;
> >
> > Should you check that the given timeout is in valid range here, like it is
> > done in the original driver?
> 
> I thought the core would handle that.  If not maybe it should validate the
> timeout based on the min/max values.  Wim?

The core doesn't handle that.
What happens is: We set the parameter normally the same as the default value.
The user can change this when loading the module by another value.
We then normally need to check if this value makes any sense for the hardware.
If not we reset it back to the default value.

The core is however not aware of the default value.
Several solutions are possible:
1) we add a timeout_default value into the struct (waste of space imho)
2) we set it to timeout_max if out of range
3) we set it to timeout_min if less then timeout_min or timeout_max if bigger then timeout_max
4) we can add an operation to handle this so that the driver can decide

For now I would leave the check in. In parallel we could indeed adopt a common behaviour and add it to the core.

Kind regards,
Wim.

PS: Congratulations with your first child!

--
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/