Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Al Stone wrote:

The issue for me in that case is that the SBSA requires a two stage timeout,

>
>Hmm - really ? This makes me want to step back a bit and re-read the 
specification
>to understand where it says that, and what the reasoning might be for such a
>requirement.

As far as I can tell, that's what the SBSA is requiring.  My understanding is
that the hardware is to first assert a WS0 signal when the timer expires.  If
the timer expires and WS0 has already been asserted, the WS1 signal is to be
asserted.  When WS1 is asserted, the system is to do a hard reset (Section 5.2,
"Server Base System Architecture", ARM-DEN-0029 Version 2.3).  I'm interpreting
the occurrence of WS0 as the first stage and WS1 as the second.

To me, at least, this makes sense in a server environment.  The WS0 occurs,
which gives me some time to save key info or try to recover before WS1 occurs
(or kexec, or any other cleverness).


I'm having some problem with the word "requires".

I think it applies only to the hardware.  That is, there must be a WS0 
timeout/event, and then after that there must be a WS1 timeout/event.


I don't think there is any "requirement" for software to do anything 
with WS0.  That's why I don't think pre-timeout is necessary for the 
driver to be SBSA-compliant.  All of the drivers for existing two-stage 
watchdog devices treat the device as a one-stage device, because the 
watchdog API does not support pre-timeout.  Are all of them also 
"broken"?  No.  So it would not be broken for an SBSA watchdog driver to 
ignore WS0.


I would have no problem with the following sequence of events:

1) We merge in a driver that treats the SBSA watchdog as a single-stage 
watchdog that does a hard reset at WS1 and ignores WS0.  My driver, with 
a few changes, would qualify.


2) We add support for pre-timeout to the Watchdog interface.  Fu can 
take all the time in the world (as far as I'm concerned) getting this 
perfected.  My only request is that the new pre-timeout API supports 
hardware where the timeout between stages must be equal.  That is, if 
timeout to stage 1 (call it T1) is X seconds, then the timeout to stage 
2 (call it T2) is another X seconds.  T2 = T1.  Of course, the API 
should also support hardware where T2 != T1.


3) The existing SBSA watchdog driver is updated to support the new 
pre-timeout API.  It would enforce the requirement that T2 = T1.


This approach will allow us to get a working SBSA watchdog driver into 
4.5 without much fuss.


--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Al Stone]

On 11/12/2015 05:25 PM, Guenter Roeck wrote:
> On 11/12/2015 04:06 PM, Al Stone wrote:
>> On 11/05/2015 09:41 AM, Guenter Roeck wrote:
>>> On 11/05/2015 07:00 AM, Fu Wei wrote:
 Hi Timur,

 On 5 November 2015 at 22:40, Timur Tabi  wrote:
> Fu Wei wrote:
>>
>> Did you really read the "Note" above OK, let me paste it again
>> and again:
>>
>> SBSA 2.3 Page 23 :
>> If a larger watch period is required then the compare value can be
>> programmed directly into the compare value register.
>
>
> Well, okay.  Sorry, I should have read what you pasted more closely. But I

 Thanks for reading it again.

> think that means during initialization, not during the WS0 timeout.

 I really don't see SBSA say "during initialization, not during the WS0
 timeout",
 please point it out the page number and the line number in SBSA spec.
 maybe I miss it?
 Thanks for your help in advance.

>
> Anyway, I still don't like the fact that you're programming WCV in the

 "you don't like" doesn't mean "it is wrong" or "we can't do this", so
 I will keep this way unless we have better idea to extend second stage
 timeout.

> interrupt handler, but I'm not going to make a big deal about it any more.

 Deal, Thanks a lot.

>>>
>>> The problem with your driver, as I see it, is that dealing with WS0/WS1
>>> and pretimeout makes the driver so complex that, at least for my part,
>>> I am very wary about it. The driver could long since have been accepted
>>> if it were not for that. Besides that, I really believe that any system 
>>> designer
>>> using the highest permitted frequency should be willing to live with the
>>> consequences, and not force the implementation of a a complex driver.
>>>
>>> Ultimately, you'll have to decide if you want a simple driver accepted, or
>>> a complex driver hanging in the review queue forever.
>>>
>>> Thanks,
>>> Guenter
>>
>> Sorry to poke my head in late like this, but I do have a vested interest in 
>> the
>> outcome so I'm very curious.  For my work, I need to have an ACPI-supported,
>> SBSA-compliant watchdog timer for arm64, and this series is one of the key
>> pieces to getting there.  The plan for me has been: (1) get an FDT based SBSA
>> watchdog timer, (2) add in kernel code to handle the ACPI GTDT table 
>> describing
>> timers, then (3) munge the SBSA watchdog timer for use by ACPI.
>>
>> So, is this an actual NAK of the patch series as is?  I think it is, but I 
>> want
>> it to be clear, and it has not been explicit yet.
>>
> I am not the maintainer, so I don't make the call. All I am saying is that I 
> don't
> feel comfortable with the code as is. Part of it is due to the the 
> specification's
> complexity, which leaves space for (mis)interpretations and bad 
> implementations.
> 
> Either case, this is just my personal opinion. All you'll have to do is to 
> convince
> Wim to accept your patch.

Ah, okay.  I was a little confused then about who was maintainer.  Sorry about
that.

>> If it is a NAK, that's fine, but I also want to be sure I understand what the
>> objections are.  Based on my understanding of the discussion so far over the
>> multiple versions, I think the primary objection is that the use of 
>> pretimeout
>> makes this driver too complex, and indeed complex enough that there is some
>> concern that it could destabilize a running system.  Do I have that right?
>>
>> The other possible item I could conclude out of the discussion is that we do
>> not want to have the pretimeout code as part of the watchdog framework; is 
>> that
>> also the case or did I misunderstand?
>>
> Nothing really to do with pretimeout, but with the complexity of implementing 
> it
> for this driver.
> 
> As for pretimeout, it does have its issues. Some people say that hitting
> a pretimeout should result in a panic, others just as strongly say that it
> should just dump a message to the console. Which does make me a bit wary, 
> since
> it means that it may be implemented differently in different drivers, which
> I consider highly undesirable.

Right.  Based on Timur's input, I think I understand this much better now.  I'm
not 100% convinced I know how to fix it, but I'll try some things on hardware to
test out some ideas.

>> And finally, a simpler, single stage timeout watchdog driver would be a
>> reasonable thing to accept, yes?  I can see where that would make sense.
>>
> I am quite sure that such a driver would long since have been accepted.
> 
>> The issue for me in that case is that the SBSA requires a two stage timeout,
> 
> Hmm - really ? This makes me want to step back a bit and re-read the 
> specification
> to understand where it says that, and what the reasoning might be for such a
> requirement.

As far as I can tell, that's what the SBSA is requiring.  My understanding is
that the hardware is to first assert a WS0 signal when the timer 

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Al Stone]

Sorry for the delayed response...I've got some difficult family things to work
on IRL that are taking priority...

On 11/12/2015 05:23 PM, Timur Tabi wrote:
> On 11/12/2015 06:06 PM, Al Stone wrote:
>> If it is a NAK, that's fine, but I also want to be sure I understand what the
>> objections are.  Based on my understanding of the discussion so far over the
>> multiple versions, I think the primary objection is that the use of 
>> pretimeout
>> makes this driver too complex, and indeed complex enough that there is some
>> concern that it could destabilize a running system.  Do I have that right?
> 
> I don't have a problem with the concept of pre-timeout per se.  My primary
> objection is this code:
> 
>> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
>> +{
>> +   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
>> +   struct watchdog_device *wdd = >wdd;
>> +
>> +   /* We don't use pretimeout, trigger WS1 now */
>> +   if (!wdd->pretimeout)
>> +   sbsa_gwdt_set_wcv(wdd, 0);
> 
> This driver depends on an interrupt handler in order to properly program the
> hardware.  Unlike some other devices, the SBSA watchdog does not need 
> assistance
> to reset on a timeout -- it is a "fire and forget" device.  What happens if
> there is a hard lockup, and interrupts no longer work?

Aha.  I see now.  That helps clarify a lot.  Thanks.

> The reason why Fu does this is because he wants to support a pre-timeout value
> that's independent of the timeout value.  The SBSA watchdog is normally
> programmed where real timeout equals twice the pre-timeout.  I would prefer 
> that
> the driver adhere to this limitation.  That would eliminate the need to
> pre-program the hardware in the interrupt handler.

The "normally programmed" limitation described is interesting; forgive my
ignorance, but where is that specified?  I couldn't find anything that specific
in the SBSA, or the ARM ARM, but I could have missed it.  That being said,
keeping them independent at least seems like a good idea; if I think about
kdump/kexec or some other recovery mechanism wanting to perhaps copy part of
RAM or flush a filesystem/database, or maybe do some other magic to recover
enough to be able to reset the timer, that may be a really long interval on a
large server.  I could easily see that being very different from a watchdog
timer that's meant to just make sure the platform is still making progress.
Conversely, I could see that recovery interval being very small or zero on
a guest OS, for example, and the watchdog still different.

>> And finally, a simpler, single stage timeout watchdog driver would be a
>> reasonable thing to accept, yes?  I can see where that would make sense.
> 
> I would be okay with merging such a driver, and then enhancing it later to add
> pre-timeout support.
> 
>> The issue for me in that case is that the SBSA requires a two stage timeout,
>> so a single stage driver has no real value for me.
> 
> There are plenty of existing watchdog devices that have a two-stage timeout 
> but
> the driver treats it as a single stage.  The PowerPC watchdog driver is like
> that.  The hardware is programmed for the second stage to cause a hardware
> reset, and the interrupt handler is typically a no-op or just a printk().
> 

Hrm.  Thanks for the pointer.  I _think_ I see a way to do that with arm64, and
perhaps combine this driver's functionality with what Timur did originally, but
still have it reasonably straightforward.  I need to do the experiments, though,
and see if it actually works first.

-- 
ciao,
al
---
Al Stone
Software Engineer
Linaro Enterprise Group
al.st...@linaro.org
---
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Al Stone wrote:

The issue for me in that case is that the SBSA requires a two stage timeout,

>
>Hmm - really ? This makes me want to step back a bit and re-read the 
specification
>to understand where it says that, and what the reasoning might be for such a
>requirement.

As far as I can tell, that's what the SBSA is requiring.  My understanding is
that the hardware is to first assert a WS0 signal when the timer expires.  If
the timer expires and WS0 has already been asserted, the WS1 signal is to be
asserted.  When WS1 is asserted, the system is to do a hard reset (Section 5.2,
"Server Base System Architecture", ARM-DEN-0029 Version 2.3).  I'm interpreting
the occurrence of WS0 as the first stage and WS1 as the second.

To me, at least, this makes sense in a server environment.  The WS0 occurs,
which gives me some time to save key info or try to recover before WS1 occurs
(or kexec, or any other cleverness).


I'm having some problem with the word "requires".

I think it applies only to the hardware.  That is, there must be a WS0 
timeout/event, and then after that there must be a WS1 timeout/event.


I don't think there is any "requirement" for software to do anything 
with WS0.  That's why I don't think pre-timeout is necessary for the 
driver to be SBSA-compliant.  All of the drivers for existing two-stage 
watchdog devices treat the device as a one-stage device, because the 
watchdog API does not support pre-timeout.  Are all of them also 
"broken"?  No.  So it would not be broken for an SBSA watchdog driver to 
ignore WS0.


I would have no problem with the following sequence of events:

1) We merge in a driver that treats the SBSA watchdog as a single-stage 
watchdog that does a hard reset at WS1 and ignores WS0.  My driver, with 
a few changes, would qualify.


