| Message ID | 20180705021341.8881-2-peter.trantiendat@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Joe Hershberger |
| Headers | show |
| Series | net: Store waiting packet in a different buffer when making ARP requests | expand |
On Wed, Jul 4, 2018 at 9:13 PM, Tran Tien Dat <peter.trantiendat@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.trantiendat@gmail.com> Seems good, thanks! Acked-by: Joe Hershberger <joe.hershberger@ni.com> > --- > > 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
On Mon, Jul 9, 2018 at 11:48 AM, Joe Hershberger <joe.hershberger@ni.com> wrote: > On Wed, Jul 4, 2018 at 9:13 PM, Tran Tien Dat > <peter.trantiendat@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.trantiendat@gmail.com> > > Seems good, thanks! > > Acked-by: Joe Hershberger <joe.hershberger@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.hershberger@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
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;
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.trantiendat@gmail.com> --- 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(-)