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