2) We add support for pre-timeout to the Watchdog interface.  Fu can 
take all the time in the world (as far as I'm concerned) getting this 
perfected.  My only request is that the new pre-timeout API supports 
hardware where the timeout between stages must be equal.  That is, if 
timeout to stage 1 (call it T1) is X seconds, then the timeout to stage 
2 (call it T2) is another X seconds.  T2 = T1.  Of course, the API 
should also support hardware where T2 != T1.


3) The existing SBSA watchdog driver is updated to support the new 
pre-timeout API.  It would enforce the requirement that T2 = T1.


This approach will allow us to get a working SBSA watchdog driver into 
4.5 without much fuss.


--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Al Stone]

Sorry for the delayed response...I've got some difficult family things to work
on IRL that are taking priority...

On 11/12/2015 05:23 PM, Timur Tabi wrote:
> On 11/12/2015 06:06 PM, Al Stone wrote:
>> If it is a NAK, that's fine, but I also want to be sure I understand what the
>> objections are.  Based on my understanding of the discussion so far over the
>> multiple versions, I think the primary objection is that the use of 
>> pretimeout
>> makes this driver too complex, and indeed complex enough that there is some
>> concern that it could destabilize a running system.  Do I have that right?
> 
> I don't have a problem with the concept of pre-timeout per se.  My primary
> objection is this code:
> 
>> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
>> +{
>> +   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
>> +   struct watchdog_device *wdd = >wdd;
>> +
>> +   /* We don't use pretimeout, trigger WS1 now */
>> +   if (!wdd->pretimeout)
>> +   sbsa_gwdt_set_wcv(wdd, 0);
> 
> This driver depends on an interrupt handler in order to properly program the
> hardware.  Unlike some other devices, the SBSA watchdog does not need 
> assistance
> to reset on a timeout -- it is a "fire and forget" device.  What happens if
> there is a hard lockup, and interrupts no longer work?

Aha.  I see now.  That helps clarify a lot.  Thanks.

> The reason why Fu does this is because he wants to support a pre-timeout value
> that's independent of the timeout value.  The SBSA watchdog is normally
> programmed where real timeout equals twice the pre-timeout.  I would prefer 
> that
> the driver adhere to this limitation.  That would eliminate the need to
> pre-program the hardware in the interrupt handler.

The "normally programmed" limitation described is interesting; forgive my
ignorance, but where is that specified?  I couldn't find anything that specific
in the SBSA, or the ARM ARM, but I could have missed it.  That being said,
keeping them independent at least seems like a good idea; if I think about
kdump/kexec or some other recovery mechanism wanting to perhaps copy part of
RAM or flush a filesystem/database, or maybe do some other magic to recover
enough to be able to reset the timer, that may be a really long interval on a
large server.  I could easily see that being very different from a watchdog
timer that's meant to just make sure the platform is still making progress.
Conversely, I could see that recovery interval being very small or zero on
a guest OS, for example, and the watchdog still different.

>> And finally, a simpler, single stage timeout watchdog driver would be a
>> reasonable thing to accept, yes?  I can see where that would make sense.
> 
> I would be okay with merging such a driver, and then enhancing it later to add
> pre-timeout support.
> 
>> The issue for me in that case is that the SBSA requires a two stage timeout,
>> so a single stage driver has no real value for me.
> 
> There are plenty of existing watchdog devices that have a two-stage timeout 
> but
> the driver treats it as a single stage.  The PowerPC watchdog driver is like
> that.  The hardware is programmed for the second stage to cause a hardware
> reset, and the interrupt handler is typically a no-op or just a printk().
> 

Hrm.  Thanks for the pointer.  I _think_ I see a way to do that with arm64, and
perhaps combine this driver's functionality with what Timur did originally, but
still have it reasonably straightforward.  I need to do the experiments, though,
and see if it actually works first.

-- 
ciao,
al
---
Al Stone
Software Engineer
Linaro Enterprise Group
al.st...@linaro.org
---
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Al Stone]

On 11/12/2015 05:25 PM, Guenter Roeck wrote:
> On 11/12/2015 04:06 PM, Al Stone wrote:
>> On 11/05/2015 09:41 AM, Guenter Roeck wrote:
>>> On 11/05/2015 07:00 AM, Fu Wei wrote:
 Hi Timur,

 On 5 November 2015 at 22:40, Timur Tabi  wrote:
> Fu Wei wrote:
>>
>> Did you really read the "Note" above OK, let me paste it again
>> and again:
>>
>> SBSA 2.3 Page 23 :
>> If a larger watch period is required then the compare value can be
>> programmed directly into the compare value register.
>
>
> Well, okay.  Sorry, I should have read what you pasted more closely. But I

 Thanks for reading it again.

> think that means during initialization, not during the WS0 timeout.

 I really don't see SBSA say "during initialization, not during the WS0
 timeout",
 please point it out the page number and the line number in SBSA spec.
 maybe I miss it?
 Thanks for your help in advance.

>
> Anyway, I still don't like the fact that you're programming WCV in the

 "you don't like" doesn't mean "it is wrong" or "we can't do this", so
 I will keep this way unless we have better idea to extend second stage
 timeout.

> interrupt handler, but I'm not going to make a big deal about it any more.

 Deal, Thanks a lot.

>>>
>>> The problem with your driver, as I see it, is that dealing with WS0/WS1
>>> and pretimeout makes the driver so complex that, at least for my part,
>>> I am very wary about it. The driver could long since have been accepted
>>> if it were not for that. Besides that, I really believe that any system 
>>> designer
>>> using the highest permitted frequency should be willing to live with the
>>> consequences, and not force the implementation of a a complex driver.
>>>
>>> Ultimately, you'll have to decide if you want a simple driver accepted, or
>>> a complex driver hanging in the review queue forever.
>>>
>>> Thanks,
>>> Guenter
>>
>> Sorry to poke my head in late like this, but I do have a vested interest in 
>> the
>> outcome so I'm very curious.  For my work, I need to have an ACPI-supported,
>> SBSA-compliant watchdog timer for arm64, and this series is one of the key
>> pieces to getting there.  The plan for me has been: (1) get an FDT based SBSA
>> watchdog timer, (2) add in kernel code to handle the ACPI GTDT table 
>> describing
>> timers, then (3) munge the SBSA watchdog timer for use by ACPI.
>>
>> So, is this an actual NAK of the patch series as is?  I think it is, but I 
>> want
>> it to be clear, and it has not been explicit yet.
>>
> I am not the maintainer, so I don't make the call. All I am saying is that I 
> don't
> feel comfortable with the code as is. Part of it is due to the the 
> specification's
> complexity, which leaves space for (mis)interpretations and bad 
> implementations.
> 
> Either case, this is just my personal opinion. All you'll have to do is to 
> convince
> Wim to accept your patch.

Ah, okay.  I was a little confused then about who was maintainer.  Sorry about
that.

