(Added Ben to CC)
On Wednesday 11 February 2009, Wolfgang Denk wrote:
- cpu_eth_init(bis);
- pci_eth_init(bis);
- /*
* Return 0 so that cpu_eth_init() won't get executed again*/- return 0;
What happens in case of errors? This looks broken to me, or I misinderstand the comment.
This is the code calling board_eth_init() from net/eth.c:
/* Try board-specific initialization first. If it fails or isn't * present, try the cpu-specific initialization */ if (board_eth_init(bis) < 0) cpu_eth_init(bis);So if we return with an error in board_eth_init(), cpu_eth_init() will get called again. Another way to fix this problem would be this implementation:
I consider this a buggy design that should be fixed. It should be possible to handle the situation that pci_eth_init() returns an error code.
pci_eth_init() is called to add additional *optional* network interfaces. Since PCI boards may or may not exist, I think that a non existant PCI ethernet device should not result in an error.
What sort of error handling do you have in mind here?
board_eth_init() { pci_eth_init(bis);
/* * Return -1 so that cpu_eth_init() will get executed in net/eth.c */ return -1; }
But I like the former implementation better, since the cpu internal ethernet interfaces are added first to the network devices list.
That would be as bad as the previous solution, or actually worse as it looks as if board_eth_init() was always failing.
Right. That's also one reason why I implemented the first version.
I think the key problems is here:
/* Try board-specific initialization first. If it fails or isn't * present, try the cpu-specific initialization */ if (board_eth_init(bis) < 0) cpu_eth_init(bis);I think we must differentiate between board_eth_init() not existing and a failure in board_eth_init(); these are two very different situations.
board_eth_init() not existing is the default. We have a weak implementation for board_eth_init() in eth.c:
/* * CPU and board-specific Ethernet initializations. Aliased function * signals caller to move on */ static int __def_eth_init(bd_t *bis) { return -1; } int cpu_eth_init(bd_t *bis) __attribute((weak, alias("__def_eth_init"))); int board_eth_init(bd_t *bis) __attribute((weak, alias("__def_eth_init")));
What change do you have in mind here?
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================