Message ID | 20160726111841.59B321E42D0@proxy.tng.vnc.biz |
---|---|
State | Accepted |
Commit | 644074671e702e8dc0ef642b353845a40f8053da |
Delegated to: | Tom Rini |
Headers | show |
Hi, On 13 July 2016 at 04:56, Andreas J. Reichel <Andreas.Reichel@tngtech.com> wrote: > Hardware: CM-FX6 Module from Compulab > > This patch fixes unwanted watchdog resets while the user enters > a command at the U-Boot prompt. > > As found on the CM-FX6 board from Compulab, when having enabled the > watchdog, a missing WATCHDOG_RESET call in common/console.c causes > this and alike boards to reset when the watchdog's timeout has > elapsed while waiting at the U-Boot prompt. > > Despite the user could press several keys within the watchdog > timeout limit, the while loop in cli_readline.c, line 261, does only > call WATCHDOG_RESET if first == 1, which gets set to 0 in the 1st > loop iteration. This leads to a watchdog timeout no matter if the > user presses keys or not. > > Although, this affects other boards as well as it touches > common/console.c, the macro WATCHDOG_RESET expands to {} if watchdog > support isn't configured. Hence, there's no harm caused and no need to > surround it by #ifdef in this case. > > * Symptom: > U-Boot resets after watchdog times out when in commandline prompt > and watchdog is enabled. > > * Reasoning: > When U-Boot shows the commandline prompt, the following function > call stack is executed while waiting for a keypress: > > common/main.c: > main_loop => common/cli.c: cli_loop() => > common/cli_hush.c: > parse_file_outer => parse_stream_outer => > parse_stream => b_getch(i) => > i->get(i) => file_get => > get_user_input => cmdedit_read_input => > uboot_cli_readline => > common/cli_readline.c: > cli_readline => cli_readline_into_buffer => > cread_line => getcmd_getch (== getc) => > common/console.c: > fgetc => console_tstc > > common/console.c: > (with CONFIG_CONSOLE_MUX is set) > > - in console_tstc line 181: > If dev->tstc(dev) returns 0, the global tstcdev variable doesn't get > set. This is the case if no character is in the serial buffer. > > - in fgetc(int file), line 297: > Program flow keeps looping because tstcdev does not get set. > Therefore WATCHDOG_RESET is not called, as mx_serial_tstc from > drivers/serial/serial_mxc.c does not call it. > > * Solution: > Add WATCHDOG_RESET into the loop of console_tstc. > > Note: Macro expands to {} if not configured, so no #ifdef is needed. > > * Comment: > > Signed-off-by: Christian Storm <christian.storm@tngtech.com> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > Signed-off-by: Andreas J. Reichel <Andreas.Reichel@tngtech.com> Thanks for the detailed info. Would it be better to put this in _serial_tstc()? > --- > > common/console.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/common/console.c b/common/console.c > index 12293f3..054b6cd 100644 > --- a/common/console.c > +++ b/common/console.c > @@ -16,6 +16,7 @@ > #include <stdio_dev.h> > #include <exports.h> > #include <environment.h> > +#include <watchdog.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -294,6 +295,7 @@ int fgetc(int file) > * Effectively poll for input wherever it may be available. > */ > for (;;) { > + WATCHDOG_RESET(); > /* > * Upper layer may have already called tstc() so > * check for that first. > -- > 2.8.2 > Regards, Simon
+Tom, in case this should go into the release. On 1 August 2016 at 05:49, Andreas J. Reichel <Andreas.Reichel@tngtech.com> wrote: > This patch fixes unwanted watchdog resets while the user enters > a command at the U-Boot prompt. > > As found on the CM-FX6 board from Compulab, when having enabled the > watchdog, a missing WATCHDOG_RESET call in _serial_tstc() in > (/drivers/serial/serial-uclass.c) causes this and alike boards to > reset when the watchdog's timeout has elapsed while waiting at the > U-Boot prompt. > > Despite the user could press several keys within the watchdog > timeout limit, the while loop in cli_readline.c, line 261, does only > call WATCHDOG_RESET if first == 1, which gets set to 0 in the 1st > loop iteration. This leads to a watchdog timeout no matter if the > user presses keys or not. > > The problem is solved with a call to WATCHDOG_RESET in _serial_tstc, > defined in drivers/serial/serial-uclass.c. > > Since the macro WATCHDOG_RESET expands to {} if watchdog support > isn't configured, there's no need to surround it by #ifdef in this > case. > > * Symptom: > U-Boot resets after watchdog times out when in commandline prompt > and watchdog is enabled. > > * Reasoning: > When U-Boot shows the commandline prompt, the following function > call stack is executed while waiting for a keypress: > > common/main.c: > main_loop => common/cli.c: cli_loop() => > common/cli_hush.c: > parse_file_outer => parse_stream_outer => > parse_stream => b_getch(i) => > i->get(i) => file_get => > get_user_input => cmdedit_read_input => > uboot_cli_readline => > common/cli_readline.c: > cli_readline => cli_readline_into_buffer => > cread_line => getcmd_getch (== getc) => > commonn/console.c: > fgetc => console_tstc > > - in console_tstc line 181: > If dev->tstc(dev) returns 0, the global tstcdev variable doesn't > get set. This is the case if no character is in the serial buffer. > > - in fgetc(int file), line 297: > Program flow keeps looping because tstcdev does not get set. > Therefore WATCHDOG_RESET is not called, as mx_serial_tstc from > drivers/serial/serial_mxc.c does not call it. > > - Initialization calls drv_system_init in stdio.c, which sets > dev.tstc = stdio_serial_tstc. Thus, dev->tstc(dev) calls serial_tstc() > which in turn calls _serial_tstc(). > > Hence, _serial_tstc() needs to call WATCHDOG_RESET() to periodically > reset the watchdog while cli_readline waits for user input. > > Signed-off-by: Christian Storm <christian.storm@tngtech.com> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > Signed-off-by: Andreas J. Reichel <Andreas.Reichel@tngtech.com> > --- > > Changes in v2: > - Move WATCHDOG_RESET() call from common/console.c to > drivers/serial/serial-uclass.c. > > drivers/serial/serial-uclass.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c > index 0ce5c44..72cf808 100644 > --- a/drivers/serial/serial-uclass.c > +++ b/drivers/serial/serial-uclass.c > @@ -155,6 +155,7 @@ static int _serial_tstc(struct udevice *dev) > { > struct dm_serial_ops *ops = serial_get_ops(dev); > > + WATCHDOG_RESET(); > if (ops->pending) > return ops->pending(dev, true); Great explanation, thank you. Acked-by: Simon Glass <sjg@chromium.org> > > -- > 2.8.2 >
Hi Tom, is my patch going to be applied or is the problem resolved otherwhise? Kind regards Andreas On Mo, Sep 05, 2016 at 07:04:53 -0600, Simon Glass wrote: > +Tom, in case this should go into the release. > > On 1 August 2016 at 05:49, Andreas J. Reichel > <Andreas.Reichel@tngtech.com> wrote: > > This patch fixes unwanted watchdog resets while the user enters > > a command at the U-Boot prompt. > > > > As found on the CM-FX6 board from Compulab, when having enabled the > > watchdog, a missing WATCHDOG_RESET call in _serial_tstc() in > > (/drivers/serial/serial-uclass.c) causes this and alike boards to > > reset when the watchdog's timeout has elapsed while waiting at the > > U-Boot prompt. > > > > Despite the user could press several keys within the watchdog > > timeout limit, the while loop in cli_readline.c, line 261, does only > > call WATCHDOG_RESET if first == 1, which gets set to 0 in the 1st > > loop iteration. This leads to a watchdog timeout no matter if the > > user presses keys or not. > > > > The problem is solved with a call to WATCHDOG_RESET in _serial_tstc, > > defined in drivers/serial/serial-uclass.c. > > > > Since the macro WATCHDOG_RESET expands to {} if watchdog support > > isn't configured, there's no need to surround it by #ifdef in this > > case. > > > > * Symptom: > > U-Boot resets after watchdog times out when in commandline prompt > > and watchdog is enabled. > > > > * Reasoning: > > When U-Boot shows the commandline prompt, the following function > > call stack is executed while waiting for a keypress: > > > > common/main.c: > > main_loop => common/cli.c: cli_loop() => > > common/cli_hush.c: > > parse_file_outer => parse_stream_outer => > > parse_stream => b_getch(i) => > > i->get(i) => file_get => > > get_user_input => cmdedit_read_input => > > uboot_cli_readline => > > common/cli_readline.c: > > cli_readline => cli_readline_into_buffer => > > cread_line => getcmd_getch (== getc) => > > commonn/console.c: > > fgetc => console_tstc > > > > - in console_tstc line 181: > > If dev->tstc(dev) returns 0, the global tstcdev variable doesn't > > get set. This is the case if no character is in the serial buffer. > > > > - in fgetc(int file), line 297: > > Program flow keeps looping because tstcdev does not get set. > > Therefore WATCHDOG_RESET is not called, as mx_serial_tstc from > > drivers/serial/serial_mxc.c does not call it. > > > > - Initialization calls drv_system_init in stdio.c, which sets > > dev.tstc = stdio_serial_tstc. Thus, dev->tstc(dev) calls serial_tstc() > > which in turn calls _serial_tstc(). > > > > Hence, _serial_tstc() needs to call WATCHDOG_RESET() to periodically > > reset the watchdog while cli_readline waits for user input. > > > > Signed-off-by: Christian Storm <christian.storm@tngtech.com> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > Signed-off-by: Andreas J. Reichel <Andreas.Reichel@tngtech.com> > > --- > > > > Changes in v2: > > - Move WATCHDOG_RESET() call from common/console.c to > > drivers/serial/serial-uclass.c. > > > > drivers/serial/serial-uclass.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c > > index 0ce5c44..72cf808 100644 > > --- a/drivers/serial/serial-uclass.c > > +++ b/drivers/serial/serial-uclass.c > > @@ -155,6 +155,7 @@ static int _serial_tstc(struct udevice *dev) > > { > > struct dm_serial_ops *ops = serial_get_ops(dev); > > > > + WATCHDOG_RESET(); > > if (ops->pending) > > return ops->pending(dev, true); > > Great explanation, thank you. > > Acked-by: Simon Glass <sjg@chromium.org> > > > > > -- > > 2.8.2 > >
On Mon, Sep 19, 2016 at 01:59:44PM +0200, Andreas Reichel wrote: > Hi Tom, > > is my patch going to be applied or is the problem resolved > otherwhise? Sorry, lost in my patchwork queue. I'll pick this up soon now. > > Kind regards > Andreas > > On Mo, Sep 05, 2016 at 07:04:53 -0600, Simon Glass wrote: > > +Tom, in case this should go into the release. > > > > On 1 August 2016 at 05:49, Andreas J. Reichel > > <Andreas.Reichel@tngtech.com> wrote: > > > This patch fixes unwanted watchdog resets while the user enters > > > a command at the U-Boot prompt. > > > > > > As found on the CM-FX6 board from Compulab, when having enabled the > > > watchdog, a missing WATCHDOG_RESET call in _serial_tstc() in > > > (/drivers/serial/serial-uclass.c) causes this and alike boards to > > > reset when the watchdog's timeout has elapsed while waiting at the > > > U-Boot prompt. > > > > > > Despite the user could press several keys within the watchdog > > > timeout limit, the while loop in cli_readline.c, line 261, does only > > > call WATCHDOG_RESET if first == 1, which gets set to 0 in the 1st > > > loop iteration. This leads to a watchdog timeout no matter if the > > > user presses keys or not. > > > > > > The problem is solved with a call to WATCHDOG_RESET in _serial_tstc, > > > defined in drivers/serial/serial-uclass.c. > > > > > > Since the macro WATCHDOG_RESET expands to {} if watchdog support > > > isn't configured, there's no need to surround it by #ifdef in this > > > case. > > > > > > * Symptom: > > > U-Boot resets after watchdog times out when in commandline prompt > > > and watchdog is enabled. > > > > > > * Reasoning: > > > When U-Boot shows the commandline prompt, the following function > > > call stack is executed while waiting for a keypress: > > > > > > common/main.c: > > > main_loop => common/cli.c: cli_loop() => > > > common/cli_hush.c: > > > parse_file_outer => parse_stream_outer => > > > parse_stream => b_getch(i) => > > > i->get(i) => file_get => > > > get_user_input => cmdedit_read_input => > > > uboot_cli_readline => > > > common/cli_readline.c: > > > cli_readline => cli_readline_into_buffer => > > > cread_line => getcmd_getch (== getc) => > > > commonn/console.c: > > > fgetc => console_tstc > > > > > > - in console_tstc line 181: > > > If dev->tstc(dev) returns 0, the global tstcdev variable doesn't > > > get set. This is the case if no character is in the serial buffer. > > > > > > - in fgetc(int file), line 297: > > > Program flow keeps looping because tstcdev does not get set. > > > Therefore WATCHDOG_RESET is not called, as mx_serial_tstc from > > > drivers/serial/serial_mxc.c does not call it. > > > > > > - Initialization calls drv_system_init in stdio.c, which sets > > > dev.tstc = stdio_serial_tstc. Thus, dev->tstc(dev) calls serial_tstc() > > > which in turn calls _serial_tstc(). > > > > > > Hence, _serial_tstc() needs to call WATCHDOG_RESET() to periodically > > > reset the watchdog while cli_readline waits for user input. > > > > > > Signed-off-by: Christian Storm <christian.storm@tngtech.com> > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > > Signed-off-by: Andreas J. Reichel <Andreas.Reichel@tngtech.com> > > > --- > > > > > > Changes in v2: > > > - Move WATCHDOG_RESET() call from common/console.c to > > > drivers/serial/serial-uclass.c. > > > > > > drivers/serial/serial-uclass.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c > > > index 0ce5c44..72cf808 100644 > > > --- a/drivers/serial/serial-uclass.c > > > +++ b/drivers/serial/serial-uclass.c > > > @@ -155,6 +155,7 @@ static int _serial_tstc(struct udevice *dev) > > > { > > > struct dm_serial_ops *ops = serial_get_ops(dev); > > > > > > + WATCHDOG_RESET(); > > > if (ops->pending) > > > return ops->pending(dev, true); > > > > Great explanation, thank you. > > > > Acked-by: Simon Glass <sjg@chromium.org> > > > > > > > > -- > > > 2.8.2 > > > > > -- > Andreas Reichel > Dipl.-Phys. (Univ.) > Software Consultant > > Andreas.Reichel@tngtech.com > +49-174-3180074 > > TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterföhring > Geschäftsführer: Henrik Klagges, Christoph Stock, Dr. Robert Dahlke > Sitz: Unterföhring * Amtsgericht München * HRB 135082
On Wed, Jul 13, 2016 at 12:56:51PM +0200, Andreas J. Reichel wrote: > Hardware: CM-FX6 Module from Compulab > > This patch fixes unwanted watchdog resets while the user enters > a command at the U-Boot prompt. > > As found on the CM-FX6 board from Compulab, when having enabled the > watchdog, a missing WATCHDOG_RESET call in common/console.c causes > this and alike boards to reset when the watchdog's timeout has > elapsed while waiting at the U-Boot prompt. > > Despite the user could press several keys within the watchdog > timeout limit, the while loop in cli_readline.c, line 261, does only > call WATCHDOG_RESET if first == 1, which gets set to 0 in the 1st > loop iteration. This leads to a watchdog timeout no matter if the > user presses keys or not. > > Although, this affects other boards as well as it touches > common/console.c, the macro WATCHDOG_RESET expands to {} if watchdog > support isn't configured. Hence, there's no harm caused and no need to > surround it by #ifdef in this case. > > * Symptom: > U-Boot resets after watchdog times out when in commandline prompt > and watchdog is enabled. > > * Reasoning: > When U-Boot shows the commandline prompt, the following function > call stack is executed while waiting for a keypress: > > common/main.c: > main_loop => common/cli.c: cli_loop() => > common/cli_hush.c: > parse_file_outer => parse_stream_outer => > parse_stream => b_getch(i) => > i->get(i) => file_get => > get_user_input => cmdedit_read_input => > uboot_cli_readline => > common/cli_readline.c: > cli_readline => cli_readline_into_buffer => > cread_line => getcmd_getch (== getc) => > common/console.c: > fgetc => console_tstc > > common/console.c: > (with CONFIG_CONSOLE_MUX is set) > > - in console_tstc line 181: > If dev->tstc(dev) returns 0, the global tstcdev variable doesn't get > set. This is the case if no character is in the serial buffer. > > - in fgetc(int file), line 297: > Program flow keeps looping because tstcdev does not get set. > Therefore WATCHDOG_RESET is not called, as mx_serial_tstc from > drivers/serial/serial_mxc.c does not call it. > > * Solution: > Add WATCHDOG_RESET into the loop of console_tstc. > > Note: Macro expands to {} if not configured, so no #ifdef is needed. > > * Comment: > > Signed-off-by: Christian Storm <christian.storm@tngtech.com> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > Signed-off-by: Andreas J. Reichel <Andreas.Reichel@tngtech.com> > Acked-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
diff --git a/common/console.c b/common/console.c index 12293f3..054b6cd 100644 --- a/common/console.c +++ b/common/console.c @@ -16,6 +16,7 @@ #include <stdio_dev.h> #include <exports.h> #include <environment.h> +#include <watchdog.h> DECLARE_GLOBAL_DATA_PTR; @@ -294,6 +295,7 @@ int fgetc(int file) * Effectively poll for input wherever it may be available. */ for (;;) { + WATCHDOG_RESET(); /* * Upper layer may have already called tstc() so * check for that first.