Re: [PATCH 1/2] watchdog: NXP LPC18XX Windowed Watchdog Timer Driver

[Guenter Roeck <linux@roeck-us.net>]

On 06/28/2015 11:13 AM, Ariel D'Alessandro wrote:
> Hi Guenter,
> Thanks for your quick reply.
>
My pleasure ....

>>> +/*
>>> + * NXP LPC18xx Windowed Watchdog Timer (WWDT)
>>> + *
>>
>> What does "Windowed" stand for ? That term doesn't really mean anything
>> to me. How does it differ from a non-windowed watchdog driver ?
>
> "NXP LPC18xx Windowed Watchdog Timer" is the full specific name of the
> Watchdog, as it's written in the LPC18xx User Manual (UM10430).
>
> "Windowed" means that, besides the maximum, a minimum time-out period
> can be set, requiring reload operation (feed) to occur between these two
> limits.
>
> Linux watchdog framework doesn't take this property into account, so
> "Windowed" term may not have much significance here. Do you think it
> should be removed?
>
I would suggest to drop the term - without explanation it is just confusing.

>>> +/* Timeout values in seconds */
>>> +#define LPC_WDT_DEF_TIMEOUT		1
>>> +
>>
>> One second ? This is highly unusual. 30 or 60 seconds is more common,
>> and one second would be very challenging for user space.
>>
>> Any special reason for using such a tight default ?
>
> Considering that LPC18xx Watchdog has a fixed divide-by-4 clock
> pre-scaler and a 24-bit counter and that Watchdog clock runs at a fixed
> frequency of 12MHz, timeout range goes from 1 to 5 seconds.
>
> I think you're right, 1 sec is very challenging, so it's 5 secs then.
>
Ultimately you might want to consider a soft timer as backup to the system
timeout. But that can be done later if/when needed.

>>> +
>>> +static int lpc_wdt_remove(struct platform_device *pdev)
>>> +{
>>> +	struct lpc_wdt_dev *lpc_wdt = platform_get_drvdata(pdev);
>>> +
>>> +	lpc_wdt_stop(&lpc_wdt->wdt_dev);
>>
>> This will keep the timer enabled. It would be interesting to see what
>> happens if you build the driver as module and unload it. I think
>> it will crash. Can you try ?
>
> Yes, you're right. After module gets unloaded, timer keeps enabled with
> a callback that was deallocated from memory.
>
> Since Watchdog cannot be disabled in hardware, it's not going to be
> possible to remove the driver without resetting the system. Unless we
> could keep a timer set and running outside the module, it won't be
> removable.
>
> What do you think?
>

My take is that the driver should be loadable as module, which implies
that it must be removable. Whoever actually does remove the driver
is really asking for trouble and doesn't deserve better.

On the other side, there are a few other watchdogs with similar properties.
Maybe we can just take guidance from there. Can you have a look ?
I can look myself, but it may take a few days.

Thanks,
Guenter