From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Warren Date: Wed, 11 Feb 2009 06:53:55 -0800 Subject: [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards In-Reply-To: <20090211130259.5AFAD832E893@gemini.denx.de> References: <1234342325-28950-1-git-send-email-sr@denx.de> <20090211121930.7F9B4832E893@gemini.denx.de> <200902111352.04598.sr@denx.de> <20090211130259.5AFAD832E893@gemini.denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Stefan/Wolfgang, On Wed, Feb 11, 2009 at 5:02 AM, Wolfgang Denk wrote: > Dear Stefan, > > In message <200902111352.04598.sr@denx.de> you 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. > > > I think this is right, although the undocumented return code of 0 may not be (more on this later in this e-mail) > > > 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. > What handling should there be in the case of a device initialization error? Drivers currently printf something and don't register the device. What else is appropriate? > > > 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. > > 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. > Sorry this is confusing. The -1 return code currently means "does not exist", although there are a few individual drivers that return -1 on failure, and should be cleaned up. All of the PCI drivers return the number of interfaces found (i.e. return 0 if nothing is found or there's an error). While this flaunts UNIX convention, it has the advantage of letting us count the interfaces at initialization without stepping through the list. I was thinking of taking advantage of this to get rid of the CONFIG_NET_MULTI thing some time in the future. > > Best regards, > > Wolfgang Denk > regards, Ben