On 24 January 2015 at 19:09, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On 01/24/2015 09:31 AM, Rafał Miłecki wrote:
>>
>> On 24 January 2015 at 17:54, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>>
>>> On 01/24/2015 08:45 AM, Rafał Miłecki wrote:
>>>>
>>>>
>>>> On 24 January 2015 at 17:32, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>>>>
>>>>>
>>>>> On 01/24/2015 05:47 AM, Rafał Miłecki wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Just like in case of other watchdog drivers, use the new kernel core
>>>>>> API to provide restart support.
>>>>>>
>>>>>> Signed-off-by: Rafał Miłecki <zajec5@xxxxxxxxx>
>>>>>> ---
>>>>>> drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>>>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/bcm47xx_wdt.c
>>>>>> b/drivers/watchdog/bcm47xx_wdt.c
>>>>>> index 9816485..dac3c5d 100644
>>>>>> --- a/drivers/watchdog/bcm47xx_wdt.c
>>>>>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>>>>>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct
>>>>>> notifier_block *this,
>>>>>> return NOTIFY_DONE;
>>>>>> }
>>>>>>
>>>>>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned
>>>>>> long
>>>>>> mode,
>>>>>> + void *cmd)
>>>>>> +{
>>>>>> + struct bcm47xx_wdt *wdt;
>>>>>> +
>>>>>> + wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>>>>>> + wdt->timer_set(wdt, 1);
>>>>>> +
>>>>>> + return NOTIFY_DONE;
>>>>>> +}
>>>>>> +
>>>>>> static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>>>>>> .owner = THIS_MODULE,
>>>>>> .start = bcm47xx_wdt_soft_start,
>>>>>> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct
>>>>>> platform_device
>>>>>> *pdev)
>>>>>> watchdog_set_nowayout(&wdt->wdd, nowayout);
>>>>>>
>>>>>> wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
>>>>>> -
>>>>>> ret = register_reboot_notifier(&wdt->notifier);
>>>>>> if (ret)
>>>>>> goto err_timer;
>>>>>>
>>>>>> - ret = watchdog_register_device(&wdt->wdd);
>>>>>> + wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
>>>>>> + wdt->restart_handler.priority = 128;
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> This should be low priority since it doesn't immediately restart the
>>>>> system
>>>>> but uses a watchdog timeout.
>>>>
>>>>
>>>>
>>>> I think this is also the only way to reboot Broadcom device. Is there
>>>> some howto on values I should use? Should I change it to sth like 64?
>>>> Or what number?
>>>>
>>>
>>> 64 is fine.
>>>
>>> You never know if someone using that chip implements, for example, reset
>>> with a gpio pin.
>>
>>
>> Sure, that's why I wanted to left 129-255 values for other situations.
>> As watchdog reset is used in 99% of cases, I wanted to use the middle
>> priority for it.
>>
> ... and then forcing everyone else to use a higher priority than 128,
> resulting in a priority war. Guess those who predicted that this would
> happen were right after all :-(.
At least you're there to stop non-aware developers ;)
But honestly maybe it'd be worth to document this priority a bit
better. Documentation from kernel/reboot.c:
> 128: Default restart handler; use if no other restart handler is expected to be available, and/or if restart functionality is sufficient to restart the entire system
made me believe I'm doing well. Apparently I didn't understand it well enough :|
>> Actually it seems that currently every watchdog driver with restart
>> handler uses priority 128. This includes: alim7101_wdt.c dw_wdt.c
>> imx2_wdt.c meson_wdt.c moxart_wdt.c s3c2410_wdt.c sunxi_wdt.c
>>
>> Are you sure this value 128 is wrong in case of bcm47xx_wdt? What
>> makes it alright for all other drivers?
>>
> The other drivers do not use a watchdog timeout to accomplish an indirect
> restart, but use a register from the set of registers used by the watchdog
> driver to implement the restart. Or at least that is how it should be.
Uh, that "register from the set of registers used by the watchdog
driver to implement the restart" make it really complex to follow for
someone non-involved into restart/priorities/watchdog.
I can't really see the difference, I'll just trust your comment.
P.S.
I just checked few watchdog drivers and:
1) meson_wdt.c uses
while (1) {
(..., no break)
mdelay(5);
}
2) sunxi_wdt.c uses
while (1) {
mdelay(5);
(..., no break)
}
Since they loop forever in restart handler it seems they also force
all other handlers to use priority higher than 128.
--
Rafał
--
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
