Skip to contentSkip to conversation

watchdog: ath79: fix maximum timeout

PATCH In review John Crispin 2 participants 4 messages May 7, 2018

Subsystems (2)
MIPS Watchdog device drivers
lore
Share & tools
lore

John Crispin

author
#0 lore raw

PATCH 1/1 watchdog: ath79: fix maximum timeout

If the userland tries to set a timeout higher than the max_timeout, then we should fallback to max_timeout.

Signed-off-by John Crispin <[email protected]>
---
 drivers/watchdog/ath79_wdt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/watchdog/ath79_wdt.c +6 -2
index e2209bf5fa8a..c2fc6c3d0092 100644
@@ -115,10 +115,14 @@ static inline void ath79_wdt_disable(void)
 
 static int ath79_wdt_set_timeout(int val)
 {
-	if (val < 1 || val > max_timeout)
+	if (val < 1)
 		return -EINVAL;
 
-	timeout = val;
+	if (val > max_timeout)
+		timeout = max_timeout;
+	else
+		timeout = val;
+
 	ath79_wdt_keepalive();
 
 	return 0;
Signature · 1 line
2.11.0
maintainer
#1 lore raw
Quoted 3 lines · John Crispin

If the userland tries to set a timeout higher than the max_timeout, then we should fallback to max_timeout.

We don't do that for drivers using the watchdog core, so we should not do it here either for consistency.

Guenter

Quoted 26 lines
Signed-off-by John Crispin <[email protected]> > ---
  drivers/watchdog/ath79_wdt.c | 8 ++++++--
  1 file changed, 6 insertions(+), 2 deletions(-)
drivers/watchdog/ath79_wdt.c +6 -2
index e2209bf5fa8a..c2fc6c3d0092 100644
@@ -115,10 +115,14 @@ static inline void ath79_wdt_disable(void)
 
  static int ath79_wdt_set_timeout(int val)
  {
-	if (val < 1 || val > max_timeout)
+	if (val < 1)
  		return -EINVAL;
 
-	timeout = val;
+	if (val > max_timeout)
+		timeout = max_timeout;
+	else
+		timeout = val;
+
  	ath79_wdt_keepalive();
 
  	return 0;
author
#2 lore raw
Quoted 9 lines · Guenter Roeck
Quoted 3 lines · John Crispin

If the userland tries to set a timeout higher than the max_timeout, then we should fallback to max_timeout.

We don't do that for drivers using the watchdog core, so we should not do it here either for consistency.

Guenter

Hi,
thanks for the quick feedback. I'll mark the patch as "rejected by upstream due to subsystem consistency" inside OpenWrt. John

Quoted 26 lines
Quoted 24 lines
Signed-off-by John Crispin <[email protected]> > ---

  drivers/watchdog/ath79_wdt.c | 8 ++++++--

  1 file changed, 6 insertions(+), 2 deletions(-)
drivers/watchdog/ath79_wdt.c +6 -1
index e2209bf5fa8a..c2fc6c3d0092 100644
@@ -115,10 +115,14 @@ static inline void ath79_wdt_disable(void)
     static int ath79_wdt_set_timeout(int val)
   {
-    if (val < 1 || val > max_timeout)
+    if (val < 1)
           return -EINVAL;
   -    timeout = val;
+    if (val > max_timeout)
+        timeout = max_timeout;
+    else
+        timeout = val;
+
       ath79_wdt_keepalive();
         return 0;
maintainer
#3 lore raw
Quoted 8 lines · John Crispin
Quoted 5 lines · Guenter Roeck
Quoted 3 lines · John Crispin

If the userland tries to set a timeout higher than the max_timeout, then we should fallback to max_timeout.

Quoted 8 lines
Quoted 4 lines

We don't do that for drivers using the watchdog core, so we should not do it here either for consistency.

Guenter

Hi,
thanks for the quick feedback. I'll mark the patch as "rejected by upstream due to subsystem consistency" inside OpenWrt.

Note that I am not completely opposed to the idea, but it should go through the watchdog core. You could convert the driver to use it, and then make the case that the subsystem should report <min, max> to userspace, relax set_timeout to adjust the requested timeout to [min,max] instead of letting userspace guess it, or both. Plus, of course, submit patches to actually implement and document the necessary changes.

Thanks,
Guenter