[lm-sensors] [PATCH] w83793 watchdog support.

Sven Anders anders at anduras.de
Thu Jan 21 10:47:52 CET 2010


Hello!

Thanks for you last comments on the code. Now after the holidays, I found
time to write you an answer and sent you an updated patch as well.


>> The timeout here is an global variable, so it's already done here.
>>
> Erm, no.
>
> The watchdog_set_timeout() function currently writes the timeout passed in
> to the chip, but that will only change the timeout now, and not for future
> triggers. The way the ioctl should work is that the timeout between 2 triggers
> (before the system resets) should be changed, so the value written by
> watchdog_trigger() should be changed. And since this driver can support multiple
> ic's (unlikely this ever happens but still) the timeout should be stored in
> the w83793_data struct so that it can be changed per device. The module parameter
> can then be used to initialize the per device timeout when the
> device is probed.

Ok, I thought I already implemented the assignment but I really missed it.
But you're correct, the value should be implemented in the data struct. I've
done it in the current patch.

>>> In "static int watchdog_close(struct inode *inode, struct file *filp)"
>>> you are missing:
>>>
>>>           mutex_lock(&watchdog_data_mutex);
>>>           kref_put(&data->kref, w83793_release_resources);
>>>           mutex_unlock(&watchdog_data_mutex);
>>
>> I'm not sure if you're right. Closing the watchdog did not remove the
>> module nor does it disable it configurability.
>> But maybe I misunderstood something here.
>
> Well I'm sure I'm right :)   (please don't take this wrong and please
> keep asking questions).
>
> The problem with the free-ing of the per device data struct is that there
> are 2 ways to get to it:
>
> 1) Through the hwmon interface
> 2) Through the watchdog device
>
> Now the following can happen:
>
> 1) Driver attaches to w83793 i2c device
> 2) app opens /dev/watchdog
> 3) Driver detaches from w83793 i2c device
>     (for example because the i2c master module
>      gets unloaded)

Is it really possible to unload the i2c master module without
unloading any dependend module first? I thought this isn't possible...

> Now before your changes 3) Would remove the hwmon interface
> (and thus the only path to the per device data) and then
> the per device data would be freed.
>
> However now, someone can still have a reference to the per device
> data (the app which has /dev/watchdog open). This is why we now
> use reference counting for the per device data struct (this is
> what the kref stuff does). So we start by initializing the
> kref in probe() which puts its counter at 1, then the
> watchdog open function increases the counter (the kref_get)
> to 2. Then when the driver detaches, the w83793_remove() function
> decreases the counter (the kref_put) to 1, this does not
> call w83793_release_resources, as that will only get called when
> the kref hits 0.
>
> Now when:
> 4) The app closes /dev/watchdog
>
> The close function should call kref_put too, so that the counter
> will hit 0 and the data struct gets free-ed.

Ok, I think I now understand this.

>Notes:
>
> 1) that the misc_deregister() call in w83793_remove() only
> removes the /dev/watchdog node, and does not close any open
> filehandles already pointing to the watchdog (closing of filehandles
> is up to the application not the kernel).
>
> 2) that this (the i2c client being detached while /dev/watchdog is still
> open), is the reason for the data->client check in all watchdog functions
> which actually read/write the chip.

I assume the already running watchdog app can still access the opened
file even if the /dev/watchdog node does not exist anymore. So this
should be a problem.


There is one small problem left:

If the watchdog_open() functions failes with EBUSY, we must not
increase the counter.


Ok. This was my first encounter with the i2c framework. Until now I was
only using direct hardware access, which was easier because anything was
coded (and called!) in only one module source file.
Thanks for your explantations!

>> On the other hand, I think you are missing the 'reboot notifier' in
>> your fschmd driver code.
>
> The fsc chips are all part of Fujitsu Siemens Computers, iow the manufacturer
> of the motherboard == the manufacturer of the watchdog. As such the watchdog
> always gets initialized by the BIOS, so there is no reason to disable
> it before shutdown / reboot.

But you will need it, to prevent the watchdog to reboot the machine, if the
shutdown sequence needs more time to shutdown than the watchdog is set to.

Consider the following szenario:

1) Watchdog timeout is set to 1 Minute.
2) Watchdog helper tool resets the watchdog timer every 30 seconds.
3) User executes "shutdown" binary.
4) Watchdog timer is currently at 31 seconds and the watchdog helper
   tool is stopped first.
5) Watchdog timer has now about 30 seconds left.
6) Heavy loaded database needs much time to write it's caches or perform any
   other tasks during shutdown. For instance it will need about 2 minutes.
7) After 30 seconds the hardware watchdog kicks in and reboots the machine
   leaving the database in an inconsistent state.


Please notify me, if I need to make some more changes or if you sent the patch
upstream.

Regards
 Sven Anders

-- 
 Sven Anders <anders at anduras.de>                 () Ascii Ribbon Campaign
                                                 /\ Support plain text e-mail
 ANDURAS service solutions AG
 Innstraße 71 - 94036 Passau - Germany
 Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55

Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032
Mitglieder des Vorstands: Sven Anders, Marcus Junker
Vorsitzender des Aufsichtsrats: Mark Peters
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83793_wdt.patch
Type: text/x-patch
Size: 16217 bytes
Desc: not available
URL: <http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20100121/669d7ead/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: anders.vcf
Type: text/x-vcard
Size: 338 bytes
Desc: not available
URL: <http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20100121/669d7ead/attachment.vcf>


More information about the lm-sensors mailing list