[PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

[PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

[Pali Rohár]

Now when command loady can be aborted / cancelled by CTRL+C, change wait
timeout for initial packet to infinite. This would allow user to not be
hurry when locating file which want to send. Commands loadb and loads
already waits infinitely too.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 common/xyzModem.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/common/xyzModem.c b/common/xyzModem.c
index ece25acb183b..d6be489a174b 100644
--- a/common/xyzModem.c
+++ b/common/xyzModem.c
@@ -449,8 +449,14 @@ xyzModem_stream_open (connection_info_t * info, int *err)
       return 0;
     }
 
-  while (retries-- > 0)
+  while (1)
     {
+      if (--retries <= 0)
+        {
+          retries = xyzModem_MAX_RETRIES;
+          crc_retries = xyzModem_MAX_RETRIES_WITH_CRC;
+          xyz.crc_mode = true;
+        }
       stat = xyzModem_get_hdr ();
       if (stat == 0)
 	{
-- 
2.20.1

[PATCH 2/2] xyz-modem: Wait infinitely for initial x-modem packet

[Pali Rohár]

Implement same thing also for x-modem protocol. As x-modem protocol does
not have header packet, first packet is directly first data packet.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 common/xyzModem.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/common/xyzModem.c b/common/xyzModem.c
index d6be489a174b..39b40071a13b 100644
--- a/common/xyzModem.c
+++ b/common/xyzModem.c
@@ -50,6 +50,7 @@ static struct
   int len, mode, total_retries;
   int total_SOH, total_STX, total_CAN;
   bool crc_mode, at_eof, tx_ack;
+  bool first_xmodem_packet;
   unsigned long file_length, read_length;
 } xyz;
 
@@ -439,12 +440,14 @@ xyzModem_stream_open (connection_info_t * info, int *err)
   xyz.total_CAN = 0;
   xyz.read_length = 0;
   xyz.file_length = 0;
+  xyz.first_xmodem_packet = false;
 
   CYGACC_COMM_IF_PUTC (*xyz.__chan, (xyz.crc_mode ? 'C' : NAK));
 
   if (xyz.mode == xyzModem_xmodem)
     {
       /* X-modem doesn't have an information header - exit here */
+      xyz.first_xmodem_packet = true;
       xyz.next_blk = 1;
       return 0;
     }
@@ -512,6 +515,8 @@ xyzModem_stream_read (char *buf, int size, int *err)
 	      stat = xyzModem_get_hdr ();
 	      if (stat == 0)
 		{
+		  if (xyz.mode == xyzModem_xmodem && xyz.first_xmodem_packet)
+		    xyz.first_xmodem_packet = false;
 		  if (xyz.blk == xyz.next_blk)
 		    {
 		      xyz.tx_ack = true;
@@ -589,7 +594,7 @@ xyzModem_stream_read (char *buf, int size, int *err)
 	      xyz.total_retries++;
 	      ZM_DEBUG (zm_dprintf ("NAK (%d)\n", __LINE__));
 	    }
-	  if (stat < 0)
+	  if (stat < 0 && (!xyz.first_xmodem_packet || stat != xyzModem_timeout))
 	    {
 	      *err = stat;
 	      xyz.len = -1;
-- 
2.20.1

Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

[Wolfgang Denk]

Dear Pali Rohár,

In message <20210910204653.3066-1-pali@kernel.org> you wrote:
> Now when command loady can be aborted / cancelled by CTRL+C, change wait
> timeout for initial packet to infinite. This would allow user to not be
> hurry when locating file which want to send. Commands loadb and loads
> already waits infinitely too.

I'm not sure if this is a good idea.

If you use loady in any kind of scripts, this would now hard hang
the system, while until now it was possible to recover from the
error.

This is a change to the user interface that is not really necessary,
so I recommend NOT to change the behaviour here, especially as it
does not hurt.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"I've finally learned what `upward compatible' means. It means we get
to keep all our old mistakes." - Dennie van Tassel

Re: [PATCH 2/2] xyz-modem: Wait infinitely for initial x-modem packet

[Wolfgang Denk]

Dear Pali Rohár,

In message <20210910204653.3066-2-pali@kernel.org> you wrote:
> Implement same thing also for x-modem protocol. As x-modem protocol does
> not have header packet, first packet is directly first data packet.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  common/xyzModem.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Ditto here.   Please don't.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"I believe the use of noise to make  music  will  increase  until  we
reach  a  music  produced  through  the aid of electrical instruments
which will make available for musical purposes  any  and  all  sounds
that can be heard."                        - composer John Cage, 1937

Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

[Pali Rohár]

On Monday 13 September 2021 12:42:42 Wolfgang Denk wrote:
> Dear Pali Rohár,
> 
> In message <20210910204653.3066-1-pali@kernel.org> you wrote:
> > Now when command loady can be aborted / cancelled by CTRL+C, change wait
> > timeout for initial packet to infinite. This would allow user to not be
> > hurry when locating file which want to send. Commands loadb and loads
> > already waits infinitely too.
> 
> I'm not sure if this is a good idea.
> 
> If you use loady in any kind of scripts, this would now hard hang
> the system, while until now it was possible to recover from the
> error.

Yes, this is a good point. But on the other hand, 'loadb' and 'loads'
commands already have this behavior. So question is if it is better to
have same behavior in all 'load?' commands or each 'load?' would behave
differently... Because for software which transmit files and supports
more protocols (e.g. both x-modem and kermit) it may be a nightmare if
receiver behaves differently...

> This is a change to the user interface that is not really necessary,
> so I recommend NOT to change the behaviour here, especially as it
> does not hurt.

Well... there is an issue if you do not start file transfer in the
timeout which current 'loady' command expects.

If you do not have integrated y-modem support in your terminal you have
to do:

1) open terminal and write 'loady' into U-Boot console
2) disconnect terminal
3) start y-modem software
4) choose file to transmit
5) instruct y-modem software to start transfer

