Re: [PATCHv7 1/8] watchdog: Extend kernel API to know about HW limitations

[Timo Kokkonen <timo.kokkonen@offcode.fi>]

On 24.04.2015 20:08, Guenter Roeck wrote:
> On Wed, Apr 22, 2015 at 02:11:35PM +0300, Timo Kokkonen wrote:
>> There is a great deal of diversity in the watchdog hardware found on
>> different devices. Differen hardware have different contstraints on
>> them, many of the constraints that are excessively difficult for the
>> user space to satisfy.
>>
>> One such constraint is the length of the timeout value, which in many
>> cases can be just a few seconds. Drivers are creating ad hoc solutions
>> with timers and workqueues to extend the timeout in order to give user
>> space more time between updates. Looking at the drivers it is clear
>> that this has resulted to a lot of duplicate code.
>>
>> Add an extension to the watchdog kernel API that allows the driver to
>> describe tis HW constraints to the watchdog code. A kernel worker in
>> the core is then used to extend the watchdog timeout on behalf of the
>> user space. This allows the drivers to be simplified as core takes
>> over the timer extending.
>>
>> Tested-by: Wenyou Yang <wenyou.yang@atmel.com>
>> Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi>
>
> I started twice to review this series, and each time there is a new version
> before I can finish the review. Guess I'll wait until it settles down a bit
> before trying again :-(. Just a quick comment below.
>

Yeah, I didn't quite know how much time I had available to work with 
these patches, so I kept on sending fixed series out in a hope I get 
some feedback before making too big steps in any false direction.. And I 
ended up doing a  bit more than I thought at first. But I'm not going to 
make a new one until I have got enough feedback from this version. So 
please go ahead and review this version when you have time.

>> ---
>>   drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++--
>>   drivers/watchdog/watchdog_dev.c  |  75 ++++++++++++++++++++++++++---
>>   include/linux/watchdog.h         |  23 +++++++++
>>   3 files changed, 189 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
>> index cec9b55..fd12489 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>>   EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>>
>>   /**
>> + * watchdog_init_parms() - initialize generic watchdog parameters
>> + * @wdd: Watchdog device to operate
>> + * @dev: Device that stores the device tree properties
>> + *
>> + * Initialize the generic timeout parameters. The driver needs to set
>> + * hw_features bitmask from @wdd prior calling this function in order
>> + * to ensure the core knows how to handle the HW.
>> + *
>> + * A zero is returned on success and -EINVAL for failure.
>> + */
>> +int watchdog_init_params(struct watchdog_device *wdd, struct device *dev)
>> +{
>> +	int ret = 0;
>> +
>> +	ret = watchdog_init_timeout(wdd, wdd->timeout, dev);
>
> You are changing the semantics of watchdog_init_timeout here;
> for all practical purposes it no longer accepts the timeout passed
> as parameter, but expects the timeout to be configured in wdd->timeout
> instead. Please don't do that.

hmm.. Yes, this isn't quite right. What I thought it should be is that 
the driver initializes  the values with sane value (either from module 
parameter or some default) and then if that's not set (wdd->timeout is 
zero), watchdog_init_params() gets the value from device tree. If we 
still don't get any preference for the timeout, then use some reasonable 
default, such as 60 seconds. How's that?

Right now the module parameter basically gets ignored altogether, which 
is wrong.

-Timo