[U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs cpu_eth_init() on AMCC boards

[Ben Warren]

Hi Stefan/Wolfgang,

On Wed, Feb 11, 2009 at 5:02 AM, Wolfgang Denk <wd at denx.de> wrote:

> Dear Stefan,
>
> In message <200902111352.04598.sr at 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