And if 'loady' timeouts between 2) - 5) then it returns back to the
U-Boot console. So y-modem software in step 5) starts interaction with
U-Boot console instead of 'loady' and try to interpret U-Boot prompt as
y-modem sequence... which is detected as garbage and y-modem software
would either wait or try retransmit... which sends some garbage to
U-Boot console and something may be executed.

'loadb' commands does not have this issue as it waits either for CTRL+C
or for successful kermit transfer.


So... I do not know what is better if current behavior or this new which
changes UI interaction.

> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "I've finally learned what `upward compatible' means. It means we get
> to keep all our old mistakes." - Dennie van Tassel

Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

[Wolfgang Denk]

Dear Pali Rohár,

In message <20210913110806.27hc36n6gmhw6uq4@pali> you wrote:
>
> > If you use loady in any kind of scripts, this would now hard hang
> > the system, while until now it was possible to recover from the
> > error.
>
> Yes, this is a good point. But on the other hand, 'loadb' and 'loads'
> commands already have this behavior. So question is if it is better to
> have same behavior in all 'load?' commands or each 'load?' would behave
> differently... Because for software which transmit files and supports
> more protocols (e.g. both x-modem and kermit) it may be a nightmare if
> receiver behaves differently...

Yes, you are right, there is an unlucky difference in behaviour.
But all these interfaces are pretty old, and I would not invest
efforts to fix a aproblem nobody ever noticed before, at the risk
of breaking existing stuff.

I wonder if there are any users of 'loads' left - the Motorola
S-record format is close to 50 years old and cumbersome to use.  I
can't even remeber when I used it the last time - must be 15+ years
or such.

'loadb" is a different thing, but there you usually have kermit on
the other side, and usually run an interactive session (or an
automated one using something like  tbot ) - in any case, you
normally have to interact on both sides.

I never had a problem wih the current behaviour there.


> If you do not have integrated y-modem support in your terminal you have
> to do:
>
> 1) open terminal and write 'loady' into U-Boot console
> 2) disconnect terminal
> 3) start y-modem software
> 4) choose file to transmit
> 5) instruct y-modem software to start transfer
>
> And if 'loady' timeouts between 2) - 5) then it returns back to the

If this happens, the timeout is inconveniently short of you are too
slow.  I think what would be helpful is to make the timeout
adjustable (env var).

> So... I do not know what is better if current behavior or this new which
> changes UI interaction.

We can do both, and still solve your problem: make the timeout
adjustable so you can set it to something you can conveniently work
with.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"I can call spirits from the vasty deep."
"Why so can I, or so can any man; but will they come when you do call
for them?"          - Shakespeare, 1 King Henry IV, Act III, Scene I.

Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

[Pali Rohár]

On Monday 13 September 2021 14:12:54 Wolfgang Denk wrote:
> Dear Pali Rohár,
> 
> In message <20210913110806.27hc36n6gmhw6uq4@pali> you wrote:
> >
> > > If you use loady in any kind of scripts, this would now hard hang
> > > the system, while until now it was possible to recover from the
> > > error.
> >
> > Yes, this is a good point. But on the other hand, 'loadb' and 'loads'
> > commands already have this behavior. So question is if it is better to
> > have same behavior in all 'load?' commands or each 'load?' would behave
> > differently... Because for software which transmit files and supports
> > more protocols (e.g. both x-modem and kermit) it may be a nightmare if
> > receiver behaves differently...
> 
> Yes, you are right, there is an unlucky difference in behaviour.
> But all these interfaces are pretty old, and I would not invest
> efforts to fix a aproblem nobody ever noticed before, at the risk
> of breaking existing stuff.
> 
> I wonder if there are any users of 'loads' left - the Motorola
> S-record format is close to 50 years old and cumbersome to use.

Wow, I did not know that it is such old.

> I can't even remeber when I used it the last time - must be 15+ years
> or such.
> 
> 'loadb" is a different thing, but there you usually have kermit on
> the other side, and usually run an interactive session (or an
> automated one using something like  tbot ) - in any case, you
> normally have to interact on both sides.

There is also gkermit tool which do not implement interactive session,
only file transfer itself. And it is present in more linux distributions
so is quite popular...

> I never had a problem wih the current behaviour there.
> 
> 
> > If you do not have integrated y-modem support in your terminal you have
> > to do:
> >
> > 1) open terminal and write 'loady' into U-Boot console
> > 2) disconnect terminal
> > 3) start y-modem software
> > 4) choose file to transmit
> > 5) instruct y-modem software to start transfer
> >
> > And if 'loady' timeouts between 2) - 5) then it returns back to the
> 
> If this happens, the timeout is inconveniently short of you are too
> slow.  I think what would be helpful is to make the timeout
> adjustable (env var).