>> If it is a NAK, that's fine, but I also want to be sure I understand what the
>> objections are.  Based on my understanding of the discussion so far over the
>> multiple versions, I think the primary objection is that the use of 
>> pretimeout
>> makes this driver too complex, and indeed complex enough that there is some
>> concern that it could destabilize a running system.  Do I have that right?
>>
>> The other possible item I could conclude out of the discussion is that we do
>> not want to have the pretimeout code as part of the watchdog framework; is 
>> that
>> also the case or did I misunderstand?
>>
> Nothing really to do with pretimeout, but with the complexity of implementing 
> it
> for this driver.
> 
> As for pretimeout, it does have its issues. Some people say that hitting
> a pretimeout should result in a panic, others just as strongly say that it
> should just dump a message to the console. Which does make me a bit wary, 
> since
> it means that it may be implemented differently in different drivers, which
> I consider highly undesirable.

Right.  Based on Timur's input, I think I understand this much better now.  I'm
not 100% convinced I know how to fix it, but I'll try some things on hardware to
test out some ideas.

>> And finally, a simpler, single stage timeout watchdog driver would be a
>> reasonable thing to accept, yes?  I can see where that would make sense.
>>
> I am quite sure that such a driver would long since have been accepted.
> 
>> The issue for me in that case is that the SBSA requires a two stage timeout,
> 
> Hmm - really ? This makes me want to step back a bit and re-read the 
> specification
> to understand where it says that, and what the reasoning might be for such a
> requirement.

As far as I can tell, that's what the SBSA is requiring.  My understanding is
that the hardware is to first assert a WS0 

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Guenter Roeck]

On 11/12/2015 04:06 PM, Al Stone wrote:

On 11/05/2015 09:41 AM, Guenter Roeck wrote:

On 11/05/2015 07:00 AM, Fu Wei wrote:

Hi Timur,

On 5 November 2015 at 22:40, Timur Tabi  wrote:

Fu Wei wrote:


Did you really read the "Note" above OK, let me paste it again
and again:

SBSA 2.3 Page 23 :
If a larger watch period is required then the compare value can be
programmed directly into the compare value register.



Well, okay.  Sorry, I should have read what you pasted more closely. But I


Thanks for reading it again.


think that means during initialization, not during the WS0 timeout.


I really don't see SBSA say "during initialization, not during the WS0 timeout",
please point it out the page number and the line number in SBSA spec.
maybe I miss it?
Thanks for your help in advance.



Anyway, I still don't like the fact that you're programming WCV in the


"you don't like" doesn't mean "it is wrong" or "we can't do this", so
I will keep this way unless we have better idea to extend second stage
timeout.


interrupt handler, but I'm not going to make a big deal about it any more.


Deal, Thanks a lot.



The problem with your driver, as I see it, is that dealing with WS0/WS1
and pretimeout makes the driver so complex that, at least for my part,
I am very wary about it. The driver could long since have been accepted
if it were not for that. Besides that, I really believe that any system designer
using the highest permitted frequency should be willing to live with the
consequences, and not force the implementation of a a complex driver.

Ultimately, you'll have to decide if you want a simple driver accepted, or
a complex driver hanging in the review queue forever.

Thanks,
Guenter


Sorry to poke my head in late like this, but I do have a vested interest in the
outcome so I'm very curious.  For my work, I need to have an ACPI-supported,
SBSA-compliant watchdog timer for arm64, and this series is one of the key
pieces to getting there.  The plan for me has been: (1) get an FDT based SBSA
watchdog timer, (2) add in kernel code to handle the ACPI GTDT table describing
timers, then (3) munge the SBSA watchdog timer for use by ACPI.

So, is this an actual NAK of the patch series as is?  I think it is, but I want
it to be clear, and it has not been explicit yet.


I am not the maintainer, so I don't make the call. All I am saying is that I 
don't
feel comfortable with the code as is. Part of it is due to the the 
specification's
complexity, which leaves space for (mis)interpretations and bad implementations.

Either case, this is just my personal opinion. All you'll have to do is to 
convince
Wim to accept your patch.


If it is a NAK, that's fine, but I also want to be sure I understand what the
objections are.  Based on my understanding of the discussion so far over the
multiple versions, I think the primary objection is that the use of pretimeout
makes this driver too complex, and indeed complex enough that there is some
concern that it could destabilize a running system.  Do I have that right?

The other possible item I could conclude out of the discussion is that we do
not want to have the pretimeout code as part of the watchdog framework; is that
also the case or did I misunderstand?


Nothing really to do with pretimeout, but with the complexity of implementing it
for this driver.

As for pretimeout, it does have its issues. Some people say that hitting
a pretimeout should result in a panic, others just as strongly say that it
should just dump a message to the console. Which does make me a bit wary, since
it means that it may be implemented differently in different drivers, which
I consider highly undesirable.


And finally, a simpler, single stage timeout watchdog driver would be a
reasonable thing to accept, yes?  I can see where that would make sense.


I am quite sure that such a driver would long since have been accepted.


The issue for me in that case is that the SBSA requires a two stage timeout,


Hmm - really ? This makes me want to step back a bit and re-read the 
specification
to understand where it says that, and what the reasoning might be for such a
requirement.


so a single stage driver has no real value for me.  Now, if I can later add in
changes to make the driver into a two stage driver so it is SBSA-compliant,
that would also work, but it will make the driver more complex again.  At that
point, I think I've now gone in a logical circle and the changes would not be
accepted so I could never get to my goal of an SBSA-compliant driver.  I don't
think that's what was meant, so what did I miss?

Thanks in advance for any clarifications that can be provided.I really do
appreciate it.  Email is not always the clearest mechanism for communication
so sometimes I have to ask odd questions like these so I can understand what
is happening.



I don't really follow the logic here. Why ask if a single stage driver would
have been accepted just to point out that it would have no 

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

On 11/12/2015 06:06 PM, Al Stone wrote:

If it is a NAK, that's fine, but I also want to be sure I understand what the
objections are.  Based on my understanding of the discussion so far over the
multiple versions, I think the primary objection is that the use of pretimeout
makes this driver too complex, and indeed complex enough that there is some
concern that it could destabilize a running system.  Do I have that right?


I don't have a problem with the concept of pre-timeout per se.  My 
primary objection is this code:


> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
> +{
> +   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
> +   struct watchdog_device *wdd = >wdd;
> +
> +   /* We don't use pretimeout, trigger WS1 now */
> +   if (!wdd->pretimeout)
> +   sbsa_gwdt_set_wcv(wdd, 0);

This driver depends on an interrupt handler in order to properly program 
the hardware.  Unlike some other devices, the SBSA watchdog does not 
need assistance to reset on a timeout -- it is a "fire and forget" 
device.  What happens if there is a hard lockup, and interrupts no 
longer work?


The reason why Fu does this is because he wants to support a pre-timeout 
value that's independent of the timeout value.  The SBSA watchdog is 
normally programmed where real timeout equals twice the pre-timeout.  I 
would prefer that the driver adhere to this limitation.  That would 
eliminate the need to pre-program the hardware in the interrupt handler.



And finally, a simpler, single stage timeout watchdog driver would be a
reasonable thing to accept, yes?  I can see where that would make sense.


I would be okay with merging such a driver, and then enhancing it later 
to add pre-timeout support.



The issue for me in that case is that the SBSA requires a two stage timeout,
so a single stage driver has no real value for me.


There are plenty of existing watchdog devices that have a two-stage 
timeout but the driver treats it as a single stage.  The PowerPC 
watchdog driver is like that.  The hardware is programmed for the second 
stage to cause a hardware reset, and the interrupt handler is typically 
a no-op or just a printk().


--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Al Stone]

On 11/05/2015 09:41 AM, Guenter Roeck wrote:
> On 11/05/2015 07:00 AM, Fu Wei wrote:
>> Hi Timur,
>>
>> On 5 November 2015 at 22:40, Timur Tabi  wrote:
>>> Fu Wei wrote:

 Did you really read the "Note" above OK, let me paste it again
 and again:

 SBSA 2.3 Page 23 :
 If a larger watch period is required then the compare value can be
 programmed directly into the compare value register.
>>>
>>>
>>> Well, okay.  Sorry, I should have read what you pasted more closely. But I
>>
>> Thanks for reading it again.
>>
>>> think that means during initialization, not during the WS0 timeout.
>>
>> I really don't see SBSA say "during initialization, not during the WS0 
>> timeout",
>> please point it out the page number and the line number in SBSA spec.
>> maybe I miss it?
>> Thanks for your help in advance.
>>
>>>
>>> Anyway, I still don't like the fact that you're programming WCV in the
>>
>> "you don't like" doesn't mean "it is wrong" or "we can't do this", so
>> I will keep this way unless we have better idea to extend second stage
>> timeout.
>>
>>> interrupt handler, but I'm not going to make a big deal about it any more.
>>
>> Deal, Thanks a lot.
>>
> 
> The problem with your driver, as I see it, is that dealing with WS0/WS1
> and pretimeout makes the driver so complex that, at least for my part,
> I am very wary about it. The driver could long since have been accepted
> if it were not for that. Besides that, I really believe that any system 
> designer
> using the highest permitted frequency should be willing to live with the
> consequences, and not force the implementation of a a complex driver.
> 
> Ultimately, you'll have to decide if you want a simple driver accepted, or
> a complex driver hanging in the review queue forever.
> 
> Thanks,
> Guenter

