at91sam9_wdt: Allow watchdog to reset device at early boot

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "atmel,no-early-timer" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
  drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
  2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

index f90e294..1b9289e 100644
@@ -28,6 +28,10 @@ Optional properties:
  	entering idle state.
  - atmel,dbg-halt : Should be present if you want to stop the watchdog when
  	entering debug state.
+- atmel,no-early-timer : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.
 
  Example:
  	watchdog at fffffd40 {

drivers/watchdog/at91sam9_wdt.c +5 -1

index 489729b..8cac712 100644
@@ -89,6 +89,7 @@ struct at91wdt {
  	u32 mr_mask;
  	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
  	bool nowayout;
+	bool no_early_timer;
  	unsigned int irq;
  };
 

@@ -122,7 +123,7 @@ static void at91_ping(unsigned long data)
  {
  	struct at91wdt *wdt = (struct at91wdt *)data;
  	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
+		(!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) {
  		at91_wdt_reset(wdt);
  		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
  	} else {

@@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 
  	wdt->mr |= max | ((max - min) << 16);
 
+	if (of_property_read_bool(np, "atmel,no-early-timer"))
+		wdt->no_early_timer = 1;
+
  	return 0;
  }
  #else

Hi,

Quoted 10 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

Quoted 6 lines

To resolve the issue, add a new device tree property "atmel,no-early-timer" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Any thoughts on this one? Should I resend this with some more people or lists to get someone to comment or review this patch?

Quoted 33 lines

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
  drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
  2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

index f90e294..1b9289e 100644
@@ -28,6 +28,10 @@ Optional properties:
  	entering idle state.
  - atmel,dbg-halt : Should be present if you want to stop the watchdog when
  	entering debug state.
+- atmel,no-early-timer : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.
 
  Example:
  	watchdog at fffffd40 {

drivers/watchdog/at91sam9_wdt.c +1 -0

index 489729b..8cac712 100644
@@ -89,6 +89,7 @@ struct at91wdt {
  	u32 mr_mask;
  	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
  	bool nowayout;
+	bool no_early_timer;
  	unsigned int irq;
  };
 

drivers/watchdog/at91sam9_wdt.c static void at91_ping(unsigned long data) · line 126

Quoted 6 lines

drivers/watchdog/at91sam9_wdt.c +1 -1

@@ -122,7 +123,7 @@ static void at91_ping(unsigned long data)
  {
  	struct at91wdt *wdt = (struct at91wdt *)data;
  	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
+		(!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) {

Quoted 5 lines

drivers/watchdog/at91sam9_wdt.c +0 -0

  		at91_wdt_reset(wdt);
  		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
  	} else {

@@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 

drivers/watchdog/at91sam9_wdt.c static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt) · line 325

Quoted 8 lines

drivers/watchdog/at91sam9_wdt.c +3 -0

  	wdt->mr |= max | ((max - min) << 16);
 
+	if (of_property_read_bool(np, "atmel,no-early-timer"))
+		wdt->no_early_timer = 1;
+
  	return 0;
  }
  #else

On 12/11/2014 09:20, Timo Kokkonen :

Quoted 5 lines

Hi,

Any thoughts on this one? Should I resend this with some more people or lists to get someone to comment or review this patch?

I would like to know if this property added to the device-tree is already defined by another watchdog timer.

Indeed, it seems a pretty generic behavior and I am surprised it doesn't have an already existing implementation.

...

Quoted 33 lines

Quoted 33 lines

Signed-off-by Timo Kokkonen <[email protected]>

---
   Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
   drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
   2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

index f90e294..1b9289e 100644
@@ -28,6 +28,10 @@ Optional properties:
   	entering idle state.
   - atmel,dbg-halt : Should be present if you want to stop the watchdog when
   	entering debug state.
+- atmel,no-early-timer : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.
 
   Example:
   	watchdog at fffffd40 {

drivers/watchdog/at91sam9_wdt.c +1 -0

index 489729b..8cac712 100644
@@ -89,6 +89,7 @@ struct at91wdt {
   	u32 mr_mask;
   	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
   	bool nowayout;
+	bool no_early_timer;
   	unsigned int irq;
   };
 

Quoted 6 lines

drivers/watchdog/at91sam9_wdt.c static void at91_ping(unsigned long data) · line 126

Quoted 6 lines

drivers/watchdog/at91sam9_wdt.c +1 -1

@@ -122,7 +123,7 @@ static void at91_ping(unsigned long data)
   {
   	struct at91wdt *wdt = (struct at91wdt *)data;
   	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
+		(!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) {

... Nitpicking: there seems to be a indentation mismatch here: use tab + spaces to align to previous line...

Quoted 5 lines

Quoted 5 lines

drivers/watchdog/at91sam9_wdt.c +0 -0

   		at91_wdt_reset(wdt);
   		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
   	} else {

@@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 

Quoted 8 lines

drivers/watchdog/at91sam9_wdt.c static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt) · line 325

Quoted 8 lines

drivers/watchdog/at91sam9_wdt.c +3 -0

   	wdt->mr |= max | ((max - min) << 16);
 
+	if (of_property_read_bool(np, "atmel,no-early-timer"))
+		wdt->no_early_timer = 1;
+
   	return 0;
   }
   #else

Otherwise, it seems clean. Can you please re-send a version with the Device tree maintainer in CC?

Best regards,

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
  drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
  2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

index f90e294..a0b7b75 100644
@@ -28,6 +28,10 @@ Optional properties:
  	entering idle state.
  - atmel,dbg-halt : Should be present if you want to stop the watchdog when
  	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.
 
  Example:
  	watchdog at fffffd40 {

drivers/watchdog/at91sam9_wdt.c +5 -1

index 489729b..78b1977 100644
@@ -89,6 +89,7 @@ struct at91wdt {
  	u32 mr_mask;
  	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
  	bool nowayout;
+	bool no_early_timer;
  	unsigned int irq;
  };
 

@@ -122,7 +123,7 @@ static void at91_ping(unsigned long data)
  {
  	struct at91wdt *wdt = (struct at91wdt *)data;
  	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
+		(!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) {
  		at91_wdt_reset(wdt);
  		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
  	} else {

@@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 
  	wdt->mr |= max | ((max - min) << 16);
 
+	if (of_property_read_bool(np, "enable-early-reset"))
+		wdt->no_early_timer = 1;
+
  	return 0;
  }
  #else

Hi,

Quoted 16 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Quoted 1 line

Signed-off-by Timo Kokkonen <[email protected]>

I forgot to put the PATCHv2 on the subject line.. But anyway, any thoughts about it? Is there something that should be done to get it forward?

Quoted 52 lines

---
  Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
  drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
  2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

index f90e294..a0b7b75 100644
@@ -28,6 +28,10 @@ Optional properties:
  	entering idle state.
  - atmel,dbg-halt : Should be present if you want to stop the watchdog when
  	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.
 
  Example:
  	watchdog at fffffd40 {

drivers/watchdog/at91sam9_wdt.c +5 -1

index 489729b..78b1977 100644
@@ -89,6 +89,7 @@ struct at91wdt {
  	u32 mr_mask;
  	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
  	bool nowayout;
+	bool no_early_timer;
  	unsigned int irq;
  };
 

@@ -122,7 +123,7 @@ static void at91_ping(unsigned long data)
  {
  	struct at91wdt *wdt = (struct at91wdt *)data;
  	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
+		(!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) {
  		at91_wdt_reset(wdt);
  		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
  	} else {

@@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 
  	wdt->mr |= max | ((max - min) << 16);
 
+	if (of_property_read_bool(np, "enable-early-reset"))
+		wdt->no_early_timer = 1;
+
  	return 0;
  }
  #else

Quoted 19 lines · Timo Kokkonen

Hi,

Quoted 16 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Quoted 4 lines

Quoted 1 line

Signed-off-by Timo Kokkonen <[email protected]>

I forgot to put the PATCHv2 on the subject line.. But anyway, any thoughts about it? Is there something that should be done to get it forward?

Sorry for not having come back to you quickly.

The only thing that tend to prevent me from taking this patch is the fact that this DT property is mostly a software, Linux-specific one... Which is somehow not covered by the DT.
This might explain as well why this property is not present on other SoCs.

Can we have other people's advices?

Bye,

Quoted 54 lines

Quoted 52 lines

---
   Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
   drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
   2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

index f90e294..a0b7b75 100644
@@ -28,6 +28,10 @@ Optional properties:
   	entering idle state.
   - atmel,dbg-halt : Should be present if you want to stop the watchdog when
   	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.
 
   Example:
   	watchdog at fffffd40 {

drivers/watchdog/at91sam9_wdt.c +5 -1

index 489729b..78b1977 100644
@@ -89,6 +89,7 @@ struct at91wdt {
   	u32 mr_mask;
   	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
   	bool nowayout;
+	bool no_early_timer;
   	unsigned int irq;
   };
 

@@ -122,7 +123,7 @@ static void at91_ping(unsigned long data)
   {
   	struct at91wdt *wdt = (struct at91wdt *)data;
   	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
+		(!watchdog_active(&wdt->wdd) && !wdt->no_early_timer)) {
   		at91_wdt_reset(wdt);
   		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
   	} else {

@@ -316,6 +317,9 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 
   	wdt->mr |= max | ((max - min) << 16);
 
+	if (of_property_read_bool(np, "enable-early-reset"))
+		wdt->no_early_timer = 1;
+
   	return 0;
   }
   #else

Quoted 32 lines · Nicolas Ferre

Quoted 19 lines · Timo Kokkonen

Hi,

Quoted 16 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Quoted 4 lines

Quoted 1 line

Signed-off-by Timo Kokkonen <[email protected]>

I forgot to put the PATCHv2 on the subject line.. But anyway, any thoughts about it? Is there something that should be done to get it forward?

Sorry for not having come back to you quickly.

The only thing that tend to prevent me from taking this patch is the fact that this DT property is mostly a software, Linux-specific one... Which is somehow not covered by the DT.
This might explain as well why this property is not present on other SoCs.

Quoted 2 lines

Can we have other people's advices?

We have been thinking about a more generic (infrastructure based) solution for the problem at hand, but that was a bit more complex and would specify the actual timeout during boot, not just a boolean like suggested here.

As for DT not supposed to be used for configuration, that is really a tricky problem which is hard to solve. I seem to recall, though, that it may be now acceptable under certain conditions. A module parameter might be easier.

Hi Guenter,

Quoted 36 lines · On Thu, 27 Nov 2014 09:23:30 -0800 Guenter Roeck <[email protected]> wrote:

Quoted 34 lines · Nicolas Ferre

Quoted 19 lines · Timo Kokkonen

Hi,

Quoted 16 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Quoted 4 lines

Quoted 1 line

Signed-off-by Timo Kokkonen <[email protected]>

I forgot to put the PATCHv2 on the subject line.. But anyway, any thoughts about it? Is there something that should be done to get it forward?

Sorry for not having come back to you quickly.

The only thing that tend to prevent me from taking this patch is the fact that this DT property is mostly a software, Linux-specific one... Which is somehow not covered by the DT.
This might explain as well why this property is not present on other SoCs.

Can we have other people's advices?

Quoted 3 lines

We have been thinking about a more generic (infrastructure based) solution for the problem at hand, but that was a bit more complex and would specify the actual timeout during boot, not just a boolean like suggested here.

Can't we keep the same timeout (the one specified in the timeout-sec property) ?

Quoted 5 lines

As for DT not supposed to be used for configuration, that is really a tricky problem which is hard to solve. I seem to recall, though, that it may be now acceptable under certain conditions. A module parameter might be easier.

I'm not a big fan of passing these kind information through module params, cause it's kind of hard to assign parameters when you have multiple device instances (it might not be applicable to watchdog devices though).
Moreover, adding more module parameters will just expand the cmdline and make it less and less readable.

Anyway, this is not my call to make :-).

Quoted 5 lines · Boris Brezillon

Quoted 4 lines

As for DT not supposed to be used for configuration, that is really a tricky problem which is hard to solve. I seem to recall, though, that it may be now acceptable under certain conditions. A module parameter might be easier.

Quoted 7 lines

I'm not a big fan of passing these kind information through module params, cause it's kind of hard to assign parameters when you have multiple device instances (it might not be applicable to watchdog devices though).
Moreover, adding more module parameters will just expand the cmdline and make it less and less readable.

Agreed but ...

Quoted 1 line

Anyway, this is not my call to make :-).

it isn't us who restrict the DT scope (though of course timeout-sec _is_ configuration, but that was before things got more restrictive).

Quoted 52 lines · Boris Brezillon

Hi Guenter,

Quoted 46 lines · On Thu, 27 Nov 2014 09:23:30 -0800 Guenter Roeck <[email protected]> wrote:

Quoted 31 lines · Nicolas Ferre

Quoted 22 lines · Timo Kokkonen

Hi,

Quoted 19 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking
the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at
all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen
before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Quoted 5 lines

Quoted 1 line

Signed-off-by Timo Kokkonen <[email protected]>

I forgot to put the PATCHv2 on the subject line.. But anyway, any thoughts about it? Is there something that should be done to get it forward?

Sorry for not having come back to you quickly.

Quoted 8 lines

The only thing that tend to prevent me from taking this patch is the fact that this DT property is mostly a software, Linux-specific one... Which is somehow not covered by the DT.
This might explain as well why this property is not present on other SoCs.

Can we have other people's advices?

We have been thinking about a more generic (infrastructure based) solution
for the problem at hand, but that was a bit more complex and would specify
the actual timeout during boot, not just a boolean like suggested here.

Quoted 3 lines

Can't we keep the same timeout (the one specified in the timeout-sec property) ?

That doesn't take into account systems where - for whatever reason - the initial timeout needs to be longer. I do not think it is a good idea to unnecessarily limit functionality. We may make the additional timeout optional - in that case timeout-sec could be used as default.

Quoted 6 lines

Quoted 5 lines

As for DT not supposed to be used for configuration, that is really a tricky problem which is hard to solve. I seem to recall, though, that it may be now acceptable under certain conditions. A module parameter might be easier.

Quoted 7 lines

I'm not a big fan of passing these kind information through module params, cause it's kind of hard to assign parameters when you have multiple device instances (it might not be applicable to watchdog devices though).
Moreover, adding more module parameters will just expand the cmdline and make it less and less readable.

Agreed but ...

Quoted 1 line

Anyway, this is not my call to make :-).

it isn't us who restrict the DT scope (though of course timeout-sec _is_ configuration, but that was before things got more restrictive).

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
 Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
 drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +0 -0

index f90e294..a0b7b75 100644

Hi Timo,

Sorry for the late reply.

Quoted 27 lines · On Fri, 21 Nov 2014 14:23:30 +0200 Timo Kokkonen <[email protected]>…

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
  drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
  2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +0 -0

index f90e294..a0b7b75 100644

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt Optional properties: · line 34

Quoted 7 lines

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

@@ -28,6 +28,10 @@ Optional properties:
  	entering idle state.
  - atmel,dbg-halt : Should be present if you want to stop the watchdog when
  	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.

If you want to make this property generic, maybe you should document it in a generic binding doc (Documentation/devicetree/bindings/watchdog/watchdog.txt ?). Once you're at it, maybe you could document the generic timeout-sec property in this file.
Moreover, you might want to parse this property in watchdog_core.c and store the information in the watchdog_device struct.

Apart from that, I'm not opposed to the approach, and I'm fine with the current implementation.

Regards,

Boris

Hi Timo,

Sorry for the late reply.

Quoted 27 lines · On Fri, 21 Nov 2014 14:23:30 +0200 Timo Kokkonen <[email protected]>…

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
  drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
  2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +0 -0

index f90e294..a0b7b75 100644

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt Optional properties: · line 34

Quoted 7 lines

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

@@ -28,6 +28,10 @@ Optional properties:
  	entering idle state.
  - atmel,dbg-halt : Should be present if you want to stop the watchdog when
  	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.

If you want to make this property generic, maybe you should document it in a generic binding doc (Documentation/devicetree/bindings/watchdog/watchdog.txt ?). Once you're at it, maybe you could document the generic timeout-sec property in this file.
Moreover, you might want to parse this property in watchdog_core.c and store the information in the watchdog_device struct.

Apart from that, I'm not opposed to the approach, and I'm fine with the current implementation.

Regards,

Boris

Quoted 42 lines · Boris Brezillon

Hi Timo,

Sorry for the late reply.

Quoted 27 lines · On Fri, 21 Nov 2014 14:23:30 +0200 Timo Kokkonen <[email protected]>…

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
  drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
  2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +0 -0

index f90e294..a0b7b75 100644

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt Optional properties: · line 34

Quoted 7 lines

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

@@ -28,6 +28,10 @@ Optional properties:
  	entering idle state.
  - atmel,dbg-halt : Should be present if you want to stop the watchdog when
  	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.

Quoted 7 lines

If you want to make this property generic, maybe you should document it in a generic binding doc (Documentation/devicetree/bindings/watchdog/watchdog.txt ?). Once you're at it, maybe you could document the generic timeout-sec property in this file.
Moreover, you might want to parse this property in watchdog_core.c and store the information in the watchdog_device struct.

I gave a little thought about this today and we could maybe have it like watchdog_init_timeout() is today. But I can't really think of any generic handling for this property, anything else except storing the parsed value to a variable in watchdog_device struct. Everything else is HW specific, except how we read the variable.. If we had some logic or checking for this variable (other than to check it is not negative) maybe then it would make sense.

So I could write a patch to document generic watchdog properties in Documentation/devicetree/bindings/watchdog/watchdog.txt (anything else generic that would go there except timeout-sec and enable-early-reset?)

and change this patch so that it doesn't document anything in atmel-wdt.txt as we already have it documented in generic watchdog.txt.

Sounds good?

Hi Timo,

Quoted 51 lines · On Fri, 05 Dec 2014 14:57:05 +0200 Timo Kokkonen <[email protected]>…

Quoted 42 lines · Boris Brezillon

Hi Timo,

Sorry for the late reply.

Quoted 27 lines · On Fri, 21 Nov 2014 14:23:30 +0200 Timo Kokkonen <[email protected]>…

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
   Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
   drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
   2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +0 -0

index f90e294..a0b7b75 100644

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt Optional properties: · line 34

Quoted 7 lines

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

@@ -28,6 +28,10 @@ Optional properties:
   	entering idle state.
   - atmel,dbg-halt : Should be present if you want to stop the watchdog when
   	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.

Quoted 7 lines

If you want to make this property generic, maybe you should document it in a generic binding doc (Documentation/devicetree/bindings/watchdog/watchdog.txt ?). Once you're at it, maybe you could document the generic timeout-sec property in this file.
Moreover, you might want to parse this property in watchdog_core.c and store the information in the watchdog_device struct.

Quoted 7 lines

I gave a little thought about this today and we could maybe have it like watchdog_init_timeout() is today. But I can't really think of any generic handling for this property, anything else except storing the parsed value to a variable in watchdog_device struct. Everything else is HW specific, except how we read the variable.. If we had some logic or checking for this variable (other than to check it is not negative) maybe then it would make sense.

Okay, I'm fine with keeping this DT property parsing out of the core.

Quoted 4 lines

So I could write a patch to document generic watchdog properties in Documentation/devicetree/bindings/watchdog/watchdog.txt (anything else generic that would go there except timeout-sec and enable-early-reset?)

I'm not sure you and Guenter agreed on the 'early-keepalive-sec' property (and this one won't be used in atmel driver since you can't change the timeout once set), so I'd say the timeout-sec and enable-early-reset are the only common properties for now.

Quoted 4 lines

and change this patch so that it doesn't document anything in atmel-wdt.txt as we already have it documented in generic watchdog.txt.

Sounds good?

Yes, sounds good to me.

Regards,

Boris

Quoted 64 lines · Boris Brezillon

Hi Timo,

Quoted 51 lines · On Fri, 05 Dec 2014 14:57:05 +0200 Timo Kokkonen <[email protected]>…

Quoted 42 lines · Boris Brezillon

Hi Timo,

Sorry for the late reply.

Quoted 27 lines · On Fri, 21 Nov 2014 14:23:30 +0200 Timo Kokkonen <[email protected]>…

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
  drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
  2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +0 -0

index f90e294..a0b7b75 100644

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt Optional properties: · line 34

Quoted 7 lines

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

@@ -28,6 +28,10 @@ Optional properties:
  	entering idle state.
  - atmel,dbg-halt : Should be present if you want to stop the watchdog when
  	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.

Quoted 7 lines

If you want to make this property generic, maybe you should document it in a generic binding doc (Documentation/devicetree/bindings/watchdog/watchdog.txt ?). Once you're at it, maybe you could document the generic timeout-sec property in this file.
Moreover, you might want to parse this property in watchdog_core.c and store the information in the watchdog_device struct.

Quoted 7 lines

I gave a little thought about this today and we could maybe have it like watchdog_init_timeout() is today. But I can't really think of any generic handling for this property, anything else except storing the parsed value to a variable in watchdog_device struct. Everything else is HW specific, except how we read the variable.. If we had some logic or checking for this variable (other than to check it is not negative) maybe then it would make sense.

Quoted 1 line

Okay, I'm fine with keeping this DT property parsing out of the core.

Ok, good.

Quoted 9 lines

Quoted 4 lines

So I could write a patch to document generic watchdog properties in Documentation/devicetree/bindings/watchdog/watchdog.txt (anything else generic that would go there except timeout-sec and enable-early-reset?)

I'm not sure you and Guenter agreed on the 'early-keepalive-sec' property (and this one won't be used in atmel driver since you can't change the timeout once set), so I'd say the timeout-sec and enable-early-reset are the only common properties for now.

Actually it can be done in Atmel driver as well. It already has a timer that keeps on pinging the watchdog until user space opens it. We just modify the code so that it keeps on pinging the watchdog until early-keepalive-sec has expired. No need to change the HW timeouts at all. And I rather have it implemented that way anyway, the 16 second timeout is a little tight if there happens to be a lot of stuff already in the early user space.

Yeah, I didn't get anything back from Guenter about my early-keepalive-sec property proposal. Frankly, I already forgot about it my self already. I guess I'll get back to you with v3 set once I get time for it. Probably easiest to continue from there.

Quoted 87 lines · Timo Kokkonen

Quoted 64 lines · Boris Brezillon

Hi Timo,

Quoted 51 lines · On Fri, 05 Dec 2014 14:57:05 +0200 Timo Kokkonen <[email protected]>…

Quoted 42 lines · Boris Brezillon

Hi Timo,

Sorry for the late reply.

Quoted 27 lines · On Fri, 21 Nov 2014 14:23:30 +0200 Timo Kokkonen <[email protected]>…

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
   Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
   drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
   2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +0 -0

index f90e294..a0b7b75 100644

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt Optional properties: · line 34

Quoted 7 lines

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

@@ -28,6 +28,10 @@ Optional properties:
   	entering idle state.
   - atmel,dbg-halt : Should be present if you want to stop the watchdog when
   	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.

Quoted 7 lines

If you want to make this property generic, maybe you should document it in a generic binding doc (Documentation/devicetree/bindings/watchdog/watchdog.txt ?). Once you're at it, maybe you could document the generic timeout-sec property in this file.
Moreover, you might want to parse this property in watchdog_core.c and store the information in the watchdog_device struct.

Quoted 7 lines

I gave a little thought about this today and we could maybe have it like watchdog_init_timeout() is today. But I can't really think of any generic handling for this property, anything else except storing the parsed value to a variable in watchdog_device struct. Everything else is HW specific, except how we read the variable.. If we had some logic or checking for this variable (other than to check it is not negative) maybe then it would make sense.

Quoted 1 line

Okay, I'm fine with keeping this DT property parsing out of the core.

Ok, good.

Quoted 9 lines

Quoted 4 lines

So I could write a patch to document generic watchdog properties in Documentation/devicetree/bindings/watchdog/watchdog.txt (anything else generic that would go there except timeout-sec and enable-early-reset?)

I'm not sure you and Guenter agreed on the 'early-keepalive-sec' property (and this one won't be used in atmel driver since you can't change the timeout once set), so I'd say the timeout-sec and enable-early-reset are the only common properties for now.

Actually it can be done in Atmel driver as well. It already has a timer that keeps on pinging the watchdog until user space opens it. We just modify the code so that it keeps on pinging the watchdog until early-keepalive-sec has expired. No need to change the HW timeouts at all. And I rather have it implemented that way anyway, the 16 second timeout is a little tight if there happens to be a lot of stuff already in the early user space.

Quoted 5 lines

Yeah, I didn't get anything back from Guenter about my early-keepalive-sec property proposal. Frankly, I already forgot about it my self already. I guess I'll get back to you with v3 set once I get time for it. Probably easiest to continue from there.

Sorry for that, folks. I have been a bit overwhelmed in the last cycle.

early-timeout-sec seems to be the best property name to me right now. It is aligned with the existing timeout-sec. early-keepalive-sec seems kind of different just for the purpose of being different.

Not sure about how to name enable-early-reset. I'd prefer to have something generic, even if only implemented in a single driver for now, but I don't really know right now what that might/should look like. Maybe just "enable-early" to indicate that the watchdog should be enabled during init ?

Quoted 93 lines · Guenter Roeck

Quoted 87 lines · Timo Kokkonen

Quoted 64 lines · Boris Brezillon

Hi Timo,

Quoted 51 lines · On Fri, 05 Dec 2014 14:57:05 +0200 Timo Kokkonen <[email protected]>…

Quoted 42 lines · Boris Brezillon

Hi Timo,

Sorry for the late reply.

Quoted 27 lines · On Fri, 21 Nov 2014 14:23:30 +0200 Timo Kokkonen <[email protected]>…

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "enable-early-reset" which will prevent the kernel timer from pinging the watchdog HW on behalf of user space. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/atmel-wdt.txt | 4 ++++
  drivers/watchdog/at91sam9_wdt.c                          | 6 +++++-
  2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +0 -0

index f90e294..a0b7b75 100644

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt Optional properties: · line 34

Quoted 7 lines

Documentation/devicetree/bindings/watchdog/atmel-wdt.txt +4 -0

@@ -28,6 +28,10 @@ Optional properties:
  	entering idle state.
  - atmel,dbg-halt : Should be present if you want to stop the watchdog when
  	entering debug state.
+- enable-early-reset : Should be present if you want to let the
+	watchdog timer to expire even before user space has opened the
+	device. If not set, a kernel timer will keep on pinging the
+	watchdog until it is opened.

Quoted 7 lines

If you want to make this property generic, maybe you should document it in a generic binding doc (Documentation/devicetree/bindings/watchdog/watchdog.txt ?). Once you're at it, maybe you could document the generic timeout-sec property in this file.
Moreover, you might want to parse this property in watchdog_core.c and store the information in the watchdog_device struct.

Quoted 7 lines

I gave a little thought about this today and we could maybe have it like watchdog_init_timeout() is today. But I can't really think of any generic handling for this property, anything else except storing the parsed value to a variable in watchdog_device struct. Everything else is HW specific, except how we read the variable.. If we had some logic or checking for this variable (other than to check it is not negative) maybe then it would make sense.

Quoted 1 line

Okay, I'm fine with keeping this DT property parsing out of the core.

Ok, good.

Quoted 9 lines

Quoted 4 lines

So I could write a patch to document generic watchdog properties in Documentation/devicetree/bindings/watchdog/watchdog.txt (anything else generic that would go there except timeout-sec and enable-early-reset?)

I'm not sure you and Guenter agreed on the 'early-keepalive-sec' property (and this one won't be used in atmel driver since you can't change the timeout once set), so I'd say the timeout-sec and enable-early-reset are the only common properties for now.

Actually it can be done in Atmel driver as well. It already has a timer that keeps on pinging the watchdog until user space opens it. We just modify the code so that it keeps on pinging the watchdog until early-keepalive-sec has expired. No need to change the HW timeouts at all. And I rather have it implemented that way anyway, the 16 second timeout is a little tight if there happens to be a lot of stuff already in the early user space.

Quoted 5 lines

Yeah, I didn't get anything back from Guenter about my early-keepalive-sec property proposal. Frankly, I already forgot about it my self already. I guess I'll get back to you with v3 set once I get time for it. Probably easiest to continue from there.

Quoted 5 lines

Sorry for that, folks. I have been a bit overwhelmed in the last cycle.

early-timeout-sec seems to be the best property name to me right now. It is aligned with the existing timeout-sec. early-keepalive-sec seems kind of different just for the purpose of being different.

early-timeout-sec sounds good to me. I'll go with that.

Quoted 4 lines

Not sure about how to name enable-early-reset. I'd prefer to have something generic, even if only implemented in a single driver for now, but I don't really know right now what that might/should look like. Maybe just "enable-early" to indicate that the watchdog should be enabled during init ?

Do we need the enable-early or such property at all? Just leave early-timeout-sec to zero and then let it behave just like enable-early would do?

Quoted 11 lines · Timo Kokkonen

Quoted 4 lines · Guenter Roeck

Not sure about how to name enable-early-reset. I'd prefer to have something generic, even if only implemented in a single driver for now, but I don't really know right now what that might/should look like. Maybe just "enable-early" to indicate that the watchdog should be enabled during init ?

Do we need the enable-early or such property at all? Just leave early-timeout-sec to zero and then let it behave just like enable-early would do?

Problem is that the possible conditions are all over the place for "early" watchdog handling.

- Disable watchdog
- Enable watchdog (or keep it enabled), and keep it alive until user space kicks in (ie possibly forever)
- Enable watchdog or keep it enabled, and keep it alive for a specified period of time.
- Keep watchdog enabled if it is already enabled, otherwise keep it disabled.

There are probably more conditions which I don't recall right now. Which of those conditions would you address with "early-timeout = <0>;" ? "enable watchdog early and keep it alive until user space kicks in", or "keep watchdog enabled if already running, and set specified early timeout" ? One could argue either way which one of the two meanings it should be.

Quoted 24 lines · Guenter Roeck

Quoted 11 lines · Timo Kokkonen

Quoted 4 lines · Guenter Roeck

Not sure about how to name enable-early-reset. I'd prefer to have something generic, even if only implemented in a single driver for now, but I don't really know right now what that might/should look like. Maybe just "enable-early" to indicate that the watchdog should be enabled during init ?

Do we need the enable-early or such property at all? Just leave early-timeout-sec to zero and then let it behave just like enable-early would do?

Problem is that the possible conditions are all over the place for "early" watchdog handling.

- Disable watchdog
- Enable watchdog (or keep it enabled), and keep it alive until user space kicks in (ie possibly forever)
- Enable watchdog or keep it enabled, and keep it alive for a specified period of time.
- Keep watchdog enabled if it is already enabled, otherwise keep it disabled.

Quoted 6 lines

There are probably more conditions which I don't recall right now. Which of those conditions would you address with "early-timeout = <0>;" ? "enable watchdog early and keep it alive until user space kicks in", or "keep watchdog enabled if already running, and set specified early timeout" ? One could argue either way which one of the two meanings it should be.

Okay, let me elaborate my point of view a bit.

The use case we are concerned about is that we have a device that we rather not let freeze up at any point. This is what we use the watchdog for. The only missing corner case right now is the point where kernel driver initializes the watchdog hardware and pings it on behalf of user space until a watchdog daemon opens it and starts kicking. This is kind of bad as kernel might lock up or user space might crash before we get to the point where the daemon starts taking care of petting the watchdog. So this is what we are trying to fix.

Right. Some other hardware behave differently to the one in Atmel. They might have watchdog stopped by bootloader or it might not be running at all until someone starts it. What do do with such case? If we are still concerned about the same use case I described above, I would say the reasonable thing to do is to make sure the watchdog is started as early as possible and not stopped at any point at all, if possible. If it needs to be explicitly enabled, bootloader should do it. If it didn't do it, then kernel should do it.

Now that I think of it, what we really are interested in is to defer starting of the watchdog to give user space more time to start up. In Atmel HW it's more tricky as the driver can't be stopped. And in other hardware we could simply disable it altogether until we enable it after specific timeout, but we might crash before the timeout expires, in which case we would not get a chance to enable it. So the right thing to do is to enable the watchdog as early as possible, kick it on behalf of the user space until the timeout expires. Special case would be when the timeout is zero, when we just ensure watchdog is running but we don't do anything to prolong the first expiration.

I can't think of any other use case someone would be interested in, but I'm positive that there are plenty of products on the market right now that have the requirement for race free guarantee that watchdog never stops.

So given the conditions you listed, what I think is really important to fix is "Enable watchdog or keep it enabled, and keep it alive for a specified period of time". The only other choice we have right now is "Disable watchdog and let user space enabled it later, if ever". Yeah, maybe we could cover those other use cases too. Maybe someone is using bootloader to decide what to do with watchdog and kernel should somehow respect that. I don't know if that makes sense or if it would be reasonable assumption..

Any more thoughts?

Quoted 81 lines · Timo Kokkonen

Quoted 27 lines · Guenter Roeck

Quoted 10 lines · Timo Kokkonen

Quoted 7 lines · Guenter Roeck

Not sure about how to name enable-early-reset. I'd prefer to have something
generic, even if only implemented in a single driver for now, but I don't
really know right now what that might/should look like. Maybe just "enable-early" to indicate that the watchdog should be enabled during init ?

Quoted 4 lines

Do we need the enable-early or such property at all? Just leave early-timeout-sec to zero and then let it behave just like enable-early would do?

Problem is that the possible conditions are all over the place for "early" watchdog handling.

- Disable watchdog
- Enable watchdog (or keep it enabled), and keep it alive until user space kicks in (ie possibly forever)
- Enable watchdog or keep it enabled, and keep it alive for a specified period of time.
- Keep watchdog enabled if it is already enabled, otherwise keep it disabled.

Quoted 7 lines

There are probably more conditions which I don't recall right now. Which of those conditions would you address with "early-timeout = <0>;" ?
"enable watchdog early and keep it alive until user space kicks in", or "keep watchdog enabled if already running, and set specified early timeout" ? One could argue either way which one of the two meanings it should be.

Okay, let me elaborate my point of view a bit.

The use case we are concerned about is that we have a device that we rather not let freeze up at any point. This is what we use the watchdog for. The only missing corner case right now is the point where kernel driver initializes the watchdog hardware and pings it on behalf of user space until a watchdog daemon opens it and starts kicking. This is kind of bad as kernel might lock up or user space might crash before we get to the point where the daemon starts taking care of petting the watchdog. So this is what we are trying to fix.

Right. Some other hardware behave differently to the one in Atmel. They might have watchdog stopped by bootloader or it might not be running at all until someone starts it. What do do with such case? If we are still concerned about the same use case I described above, I would say the reasonable thing to do is to make sure the watchdog is started as early as possible and not stopped at any point at all, if possible. If it needs to be explicitly enabled, bootloader should do it. If it didn't do it, then kernel should do it.

Now that I think of it, what we really are interested in is to defer starting of the watchdog to give user space more time to start up. In Atmel HW it's more tricky as the driver can't be stopped. And in other hardware we could simply disable it altogether until we enable it after specific timeout, but we might crash before the timeout expires, in which case we would not get a chance to enable it. So the right thing to do is to enable the watchdog as early as possible, kick it on behalf of the user space until the timeout expires. Special case would be when the timeout is zero, when we just ensure watchdog is running but we don't do anything to prolong the first expiration.

I can't think of any other use case someone would be interested in, but I'm positive that there are plenty of products on the market right now that have the requirement for race free guarantee that watchdog never stops.

So given the conditions you listed, what I think is really important to fix is "Enable watchdog or keep it enabled, and keep it alive for a specified period of time". The only other choice we have right now is "Disable watchdog and let user space enabled it later, if ever". Yeah, maybe we could cover those other use cases too. Maybe someone is using bootloader to decide what to do with watchdog and kernel should somehow respect that. I don't know if that makes sense or if it would be reasonable assumption..

Quoted 2 lines

Any more thoughts?

To make progress, can you update the patch using early-timeout-sec as discussed ?

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
 Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
 drivers/watchdog/at91sam9_wdt.c                         | 9 ++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/watchdog.txt +3 -0

index 7e3686c..32647cf 100644
@@ -4,9 +4,16 @@ using these definitions.
 
 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

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
  drivers/watchdog/at91sam9_wdt.c                         | 9 ++++++++-
  2 files changed, 15 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/watchdog.txt +2 -0

index 7e3686c..32647cf 100644
@@ -4,9 +4,16 @@ using these definitions.
 
  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

Documentation/devicetree/bindings/watchdog/watchdog.txt +0 -0

  };

drivers/watchdog/at91sam9_wdt.c +8 -1

index 6df9405..1b40bfa 100644
@@ -89,6 +89,8 @@ struct at91wdt {
  	u32 mr_mask;
  	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
  	bool nowayout;
+	/* Timeout in jiffies for stopping the early timer */
+	unsigned long early_timer;
  	unsigned int irq;
  };
 

@@ -122,7 +124,8 @@ static void at91_ping(unsigned long data)
  {
  	struct at91wdt *wdt = (struct at91wdt *)data;
  	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
+		(time_before(jiffies, wdt->early_timer) &&
+			!watchdog_active(&wdt->wdd))) {
  		at91_wdt_reset(wdt);
  		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
  	} else {

@@ -316,6 +319,10 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 
  	wdt->mr |= max | ((max - min) << 16);
 
+	if (!of_property_read_u32_index(np, "early-timeout-sec", 0,
+					(u32 *)&wdt->early_timer))
+		wdt->early_timer = wdt->early_timer * HZ + jiffies;
+
  	return 0;
  }
  #else

Quoted 39 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
  Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
  drivers/watchdog/at91sam9_wdt.c                         | 9 ++++++++-
  2 files changed, 15 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/watchdog.txt +6 -0

index 7e3686c..32647cf 100644
@@ -4,9 +4,16 @@ using these definitions.
 
  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:
 

Documentation/devicetree/bindings/watchdog/watchdog.txt using these definitions. · line 18

Quoted 3 lines

Documentation/devicetree/bindings/watchdog/watchdog.txt +1 -0

  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.

Hi Guenter,

Quoted 40 lines · On Wed, 18 Feb 2015 05:59:01 -0800 Guenter Roeck <[email protected]> wrote:

Quoted 39 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
   Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
   drivers/watchdog/at91sam9_wdt.c                         | 9 ++++++++-
   2 files changed, 15 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/watchdog.txt +6 -0

index 7e3686c..32647cf 100644
@@ -4,9 +4,16 @@ using these definitions.
 
   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:
 

Quoted 8 lines

Documentation/devicetree/bindings/watchdog/watchdog.txt using these definitions. · line 18

Quoted 3 lines

Documentation/devicetree/bindings/watchdog/watchdog.txt +1 -0

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

Quoted 26 lines

Quoted 11 lines

Quoted 11 lines

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

Documentation/devicetree/bindings/watchdog/watchdog.txt +5 -0

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

Quoted 8 lines

Documentation/devicetree/bindings/watchdog/watchdog.txt using these definitions. · line 18

Quoted 3 lines

  watchdog {
  	 timeout-sec = <60>;

Documentation/devicetree/bindings/watchdog/watchdog.txt +1 -0

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

Quoted 5 lines

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.

Hi,

Quoted 35 lines · Guenter Roeck

Quoted 26 lines

Quoted 11 lines

Quoted 11 lines

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

Documentation/devicetree/bindings/watchdog/watchdog.txt +5 -0

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

Quoted 8 lines

Documentation/devicetree/bindings/watchdog/watchdog.txt using these definitions. · line 18

Quoted 3 lines

  watchdog {
  	 timeout-sec = <60>;

Documentation/devicetree/bindings/watchdog/watchdog.txt +1 -0

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

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 43 lines · Alexandre Belloni

Hi,

Quoted 39 lines · Guenter Roeck

Quoted 26 lines

Quoted 11 lines

Quoted 11 lines

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

Documentation/devicetree/bindings/watchdog/watchdog.txt +5 -0

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

Quoted 8 lines

Documentation/devicetree/bindings/watchdog/watchdog.txt using these definitions. · line 18

Quoted 3 lines

  watchdog {
  	 timeout-sec = <60>;

Documentation/devicetree/bindings/watchdog/watchdog.txt +1 -0

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

Quoted 5 lines

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.

Quoted 4 lines

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.

Yes, that would be fine ok me.

Quoted 44 lines · On Wed, 18 Feb 2015 09:50:02 -0800 Guenter Roeck <[email protected]> wrote:

Quoted 43 lines · Alexandre Belloni

Hi,

Quoted 39 lines · Guenter Roeck

Quoted 26 lines

Quoted 11 lines

Quoted 11 lines

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

(quoted diff) +5 -0

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

Quoted 8 lines

Review

Quoted 3 lines

   watchdog {
   	 timeout-sec = <60>;

(quoted diff) +1 -0

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

Quoted 5 lines

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.

Quoted 5 lines

Quoted 4 lines

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.

Yes, that would be fine ok me.

Great!
Timo can you change the documentation accordingly ?

Hi,

Quoted 35 lines · Guenter Roeck

Quoted 26 lines

Quoted 11 lines

Quoted 11 lines

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

Documentation/devicetree/bindings/watchdog/watchdog.txt +5 -0

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

Quoted 8 lines

Quoted 3 lines

  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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 38 lines · Alexandre Belloni

Hi,

Quoted 35 lines · Guenter Roeck

Quoted 26 lines

Quoted 11 lines

Quoted 11 lines

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

Documentation/devicetree/bindings/watchdog/watchdog.txt +5 -0

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

Quoted 8 lines

Quoted 3 lines

   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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Hi,

Quoted 61 lines · Rob Herring

Quoted 49 lines · Alexandre Belloni

Hi,

Quoted 35 lines · Guenter Roeck

Quoted 30 lines

Quoted 18 lines

Quoted 6 lines

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

(quoted diff) +1 -0

+  that the driver keeps on ticking the watchdog HW on behalf of user

Review

Quoted 10 lines

(quoted diff) +5 -0

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

Quoted 4 lines

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.

Quoted 5 lines

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

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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

Quoted 73 lines · Timo Kokkonen

Hi,

Quoted 61 lines · Rob Herring

Quoted 49 lines · Alexandre Belloni

Hi,

Quoted 35 lines · Guenter Roeck

Quoted 30 lines

Quoted 18 lines

Quoted 6 lines

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

(quoted diff) +1 -0

+  that the driver keeps on ticking the watchdog HW on behalf of user

Review

Quoted 10 lines

(quoted diff) +5 -0

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

Quoted 4 lines

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.

Quoted 5 lines

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

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Well, the 16 sec maximum may be something useful to put into the DT as that actually is a property of the h/w.

Quoted 9 lines

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.

I fully agree the current design is broken. We should fix that in a generic way.

Quoted 9 lines

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.

Deferring would assume that the watchdog is not already enabled.

Putting in how long the kernel should service the watchdog in DT is like putting soft or hard lockup detection times into DT. These are kernel settings. If you need to change this, you should update your kernel or kernel settings, not the DT.

Quoted 4 lines · Rob Herring

Quoted 2 lines · Timo Kokkonen

Hi,

Quoted 10 lines

Quoted 9 lines

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.

Quoted 7 lines

Deferring would assume that the watchdog is not already enabled.

Putting in how long the kernel should service the watchdog in DT is like putting soft or hard lockup detection times into DT. These are kernel settings. If you need to change this, you should update your kernel or kernel settings, not the DT.

The time to userspace handover may differ from HW variant to HW variant. Some may load faster, some may load slower.

Similar, the runtime watchdog timeout may be different from system to system. On a system with faster CPU, and/or one with faster io, one may want (or need) a faster watchdog timeout. I assumed that was accepted and understood when the timeout-sec property was introduced a long time ago, but maybe not.

Yes, the problem should be resolved in a generic way. This has been on my mind for a long time, including the problem if a watchdog should or should not stay or become enabled during early boot. All those are system properties which should be addressed generically, and there should be a means to express those properties in devicetree.

Problem goes back into the old back-and-forth of what can be in devicetree or not. Sorting that out always takes a long time and a substantial amount of effort. Unfortunately, I don't have that time (writing the code would probably be trivial in comparison).

Quoted 5 lines · On Fri, 20 Feb 2015 08:28:55 -0800 Guenter Roeck <[email protected]> wrote:

Quoted 4 lines · Rob Herring

Quoted 2 lines · Timo Kokkonen

Hi,

[...]

Quoted 32 lines

Quoted 10 lines

Quoted 9 lines

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.

Quoted 7 lines

Deferring would assume that the watchdog is not already enabled.

Putting in how long the kernel should service the watchdog in DT is like putting soft or hard lockup detection times into DT. These are kernel settings. If you need to change this, you should update your kernel or kernel settings, not the DT.

The time to userspace handover may differ from HW variant to HW variant. Some may load faster, some may load slower.

Similar, the runtime watchdog timeout may be different from system to system. On a system with faster CPU, and/or one with faster io, one may want (or need) a faster watchdog timeout. I assumed that was accepted and understood when the timeout-sec property was introduced a long time ago, but maybe not.

Yes, the problem should be resolved in a generic way. This has been on my mind for a long time, including the problem if a watchdog should or should not stay or become enabled during early boot. All those are system properties which should be addressed generically, and there should be a means to express those properties in devicetree.

Quoted 4 lines

Problem goes back into the old back-and-forth of what can be in devicetree or not. Sorting that out always takes a long time and a substantial amount of effort. Unfortunately, I don't have that time (writing the code would probably be trivial in comparison).

I once promised myself to propose this stupid idea, so here it is:
How about creating a new concept called CT (for Config Tree), that would reuse the DT syntax and tree representation and add system config information to the appropriate devices (something like an overlay you'll put on top of your DT).

Joke apart, I've had the same kind of discussion quite often lately. While I understand DT guys concern to control which property should go into the DT which ones should not, some system config are better described next to the device they are attached to rather than in the cmdline.

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
drivers/watchdog/at91sam9_wdt.c | 9 ++++++++-

2 files changed, 15 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/watchdog.txt +0 -0

index 7e3686c..32647cf 100644
@@ -4,9 +4,16 @@ using these definitions.
 

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

Documentation/devicetree/bindings/watchdog/watchdog.txt +5 -0

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

Documentation/devicetree/bindings/watchdog/watchdog.txt +1 -0

+	 early-timeout-sec = <120>;

};

drivers/watchdog/at91sam9_wdt.c +8 -1

index 6df9405..1b40bfa 100644
@@ -89,6 +89,8 @@ struct at91wdt {
 	u32 mr_mask;
 	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
 	bool nowayout;
+	/* Timeout in jiffies for stopping the early timer */
+	unsigned long early_timer;
 	unsigned int irq;
 };
 

@@ -122,7 +124,8 @@ static void at91_ping(unsigned long data)
 {
 	struct at91wdt *wdt = (struct at91wdt *)data;
 	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
+		(time_before(jiffies, wdt->early_timer) &&
+			!watchdog_active(&wdt->wdd))) {
 		at91_wdt_reset(wdt);
 		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
 	} else {

@@ -316,6 +319,10 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 
 	wdt->mr |= max | ((max - min) << 16);
 
+	if (!of_property_read_u32_index(np, "early-timeout-sec", 0,
+					(u32 *)&wdt->early_timer))
+		wdt->early_timer = wdt->early_timer * HZ + jiffies;
+
 	return 0;
 }
 #else

Signature · 7 lines

2.1.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 19 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Quoted 4 lines

Signed-off-by Timo Kokkonen <[email protected]>

---
Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
drivers/watchdog/at91sam9_wdt.c | 9 ++++++++-

This should not be handled by the driver but the kernel in a generic way

Hi Jean-Christophe,

Quoted 19 lines · On Fri, 20 Feb 2015 15:48:22 +0800 Jean-Christophe PLAGNIOL-VILLARD…

Quoted 19 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Timo Kokkonen <[email protected]>

---
Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
drivers/watchdog/at91sam9_wdt.c | 9 ++++++++-

This should not be handled by the driver but the kernel in a generic way

Could you detail a bit more what you have in mind ?

Best Regards,

Boris

Signature · 8 lines

Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 32 lines · Boris Brezillon

Hi Jean-Christophe,

Quoted 24 lines · On Fri, 20 Feb 2015 15:48:22 +0800 Jean-Christophe PLAGNIOL-VILLARD…

Quoted 23 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
drivers/watchdog/at91sam9_wdt.c | 9 ++++++++-

Quoted 3 lines

Quoted 1 line

This should not be handled by the driver but the kernel in a generic way

Could you detail a bit more what you have in mind ?

move this timeout on the linux thread that keep alive the watchdog not in the driver

Hi Jean-Christophe,

Quoted 32 lines · On Sat, 21 Feb 2015 00:33:17 +0800 Jean-Christophe PLAGNIOL-VILLARD…

Quoted 32 lines · Boris Brezillon

Hi Jean-Christophe,

Quoted 24 lines · On Fri, 20 Feb 2015 15:48:22 +0800 Jean-Christophe PLAGNIOL-VILLARD…

Quoted 23 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Signed-off-by Timo Kokkonen <[email protected]>

---
Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
drivers/watchdog/at91sam9_wdt.c | 9 ++++++++-

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

This should not be handled by the driver but the kernel in a generic way

Could you detail a bit more what you have in mind ?

move this timeout on the linux thread that keep alive the watchdog not in the driver

AFAIK there's no such thing (if there is, could you point me to the source file where this thread is defined ?), and each driver are registering their own timer (if they need one).
If you're suggesting to add such common logic to watchdog core, why don't you propose something ?

Timo's need is quite generic, but nobody seemed to bother with that before.

Moreover, using an at91 specific implementation does not prevent migrating to a more generic implementation when it's available. Actually, it's rather difficult to design a generic infrastructure until you have dealt with several devices requiring the same feature, and that's obviously not the case here.

Quoted 8 lines · Boris Brezillon

Hi Jean-Christophe,

On Sat, 21 Feb 2015 00:33:17 +0800
Jean-Christophe PLAGNIOL-VILLARD <[email protected]> wrote:

Timo's need is quite generic, but nobody seemed to bother with that before.

The problem has been discussed before. There are even some patches, but they were too specific and limited in scope for my liking.

As I said in my other reply, to move forward we would need someone who has the time and energy to get an agreement with the DT folks about an acceptable means to express the properties needed for a specific hardware, and to actually implement the necessary code.

Quoted 6 lines

Moreover, using an at91 specific implementation does not prevent migrating to a more generic implementation when it's available. Actually, it's rather difficult to design a generic infrastructure until you have dealt with several devices requiring the same feature, and that's obviously not the case here.

Absolutely agree. If we can not even get a property like the one suggested here accepted, it is completely pointless to even think about a more generic solution that would work for all watchdog drivers.

Hi,

Quoted 24 lines · Guenter Roeck

Quoted 8 lines · Boris Brezillon

Hi Jean-Christophe,

On Sat, 21 Feb 2015 00:33:17 +0800
Jean-Christophe PLAGNIOL-VILLARD <[email protected]> wrote:

Timo's need is quite generic, but nobody seemed to bother with that before.

The problem has been discussed before. There are even some patches, but they were too specific and limited in scope for my liking.

As I said in my other reply, to move forward we would need someone who has the time and energy to get an agreement with the DT folks about an acceptable means to express the properties needed for a specific hardware, and to actually implement the necessary code.

Quoted 6 lines

Moreover, using an at91 specific implementation does not prevent migrating to a more generic implementation when it's available. Actually, it's rather difficult to design a generic infrastructure until you have dealt with several devices requiring the same feature, and that's obviously not the case here.

Quoted 4 lines

Absolutely agree. If we can not even get a property like the one suggested here accepted, it is completely pointless to even think about a more generic solution that would work for all watchdog drivers.

I'm not really sure that I understand what we are really arguing here, but seems that this is not getting any forward unless we get in touch with the DT people who get to decide whether this kind of property belongs to device tree or not. Who are these people anyway? Which list should I write an email to get in touch with them? Why are we not CC'ing them already?

Anyway, the way I tried to express it in the v4 patch set, we have a generic device tree property that does not try to imply any sort of implementation or HW details. The description in watchdog.txt tries to state the purpose of the property clearly so that other driver writers could make other drivers to support it correctly. And then there is at91 specific implementation because that's the only watchdog hardware currently on my desk that I can easily test it with.

I can't think of how I could make this more generic, not without making more changes to the way drivers initialize itself with the watchdog core. And that would require changing a lot of drivers, at least if we intend to make it work so that the watchdog core takes care of this instead of the driver. It's a nice idea but I think it's overly complex given the amount of functionality there needs to be in different drivers and the diversity between drivers.

To me there is nothing wrong with having this done also via a kernel configuration option. We could simply have CONFIG_WATCHDOG_EARLY_TIMEOUT_SEC option that works exactly the same way as the proposed device tree property. To make it clear only some drivers support this option, we could let each driver select an enabler config option CONFIG_WATCHDOG_HAS_DEFERRABLE_EARLY_RESET or such that is used to hide the config option unless we are building a watchdog that supports the given option. Or something like that. But that would be less flexible, as if we want to run the same kernel binary on different arm boards we can't make these values per board any more. The use case I am dealing with doesn't care about this, but I was thinking someone else might care. Which is why I thought it should be run time configurable via device tree instead of a compile time option.

But now that I have mentioned arm boards, I noticed we are talking about generic behaviour and there are also watchdogs running on architectures where device trees are not supported. That said, it might be better to make it compile time configurable now and add other configuration options later.

Any thoughts about that?

Hi Timo,

Quoted 32 lines · On Mon, 23 Feb 2015 09:29:41 +0200 Timo Kokkonen <[email protected]>…

Hi,

Quoted 24 lines · Guenter Roeck

Quoted 8 lines · Boris Brezillon

Hi Jean-Christophe,

On Sat, 21 Feb 2015 00:33:17 +0800
Jean-Christophe PLAGNIOL-VILLARD <[email protected]> wrote:

Timo's need is quite generic, but nobody seemed to bother with that before.

The problem has been discussed before. There are even some patches, but they were too specific and limited in scope for my liking.

As I said in my other reply, to move forward we would need someone who has the time and energy to get an agreement with the DT folks about an acceptable means to express the properties needed for a specific hardware, and to actually implement the necessary code.

Quoted 6 lines

Moreover, using an at91 specific implementation does not prevent migrating to a more generic implementation when it's available. Actually, it's rather difficult to design a generic infrastructure until you have dealt with several devices requiring the same feature, and that's obviously not the case here.

Quoted 4 lines

Absolutely agree. If we can not even get a property like the one suggested here accepted, it is completely pointless to even think about a more generic solution that would work for all watchdog drivers.

Quoted 6 lines

I'm not really sure that I understand what we are really arguing here, but seems that this is not getting any forward unless we get in touch with the DT people who get to decide whether this kind of property belongs to device tree or not. Who are these people anyway? Which list should I write an email to get in touch with them? Why are we not CC'ing them already?

I thought they were in the recipient list, but they are not. If you want to be sure to include all the relevant maintainers and MLs you can use scripts/get_maintainer.pl, this would have given you the DT maintainers and DT ML.

BTW, the discussion has already started (Rob is a DT maintainer) ;-).

Quoted 8 lines

Anyway, the way I tried to express it in the v4 patch set, we have a generic device tree property that does not try to imply any sort of implementation or HW details. The description in watchdog.txt tries to state the purpose of the property clearly so that other driver writers could make other drivers to support it correctly. And then there is at91 specific implementation because that's the only watchdog hardware currently on my desk that I can easily test it with.

The debate here is not whether the property is generic enough or not, but whether it is hardware or software related.
DTs are supposed to describe HW and not the use you make of it (HW configuration), the problem is: sometime you have to describe that to get your hardware to work properly.

Quoted 9 lines

I can't think of how I could make this more generic, not without making more changes to the way drivers initialize itself with the watchdog core. And that would require changing a lot of drivers, at least if we intend to make it work so that the watchdog core takes care of this instead of the driver. It's a nice idea but I think it's overly complex given the amount of functionality there needs to be in different drivers and the diversity between drivers.

Quoted 13 lines

To me there is nothing wrong with having this done also via a kernel configuration option. We could simply have CONFIG_WATCHDOG_EARLY_TIMEOUT_SEC option that works exactly the same way as the proposed device tree property. To make it clear only some drivers support this option, we could let each driver select an enabler config option CONFIG_WATCHDOG_HAS_DEFERRABLE_EARLY_RESET or such that is used to hide the config option unless we are building a watchdog that supports the given option. Or something like that. But that would be less flexible, as if we want to run the same kernel binary on different arm boards we can't make these values per board any more. The use case I am dealing with doesn't care about this, but I was thinking someone else might care. Which is why I thought it should be run time configurable via device tree instead of a compile time option.

IMHO, a viable alternative to the early-timeout-sec property would be a module parameter passed through the kernel cmdline.
But as I said, attaching such parameters to the appropriate watchdog device (say you have several watchdogs in your system) is not as easy as using the DT representation.

Is early-timeout-sec generic enough to be global to the watchdog framework ?

Quoted 6 lines

But now that I have mentioned arm boards, I noticed we are talking about generic behaviour and there are also watchdogs running on architectures where device trees are not supported. That said, it might be better to make it compile time configurable now and add other configuration options later.

As long as the early_timeout_sec is part of the generic watchdog struct, your driver shouldn't care where the value comes from (pdata, DT, ACPI, ...), this is why making DT parsing code part of the core is a better approach IMO.

Quoted 1 line

Quoted 1 line

Any thoughts about that?

As I said, the Kconfig option does not seem the right approach to me, using a module parameter would be more appropriate IMO.

After all, we do have already watchdog_init_timeout function that parses the watchdog timeout argument from either device tree or from command line. How about if we expanded that interface? Maybe have a more generic watchdog_init_parmas() or something that parses the generic watchdog arguments, either from command line, device tree or ACPI or something. The device driver could replace call from watchdog_init_timeout() to watchdog_init_params() once it is ready to support other generic parameters, such as early-timeout-sec. Then the watchdog driver could do the right thing about whether watchdog should be left running or stopped and how long time should be given.

Alternatively, we could also let the watchdog core know a little more about the actual watchdog hardware, such as whether the HW is stoppable, whether it needs manual pinging by the kernel until user space has taken over. Or

maybe we can just extend the timeout values until the user space has first opened it and then shorten the timeout automatically so that it doesn't take that long for the device to reset after a crash. Or some other behaviour that is common to many users. Suggestions are welcome.

Anyway, that is something that needs to be done to make watchdog core take more control over more of the generic watchdog behaviour. It just needs to be done so that we don't need to convert all drivers at once.

On Mon, Feb 23, 2015 at 11:11:38AM +0200, Timo Kokkonen wrote:
[ ... ]

Quoted 12 lines

After all, we do have already watchdog_init_timeout function that parses the watchdog timeout argument from either device tree or from command line. How about if we expanded that interface? Maybe have a more generic watchdog_init_parmas() or something that parses the generic watchdog arguments, either from command line, device tree or ACPI or something. The device driver could replace call from watchdog_init_timeout() to watchdog_init_params() once it is ready to support other generic parameters, such as early-timeout-sec. Then the watchdog driver could do the right thing about whether watchdog should be left running or stopped and how long time should be given.

Good idea.

Quoted 3 lines

Alternatively, we could also let the watchdog core know a little more about the actual watchdog hardware, such as whether the HW is stoppable, whether it needs manual pinging by the kernel until user space has taken over. Or

Yes, all that will be needed. But, still, the stop-gap is that we'll need to get buyin from the DT folks for the necessary properties. I have had the outline for the necessary watchdog core implementation in my mind for a long time, but I just don't have the time (nor the patience, quite frankly) to get DT buyin.

Quoted 9 lines

maybe we can just extend the timeout values until the user space has first opened it and then shorten the timeout automatically so that it doesn't take that long for the device to reset after a crash. Or some other behaviour that is common to many users. Suggestions are welcome.

Anyway, that is something that needs to be done to make watchdog core take more control over more of the generic watchdog behaviour. It just needs to be done so that we don't need to convert all drivers at once.

Correct. It should not be that difficult do do that if designed properly.

Guenter

Quoted 15 lines · Guenter Roeck

On Mon, Feb 23, 2015 at 11:11:38AM +0200, Timo Kokkonen wrote:
[ ... ]

Quoted 12 lines

After all, we do have already watchdog_init_timeout function that parses the watchdog timeout argument from either device tree or from command line. How about if we expanded that interface? Maybe have a more generic watchdog_init_parmas() or something that parses the generic watchdog arguments, either from command line, device tree or ACPI or something. The device driver could replace call from watchdog_init_timeout() to watchdog_init_params() once it is ready to support other generic parameters, such as early-timeout-sec. Then the watchdog driver could do the right thing about whether watchdog should be left running or stopped and how long time should be given.

Quoted 5 lines

Good idea.

Quoted 3 lines

Alternatively, we could also let the watchdog core know a little more about the actual watchdog hardware, such as whether the HW is stoppable, whether it needs manual pinging by the kernel until user space has taken over. Or

Whether the h/w is stoppable or not is certainly a reasonable DT property. The maximum h/w timeout would be too (although that may just be a property of counter size and clock rate).

Quoted 5 lines

Yes, all that will be needed. But, still, the stop-gap is that we'll need to get buyin from the DT folks for the necessary properties. I have had the outline for the necessary watchdog core implementation in my mind for a long time, but I just don't have the time (nor the patience, quite frankly) to get DT buyin.

Defining wdog core functionality and behavior has little to do with DT and you don't need buy in. Presumably you care about non-DT platforms as well. Moving more of the intelligence to the core would be a good thing and for the most part should be independent of DT.

Quoted 19 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Quoted 4 lines

Signed-off-by Timo Kokkonen <[email protected]>

---
Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
drivers/watchdog/at91sam9_wdt.c | 9 ++++++++-

This should not be handled by the driver but the kernel in a generic way

Quoted 65 lines

2 files changed, 15 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/watchdog.txt +0 -0

index 7e3686c..32647cf 100644
@@ -4,9 +4,16 @@ using these definitions.
 

Optional properties:
- timeout-sec: Contains the watchdog timeout in seconds.
+- early-timeout-sec: If present, specifies a timeout value in seconds

Documentation/devicetree/bindings/watchdog/watchdog.txt +5 -0

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

Documentation/devicetree/bindings/watchdog/watchdog.txt +1 -0

+	 early-timeout-sec = <120>;

};

drivers/watchdog/at91sam9_wdt.c +8 -2

index 6df9405..1b40bfa 100644
@@ -89,6 +89,8 @@ struct at91wdt {
 	u32 mr_mask;
 	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
 	bool nowayout;
+	/* Timeout in jiffies for stopping the early timer */
+	unsigned long early_timer;
 	unsigned int irq;
 };
 

@@ -122,7 +124,8 @@ static void at91_ping(unsigned long data)
 {
 	struct at91wdt *wdt = (struct at91wdt *)data;
 	if (time_before(jiffies, wdt->next_heartbeat) ||
-	    !watchdog_active(&wdt->wdd)) {
+		(time_before(jiffies, wdt->early_timer) &&
+			!watchdog_active(&wdt->wdd))) {
 		at91_wdt_reset(wdt);
 		mod_timer(&wdt->timer, jiffies + wdt->heartbeat);
 	} else {

@@ -316,6 +319,10 @@ static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 
 	wdt->mr |= max | ((max - min) << 16);
 
+	if (!of_property_read_u32_index(np, "early-timeout-sec", 0,
+					(u32 *)&wdt->early_timer))
+		wdt->early_timer = wdt->early_timer * HZ + jiffies;
+
 	return 0;
 }
 #else
--

2.1.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 19 lines · Timo Kokkonen

By default the driver will start a kernel timer which keeps on kicking the watchdog HW until user space has opened the watchdog device. Usually this is desirable as the watchdog HW is running by default and the user space may not have any watchdog daemon running at all.

However, on production systems it may be mandatory that also early crashes and lockups will lead to a watchdog reset, even if they happen before the user space has opened the watchdog device.

To resolve the issue, add a new device tree property "early-timeout-sec" which will let the kernel timer to ping the watchdog HW only as long as the specified timeout permits. The default is still to use kernel timer, but more strict behavior can be enabled via the device tree property.

Quoted 4 lines

Signed-off-by Timo Kokkonen <[email protected]>

---
Documentation/devicetree/bindings/watchdog/watchdog.txt | 7 +++++++
drivers/watchdog/at91sam9_wdt.c | 9 ++++++++-

This should not be handled by the driver but the kernel in a generic way

Currently we have no means to adjust the watchdog behavior on early start up before user space has opened the device. The generic behavior among the watchdog drivers is to not start the watchdog at all (or disable it, if possible, or in case at91sam9_wdt, start a kernel timer that keeps on pinging the watchdog on behalf of user space until a watchdog daemon opens it). This unfortunately opens a time window between starting the driver and starting the user space watchdog daemon where a crash of the kernel or user space might prevent the watchdog from ever resetting the device at all. This is obviously bad on production systems.

Introduce a new device tree property that, when set, changes the watchdog driver behavior in such way that there no longer is any window where crash is not caught by the watchdog.

This is the third version of this patch. Previous versions of the patch were using an atmel specific property name. This patch uses a generic name instead as the functionality itself has nothing atmel related. I am also introducing watchdog.txt for describing the generic watchdog device tree bindings as there was no such file before.

Timo Kokkonen (2):
devicetree: Document generic watchdog properties
at91sam9_wdt: Allow watchdog to reset device at early boot

 .../devicetree/bindings/watchdog/watchdog.txt         | 19 +++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.c                       |  9 ++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/watchdog/watchdog.txt

Hi Timo,

Could you send new versions of this in their own thread (not in reply to previous versions), and increment the version in patch 1 and 2 ?

Could you also summarize what has changed between each versions in this cover letter ?

Best Regards,

Boris

Quoted 30 lines · On Wed, 18 Feb 2015 14:57:20 +0200 Timo Kokkonen <[email protected]>…

Currently we have no means to adjust the watchdog behavior on early start up before user space has opened the device. The generic behavior among the watchdog drivers is to not start the watchdog at all (or disable it, if possible, or in case at91sam9_wdt, start a kernel timer that keeps on pinging the watchdog on behalf of user space until a watchdog daemon opens it). This unfortunately opens a time window between starting the driver and starting the user space watchdog daemon where a crash of the kernel or user space might prevent the watchdog from ever resetting the device at all. This is obviously bad on production systems.

Introduce a new device tree property that, when set, changes the watchdog driver behavior in such way that there no longer is any window where crash is not caught by the watchdog.

This is the third version of this patch. Previous versions of the patch were using an atmel specific property name. This patch uses a generic name instead as the functionality itself has nothing atmel related. I am also introducing watchdog.txt for describing the generic watchdog device tree bindings as there was no such file before.

Timo Kokkonen (2):
devicetree: Document generic watchdog properties
at91sam9_wdt: Allow watchdog to reset device at early boot

  .../devicetree/bindings/watchdog/watchdog.txt         | 19 +++++++++++++++++++
  drivers/watchdog/at91sam9_wdt.c                       |  9 ++++++++-
  2 files changed, 27 insertions(+), 1 deletion(-)

create mode 100644 Documentation/devicetree/bindings/watchdog/watchdog.txt