[PATCH 07/21] Revert "net: wget: Support retransmission a dropped packet"

Simon Glass sjg at chromium.org
Tue Aug 13 14:20:15 CEST 2024


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.

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