From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Frysinger Date: Wed, 17 Nov 2010 05:54:48 -0500 Subject: [U-Boot] [PATCH 1/3] Add support for Net Boot Controller (NBC) packet In-Reply-To: <732b4e4395e5c02bb01c1fd7b7f8ce085a208950.1289929762.git.blunderer@blunderer.org> References: <732b4e4395e5c02bb01c1fd7b7f8ce085a208950.1289929762.git.blunderer@blunderer.org> Message-ID: <201011170554.49352.vapier@gentoo.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tuesday, November 16, 2010 13:04:31 tristan.lelong at blunderer.org wrote: > --- a/common/main.c > +++ b/common/main.c > > +#ifdef CONFIG_CMD_NBC why did you pick "CONFIG_CMD_NBC" ? there is no "nbc" command that is run from the u-boot console, so "CONFIG_CMD_xxx" makes no sense. > + if (!abort) > + abort = NBCWait (); incorrect style here and in many places -- no space before the paren. i wont bother pointing out this in other places ... find & fix yourself. > --- a/net/Makefile > +++ b/net/Makefile > @@ -35,6 +35,8 @@ COBJS-$(CONFIG_CMD_NFS) += nfs.o > COBJS-$(CONFIG_CMD_RARP) += rarp.o > COBJS-$(CONFIG_CMD_SNTP) += sntp.o > COBJS-$(CONFIG_CMD_NET) += tftp.o > +COBJS-$(CONFIG_CMD_NBC) += nbc.o > + > do not add useless blank lines > --- /dev/null > +++ b/net/nbc.c > @@ -0,0 +1,143 @@ > +/* > + * NBC support driver > + * Network Boot Controller > + * > + * Copyright (c) 2010 Tristan Lelong > + * > + */ licensing info is missing > +static void NBCHandler (uchar * pkt, unsigned dest, unsigned src, unsigned > len) +{ style problems here -- no space after that asterisk > + int cnt = 0; > + char src_ip[16] = ""; > + char *nbcsource = NULL; > + char ip[16] = ""; > + char mac[18] = ""; > + char hostname[255] = ""; > + > + struct _nbc_packet *nbc_pkt = (struct _nbc_packet *)pkt; > + nbcsource = getenv ("nbcsource"); > + > + /* check packet validity */ > + if (nbc_pkt->id != 0xD3) magic numbers should be defines in headers, not inlined in source files > + goto fail; > + if (strncmp (nbc_pkt->head, "NBC", 3)) > + goto fail; > + if (nbc_pkt->size != len) > + goto fail; > + > + sprintf (src_ip, "%lu.%lu.%lu.%lu", ((NetSrcIP >> 0) & 0xFF), > + ((NetSrcIP >> 8) & 0xFF), ((NetSrcIP >> 16) & 0xFF), > + ((NetSrcIP >> 24) & 0xFF)); we have printf modifiers to handle ip addresses already. use that instead. > + unsigned char *pkt_mac = (unsigned char *)chk->data; > + sprintf (mac, "%02X:%02X:%02X:%02X:%02X:%02X", > + pkt_mac[0], pkt_mac[1], pkt_mac[2], pkt_mac[3], > + pkt_mac[4], pkt_mac[5]); there are printf modifiers for MAC addresse too. although i dont think you need to go screwing with this in the first place. there are eth helpers for manipulating MAC addresses you should be using instead. see doc/README.enetaddr. > + if (NBC_Mode_On) { > + return 1; > + } useless braces > --- /dev/null > +++ b/net/nbc.h > @@ -0,0 +1,39 @@ > +#define NBC_SERVICE_PORT 4446 > +#define NBC_TIMEOUT 2000 > + > +#define NBC_CHK_HEADER_SIZE 5 > +#define NBC_HEADER_SIZE 5 > +#define NBC_CHK_IP_SIZE 4 > +#define NBC_CHK_MAC_SIZE 6 > +#define NBC_CHK_HOSTNAME_SIZE 255 indentation here is all screwd up. it's inconsistent and mixes spaces & tabs. > --- a/net/net.c > +++ b/net/net.c > @@ -1631,6 +1643,7 @@ NetReceive(volatile uchar * inpkt, int len) > ushort *sumptr; > ushort sumlen; > > + > xsum = ip->ip_p; > xsum += (ntohs(ip->udp_len)); > xsum += (ntohl(ip->ip_src) >> 16) & 0x0000ffff; please clean your patches of useless whitespace noise before submitting -mike -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: This is a digitally signed message part. Url : http://lists.denx.de/pipermail/u-boot/attachments/20101117/592ab88d/attachment.pgp