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.shib...@gmail.com/T/#u Best regards, Yasuharu Shibata On Wed, 14 Aug 2024 at 00:12, Simon Glass <s...@chromium.org> wrote: > > Hi Yasuharu, > > > On Tue, 13 Aug 2024 at 06:20, Simon Glass <s...@chromium.org> wrote: > > > > Hi Yasuharu, > > > > On Mon, 12 Aug 2024 at 21:50, Yasuharu Shibata > > <yasuharu.shib...@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 <tr...@konsulko.com> wrote: > > > > > > > > On Sun, Aug 11, 2024 at 08:50:18AM -0600, Simon Glass wrote: > > > > > +Ying-Chun Liu (PaulLiu) <paul....@linaro.org> > > > > > > > > > > Hi Yasuharu, > > > > > > > > > > On Sat, 10 Aug 2024 at 20:48, Yasuharu Shibata > > > > > <yasuharu.shib...@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
