Hi Josh & Frank, A few nits. Overall a nice patch.
One question: Will multicast TFTP still work when this is applied? Josh Boyer wrote: > From: Frank Haverkamp <[EMAIL PROTECTED]> > > http://tools.ietf.org/html/rfc2348 describes the TFTP block size option > which allows larger packtes than the 512 byte default. This reduces the > s/packtes/packets/ > number of TFTP ACKs significantly and improves performance. > > To get the most benefit out of the tftp block size option the support > of defragementation of IP/UDP packet is helpful. The current implemenation > s/defragementation/defragmentation/ s/implemenation/implementation/ > should work even with packets received out of order. To enable the large > packet size the user should set "tftp_block_size" so a value like 16352. > > s/so/to/ > We experimented with different packet sizes and found that more than those > 16KiB do not contribute much to the performance anymore. Therefor I limited > s/Therefor/Therefore/ > the defragmentation buffer to 16KiB no too waste memory. > > so as to not waste memory > Signed-off-by: Frank Haverkamp <[EMAIL PROTECTED]> > Signed-off-by: Josh Boyer <[EMAIL PROTECTED]> > > --- > include/net.h | 17 ++++++ > net/net.c | 156 > ++++++++++++++++++++++++++++++++++++++++++++++++++-------- > net/tftp.c | 22 ++++++++ > net/tftp.h | 10 +++ > 4 files changed, 185 insertions(+), 20 deletions(-) > > --- u-boot.git.orig/include/net.h > +++ u-boot.git/include/net.h > @@ -200,6 +200,13 @@ typedef struct { > ushort udp_xsum; /* Checksum */ > } IP_t; > > +#define IP_OFFS 0x1FFF /* ip offset *= 8 */ > +#define IP_OFFS_SHIFT 3 /* in 8 byte steps */ > +#define IP_FLAGS 0xE000 /* first 3 bits */ > +#define IP_FLAGS_RES 0x8000 /* reserved */ > +#define IP_FLAGS_DFRAG 0x4000 /* don't fragments */ > +#define IP_FLAGS_MFRAG 0x2000 /* more fragments */ > + > Please align these better. After applying, it's: #define IP_OFFS 0x1FFF /* ip offset *= 8 */ #define IP_OFFS_SHIFT 3 /* in 8 byte steps */ #define IP_FLAGS 0xE000 /* first 3 bits */ #define IP_FLAGS_RES 0x8000 /* reserved */ #define IP_FLAGS_DFRAG 0x4000 /* don't fragments */ #define IP_FLAGS_MFRAG 0x2000 /* more fragments */ > #define IP_HDR_SIZE_NO_UDP (sizeof (IP_t) - 8) > #define IP_HDR_SIZE (sizeof (IP_t)) > > @@ -282,6 +289,16 @@ typedef struct icmphdr { > #define PKTSIZE_ALIGN 1536 > /*#define PKTSIZE 608*/ > > + /* > + * IP/UDP Fragmentation support > + * See: http://en.wikipedia.org/wiki/IPv4#Fragmentation_and_reassembly > + * MAX possible UDP packet size is 64 KiB, if there is memory available. > + */ > +#define NET_ETH_MTU 1500 > +#define NET_FRAG_BUF_SIZE (16 * 1024) /* MAX is 64 KiB */ > +#define NET_UDP_FRAG_SIZE (NET_ETH_MTU - IP_HDR_SIZE_NO_UDP) /* 1480 */ > +#define NET_FRAG_BUF_USED (NET_FRAG_BUF_SIZE / NET_UDP_FRAG_SIZE + 1) > + > /* > * Maximum receive ring size; that is, the number of packets > * we can buffer before overflow happens. Basically, this just > --- u-boot.git.orig/net/net.c > +++ u-boot.git/net/net.c > @@ -192,6 +192,15 @@ volatile uchar PktBuf[(PKTBUFSRX+1) * PK > > volatile uchar *NetRxPackets[PKTBUFSRX]; /* Receive packets > */ > > +/* Packet fragmentation support */ > +static uint16_t ip_id = 0; /* sequence number */ > +static uint16_t udp_len = 0; > +static uint16_t udp_src = 0; > +static uint16_t udp_dst = 0; > +static int max_idx = 0; > +static uchar NetFragBuf[NET_FRAG_BUF_SIZE]; > +static char NetFragBufUsed[NET_FRAG_BUF_USED] = { 0, }; > + > static rxhand_f *packetHandler; /* Current RX packet handler > */ > static thand_f *timeHandler; /* Current timeout handler > */ > static ulong timeStart; /* Time base value > */ > @@ -288,6 +297,13 @@ NetLoop(proto_t protocol) > { > bd_t *bd = gd->bd; > > + /* Packet fragmentation support */ > + ip_id = udp_len = udp_src = udp_dst = max_idx = 0; > + memset(NetFragBuf, 0xFF, sizeof(NetFragBuf)); > + memset(NetFragBufUsed, 0, sizeof(NetFragBufUsed)); > + printf("NetFragBuf @ %08x max tftp_block_size=%d udp_frag_size=%d\n", > + NetFragBuf, TFTP_BLOCK_SIZE_MAX, NET_UDP_FRAG_SIZE); > + > #ifdef CONFIG_NET_MULTI > NetRestarted = 0; > NetDevExists = 0; > @@ -1150,6 +1166,39 @@ static void CDPStart(void) > } > #endif > > +#ifdef CONFIG_UDP_CHECKSUM > +/* > + * @sumptr: Points to UDP data > + * @sumlen: Size of UDP data > + * @xsum: UDP checksum across IP source, destination address, protocol and > size > + * > + * Returns 0 when checksum is correct and 1 if it is not > Can you return -1 on failure? Not a big deal, just more conventional. > + */ > +static int udp_checksum(ushort *sumptr, ushort sumlen, ulong xsum) > +{ > + while (sumlen > 1) { > + ushort sumdata; > + > + sumdata = *sumptr++; > + xsum += ntohs(sumdata); > + sumlen -= 2; > + } > + if (sumlen > 0) { > + ushort sumdata; > + > + sumdata = *(unsigned char *) sumptr; > + sumdata = (sumdata << 8) & 0xff00; > + xsum += sumdata; > + } > + while ((xsum >> 16) != 0) { > + xsum = (xsum & 0x0000ffff) + ((xsum >> 16) & 0x0000ffff); > + } > + if ((xsum != 0x00000000) && (xsum != 0x0000ffff)) > + return 1; > + > + return 0; > +} > +#endif /* CONFIG_UDP_CHECKSUM */ > > void > NetReceive(volatile uchar * inpkt, int len) > @@ -1164,6 +1213,7 @@ NetReceive(volatile uchar * inpkt, int l > int iscdp; > #endif > ushort cti = 0, vlanid = VLAN_NONE, myvlanid, mynvlanid; > + uint32_t off; /* ip_off for fragmentation */ > Can you pick a better variable name than 'off'? 'offset' maybe? > > #ifdef ET_DEBUG > printf("packet received\n"); > @@ -1404,9 +1454,11 @@ NetReceive(volatile uchar * inpkt, int l > if ((ip->ip_hl_v & 0xf0) != 0x40) { > return; > } > +#if 0 /* Obsolete after adding the fragmentation support */ > if (ip->ip_off & htons(0x1fff)) { /* Can't deal w/ fragments */ > return; > } > +#endif > Please delete this block if it's now dead code. > /* can't deal with headers > 20 bytes */ > if ((ip->ip_hl_v & 0x0f) > 0x05) { > return; > @@ -1422,6 +1474,88 @@ NetReceive(volatile uchar * inpkt, int l > #endif > return; > } > + > + /* > + * Fragmentation support. We need to check the ip_id > + * and if all fragments were received correctly. > + */ > + off = (ntohs(ip->ip_off) & IP_OFFS) << IP_OFFS_SHIFT; > + if ((off != 0) || (ip->ip_off & htons(IP_FLAGS_MFRAG))) { > + int size, idx, complete; > + char *start; > + > + /* New fragmented packet arrived, clear data. */ > + if (ntohs(ip->ip_id) != ip_id) { > + ip_id = ntohs(ip->ip_id); > + memset(NetFragBufUsed, 0, > sizeof(NetFragBufUsed)); > + udp_len = udp_src = udp_dst = max_idx = 0; > + } > + > + idx = off / NET_UDP_FRAG_SIZE; > + > + /* Packet does not fit into IP/UDP fragmentation buf */ > + if (idx >= NET_FRAG_BUF_USED) { > + return; > + } > + > + NetFragBufUsed[idx] = 1; > + > + /* Copy the UDP hdr with the data for 1st > + fragment, else copy just payload */ > + if (off == 0) { > + udp_len = ntohs(ip->udp_len); > + udp_src = ntohs(ip->udp_src); > + udp_dst = ntohs(ip->udp_dst); > + } > + size = ntohs(ip->ip_len) - IP_HDR_SIZE_NO_UDP; > + start = (char *)ip + IP_HDR_SIZE_NO_UDP; > + memcpy(NetFragBuf + off, start, size); > + > + /* > + * When last fragement has been received we > s/fragement/fragment/ > + * know the number of fragments we expect. If > + * all have arrived we process the packet. > + */ > + if (((off != 0) && !(ip->ip_off & > htons(IP_FLAGS_MFRAG)))) > + max_idx = idx; > + > + if (max_idx == 0) > + return; > + > + complete = 1; > + for (idx = 0; idx < max_idx; idx++) { > + if (NetFragBufUsed[idx] == 0) { > + complete = 0; > + break; > + } > + } > + if (!complete) > + return; > +#ifdef CONFIG_UDP_CHECKSUM > + if (ip->udp_xsum != 0) { > + ulong xsum = ip->ip_p; > + uint16_t *sumptr; > + > + xsum += udp_len; > + xsum += (ntohl(ip->ip_src) >> 16) & 0xffff; > + xsum += (ntohl(ip->ip_src) >> 0) & 0xffff; > + xsum += (ntohl(ip->ip_dst) >> 16) & 0xffff; > + xsum += (ntohl(ip->ip_dst) >> 0) & 0xffff; > + sumptr = (ushort *)NetFragBuf; > + > + if (udp_checksum(sumptr, udp_len, xsum)) { > + putc('U'); > + return; > + } > + } > +#endif /* CONFIG_UDP_CHECKSUM */ > + (*packetHandler)(NetFragBuf + 8, > + udp_dst, > + udp_src, > + udp_len - 8); > Can some of these arguments share lines? > + return; > + } > + > /* > * watch for ICMP host redirects > * > @@ -1502,26 +1636,8 @@ NetReceive(volatile uchar * inpkt, int l > sumlen = ntohs(ip->udp_len); > sumptr = (ushort *) &(ip->udp_src); > > - while (sumlen > 1) { > - ushort sumdata; > - > - sumdata = *sumptr++; > - xsum += ntohs(sumdata); > - sumlen -= 2; > - } > - if (sumlen > 0) { > - ushort sumdata; > - > - sumdata = *(unsigned char *) sumptr; > - sumdata = (sumdata << 8) & 0xff00; > - xsum += sumdata; > - } > - while ((xsum >> 16) != 0) { > - xsum = (xsum & 0x0000ffff) + ((xsum >> 16) & > 0x0000ffff); > - } > - if ((xsum != 0x00000000) && (xsum != 0x0000ffff)) { > - printf(" UDP wrong checksum %08lx %08x\n", > - xsum, ntohs(ip->udp_xsum)); > + if (udp_checksum(sumptr, sumlen, xsum)) { > + putc('U'); > return; > } > } > --- u-boot.git.orig/net/tftp.c > +++ u-boot.git/net/tftp.c > @@ -456,6 +456,7 @@ TftpTimeout (void) > void > TftpStart (void) > { > + char *s, *err; > #ifdef CONFIG_TFTP_PORT > char *ep; /* Environment pointer */ > #endif > @@ -518,6 +519,27 @@ TftpStart (void) > > puts ("Loading: *\b"); > > + /* Get alternate tftp_block_size */ > + if ((s = getenv("tftp_block_size")) != NULL) { > Kind of a long environment variable name. Maybe "tftp_bs"? > + err = NULL; > + > + TftpBlkSizeOption = simple_strtoul(s, &err, 10); > + if (*err) { > + printf("ERR: \"tftp_block_size\" is not a number\n"); > + TftpBlkSizeOption = TFTP_BLOCK_SIZE; > + } > + /* > + * Reject values which require extensive handling. > + * block size of 1428 octets (Ethernet MTU, less > + * the TFTP, UDP and IP header lengths). > + */ > + if (TftpBlkSizeOption > TFTP_BLOCK_SIZE_MAX) { > + printf("ERR: tftp_block_sizes larger than %d not " > + "supported\n", TFTP_BLOCK_SIZE_MAX); > + TftpBlkSizeOption = TFTP_BLOCK_SIZE; > + } > + } > + > NetSetTimeout (TIMEOUT * CFG_HZ, TftpTimeout); > NetSetHandler (TftpHandler); > > --- u-boot.git.orig/net/tftp.h > +++ u-boot.git/net/tftp.h > @@ -8,11 +8,21 @@ > #ifndef __TFTP_H__ > #define __TFTP_H__ > > +#include <net.h> > + > /**********************************************************************/ > /* > * Global functions and variables. > */ > > +/* > + * Maximum TFTP block size bound to max size of fragmented IP/UDP > + * packets minus TFTP and UDP/IP overhead. TFTP overhead is 2 byte > + * opcode and 2 byte block-number. > + */ > +#define TFTP_BLOCK_SIZE_MAX (NET_FRAG_BUF_SIZE - sizeof(IP_t) - 4) > + > + > /* tftp.c */ > extern void TftpStart (void); /* Begin TFTP get */ > > > regards, Ben _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
