From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Wed, 11 Feb 2009 14:02:59 +0100
Subject: [U-Boot] [PATCH] ppc4xx: Fix problem with board_eth_init() vs
	cpu_eth_init() on AMCC boards
In-Reply-To: <200902111352.04598.sr@denx.de>
References: <1234342325-28950-1-git-send-email-sr@denx.de>
	<20090211121930.7F9B4832E893@gemini.denx.de>
	<200902111352.04598.sr@denx.de>
Message-ID: <20090211130259.5AFAD832E893@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.
> 
> 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.

> 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.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Well I don't see why I have to make one man  miserable  when  I  can
make so many men happy."              - Ellyn Mustard, about marriage