On Mon, Jul 9, 2018 at 11:48 AM, Joe Hershberger <joe.hershber...@ni.com> wrote: > On Wed, Jul 4, 2018 at 9:13 PM, Tran Tien Dat > <peter.trantien...@gmail.com> wrote: >> U-Boot has 1 common buffer to send Ethernet frames, pointed to by >> net_tx_packet. When sending to an IP address without knowing the MAC >> address, U-Boot makes an ARP request (using the arp_tx_packet buffer) to >> find out the MAC address of the IP addressr. When a matching ARP reply is >> received, U-Boot continues sending the frame stored in the net_tx_packet >> buffer. >> >> However, in the mean time, if U-Boot needs to send out any network packets >> (e.g. replying ping packets or ARP requests for its own IP address etc.), >> it will use the net_tx_packet buffer to prepare the new packet. Thus this >> buffer is no longer the original packet meant to be transmitted after the >> ARP reply. The original packet will be lost. >> >> U-Boot has another buffer, pointed to by arp_tx_packet which is used to >> prepare ARP requests. ARP requests use this buffer instead of the normal >> net_tx_packet in order to avoid modifying the waiting packet to be sent. >> However, this approach does not prevent other parts of the codes from >> modifying the waiting packet to be sent, as explained above. This patch >> repurposes the arp_tx_packet buffer to be used to store the waiting packet >> to be sent, and use the normal net_tx_packet buffer to send ARP request >> instead. >> >> Signed-off-by: Tran Tien Dat <peter.trantien...@gmail.com> > > Seems good, thanks! > > Acked-by: Joe Hershberger <joe.hershber...@ni.com>
Upon testing and looking into this further, I don't like it. It adds a copy and breaks the network stack. Most network tests fail after applying this patch. so... Nacked-by: Joe Hershberger <joe.hershber...@ni.com> I think the buffer to use should be selected at the point of an async reply instead of preparing for something that probably will not happen. Sorry, -Joe > >> --- >> >> Changes in v2: >> - Provide more detailed description >> >> net/arp.c | 18 ++++++++++-------- >> net/arp.h | 1 + >> net/net.c | 3 +++ >> 3 files changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/net/arp.c b/net/arp.c >> index b8a71684cd..f5e2c0b0cf 100644 >> --- a/net/arp.c >> +++ b/net/arp.c >> @@ -35,8 +35,8 @@ int arp_wait_tx_packet_size; >> ulong arp_wait_timer_start; >> int arp_wait_try; >> >> -static uchar *arp_tx_packet; /* THE ARP transmit packet */ >> -static uchar arp_tx_packet_buf[PKTSIZE_ALIGN + PKTALIGN]; >> +uchar *arp_wait_tx_packet; /* THE waiting transmit packet after ARP */ >> +static uchar arp_wait_tx_packet_buf[PKTSIZE_ALIGN + PKTALIGN]; >> >> void arp_init(void) >> { >> @@ -45,8 +45,8 @@ void arp_init(void) >> net_arp_wait_packet_ip.s_addr = 0; >> net_arp_wait_reply_ip.s_addr = 0; >> arp_wait_tx_packet_size = 0; >> - arp_tx_packet = &arp_tx_packet_buf[0] + (PKTALIGN - 1); >> - arp_tx_packet -= (ulong)arp_tx_packet % PKTALIGN; >> + arp_wait_tx_packet = &arp_wait_tx_packet_buf[0] + (PKTALIGN - 1); >> + arp_wait_tx_packet -= (ulong)arp_wait_tx_packet % PKTALIGN; >> } >> >> void arp_raw_request(struct in_addr source_ip, const uchar *target_ethaddr, >> @@ -58,7 +58,7 @@ void arp_raw_request(struct in_addr source_ip, const uchar >> *target_ethaddr, >> >> debug_cond(DEBUG_DEV_PKT, "ARP broadcast %d\n", arp_wait_try); >> >> - pkt = arp_tx_packet; >> + pkt = net_tx_packet; >> >> eth_hdr_size = net_set_ether(pkt, net_bcast_ethaddr, PROT_ARP); >> pkt += eth_hdr_size; >> @@ -76,7 +76,7 @@ void arp_raw_request(struct in_addr source_ip, const uchar >> *target_ethaddr, >> memcpy(&arp->ar_tha, target_ethaddr, ARP_HLEN); /* target ET addr */ >> net_write_ip(&arp->ar_tpa, target_ip); /* target IP addr */ >> >> - net_send_packet(arp_tx_packet, eth_hdr_size + ARP_HDR_SIZE); >> + net_send_packet(net_tx_packet, eth_hdr_size + ARP_HDR_SIZE); >> } >> >> void arp_request(void) >> @@ -217,9 +217,11 @@ void arp_receive(struct ethernet_hdr *et, struct >> ip_udp_hdr *ip, int len) >> >> /* set the mac address in the waiting packet's header >> and transmit it */ >> - memcpy(((struct ethernet_hdr >> *)net_tx_packet)->et_dest, >> + memcpy(((struct ethernet_hdr *)arp_wait_tx_packet) >> + ->et_dest, >> &arp->ar_sha, ARP_HLEN); >> - net_send_packet(net_tx_packet, >> arp_wait_tx_packet_size); >> + net_send_packet(arp_wait_tx_packet, >> + arp_wait_tx_packet_size); >> >> /* no arp request pending now */ >> net_arp_wait_packet_ip.s_addr = 0; >> diff --git a/net/arp.h b/net/arp.h >> index afb86958f3..65d73927a7 100644 >> --- a/net/arp.h >> +++ b/net/arp.h >> @@ -20,6 +20,7 @@ extern uchar *arp_wait_packet_ethaddr; >> extern int arp_wait_tx_packet_size; >> extern ulong arp_wait_timer_start; >> extern int arp_wait_try; >> +extern uchar *arp_wait_tx_packet; >> >> void arp_init(void); >> void arp_request(void); >> diff --git a/net/net.c b/net/net.c >> index f35695b4fc..6325ad3e1a 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -836,6 +836,9 @@ int net_send_udp_packet(uchar *ether, struct in_addr >> dest, int dport, int sport, >> >> /* size of the waiting packet */ >> arp_wait_tx_packet_size = pkt_hdr_size + payload_len; >> + /* copy current packet to ARP waiting packet buffer */ >> + memcpy(arp_wait_tx_packet, net_tx_packet, >> + arp_wait_tx_packet_size); >> >> /* and do the ARP request */ >> arp_wait_try = 1; >> -- >> 2.18.0 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> https://lists.denx.de/listinfo/u-boot _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot