[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <555DB0B8.9000503@roeck-us.net>
Date: Thu, 21 May 2015 03:17:28 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Fu Wei <fu.wei@...aro.org>
CC: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
Linaro ACPI Mailman List <linaro-acpi@...ts.linaro.org>,
linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
Wei Fu <tekkamanninja@...il.com>,
G Gregory <graeme.gregory@...aro.org>,
Al Stone <al.stone@...aro.org>,
Hanjun Guo <hanjun.guo@...aro.org>,
Timur Tabi <timur@...eaurora.org>,
Ashwin Chaugule <ashwin.chaugule@...aro.org>,
Arnd Bergmann <arnd@...db.de>, vgandhi@...eaurora.org,
wim@...ana.be, Jon Masters <jcm@...hat.com>,
Leo Duran <leo.duran@....com>, Jon Corbet <corbet@....net>,
"mark.rutland" <mark.rutland@....com>
Subject: Re: [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@...ck-us.net> wrote:
>> On 05/21/2015 01:32 AM, fu.wei@...aro.org wrote:
>>>
>>> From: Fu Wei <fu.wei@...aro.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@...aro.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists