[PATCH 07/21] Revert "net: wget: Support retransmission a dropped packet"
Simon Glass
sjg at chromium.org
Tue Aug 13 17:12:12 CEST 2024
Hi Yasuharu,
On Tue, 13 Aug 2024 at 06:20, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Yasuharu,
>
> On Mon, 12 Aug 2024 at 21:50, Yasuharu Shibata
> <yasuharu.shibata at gmail.com> wrote:
> >
> > Dear Simon and Tom,
> >
> > Thank you for your details of the test and your advice.
> > The test has a problem that tcp_ack isn't calculated by the received data size.
> > sb_ack_handler() in test/cmd/wget.c, tcp_send->tcp_ack calculated by
> > following code:
> >
> > tcp_send->tcp_seq = htonl(ntohl(tcp->tcp_ack));
> > tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) + 1);
> >
> > tcp_ack needs to be calculated by the received TCP payload size.
> >
> > Also, wget command may have a problem that
> > HTTP response from server must be divided into more than two packets.
> > wget command receive packets and change internal state in
> > wget_handler() as follows:
> >
> > 1. When current_wget_state is WGET_CONNECTED,
> > wget_handler() calls wget_connected().
> > It receives HTTP response and moves WGET_TRANSFERRING.
> > 2. When current_wget_state is WGET_TRANSFERRING
> > and wget_tcp_state is TCP_ESTABLISHED,
> > wget_handler() receives HTTP response and
> > wget_loop_state sets NETLOOP_SUCCESS.
> > 3. When current_wget_state is WGET_TRANSFERRING
> > and wget_tcp_state is TCP_CLOSE_WAIT,
> > call net_set_state with wget_loop_state.
> >
> > If there are more than two HTTP response packets,
> > wget executes in the order 1 -> 2 -> 3 and terminates properly.
> > If there is only one HTTP response packet, wget executes in the order 1 -> 3,
> > wget_loop_state remains NETLOOP_CONTINUE and wget doesn't terminate
> > successfully.
> >
> > I wrote the patch at the end of this mail.
> > The wget issue is temporarily fixed on the test side.
> > This may need to be fixed on the wget side in the future.
> > In addition, I fixed the HTTP response returned at the correct timing.
> > If the patch is OK, I will send format-patch.
>
> Thank you for looking at this and for all the info. Yes, the patch
> looks good to me.
Also, would you mind adding a patch to enable the command for sandbox,
so the test runs in CI?
Thanks,
Simon
>
> Regards,
> Simon
>
> >
> > Best regards,
> > Yasuharu Shibata
> >
> > —
> > diff --git a/test/cmd/wget.c b/test/cmd/wget.c
> > index 356a4dcd8f..32542cdfe2 100644
> > --- a/test/cmd/wget.c
> > +++ b/test/cmd/wget.c
> > @@ -26,6 +26,8 @@
> > #define SHIFT_TO_TCPHDRLEN_FIELD(x) ((x) << 4)
> > #define LEN_B_TO_DW(x) ((x) >> 2)
> >
> > +int net_set_ack_options(union tcp_build_pkt *b);
> > +
> > static int sb_arp_handler(struct udevice *dev, void *packet,
> > unsigned int len)
> > {
> > @@ -105,6 +107,10 @@ static int sb_ack_handler(struct udevice *dev,
> > void *packet,
> > const char *payload1 = "HTTP/1.1 200 OK\r\n"
> > "Content-Length: 30\r\n\r\n\r\n"
> > "<html><body>Hi</body></html>\r\n";
> > + union tcp_build_pkt *b = (union tcp_build_pkt *)tcp;
> > + const int recv_payload_len = len - net_set_ack_options(b) -
> > IP_HDR_SIZE - ETHER_HDR_SIZE;
> > + static int next_seq = 0;
> > + const int bottom_payload_len = 10;
> >
> > /* Don't allow the buffer to overrun */
> > if (priv->recv_packets >= PKTBUFSRX)
> > @@ -119,13 +125,31 @@ static int sb_ack_handler(struct udevice *dev,
> > void *packet,
> > tcp_send->tcp_dst = tcp->tcp_src;
> > data = (void *)tcp_send + IP_TCP_HDR_SIZE;
> >
> > - if (ntohl(tcp->tcp_seq) == 1 && ntohl(tcp->tcp_ack) == 1) {
> > + if (ntohl(tcp->tcp_seq) == 1 && ntohl(tcp->tcp_ack) == 1 &&
> > recv_payload_len == 0) {
> > + // ignore ACK for three-way handshaking
> > + return 0;
> > + } else if (ntohl(tcp->tcp_seq) == 1 && ntohl(tcp->tcp_ack) == 1) {
> > + // recv HTTP request message and reply top half data
> > tcp_send->tcp_seq = htonl(ntohl(tcp->tcp_ack));
> > - tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) + 1);
> > - payload_len = strlen(payload1);
> > + tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) +
> > recv_payload_len);
> > +
> > + payload_len = strlen(payload1) - bottom_payload_len;
> > memcpy(data, payload1, payload_len);
> > tcp_send->tcp_flags = TCP_ACK;
> > - } else if (ntohl(tcp->tcp_seq) == 2) {
> > +
> > + next_seq = ntohl(tcp_send->tcp_seq) + payload_len;
> > + } else if (ntohl(tcp->tcp_ack) == next_seq) {
> > + // reply bottom half data
> > + const int top_payload_len = strlen(payload1) -
> > bottom_payload_len;
> > +
> > + tcp_send->tcp_seq = htonl(next_seq);
> > + tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) +
> > recv_payload_len);
> > +
> > + payload_len = bottom_payload_len;
> > + memcpy(data, payload1 + top_payload_len, payload_len);
> > + tcp_send->tcp_flags = TCP_ACK;
> > + } else {
> > + // close connection
> > tcp_send->tcp_seq = htonl(ntohl(tcp->tcp_ack));
> > tcp_send->tcp_ack = htonl(ntohl(tcp->tcp_seq) + 1);
> > payload_len = 0;
> > @@ -148,11 +172,9 @@ static int sb_ack_handler(struct udevice *dev,
> > void *packet,
> > pkt_len,
> > IPPROTO_TCP);
> >
> > - if (ntohl(tcp->tcp_seq) == 1 || ntohl(tcp->tcp_seq) == 2) {
> > - priv->recv_packet_length[priv->recv_packets] =
> > - ETHER_HDR_SIZE + IP_TCP_HDR_SIZE + payload_len;
> > - ++priv->recv_packets;
> > - }
> > + priv->recv_packet_length[priv->recv_packets] =
> > + ETHER_HDR_SIZE + IP_TCP_HDR_SIZE + payload_len;
> > + ++priv->recv_packets;
> >
> > return 0;
> >
> >
> > }
> >
> > On Tue, 13 Aug 2024 at 03:46, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Sun, Aug 11, 2024 at 08:50:18AM -0600, Simon Glass wrote:
> > > > +Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
> > > >
> > > > Hi Yasuharu,
> > > >
> > > > On Sat, 10 Aug 2024 at 20:48, Yasuharu Shibata
> > > > <yasuharu.shibata at gmail.com> wrote:
> > > > >
> > > > > Dear Simon,
> > > > >
> > > > > Could you inform me how the wget test was broken?
> > > > > As I explained in the commit log, I fixed the bug in specific conditions.
> > > > > Without the details of how the break happened,
> > > > > it is difficult to find a proper patch.
> > > > >
> > > > > Best regards,
> > > > > Yasuharu Shibata
> > > >
> > > > Yes, I should have mentioned that. It is test/cmd/wget.c - see also [1].
> > > >
> > > > This is a good run:
> > > > Test: net_test_wget: wget.c
> > > > HTTP/1.1 200 OK
> > > > Packets received 5, Transfer Successful
> > > > Bytes transferred = 32 (20 hex)
> > > > md5 for 00020000 ... 0002001f ==> 234af48e94b0085060249ecb5942ab57
> > > > Failures: 0
> > > >
> > > > With your patch it hangs:
> > > >
> > > > Test: net_test_wget: wget.c
> > > > HTTP/1.1 200 OKT
> > >
> > > Can you please figure out what's going on with the sandbox test then?
> > > The change in question fixes wget support on real hardware, and also the
> > > other wget tests (as part of the lwIP series) work fine as well. This
> > > seems like something specific to that test.
> > >
> > > --
> > > Tom
More information about the U-Boot
mailing list