Timeout is not too slow, but sometimes user is (when is interrupted by
other things during selecting file). And then it is not obvious why
sx/sb command is failing... compared to transfer via gkermit which do
not have this "problem".

> > So... I do not know what is better if current behavior or this new which
> > changes UI interaction.
> 
> We can do both, and still solve your problem: make the timeout
> adjustable so you can set it to something you can conveniently work
> with.

Good point! env with timeout (or easier would be retry count?) seems
like a ideal solution how to "define" behavior without changing default
one. And e.g. negative value can mean infinite to support all possible
combinations.

> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "I can call spirits from the vasty deep."
> "Why so can I, or so can any man; but will they come when you do call
> for them?"          - Shakespeare, 1 King Henry IV, Act III, Scene I.

Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

[Wolfgang Denk]

Dear Pali Rohár,

In message <20210913122245.my6ik4yjy7rwlh65@pali> you wrote:
>
> Timeout is not too slow, but sometimes user is (when is interrupted by
> other things during selecting file). And then it is not obvious why
> sx/sb command is failing... compared to transfer via gkermit which do
> not have this "problem".

I see.

> > > So... I do not know what is better if current behavior or this new which
> > > changes UI interaction.
> > 
> > We can do both, and still solve your problem: make the timeout
> > adjustable so you can set it to something you can conveniently work
> > with.
>
> Good point! env with timeout (or easier would be retry count?) seems
> like a ideal solution how to "define" behavior without changing default
> one. And e.g. negative value can mean infinite to support all possible
> combinations.

I would recommend to define a timeout (in seconds), this is easier
to understand for the end user - without looking at the source code
they cannot know how a retry count translates into a time interval.
And yes, setting the timeout to 0 could mean it waits forever.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The optimum committee has no members.
                                                   - Norman Augustine

Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

[Tom Rini]

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

On Mon, Sep 13, 2021 at 01:08:06PM +0200, Pali Rohár wrote:
> On Monday 13 September 2021 12:42:42 Wolfgang Denk wrote:
> > Dear Pali Rohár,
> > 
> > In message <20210910204653.3066-1-pali@kernel.org> you wrote:
> > > Now when command loady can be aborted / cancelled by CTRL+C, change wait
> > > timeout for initial packet to infinite. This would allow user to not be
> > > hurry when locating file which want to send. Commands loadb and loads
> > > already waits infinitely too.
> > 
> > I'm not sure if this is a good idea.
> > 
> > If you use loady in any kind of scripts, this would now hard hang
> > the system, while until now it was possible to recover from the
> > error.
> 
> Yes, this is a good point. But on the other hand, 'loadb' and 'loads'
> commands already have this behavior. So question is if it is better to
> have same behavior in all 'load?' commands or each 'load?' would behave
> differently... Because for software which transmit files and supports
> more protocols (e.g. both x-modem and kermit) it may be a nightmare if
> receiver behaves differently...
> 
> > This is a change to the user interface that is not really necessary,
> > so I recommend NOT to change the behaviour here, especially as it
> > does not hurt.
> 
> Well... there is an issue if you do not start file transfer in the
> timeout which current 'loady' command expects.
> 
> If you do not have integrated y-modem support in your terminal you have
> to do:
> 
> 1) open terminal and write 'loady' into U-Boot console
> 2) disconnect terminal
> 3) start y-modem software
> 4) choose file to transmit
> 5) instruct y-modem software to start transfer

So, when does this happen?  It's been a long long time since I used
minicom, but I know in screen you can do a filter to trigger the
transfer, ie:
:exec !! /home/trini/bin/load-boot-xymodem.sh
is one of the things in my notes and little script is just to sx SPL and
then sx --ymodem U-Boot itself.  I know tmux is the preferred tool of
power users these days but I assume it can do similar.

-- 
Tom

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

[PATCH v2] xyz-modem: Allow to configure initial timeout for loadx and loady

[Pali Rohár]

Now when loadx and loady commands could be aborted / cancelled by CTRL+C,
allow to configure timeout for initial x/y-modem packet via env variable
$loadxy_timeout and by default use value from new compile-time config
option CONFIG_CMD_LOADXY_TIMEOUT. Value is in seconds and zero value means
infinite timeout. Default value is 90s which is the measured value used
before this change for loadx and loady commands.

Other load commands loadb and loads already waits infinitely. Same behavior
for loadx and loady commands can be achieved by setting $loadxy_timeout or
CONFIG_CMD_LOADXY_TIMEOUT to 0.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
Changes in v2:
* Allow to set timeout via env instead of permanent infinite timeout
---
 cmd/Kconfig       |  7 +++++++
 common/xyzModem.c | 39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 211ebe9c8783..54af3769a673 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1194,6 +1194,13 @@ config CMD_LOADS
 	help
 	  Load an S-Record file over serial line
 
+config CMD_LOADXY_TIMEOUT
+	int "loadxy_timeout"
+	range 0 2000
+	default 90
+	help
+	  Initial timeout for loadx and loady commands. Zero means infinity.
+
 config CMD_LSBLK
 	depends on BLK
 	bool "lsblk - list block drivers and devices"
