[PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

On 5 February 2016 at 22:42, Guenter Roeck <linux at roeck-us.net> wrote:
> On 02/05/2016 01:51 AM, Fu Wei wrote:
>>
>> Hi Guenter,
>>
>> On 4 February 2016 at 13:17, Guenter Roeck <linux at roeck-us.net> wrote:
>>>
>>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>>
>>>>
>>>> On 4 February 2016 at 02:45, Timur Tabi <timur at codeaurora.org> wrote:
>>>>>
>>>>>
>>>>> Fu Wei wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> As you know I have made the pre-timeout support patch, If people like
>>>>>> it, i am happy to go on upstream it separately.
>>>>>>
>>>>>> If we want to use pre-timeout here, user only can use get_pretimeout
>>>>>> and disable panic by setting pretimeout to 0
>>>>>> but user can not really set pretimeout, because "pre-timeout  ==
>>>>>> timeout / 2 (always)".
>>>>>> if user want to change pretimeout, he/she has to set_time instead.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Ok, I think patches 4 and 5 should be combined, and I think the Kconfig
>>>>> entry should be removed and just use panic_enabled.
>>>
>>>
>>>
>>> Agreed.
>>
>>
>> np, will do
>>
>>>>
>>>>
>>>> NP, will update this patchset like that ,  thanks :-)
>>>>
>>>
>>> Also, if panic is enabled, the timeout needs to be adjusted accordingly
>>> (to only panic after the entire timeout period has expired, not after
>>> half of it). We can not panic the system after timeout / 2.
>>
>>
>> OK, my thought is
>>
>> if panic is enabled :
>> |--------WOR-------WS0--------WOR-------WS1
>> |------timeout------(panic)------timeout-----reset
>>
>> if panic is disabled .
>> |--------WOR-------WS0--------WOR-------WS1
>> |---------------------timeout---------------------reset
>>
>>   panic_enabled only can be configured when module is loaded by module
>> parameter
>>
>> But user should know that max_timeout(panic_enable) =
>> max_timeout(panic_disable) / 2
>>
>
> That means you'll have to update max_timeout accordingly.

panic_enabled only can be configured when module is loaded, so we
don't need to update it.

max_timeout will only be set up in the init stage.

Does it make sense ? :-)

>
>>>
>>> I am not too happy with the parameter name (panic_enabled). How about
>>> "action", to match machzwd ?
>>
>>
>> yes, makes sense. Maybe we can do something  like this:
>>
>> /*
>>   * action refers to action taken when watchdog gets WS0
>>   * 0 = SKIP
>>   * 1 = PANIC
>>   * defaults to SKIP (0)
>>   */
>> static int action;
>> module_param(action, int, 0);
>> MODULE_PARM_DESC(action, "after watchdog gets WS0 interrupt, do: "
>> "0 = SKIP(*)  1 = PANIC");
>>
> Yes, though I would suggest to use lower case letters.

yes,  NP, will do , Thanks :-)

>
> Thanks,
> Guenter
>



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021