Hi,
On 18.02.2015 23:11, Rob Herring wrote:
On Wed, Feb 18, 2015 at 10:00 AM, Alexandre Belloni
<alexandre.belloni@xxxxxxxxxxxxxxxxxx> wrote:
Hi,
On 18/02/2015 at 06:50:44 -0800, Guenter Roeck wrote :
Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds
+ that the driver keeps on ticking the watchdog HW on behalf of user
+ space. Once this timeout expires watchdog is left to expire in
+ timeout-sec seconds. If this propery is set to zero, watchdog is
+ started (or left running) so that a reset occurs in timeout-sec
+ since the watchdog was started.
Example:
watchdog {
timeout-sec = <60>;
+ early-timeout-sec = <120>;
That is not a generic property as you defined it; if so,
it would have to be implemented in the watchdog core code,
not in the at91 code. You'll have to document it in the bindings
description for at91sam9_wdt.
Then, if this is a controller specific property, it should be defined
with the 'atmel,' prefix.
We're kind of looping here: the initial discussion was "is there a need
for this property to be a generic one ?", and now you're saying no,
while you previously left the door opened.
Tomi is proposing a generic approach, as you asked him to. I agree that
parsing the property in core code and making its value part of the
generic watchdog struct makes sense (that's what I proposed to Tomi a
few weeks ago).
Hmm ... the problem here is that the property description creates the
assumption or expectation that the property is used if defined,
which is not the case.
I am not sure how to best resolve this. Maybe a comment in the property
description stating that implementation of is device (driver) dependent ?
After all, that is true for the timeout-sec property as well.
I would leave it in the generic file and state that it may not be
implemented in the driver. That way, the property is documented for new
driver writers.
That is pretty much true of any optional property. Whether implemented
in the driver or core is an implementation detail that does not belong
in the binding.
I find this property pretty questionable. Certainly having the kernel
service a watchdog either enabled at reset, in the bootloader, or by
the kernel is a useful feature. A timeout for "how long userspace
watchdog daemon takes to start" does not belong in DT. timeout-sec
should be the default/initial timeout and long enough for userspace to
start. Userspace can then change it to a more suitable value.
That would be a good workaround if we had enough time in the watchdog HW
to wait long enough for the user space to start up. For example in atmel
HW the maximum is 16 seconds, which may not be enough for the kernel to
boot up and the user space to start the watchdog daemon.
But even that is not enough as all of the watchdog drivers attempt to
*disable* the watchdog device before user space opens it. What good is a
watchdog if it is disabled by the kernel and we got stuck before the
daemon wakes up and re-enables it? This the problem with all of the
watchdog drivers right now. There are plenty of products out there that
can't deal with this kind of limitation. They are all hacking around it
one way or another. If there is a crash, the watchdog must reset the
device. I can't think of any other run time way to configure the
watchdog for this kind of situation than having a device tree property
for it.
What I am proposing here is a way to solve this without hacking. I was
told to think also a way to defer starting the watchdog for a given
timeout so that user space would have more time to wake up, which
sounded like a good idea. And this obviously needs to be implemented so
that the watchdog is guaranteed to reset the device should there be a
crash of any kind that prevents the watchdog daemon from starting up.
There are a lot of details that need to be taken care of properly and
therefore watchdog core can't do much about it, which is why I thought
there is no much point trying to do it in watchdog core.
But never the less I can try to state this in the documentation just to
make clear what we are trying to solve here.
Thanks for all the good comments!
-Timo
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
