Message ID | 1327020811-1538-23-git-send-email-joe.hershberger@ni.com |
---|---|
State | Superseded |
Delegated to: | Joe Hershberger |
Headers | show |
On Thursday 19 January 2012 19:53:25 Joe Hershberger wrote: > --- a/include/net.h > +++ b/include/net.h > > -extern int NetLoop(enum proto_t); > +extern int NetLoop(enum proto_t protocol); explicit variable names are unnecessary when the type is self explanatory > --- a/net/net.c > +++ b/net/net.c > > -static void NetInitLoop(enum proto_t protocol) > +static void NetInitLoop(void) this could prob do with a sep patch ... > +void > +NetInit(void) > +{ > + static int first_call = 1; > + > + if (first_call) { > + /* > + * Setup packet buffers, aligned correctly. > + */ > + int i; > + NetTxPacket = &PktBuf[0] + (PKTALIGN - 1); > + NetTxPacket -= (ulong)NetTxPacket % PKTALIGN; > + for (i = 0; i < PKTBUFSRX; i++) > + NetRxPackets[i] = NetTxPacket + (i+1)*PKTSIZE_ALIGN; > + > + ArpInit(); > + > + /* Only need to setup buffer pointers once. */ > + first_call = 0; > + } it *seems* like it should be fine, but i worry what might be lurking. guess we'll find out :P. > @@ -627,6 +639,12 @@ NetSendUDPPacket > > + /* make sure the NetTxPacket is initialized (NetInit() was called) */ > + if (NetTxPacket == NULL) { > + puts("*** ERROR: NetTxPacket buffer pointer is NULL!\n"); > + return -1; > + } why check here only (udp) ? and why check at all ? this should be an assert() in the comment netsendpacket entry point ... -mike
Hi Mike, On Fri, Feb 3, 2012 at 6:37 AM, Mike Frysinger <vapier@gentoo.org> wrote: > On Thursday 19 January 2012 19:53:25 Joe Hershberger wrote: >> @@ -627,6 +639,12 @@ NetSendUDPPacket >> >> + /* make sure the NetTxPacket is initialized (NetInit() was called) */ >> + if (NetTxPacket == NULL) { >> + puts("*** ERROR: NetTxPacket buffer pointer is NULL!\n"); >> + return -1; >> + } > > why check here only (udp) ? and why check at all ? this should be an > assert() in the comment netsendpacket entry point ... This is specifically needed here because the NetSendUDPPacket() function expects to use the NetTxPacket as a buffer and will write header information into it. The NetSendPacket() function takes the packet buffer itself as a parameter, so it isn't expecting anything as a side-band precondition to avoid crashing. I specifically added the check because the failure to initialize NetTxPacket in all code paths caused problems. It is easy to run into this if someone were to add a new thing that can send a packet. I will change it to an assert. -Joe
diff --git a/include/net.h b/include/net.h index af6a803..1861406 100644 --- a/include/net.h +++ b/include/net.h @@ -400,7 +400,8 @@ extern IPaddr_t Mcast_addr; #endif /* Initialize the network adapter */ -extern int NetLoop(enum proto_t); +extern void NetInit(void); +extern int NetLoop(enum proto_t protocol); /* Shutdown adapters and cleanup */ extern void NetStop(void); @@ -439,10 +440,10 @@ extern void NetSetState(int state); #define NETLOOP_SUCCESS 3 #define NETLOOP_FAIL 4 -/* Transmit "NetTxPacket" */ +/* Transmit a packet */ extern void NetSendPacket(uchar *, int); -/* Transmit UDP packet, performing ARP request if needed +/* Transmit "NetTxPacket" as UDP packet, performing ARP request if needed (ether will be populated) */ extern int NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int payload_len); diff --git a/net/net.c b/net/net.c index 1c0c822..51d899b 100644 --- a/net/net.c +++ b/net/net.c @@ -236,7 +236,7 @@ void net_auto_load(void) TftpStart(TFTPGET); } -static void NetInitLoop(enum proto_t protocol) +static void NetInitLoop(void) { static int env_changed_id; bd_t *bd = gd->bd; @@ -269,6 +269,30 @@ NetCleanupLoop(void) NetSetTimeout(0, NULL); } +void +NetInit(void) +{ + static int first_call = 1; + + if (first_call) { + /* + * Setup packet buffers, aligned correctly. + */ + int i; + NetTxPacket = &PktBuf[0] + (PKTALIGN - 1); + NetTxPacket -= (ulong)NetTxPacket % PKTALIGN; + for (i = 0; i < PKTBUFSRX; i++) + NetRxPackets[i] = NetTxPacket + (i+1)*PKTSIZE_ALIGN; + + ArpInit(); + + /* Only need to setup buffer pointers once. */ + first_call = 0; + } + + NetInitLoop(); +} + /**********************************************************************/ /* * Main network processing loop. @@ -276,26 +300,14 @@ NetCleanupLoop(void) int NetLoop(enum proto_t protocol) { - int i; bd_t *bd = gd->bd; int ret = -1; NetRestarted = 0; NetDevExists = 0; - - NetTxPacket = NULL; NetTryCount = 1; - ArpInit(); - - /* - * Setup packet buffers, aligned correctly. - */ - NetTxPacket = &PktBuf[0] + (PKTALIGN - 1); - NetTxPacket -= (ulong)NetTxPacket % PKTALIGN; - for (i = 0; i < PKTBUFSRX; i++) - NetRxPackets[i] = NetTxPacket + (i+1)*PKTSIZE_ALIGN; - + NetInit(); eth_halt(); eth_set_current(); if (eth_init(bd) < 0) { @@ -313,7 +325,7 @@ restart: * here on, this code is a state machine driven by received * packets and timer events. */ - NetInitLoop(protocol); + NetInitLoop(); switch (net_check_prereq(protocol)) { case 1: @@ -627,6 +639,12 @@ NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, { uchar *pkt; + /* make sure the NetTxPacket is initialized (NetInit() was called) */ + if (NetTxPacket == NULL) { + puts("*** ERROR: NetTxPacket buffer pointer is NULL!\n"); + return -1; + } + /* convert to new style broadcast */ if (dest == 0) dest = 0xFFFFFFFF;
A new non-static function NetInit() will initialize buffers and read from the env Only update from the env on each entry to NetLoop() Check when attempting to send a packet that the buffers were initialized Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> Cc: Joe Hershberger <joe.hershberger@gmail.com> Cc: Wolfgang Denk <wd@denx.de> --- include/net.h | 7 ++++--- net/net.c | 48 +++++++++++++++++++++++++++++++++--------------- 2 files changed, 37 insertions(+), 18 deletions(-)