[PATCH 07/21] Revert "net: wget: Support retransmission a dropped packet"
Yasuharu Shibata
yasuharu.shibata at gmail.com
Wed Aug 14 15:05:00 CEST 2024
Dear Simon,
Thank you for checking my patch.
I sent patch series including enabling wget command for sandbox.
https://lore.kernel.org/u-boot/20240814124108.2885-1-yasuharu.shibata@gmail.com/T/#u
Best regards,
Yasuharu Shibata
On Wed, 14 Aug 2024 at 00:12, Simon Glass <sjg at chromium.org> wrote:
>
> 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