[PATCH 0/7] xyz-modem: Fix cancelling and closing transfers

[PATCH 0/7] xyz-modem: Fix cancelling and closing transfers

[Pali Rohár]

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(-)

-- 
2.20.1

[PATCH 1/7] xyz-modem: Fix crash after cancelling transfer

[Pali Rohár]

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)
-- 
2.20.1

[PATCH 2/7] xyz-modem: Fix x-modem "xyzModem_eof error" at the end of file

[Pali Rohár]

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;
 		}
-- 
2.20.1

[PATCH 3/7] xyz-modem: Put xyzModem_stream_close debug diagnostic message into ZM_DEBUG()

[Pali Rohár]

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 ());
 }
 
-- 
2.20.1

[PATCH 4/7] xyz-modem: Close stream after processing/sending terminate sequence

[Pali Rohár]

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));
-- 
2.20.1

[PATCH 5/7] xyz-modem: Properly abort/terminate transfer on error

[Pali Rohár]

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);
-- 
2.20.1

[PATCH 6/7] xyz-modem: Show information about finished transfer

[Pali Rohár]

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 "
-- 
2.20.1

[PATCH 7/7] xyz-modem: Allow to cancel transfer also by CTRL+C

[Pali Rohár]

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)
-- 
2.20.1

Re: [PATCH 1/7] xyz-modem: Fix crash after cancelling transfer

[Heinrich Schuchardt]

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-union-and-enumeration-documentation

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)
>   	{
>   	  len = xyz.len;
>   	  if (size < len)
> 

Re: [PATCH 4/7] xyz-modem: Close stream after processing/sending terminate sequence

[Heinrich Schuchardt]

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-documentation

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));
> 

Re: [PATCH 6/7] xyz-modem: Show information about finished transfer

[Heinrich Schuchardt]

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;
> +		}
>   	} 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 "
> 

Re: [PATCH 3/7] xyz-modem: Put xyzModem_stream_close debug diagnostic message into ZM_DEBUG()

[Heinrich Schuchardt]

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_DEBUG (zm_flush ());

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

>   }
>
>

Re: [PATCH 6/7] xyz-modem: Show information about finished transfer

[Pali Rohár]

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;
> > +		}
> >   	} 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 "
> > 

Re: [PATCH 3/7] xyz-modem: Put xyzModem_stream_close debug diagnostic message into ZM_DEBUG()

[Pali Rohár]

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_DEBUG (zm_flush ());
> 
> 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
> 
> >   }
> > 
> > 

Re: [PATCH 4/7] xyz-modem: Close stream after processing/sending terminate sequence

[Pali Rohár]

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-documentation
> 
> 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));
> > 

Re: [PATCH 2/7] xyz-modem: Fix x-modem "xyzModem_eof error" at the end of file

[Simon Glass]

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>

Re: [PATCH 5/7] xyz-modem: Properly abort/terminate transfer on error

[Simon Glass]

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

Re: [PATCH 7/7] xyz-modem: Allow to cancel transfer also by CTRL+C

[Simon Glass]

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>

Re: [PATCH 5/7] xyz-modem: Properly abort/terminate transfer on error

[Pali Rohár]

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

Re: [PATCH 0/7] xyz-modem: Fix cancelling and closing transfers

[Pali Rohár]

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
> 

Re: [PATCH 1/7] xyz-modem: Fix crash after cancelling transfer

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]