[U-Boot] [PATCH v2 1/1] net: Store waiting packet in a different buffer when making ARP requests
Joe Hershberger
joe.hershberger at ni.com
Tue Jul 17 21:47:14 UTC 2018
On Mon, Jul 9, 2018 at 11:48 AM, Joe Hershberger <joe.hershberger at ni.com> wrote:
> On Wed, Jul 4, 2018 at 9:13 PM, Tran Tien Dat
> <peter.trantiendat at 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 at gmail.com>
>
> Seems good, thanks!
>
> Acked-by: Joe Hershberger <joe.hershberger at 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 at 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 at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list