Re: [PATCH v3 2/2] watchdog: fix w83627hf_wdt clear timeout expired

[Tony Chung]

Thanks Guenter!
I agree with you.  My first reaction was also about a small watchdog
server that will start in early boot process.  There are pros and
cons.   For example, there are many types of watchdog devices such as
ipmi_watchdog which can accept more than  255 seconds for timeout. So
you really need udev to pick the correct watchdog driver.  It could be
very complex.

Our requirement don't need watchdog protection during the boot process
until application is fully up but a driver should not assume anything.
 Anyway, an unexpected reboot is definitely a bug that need to be
fixed.   It is really easily reproducible.  Depending on your hardware
and BIOS settings, just reboot the boot, wait for 5 minutes and then
run "insmod w83627hf_wdt.ko".  The box just reboot by itself.  The
watchdog sever is not even started.

This line is actually the original fix that is running over a year:
    outb_p(0, WDT_EFDR);    /* disable to prevent reboot */

When I tried to cleanup it up,  I thought I don't need it but it
turned out it was still needed.
When I changed it from 0xC0 to 0xD0, it still reboot.

Thanks and best regards,
@Tony

On Mon, Apr 1, 2013 at 5:07 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On Sun, Mar 31, 2013 at 10:59:32PM -0700, Tony Chung wrote:
>> Observed that the w83627hf watchdog timer start counting during reboot.
>> If the system load the driver after 5  minutes, it rebooted immediately because of timer expired.
>> For example, fsck took more than 5 minutes to run, then the computer reboot as soon as the driver was loaded.
>>
> Thinking about it, I wonder if that really solves your problem.
>
> If the watchdog driver is built into the kernel, and if the watchdog is already
> running, you still may have the problem that it may time out again and trigger
> (again) before the watchdog application can be loaded. Ultimately, in such a
> situation, the watchdog application, or at least a primitive one, should really
> be started from initramfs and possibly be replaced later on by the "real" watchdog
> application. The watchdog package includes a small binary for that purpose,
> though I have no idea if is is loaded in initramfs or only later.
>
> Ultimately, that also applies to the original problem you tried to solve with
> the longer timeout. It doesn't seem correct to have to set a long timeout to
> address excessive boot times; in a way that defeats the purpose of a watchdog.
> A cleaner solution for that problem would be to make sure that a small watchdog
> application is started early in the boot process.
>
>> Signed-off-by: Tony Chung <tonychung00@xxxxxxxxx>
>> ---
>>  drivers/watchdog/w83627hf_wdt.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
>> index 8f1111d..7eaa226 100644
>> --- a/drivers/watchdog/w83627hf_wdt.c
>> +++ b/drivers/watchdog/w83627hf_wdt.c
>> @@ -168,14 +168,16 @@ static void w83627hf_init(void)
>>               pr_info("Watchdog already running. Resetting timeout to %d sec\n",
>>                       wdt_get_timeout_secs(timeout));
>>               outb_p(timeout, WDT_EFDR);    /* Write back to CRF6 */
>> +     } else {
>> +             outb_p(0, WDT_EFDR);    /* disable to prevent reboot */
>>       }
> Did I miss that earlier, or did yo add it in this version ?
>
> Reading the counter register just returned 0, so what are the benefits
> of writing 0 into it ?
>
> Thanks,
> Guenter
>
>>
>>       w83627hf_setup_crf5();
>>
>>       outb_p(0xF7, WDT_EFER); /* Select CRF7 */
>>       t = inb_p(WDT_EFDR);      /* read CRF7 */
>> -     t &= ~0xC0;               /* disable keyboard & mouse turning off
>> -                                 watchdog */
>> +     t &= ~0xD0;               /* clear timeout occurred and disable keyboard
>> +                                  & mouse turning off watchdog */
>>       outb_p(t, WDT_EFDR);    /* Write back to CRF7 */
>>
>>       w83627hf_unselect_wd_register();
>> --
>> 1.7.0.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>



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