Hi Daniel,
On Mon, Jun 29, 2020 at 09:50:51PM +0200, Daniel Glöckner wrote:
> Hello Sascha,
>
> Am 19.06.20 um 09:44 schrieb Sascha Hauer:
> > +struct fastboot_net {
> > + struct fastboot fastboot;
> > +
> > + struct net_connection *net_con;
> > + struct fastboot_header response_header;
> > + struct poller_struct poller;
> > + struct work_queue wq;
> > + u64 host_waits_since;
> > + u64 last_download_pkt;
> > + bool sequence_number_seen;
> > + bool active_download;
> > + bool reinit;
> > + bool send_keep_alive;
> > + enum may_send may_send;
> > +
> > + IPaddr_t host_addr;
> > + u16 host_port;
> > + u8 host_mac[ETH_ALEN];
> > + u16 sequence_number;
> > + u16 last_payload_len;
> > + uchar last_payload[FASTBOOT_MAX_CMD_LEN + sizeof(struct fastboot_header)];
>
> This is not FASTBOOT_MAX_CMD_LEN. It's the 64 that is strewn around in
> fastboot_tx_print. Adding a new constant FASTBOOT_MAX_MSG_LEN would be
> correct.
There's no check that the packet copied into last_payload has this
maximum size. I switched to storing the packet in an allocated buffer,
with this the define is not needed anymore.
> > + net_write_ip(&fbn->net_con->ip->daddr, fbn->host_addr);
> > + fbn->net_con->udp->uh_dport = fbn->host_port;
> > + net_udp_send(fbn->net_con, fbn->last_payload_len);
> > +
> > + fbn->may_send = MAY_NOT_SEND;
>
> You moved that line below net_udp_send. Is there any risk that
>
> 1. our work queue executes a command which calls fastboot_tx_print
> 2. the net_udp_send caused by that fastboot_tx_print sleeps
> 3. our poller is executed and decides to send a message because
> may_send is still MAY_SEND_MESSAGE
Ok, changed that.
> > + fbn->last_download_pkt = get_time_ns();
> > +}
>
> Although you added that last_download_pkt timeout check to the poller,
> there is still the risk that we will never close download_fd if
> fastboot_net_abort is called (f.ex. by the first fastboot_tx_print
> inside cb_download) before we open download_fd. In that case there
> is no poller to check for the timeout.
I was not able to provoke such a behaviour. It seems that
fastboot_abort() is called often enough that this doesn't happen. In
doubt it happens when the next fastboot session is initiated.
> > + if (fastboot_data_len >= FASTBOOT_MAX_CMD_LEN) {
>
> Still off-by-one. Replace >= with >
Ok, fixed.
> > + ret = poller_register(&fbn->poller, "fastboot");
> > + if (ret) {
> > + pr_err("Cannot register poller: %s\n", strerror(-ret));
> > + return;
>
> It is not obvious that a second FASTBOOT_INIT will _not_ cause this
> error because fastboot_net_abort unregistered the previous poller.
> I would at least add a comment to the fastboot_net_abort(fbn) line.
Added a comment.
> > +{
> > + struct fastboot_net *fbn = container_of(poller, struct fastboot_net,
> > + poller);
> > +
> > + if (fbn->active_download && is_timeout(fbn->last_download_pkt, 5 * SECOND)) {
>
> Should pollers prefer is_timeout_non_interruptible over is_timeout?
Since with the current approach we no longer execute pollers inside of
pollers this shouldn't make a difference.
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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
