[lkml]   [2007]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [HP ProLiant WatchDog driver] hpwdt HP WatchDog Patch <resend>
    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
    * 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
    * 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.


    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2007-11-04 20:15    [W:0.025 / U:6.948 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site