lkml.org 
[lkml]   [2015]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 5/7] Watchdog: introduce "pretimeout" into framework
On 05/21/2015 03:05 AM, Fu Wei wrote:
> Hi Guenter,
>
> Thanks for review. :-)
> feedback inline below
>
> On 21 May 2015 at 17:04, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
>>>
>>> From: Fu Wei <fu.wei@linaro.org>
>>>
>>> Also update Documentation/watchdog/watchdog-kernel-api.txt to
>>> introduce:
>>> (1)the new elements in the watchdog_device and watchdog_ops struct;
>>> (2)the new API "watchdog_init_timeouts".
>>>
>>> Reasons:
>>> (1)kernel already has two watchdog drivers are using "pretimeout":
>>> drivers/char/ipmi/ipmi_watchdog.c
>>> drivers/watchdog/kempld_wdt.c(but the definition is different)
>>> (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
>>>
>>> Signed-off-by: Fu Wei <fu.wei@linaro.org>
>>> ---
>>
>>
>> [ ... ]
>>
>>> +extern int watchdog_init_timeouts(struct watchdog_device *wdd,
>>> + unsigned int pretimeout_parm,
>>> + unsigned int timeout_parm,
>>> + void (*update_limits)(struct
>>> watchdog_device *),
>>> + struct device *dev);
>>>
>>> -The watchdog_init_timeout function allows you to initialize the timeout
>>> field
>>> -using the module timeout parameter or by retrieving the timeout-sec
>>> property from
>>> -the device tree (if the module timeout parameter is invalid). Best
>>> practice is
>>> -to set the default timeout value as timeout value in the watchdog_device
>>> and
>>> -then use this function to set the user "preferred" timeout value.
>>> +The watchdog_init_timeouts function allows you to initialize the
>>> pretimeout and
>>> +timeout fields using the module pretimeout and timeout parameter or by
>>> +retrieving the elements in the timeout-sec property(the first element is
>>> for
>>> +timeout, the second one is for pretimeout) from the device tree(if the
>>> module
>>> +pretimeout and timeout parameter are invalid).
>>> +Normally, the pretimeout value will affect the limitation of timeout, and
>>> it
>>> +is also hardware related. So you can write a function in your driver to
>>> update
>>> +the limitation of timeout, according to the pretimeout value. Then pass
>>> the
>>> +function pointer by the update_limits parameter. If you driver doesn't
>>> +need this adjustment, just pass NULL to the update_limits parameter.
>>
>>
>> You've lost me a bit with the update_limits function.
>> watchdog_init_timeouts()
>> is called from the driver.
>
> yes, that is the help function which will be called from watchdog
> driver, like SBSA watchdog driver
>
>> Why should the function have to call back into
>> the
>> driver to update the parameters which are passed from the driver ?
>
> Let me explain this, please correct me if I misunderstand something.
> According to the concept of "pretimeout" in kernel, the timeout
> contains the pretimeout, like
>
> * Kernel/API: P---------| pretimeout
> * |-------------------------------T timeout
>
> If you set up the value of pretimeout, that means pretimeout
> <min_timeout < timeout < max_timeout < (pretimeout +
> max_timeout_for_1th_stage)
> For min_timeout > pretimeout. if some one setup a timeout like :
> pretimeout > timeout > min_timeout, I think that could be a problem
> For max_timeout < (pretimeout + max_timeout_for_1th_stage), if some
> one setup a timeout like (pretimeout + max_timeout_for_1th_stage) <
> timeout > max_timeout .
>
> I have explained a little in doc, but the adjustment may have
> something to do with hardware, like max_timeout_for_1th_stage(in SBSA
> watchdog , limited by WCV)
>
> maybe this problem wouldn't happen ,if you set up max_timeout to a
> small number. so you can pass NULL to the pointer.
> but I think maybe for other device , that may happen.
>
>> Seems to me the driver can do that calculation first, then call
>> watchdog_init_timeouts() with the result. Am I missing something ?
>
> maybe I am overthinking it :-)
> please correct me
>

I just sent a more complete review. In general I think this problem
(where the driver needs to update timeout limits based on the value of
pretimeout) is very driver specific, and should be kept in the driver.
I would prefer to keep it out of the API if possible.

Unless I am missing something, it should be possible to call the
update_limits function in the driver after calling init_timeouts.

Thanks,
Guenter



\
 
 \ /
  Last update: 2015-05-21 13:01    [W:0.118 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site