| Message ID | 1303143594-5345-6-git-send-email-luca.ceresoli@comelit.it |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Luca, > Signed-off-by: Luca Ceresoli <luca.ceresoli@comelit.it> > Cc: Wolfgang Denk <wd@denx.de> > --- > Changes in v2: none. Only one comment below > > net/tftp.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > net/tftp.h | 6 +++++ > 2 files changed, 74 insertions(+), 4 deletions(-) > > diff --git a/net/tftp.c b/net/tftp.c > index e166a63..87eb0b8 100644 > --- a/net/tftp.c > +++ b/net/tftp.c > @@ -2,6 +2,8 @@ > * Copyright 1994, 1995, 2000 Neil Russell. > * (See License) > * Copyright 2000, 2001 DENX Software Engineering, Wolfgang Denk, wd@denx.de > + * Copyright 2011 Comelit Group SpA, > + * Luca Ceresoli <luca.ceresoli@comelit.it> > */ > > #include <common.h> > @@ -74,6 +76,7 @@ static short TftpNumchars; /* The number of hashes we printed */ > #define STATE_TOO_LARGE 3 > #define STATE_BAD_MAGIC 4 > #define STATE_OACK 5 > +#define STATE_RECV_WRQ 6 > > #define TFTP_BLOCK_SIZE 512 /* default TFTP block size */ > #define TFTP_SEQUENCE_SIZE ((ulong)(1<<16)) /* sequence number is 16 bit */ > @@ -241,6 +244,10 @@ TftpSend (void) > TftpBlock=ext2_find_next_zero_bit(Bitmap,(Mapsize*8),0); > /*..falling..*/ > #endif > + > +#ifdef CONFIG_CMD_TFTPSRV > + case STATE_RECV_WRQ: > +#endif > case STATE_DATA: > xp = pkt; > s = (ushort *)pkt; > @@ -293,7 +300,11 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, > #endif > return; > } > - if (TftpState != STATE_SEND_RRQ && src != TftpRemotePort) > + if (TftpState != STATE_SEND_RRQ && > +#ifdef CONFIG_CMD_TFTPSRV > + TftpState != STATE_RECV_WRQ && > +#endif > + src != TftpRemotePort) > return; Hm, I have to admit that I do not really like adding so many ifdefs into the tftp code and even into the state machine code. Can you give me a number on how much larger u-boot gets on your platform with and without the server support? If this is not too much, maybe we should include support always. If it is too much, maybe we can at least keep the state machine without ifdefs - if I see it correctly, this will only add at maximum a few bytes and STATE_RECW_WRQ will simply never be entered, correct? Cheers Detlev
Detlev Zundel wrote: > Hi Luca, > >> Signed-off-by: Luca Ceresoli<luca.ceresoli@comelit.it> >> Cc: Wolfgang Denk<wd@denx.de> >> --- >> Changes in v2: none. > Only one comment below > >> net/tftp.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- >> net/tftp.h | 6 +++++ >> 2 files changed, 74 insertions(+), 4 deletions(-) >> >> diff --git a/net/tftp.c b/net/tftp.c >> index e166a63..87eb0b8 100644 >> --- a/net/tftp.c >> +++ b/net/tftp.c >> @@ -2,6 +2,8 @@ >> * Copyright 1994, 1995, 2000 Neil Russell. >> * (See License) >> * Copyright 2000, 2001 DENX Software Engineering, Wolfgang Denk, wd@denx.de >> + * Copyright 2011 Comelit Group SpA, >> + * Luca Ceresoli<luca.ceresoli@comelit.it> >> */ >> >> #include<common.h> >> @@ -74,6 +76,7 @@ static short TftpNumchars; /* The number of hashes we printed */ >> #define STATE_TOO_LARGE 3 >> #define STATE_BAD_MAGIC 4 >> #define STATE_OACK 5 >> +#define STATE_RECV_WRQ 6 >> >> #define TFTP_BLOCK_SIZE 512 /* default TFTP block size */ >> #define TFTP_SEQUENCE_SIZE ((ulong)(1<<16)) /* sequence number is 16 bit */ >> @@ -241,6 +244,10 @@ TftpSend (void) >> TftpBlock=ext2_find_next_zero_bit(Bitmap,(Mapsize*8),0); >> /*..falling..*/ >> #endif >> + >> +#ifdef CONFIG_CMD_TFTPSRV >> + case STATE_RECV_WRQ: >> +#endif >> case STATE_DATA: >> xp = pkt; >> s = (ushort *)pkt; >> @@ -293,7 +300,11 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, >> #endif >> return; >> } >> - if (TftpState != STATE_SEND_RRQ&& src != TftpRemotePort) >> + if (TftpState != STATE_SEND_RRQ&& >> +#ifdef CONFIG_CMD_TFTPSRV >> + TftpState != STATE_RECV_WRQ&& >> +#endif >> + src != TftpRemotePort) >> return; > Hm, I have to admit that I do not really like adding so many ifdefs into > the tftp code and even into the state machine code. Can you give me a > number on how much larger u-boot gets on your platform with and without > the server support? If this is not too much, maybe we should include > support always. If it is too much, maybe we can at least keep the state > machine without ifdefs - if I see it correctly, this will only add at > maximum a few bytes and STATE_RECW_WRQ will simply never be entered, > correct? Here are my measurements for the dig297 board (ARMV7 OMAP3): text data bss dec hex 357085 8684 214264 580033 8d9c1 TFTPSRV on 356327 8660 214264 579251 8d6b3 TFTPSRV off, without #ifs 356355 8660 214268 579283 8d6d3 TFTPSRV off, with #ifs (as in PATCH v2) The TFTP server adds 730 bytes to the text, so I guess it's worth to keep it optional. To my big surprise, removing most bad-looking #ifs (all of those that save just one line) I got a *smaller* U-Boot! I repeated the test three times to be extra sure, the figures are correct: without the #ifs we save a few bytes. Obvious enough, I'm going to remove the #ifs. Luca
Hi Luca, [...] >> Hm, I have to admit that I do not really like adding so many ifdefs into >> the tftp code and even into the state machine code. Can you give me a >> number on how much larger u-boot gets on your platform with and without >> the server support? If this is not too much, maybe we should include >> support always. If it is too much, maybe we can at least keep the state >> machine without ifdefs - if I see it correctly, this will only add at >> maximum a few bytes and STATE_RECW_WRQ will simply never be entered, >> correct? > Here are my measurements for the dig297 board (ARMV7 OMAP3): > > text data bss dec hex > 357085 8684 214264 580033 8d9c1 TFTPSRV on > 356327 8660 214264 579251 8d6b3 TFTPSRV off, without #ifs > 356355 8660 214268 579283 8d6d3 TFTPSRV off, with #ifs (as in PATCH v2) > > The TFTP server adds 730 bytes to the text, so I guess it's worth to > keep it optional. Ok, thanks for the number. > To my big surprise, removing most bad-looking #ifs (all of those that > save just one line) I got a *smaller* U-Boot! I repeated the test > three times to be extra sure, the figures are correct: without the > #ifs we save a few bytes. > > Obvious enough, I'm going to remove the #ifs. :) Another instance of the difficulty to predict modern compilers. Cheers Detlev
diff --git a/net/tftp.c b/net/tftp.c index e166a63..87eb0b8 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -2,6 +2,8 @@ * Copyright 1994, 1995, 2000 Neil Russell. * (See License) * Copyright 2000, 2001 DENX Software Engineering, Wolfgang Denk, wd@denx.de + * Copyright 2011 Comelit Group SpA, + * Luca Ceresoli <luca.ceresoli@comelit.it> */ #include <common.h> @@ -74,6 +76,7 @@ static short TftpNumchars; /* The number of hashes we printed */ #define STATE_TOO_LARGE 3 #define STATE_BAD_MAGIC 4 #define STATE_OACK 5 +#define STATE_RECV_WRQ 6 #define TFTP_BLOCK_SIZE 512 /* default TFTP block size */ #define TFTP_SEQUENCE_SIZE ((ulong)(1<<16)) /* sequence number is 16 bit */ @@ -241,6 +244,10 @@ TftpSend (void) TftpBlock=ext2_find_next_zero_bit(Bitmap,(Mapsize*8),0); /*..falling..*/ #endif + +#ifdef CONFIG_CMD_TFTPSRV + case STATE_RECV_WRQ: +#endif case STATE_DATA: xp = pkt; s = (ushort *)pkt; @@ -293,7 +300,11 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, #endif return; } - if (TftpState != STATE_SEND_RRQ && src != TftpRemotePort) + if (TftpState != STATE_SEND_RRQ && +#ifdef CONFIG_CMD_TFTPSRV + TftpState != STATE_RECV_WRQ && +#endif + src != TftpRemotePort) return; if (len < 2) { @@ -307,12 +318,24 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, switch (ntohs(proto)) { case TFTP_RRQ: - case TFTP_WRQ: case TFTP_ACK: break; default: break; +#ifdef CONFIG_CMD_TFTPSRV + case TFTP_WRQ: + debug("Got WRQ\n"); + TftpRemoteIP = sip; + TftpRemotePort = src; + TftpOurPort = 1024 + (get_timer(0) % 3072); + TftpLastBlock = 0; + TftpBlockWrap = 0; + TftpBlockWrapOffset = 0; + TftpSend(); /* Send ACK(0) */ + break; +#endif + case TFTP_OACK: debug("Got OACK: %s %s\n", pkt, @@ -383,7 +406,11 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, if (TftpState == STATE_SEND_RRQ) debug("Server did not acknowledge timeout option!\n"); - if (TftpState == STATE_SEND_RRQ || TftpState == STATE_OACK) { + if (TftpState == STATE_SEND_RRQ || +#ifdef CONFIG_CMD_TFTPSRV + TftpState == STATE_RECV_WRQ || +#endif + TftpState == STATE_OACK) { /* first block received */ TftpState = STATE_DATA; TftpRemotePort = src; @@ -518,7 +545,10 @@ TftpTimeout (void) } else { puts ("T "); NetSetTimeout (TftpTimeoutMSecs, TftpTimeout); - TftpSend (); +#ifdef CONFIG_CMD_TFTPSRV + if (TftpState != STATE_RECV_WRQ) +#endif + TftpSend(); } } @@ -639,6 +669,40 @@ TftpStart (void) TftpSend (); } +#ifdef CONFIG_CMD_TFTPSRV +void +TftpStartServer(void) +{ + tftp_filename[0] = 0; + +#if defined(CONFIG_NET_MULTI) + printf("Using %s device\n", eth_get_name()); +#endif + printf("Listening for TFTP transfer on %pI4\n", &NetOurIP); + printf("Load address: 0x%lx\n", load_addr); + + puts("Loading: *\b"); + + TftpTimeoutCountMax = TIMEOUT_COUNT; + TftpTimeoutCount = 0; + TftpTimeoutMSecs = TIMEOUT; + NetSetTimeout(TftpTimeoutMSecs, TftpTimeout); + + /* Revert TftpBlkSize to dflt */ + TftpBlkSize = TFTP_BLOCK_SIZE; + TftpBlock = 0; + TftpOurPort = WELL_KNOWN_PORT; + +#ifdef CONFIG_TFTP_TSIZE + TftpTsize = 0; + TftpNumchars = 0; +#endif + + TftpState = STATE_RECV_WRQ; + NetSetHandler(TftpHandler); +} +#endif /* CONFIG_CMD_TFTPSRV */ + #ifdef CONFIG_MCAST_TFTP /* Credits: atftp project. */ diff --git a/net/tftp.h b/net/tftp.h index e3dfb26..3abdf7b 100644 --- a/net/tftp.h +++ b/net/tftp.h @@ -2,6 +2,8 @@ * LiMon - BOOTP/TFTP. * * Copyright 1994, 1995, 2000 Neil Russell. + * Copyright 2011 Comelit Group SpA + * Luca Ceresoli <luca.ceresoli@comelit.it> * (See License) */ @@ -16,6 +18,10 @@ /* tftp.c */ extern void TftpStart (void); /* Begin TFTP get */ +#ifdef CONFIG_CMD_TFTPSRV +extern void TftpStartServer(void); /* Wait for incoming TFTP put */ +#endif + /**********************************************************************/ #endif /* __TFTP_H__ */
Signed-off-by: Luca Ceresoli <luca.ceresoli@comelit.it> Cc: Wolfgang Denk <wd@denx.de> --- Changes in v2: none. net/tftp.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- net/tftp.h | 6 +++++ 2 files changed, 74 insertions(+), 4 deletions(-)