Sorry to poke my head in late like this, but I do have a vested interest in the
outcome so I'm very curious.  For my work, I need to have an ACPI-supported,
SBSA-compliant watchdog timer for arm64, and this series is one of the key
pieces to getting there.  The plan for me has been: (1) get an FDT based SBSA
watchdog timer, (2) add in kernel code to handle the ACPI GTDT table describing
timers, then (3) munge the SBSA watchdog timer for use by ACPI.

So, is this an actual NAK of the patch series as is?  I think it is, but I want
it to be clear, and it has not been explicit yet.

If it is a NAK, that's fine, but I also want to be sure I understand what the
objections are.  Based on my understanding of the discussion so far over the
multiple versions, I think the primary objection is that the use of pretimeout
makes this driver too complex, and indeed complex enough that there is some
concern that it could destabilize a running system.  Do I have that right?

The other possible item I could conclude out of the discussion is that we do
not want to have the pretimeout code as part of the watchdog framework; is that
also the case or did I misunderstand?

And finally, a simpler, single stage timeout watchdog driver would be a
reasonable thing to accept, yes?  I can see where that would make sense.

The issue for me in that case is that the SBSA requires a two stage timeout,
so a single stage driver has no real value for me.  Now, if I can later add in
changes to make the driver into a two stage driver so it is SBSA-compliant,
that would also work, but it will make the driver more complex again.  At that
point, I think I've now gone in a logical circle and the changes would not be
accepted so I could never get to my goal of an SBSA-compliant driver.  I don't
think that's what was meant, so what did I miss?

Thanks in advance for any clarifications that can be provided.I really do
appreciate it.  Email is not always the clearest mechanism for communication
so sometimes I have to ask odd questions like these so I can understand what
is happening.

-- 
ciao,
al
---
Al Stone
Software Engineer
Linaro Enterprise Group
al.st...@linaro.org
---
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

On 11/12/2015 06:06 PM, Al Stone wrote:

If it is a NAK, that's fine, but I also want to be sure I understand what the
objections are.  Based on my understanding of the discussion so far over the
multiple versions, I think the primary objection is that the use of pretimeout
makes this driver too complex, and indeed complex enough that there is some
concern that it could destabilize a running system.  Do I have that right?


I don't have a problem with the concept of pre-timeout per se.  My 
primary objection is this code:


> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
> +{
> +   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
> +   struct watchdog_device *wdd = >wdd;
> +
> +   /* We don't use pretimeout, trigger WS1 now */
> +   if (!wdd->pretimeout)
> +   sbsa_gwdt_set_wcv(wdd, 0);

This driver depends on an interrupt handler in order to properly program 
the hardware.  Unlike some other devices, the SBSA watchdog does not 
need assistance to reset on a timeout -- it is a "fire and forget" 
device.  What happens if there is a hard lockup, and interrupts no 
longer work?


The reason why Fu does this is because he wants to support a pre-timeout 
value that's independent of the timeout value.  The SBSA watchdog is 
normally programmed where real timeout equals twice the pre-timeout.  I 
would prefer that the driver adhere to this limitation.  That would 
eliminate the need to pre-program the hardware in the interrupt handler.



And finally, a simpler, single stage timeout watchdog driver would be a
reasonable thing to accept, yes?  I can see where that would make sense.


I would be okay with merging such a driver, and then enhancing it later 
to add pre-timeout support.



The issue for me in that case is that the SBSA requires a two stage timeout,
so a single stage driver has no real value for me.


There are plenty of existing watchdog devices that have a two-stage 
timeout but the driver treats it as a single stage.  The PowerPC 
watchdog driver is like that.  The hardware is programmed for the second 
stage to cause a hardware reset, and the interrupt handler is typically 
a no-op or just a printk().


--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Al Stone]

On 11/05/2015 09:41 AM, Guenter Roeck wrote:
> On 11/05/2015 07:00 AM, Fu Wei wrote:
>> Hi Timur,
>>
>> On 5 November 2015 at 22:40, Timur Tabi  wrote:
>>> Fu Wei wrote:

 Did you really read the "Note" above OK, let me paste it again
 and again:

 SBSA 2.3 Page 23 :
 If a larger watch period is required then the compare value can be
 programmed directly into the compare value register.
>>>
>>>
>>> Well, okay.  Sorry, I should have read what you pasted more closely. But I
>>
>> Thanks for reading it again.
>>
>>> think that means during initialization, not during the WS0 timeout.
>>
>> I really don't see SBSA say "during initialization, not during the WS0 
>> timeout",
>> please point it out the page number and the line number in SBSA spec.
>> maybe I miss it?
>> Thanks for your help in advance.
>>
>>>
>>> Anyway, I still don't like the fact that you're programming WCV in the
>>
>> "you don't like" doesn't mean "it is wrong" or "we can't do this", so
>> I will keep this way unless we have better idea to extend second stage
>> timeout.
>>
>>> interrupt handler, but I'm not going to make a big deal about it any more.
>>
>> Deal, Thanks a lot.
>>
> 
> The problem with your driver, as I see it, is that dealing with WS0/WS1
> and pretimeout makes the driver so complex that, at least for my part,
> I am very wary about it. The driver could long since have been accepted
> if it were not for that. Besides that, I really believe that any system 
> designer
> using the highest permitted frequency should be willing to live with the
> consequences, and not force the implementation of a a complex driver.
> 
> Ultimately, you'll have to decide if you want a simple driver accepted, or
> a complex driver hanging in the review queue forever.
> 
> Thanks,
> Guenter

Sorry to poke my head in late like this, but I do have a vested interest in the
outcome so I'm very curious.  For my work, I need to have an ACPI-supported,
SBSA-compliant watchdog timer for arm64, and this series is one of the key
pieces to getting there.  The plan for me has been: (1) get an FDT based SBSA
watchdog timer, (2) add in kernel code to handle the ACPI GTDT table describing
timers, then (3) munge the SBSA watchdog timer for use by ACPI.

So, is this an actual NAK of the patch series as is?  I think it is, but I want
it to be clear, and it has not been explicit yet.

If it is a NAK, that's fine, but I also want to be sure I understand what the
objections are.  Based on my understanding of the discussion so far over the
multiple versions, I think the primary objection is that the use of pretimeout
makes this driver too complex, and indeed complex enough that there is some
concern that it could destabilize a running system.  Do I have that right?

The other possible item I could conclude out of the discussion is that we do
not want to have the pretimeout code as part of the watchdog framework; is that
also the case or did I misunderstand?

And finally, a simpler, single stage timeout watchdog driver would be a
reasonable thing to accept, yes?  I can see where that would make sense.

The issue for me in that case is that the SBSA requires a two stage timeout,
so a single stage driver has no real value for me.  Now, if I can later add in
changes to make the driver into a two stage driver so it is SBSA-compliant,
that would also work, but it will make the driver more complex again.  At that
point, I think I've now gone in a logical circle and the changes would not be
accepted so I could never get to my goal of an SBSA-compliant driver.  I don't
think that's what was meant, so what did I miss?

Thanks in advance for any clarifications that can be provided.I really do
appreciate it.  Email is not always the clearest mechanism for communication
so sometimes I have to ask odd questions like these so I can understand what
is happening.

-- 
ciao,
al
---
Al Stone
Software Engineer
Linaro Enterprise Group
al.st...@linaro.org
---
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Guenter Roeck]

On 11/12/2015 04:06 PM, Al Stone wrote:

On 11/05/2015 09:41 AM, Guenter Roeck wrote:

On 11/05/2015 07:00 AM, Fu Wei wrote:

Hi Timur,

On 5 November 2015 at 22:40, Timur Tabi  wrote:

Fu Wei wrote:


Did you really read the "Note" above OK, let me paste it again
and again:

SBSA 2.3 Page 23 :
If a larger watch period is required then the compare value can be
programmed directly into the compare value register.



Well, okay.  Sorry, I should have read what you pasted more closely. But I


Thanks for reading it again.


think that means during initialization, not during the WS0 timeout.


I really don't see SBSA say "during initialization, not during the WS0 timeout",
please point it out the page number and the line number in SBSA spec.
maybe I miss it?
Thanks for your help in advance.



Anyway, I still don't like the fact that you're programming WCV in the


"you don't like" doesn't mean "it is wrong" or "we can't do this", so
I will keep this way unless we have better idea to extend second stage
timeout.


interrupt handler, but I'm not going to make a big deal about it any more.


Deal, Thanks a lot.



The problem with your driver, as I see it, is that dealing with WS0/WS1
and pretimeout makes the driver so complex that, at least for my part,
I am very wary about it. The driver could long since have been accepted
if it were not for that. Besides that, I really believe that any system designer
using the highest permitted frequency should be willing to live with the
consequences, and not force the implementation of a a complex driver.

Ultimately, you'll have to decide if you want a simple driver accepted, or
a complex driver hanging in the review queue forever.

Thanks,
Guenter


Sorry to poke my head in late like this, but I do have a vested interest in the
outcome so I'm very curious.  For my work, I need to have an ACPI-supported,
SBSA-compliant watchdog timer for arm64, and this series is one of the key
pieces to getting there.  The plan for me has been: (1) get an FDT based SBSA
watchdog timer, (2) add in kernel code to handle the ACPI GTDT table describing
timers, then (3) munge the SBSA watchdog timer for use by ACPI.

So, is this an actual NAK of the patch series as is?  I think it is, but I want
it to be clear, and it has not been explicit yet.


