Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325720 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Timur Tabi <timur@codeaurora.org> - 2016-02-03 18:30 +0100
    Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-03 18:50 +0100
      Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Timur Tabi <timur@codeaurora.org> - 2016-02-03 19:00 +0100
        Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-03 19:10 +0100
          Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Timur Tabi <timur@codeaurora.org> - 2016-02-03 19:10 +0100
            Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-03 19:20 +0100
              Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Timur Tabi <timur@codeaurora.org> - 2016-02-03 19:30 +0100
                Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-03 19:40 +0100
                  Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Timur Tabi <timur@codeaurora.org> - 2016-02-03 19:50 +0100
                    Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-04 00:10 +0100
                      Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Guenter Roeck <linux@roeck-us.net> - 2016-02-04 06:20 +0100
                        Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Timur Tabi <timur@codeaurora.org> - 2016-02-04 14:50 +0100
                          Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Guenter Roeck <linux@roeck-us.net> - 2016-02-04 15:50 +0100
                        Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-05 11:00 +0100
                          Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Timur Tabi <timur@codeaurora.org> - 2016-02-05 14:10 +0100
                            Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half  timeout panic support Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-02-05 14:40 +0100
                          Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half  timeout panic support Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-02-05 14:10 +0100
                          Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Guenter Roeck <linux@roeck-us.net> - 2016-02-05 15:50 +0100
                            Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-05 19:30 +0100
                              Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Guenter Roeck <linux@roeck-us.net> - 2016-02-06 01:00 +0100
                                Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-06 19:10 +0100
                                  Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Timur Tabi <timur@codeaurora.org> - 2016-02-06 20:00 +0100
                                    Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-07 03:10 +0100
                                  Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Guenter Roeck <linux@roeck-us.net> - 2016-02-06 20:00 +0100
                                    Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout  panic support Fu Wei <fu.wei@linaro.org> - 2016-02-07 03:20 +0100

Page 1 of 2  [1] 2  Next page →

#1325720 — Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

fu.wei@linaro.org wrote:
> +#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
> +static bool panic_enabled = true;

I think this should default to 'false', because IMHO, this seems like an 
odd feature.  I'm not crazy about the fact that there's a Kconfig option 
for it either, but I'm not going to NACK this patch.

I personally would prefer to drop this patch, and just wait for 
full-blown pre-timeout support.  It feels like a debugging feature that 
doesn't really belong upstream.  But like I said, it's just my opinion, 
and I won't complain if I'm outvoted.

[toc] | [next] | [standalone]

#1325735

Hi Timur,

Thanks for your rapid feedback :-)

On 4 February 2016 at 01:27, Timur Tabi <timur@codeaurora.org> wrote:
> fu.wei@linaro.org wrote:
>>
>> +#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
>> +static bool panic_enabled = true;
>
>
> I think this should default to 'false', because IMHO, this seems like an odd

yes, It make sense to make it default to 'false'.

> feature.  I'm not crazy about the fact that there's a Kconfig option for it
> either, but I'm not going to NACK this patch.
>
> I personally would prefer to drop this patch, and just wait for full-blown
> pre-timeout support.  It feels like a debugging feature that doesn't really

sorry, are you saying : using pre-timeout instead of this half timeout?

But even we have pre-timeout support, pre-timeout  == timeout / 2, it
can not be configured without touch timeout.

if you want pre-timeout  != timeout / 2, we have to modify WCV in the
interrupt routine.
 (because of the explicit watchdog refresh  mechanism)

Could you let me know why we need pre-timeout  here ?? :-)

> belong upstream.  But like I said, it's just my opinion, and I won't
> complain if I'm outvoted.

 I think this debugging feature is the  purpose of the two-stage
watchdog, if I understand correctly



-- 
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

[toc] | [prev] | [next] | [standalone]

#1325742

Fu Wei wrote:
> sorry, are you saying : using pre-timeout instead of this half timeout?
>
> But even we have pre-timeout support, pre-timeout  == timeout / 2, it
> can not be configured without touch timeout.
>
> if you want pre-timeout  != timeout / 2, we have to modify WCV in the
> interrupt routine.
>   (because of the explicit watchdog refresh  mechanism)
>
> Could you let me know why we need pre-timeout  here ??:-)

