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

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

Hello Sascha,

Am 17.06.20 um 10:11 schrieb Sascha Hauer:

> +	ret = fastboot_net_wait_may_send(fbn);
> +	if (ret)
> +		return ret;

Where is the part that aborts the session on timeout?

> +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?
If FASTBOOT_INIT is received before active_download is set to true, nobody
will close fb->download_fd.

> +static void fastboot_handle_type_fastboot(struct fastboot_net *fbn,
> +					  struct fastboot_header header,
> +					  char *fastboot_data,
> +					  unsigned int fastboot_data_len)
> +{
> +	fbn->response_header = header;
> +	fbn->host_waits_since = get_time_ns();
> +	fbn->may_send = true;

No MAY_SEND_ACK and MAY_SEND_MESSAGE? So you choose to ignore protocol
violations?

> +	if (fbn->active_download) {
> +		if (!fastboot_data_len && fbn->fastboot.download_bytes
> +					   == fbn->fastboot.download_size) {
> +			fastboot_download_finished(&fbn->fastboot);

Now I see why you dropped that fastboot_tx_print in the previous patch.

> +		} else {
> +			fastboot_data_download(fbn, fastboot_data,
> +					       fastboot_data_len);
> +		}
> +	} else {

Fastboot does not allow to queue multiple commands. A command must have
finished before the host may send the next one. That's why there was
an "else if (!fbn->command[0])".

> +		if (fastboot_data_len >= FASTBOOT_MAX_CMD_LEN) {

Off-by-one error: if (fastboot_data_len > FASTBOOT_MAX_CMD_LEN) {

> +		} else if (fastboot_data_len) {
> +			struct fastboot_work *w;
> +
> +			w = xzalloc(sizeof(*w));

Why can't we embed struct fastboot_work in struct fastboot_net?
As I wrote above it is impossible to queue multiple commands.

> +			w->fbn = fbn;
> +			memcpy(w->command, fastboot_data, fastboot_data_len);
> +			w->command[fastboot_data_len] = 0;
> +
> +			wq_queue_work(&fbn->wq, &w->work);
> +		}
> +	}
> +
> +	if (!fbn->fastboot.active)
> +		fbn->active_download = false;

These two lines were gone in v3.

> +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.


All in all I am not sure I like your simplifications wrt. ack handling,
may_send, and sending multiple messages.

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