[PATCH] loadb: Properly indicate aborted kermit transfer

[PATCH] loadb: Properly indicate aborted kermit transfer

[Pali Rohár]

When k_recv() returns zero it indicates that kermit transfer was aborted.
Function do_load_serial_bin() (caller of load_serial_bin()) interprets
value ~0 as aborted transfer, so properly propagates information about
aborted transfer from k_recv() to do_load_serial_bin().

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 cmd/load.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/cmd/load.c b/cmd/load.c
index 462340ad2cde..249ebd4ae078 100644
--- a/cmd/load.c
+++ b/cmd/load.c
@@ -551,6 +551,9 @@ static ulong load_serial_bin(ulong offset)
 		udelay(1000);
 	}
 
+	if (size == 0)
+		return ~0; /* Download aborted */
+
 	flush_cache(offset, size);
 
 	printf("## Total Size      = 0x%08x = %d Bytes\n", size, size);
-- 
2.20.1

Re: [PATCH] loadb: Properly indicate aborted kermit transfer

[Heinrich Schuchardt]

On 8/6/21 6:07 PM, Pali Rohár wrote:
> When k_recv() returns zero it indicates that kermit transfer was aborted.
> Function do_load_serial_bin() (caller of load_serial_bin()) interprets
> value ~0 as aborted transfer, so properly propagates information about
> aborted transfer from k_recv() to do_load_serial_bin().
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>   cmd/load.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/cmd/load.c b/cmd/load.c
> index 462340ad2cde..249ebd4ae078 100644
> --- a/cmd/load.c
> +++ b/cmd/load.c
> @@ -551,6 +551,9 @@ static ulong load_serial_bin(ulong offset)
>   		udelay(1000);
>   	}
>
> +	if (size == 0)
> +		return ~0; /* Download aborted */
> +

I cannot see where k_recv() sets the return value to 0 if the download
is interrupted after transferring the first packages. So isn't some
logic in k_recv() missing to identify an abort?

Best regards

Heinrich

>   	flush_cache(offset, size);
>
>   	printf("## Total Size      = 0x%08x = %d Bytes\n", size, size);
>

Re: [PATCH] loadb: Properly indicate aborted kermit transfer

[Pali Rohár]

On Friday 06 August 2021 18:23:49 Heinrich Schuchardt wrote:
> On 8/6/21 6:07 PM, Pali Rohár wrote:
> > When k_recv() returns zero it indicates that kermit transfer was aborted.
> > Function do_load_serial_bin() (caller of load_serial_bin()) interprets
> > value ~0 as aborted transfer, so properly propagates information about
> > aborted transfer from k_recv() to do_load_serial_bin().
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >   cmd/load.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/cmd/load.c b/cmd/load.c
> > index 462340ad2cde..249ebd4ae078 100644
> > --- a/cmd/load.c
> > +++ b/cmd/load.c
> > @@ -551,6 +551,9 @@ static ulong load_serial_bin(ulong offset)
> >   		udelay(1000);
> >   	}
> > 
> > +	if (size == 0)
> > +		return ~0; /* Download aborted */
> > +
> 
> I cannot see where k_recv() sets the return value to 0 if the download
> is interrupted after transferring the first packages.

I must admit that I have not decoded whole kermit protocol. But by
testing show that ETX (CTRL+C) is transferred when I abort kermit
upload. And this ETX is handled in switch(getchar()) where is return 0;
and also after sending more kermit packets (not only at beginning).

> So isn't some logic in k_recv() missing to identify an abort?

This is a good question. I would guess that "yes". But to answer it I
need to fully decode kermit protocol...

But at least at this one place it really returns zero on aborted file
transfer.

> Best regards
> 
> Heinrich
> 
> >   	flush_cache(offset, size);
> > 
> >   	printf("## Total Size      = 0x%08x = %d Bytes\n", size, size);
> > 

Re: [PATCH] loadb: Properly indicate aborted kermit transfer

[Pali Rohár]

On Friday 06 August 2021 18:37:15 Pali Rohár wrote:
> On Friday 06 August 2021 18:23:49 Heinrich Schuchardt wrote:
> > On 8/6/21 6:07 PM, Pali Rohár wrote:
> > > When k_recv() returns zero it indicates that kermit transfer was aborted.
> > > Function do_load_serial_bin() (caller of load_serial_bin()) interprets
> > > value ~0 as aborted transfer, so properly propagates information about
> > > aborted transfer from k_recv() to do_load_serial_bin().
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > >   cmd/load.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/cmd/load.c b/cmd/load.c
> > > index 462340ad2cde..249ebd4ae078 100644
> > > --- a/cmd/load.c
> > > +++ b/cmd/load.c
> > > @@ -551,6 +551,9 @@ static ulong load_serial_bin(ulong offset)
> > >   		udelay(1000);
> > >   	}
> > > 
> > > +	if (size == 0)
> > > +		return ~0; /* Download aborted */
> > > +
> > 
> > I cannot see where k_recv() sets the return value to 0 if the download
> > is interrupted after transferring the first packages.
> 
> I must admit that I have not decoded whole kermit protocol. But by
> testing show that ETX (CTRL+C) is transferred when I abort kermit
> upload. And this ETX is handled in switch(getchar()) where is return 0;
> and also after sending more kermit packets (not only at beginning).

So in any case, this patch fixes handling of kermit termination by
pressing CTRL+C key. Simple test is to run 'loadb' and then press
CTRL+C. Other option for testing is to run 'loadb' then start kermit
transfer by ckermit, terminate ckermit transfer and then press CTRL+C.
And this patch fixes also handling of this scenario.

> > So isn't some logic in k_recv() missing to identify an abort?
> 
> This is a good question. I would guess that "yes". But to answer it I
> need to fully decode kermit protocol...
> 
> But at least at this one place it really returns zero on aborted file
> transfer.
> 
> > Best regards
> > 
> > Heinrich
> > 
> > >   	flush_cache(offset, size);
> > > 
> > >   	printf("## Total Size      = 0x%08x = %d Bytes\n", size, size);
> > > 

Re: [PATCH] loadb: Properly indicate aborted kermit transfer

[Tom Rini]

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

On Fri, Aug 06, 2021 at 06:07:39PM +0200, Pali Rohár wrote:

> When k_recv() returns zero it indicates that kermit transfer was aborted.
> Function do_load_serial_bin() (caller of load_serial_bin()) interprets
> value ~0 as aborted transfer, so properly propagates information about
> aborted transfer from k_recv() to do_load_serial_bin().
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Applied to u-boot/next, thanks!

-- 
Tom

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