What I meant was that if we had full-blown pre-timeout support in the 
watchdog layer, then you could use that to implement the 
panic-on-half-timeout feature.

When pre-timeout is implemented, will you modify the interrupt handler 
to use it?

>> >belong upstream.  But like I said, it's just my opinion, and I won't
>> >complain if I'm outvoted.
>   I think this debugging feature is the  purpose of the two-stage
> watchdog, if I understand correctly

Hmmm... that make sense.  I think maybe you should drop the Kconfig 
option, and just have "static bool panic_enabled = false;"  Also, then 
do this:

if (panic_enabled) {
	ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
			       pdev->name, gwdt);
	if (ret) {
		dev_err(dev, "unable to request IRQ %d\n", irq);
		return ret;
	}
}

That way, the interrupt handler is never registered if the command-line 
parameter is not specified.

[toc] | [prev] | [next] | [standalone]

#1325774

Hi Timur

On 4 February 2016 at 01:53, Timur Tabi <timur@codeaurora.org> wrote:
> Fu Wei wrote:
>>
>> sorry, are you saying : using pre-timeout instead of this half timeout?
>>
>> But even we have pre-timeout support, pre-timeout  == timeout / 2, it
>> can not be configured without touch timeout.
>>
>> if you want pre-timeout  != timeout / 2, we have to modify WCV in the
>> interrupt routine.
>>   (because of the explicit watchdog refresh  mechanism)
>>
>> Could you let me know why we need pre-timeout  here ??:-)
>
>
> What I meant was that if we had full-blown pre-timeout support in the
> watchdog layer, then you could use that to implement the
> panic-on-half-timeout feature.
>
> When pre-timeout is implemented, will you modify the interrupt handler to
> use it?

Sorry I am little confused.

Actually I am taking your suggestion to avoid touching WCV in
interrupt routine.
So even we have pre-timeout support , it is useless for this
panic-on-half-timeout feature,
because pre-timeout  == timeout / 2 (always).

So maybe I misunderstand your suggestion,
could you let me know : why we want pre-timeout here?


>
>>> >belong upstream.  But like I said, it's just my opinion, and I won't
>>> >complain if I'm outvoted.
>>
>>   I think this debugging feature is the  purpose of the two-stage
>> watchdog, if I understand correctly
>
>
> Hmmm... that make sense.  I think maybe you should drop the Kconfig option,
> and just have "static bool panic_enabled = false;"  Also, then do this:
>
> if (panic_enabled) {
>         ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
>                                pdev->name, gwdt);
>         if (ret) {
>                 dev_err(dev, "unable to request IRQ %d\n", irq);
>                 return ret;
>         }
> }

yes, agree

>
> That way, the interrupt handler is never registered if the command-line
> parameter is not specified.
>



-- 
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

[toc] | [prev] | [next] | [standalone]

#1325781

Fu Wei wrote:
> Actually I am taking your suggestion to avoid touching WCV in
> interrupt routine.
> So even we have pre-timeout support , it is useless for this
> panic-on-half-timeout feature,
> because pre-timeout  == timeout / 2 (always).
>
> So maybe I misunderstand your suggestion,
> could you let me know : why we want pre-timeout here?

Maybe I'm confused.

For pre-timeout, I think the SBSA watchdog driver should support only 
half-timeout.  That is, the user cannot configure the length of the 
pre-timeout with this driver.  He can only enable it, and it is 
automatically set to 1/2 timeout.

So when pre-timeout occurs, the interrupt handler calls panic() or 
whatever it's supposed to do.

So "pre-timeout  == timeout / 2 (always)" is exactly what we want.

[toc] | [prev] | [next] | [standalone]

#1325796