I am not the maintainer, so I don't make the call. All I am saying is that I 
don't
feel comfortable with the code as is. Part of it is due to the the 
specification's
complexity, which leaves space for (mis)interpretations and bad implementations.

Either case, this is just my personal opinion. All you'll have to do is to 
convince
Wim to accept your patch.


If it is a NAK, that's fine, but I also want to be sure I understand what the
objections are.  Based on my understanding of the discussion so far over the
multiple versions, I think the primary objection is that the use of pretimeout
makes this driver too complex, and indeed complex enough that there is some
concern that it could destabilize a running system.  Do I have that right?

The other possible item I could conclude out of the discussion is that we do
not want to have the pretimeout code as part of the watchdog framework; is that
also the case or did I misunderstand?


Nothing really to do with pretimeout, but with the complexity of implementing it
for this driver.

As for pretimeout, it does have its issues. Some people say that hitting
a pretimeout should result in a panic, others just as strongly say that it
should just dump a message to the console. Which does make me a bit wary, since
it means that it may be implemented differently in different drivers, which
I consider highly undesirable.


And finally, a simpler, single stage timeout watchdog driver would be a
reasonable thing to accept, yes?  I can see where that would make sense.


I am quite sure that such a driver would long since have been accepted.


The issue for me in that case is that the SBSA requires a two stage timeout,


Hmm - really ? This makes me want to step back a bit and re-read the 
specification
to understand where it says that, and what the reasoning might be for such a
requirement.


so a single stage driver has no real value for me.  Now, if I can later add in
changes to make the driver into a two stage driver so it is SBSA-compliant,
that would also work, but it will make the driver more complex again.  At that
point, I think I've now gone in a logical circle and the changes would not be
accepted so I could never get to my goal of an SBSA-compliant driver.  I don't
think that's what was meant, so what did I miss?

Thanks in advance for any clarifications that can be provided.I really do
appreciate it.  Email is not always the clearest mechanism for communication
so sometimes I have to ask odd questions like these so I can understand what
is happening.



I don't really follow the logic here. Why ask if a single stage driver would
have been accepted just to point out that 

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Timur

On 6 November 2015 at 01:59, Timur Tabi  wrote:
> On 11/05/2015 10:41 AM, Guenter Roeck wrote:
>>
>>
>> Ultimately, you'll have to decide if you want a simple driver accepted, or
>> a complex driver hanging in the review queue forever.
>
>
> Please note that I did post such a driver back in May:
>
> http://www.spinics.net/lists/linux-watchdog/msg06567.html
>
> Officially I withdrew it in favor of Fu's, but the decision which driver to
> pick up is not mine to make.  I can resubmit driver if you'd like.

I totally  can not  agree with "make WS1 as backup",
and that not a SBSA watchdog dirver, because that just have one stage

And the short timeout is useless for kexec/kdump in real practice.

>
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.



-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

On 11/05/2015 10:41 AM, Guenter Roeck wrote:


Ultimately, you'll have to decide if you want a simple driver accepted, or
a complex driver hanging in the review queue forever.


Please note that I did post such a driver back in May:

http://www.spinics.net/lists/linux-watchdog/msg06567.html

Officially I withdrew it in favor of Fu's, but the decision which driver 
to pick up is not mine to make.  I can resubmit driver if you'd like.


--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Guenter,

Great thanks for that you are still reviewing this patchset, thanks
for your patient.

On 6 November 2015 at 00:41, Guenter Roeck  wrote:
> On 11/05/2015 07:00 AM, Fu Wei wrote:
>>
>> Hi Timur,
>>
>> On 5 November 2015 at 22:40, Timur Tabi  wrote:
>>>
>>> Fu Wei wrote:


 Did you really read the "Note" above OK, let me paste it again
 and again:

 SBSA 2.3 Page 23 :
 If a larger watch period is required then the compare value can be
 programmed directly into the compare value register.
>>>
>>>
>>>
>>> Well, okay.  Sorry, I should have read what you pasted more closely. But
>>> I
>>
>>
>> Thanks for reading it again.
>>
>>> think that means during initialization, not during the WS0 timeout.
>>
>>
>> I really don't see SBSA say "during initialization, not during the WS0
>> timeout",
>> please point it out the page number and the line number in SBSA spec.
>> maybe I miss it?
>> Thanks for your help in advance.
>>
>>>
>>> Anyway, I still don't like the fact that you're programming WCV in the
>>
>>
>> "you don't like" doesn't mean "it is wrong" or "we can't do this", so
>> I will keep this way unless we have better idea to extend second stage
>> timeout.
>>
>>> interrupt handler, but I'm not going to make a big deal about it any
>>> more.
>>
>>
>> Deal, Thanks a lot.
>>
>
> The problem with your driver, as I see it, is that dealing with WS0/WS1
> and pretimeout makes the driver so complex that, at least for my part,
> I am very wary about it. The driver could long since have been accepted
> if it were not for that. Besides that, I really believe that any system
> designer
> using the highest permitted frequency should be willing to live with the
> consequences, and not force the implementation of a a complex driver.

yes, comparing with those "one stage" watchdogs driver, I admit my
SBSA driver is a little complex
this complex come from :
(1) In SBSA spec, there are two stage.

because of these two stages, I looked for the solution in the kernel,
then I found "pretimeout".
I have explained a lot why I decide to use pretimeout which is
existing concept in Linux kernel
I just tried to introduce a existing concept  into watchdog framework.
Maybe people will say, there is only two or one drivers use that, but
it doesn't mean we can't have more in the future.
Maybe people will say, let's do this when we have more  two stages
watchdog. But if I need this now,  why not just make it this time.

if we use this driver as one stage watchdog,  it violates the SBSA
spec, and it can not be call SBSA watchdog driver.

(2) in SBSA watchdog, WOR is a 32bit register, we have a timeout limitation.

first of all, according to kernel documentation,
Documentation/watchdog/watchdog-api.txt
-
Pretimeouts:

Some watchdog timers can be set to have a trigger go off before the
actual time they will reset the system.  This can be done with an NMI,
interrupt, or other mechanism.  This allows Linux to record useful
information (like panic information and kernel coredumps) before it
resets.
---
So I decide to use panic() in first timeout handler, then we can use kdump

But in the real practice, 10s is not enough for kexec(let alone
kdump), even we can have 100s, 200s, it is not enough for a real
server which has huge ram to finish kdump.
So I decide to reprogram WCV for longer timer value.

except this two point, there is not more complex than other watchdog drivers.

And for these two complex point, I have explained the reason.
And this driver has been test on two platforms

Foundation model
Seattle

and will be more.

And this driver has been test with kdump, and works.

>
> Ultimately, you'll have to decide if you want a simple driver accepted, or
> a complex driver hanging in the review queue forever.

I have tried to simplify this driver, and I understand why we need a
simple driver, and why you are wary about it.
Even we want "simple" driver,  but we can't ignore the SBSA watchdog
feature in the spec.

If I can find a better way to solve these two complex point to make
this driver simple, I will absolutely do it.
But before we get better solution, I can not make a "simple" non-SBSA
watchdog driver to be accepted just for "my driver be accepted".
at least I need to convince myself: my driver is not just a simple
driver, but also a right driver for target device.
Maybe it will take a very long time,  but at least for now, I believe
I am doing the right way.

If you have suggestion to make this driver simple, I absolutely listen
to it and try to do it just like previous review.

Again, I appreciate your help and review very much. :-)


>
> 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
--
To unsubscribe from this 

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Guenter Roeck]

On 11/05/2015 07:00 AM, Fu Wei wrote:

Hi Timur,

On 5 November 2015 at 22:40, Timur Tabi  wrote:

Fu Wei wrote:


Did you really read the "Note" above OK, let me paste it again
and again:

SBSA 2.3 Page 23 :
If a larger watch period is required then the compare value can be
programmed directly into the compare value register.



Well, okay.  Sorry, I should have read what you pasted more closely. But I


Thanks for reading it again.


think that means during initialization, not during the WS0 timeout.


I really don't see SBSA say "during initialization, not during the WS0 timeout",
please point it out the page number and the line number in SBSA spec.
maybe I miss it?
Thanks for your help in advance.



Anyway, I still don't like the fact that you're programming WCV in the


"you don't like" doesn't mean "it is wrong" or "we can't do this", so
I will keep this way unless we have better idea to extend second stage
timeout.


interrupt handler, but I'm not going to make a big deal about it any more.


Deal, Thanks a lot.



The problem with your driver, as I see it, is that dealing with WS0/WS1
and pretimeout makes the driver so complex that, at least for my part,
I am very wary about it. The driver could long since have been accepted
if it were not for that. Besides that, I really believe that any system designer
using the highest permitted frequency should be willing to live with the
consequences, and not force the implementation of a a complex driver.

Ultimately, you'll have to decide if you want a simple driver accepted, or
a complex driver hanging in the review queue forever.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Timur,

On 5 November 2015 at 22:40, Timur Tabi  wrote:
> Fu Wei wrote:
>>
>> Did you really read the "Note" above OK, let me paste it again
>> and again:
>>
>> SBSA 2.3 Page 23 :
>> If a larger watch period is required then the compare value can be
>> programmed directly into the compare value register.
>
>
> Well, okay.  Sorry, I should have read what you pasted more closely. But I

Thanks for reading it again.

> think that means during initialization, not during the WS0 timeout.

