[PATCH 0/7] xyz-modem: Fix cancelling and closing transfers
This patch series fixes U-Boot crash during cancellation of x/y-modem transfers and contains other various fixes related to finishing transfers.
Pali Rohár (7): xyz-modem: Fix crash after cancelling transfer xyz-modem: Fix x-modem "xyzModem_eof error" at the end of file xyz-modem: Put xyzModem_stream_close debug diagnostic message into ZM_DEBUG() xyz-modem: Close stream after processing/sending terminate sequence xyz-modem: Properly abort/terminate transfer on error xyz-modem: Show information about finished transfer xyz-modem: Allow to cancel transfer also by CTRL+C
cmd/load.c | 32 ++++++++++++++++++++++++++++++-- common/xyzModem.c | 12 ++++++++---- 2 files changed, 38 insertions(+), 6 deletions(-)
Variable xyz.len is set to -1 on error. At the end xyzModem_stream_read() function calls memcpy() with length from variable xyz.len. If this variable is set to -1 then value passed to memcpy is casted to unsigned value, which means to copy whole address space. Which then cause U-Boot crash. E.g. on arm64 it cause CPU crash: "Synchronous Abort" handler, esr 0x96000006
Fix this issue by checking that value stored in xyz.len is valid prior trying to use it.
Signed-off-by: Pali Rohár pali@kernel.org --- common/xyzModem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/xyzModem.c b/common/xyzModem.c index fc3459ebbafe..b1b72aae0baf 100644 --- a/common/xyzModem.c +++ b/common/xyzModem.c @@ -494,7 +494,7 @@ xyzModem_stream_read (char *buf, int size, int *err) total = 0; stat = xyzModem_cancel; /* Try and get 'size' bytes into the buffer */ - while (!xyz.at_eof && (size > 0)) + while (!xyz.at_eof && xyz.len >= 0 && (size > 0)) { if (xyz.len == 0) { @@ -587,7 +587,7 @@ xyzModem_stream_read (char *buf, int size, int *err) } } /* Don't "read" data from the EOF protocol package */ - if (!xyz.at_eof) + if (!xyz.at_eof && xyz.len > 0) { len = xyz.len; if (size < len)
On 03.08.21 16:28, Pali Rohár wrote:
Variable xyz.len is set to -1 on error. At the end xyzModem_stream_read() function calls memcpy() with length from variable xyz.len. If this variable is set to -1 then value passed to memcpy is casted to unsigned value, which means to copy whole address space. Which then cause U-Boot crash. E.g. on arm64 it cause CPU crash: "Synchronous Abort" handler, esr 0x96000006
Fix this issue by checking that value stored in xyz.len is valid prior trying to use it.
Signed-off-by: Pali Rohár pali@kernel.org
The changes below seem reasonable.
I would prefer if you could document the elements of struct xyz first. Just add Sphinx style comments to the structure and indicate for element len that a negative value signals an error.
Cf. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#structure-u...
Acked-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
common/xyzModem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/xyzModem.c b/common/xyzModem.c index fc3459ebbafe..b1b72aae0baf 100644 --- a/common/xyzModem.c +++ b/common/xyzModem.c @@ -494,7 +494,7 @@ xyzModem_stream_read (char *buf, int size, int *err) total = 0; stat = xyzModem_cancel; /* Try and get 'size' bytes into the buffer */
- while (!xyz.at_eof && (size > 0))
- while (!xyz.at_eof && xyz.len >= 0 && (size > 0)) { if (xyz.len == 0) {
@@ -587,7 +587,7 @@ xyzModem_stream_read (char *buf, int size, int *err) } } /* Don't "read" data from the EOF protocol package */
if (!xyz.at_eof)
if (!xyz.at_eof && xyz.len > 0)
On Tue, Aug 03, 2021 at 04:28:38PM +0200, Pali Rohár wrote:
Variable xyz.len is set to -1 on error. At the end xyzModem_stream_read() function calls memcpy() with length from variable xyz.len. If this variable is set to -1 then value passed to memcpy is casted to unsigned value, which means to copy whole address space. Which then cause U-Boot crash. E.g. on arm64 it cause CPU crash: "Synchronous Abort" handler, esr 0x96000006
Fix this issue by checking that value stored in xyz.len is valid prior trying to use it.
Signed-off-by: Pali Rohár pali@kernel.org Acked-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
With a quick X/Y modem test boot on am335x_evm: For the series, applied to u-boot/next, thanks!
In x-modem protocol EOF is not an error state at the end of file.
Signed-off-by: Pali Rohár pali@kernel.org --- common/xyzModem.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/common/xyzModem.c b/common/xyzModem.c index b1b72aae0baf..631c44e11adf 100644 --- a/common/xyzModem.c +++ b/common/xyzModem.c @@ -572,6 +572,8 @@ xyzModem_stream_read (char *buf, int size, int *err) CYGACC_COMM_IF_PUTC (*xyz.__chan, ACK); ZM_DEBUG (zm_dprintf ("FINAL ACK (%d)\n", __LINE__)); } + else + stat = 0; xyz.at_eof = true; break; }
On Tue, 3 Aug 2021 at 08:29, Pali Rohár pali@kernel.org wrote:
In x-modem protocol EOF is not an error state at the end of file.
Signed-off-by: Pali Rohár pali@kernel.org
common/xyzModem.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
This is how all other debug / diagnostic messages are handled.
Signed-off-by: Pali Rohár pali@kernel.org --- common/xyzModem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/xyzModem.c b/common/xyzModem.c index 631c44e11adf..c200c9ff9177 100644 --- a/common/xyzModem.c +++ b/common/xyzModem.c @@ -608,10 +608,10 @@ xyzModem_stream_read (char *buf, int size, int *err) void xyzModem_stream_close (int *err) { - diag_printf + ZM_DEBUG (zm_dprintf ("xyzModem - %s mode, %d(SOH)/%d(STX)/%d(CAN) packets, %d retries\n", xyz.crc_mode ? "CRC" : "Cksum", xyz.total_SOH, xyz.total_STX, - xyz.total_CAN, xyz.total_retries); + xyz.total_CAN, xyz.total_retries)); ZM_DEBUG (zm_flush ()); }
On 03.08.21 16:28, Pali Rohár wrote:
This is how all other debug / diagnostic messages are handled.
This commit message is misleading. What you do is hide an output.
Signed-off-by: Pali Rohár pali@kernel.org
common/xyzModem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/xyzModem.c b/common/xyzModem.c index 631c44e11adf..c200c9ff9177 100644 --- a/common/xyzModem.c +++ b/common/xyzModem.c @@ -608,10 +608,10 @@ xyzModem_stream_read (char *buf, int size, int *err) void xyzModem_stream_close (int *err) {
- diag_printf
- ZM_DEBUG (zm_dprintf ("xyzModem - %s mode, %d(SOH)/%d(STX)/%d(CAN) packets, %d retries\n", xyz.crc_mode ? "CRC" : "Cksum", xyz.total_SOH, xyz.total_STX,
xyz.total_CAN, xyz.total_retries);
xyz.total_CAN, xyz.total_retries));
zm_dprintf prints into the debug buffer which seems not to be overflow protected.
diag_printf prints to the console.
Why do you want to hide this message? Isn't the CRC what the user will want to check? And if many retries occur doesn't this alert the user that the transfer may be corrupted?
Best regards
Heinrich
}
On Wednesday 04 August 2021 11:30:25 Heinrich Schuchardt wrote:
On 03.08.21 16:28, Pali Rohár wrote:
This is how all other debug / diagnostic messages are handled.
This commit message is misleading. What you do is hide an output.
Signed-off-by: Pali Rohár pali@kernel.org
common/xyzModem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/xyzModem.c b/common/xyzModem.c index 631c44e11adf..c200c9ff9177 100644 --- a/common/xyzModem.c +++ b/common/xyzModem.c @@ -608,10 +608,10 @@ xyzModem_stream_read (char *buf, int size, int *err) void xyzModem_stream_close (int *err) {
- diag_printf
- ZM_DEBUG (zm_dprintf ("xyzModem - %s mode, %d(SOH)/%d(STX)/%d(CAN) packets, %d retries\n", xyz.crc_mode ? "CRC" : "Cksum", xyz.total_SOH, xyz.total_STX,
xyz.total_CAN, xyz.total_retries);
xyz.total_CAN, xyz.total_retries));zm_dprintf prints into the debug buffer which seems not to be overflow protected.
diag_printf prints to the console.
Why do you want to hide this message?
I'm not hiding it. I'm just moving debug output to be send via same way as others.
Isn't the CRC what the user will want to check?
It does not print any CRC, so nothing to check. It print information which type of CRC was used during transfer.
And if many retries occur doesn't this alert the user that the transfer may be corrupted?
Hm... corruption itself is handled by protocol. So if it happens then transfer is aborted and via other patches is indicated to user.
Anyway, if this function xyzModem_stream_close() prior calling xyzModem_stream_terminate() then nothing is visible to user because it is send to the x/y-modem sending application, which drop it as garbage according to the protocol.
Such information can be send after restoring terminal from file transfer state back to the user input/output state.
Best regards
Heinrich
}
Obviously it is not possible to send terminate sequence over stream after closing stream.
Signed-off-by: Pali Rohár pali@kernel.org --- cmd/load.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/load.c b/cmd/load.c index b7894d7db02a..fb8c191fb64f 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -1006,8 +1006,8 @@ static ulong load_serial_ymodem(ulong offset, int mode) printf("%s\n", xyzModem_error(err)); }
- xyzModem_stream_close(&err); xyzModem_stream_terminate(false, &getcxmodem); + xyzModem_stream_close(&err);
flush_cache(offset, ALIGN(size, ARCH_DMA_MINALIGN));
On 03.08.21 16:28, Pali Rohár wrote:
Obviously it is not possible to send terminate sequence over stream after closing stream.
xyzModem_stream_close() does not close anything; it flushes the stream. xyzModem_stream_terminate() sets xyz.at_eof = true and gives feedback to the user.
So I think this change is incorrect.
What we lack is documentation of the xyzModem functions in include/xyzModem.h.
Cf. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-do...
Best regards
Heinrich
Signed-off-by: Pali Rohár pali@kernel.org
cmd/load.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/load.c b/cmd/load.c index b7894d7db02a..fb8c191fb64f 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -1006,8 +1006,8 @@ static ulong load_serial_ymodem(ulong offset, int mode) printf("%s\n", xyzModem_error(err)); }
- xyzModem_stream_close(&err); xyzModem_stream_terminate(false, &getcxmodem);
xyzModem_stream_close(&err);
flush_cache(offset, ALIGN(size, ARCH_DMA_MINALIGN));
On Wednesday 04 August 2021 10:59:23 Heinrich Schuchardt wrote:
On 03.08.21 16:28, Pali Rohár wrote:
Obviously it is not possible to send terminate sequence over stream after closing stream.
xyzModem_stream_close() does not close anything; it flushes the stream. xyzModem_stream_terminate() sets xyz.at_eof = true and gives feedback to the user.
So I think this change is incorrect.
I do not think it is incorrect. Anything like user output/debug messages which are flushed prior giving feedback to user (via CANs to restore terminal back to normal state) would not be visible on user terminal, but would be received by x/y-modem application (which interprets it as garbage as does not conform to x/y-modem protocol).
What we lack is documentation of the xyzModem functions in include/xyzModem.h.
I fully agree. Documentation on this topic is missing, code is in not-so-good state and and it would be great to cleanup and document it... But I really do not have a time for writing documentation for this stuff :-(
Cf. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-do...
Best regards
Heinrich
Signed-off-by: Pali Rohár pali@kernel.org
cmd/load.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/load.c b/cmd/load.c index b7894d7db02a..fb8c191fb64f 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -1006,8 +1006,8 @@ static ulong load_serial_ymodem(ulong offset, int mode) printf("%s\n", xyzModem_error(err)); }
- xyzModem_stream_close(&err); xyzModem_stream_terminate(false, &getcxmodem);
- xyzModem_stream_close(&err); flush_cache(offset, ALIGN(size, ARCH_DMA_MINALIGN));
Transfer termination tries to instruct sender that transfer was terminated.
Print error message and indicates aborted transfer in return value.
Signed-off-by: Pali Rohár pali@kernel.org --- cmd/load.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/cmd/load.c b/cmd/load.c index fb8c191fb64f..1ed05a9cd21e 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -975,6 +975,7 @@ static ulong load_serial_ymodem(ulong offset, int mode) res = xyzModem_stream_open(&info, &err); if (!res) {
+ err = 0; while ((res = xyzModem_stream_read(ymodemBuf, 1024, &err)) > 0) { store_addr = addr + offset; @@ -987,6 +988,9 @@ static ulong load_serial_ymodem(ulong offset, int mode) rc = flash_write((char *) ymodemBuf, store_addr, res); if (rc != 0) { + xyzModem_stream_terminate(true, &getcxmodem); + xyzModem_stream_close(&err); + printf("\n"); flash_perror(rc); return (~0); } @@ -998,12 +1002,20 @@ static ulong load_serial_ymodem(ulong offset, int mode) }
} + if (err) { + xyzModem_stream_terminate((err == xyzModem_cancel) ? false : true, &getcxmodem); + xyzModem_stream_close(&err); + printf("\n%s\n", xyzModem_error(err)); + return (~0); /* Download aborted */ + } + if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) efi_set_bootdev("Uart", "", "", map_sysmem(offset, 0), size);
} else { - printf("%s\n", xyzModem_error(err)); + printf("\n%s\n", xyzModem_error(err)); + return (~0); /* Download aborted */ }
xyzModem_stream_terminate(false, &getcxmodem);
Hi Pali,
On Tue, 3 Aug 2021 at 08:29, Pali Rohár pali@kernel.org wrote:
Transfer termination tries to instruct sender that transfer was terminated.
Print error message and indicates aborted transfer in return value.
Signed-off-by: Pali Rohár pali@kernel.org
cmd/load.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/cmd/load.c b/cmd/load.c index fb8c191fb64f..1ed05a9cd21e 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -975,6 +975,7 @@ static ulong load_serial_ymodem(ulong offset, int mode) res = xyzModem_stream_open(&info, &err); if (!res) {
err = 0; while ((res = xyzModem_stream_read(ymodemBuf, 1024, &err)) > 0) { store_addr = addr + offset;@@ -987,6 +988,9 @@ static ulong load_serial_ymodem(ulong offset, int mode) rc = flash_write((char *) ymodemBuf, store_addr, res); if (rc != 0) {
xyzModem_stream_terminate(true, &getcxmodem);xyzModem_stream_close(&err);printf("\n"); flash_perror(rc); return (~0); }@@ -998,12 +1002,20 @@ static ulong load_serial_ymodem(ulong offset, int mode) }
}
if (err) {xyzModem_stream_terminate((err == xyzModem_cancel) ? false : true, &getcxmodem);xyzModem_stream_close(&err);printf("\n%s\n", xyzModem_error(err));return (~0); /* Download aborted */}if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) efi_set_bootdev("Uart", "", "", map_sysmem(offset, 0), size); } else {
printf("%s\n", xyzModem_error(err));
printf("\n%s\n", xyzModem_error(err));return (~0); /* Download aborted */
Should not have brackets around return value.
} xyzModem_stream_terminate(false, &getcxmodem);-- 2.20.1
Regards, Simon
On Thursday 12 August 2021 15:46:23 Simon Glass wrote:
Hi Pali,
On Tue, 3 Aug 2021 at 08:29, Pali Rohár pali@kernel.org wrote:
Transfer termination tries to instruct sender that transfer was terminated.
Print error message and indicates aborted transfer in return value.
Signed-off-by: Pali Rohár pali@kernel.org
cmd/load.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/cmd/load.c b/cmd/load.c index fb8c191fb64f..1ed05a9cd21e 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -975,6 +975,7 @@ static ulong load_serial_ymodem(ulong offset, int mode) res = xyzModem_stream_open(&info, &err); if (!res) {
err = 0; while ((res = xyzModem_stream_read(ymodemBuf, 1024, &err)) > 0) { store_addr = addr + offset;@@ -987,6 +988,9 @@ static ulong load_serial_ymodem(ulong offset, int mode) rc = flash_write((char *) ymodemBuf, store_addr, res); if (rc != 0) {
xyzModem_stream_terminate(true, &getcxmodem);xyzModem_stream_close(&err);printf("\n"); flash_perror(rc); return (~0); }@@ -998,12 +1002,20 @@ static ulong load_serial_ymodem(ulong offset, int mode) }
}
if (err) {xyzModem_stream_terminate((err == xyzModem_cancel) ? false : true, &getcxmodem);xyzModem_stream_close(&err);printf("\n%s\n", xyzModem_error(err));return (~0); /* Download aborted */}if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) efi_set_bootdev("Uart", "", "", map_sysmem(offset, 0), size); } else {
printf("%s\n", xyzModem_error(err));
printf("\n%s\n", xyzModem_error(err));return (~0); /* Download aborted */Should not have brackets around return value.
Hello! I have just copied used style around, to not mix different styles.
} xyzModem_stream_terminate(false, &getcxmodem);-- 2.20.1
Regards, Simon
Show "## Start Addr" or "## Binary (...) download aborted" information like in Kermit "loadb" command.
Signed-off-by: Pali Rohár pali@kernel.org --- cmd/load.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/cmd/load.c b/cmd/load.c index 1ed05a9cd21e..573c44b912fd 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -474,6 +474,14 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
addr = load_serial_ymodem(offset, xyzModem_ymodem);
+ if (addr == ~0) { + image_load_addr = 0; + printf("## Binary (ymodem) download aborted\n"); + rcode = 1; + } else { + printf("## Start Addr = 0x%08lX\n", addr); + image_load_addr = addr; + } } else if (strcmp(argv[0],"loadx")==0) { printf("## Ready for binary (xmodem) download " "to 0x%08lX at %d bps...\n", @@ -482,6 +490,14 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
addr = load_serial_ymodem(offset, xyzModem_xmodem);
+ if (addr == ~0) { + image_load_addr = 0; + printf("## Binary (xmodem) download aborted\n"); + rcode = 1; + } else { + printf("## Start Addr = 0x%08lX\n", addr); + image_load_addr = addr; + } } else {
printf("## Ready for binary (kermit) download "
On 03.08.21 16:28, Pali Rohár wrote:
Show "## Start Addr" or "## Binary (...) download aborted" information like in Kermit "loadb" command.
Signed-off-by: Pali Rohár pali@kernel.org
cmd/load.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/cmd/load.c b/cmd/load.c index 1ed05a9cd21e..573c44b912fd 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -474,6 +474,14 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
addr = load_serial_ymodem(offset, xyzModem_ymodem);
if (addr == ~0) {image_load_addr = 0;printf("## Binary (ymodem) download aborted\n");
xyzModem_stream_terminate() already prints a message "!!!! TRANSFER ABORT !!!!\n" if the transfer is aborted.
rcode = 1;} else {printf("## Start Addr = 0x%08lX\n", addr);
In existing code '## ' is used to indicate errors. This message is not for an error.
Please, avoid alternative language referring to the load address. Cf. net/nfs.c:918: printf("\nLoad address: 0x%lx\nLoading: *\b", image_load_addr); net/tftp.c:851: printf("Load address: 0x%lx\n", tftp_load_addr); net/tftp.c:905: printf("Load address: 0x%lx\n", tftp_load_addr)
Please, use log_err() and log_info() instead of printf().
Best regards
Heinrich
image_load_addr = addr;}@@ -482,6 +490,14 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc,
addr = load_serial_ymodem(offset, xyzModem_xmodem);
if (addr == ~0) {image_load_addr = 0;printf("## Binary (xmodem) download aborted\n");rcode = 1;} else {printf("## Start Addr = 0x%08lX\n", addr);image_load_addr = addr;}} else {
printf("## Ready for binary (kermit) download "
On Wednesday 04 August 2021 11:15:53 Heinrich Schuchardt wrote:
On 03.08.21 16:28, Pali Rohár wrote:
Show "## Start Addr" or "## Binary (...) download aborted" information like in Kermit "loadb" command.
Signed-off-by: Pali Rohár pali@kernel.org
cmd/load.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/cmd/load.c b/cmd/load.c index 1ed05a9cd21e..573c44b912fd 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -474,6 +474,14 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc, addr = load_serial_ymodem(offset, xyzModem_ymodem);
if (addr == ~0) {image_load_addr = 0;printf("## Binary (ymodem) download aborted\n");xyzModem_stream_terminate() already prints a message "!!!! TRANSFER ABORT !!!!\n" if the transfer is aborted.
But prior returning terminal into the normal state. So in most cases receiver does not see this message as it would be eat by ymodem client application.
I have tested it and it is not visible in ckermit tool (which is terminal with support for xmodem, ymodem and kermit protocols).
rcode = 1;} else {printf("## Start Addr = 0x%08lX\n", addr);In existing code '## ' is used to indicate errors. This message is not for an error.
Existing code also already uses it for information about starting transfer and also for information about size of the transfer. This is for all load* commands.
And for loadb and loads it prints also this "Start Addr", so for consistency with older load* transfers commands I added it also into xmodem and ymodem.
And I used exactly same style and format, to have uniform output.
Please, avoid alternative language referring to the load address. Cf. net/nfs.c:918: printf("\nLoad address: 0x%lx\nLoading: *\b", image_load_addr); net/tftp.c:851: printf("Load address: 0x%lx\n", tftp_load_addr); net/tftp.c:905: printf("Load address: 0x%lx\n", tftp_load_addr)
Please, use log_err() and log_info() instead of printf().
Best regards
Heinrich
image_load_addr = addr;}@@ -482,6 +490,14 @@ static int do_load_serial_bin(struct cmd_tbl *cmdtp, int flag, int argc, addr = load_serial_ymodem(offset, xyzModem_xmodem);
if (addr == ~0) {image_load_addr = 0;printf("## Binary (xmodem) download aborted\n");rcode = 1;} else {printf("## Start Addr = 0x%08lX\n", addr);image_load_addr = addr;}
Currently it is possible to cancel loadx and loady commands by pressing CTRL+X (CAN character) at least 3 times quickly.
All other U-Boot commands, including loadb and loads can be cancelled by CTRL+C. So allow it also in xyz-modem code used by loadx and loady commands. Implement it by handling CTRL+C (ETX character) in the same way as CTRL+X (CAN character).
Due to how x/y-modem protocol works, it is required to press CTRL+C or CTRL+X at least 3 times quickly.
Signed-off-by: Pali Rohár pali@kernel.org --- common/xyzModem.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/common/xyzModem.c b/common/xyzModem.c index c200c9ff9177..ece25acb183b 100644 --- a/common/xyzModem.c +++ b/common/xyzModem.c @@ -32,6 +32,7 @@ /* Values magic to the protocol */ #define SOH 0x01 #define STX 0x02 +#define ETX 0x03 /* ^C for interrupt */ #define EOT 0x04 #define ACK 0x06 #define BSP 0x08 @@ -283,6 +284,7 @@ xyzModem_get_hdr (void) hdr_found = true; break; case CAN: + case ETX: xyz.total_CAN++; ZM_DEBUG (zm_dump (__LINE__)); if (++can_total == xyzModem_CAN_COUNT)
On Tue, 3 Aug 2021 at 08:29, Pali Rohár pali@kernel.org wrote:
Currently it is possible to cancel loadx and loady commands by pressing CTRL+X (CAN character) at least 3 times quickly.
All other U-Boot commands, including loadb and loads can be cancelled by CTRL+C. So allow it also in xyz-modem code used by loadx and loady commands. Implement it by handling CTRL+C (ETX character) in the same way as CTRL+X (CAN character).
Due to how x/y-modem protocol works, it is required to press CTRL+C or CTRL+X at least 3 times quickly.
Signed-off-by: Pali Rohár pali@kernel.org
common/xyzModem.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
On Tuesday 03 August 2021 16:28:37 Pali Rohár wrote:
This patch series fixes U-Boot crash during cancellation of x/y-modem transfers and contains other various fixes related to finishing transfers.
Hello! Could you review this patch series? I have already answered all questions / comments...
Pali Rohár (7): xyz-modem: Fix crash after cancelling transfer xyz-modem: Fix x-modem "xyzModem_eof error" at the end of file xyz-modem: Put xyzModem_stream_close debug diagnostic message into ZM_DEBUG() xyz-modem: Close stream after processing/sending terminate sequence xyz-modem: Properly abort/terminate transfer on error xyz-modem: Show information about finished transfer xyz-modem: Allow to cancel transfer also by CTRL+C
cmd/load.c | 32 ++++++++++++++++++++++++++++++-- common/xyzModem.c | 12 ++++++++---- 2 files changed, 38 insertions(+), 6 deletions(-)
-- 2.20.1
participants (5)
-
Heinrich Schuchardt -
Heinrich Schuchardt -
Pali Rohár -
Simon Glass -
Tom Rini