On 4 February 2016 at 02:08, Timur Tabi <timur@codeaurora.org> wrote:
> Fu Wei wrote:
>>
>> Actually I am taking your suggestion to avoid touching WCV in
>> interrupt routine.
>> So even we have pre-timeout support , it is useless for this
>> panic-on-half-timeout feature,
>> because pre-timeout  == timeout / 2 (always).
>>
>> So maybe I misunderstand your suggestion,
>> could you let me know : why we want pre-timeout here?
>
>
> Maybe I'm confused.
>
> For pre-timeout, I think the SBSA watchdog driver should support only
> half-timeout.  That is, the user cannot configure the length of the
> pre-timeout with this driver.  He can only enable it, and it is
> automatically set to 1/2 timeout.

Actually,
the SBSA watchdog driver should support only  half-timeout for panic
the user cannot configure the length of "panic time",  He can only
enable it, and it is automatically set to 1/2 timeout.

we don't need pre-timeout here.

Hope I understand you correctly :-) sorry for your confusion

>
> So when pre-timeout occurs, the interrupt handler calls panic() or whatever
> it's supposed to do.

Actually,
So when 1/2 timeout occurs, the interrupt handler calls panic() or
whatever it's supposed to do.

>
> So "pre-timeout  == timeout / 2 (always)" is exactly what we want.

our patchset is doing this way.



-- 
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

[toc] | [prev] | [next] | [standalone]

#1325808

Fu Wei wrote:
> Actually,
> the SBSA watchdog driver should support only  half-timeout for panic
> the user cannot configure the length of "panic time",  He can only
> enable it, and it is automatically set to 1/2 timeout.
>
> we don't need pre-timeout here.
>
> Hope I understand you correctly:-)  sorry for your confusion
>
>> >
>> >So when pre-timeout occurs, the interrupt handler calls panic() or whatever
>> >it's supposed to do.
> Actually,
> So when 1/2 timeout occurs, the interrupt handler calls panic() or
> whatever it's supposed to do.
>

I understand all that.  What I'm saying is that, in the future, when 
pre-timeout support is added to the watchdog layer, I think it makes 
sense to modify this driver to use pre-timeout support.

[toc] | [prev] | [next] | [standalone]

#1325810

On 4 February 2016 at 02:26, Timur Tabi <timur@codeaurora.org> wrote:
> Fu Wei wrote:
>>
>> Actually,
>> the SBSA watchdog driver should support only  half-timeout for panic
>> the user cannot configure the length of "panic time",  He can only
>> enable it, and it is automatically set to 1/2 timeout.
>>
>> we don't need pre-timeout here.
>>
>> Hope I understand you correctly:-)  sorry for your confusion
>>
>>> >
>>> >So when pre-timeout occurs, the interrupt handler calls panic() or
>>> > whatever
>>> >it's supposed to do.
>>
>> Actually,
>> So when 1/2 timeout occurs, the interrupt handler calls panic() or
>> whatever it's supposed to do.
>>
>
> I understand all that.  What I'm saying is that, in the future, when
> pre-timeout support is added to the watchdog layer, I think it makes sense
> to modify this driver to use pre-timeout support.

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.




-- 
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

[toc] | [prev] | [next] | [standalone]

#1325840

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.

[toc] | [prev] | [next] | [standalone]

#1326117

On 4 February 2016 at 02:45, Timur Tabi <timur@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.

NP, will update this patchset like that ,  thanks :-)


-- 
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

[toc] | [prev] | [next] | [standalone]

#1326404

On 02/03/2016 03:00 PM, Fu Wei wrote:
> On 4 February 2016 at 02:45, Timur Tabi <timur@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 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.

I am not too happy with the parameter name (panic_enabled). How about
"action", to match machzwd ?

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1326814

Guenter Roeck wrote:
> 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.

It's a debugging feature, not an actual watchdog timeout panic.  That's 
why it's disabled by default.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[toc] | [prev] | [next] | [standalone]

#1326885

On 02/04/2016 05:48 AM, Timur Tabi wrote:
> Guenter Roeck wrote:
>> 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.
>
> It's a debugging feature, not an actual watchdog timeout panic.  That's why it's disabled by default.
>

"* When the first stage(the half timeout) is reached, WS0 interrupt is
  * triggered, at this moment the second watch period starts;
  * In the WS0 interrupt routine, panic will be triggered for saving the
  * system context.
  * If the system is getting into trouble and cannot be reset by panic or
  * restart properly by the kdump kernel(if supported), then the second
  * stage (the timeout) will be reached, system will be reset by WS1."