diff --git a/common/xyzModem.c b/common/xyzModem.c
index ece25acb183b..700df8edd14a 100644
--- a/common/xyzModem.c
+++ b/common/xyzModem.c
@@ -26,6 +26,7 @@
 #include <stdarg.h>
 #include <u-boot/crc.h>
 #include <watchdog.h>
+#include <env.h>
 
 /* Assumption - run xyzModem protocol over the console port */
 
@@ -50,6 +51,8 @@ static struct
   int len, mode, total_retries;
   int total_SOH, total_STX, total_CAN;
   bool crc_mode, at_eof, tx_ack;
+  bool first_xmodem_packet;
+  ulong initial_time, timeout;
   unsigned long file_length, read_length;
 } xyz;
 
@@ -409,6 +412,18 @@ xyzModem_get_hdr (void)
   return 0;
 }
 
+static
+ulong
+xyzModem_get_initial_timeout (void)
+{
+  /* timeout is in seconds, non-positive timeout value is infinity */
+  const char *timeout_str = env_get("loadxy_timeout");
+  if (timeout_str)
+    return 1000 * simple_strtol(timeout_str, NULL, 10);
+  else
+    return 1000 * CONFIG_CMD_LOADXY_TIMEOUT;
+}
+
 int
 xyzModem_stream_open (connection_info_t * info, int *err)
 {
@@ -439,18 +454,28 @@ xyzModem_stream_open (connection_info_t * info, int *err)
   xyz.total_CAN = 0;
   xyz.read_length = 0;
   xyz.file_length = 0;
+  xyz.first_xmodem_packet = false;
+  xyz.initial_time = get_timer(0);
+  xyz.timeout = xyzModem_get_initial_timeout();
 
   CYGACC_COMM_IF_PUTC (*xyz.__chan, (xyz.crc_mode ? 'C' : NAK));
 
   if (xyz.mode == xyzModem_xmodem)
     {
       /* X-modem doesn't have an information header - exit here */
+      xyz.first_xmodem_packet = true;
       xyz.next_blk = 1;
       return 0;
     }
 
-  while (retries-- > 0)
+  while (!(xyz.timeout && get_timer(xyz.initial_time) > xyz.timeout))
     {
+      if (--retries <= 0)
+        {
+          retries = xyzModem_MAX_RETRIES;
+          crc_retries = xyzModem_MAX_RETRIES_WITH_CRC;
+          xyz.crc_mode = true;
+        }
       stat = xyzModem_get_hdr ();
       if (stat == 0)
 	{
@@ -503,9 +528,19 @@ xyzModem_stream_read (char *buf, int size, int *err)
 	  retries = xyzModem_MAX_RETRIES;
 	  while (retries-- > 0)
 	    {
+	      if (xyz.first_xmodem_packet && xyz.timeout &&
+		  get_timer(xyz.initial_time) > xyz.timeout)
+		{
+		  *err = xyzModem_timeout;
+		  xyz.len = -1;
+		  return total;
+		}
+
 	      stat = xyzModem_get_hdr ();
 	      if (stat == 0)
 		{
+		  if (xyz.mode == xyzModem_xmodem && xyz.first_xmodem_packet)
+		    xyz.first_xmodem_packet = false;
 		  if (xyz.blk == xyz.next_blk)
 		    {
 		      xyz.tx_ack = true;
@@ -583,7 +618,7 @@ xyzModem_stream_read (char *buf, int size, int *err)
 	      xyz.total_retries++;
 	      ZM_DEBUG (zm_dprintf ("NAK (%d)\n", __LINE__));
 	    }
-	  if (stat < 0)
+	  if (stat < 0 && (!xyz.first_xmodem_packet || stat != xyzModem_timeout))
 	    {
 	      *err = stat;
 	      xyz.len = -1;
-- 
2.20.1

Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

[Pali Rohár]

On Monday 13 September 2021 15:02:50 Wolfgang Denk wrote:
> Dear Pali Rohár,
> 
> In message <20210913122245.my6ik4yjy7rwlh65@pali> you wrote:
> >
> > Timeout is not too slow, but sometimes user is (when is interrupted by
> > other things during selecting file). And then it is not obvious why
> > sx/sb command is failing... compared to transfer via gkermit which do
> > not have this "problem".
> 
> I see.
> 
> > > > So... I do not know what is better if current behavior or this new which
> > > > changes UI interaction.
> > > 
> > > We can do both, and still solve your problem: make the timeout
> > > adjustable so you can set it to something you can conveniently work
> > > with.
> >
> > Good point! env with timeout (or easier would be retry count?) seems
> > like a ideal solution how to "define" behavior without changing default
> > one. And e.g. negative value can mean infinite to support all possible
> > combinations.
> 
> I would recommend to define a timeout (in seconds), this is easier
> to understand for the end user - without looking at the source code
> they cannot know how a retry count translates into a time interval.
> And yes, setting the timeout to 0 could mean it waits forever.

This behavior is now implemented in v2. Current default timeout
(measured 90s) stay same for compatibility reasons.

Re: [PATCH v2] xyz-modem: Allow to configure initial timeout for loadx and loady

[Tom Rini]

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

On Sat, Aug 27, 2022 at 01:48:35PM +0200, Pali Rohár wrote:

> Now when loadx and loady commands could be aborted / cancelled by CTRL+C,
> allow to configure timeout for initial x/y-modem packet via env variable
> $loadxy_timeout and by default use value from new compile-time config
> option CONFIG_CMD_LOADXY_TIMEOUT. Value is in seconds and zero value means
> infinite timeout. Default value is 90s which is the measured value used
> before this change for loadx and loady commands.
> 
> Other load commands loadb and loads already waits infinitely. Same behavior
> for loadx and loady commands can be achieved by setting $loadxy_timeout or
> CONFIG_CMD_LOADXY_TIMEOUT to 0.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> Changes in v2:
> * Allow to set timeout via env instead of permanent infinite timeout

This breaks platforms like "porter" where we have SPL_YMODEM support but
do not have environment support.

-- 
Tom

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

Re: [PATCH v2] xyz-modem: Allow to configure initial timeout for loadx and loady

[Pali Rohár]

On Saturday 27 August 2022 08:53:08 Tom Rini wrote:
> On Sat, Aug 27, 2022 at 01:48:35PM +0200, Pali Rohár wrote:
> 
> > Now when loadx and loady commands could be aborted / cancelled by CTRL+C,
> > allow to configure timeout for initial x/y-modem packet via env variable
> > $loadxy_timeout and by default use value from new compile-time config
> > option CONFIG_CMD_LOADXY_TIMEOUT. Value is in seconds and zero value means
> > infinite timeout. Default value is 90s which is the measured value used
> > before this change for loadx and loady commands.
> > 
> > Other load commands loadb and loads already waits infinitely. Same behavior
> > for loadx and loady commands can be achieved by setting $loadxy_timeout or
> > CONFIG_CMD_LOADXY_TIMEOUT to 0.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> > Changes in v2:
> > * Allow to set timeout via env instead of permanent infinite timeout
> 
> This breaks platforms like "porter" where we have SPL_YMODEM support but
> do not have environment support.

By "breaks" do you mean compile error? Because if env is not set then
CONFIG_CMD_LOADXY_TIMEOUT is used. Just I'm not sure if env_get()
function returns NULL or what when there is no environment support.

Re: [PATCH v2] xyz-modem: Allow to configure initial timeout for loadx and loady

[Tom Rini]

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

On Sat, Aug 27, 2022 at 02:56:15PM +0200, Pali Rohár wrote:
> On Saturday 27 August 2022 08:53:08 Tom Rini wrote:
> > On Sat, Aug 27, 2022 at 01:48:35PM +0200, Pali Rohár wrote:
> > 
> > > Now when loadx and loady commands could be aborted / cancelled by CTRL+C,
> > > allow to configure timeout for initial x/y-modem packet via env variable
> > > $loadxy_timeout and by default use value from new compile-time config
> > > option CONFIG_CMD_LOADXY_TIMEOUT. Value is in seconds and zero value means
> > > infinite timeout. Default value is 90s which is the measured value used
> > > before this change for loadx and loady commands.
> > > 
> > > Other load commands loadb and loads already waits infinitely. Same behavior
> > > for loadx and loady commands can be achieved by setting $loadxy_timeout or
> > > CONFIG_CMD_LOADXY_TIMEOUT to 0.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > > Changes in v2:
> > > * Allow to set timeout via env instead of permanent infinite timeout
> > 
> > This breaks platforms like "porter" where we have SPL_YMODEM support but
> > do not have environment support.
> 
> By "breaks" do you mean compile error? Because if env is not set then
> CONFIG_CMD_LOADXY_TIMEOUT is used. Just I'm not sure if env_get()
> function returns NULL or what when there is no environment support.

Yes, it fails to link.

-- 
Tom

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

Re: [PATCH v2] xyz-modem: Allow to configure initial timeout for loadx and loady

[Pali Rohár]

On Saturday 27 August 2022 10:30:05 Tom Rini wrote:
> On Sat, Aug 27, 2022 at 02:56:15PM +0200, Pali Rohár wrote:
> > On Saturday 27 August 2022 08:53:08 Tom Rini wrote:
> > > On Sat, Aug 27, 2022 at 01:48:35PM +0200, Pali Rohár wrote:
> > > 
> > > > Now when loadx and loady commands could be aborted / cancelled by CTRL+C,
> > > > allow to configure timeout for initial x/y-modem packet via env variable
> > > > $loadxy_timeout and by default use value from new compile-time config
> > > > option CONFIG_CMD_LOADXY_TIMEOUT. Value is in seconds and zero value means
> > > > infinite timeout. Default value is 90s which is the measured value used
> > > > before this change for loadx and loady commands.
> > > > 
> > > > Other load commands loadb and loads already waits infinitely. Same behavior
> > > > for loadx and loady commands can be achieved by setting $loadxy_timeout or
> > > > CONFIG_CMD_LOADXY_TIMEOUT to 0.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > > Changes in v2:
> > > > * Allow to set timeout via env instead of permanent infinite timeout
> > > 
> > > This breaks platforms like "porter" where we have SPL_YMODEM support but
> > > do not have environment support.
> > 
> > By "breaks" do you mean compile error? Because if env is not set then
> > CONFIG_CMD_LOADXY_TIMEOUT is used. Just I'm not sure if env_get()
> > function returns NULL or what when there is no environment support.
> 
> Yes, it fails to link.

Then just #ifdef for env is needed. I will prepare v3. Which defconfig it is?

[PATCH v3] xyz-modem: Allow to configure initial timeout for loadx and loady

[Pali Rohár]

Now when loadx and loady commands could be aborted / cancelled by CTRL+C,
allow to configure timeout for initial x/y-modem packet via env variable
$loadxy_timeout and by default use value from new compile-time config
option CONFIG_CMD_LOADXY_TIMEOUT. Value is in seconds and zero value means
infinite timeout. Default value is 90s which is the value used before this
change for loadx command.

Other load commands loadb and loads already waits infinitely. Same behavior
for loadx and loady commands can be achieved by setting $loadxy_timeout or
CONFIG_CMD_LOADXY_TIMEOUT to 0.

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v3:
* Guard env_get via #ifdef (same what was used for d64a8fd0a902 "hwconfig:
  Allow to compile it without env support")

Changes in v2:
* Do not remove MVEBU_TEST_PIN_LATCH_N and MVEBU_XTAL_MODE_MASK macros
---
 cmd/Kconfig       |  7 +++++++
 common/xyzModem.c | 40 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 211ebe9c8783..54af3769a673 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1194,6 +1194,13 @@ config CMD_LOADS
 	help
 	  Load an S-Record file over serial line
 
+config CMD_LOADXY_TIMEOUT
+	int "loadxy_timeout"
+	range 0 2000
+	default 90
+	help
+	  Initial timeout for loadx and loady commands. Zero means infinity.
+
 config CMD_LSBLK
 	depends on BLK
 	bool "lsblk - list block drivers and devices"
diff --git a/common/xyzModem.c b/common/xyzModem.c
index ece25acb183b..a68d3929024b 100644
--- a/common/xyzModem.c
+++ b/common/xyzModem.c
@@ -26,6 +26,7 @@
 #include <stdarg.h>
 #include <u-boot/crc.h>
 #include <watchdog.h>
+#include <env.h>
 
 /* Assumption - run xyzModem protocol over the console port */
 
@@ -50,6 +51,8 @@ static struct
   int len, mode, total_retries;
   int total_SOH, total_STX, total_CAN;
   bool crc_mode, at_eof, tx_ack;
+  bool first_xmodem_packet;
+  ulong initial_time, timeout;
   unsigned long file_length, read_length;
 } xyz;
 
@@ -409,6 +412,19 @@ xyzModem_get_hdr (void)
   return 0;
 }
 
+static
+ulong
+xyzModem_get_initial_timeout (void)
+{
+  /* timeout is in seconds, non-positive timeout value is infinity */
+#if CONFIG_IS_ENABLED(ENV_SUPPORT)
+  const char *timeout_str = env_get("loadxy_timeout");
+  if (timeout_str)
+    return 1000 * simple_strtol(timeout_str, NULL, 10);
+#endif
+  return 1000 * CONFIG_CMD_LOADXY_TIMEOUT;
+}
+
 int
 xyzModem_stream_open (connection_info_t * info, int *err)
 {
@@ -439,18 +455,28 @@ xyzModem_stream_open (connection_info_t * info, int *err)
   xyz.total_CAN = 0;
   xyz.read_length = 0;
   xyz.file_length = 0;
+  xyz.first_xmodem_packet = false;
+  xyz.initial_time = get_timer(0);
+  xyz.timeout = xyzModem_get_initial_timeout();
 
   CYGACC_COMM_IF_PUTC (*xyz.__chan, (xyz.crc_mode ? 'C' : NAK));
 
   if (xyz.mode == xyzModem_xmodem)
     {
       /* X-modem doesn't have an information header - exit here */
+      xyz.first_xmodem_packet = true;
       xyz.next_blk = 1;
       return 0;
     }
 
-  while (retries-- > 0)
+  while (!(xyz.timeout && get_timer(xyz.initial_time) > xyz.timeout))
     {
+      if (--retries <= 0)
+        {
+          retries = xyzModem_MAX_RETRIES;
+          crc_retries = xyzModem_MAX_RETRIES_WITH_CRC;
+          xyz.crc_mode = true;
+        }
       stat = xyzModem_get_hdr ();
       if (stat == 0)
 	{
@@ -503,9 +529,19 @@ xyzModem_stream_read (char *buf, int size, int *err)
 	  retries = xyzModem_MAX_RETRIES;
 	  while (retries-- > 0)
 	    {
+	      if (xyz.first_xmodem_packet && xyz.timeout &&
+		  get_timer(xyz.initial_time) > xyz.timeout)
+		{
+		  *err = xyzModem_timeout;
+		  xyz.len = -1;
+		  return total;
+		}
+
 	      stat = xyzModem_get_hdr ();
 	      if (stat == 0)
 		{
+		  if (xyz.mode == xyzModem_xmodem && xyz.first_xmodem_packet)
+		    xyz.first_xmodem_packet = false;
 		  if (xyz.blk == xyz.next_blk)
 		    {
 		      xyz.tx_ack = true;
@@ -583,7 +619,7 @@ xyzModem_stream_read (char *buf, int size, int *err)
 	      xyz.total_retries++;
 	      ZM_DEBUG (zm_dprintf ("NAK (%d)\n", __LINE__));
 	    }
-	  if (stat < 0)
+	  if (stat < 0 && (!xyz.first_xmodem_packet || stat != xyzModem_timeout))
 	    {
 	      *err = stat;
 	      xyz.len = -1;
-- 
2.20.1

Re: [PATCH v2] xyz-modem: Allow to configure initial timeout for loadx and loady

[Tom Rini]

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

On Sat, Aug 27, 2022 at 04:32:37PM +0200, Pali Rohár wrote:
> On Saturday 27 August 2022 10:30:05 Tom Rini wrote:
> > On Sat, Aug 27, 2022 at 02:56:15PM +0200, Pali Rohár wrote:
> > > On Saturday 27 August 2022 08:53:08 Tom Rini wrote:
> > > > On Sat, Aug 27, 2022 at 01:48:35PM +0200, Pali Rohár wrote:
> > > > 
> > > > > Now when loadx and loady commands could be aborted / cancelled by CTRL+C,
> > > > > allow to configure timeout for initial x/y-modem packet via env variable
> > > > > $loadxy_timeout and by default use value from new compile-time config
> > > > > option CONFIG_CMD_LOADXY_TIMEOUT. Value is in seconds and zero value means
> > > > > infinite timeout. Default value is 90s which is the measured value used
> > > > > before this change for loadx and loady commands.
> > > > > 
> > > > > Other load commands loadb and loads already waits infinitely. Same behavior
> > > > > for loadx and loady commands can be achieved by setting $loadxy_timeout or
> > > > > CONFIG_CMD_LOADXY_TIMEOUT to 0.
> > > > > 
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > ---
> > > > > Changes in v2:
> > > > > * Allow to set timeout via env instead of permanent infinite timeout
> > > > 
> > > > This breaks platforms like "porter" where we have SPL_YMODEM support but
> > > > do not have environment support.
> > > 
> > > By "breaks" do you mean compile error? Because if env is not set then
> > > CONFIG_CMD_LOADXY_TIMEOUT is used. Just I'm not sure if env_get()
> > > function returns NULL or what when there is no environment support.
> > 
> > Yes, it fails to link.
> 
> Then just #ifdef for env is needed. I will prepare v3. Which defconfig it is?

It's the "porter" defconfig, sorry I thought it would be clearer.
There's probably others too in the case of SPL_YMODEM without SPL_ENV,
it was just the first one I guessed.

-- 
Tom

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

Re: [PATCH v2] xyz-modem: Allow to configure initial timeout for loadx and loady

[Pali Rohár]

On Saturday 27 August 2022 10:41:00 Tom Rini wrote:
> On Sat, Aug 27, 2022 at 04:32:37PM +0200, Pali Rohár wrote:
> > On Saturday 27 August 2022 10:30:05 Tom Rini wrote:
> > > On Sat, Aug 27, 2022 at 02:56:15PM +0200, Pali Rohár wrote:
> > > > On Saturday 27 August 2022 08:53:08 Tom Rini wrote:
> > > > > On Sat, Aug 27, 2022 at 01:48:35PM +0200, Pali Rohár wrote:
> > > > > 
> > > > > > Now when loadx and loady commands could be aborted / cancelled by CTRL+C,
> > > > > > allow to configure timeout for initial x/y-modem packet via env variable
> > > > > > $loadxy_timeout and by default use value from new compile-time config
> > > > > > option CONFIG_CMD_LOADXY_TIMEOUT. Value is in seconds and zero value means
> > > > > > infinite timeout. Default value is 90s which is the measured value used
> > > > > > before this change for loadx and loady commands.
> > > > > > 
> > > > > > Other load commands loadb and loads already waits infinitely. Same behavior
> > > > > > for loadx and loady commands can be achieved by setting $loadxy_timeout or
> > > > > > CONFIG_CMD_LOADXY_TIMEOUT to 0.
> > > > > > 
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > * Allow to set timeout via env instead of permanent infinite timeout
> > > > > 
> > > > > This breaks platforms like "porter" where we have SPL_YMODEM support but
> > > > > do not have environment support.
> > > > 
> > > > By "breaks" do you mean compile error? Because if env is not set then
> > > > CONFIG_CMD_LOADXY_TIMEOUT is used. Just I'm not sure if env_get()
> > > > function returns NULL or what when there is no environment support.
> > > 
> > > Yes, it fails to link.
> > 
> > Then just #ifdef for env is needed. I will prepare v3. Which defconfig it is?
> 
> It's the "porter" defconfig, sorry I thought it would be clearer.
> There's probably others too in the case of SPL_YMODEM without SPL_ENV,
> it was just the first one I guessed.

Ou, I did not think that porter is defconfig. It sounds like code
porting. Anyway I have already sent v3 and now I tested it with
porter_defconfig that it compiles fine:
https://patchwork.ozlabs.org/project/uboot/patch/20220827143755.21412-1-pali@kernel.org/

Re: [PATCH v2] xyz-modem: Allow to configure initial timeout for loadx and loady

[Heinrich Schuchardt]

On 8/27/22 13:48, Pali Rohár wrote:
> Now when loadx and loady commands could be aborted / cancelled by CTRL+C,
> allow to configure timeout for initial x/y-modem packet via env variable
> $loadxy_timeout and by default use value from new compile-time config
> option CONFIG_CMD_LOADXY_TIMEOUT. Value is in seconds and zero value means
> infinite timeout. Default value is 90s which is the measured value used
> before this change for loadx and loady commands.
>
> Other load commands loadb and loads already waits infinitely. Same behavior
> for loadx and loady commands can be achieved by setting $loadxy_timeout or
> CONFIG_CMD_LOADXY_TIMEOUT to 0.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> Changes in v2:
> * Allow to set timeout via env instead of permanent infinite timeout
> ---
>   cmd/Kconfig       |  7 +++++++
>   common/xyzModem.c | 39 +++++++++++++++++++++++++++++++++++++--

The environment variable should be described in doc/usage/cmd/loady.rst

Best regards

Heinrich


>   2 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 211ebe9c8783..54af3769a673 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1194,6 +1194,13 @@ config CMD_LOADS
>   	help
>   	  Load an S-Record file over serial line
>
> +config CMD_LOADXY_TIMEOUT
> +	int "loadxy_timeout"
> +	range 0 2000
> +	default 90
> +	help
> +	  Initial timeout for loadx and loady commands. Zero means infinity.
> +
>   config CMD_LSBLK
>   	depends on BLK
>   	bool "lsblk - list block drivers and devices"
> diff --git a/common/xyzModem.c b/common/xyzModem.c
> index ece25acb183b..700df8edd14a 100644
> --- a/common/xyzModem.c
> +++ b/common/xyzModem.c
> @@ -26,6 +26,7 @@
>   #include <stdarg.h>
>   #include <u-boot/crc.h>
>   #include <watchdog.h>
> +#include <env.h>
>
>   /* Assumption - run xyzModem protocol over the console port */
>
> @@ -50,6 +51,8 @@ static struct
>     int len, mode, total_retries;
>     int total_SOH, total_STX, total_CAN;
>     bool crc_mode, at_eof, tx_ack;
> +  bool first_xmodem_packet;
> +  ulong initial_time, timeout;
>     unsigned long file_length, read_length;
>   } xyz;
>
> @@ -409,6 +412,18 @@ xyzModem_get_hdr (void)
>     return 0;
>   }
>
> +static
> +ulong
> +xyzModem_get_initial_timeout (void)
> +{
> +  /* timeout is in seconds, non-positive timeout value is infinity */
> +  const char *timeout_str = env_get("loadxy_timeout");
> +  if (timeout_str)
> +    return 1000 * simple_strtol(timeout_str, NULL, 10);
> +  else
> +    return 1000 * CONFIG_CMD_LOADXY_TIMEOUT;
> +}
> +
>   int
>   xyzModem_stream_open (connection_info_t * info, int *err)
>   {
> @@ -439,18 +454,28 @@ xyzModem_stream_open (connection_info_t * info, int *err)
>     xyz.total_CAN = 0;
>     xyz.read_length = 0;
>     xyz.file_length = 0;
> +  xyz.first_xmodem_packet = false;
> +  xyz.initial_time = get_timer(0);
> +  xyz.timeout = xyzModem_get_initial_timeout();
>
>     CYGACC_COMM_IF_PUTC (*xyz.__chan, (xyz.crc_mode ? 'C' : NAK));
>
>     if (xyz.mode == xyzModem_xmodem)
>       {
>         /* X-modem doesn't have an information header - exit here */
> +      xyz.first_xmodem_packet = true;
>         xyz.next_blk = 1;
>         return 0;
>       }
>
> -  while (retries-- > 0)
> +  while (!(xyz.timeout && get_timer(xyz.initial_time) > xyz.timeout))
>       {
> +      if (--retries <= 0)
> +        {
> +          retries = xyzModem_MAX_RETRIES;
> +          crc_retries = xyzModem_MAX_RETRIES_WITH_CRC;
> +          xyz.crc_mode = true;
> +        }
>         stat = xyzModem_get_hdr ();
>         if (stat == 0)
>   	{
> @@ -503,9 +528,19 @@ xyzModem_stream_read (char *buf, int size, int *err)
>   	  retries = xyzModem_MAX_RETRIES;
>   	  while (retries-- > 0)
>   	    {
> +	      if (xyz.first_xmodem_packet && xyz.timeout &&
> +		  get_timer(xyz.initial_time) > xyz.timeout)
> +		{
> +		  *err = xyzModem_timeout;
> +		  xyz.len = -1;
> +		  return total;
> +		}
> +
>   	      stat = xyzModem_get_hdr ();
>   	      if (stat == 0)
>   		{
> +		  if (xyz.mode == xyzModem_xmodem && xyz.first_xmodem_packet)
> +		    xyz.first_xmodem_packet = false;
>   		  if (xyz.blk == xyz.next_blk)
>   		    {
>   		      xyz.tx_ack = true;
> @@ -583,7 +618,7 @@ xyzModem_stream_read (char *buf, int size, int *err)
>   	      xyz.total_retries++;
>   	      ZM_DEBUG (zm_dprintf ("NAK (%d)\n", __LINE__));
>   	    }
> -	  if (stat < 0)
> +	  if (stat < 0 && (!xyz.first_xmodem_packet || stat != xyzModem_timeout))
>   	    {
>   	      *err = stat;
>   	      xyz.len = -1;