Re: [PATCH v2 09/10] fastboot net: implement fastboot over UDP

["Daniel Glöckner" <dg@emlix.com>]

Hello Sascha,

On Wed, May 20, 2020 at 07:52:32AM +0200, Sascha Hauer wrote:
> On Thu, May 14, 2020 at 08:21:57PM +0200, Daniel Glöckner wrote:
> > From: Edmund Henniges <eh@emlix.com>
> > 
> > This implements the UDP variant of the fastboot protocol. The only way to
> > start the service for now is to compile with CONFIG_FASTBOOT_NET_ON_BOOT.
> > The service will bind to the network interface that provides the IPv4
> > gateway.
> > 
> > Sending an OKAY packet before performing a restart is necessary since
> > contrary to USB the host will not notice when a UDP server disappears.
> > 
> > Signed-off-by: Edmund Henniges <eh@emlix.com>
> > Signed-off-by: Daniel Glöckner <dg@emlix.com>
> > ---
> >  common/fastboot.c      |   3 +
> >  include/fastboot.h     |   1 +
> >  include/fastboot_net.h |  12 +
> >  net/Kconfig            |  18 ++
> >  net/Makefile           |   1 +
> >  net/fastboot.c         | 496 +++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 531 insertions(+)
> >  create mode 100644 include/fastboot_net.h
> >  create mode 100644 net/fastboot.c
> > 
> > diff --git a/common/fastboot.c b/common/fastboot.c
> > index d50f61ff2..706547309 100644
> > --- a/common/fastboot.c
> > +++ b/common/fastboot.c
> > +static int fastboot_write_net(struct fastboot *fb, const char *buf,
> > +			      unsigned int n)
> > +{
> > +	struct fastboot_net *fbn = container_of(fb, struct fastboot_net,
> > +						fastboot);
> > +	struct fastboot_header response_header;
> > +	uchar *packet;
> > +	uchar *packet_base;
> > +
> > +	if (fbn->reinit)
> > +		return 0;
> > +
> > +	while (fbn->may_send == MAY_NOT_SEND) {
> > +		poller_call();
> > +		if (fbn->reinit)
> > +			return 0;
> > +	}
> 
> This is a potentially endless loop I can reproducibly hit here. When I
> do a "sleep 10" on the command line and at the same time start a
> fastboot transfer then there's no progress. This is expected because
> fastboot depends on the idle slice. However, when the "sleep 10" is over
> then I get a prompt and then we are stuck in this loop.
> 
> I replaced this loop with a one second timeout loop. With this we are no
> longer stuck in the loop, but the current transfer doesn't continue
> anymore.

The official fastboot tool from Google has a 60 second timeout and
retransmits packets every 0.5 seconds. A 1 second timeout on the device
feels too short. We should also retry for at least 60 seconds.

I would expect that the host retransmits its last packet beyond the 10
second sleep and the transfer to immediately continue when barebox is
no longer busy. Also keep in mind that a device will most likely never
be used over fastboot and serial console at the same time. A user who
interactively started the transfer can always restart fastboot on the
host, which will abort this loop by setting reinit.

> Afterwards I dropped everything around fbn->may_send completely and now
> everything works fluently.
> 
> Removing these send restrictions seems the right thing to me. In the end
> it's UDP, so the remote host shouldn't make any assumptions of how fast
> and if at all packets arrive there.

I haven't seen your changes, but I fear you have broken something else.
Without may_send it may be possible for a message to be sent before the
previous message has been acked by the host. Since we can retransmit
only the last message, the connection will be stuck until the host times
out. In the protocol each side has to wait for reception of a packet
with the correct sequence number before it can send the next packet. We
can't change that just be cause we think it's the right thing.

Best regards,

  Daniel


-- 
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11,
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke
Ust-IdNr.: DE 205 198 055

emlix - your embedded linux partner

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox