[U-Boot] [PATCH 1/3] Add support for Net Boot Controller (NBC) packet

[Mike Frysinger]

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 <tristan.lelong at blunderer.org>
> + *
> + */

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