I really don't see SBSA say "during initialization, not during the WS0 timeout",
please point it out the page number and the line number in SBSA spec.
maybe I miss it?
Thanks for your help in advance.

>
> Anyway, I still don't like the fact that you're programming WCV in the

"you don't like" doesn't mean "it is wrong" or "we can't do this", so
I will keep this way unless we have better idea to extend second stage
timeout.

> interrupt handler, but I'm not going to make a big deal about it any more.

Deal, Thanks a lot.

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



-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Fu Wei wrote:

Did you really read the "Note" above OK, let me paste it again
and again:

SBSA 2.3 Page 23 :
If a larger watch period is required then the compare value can be
programmed directly into the compare value register.


Well, okay.  Sorry, I should have read what you pasted more closely. 
But I think that means during initialization, not during the WS0 timeout.


Anyway, I still don't like the fact that you're programming WCV in the 
interrupt handler, but I'm not going to make a big deal about it any more.


--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Timur,

On 5 November 2015 at 22:08, Timur Tabi  wrote:
> Fu Wei wrote:
>>
>> SBSA 2.3 Page 23 :
>> Note: the watchdog offset register is 32 bits wide. This gives a
>> maximum watch period of around 10s at a system
>> counter frequency of 400MHz. If a larger watch period is required then
>> the compare value can be programmed
>> directly into the compare value register.
>>
>> 214s means your system counter is approximately at 20MHz which is in
>> the range of (10MHz ~ 400MHz)
>>
>> SBSA 2.3 Page 13 :
>> The System Counter (of the Generic Timer) shall run at a minimum
>> frequency of 10MHz and maximum of
>> 400MHz.
>
>
> Thanks, that explains a lot.
>
> If we expected customers to have a lower system counter frequency, then we
> wouldn't have to worry about the timeouts being too short.  It seems to me
> that the SBSA spec says that if you want a longer timeout, you have to lower
> the frequency.

Did you really read the "Note" above OK, let me paste it again
and again:

SBSA 2.3 Page 23 :
If a larger watch period is required then the compare value can be
programmed directly into the compare value register.

> We shouldn't be complicating the driver because some
> customers might not follow the spec.

OK it this customer might not follow the spec, that watchdog is not a
SBSA watchdog,
So please don't use SBSA watchdog driver on that non-SBSA watchdog
device, Thanks a lot

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



-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Fu Wei wrote:

SBSA 2.3 Page 23 :
Note: the watchdog offset register is 32 bits wide. This gives a
maximum watch period of around 10s at a system
counter frequency of 400MHz. If a larger watch period is required then
the compare value can be programmed
directly into the compare value register.

214s means your system counter is approximately at 20MHz which is in
the range of (10MHz ~ 400MHz)

SBSA 2.3 Page 13 :
The System Counter (of the Generic Timer) shall run at a minimum
frequency of 10MHz and maximum of
400MHz.


Thanks, that explains a lot.

If we expected customers to have a lower system counter frequency, then 
we wouldn't have to worry about the timeouts being too short.  It seems 
to me that the SBSA spec says that if you want a longer timeout, you 
have to lower the frequency.  We shouldn't be complicating the driver 
because some customers might not follow the spec.


--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Timur

On 5 November 2015 at 21:47, Timur Tabi  wrote:
> Guenter Roeck wrote:
>>
>> I would feel much more comfortable if the driver would just use the
>> standard
>> watchdog timeout and live with (worst case) 20 seconds timeout for now.
>
>
> Actually, I'm wondering where the 20 seconds comes from.  When I load my
> driver on our hardware, it calculates a maximum timeout of 214 seconds, and
> that's just to WS0.

SBSA 2.3 Page 23 :
Note: the watchdog offset register is 32 bits wide. This gives a
maximum watch period of around 10s at a system
counter frequency of 400MHz. If a larger watch period is required then
the compare value can be programmed
directly into the compare value register.

214s means your system counter is approximately at 20MHz which is in
the range of (10MHz ~ 400MHz)

SBSA 2.3 Page 13 :
The System Counter (of the Generic Timer) shall run at a minimum
frequency of 10MHz and maximum of
400MHz.

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



-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Guenter Roeck wrote:

I would feel much more comfortable if the driver would just use the
standard
watchdog timeout and live with (worst case) 20 seconds timeout for now.


Actually, I'm wondering where the 20 seconds comes from.  When I load my 
driver on our hardware, it calculates a maximum timeout of 214 seconds, 
and that's just to WS0.


--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Fu Wei wrote:

(1)It is not new.
  pre-timeout concept has been used by two drivers before this driver.
and this concept has been in kernel documentation.


It's "new" in that it's a new infrastructure.  The private API of two 
other drivers doesn't count.



(1) if we don't, for this two stages timeout, we have to config them
by one value.
that means "the first stage timeout" have to be equal to "the second
stage timeout",
For example, if we need 60 second for  "the second stage timeout", 30
or less for "the first stage timeout".
then "the first stage timeout" have to be 60s too. I don't think it 's
good idea.


Why do we care about two stages?  Don't have a pre-timeout, and just 
have one stage: the WS1 reset.  Ignore the WS0 interrupt, and program 
the timeout so that WS1 is the reset.


I'm not saying that pre-timeout is a terrible idea and we should never 
do it.  I'm saying that it's not an important feature, and we should 
only support it to the extent that the hardware provides the feature. 
We should definitely not make the driver more complicated and less safe.


If we agree that an SBSA watchdog allows for a pre-timeout at half-way 
through timeout, and that software can't change this, then we can use 
WS0 as the pre-timeout and applications just have to deal with that. 
The hardware is programmed to reset via WS1, and all we do in the 
interrupt handler is notify the application that a pre-timeout has 
occurred.


--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Guenter,

Great thanks for your feedback!

On 5 November 2015 at 13:13, Guenter Roeck  wrote:
> On 11/04/2015 05:59 PM, Timur Tabi wrote:
>>
>> On Tue, Oct 27, 2015 at 11:06 AM,   wrote:
>>>
>>> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
>>> +{
>>> +   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
>>> +   struct watchdog_device *wdd = >wdd;
>>> +
>>> +   /* We don't use pretimeout, trigger WS1 now */
>>> +   if (!wdd->pretimeout)
>>> +   sbsa_gwdt_set_wcv(wdd, 0);
>>
>>
>> So I'm still concerned about the fact this driver depends on an
>> interrupt handler in order to properly program the hardware.  Unlike
>> some other devices, the SBSA watchdog does not need assistance to
>> reset on a timeout -- it is a "fire and forget" device.  What happens
>> if there is a hard lockup, and interrupts no longer work?

The reason for this design(program WCV in interrupt handler):
(1) if we don't, the second timeout stage(pretimeout) is only  (worst
case) 10 seconds
This short time is not enough for kexec(let alone kdump), that make
panic less useful.
(2)if  a hard lockup really happens, panic won't work too.But we still
can reboot system by the help of WS1
in this case, if clk is 400MHz, we just need to wait (worst case) 10
seconds for WS1 reboot system

>>
>> Keep in mind that 99% of watchdog daemons will not enable the
>> pre-timeout feature because it's new.

Answer:
(1)It is not new.
 pre-timeout concept has been used by two drivers before this driver.
and this concept has been in kernel documentation.

(2)even it's new, it doesn't mean we can not do this at this time.
Because according to the info I got, I believe that is right way to do.
After I make a "non-pretimeout" version. and compare with the original
pretimeout version, I still believe pretimeout is best solution for
now.

Reason for using pretimeout:
(1) if we don't, for this two stages timeout, we have to config them
by one value.
that means "the first stage timeout" have to be equal to "the second
stage timeout",
For example, if we need 60 second for  "the second stage timeout", 30
or less for "the first stage timeout".
then "the first stage timeout" have to be 60s too. I don't think it 's
good idea.

>>
> Same here, really.
>
> I would feel much more comfortable if the driver would just use the standard
> watchdog timeout and live with (worst case) 20 seconds timeout for now.

The worst case is 10s. like I said above, This short time is not
enough for kexec(let alone kdump), that make WS0(and panic, even this
two stages design) less useful

> This limitation will be gone once the infrastructure is in place to handle
> such short timeouts in the watchdog core. Until then, I would argue that the

unless WOR become 64 bit (or more then 32bit),  this limitation will be there.

> system designers asked for it if they really select the highest possible
> clock rate.
>

even we can make clk to be 100MHz or lower, it is not very helpful for
a really server which has big memory. they need more time for dumping
memory for debug/analysis


> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Timur,

On 5 November 2015 at 22:08, Timur Tabi  wrote:
> Fu Wei wrote:
>>
>> SBSA 2.3 Page 23 :
>> Note: the watchdog offset register is 32 bits wide. This gives a
>> maximum watch period of around 10s at a system
>> counter frequency of 400MHz. If a larger watch period is required then
>> the compare value can be programmed
>> directly into the compare value register.
>>
>> 214s means your system counter is approximately at 20MHz which is in
>> the range of (10MHz ~ 400MHz)
>>
>> SBSA 2.3 Page 13 :
>> The System Counter (of the Generic Timer) shall run at a minimum
>> frequency of 10MHz and maximum of
>> 400MHz.
>
>
> Thanks, that explains a lot.
>
> If we expected customers to have a lower system counter frequency, then we
> wouldn't have to worry about the timeouts being too short.  It seems to me
> that the SBSA spec says that if you want a longer timeout, you have to lower
> the frequency.

