Re: [PATCH 15/19] fastboot net: implement fastboot over UDP

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, Jun 18, 2020 at 08:33:03PM +0200, Daniel Glöckner wrote:
> Hello Sascha,
> 
> Am 18.06.20 um 13:59 schrieb Sascha Hauer:
> > On Wed, Jun 17, 2020 at 09:32:45PM +0200, Daniel Glöckner wrote:
> >>> +static void fastboot_start_download_net(struct fastboot *fb)
> >>> +{
> >>> +	struct fastboot_net *fbn = container_of(fb, struct fastboot_net,
> >>> +						fastboot);
> >>> +
> >>> +	fastboot_start_download_generic(fb);
> >>> +	fbn->active_download = true;
> >>> +}
> >>
> >> You didn't base v4 on v3, did you?
> > 
> > No, you sent v3 while I had different changes in my version already.
> > 
> >> If FASTBOOT_INIT is received before active_download is set to true, nobody
> >> will close fb->download_fd.
> > 
> > Ok, I'll add a fastboot_abort() function like you did in v3.
> 
> What I was referring to are the three additional lines in v3 that call
> fastboot_download_finished here instead of setting active_download to true
> when reinit is already true.
> 
> >>> +static void fastboot_send_keep_alive(struct poller_struct *poller)
> >>> +{
> >>> +	struct fastboot_net *fbn = container_of(poller, struct fastboot_net,
> >>> +					       keep_alive_poller);
> >>> +
> >>> +	if (!fbn->send_keep_alive)
> >>> +		return;
> >>> +
> >>> +	if (!is_timeout_non_interruptible(fbn->host_waits_since,
> >>> +					 30ULL * NSEC_PER_SEC))
> >>> +		return;
> >>> +
> >>> +	if (!fbn->may_send)
> >>> +		return;
> >>> +
> >>> +	fastboot_tx_print(&fbn->fastboot, FASTBOOT_MSG_INFO, "still busy");
> >>> +
> >>> +	fbn->host_waits_since = get_time_ns();
> >>
> >> Why do we need this line?
> >> host_waits_since is assigned get_time_ns() when may_send is set to true.
> > 
> > It is needed to reinitialize the timeout when we sent the message once.
> > Without this we would send the "still busy" message every few ms after
> > we've sent it for the first time.
> 
> No, because sending a message sets may_send to false and it stays false
> until we update host_waits_since in fastboot_handle_type_fastboot.

You are right. I introduced this when I had a version that flooded the
network with keepalive packets after 30s. Apparently this no longer
happens, so I'll remove this line.

Regards,
 Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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