That doesn't sound like debugging to me.

Guenter

[toc] | [prev] | [next] | [standalone]

#1327582

Hi Guenter,

On 4 February 2016 at 13:17, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>
>> On 4 February 2016 at 02:45, Timur Tabi <timur@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

>
> 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");


>
> 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

[toc] | [prev] | [next] | [standalone]

#1327638

Thomas Petazzoni wrote:
>> if panic is enabled :
>> >|--------WOR-------WS0--------WOR-------WS1
>> >|------timeout------(panic)------timeout-----reset

> I'm quite certainly missing something completely obvious here, but how
> can you get the WS1 interrupt*after*  raising a panic? Aren't all
> interrupts disabled and the system fully halted once you get a panic(),
> especially when raised from an interrupt handler? If that's the case,
> how can the system continue to do things, such as receiving the WS1
> interrupt and resetting ?

Typically, WS1 is not an interrupt.  Instead, it's a hard system-level 
reset.

The hardware is capable of generating an interrupt for both WS0 and WS1. 
  However, the ACPI table only contains one interrupt value, and it's 
not clear whether that's supposed to be the WS0 interrupt or the WS1 
interrupts.

So this whole thing does assume a specfic watchdog configuration.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[toc] | [prev] | [next] | [standalone]

#1327694 — Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

Hello,

On Fri, 5 Feb 2016 07:08:23 -0600, Timur Tabi wrote:

> > I'm quite certainly missing something completely obvious here, but how
> > can you get the WS1 interrupt*after*  raising a panic? Aren't all
> > interrupts disabled and the system fully halted once you get a panic(),
> > especially when raised from an interrupt handler? If that's the case,
> > how can the system continue to do things, such as receiving the WS1
> > interrupt and resetting ?
> 
> Typically, WS1 is not an interrupt.  Instead, it's a hard system-level 
> reset.

Ah, right, true. I missed that aspect because on my HW, triggering a
system-level reset on WS1 is optional. I can actually get an interrupt
on both WS0 and WS1, and no reset at all.

But a normal configuration indeed involves having the WS1 event
configured in HW to be a system-level reset.

So, OK, it makes sense. Thanks for the clarification!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1327648 — Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

Hello,

On Fri, 5 Feb 2016 17:51:52 +0800, Fu Wei wrote:

> OK, my thought is
> 
> if panic is enabled :
> |--------WOR-------WS0--------WOR-------WS1
> |------timeout------(panic)------timeout-----reset

I'm quite certainly missing something completely obvious here, but how
can you get the WS1 interrupt *after* raising a panic? Aren't all
interrupts disabled and the system fully halted once you get a panic(),
especially when raised from an interrupt handler? If that's the case,
how can the system continue to do things, such as receiving the WS1
interrupt and resetting ?

Again, I'm probably missing something obvious, but I'm interested to
understand the reasoning here.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1327817

On 02/05/2016 01:51 AM, Fu Wei wrote:
> Hi Guenter,
>
> On 4 February 2016 at 13:17, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>
>>> On 4 February 2016 at 02:45, Timur Tabi <timur@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.

>>
>> 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.

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1327986

On 5 February 2016 at 22:42, Guenter Roeck <linux@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@roeck-us.net> wrote:
>>>
>>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>>
>>>>
>>>> On 4 February 2016 at 02:45, Timur Tabi <timur@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

[toc] | [prev] | [next] | [standalone]

#1328167

On 02/05/2016 10:21 AM, Fu Wei wrote:
> On 5 February 2016 at 22:42, Guenter Roeck <linux@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@roeck-us.net> wrote:
>>>>
>>>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>>>
>>>>>
>>>>> On 4 February 2016 at 02:45, Timur Tabi <timur@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 ? :-)
>
Not sure I understand your problem or question.

max_timeout will have to reflect the correct maximum timeout, under
all circumstances. It will have to be set to the correct value before
the watchdog driver is registered.

Guenter

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web