[U-Boot] [PATCH] ne2000: Fix broken build of three boards after CONFIG_NET_MULTI drop

[Mike Frysinger]

On Monday 17 October 2011 18:05:28 Bernhard Kaindl wrote:
> Lets really turn on -Werror to once for all put an end to such things.
> Lazy maintainers can aways put a CFLAGS += Wno-error into their
> makefiles if they can't fix warnings in a given directory.
> At the very least, -Werror-implicit-function-declaration should be used.
> Both flags are supported by gcc-2.95.3 or older gcc.gnu.org says.

i don't think -Werror is feasible.  there are way too many versions of gcc out 
there that we try to support from too many different vendors and too many 
architectures.  warnings tend to come and go in between versions, and -Werror 
is just hell.

personally, i don't mind it to track local development of the latest tree, but 
it isn't something i'd inflict in general.

-Werror-implicit-function-declaration however sounds wonderful.  and we don't 
have to worry about gcc versions as we have a cc-option helper:
	CFLAGS += $(call cc-option,-Werror-implicit-function-declaration)

care to send a patch ?

> >> --- a/drivers/net/ne2000_base.c
> >> +++ b/drivers/net/ne2000_base.c
> >> 
> >> +int ne2000_initialize(void)
> >> +{
> >> +	struct eth_device *dev;
> >> +
> >> +	nic.base = (u8 *) CONFIG_DRIVER_NE2000_BASE;
> >> +	nic.data = nic.base + DP_DATA;
> >> +	nic.tx_buf1 = START_PG;
> >> +	nic.tx_buf2 = START_PG2;
> >> +	nic.rx_buf_start = RX_START;
> >> +	nic.rx_buf_end = RX_END;
> > 
> > this should be using dev->priv rather than a global "nic" data structure
> 
> Understand, but the 2/3 boards using it only have a single ne2k-based
> chip, and as no one actually uses the driver multi-card, I skipped
> these larger changes to make it multi-card and use dev->priv everywhere.

i'll let that slide since i broke the build ;).  but i'd really like to see 
this fixed long term.

> >> +	dev = calloc(sizeof(*dev), 1);
> >> +	pbuf = malloc(NE2000_RX_BUFFER_SIZE);
> >> +	if (dev == NULL || pbuf == NULL)
> >> +		return -1;
> > 
> > if dev worked but pbuf failed, this leaks memory
> 
> If malloc fails here, either CONFIG_SYS_MALLOC_LEN is to small or malloc
> is broken. Three Ethernet drivers call hang() in this case, which is
> probably best in explaining to the developer who broke it.
> I can replace that with "goto error" and give an error message on it
> like the other recently merged drivers do in this init situation.

i know there's inconsistency here with some drivers, but the current "best 
practices" is to not leak and return 0.

>  > also, you should return 0 here not -1
> 
> The recently merged drivers (armada100 - heavvy reviewed and you and
> many others, the xilinx_axi_emac(acked by you) and many more return -1
> on malloc error.

i'm not perfect and sometimes i miss things :)

> >> +	if (!get_prom(dev->enetaddr, nic.base))
> >> +		return -1;
> >> +
> >> +	dp83902a_init(dev);
> > 
> > these should probably be in the eth->init step and not here
> 
> At first sight, I thought the same and did it the same but then you get
> 
> Net:   NE2000
> Warning: failed to set MAC address

OK, i looked closer at the code, and it does seem to only look up the MAC 
address, so this is correct to do in the ne2000_initialize() function.  when i 
saw the diffstat, i thought the init was refactored to do more, not less.

> >> +	eth_setenv_enetaddr("ethaddr", dev->enetaddr);
> > 
> > NAK: implement eth->write_hwaddr, and the driver should only use
> > dev->enetaddr rather than touching the env
> 
> Wrong NAK: If ethaddr is not set in env, this "return -1" below triggers
> and no call to eth->write_hwaddr can be reached (below the return -1):

a bug in a diff layer doesn't mean you can add hacks to the driver.  if the 
hardware supports changing the MAC at runtime by programming the registers, 
then implement write_hwaddr.  if it doesn't, and you think the current eth 
behavior undesirable, then start a new thread.
-mike