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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
