Re: [HP ProLiant WatchDog driver] hpwdt HP WatchDog Patch <resend>

[Wim Van Sebroeck <wim@iguana.be>]

Hi Thomas,

I reviewed yor code and have ome remarks:
* All watchdog device drivers should work with seconds. Your driver works
with minutes. Please change to seconds.
* The misc_register function (which opens your driver towards user-space)
should be the last iregister-action you take into the pci probe function.
(So just before you do the printk where you say that the driver has been
initialized).
* The open and release functions should make sure that /dev/watchdog can
only be opened once.
* /dev/watchdog is a VFS (Virtual File System) so your open function should
return with "return nonseekable_open(inode, file);".
* please put the code to start the watchdog in a seperate function (this
will make it easier to migrate your driver to the generic watchdog model
in the future).
* If you watchdog can't be stopped then you should use a timer to make sure
that when /dev/watchdog is closed, that you continue to keepalive/ping the
watchdog device. (please see mixcomwd.c (this device has the same problem)).
* please put the code to change the timeout in a seperate function.
* the default return value for the ioctl calls should be -ENOTTY (instead of
-ENOIOCTLCMD).
* the GETSTATUS call should also have a "put_user(0, (int *)arg);".
* if possible please add sparse testing code into the ioctl handling.
* Personnaly: I would rather see the generic smbios code somewhere in the
arch/x86/ directory. We will probably need that also in other (non-watchdog)
drivers in the future.

Greetings,
Wim.

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