xyz-modem: Fix timeout loop waiting with WATCHDOG

PATCH New Lokesh Vutla 3 participants 7 messages Sep 16, 2017 – Oct 9, 2017

Share & tools

Lokesh Vutla

author

2017-09-16 11:44 UTC #0 lore raw

PATCH 1/1 xyz-modem: Fix timeout loop waiting with WATCHDOG

Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting") fixes the loop delay when using a hw watchdog. But assuming that Watchdog kicking is taken care of by getc(). This is not true in case of DM_SERIAL. So, kick the watchdog before loop waiting, instead of relying on other functions.

Signed-off-by Lokesh Vutla <[email protected]>

---
- This fixes UART boot on AM335x-evm.

 common/xyzModem.c | 2 ++
 1 file changed, 2 insertions(+)

common/xyzModem.c +2 -0

index a0c5dfeece..8c4679473f 100644
@@ -26,6 +26,7 @@
 #include <xyzModem.h>
 #include <stdarg.h>
 #include <crc.h>
+#include <watchdog.h>
 
 /* Assumption - run xyzModem protocol over the console port */

@@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
 {
 
   ulong now = get_timer(0);
+  WATCHDOG_RESET();
   while (!tstc ())
     {
       if (get_timer(now) > xyzModem_CHAR_TIMEOUT)

Signature · 1 line

2.14.1

Tom Rini

2017-09-16 13:43 UTC #1 lore raw

PATCH 1/1 xyz-modem: Fix timeout loop waiting with WATCHDOG

Quoted 26 lines · Lokesh Vutla

Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting") fixes the loop delay when using a hw watchdog. But assuming that Watchdog kicking is taken care of by getc(). This is not true in case of DM_SERIAL. So, kick the watchdog before loop waiting, instead of relying on other functions.
Signed-off-by Lokesh Vutla <[email protected]>
---
- This fixes UART boot on AM335x-evm.
 common/xyzModem.c | 2 ++
 1 file changed, 2 insertions(+)
common/xyzModem.c +1 -0
index a0c5dfeece..8c4679473f 100644
@@ -26,6 +26,7 @@
 #include <xyzModem.h>
 #include <stdarg.h>
 #include <crc.h>
+#include <watchdog.h>
 
 /* Assumption - run xyzModem protocol over the console port */
 

common/xyzModem.c CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c) · line 71

Quoted 7 lines

common/xyzModem.c +1 -0

@@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
 {
 
   ulong now = get_timer(0);
+  WATCHDOG_RESET();
   while (!tstc ())
     {
       if (get_timer(now) > xyzModem_CHAR_TIMEOUT)

My worry is that other places also assume watchdog petted by getc(). Simon, is there a reason DM_SERIAL isn't doing what was done before? Thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170916/476f9285/attachment.sig>

Simon Glass

2017-09-16 16:13 UTC #2 lore raw

PATCH 1/1 xyz-modem: Fix timeout loop waiting with WATCHDOG

Hi,

Quoted 29 lines · Tom Rini

Quoted 28 lines · Lokesh Vutla
Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting") fixes the loop delay when using a hw watchdog. But assuming that Watchdog kicking is taken care of by getc(). This is not true in case of DM_SERIAL. So, kick the watchdog before loop waiting, instead of relying on other functions.
Signed-off-by Lokesh Vutla <[email protected]>
---
- This fixes UART boot on AM335x-evm.
 common/xyzModem.c | 2 ++
 1 file changed, 2 insertions(+)
common/xyzModem.c +1 -0
index a0c5dfeece..8c4679473f 100644
@@ -26,6 +26,7 @@
 #include <xyzModem.h>
 #include <stdarg.h>
 #include <crc.h>
+#include <watchdog.h>
 
 /* Assumption - run xyzModem protocol over the console port */
 
@@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
 {
 

Quoted 9 lines

common/xyzModem.c CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c) · line 71
Quoted 5 lines
common/xyzModem.c +1 -0
   ulong now = get_timer(0);
+  WATCHDOG_RESET();
   while (!tstc ())
     {
       if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
My worry is that other places also assume watchdog petted by getc(). Simon, is there a reason DM_SERIAL isn't doing what was done before? Thanks!

This should really be fixed at source. Is the problems in _serial_tstc()? The watchdog is reset with _serial_getc().

Regards,
Simon

Lokesh Vutla

author

2017-09-17 05:12 UTC #3 lore raw

PATCH 1/1 xyz-modem: Fix timeout loop waiting with WATCHDOG

Simon,

Quoted 42 lines · Simon Glass

Hi,
Quoted 29 lines · Tom Rini
Quoted 28 lines · Lokesh Vutla
Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting") fixes the loop delay when using a hw watchdog. But assuming that Watchdog kicking is taken care of by getc(). This is not true in case of DM_SERIAL. So, kick the watchdog before loop waiting, instead of relying on other functions.
Signed-off-by Lokesh Vutla <[email protected]>
---
- This fixes UART boot on AM335x-evm.
 common/xyzModem.c | 2 ++
 1 file changed, 2 insertions(+)
common/xyzModem.c +1 -0
index a0c5dfeece..8c4679473f 100644
@@ -26,6 +26,7 @@
 #include <xyzModem.h>
 #include <stdarg.h>
 #include <crc.h>
+#include <watchdog.h>
 
 /* Assumption - run xyzModem protocol over the console port */
 
@@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
 {
 
Quoted 9 lines
common/xyzModem.c CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c) · line 71
Quoted 5 lines
common/xyzModem.c +1 -0
   ulong now = get_timer(0);
+  WATCHDOG_RESET();
   while (!tstc ())
     {
       if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
My worry is that other places also assume watchdog petted by getc(). Simon, is there a reason DM_SERIAL isn't doing what was done before? Thanks!

Quoted 2 lines

This should really be fixed at source. Is the problems in _serial_tstc()? The watchdog is reset with _serial_getc().

Sorry, my bad. WATCHDOG_RESET() is being called in __serial_getc() when err is -EAGAIN. But looks like it is never hitting this case. This way ymodem fails as watchdog gets reset when downloading large files.

Thanks and regards,
Lokesh

Quoted 4 lines

Regards,
Simon

Simon Glass

2017-09-25 02:15 UTC #4 lore raw

PATCH 1/1 xyz-modem: Fix timeout loop waiting with WATCHDOG

Hi,

Quoted 45 lines · Lokesh Vutla

Simon,
Quoted 42 lines · Simon Glass
Hi,
Quoted 38 lines · Tom Rini
Quoted 26 lines · Lokesh Vutla
Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting") fixes the loop delay when using a hw watchdog. But assuming that Watchdog kicking is taken care of by getc(). This is not true in case of DM_SERIAL. So, kick the watchdog before loop waiting, instead of relying on other functions.
Signed-off-by Lokesh Vutla <[email protected]>
---
- This fixes UART boot on AM335x-evm.
 common/xyzModem.c | 2 ++
 1 file changed, 2 insertions(+)
common/xyzModem.c +1 -0
index a0c5dfeece..8c4679473f 100644
@@ -26,6 +26,7 @@
 #include <xyzModem.h>
 #include <stdarg.h>
 #include <crc.h>
+#include <watchdog.h>
 
 /* Assumption - run xyzModem protocol over the console port */
 
common/xyzModem.c CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c) · line 71
Quoted 7 lines
common/xyzModem.c +1 -0
@@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
 {
 
   ulong now = get_timer(0);
+  WATCHDOG_RESET();
   while (!tstc ())
     {
       if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
My worry is that other places also assume watchdog petted by getc(). Simon, is there a reason DM_SERIAL isn't doing what was done before? Thanks!

Quoted 6 lines

Quoted 2 lines
This should really be fixed at source. Is the problems in _serial_tstc()? The watchdog is reset with _serial_getc().
Sorry, my bad. WATCHDOG_RESET() is being called in __serial_getc() when err is -EAGAIN. But looks like it is never hitting this case. This way ymodem fails as watchdog gets reset when downloading large files.

So every time we read a character there is one available? How can that happen? Surely the CPU can run faster than the serial port?

Regards,
Simon

Lokesh Vutla

author

2017-09-27 06:27 UTC #5 lore raw

PATCH 1/1 xyz-modem: Fix timeout loop waiting with WATCHDOG

Quoted 51 lines · Simon Glass

Hi,
Quoted 48 lines · Lokesh Vutla
Simon,
Quoted 44 lines · Simon Glass
Hi,
Quoted 29 lines · Tom Rini
Quoted 28 lines · Lokesh Vutla
Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting") fixes the loop delay when using a hw watchdog. But assuming that Watchdog kicking is taken care of by getc(). This is not true in case of DM_SERIAL. So, kick the watchdog before loop waiting, instead of relying on other functions.
Signed-off-by Lokesh Vutla <[email protected]>
---
- This fixes UART boot on AM335x-evm.
 common/xyzModem.c | 2 ++
 1 file changed, 2 insertions(+)
common/xyzModem.c +1 -0
index a0c5dfeece..8c4679473f 100644
@@ -26,6 +26,7 @@
 #include <xyzModem.h>
 #include <stdarg.h>
 #include <crc.h>
+#include <watchdog.h>
 
 /* Assumption - run xyzModem protocol over the console port */
 
@@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
 {
 
Quoted 9 lines
common/xyzModem.c CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c) · line 71
Quoted 5 lines
common/xyzModem.c +1 -0
   ulong now = get_timer(0);
+  WATCHDOG_RESET();
   while (!tstc ())
     {
       if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
My worry is that other places also assume watchdog petted by getc(). Simon, is there a reason DM_SERIAL isn't doing what was done before? Thanks!
This should really be fixed at source. Is the problems in _serial_tstc()? The watchdog is reset with _serial_getc().

Quoted 6 lines

Quoted 3 lines
Sorry, my bad. WATCHDOG_RESET() is being called in __serial_getc() when err is -EAGAIN. But looks like it is never hitting this case. This way ymodem fails as watchdog gets reset when downloading large files.
So every time we read a character there is one available? How can that happen? Surely the CPU can run faster than the serial port?

For a quick experiment, I added a infinite while loop[1] instead of -EAGAIN. I never hit this case when downloading with ymodem. File downloaded successfully with $patch even with the infinite while loop. Am I missing something?

[1] http://pastebin.ubuntu.com/25625299/

Thanks and regards,
Lokesh

Simon Glass

2017-10-09 04:47 UTC #6 lore raw

PATCH 1/1 xyz-modem: Fix timeout loop waiting with WATCHDOG

Hi Lokesh,

Quoted 61 lines · Lokesh Vutla

Quoted 51 lines · Simon Glass
Hi,
Quoted 48 lines · Lokesh Vutla
Simon,
Quoted 44 lines · Simon Glass
Hi,
Quoted 29 lines · Tom Rini
Quoted 28 lines · Lokesh Vutla
Commit 2c77c0d6524eb ("xyz-modem: Change getc timeout loop waiting") fixes the loop delay when using a hw watchdog. But assuming that Watchdog kicking is taken care of by getc(). This is not true in case of DM_SERIAL. So, kick the watchdog before loop waiting, instead of relying on other functions.
Signed-off-by Lokesh Vutla <[email protected]>
---
- This fixes UART boot on AM335x-evm.
 common/xyzModem.c | 2 ++
 1 file changed, 2 insertions(+)
common/xyzModem.c +1 -0
index a0c5dfeece..8c4679473f 100644
@@ -26,6 +26,7 @@
 #include <xyzModem.h>
 #include <stdarg.h>
 #include <crc.h>
+#include <watchdog.h>
 
 /* Assumption - run xyzModem protocol over the console port */
 
@@ -64,6 +65,7 @@ CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c)
 {
 
Quoted 9 lines
common/xyzModem.c CYGACC_COMM_IF_GETC_TIMEOUT (char chan, char *c) · line 71
Quoted 5 lines
common/xyzModem.c +1 -0
   ulong now = get_timer(0);
+  WATCHDOG_RESET();
   while (!tstc ())
     {
       if (get_timer(now) > xyzModem_CHAR_TIMEOUT)
My worry is that other places also assume watchdog petted by getc(). Simon, is there a reason DM_SERIAL isn't doing what was done before? Thanks!
This should really be fixed at source. Is the problems in _serial_tstc()? The watchdog is reset with _serial_getc().
Quoted 6 lines
Quoted 3 lines
Sorry, my bad. WATCHDOG_RESET() is being called in __serial_getc() when err is -EAGAIN. But looks like it is never hitting this case. This way ymodem fails as watchdog gets reset when downloading large files.
So every time we read a character there is one available? How can that happen? Surely the CPU can run faster than the serial port?

Quoted 6 lines

For a quick experiment, I added a infinite while loop[1] instead of -EAGAIN. I never hit this case when downloading with ymodem. File downloaded successfully with $patch even with the infinite while loop. Am I missing something?
[1] http://pastebin.ubuntu.com/25625299/

Well it is very strange. I cannot imagine that a serial line could keep up with the CPU requesting bytes. I think this needs more investigation.

Regards,
Simon