kwboot : bug when board stops acking
Hi list,
I think there's a bug in kwboot. I have a faulty board that stops acking xmodem frames after a certain number of packets and kwboot instead of exiting continues to send frames up to 100%. With the one second retry timer it's very long! More than 2 hours :-(
The bug is in the kwboot_xm_sendblock() fonction : https://gitlab.denx.de/u-boot/u-boot/blob/master/tools/kwboot.c#L375 kwboot_tty_recv detects the timeout but the return value is based on the last character received (line 403) The variable c is not initialized so if it keeps the previous value which is ACK => rc=0 => infinite loop
There are several ways to fix the bug: * Surround the "switch(c)" with a test: if ( !((rc == -1) && (errno == ETIMEDOUT)) ) { rc = -1; switch (c) { case ACK: rc = 0; break; case NAK: errno = EBADMSG; break; case CAN: errno = ECANCELED; break; default: errno = EPROTO; break; } } return rc; * In the case of a TIMEOUT set c to a specific value and add the case in the switch
Do you agree with my diagnosis? What is the best solution to fix this bug? Should I provide a patch?
Thank you, Stephane
Hello Stephane!
On Wednesday 08 January 2020 15:21:12 SC wrote:
Hi list,
I think there's a bug in kwboot. I have a faulty board that stops acking xmodem frames after a certain number of packets and kwboot instead of exiting continues to send frames up to 100%. With the one second retry timer it's very long! More than 2 hours :-(
I saw very similar bugs in kwboot and together with Marek we fixed lot of issues in kwboot.
Could you test if last version of kwboot from U-Boot git master branch has still same or similar issue?
The bug is in the kwboot_xm_sendblock() fonction : https://gitlab.denx.de/u-boot/u-boot/blob/master/tools/kwboot.c#L375 kwboot_tty_recv detects the timeout but the return value is based on the last character received (line 403) The variable c is not initialized so if it keeps the previous value which is ACK => rc=0 => infinite loop
There are several ways to fix the bug:
- Surround the "switch(c)" with a test:
if ( !((rc == -1) && (errno == ETIMEDOUT)) ) { rc = -1; switch (c) { case ACK: rc = 0; break; case NAK: errno = EBADMSG; break; case CAN: errno = ECANCELED; break; default: errno = EPROTO; break; } } return rc;
- In the case of a TIMEOUT set c to a specific value and add the case in the switch
Do you agree with my diagnosis? What is the best solution to fix this bug? Should I provide a patch?
Thank you, Stephane
Hello,
On Fri, Jan 6, 2023 at 9:52 AM Pali Rohár pali@kernel.org wrote:
Hello Stephane!
On Wednesday 08 January 2020 15:21:12 SC wrote:
Hi list,
I think there's a bug in kwboot. I have a faulty board that stops acking xmodem frames after a certain number of packets and kwboot instead of exiting continues to send frames up to 100%. With the one second retry timer it's very long! More than 2 hours :-(
I saw very similar bugs in kwboot and together with Marek we fixed lot of issues in kwboot.
Could you test if last version of kwboot from U-Boot git master branch has still same or similar issue?
The bug is in the kwboot_xm_sendblock() fonction : https://gitlab.denx.de/u-boot/u-boot/blob/master/tools/kwboot.c#L375 kwboot_tty_recv detects the timeout but the return value is based on the last character received (line 403) The variable c is not initialized so if it keeps the previous value which is ACK => rc=0 => infinite loop
There are several ways to fix the bug:
- Surround the "switch(c)" with a test: if ( !((rc == -1) && (errno == ETIMEDOUT)) ) { rc = -1; switch (c) { case ACK: rc = 0; break; case NAK: errno = EBADMSG; break; case CAN: errno = ECANCELED; break; default: errno = EPROTO; break; } } return rc;
- In the case of a TIMEOUT set c to a specific value and add the case in the switch
I've been running the latest kwboot (2023.01-rc4), and I did not see this problem. And the current code looks OK to me. c is modified before it is used later: https://github.com/u-boot/u-boot/blob/master/tools/kwboot.c#L1238
And the function kwboot_xm_sendblock() in GitHub is line L1215! https://github.com/u-boot/u-boot/blob/master/tools/kwboot.c#L1215
Probably Stephane is running an older version of kwboot?
Best, Tony
Do you agree with my diagnosis? What is the best solution to fix this bug? Should I provide a patch?
Thank you, Stephane
participants (3)
-
Pali Rohár -
SC -
Tony Dinh