Did you really read the "Note" above OK, let me paste it again
and again:

SBSA 2.3 Page 23 :
If a larger watch period is required then the compare value can be
programmed directly into the compare value register.

> We shouldn't be complicating the driver because some
> customers might not follow the spec.

OK it this customer might not follow the spec, that watchdog is not a
SBSA watchdog,
So please don't use SBSA watchdog driver on that non-SBSA watchdog
device, Thanks a lot

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



-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Fu Wei wrote:

Did you really read the "Note" above OK, let me paste it again
and again:

SBSA 2.3 Page 23 :
If a larger watch period is required then the compare value can be
programmed directly into the compare value register.


Well, okay.  Sorry, I should have read what you pasted more closely. 
But I think that means during initialization, not during the WS0 timeout.


Anyway, I still don't like the fact that you're programming WCV in the 
interrupt handler, but I'm not going to make a big deal about it any more.


--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Guenter,

Great thanks for your feedback!

On 5 November 2015 at 13:13, Guenter Roeck  wrote:
> On 11/04/2015 05:59 PM, Timur Tabi wrote:
>>
>> On Tue, Oct 27, 2015 at 11:06 AM,   wrote:
>>>
>>> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
>>> +{
>>> +   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
>>> +   struct watchdog_device *wdd = >wdd;
>>> +
>>> +   /* We don't use pretimeout, trigger WS1 now */
>>> +   if (!wdd->pretimeout)
>>> +   sbsa_gwdt_set_wcv(wdd, 0);
>>
>>
>> So I'm still concerned about the fact this driver depends on an
>> interrupt handler in order to properly program the hardware.  Unlike
>> some other devices, the SBSA watchdog does not need assistance to
>> reset on a timeout -- it is a "fire and forget" device.  What happens
>> if there is a hard lockup, and interrupts no longer work?

The reason for this design(program WCV in interrupt handler):
(1) if we don't, the second timeout stage(pretimeout) is only  (worst
case) 10 seconds
This short time is not enough for kexec(let alone kdump), that make
panic less useful.
(2)if  a hard lockup really happens, panic won't work too.But we still
can reboot system by the help of WS1
in this case, if clk is 400MHz, we just need to wait (worst case) 10
seconds for WS1 reboot system

>>
>> Keep in mind that 99% of watchdog daemons will not enable the
>> pre-timeout feature because it's new.

Answer:
(1)It is not new.
 pre-timeout concept has been used by two drivers before this driver.
and this concept has been in kernel documentation.

(2)even it's new, it doesn't mean we can not do this at this time.
Because according to the info I got, I believe that is right way to do.
After I make a "non-pretimeout" version. and compare with the original
pretimeout version, I still believe pretimeout is best solution for
now.

Reason for using pretimeout:
(1) if we don't, for this two stages timeout, we have to config them
by one value.
that means "the first stage timeout" have to be equal to "the second
stage timeout",
For example, if we need 60 second for  "the second stage timeout", 30
or less for "the first stage timeout".
then "the first stage timeout" have to be 60s too. I don't think it 's
good idea.

>>
> Same here, really.
>
> I would feel much more comfortable if the driver would just use the standard
> watchdog timeout and live with (worst case) 20 seconds timeout for now.

The worst case is 10s. like I said above, This short time is not
enough for kexec(let alone kdump), that make WS0(and panic, even this
two stages design) less useful

> This limitation will be gone once the infrastructure is in place to handle
> such short timeouts in the watchdog core. Until then, I would argue that the

unless WOR become 64 bit (or more then 32bit),  this limitation will be there.

> system designers asked for it if they really select the highest possible
> clock rate.
>

even we can make clk to be 100MHz or lower, it is not very helpful for
a really server which has big memory. they need more time for dumping
memory for debug/analysis


> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Guenter,

Great thanks for that you are still reviewing this patchset, thanks
for your patient.

On 6 November 2015 at 00:41, Guenter Roeck  wrote:
> On 11/05/2015 07:00 AM, Fu Wei wrote:
>>
>> Hi Timur,
>>
>> On 5 November 2015 at 22:40, Timur Tabi  wrote:
>>>
>>> Fu Wei wrote:


 Did you really read the "Note" above OK, let me paste it again
 and again:

 SBSA 2.3 Page 23 :
 If a larger watch period is required then the compare value can be
 programmed directly into the compare value register.
>>>
>>>
>>>
>>> Well, okay.  Sorry, I should have read what you pasted more closely. But
>>> I
>>
>>
>> Thanks for reading it again.
>>
>>> think that means during initialization, not during the WS0 timeout.
>>
>>
>> I really don't see SBSA say "during initialization, not during the WS0
>> timeout",
>> please point it out the page number and the line number in SBSA spec.
>> maybe I miss it?
>> Thanks for your help in advance.
>>
>>>
>>> Anyway, I still don't like the fact that you're programming WCV in the
>>
>>
>> "you don't like" doesn't mean "it is wrong" or "we can't do this", so
>> I will keep this way unless we have better idea to extend second stage
>> timeout.
>>
>>> interrupt handler, but I'm not going to make a big deal about it any
>>> more.
>>
>>
>> Deal, Thanks a lot.
>>
>
> The problem with your driver, as I see it, is that dealing with WS0/WS1
> and pretimeout makes the driver so complex that, at least for my part,
> I am very wary about it. The driver could long since have been accepted
> if it were not for that. Besides that, I really believe that any system
> designer
> using the highest permitted frequency should be willing to live with the
> consequences, and not force the implementation of a a complex driver.

yes, comparing with those "one stage" watchdogs driver, I admit my
SBSA driver is a little complex
this complex come from :
(1) In SBSA spec, there are two stage.

because of these two stages, I looked for the solution in the kernel,
then I found "pretimeout".
I have explained a lot why I decide to use pretimeout which is
existing concept in Linux kernel
I just tried to introduce a existing concept  into watchdog framework.
Maybe people will say, there is only two or one drivers use that, but
it doesn't mean we can't have more in the future.
Maybe people will say, let's do this when we have more  two stages
watchdog. But if I need this now,  why not just make it this time.

if we use this driver as one stage watchdog,  it violates the SBSA
spec, and it can not be call SBSA watchdog driver.

(2) in SBSA watchdog, WOR is a 32bit register, we have a timeout limitation.

first of all, according to kernel documentation,
Documentation/watchdog/watchdog-api.txt
-
Pretimeouts:

Some watchdog timers can be set to have a trigger go off before the
actual time they will reset the system.  This can be done with an NMI,
interrupt, or other mechanism.  This allows Linux to record useful
information (like panic information and kernel coredumps) before it
resets.
---
So I decide to use panic() in first timeout handler, then we can use kdump

But in the real practice, 10s is not enough for kexec(let alone
kdump), even we can have 100s, 200s, it is not enough for a real
server which has huge ram to finish kdump.
So I decide to reprogram WCV for longer timer value.

except this two point, there is not more complex than other watchdog drivers.

And for these two complex point, I have explained the reason.
And this driver has been test on two platforms

Foundation model
Seattle

and will be more.

And this driver has been test with kdump, and works.

>
> Ultimately, you'll have to decide if you want a simple driver accepted, or
> a complex driver hanging in the review queue forever.

I have tried to simplify this driver, and I understand why we need a
simple driver, and why you are wary about it.
Even we want "simple" driver,  but we can't ignore the SBSA watchdog
feature in the spec.

If I can find a better way to solve these two complex point to make
this driver simple, I will absolutely do it.
But before we get better solution, I can not make a "simple" non-SBSA
watchdog driver to be accepted just for "my driver be accepted".
at least I need to convince myself: my driver is not just a simple
driver, but also a right driver for target device.
Maybe it will take a very long time,  but at least for now, I believe
I am doing the right way.

If you have suggestion to make this driver simple, I absolutely listen
to it and try to do it just like previous review.

Again, I appreciate your help and review very much. :-)


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

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Guenter Roeck]

On 11/05/2015 07:00 AM, Fu Wei wrote:

Hi Timur,

On 5 November 2015 at 22:40, Timur Tabi  wrote:

Fu Wei wrote:


Did you really read the "Note" above OK, let me paste it again
and again:

SBSA 2.3 Page 23 :
If a larger watch period is required then the compare value can be
programmed directly into the compare value register.



Well, okay.  Sorry, I should have read what you pasted more closely. But I


Thanks for reading it again.


think that means during initialization, not during the WS0 timeout.


I really don't see SBSA say "during initialization, not during the WS0 timeout",
please point it out the page number and the line number in SBSA spec.
maybe I miss it?
Thanks for your help in advance.



Anyway, I still don't like the fact that you're programming WCV in the


"you don't like" doesn't mean "it is wrong" or "we can't do this", so
I will keep this way unless we have better idea to extend second stage
timeout.


interrupt handler, but I'm not going to make a big deal about it any more.


Deal, Thanks a lot.



The problem with your driver, as I see it, is that dealing with WS0/WS1
and pretimeout makes the driver so complex that, at least for my part,
I am very wary about it. The driver could long since have been accepted
if it were not for that. Besides that, I really believe that any system designer
using the highest permitted frequency should be willing to live with the
consequences, and not force the implementation of a a complex driver.

Ultimately, you'll have to decide if you want a simple driver accepted, or
a complex driver hanging in the review queue forever.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Timur

On 6 November 2015 at 01:59, Timur Tabi  wrote:
> On 11/05/2015 10:41 AM, Guenter Roeck wrote:
>>
>>
>> Ultimately, you'll have to decide if you want a simple driver accepted, or
>> a complex driver hanging in the review queue forever.
>
>
> Please note that I did post such a driver back in May:
>
> http://www.spinics.net/lists/linux-watchdog/msg06567.html
>
> Officially I withdrew it in favor of Fu's, but the decision which driver to
> pick up is not mine to make.  I can resubmit driver if you'd like.

I totally  can not  agree with "make WS1 as backup",
and that not a SBSA watchdog dirver, because that just have one stage

And the short timeout is useless for kexec/kdump in real practice.

>
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.



-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

On 11/05/2015 10:41 AM, Guenter Roeck wrote:


Ultimately, you'll have to decide if you want a simple driver accepted, or
a complex driver hanging in the review queue forever.


Please note that I did post such a driver back in May:

http://www.spinics.net/lists/linux-watchdog/msg06567.html

Officially I withdrew it in favor of Fu's, but the decision which driver 
to pick up is not mine to make.  I can resubmit driver if you'd like.


--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Timur,

On 5 November 2015 at 22:40, Timur Tabi  wrote:
> Fu Wei wrote:
>>
>> Did you really read the "Note" above OK, let me paste it again
>> and again:
>>
>> SBSA 2.3 Page 23 :
>> If a larger watch period is required then the compare value can be
>> programmed directly into the compare value register.
>
>
> Well, okay.  Sorry, I should have read what you pasted more closely. But I

Thanks for reading it again.

> think that means during initialization, not during the WS0 timeout.

I really don't see SBSA say "during initialization, not during the WS0 timeout",
please point it out the page number and the line number in SBSA spec.
maybe I miss it?
Thanks for your help in advance.

>
> Anyway, I still don't like the fact that you're programming WCV in the

"you don't like" doesn't mean "it is wrong" or "we can't do this", so
I will keep this way unless we have better idea to extend second stage
timeout.

> interrupt handler, but I'm not going to make a big deal about it any more.

Deal, Thanks a lot.

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



-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Fu Wei wrote:

(1)It is not new.
  pre-timeout concept has been used by two drivers before this driver.
and this concept has been in kernel documentation.


It's "new" in that it's a new infrastructure.  The private API of two 
other drivers doesn't count.



(1) if we don't, for this two stages timeout, we have to config them
by one value.
that means "the first stage timeout" have to be equal to "the second
stage timeout",
For example, if we need 60 second for  "the second stage timeout", 30
or less for "the first stage timeout".
then "the first stage timeout" have to be 60s too. I don't think it 's
good idea.


Why do we care about two stages?  Don't have a pre-timeout, and just 
have one stage: the WS1 reset.  Ignore the WS0 interrupt, and program 
the timeout so that WS1 is the reset.


I'm not saying that pre-timeout is a terrible idea and we should never 
do it.  I'm saying that it's not an important feature, and we should 
only support it to the extent that the hardware provides the feature. 
We should definitely not make the driver more complicated and less safe.


If we agree that an SBSA watchdog allows for a pre-timeout at half-way 
through timeout, and that software can't change this, then we can use 
WS0 as the pre-timeout and applications just have to deal with that. 
The hardware is programmed to reset via WS1, and all we do in the 
interrupt handler is notify the application that a pre-timeout has 
occurred.


--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Fu Wei]

Hi Timur

On 5 November 2015 at 21:47, Timur Tabi  wrote:
> Guenter Roeck wrote:
>>
>> I would feel much more comfortable if the driver would just use the
>> standard
>> watchdog timeout and live with (worst case) 20 seconds timeout for now.
>
>
> Actually, I'm wondering where the 20 seconds comes from.  When I load my
> driver on our hardware, it calculates a maximum timeout of 214 seconds, and
> that's just to WS0.

SBSA 2.3 Page 23 :
Note: the watchdog offset register is 32 bits wide. This gives a
maximum watch period of around 10s at a system
counter frequency of 400MHz. If a larger watch period is required then
the compare value can be programmed
directly into the compare value register.

214s means your system counter is approximately at 20MHz which is in
the range of (10MHz ~ 400MHz)

SBSA 2.3 Page 13 :
The System Counter (of the Generic Timer) shall run at a minimum
frequency of 10MHz and maximum of
400MHz.

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



-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Guenter Roeck wrote:

I would feel much more comfortable if the driver would just use the
standard
watchdog timeout and live with (worst case) 20 seconds timeout for now.


Actually, I'm wondering where the 20 seconds comes from.  When I load my 
driver on our hardware, it calculates a maximum timeout of 214 seconds, 
and that's just to WS0.


--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

Fu Wei wrote:

SBSA 2.3 Page 23 :
Note: the watchdog offset register is 32 bits wide. This gives a
maximum watch period of around 10s at a system
counter frequency of 400MHz. If a larger watch period is required then
the compare value can be programmed
directly into the compare value register.

214s means your system counter is approximately at 20MHz which is in
the range of (10MHz ~ 400MHz)

SBSA 2.3 Page 13 :
The System Counter (of the Generic Timer) shall run at a minimum
frequency of 10MHz and maximum of
400MHz.


Thanks, that explains a lot.

If we expected customers to have a lower system counter frequency, then 
we wouldn't have to worry about the timeouts being too short.  It seems 
to me that the SBSA spec says that if you want a longer timeout, you 
have to lower the frequency.  We shouldn't be complicating the driver 
because some customers might not follow the spec.


--
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Guenter Roeck]

On 11/04/2015 05:59 PM, Timur Tabi wrote:

On Tue, Oct 27, 2015 at 11:06 AM,   wrote:

+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
+{
+   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
+   struct watchdog_device *wdd = >wdd;
+
+   /* We don't use pretimeout, trigger WS1 now */
+   if (!wdd->pretimeout)
+   sbsa_gwdt_set_wcv(wdd, 0);


So I'm still concerned about the fact this driver depends on an
interrupt handler in order to properly program the hardware.  Unlike
some other devices, the SBSA watchdog does not need assistance to
reset on a timeout -- it is a "fire and forget" device.  What happens
if there is a hard lockup, and interrupts no longer work?

Keep in mind that 99% of watchdog daemons will not enable the
pre-timeout feature because it's new.


Same here, really.

I would feel much more comfortable if the driver would just use the standard
watchdog timeout and live with (worst case) 20 seconds timeout for now.
This limitation will be gone once the infrastructure is in place to handle
such short timeouts in the watchdog core. Until then, I would argue that the
system designers asked for it if they really select the highest possible
clock rate.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

On Tue, Oct 27, 2015 at 11:06 AM,   wrote:
> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
> +{
> +   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
> +   struct watchdog_device *wdd = >wdd;
> +
> +   /* We don't use pretimeout, trigger WS1 now */
> +   if (!wdd->pretimeout)
> +   sbsa_gwdt_set_wcv(wdd, 0);

So I'm still concerned about the fact this driver depends on an
interrupt handler in order to properly program the hardware.  Unlike
some other devices, the SBSA watchdog does not need assistance to
reset on a timeout -- it is a "fire and forget" device.  What happens
if there is a hard lockup, and interrupts no longer work?

Keep in mind that 99% of watchdog daemons will not enable the
pre-timeout feature because it's new.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Timur Tabi]

On Tue, Oct 27, 2015 at 11:06 AM,   wrote:
> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
> +{
> +   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
> +   struct watchdog_device *wdd = >wdd;
> +
> +   /* We don't use pretimeout, trigger WS1 now */
> +   if (!wdd->pretimeout)
> +   sbsa_gwdt_set_wcv(wdd, 0);

So I'm still concerned about the fact this driver depends on an
interrupt handler in order to properly program the hardware.  Unlike
some other devices, the SBSA watchdog does not need assistance to
reset on a timeout -- it is a "fire and forget" device.  What happens
if there is a hard lockup, and interrupts no longer work?

Keep in mind that 99% of watchdog daemons will not enable the
pre-timeout feature because it's new.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Linaro-acpi] [PATCH v8 5/5] Watchdog: introduce ARM SBSA watchdog driver

[Guenter Roeck]

On 11/04/2015 05:59 PM, Timur Tabi wrote:

On Tue, Oct 27, 2015 at 11:06 AM,   wrote:

+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
+{
+   struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
+   struct watchdog_device *wdd = >wdd;
+
+   /* We don't use pretimeout, trigger WS1 now */
+   if (!wdd->pretimeout)
+   sbsa_gwdt_set_wcv(wdd, 0);


So I'm still concerned about the fact this driver depends on an
interrupt handler in order to properly program the hardware.  Unlike
some other devices, the SBSA watchdog does not need assistance to
reset on a timeout -- it is a "fire and forget" device.  What happens
if there is a hard lockup, and interrupts no longer work?

Keep in mind that 99% of watchdog daemons will not enable the
pre-timeout feature because it's new.


Same here, really.

I would feel much more comfortable if the driver would just use the standard
watchdog timeout and live with (worst case) 20 seconds timeout for now.
This limitation will be gone once the infrastructure is in place to handle
such short timeouts in the watchdog core. Until then, I would argue that the
system designers asked for it if they really select the highest possible
clock rate.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/