[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

A network driver should not touch the environment at all. This patch fixes
this behaviour by removing the code for setting a default/randomized MAC
address.

Instead a board should either set CONFIG_ETHADDR, CONFIG_ETH1ADDR etc. or
use some specific code within the board files, eg. if randomization is
needed.

All boards which have CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION set (actually its
only one board, edminiv2) are converted to use CONFIG_ETHADDR. Please check
your boards whether you really need randomization support or the
CONFIG_ETHADDR macro is enough.

Signed-off-by: Michael Walle <michael@walle.cc>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
Cc: Eric Cooper <ecc@cmu.edu>
Cc: Jason Cooper <u-boot@lakedaemon.net>
Cc: Siddarth Gore <gores@marvell.com>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Prafulla Wadaskar <prafulla@marvell.com>
Cc: Simon Guinot <simon.guinot@sequanux.org>
---
 drivers/net/mvgbe.c        |   23 -----------------------
 include/configs/edminiv2.h |    2 +-
 2 files changed, 1 insertions(+), 24 deletions(-)

diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c
index c7f7446..21be642 100644
--- a/drivers/net/mvgbe.c
+++ b/drivers/net/mvgbe.c
@@ -647,7 +647,6 @@ int mvgbe_initialize(bd_t *bis)
 	struct mvgbe_device *dmvgbe;
 	struct eth_device *dev;
 	int devnum;
-	char *s;
 	u8 used_ports[MAX_MVGBE_DEVS] = CONFIG_MVGBE_PORTS;
 
 	for (devnum = 0; devnum < MAX_MVGBE_DEVS; devnum++) {
@@ -702,16 +701,13 @@ error1:
 		/* must be less than NAMESIZE (16) */
 		sprintf(dev->name, "egiga%d", devnum);
 
-		/* Extract the MAC address from the environment */
 		switch (devnum) {
 		case 0:
 			dmvgbe->regs = (void *)MVGBE0_BASE;
-			s = "ethaddr";
 			break;
 #if defined(MVGBE1_BASE)
 		case 1:
 			dmvgbe->regs = (void *)MVGBE1_BASE;
-			s = "eth1addr";
 			break;
 #endif
 		default:	/* this should never happen */
@@ -720,25 +716,6 @@ error1:
 			return -1;
 		}
 
-		while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
-			/* Generate Private MAC addr if not set */
-			dev->enetaddr[0] = 0x02;
-			dev->enetaddr[1] = 0x50;
-			dev->enetaddr[2] = 0x43;
-#if defined (CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION)
-			/* Generate fixed lower MAC half using devnum */
-			dev->enetaddr[3] = 0;
-			dev->enetaddr[4] = 0;
-			dev->enetaddr[5] = devnum;
-#else
-			/* Generate random lower MAC half */
-			dev->enetaddr[3] = get_random_hex();
-			dev->enetaddr[4] = get_random_hex();
-			dev->enetaddr[5] = get_random_hex();
-#endif
-			eth_setenv_enetaddr(s, dev->enetaddr);
-		}
-
 		dev->init = (void *)mvgbe_init;
 		dev->halt = (void *)mvgbe_halt;
 		dev->send = (void *)mvgbe_send;
diff --git a/include/configs/edminiv2.h b/include/configs/edminiv2.h
index 88d32b2..2b96cb3 100644
--- a/include/configs/edminiv2.h
+++ b/include/configs/edminiv2.h
@@ -142,7 +142,7 @@
 #ifdef CONFIG_CMD_NET
 #define CONFIG_MVGBE				/* Enable Marvell GbE Driver */
 #define CONFIG_MVGBE_PORTS	{1}		/* enable port 0 only */
-#define CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION	/* don't randomize MAC */
+#define CONFIG_ETH_ADDR 02:50:43:00:00:01
 #define CONFIG_PHY_BASE_ADR	0x8
 #define CONFIG_RESET_PHY_R	/* use reset_phy() to init mv8831116 PHY */
 #define CONFIG_NETCONSOLE	/* include NetConsole support   */
-- 
1.7.2.5

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Prafulla Wadaskar]

> -----Original Message-----
> From: Michael Walle [mailto:michael at walle.cc]
> Sent: Tuesday, November 08, 2011 3:38 AM
> To: u-boot at lists.denx.de
> Cc: Michael Walle; Mike Frysinger; Valentin Longchamp; Eric
> Cooper; Jason Cooper; Siddarth Gore; Albert ARIBAUD; Prafulla
> Wadaskar; Simon Guinot
> Subject: [PATCH] mvgbe: remove setting of ethaddr within the
> driver
> 
> A network driver should not touch the environment at all. This
> patch fixes
> this behaviour by removing the code for setting a
> default/randomized MAC
> address.
> 
> Instead a board should either set CONFIG_ETHADDR,
> CONFIG_ETH1ADDR etc. or
> use some specific code within the board files, eg. if
> randomization is
> needed.

Dear Michael

Yes, it is needed on few boards.
This patch removes this support for these boards, it is good to remove it from driver code, but this functionality should not be broken, the same should be supported in board specific code.

So NAK for this patch.

I had suggested to add some common framework like common/hush.c,

Here are my suggestions-
1. commond/random_mac.c should be added
2. call back function should be provided to generate random number, those should be defined in arch specific code (for Kirkwood arch-kirkwood/cpu.c)
3. mac randomization should be enabled by CONFIG_SYS_LOCAL_MAC_RANDOMIZATION macro
4. For mvgbe uses it should be enabled by default in include/configs/mv-common.h.
5. for corner case like edminiv2, in should be undefed in board config file
6. Some documentation should be supported for this generic framework.

Regards..
Prafulla . .

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Mike Frysinger]

On Tuesday 08 November 2011 02:18:20 Prafulla Wadaskar wrote:
> Here are my suggestions-
> 1. commond/random_mac.c should be added
> 2. call back function should be provided to generate random number, those
> should be defined in arch specific code (for Kirkwood arch-kirkwood/cpu.c)
> 3. mac randomization should be enabled by
> CONFIG_SYS_LOCAL_MAC_RANDOMIZATION macro 4. For mvgbe uses it should be
> enabled by default in include/configs/mv-common.h. 5. for corner case like
> edminiv2, in should be undefed in board config file 6. Some documentation
> should be supported for this generic framework.

i think Wolfgang already NAK-ed this idea.  but he'd have to clarify.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111108/71a48366/attachment.pgp 

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Mike Frysinger,

In message <201111080858.25678.vapier@gentoo.org> you wrote:
>
> > 3. mac randomization should be enabled by
> > CONFIG_SYS_LOCAL_MAC_RANDOMIZATION macro 4. For mvgbe uses it should be
> > enabled by default in include/configs/mv-common.h. 5. for corner case like
> > edminiv2, in should be undefed in board config file 6. Some documentation
> > should be supported for this generic framework.
> 
> i think Wolfgang already NAK-ed this idea.  but he'd have to clarify.

Indeed I did NAK this.  And I tried to provide an explantion why I
consider this an inherently broken approach.

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
"Love your country but never trust its government."
- from a hand-painted road sign in central Pennsylvania

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Mike Frysinger]

On Monday 07 November 2011 17:08:09 Michael Walle wrote:
>  drivers/net/mvgbe.c        |   23 -----------------------

ACK to the changes to this file

> --- a/include/configs/edminiv2.h
> +++ b/include/configs/edminiv2.h
>
> +#define CONFIG_ETH_ADDR 02:50:43:00:00:01

NAK to this.  board configs are not allow to hardcode MACs.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111108/8a1aec41/attachment.pgp 

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

Am Di, 8.11.2011, 14:57 schrieb Mike Frysinger:
> On Monday 07 November 2011 17:08:09 Michael Walle wrote:
>>  drivers/net/mvgbe.c        |   23 -----------------------
>
> ACK to the changes to this file
>
>> --- a/include/configs/edminiv2.h
>> +++ b/include/configs/edminiv2.h
>>
>> +#define CONFIG_ETH_ADDR 02:50:43:00:00:01
>
> NAK to this.  board configs are not allow to hardcode MACs.
mh, so why is there a CONFIG_ETH_ADDR macro? is it deprecated?
README.enetaddr explicitly refers to that config option.

-- 
michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Mike Frysinger]

On Tuesday 08 November 2011 08:45:27 Michael Walle wrote:
> Am Di, 8.11.2011, 14:57 schrieb Mike Frysinger:
> > On Monday 07 November 2011 17:08:09 Michael Walle wrote:
> >> --- a/include/configs/edminiv2.h
> >> +++ b/include/configs/edminiv2.h
> >> 
> >> +#define CONFIG_ETH_ADDR 02:50:43:00:00:01
> > 
> > NAK to this.  board configs are not allow to hardcode MACs.
> 
> mh, so why is there a CONFIG_ETH_ADDR macro? is it deprecated?
> README.enetaddr explicitly refers to that config option.

it's a useful debug knob
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111108/5f715a2d/attachment.pgp 

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear "Michael Walle",

In message <89e599b8867346f857ae2f132e9717a3.squirrel@ssl.serverraum.org> you wrote:
>
> >> +#define CONFIG_ETH_ADDR 02:50:43:00:00:01
> >
> > NAK to this.  board configs are not allow to hardcode MACs.
> mh, so why is there a CONFIG_ETH_ADDR macro? is it deprecated?
> README.enetaddr explicitly refers to that config option.

There are situations where it cannot be helped, like when programming
the U-Boot image into a ROM (yes, a real ROM that cannot be changed
any more by software alone).

But in almost all normal situations this must not be used.  In any
case, we will not accept it into mainline.

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
Be kind to unkind people - they need it the most.

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

Hi Wolfgang, Hi Mike,

Am Di, 8.11.2011, 16:17 schrieb Wolfgang Denk:
> Dear "Michael Walle",
>
> In message <89e599b8867346f857ae2f132e9717a3.squirrel@ssl.serverraum.org>
> you wrote:
>>
>> >> +#define CONFIG_ETH_ADDR 02:50:43:00:00:01
>> >
>> > NAK to this.  board configs are not allow to hardcode MACs.
>> mh, so why is there a CONFIG_ETH_ADDR macro? is it deprecated?
>> README.enetaddr explicitly refers to that config option.
>
> There are situations where it cannot be helped, like when programming
> the U-Boot image into a ROM (yes, a real ROM that cannot be changed
> any more by software alone).
>
> But in almost all normal situations this must not be used.  In any
> case, we will not accept it into mainline.

I have a Buffalo Linkstation Pro LS-XHL for which i plan to submit a BSP.
On this board, the MAC address is saved within the environment, which is
in turn stored in flash. Additionally this board does not have a serial
port, thus the only available console is the network console.

If someone will mess up the environment, it is possible to clear it by
holding a button upon powerup. Atm i have the CONFIG_ETH_ADDR set to a
default MAC address to be able to use the network console with the default
environment. To restore the original MAC address, a user have to manually
replace ethaddr with the one printed on the case.

But now, if i can't use the CONFIG_ETH_ADDR, i think i'm stuck :) Or do i
miss something? (Yeah there may be the MAC randomization, if accepted).

-- 
Michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear "Michael Walle",

In message <45fd92b472c944e2cb9ebb409488ccfc.squirrel@ssl.serverraum.org> you wrote:
> 
> If someone will mess up the environment, it is possible to clear it by
> holding a button upon powerup. Atm i have the CONFIG_ETH_ADDR set to a
> default MAC address to be able to use the network console with the default
> environment. To restore the original MAC address, a user have to manually
> replace ethaddr with the one printed on the case.
> 
> But now, if i can't use the CONFIG_ETH_ADDR, i think i'm stuck :) Or do i
> miss something? (Yeah there may be the MAC randomization, if accepted).

There are more clever ways than just to clear the environment. We have
"env export" and "env import", so you can create one or more backup
copies or user proviles.  Instead of clearing the environment, you
could also load one such previously saved user profile, which then
would contain the MAC address.

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
The night sky over the planet Krikkit is the least interesting  sight
in the entire Universe.
                 - Douglas Adams _Life, the Universe, and Everything_

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

Am Dienstag 08 November 2011, 20:28:19 schrieb Wolfgang Denk:
> Dear "Michael Walle",
> 
> In message <45fd92b472c944e2cb9ebb409488ccfc.squirrel@ssl.serverraum.org> 
you wrote:
> > If someone will mess up the environment, it is possible to clear it by
> > holding a button upon powerup. Atm i have the CONFIG_ETH_ADDR set to a
> > default MAC address to be able to use the network console with the
> > default environment. To restore the original MAC address, a user have to
> > manually replace ethaddr with the one printed on the case.
> > 
> > But now, if i can't use the CONFIG_ETH_ADDR, i think i'm stuck :) Or do i
> > miss something? (Yeah there may be the MAC randomization, if accepted).
> 
> There are more clever ways than just to clear the environment. We have
> "env export" and "env import", so you can create one or more backup
> copies or user proviles.  Instead of clearing the environment, you
> could also load one such previously saved user profile, which then
> would contain the MAC address.

thanks for the hint, but there is only 512kb flash on this board (with 64k 
sectors). I don't think there is enough room for a second copy of the 
environment.

and in some way, the mac address has to find its way into one of these 
environments.

i think there are more boards like this, esp. since there is this mac 
randomization code within the mvgbe driver.

-- 
Michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Michael Walle,

In message <201111082330.40253.michael@walle.cc> you wrote:
>
> > There are more clever ways than just to clear the environment. We have
> > "env export" and "env import", so you can create one or more backup
> > copies or user proviles.  Instead of clearing the environment, you
> > could also load one such previously saved user profile, which then
> > would contain the MAC address.
> 
> thanks for the hint, but there is only 512kb flash on this board (with 64k 
> sectors). I don't think there is enough room for a second copy of the 
> environment.

You don't need a separate sector.  How bis is your environment/  I bet
if fits easily in 4 KiB or so - just find an unused part of a sector,
and put it there.  Yes, upating it will be more complicated, as you
hav eto read that sector into RAM, replace the larst part, and write it
back, but resource-restricted systems require efficient use of memory.

> i think there are more boards like this, esp. since there is this mac 
> randomization code within the mvgbe driver.

I don't want to have that in mainline.

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
The Buddha, the Godhead, resides quite as comfortably in the circuits
of a digital computer or the gears of a cycle transmission as he does
at the top of a mountain or in the petals of a flower.
            - R.  Pirsig, "Zen and the Art of Motorcycle Maintenance"

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

Am Dienstag 08 November 2011, 23:52:56 schrieb Wolfgang Denk:
> > thanks for the hint, but there is only 512kb flash on this board (with
> > 64k sectors). I don't think there is enough room for a second copy of
> > the environment.
> 
> You don't need a separate sector.  How bis is your environment/  I bet
> if fits easily in 4 KiB or so - just find an unused part of a sector,
> and put it there.  Yes, upating it will be more complicated, as you
> hav eto read that sector into RAM, replace the larst part, and write it
> back, but resource-restricted systems require efficient use of memory.
may be possible, but then there is a window of opportunity where the mac is 
already erased but the not yet written with a new one.

and there is this very easy recovery procedure.. just erase the environment, 
let the network works in some way and manually set the mac address, which 
apparently use some boards.

> > i think there are more boards like this, esp. since there is this mac
> > randomization code within the mvgbe driver.
> 
> I don't want to have that in mainline.
well it is...

atm there is the mvgbe driver:
 - which has a hardcoded mac address (or if enabled, a randomized mac address)
   and
 - touches the environment

trying to fix this, it will be nak'ed because the boards in mainline have to 
behave identically before and after the patch. but without CONFIG_ETHADDR or 
generic randomization support, which will be nak'ed, too, this is not 
possible. sounds to me like a dead end.

additionally, there are these kind of boards without non-volatile storage 
other than one tiny flash, which holds uboot and its environment and therefore 
has the mac address stored within the environment. to make it more complicated 
some of these boards have to have a working ethernet connectivity with the 
default environment.

-- 
Michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Michael Walle,

In message <201111090026.09460.michael@walle.cc> you wrote:
>
> may be possible, but then there is a window of opportunity where the mac is 
> already erased but the not yet written with a new one.

Sure.  If you look hard enough you will always find a way to brick a
device.  And if you don't look, it just takes an ingeniuos idiot to
find one.

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
"The one charm of marriage is that it makes a  life  of  deception  a
neccessity."                                            - Oscar Wilde

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

Am Mi, 9.11.2011, 00:26 schrieb Michael Walle:
> atm there is the mvgbe driver:
>  - which has a hardcoded mac address (or if enabled, a randomized mac
> address)
>    and
>  - touches the environment

oh and i've forgot to mention, that the driver is broken for boards with

 #define CONFIG_MVGBE_PORTS {0,1}

which is the reason why i initially submitted the patch.

-- 
Michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

>> atm there is the mvgbe driver:
>>  - which has a hardcoded mac address (or if enabled, a randomized mac
>> address)
>>    and
>>  - touches the environment
>
> oh and i've forgot to mention, that the driver is broken for boards with
>
>  #define CONFIG_MVGBE_PORTS {0,1}
>
> which is the reason why i initially submitted the patch.
>

Hi Wolfgang, Hi Mike, Hi Prafulla,

So was there any conclusion, now? Never touch mvgbe.c anymore?

-- 
Michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear "Michael Walle",

In message <eda0bd6e66969b68bfd22692c603b005.squirrel@ssl.serverraum.org> you wrote:
> 
> So was there any conclusion, now? Never touch mvgbe.c anymore?

Let's remove this broken code.

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
"If you are afraid of loneliness, don't marry."             - Chekhov

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

Hi Wolfgang,

>> So was there any conclusion, now? Never touch mvgbe.c anymore?
> Let's remove this broken code.

This is exactly part of this patch ([PATCH] mvgbe: remove setting of
ethaddr within the driver). This part, while ACK'ed by Mike, was NAK'ed by
Prafulla.

-- 
Michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Simon Guinot]

Hi Michael,

On Thu, Nov 10, 2011 at 10:15:28AM +0100, Michael Walle wrote:
> 
> Hi Wolfgang,
> 
> >> So was there any conclusion, now? Never touch mvgbe.c anymore?
> > Let's remove this broken code.
> 
> This is exactly part of this patch ([PATCH] mvgbe: remove setting of
> ethaddr within the driver). This part, while ACK'ed by Mike, was NAK'ed by
> Prafulla.

As some boards are relying on this broken code, maybe you should provide
an alternative ? I could be wrong, but it seems to me that just removing
the code is not enough.

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111110/a2ee012d/attachment.pgp 

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Simon Guinot,

In message <20111110110608.GB29864@kw.sim.vm.gnt> you wrote:
> 
> > > Let's remove this broken code.
> > 
> > This is exactly part of this patch ([PATCH] mvgbe: remove setting of
> > ethaddr within the driver). This part, while ACK'ed by Mike, was NAK'ed by
> > Prafulla.
>
> As some boards are relying on this broken code, maybe you should provide
> an alternative ? I could be wrong, but it seems to me that just removing
> the code is not enough.

Which exact boards are these? And how exactly are these relying on the
broken code?

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
Why is an average signature file longer than an average Perl script??

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Simon Guinot]

Hi Wolfgang,

On Thu, Nov 10, 2011 at 01:01:59PM +0100, Wolfgang Denk wrote:
> Dear Simon Guinot,
> 
> In message <20111110110608.GB29864@kw.sim.vm.gnt> you wrote:
> > 
> > > > Let's remove this broken code.
> > > 
> > > This is exactly part of this patch ([PATCH] mvgbe: remove setting of
> > > ethaddr within the driver). This part, while ACK'ed by Mike, was NAK'ed by
> > > Prafulla.
> >
> > As some boards are relying on this broken code, maybe you should provide
> > an alternative ? I could be wrong, but it seems to me that just removing
> > the code is not enough.
> 
> Which exact boards are these? And how exactly are these relying on the
> broken code?

I can't list this boards exactly. For sure, I can say that the
LaCie boards (netspace_v2 and cie) are concerned.

For this boards a random MAC address is used and the computation is
performed by the driver mvgbe. If this code is simply removed, the
network initialization for this boards will break.

I suspect a lot of Marvell boards from being in the same case.

Regards,

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111110/49719d4e/attachment.pgp 

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Simon Guinot,

In message <20111110142152.GC29864@kw.sim.vm.gnt> you wrote:
> 
> I can't list this boards exactly. For sure, I can say that the
> LaCie boards (netspace_v2 and cie) are concerned.
> 
> For this boards a random MAC address is used and the computation is
> performed by the driver mvgbe. If this code is simply removed, the
> network initialization for this boards will break.
> 
> I suspect a lot of Marvell boards from being in the same case.

Argh... I was not aware that the scope of damage already done was that
big.

But no matter how we twist and turn it, this is a real problem that
needs to be fixed, and the sooner we do it the better.

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
       There is enough for the need of everyone in this world,
       but not for the greed of everyone.     - Mahatma Gandhi

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Valentin Longchamp]

On 11/10/2011 04:53 PM, Wolfgang Denk wrote:
> In message <20111110142152.GC29864@kw.sim.vm.gnt> you wrote:
>>
>> I suspect a lot of Marvell boards from being in the same case.
> 
> Argh... I was not aware that the scope of damage already done was that
> big.
> 

On the Keymile boards (for marvell boards, km_kirkwood) we have an EEPROM that
stores the ethaddr for each board, so we should not be affected by the removal
of this "feature" (the driver setting ethaddr).

Cheers

Valentin

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Holger Brunck]

On 11/10/2011 05:30 PM, Valentin Longchamp wrote:
> On 11/10/2011 04:53 PM, Wolfgang Denk wrote:
>> In message <20111110142152.GC29864@kw.sim.vm.gnt> you wrote:
>>>
>>> I suspect a lot of Marvell boards from being in the same case.
>>
>> Argh... I was not aware that the scope of damage already done was that
>> big.
>>
> 
> On the Keymile boards (for marvell boards, km_kirkwood) we have an EEPROM that
> stores the ethaddr for each board, so we should not be affected by the removal
> of this "feature" (the driver setting ethaddr).
> 

I gave it a try on km_kirkwood. What happens on keymile boards is that we get
the warning:
Warning: failed to set MAC address
if we initially start the board without an environment saved previously.

And this is better than writing a random mac adress into the HW, I guess...

Fortunately our boards in case of an initial boot are configuring different
things e.g. the correct ethaddr,  do a saveenv and a reboot. And afterwards
everything is ok.

So yes for our boards it's ok to remove this code.

Best regards
Holger

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Mike Frysinger]

On Thursday 10 November 2011 11:30:13 Valentin Longchamp wrote:
> On 11/10/2011 04:53 PM, Wolfgang Denk wrote:
> > In message <20111110142152.GC29864@kw.sim.vm.gnt> you wrote:
> >> I suspect a lot of Marvell boards from being in the same case.
> > 
> > Argh... I was not aware that the scope of damage already done was that
> > big.
> 
> On the Keymile boards (for marvell boards, km_kirkwood) we have an EEPROM
> that stores the ethaddr for each board, so we should not be affected by
> the removal of this "feature" (the driver setting ethaddr).

curious, but is it an eeprom on an external bus, or is it connected to the 
mvgbe block ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111110/f3f92d08/attachment.pgp 

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Mike Frysinger]

On Thursday 10 November 2011 09:21:53 Simon Guinot wrote:
> On Thu, Nov 10, 2011 at 01:01:59PM +0100, Wolfgang Denk wrote:
> > Simon Guinot wrote:
> > > > > Let's remove this broken code.
> > > > 
> > > > This is exactly part of this patch ([PATCH] mvgbe: remove setting of
> > > > ethaddr within the driver). This part, while ACK'ed by Mike, was
> > > > NAK'ed by Prafulla.
> > > 
> > > As some boards are relying on this broken code, maybe you should
> > > provide an alternative ? I could be wrong, but it seems to me that
> > > just removing the code is not enough.
> > 
> > Which exact boards are these? And how exactly are these relying on the
> > broken code?
> 
> I can't list this boards exactly. For sure, I can say that the
> LaCie boards (netspace_v2 and cie) are concerned.
> 
> For this boards a random MAC address is used and the computation is
> performed by the driver mvgbe. If this code is simply removed, the
> network initialization for this boards will break.

it will be broken until you set "ethaddr" in the env right ?  i think that's 
currently the expected behavior ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111110/6dc92555/attachment.pgp 

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Simon Guinot]

On Thu, Nov 10, 2011 at 12:09:04PM -0500, Mike Frysinger wrote:
> On Thursday 10 November 2011 09:21:53 Simon Guinot wrote:
> > On Thu, Nov 10, 2011 at 01:01:59PM +0100, Wolfgang Denk wrote:
> > > Simon Guinot wrote:
> > > > > > Let's remove this broken code.
> > > > > 
> > > > > This is exactly part of this patch ([PATCH] mvgbe: remove setting of
> > > > > ethaddr within the driver). This part, while ACK'ed by Mike, was
> > > > > NAK'ed by Prafulla.
> > > > 
> > > > As some boards are relying on this broken code, maybe you should
> > > > provide an alternative ? I could be wrong, but it seems to me that
> > > > just removing the code is not enough.
> > > 
> > > Which exact boards are these? And how exactly are these relying on the
> > > broken code?
> > 
> > I can't list this boards exactly. For sure, I can say that the
> > LaCie boards (netspace_v2 and cie) are concerned.
> > 
> > For this boards a random MAC address is used and the computation is
> > performed by the driver mvgbe. If this code is simply removed, the
> > network initialization for this boards will break.
> 
> it will be broken until you set "ethaddr" in the env right ?  i think that's 
> currently the expected behavior ...

Yes it is right. I guess if someone is able to write U-Boot into a
flash, he is also able to set the environement variable "ethaddr".

An extra operation would be needed to setup U-Boot, but clearly
it is not a problem.

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111110/8b0b2ce0/attachment.pgp 

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

Am Donnerstag 10 November 2011, 15:21:53 schrieb Simon Guinot:
> For this boards a random MAC address is used and the computation is
> performed by the driver mvgbe. If this code is simply removed, the
> network initialization for this boards will break.

Moving this feature into some common code was NAK'ed by Wolfgang.

-- 
Michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Prafulla,

In message <48935aa530a6457b76b9915a1fe7faf9.squirrel@ssl.serverraum.org> Michael Walle wrote:
> 
> >> So was there any conclusion, now? Never touch mvgbe.c anymore?
> > Let's remove this broken code.
> 
> This is exactly part of this patch ([PATCH] mvgbe: remove setting of
> ethaddr within the driver). This part, while ACK'ed by Mike, was NAK'ed by
> Prafulla.

Could you please revise your assessment?  I would really appreciate if
we could clean this up.

Thanks.

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
Yes, it's a technical challenge, and  you  have  to  kind  of  admire
people  who go to the lengths of actually implementing it, but at the
same time you wonder about their IQ...
         --  Linus Torvalds in <5phda5$ml6$1@palladium.transmeta.com>

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

Hi Wolfgang,

Am Donnerstag 10 November 2011, 12:44:26 schrieb Wolfgang Denk:
> Dear Prafulla,
> 
> In message <48935aa530a6457b76b9915a1fe7faf9.squirrel@ssl.serverraum.org> 
Michael Walle wrote:
> > >> So was there any conclusion, now? Never touch mvgbe.c anymore?
> > > 
> > > Let's remove this broken code.
> > 
> > This is exactly part of this patch ([PATCH] mvgbe: remove setting of
> > ethaddr within the driver). This part, while ACK'ed by Mike, was NAK'ed
> > by Prafulla.
> 
> Could you please revise your assessment?  I would really appreciate if
> we could clean this up.
Regarding Prafulla's NAK?

Let me sum up this thread once again:
 - consens is found that the driver should be cleaned up (removing the
   randomization code)
 - at least the keymile boards are ok with that. but there may be plenty of
   other boards which rely on this (broken?) behaviour.
 - besides the fact that a driver must not touch the environment, the mvgbe
   driver also fails to do this correctly for boards which has only the second
   port enabled.
 - Prafulla NAKed because only removing the code will leave some boards in a
   non-working state.


i'm willing make a patch to move the randomization functionality to some 
generic place, but only if it is not NAKed by you beforehand. i dont have any 
time to waste ;)

-- 
Michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Michael Walle,

In message <201111162115.33330.michael@walle.cc> you wrote:
> 
> > Could you please revise your assessment?  I would really appreciate if
> > we could clean this up.
> Regarding Prafulla's NAK?

Yes - I'd really appreciate if Prafulla woudl withdraw his NAK, and
agree on a cleanup.  But he remains silent for a week or so...

> Let me sum up this thread once again:
>  - consens is found that the driver should be cleaned up (removing the
>    randomization code)
>  - at least the keymile boards are ok with that. but there may be plenty of
>    other boards which rely on this (broken?) behaviour.

The problem is this "may be".  We don't know it, and we will never
find out.  And even if we find such configurations, we have no
information if any user actually needs this feature.  The only way to
really find out is by removing this stuff now.  If anybody complains
then, we can discuss with him approaches to fix this again, in a clean
way.

>  - besides the fact that a driver must not touch the environment, the mvgbe
>    driver also fails to do this correctly for boards which has only the second
>    port enabled.
>  - Prafulla NAKed because only removing the code will leave some boards in a
>    non-working state.
> 
> i'm willing make a patch to move the randomization functionality to some 
> generic place, but only if it is not NAKed by you beforehand. i dont have any 
> time to waste ;)

Please don't move the broken code around.   Let's remove it.

Let's give Prafulla another 2 or 3 days time to respond.  If we don;t
hear from him, we go ahead.

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
Be careful what you wish for. You never know who will be listening.
                                      - Terry Pratchett, _Soul Music_

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Prafulla Wadaskar]

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de]
> Sent: Thursday, November 17, 2011 1:57 AM
> To: Michael Walle
> Cc: Prafulla Wadaskar; Valentin Longchamp; Siddarth Gore; Simon
> Guinot; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] mvgbe: remove setting of ethaddr
> within the driver
> 
> Dear Michael Walle,
> 
> In message <201111162115.33330.michael@walle.cc> you wrote:
> >
> > > Could you please revise your assessment?  I would really
> appreciate if
> > > we could clean this up.
> > Regarding Prafulla's NAK?
> 
> Yes - I'd really appreciate if Prafulla woudl withdraw his NAK,

Hi All
I withdraw my NAK for this patch.
Let's remove mac randomization support for mvgbe driver.
If needed, for any particular board, it will be addressed separately.

Regards..
Prafulla . . .

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Prafulla,

In message <F766E4F80769BD478052FB6533FA745D1A14EE2A22@SC-VEXCH4.marvell.com> you wrote:
> 
> I withdraw my NAK for this patch.
> Let's remove mac randomization support for mvgbe driver.
> If needed, for any particular board, it will be addressed separately.

Thanks a lot - highly appreciated.

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
I'm a programmer: I don't buy software, I write it.
                                                  -- Tom Christiansen

[U-Boot] [PATCH v2] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

A network driver should not touch the environment at all. This patch fixes
this behaviour by removing the code for setting a default/randomized MAC
address.

Signed-off-by: Michael Walle <michael@walle.cc>
Acked-by: Mike Frysinger <vapier@gentoo.org>
Acked-by: Prafulla Wadaskar <prafulla@marvell.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Prafulla Wadaskar <prafulla@marvell.com>
Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
Cc: Eric Cooper <ecc@cmu.edu>
Cc: Jason Cooper <u-boot@lakedaemon.net>
Cc: Siddarth Gore <gores@marvell.com>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
Cc: Simon Guinot <simon.guinot@sequanux.org>
---
 drivers/net/mvgbe.c |   23 -----------------------
 1 files changed, 0 insertions(+), 23 deletions(-)

diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c
index c7f7446..21be642 100644
--- a/drivers/net/mvgbe.c
+++ b/drivers/net/mvgbe.c
@@ -647,7 +647,6 @@ int mvgbe_initialize(bd_t *bis)
 	struct mvgbe_device *dmvgbe;
 	struct eth_device *dev;
 	int devnum;
-	char *s;
 	u8 used_ports[MAX_MVGBE_DEVS] = CONFIG_MVGBE_PORTS;
 
 	for (devnum = 0; devnum < MAX_MVGBE_DEVS; devnum++) {
@@ -702,16 +701,13 @@ error1:
 		/* must be less than NAMESIZE (16) */
 		sprintf(dev->name, "egiga%d", devnum);
 
-		/* Extract the MAC address from the environment */
 		switch (devnum) {
 		case 0:
 			dmvgbe->regs = (void *)MVGBE0_BASE;
-			s = "ethaddr";
 			break;
 #if defined(MVGBE1_BASE)
 		case 1:
 			dmvgbe->regs = (void *)MVGBE1_BASE;
-			s = "eth1addr";
 			break;
 #endif
 		default:	/* this should never happen */
@@ -720,25 +716,6 @@ error1:
 			return -1;
 		}
 
-		while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
-			/* Generate Private MAC addr if not set */
-			dev->enetaddr[0] = 0x02;
-			dev->enetaddr[1] = 0x50;
-			dev->enetaddr[2] = 0x43;
-#if defined (CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION)
-			/* Generate fixed lower MAC half using devnum */
-			dev->enetaddr[3] = 0;
-			dev->enetaddr[4] = 0;
-			dev->enetaddr[5] = devnum;
-#else
-			/* Generate random lower MAC half */
-			dev->enetaddr[3] = get_random_hex();
-			dev->enetaddr[4] = get_random_hex();
-			dev->enetaddr[5] = get_random_hex();
-#endif
-			eth_setenv_enetaddr(s, dev->enetaddr);
-		}
-
 		dev->init = (void *)mvgbe_init;
 		dev->halt = (void *)mvgbe_halt;
 		dev->send = (void *)mvgbe_send;
-- 
1.7.2.5

[U-Boot] [PATCH v2] mvgbe: remove setting of ethaddr within the driver

[Mike Frysinger]

On Thursday 17 November 2011 17:52:29 Michael Walle wrote:
> --- a/drivers/net/mvgbe.c
> +++ b/drivers/net/mvgbe.c
>
>  		switch (devnum) {
>  		case 0:
>  			dmvgbe->regs = (void *)MVGBE0_BASE;

unrelated, but usually this is pushed into an option to the init func ... the 
caller passes in the base address for the regs rather than the func trying to 
decode things itself.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111118/6f67a448/attachment.pgp>

[U-Boot] [PATCH v2] mvgbe: remove setting of ethaddr within the driver

[Prafulla Wadaskar]

> -----Original Message-----
> From: Michael Walle [mailto:michael at walle.cc]
> Sent: Friday, November 18, 2011 4:22 AM
> To: u-boot at lists.denx.de
> Cc: Wolfgang Denk; Michael Walle; Mike Frysinger; Prafulla
> Wadaskar; Valentin Longchamp; Eric Cooper; Jason Cooper;
> Siddarth Gore; Albert ARIBAUD; Simon Guinot
> Subject: [PATCH v2] mvgbe: remove setting of ethaddr within the
> driver
> 
> A network driver should not touch the environment at all. This
> patch fixes
> this behaviour by removing the code for setting a
> default/randomized MAC
> address.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Acked-by: Mike Frysinger <vapier@gentoo.org>
> Acked-by: Prafulla Wadaskar <prafulla@marvell.com>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>
> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> Cc: Eric Cooper <ecc@cmu.edu>
> Cc: Jason Cooper <u-boot@lakedaemon.net>
> Cc: Siddarth Gore <gores@marvell.com>
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Simon Guinot <simon.guinot@sequanux.org>
> ---
>  drivers/net/mvgbe.c |   23 -----------------------
>  1 files changed, 0 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c
> index c7f7446..21be642 100644
> --- a/drivers/net/mvgbe.c
> +++ b/drivers/net/mvgbe.c
> @@ -647,7 +647,6 @@ int mvgbe_initialize(bd_t *bis)
>  	struct mvgbe_device *dmvgbe;
>  	struct eth_device *dev;
>  	int devnum;
> -	char *s;
>  	u8 used_ports[MAX_MVGBE_DEVS] = CONFIG_MVGBE_PORTS;
> 
>  	for (devnum = 0; devnum < MAX_MVGBE_DEVS; devnum++) {
> @@ -702,16 +701,13 @@ error1:
>  		/* must be less than NAMESIZE (16) */
>  		sprintf(dev->name, "egiga%d", devnum);
> 
> -		/* Extract the MAC address from the environment */
>  		switch (devnum) {
>  		case 0:
>  			dmvgbe->regs = (void *)MVGBE0_BASE;
> -			s = "ethaddr";
>  			break;
>  #if defined(MVGBE1_BASE)
>  		case 1:
>  			dmvgbe->regs = (void *)MVGBE1_BASE;
> -			s = "eth1addr";
>  			break;
>  #endif
>  		default:	/* this should never happen */
> @@ -720,25 +716,6 @@ error1:
>  			return -1;
>  		}
> 
> -		while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
> -			/* Generate Private MAC addr if not set */
> -			dev->enetaddr[0] = 0x02;
> -			dev->enetaddr[1] = 0x50;
> -			dev->enetaddr[2] = 0x43;
> -#if defined (CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION)
> -			/* Generate fixed lower MAC half using devnum */
> -			dev->enetaddr[3] = 0;
> -			dev->enetaddr[4] = 0;
> -			dev->enetaddr[5] = devnum;
> -#else
> -			/* Generate random lower MAC half */
> -			dev->enetaddr[3] = get_random_hex();
> -			dev->enetaddr[4] = get_random_hex();
> -			dev->enetaddr[5] = get_random_hex();
> -#endif
> -			eth_setenv_enetaddr(s, dev->enetaddr);
> -		}
> -
>  		dev->init = (void *)mvgbe_init;
>  		dev->halt = (void *)mvgbe_halt;
>  		dev->send = (void *)mvgbe_send;
> --
> 1.7.2.5

Ack for this patch
Acked-by: Prafulla Wadaskar <prafulla@marvell.com>

Regards..
Prafulla . . .

[U-Boot] [PATCH v2] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Michael Walle,

In message <1321570349-4224-1-git-send-email-michael@walle.cc> you wrote:
> A network driver should not touch the environment at all. This patch fixes
> this behaviour by removing the code for setting a default/randomized MAC
> address.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Acked-by: Mike Frysinger <vapier@gentoo.org>
> Acked-by: Prafulla Wadaskar <prafulla@marvell.com>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>
> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> Cc: Eric Cooper <ecc@cmu.edu>
> Cc: Jason Cooper <u-boot@lakedaemon.net>
> Cc: Siddarth Gore <gores@marvell.com>
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Simon Guinot <simon.guinot@sequanux.org>
> ---
>  drivers/net/mvgbe.c |   23 -----------------------
>  1 files changed, 0 insertions(+), 23 deletions(-)

Applied to "next" branch, thanks.

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
Any sufficiently advanced  technology  is  indistinguishable  from  a
rigged demo.                              - Andy Finkel, computer guy

[U-Boot] [PATCH v2] mvgbe: remove setting of ethaddr within the driver

[Wolfgang Denk]

Dear Michael Walle,

In message <1321570349-4224-1-git-send-email-michael@walle.cc> you wrote:
> A network driver should not touch the environment at all. This patch fixes
> this behaviour by removing the code for setting a default/randomized MAC
> address.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Acked-by: Mike Frysinger <vapier@gentoo.org>
> Acked-by: Prafulla Wadaskar <prafulla@marvell.com>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>
> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> Cc: Eric Cooper <ecc@cmu.edu>
> Cc: Jason Cooper <u-boot@lakedaemon.net>
> Cc: Siddarth Gore <gores@marvell.com>
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Simon Guinot <simon.guinot@sequanux.org>
> ---
>  drivers/net/mvgbe.c |   23 -----------------------
>  1 files changed, 0 insertions(+), 23 deletions(-)

Applied to "next" branch, thanks.

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
It is a good thing for an uneducated man to read books of quotations.
                        - Sir Winston Churchill _My Early Life_ ch. 9

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Mike Frysinger]

On Tuesday 08 November 2011 12:34:17 Michael Walle wrote:
> Am Di, 8.11.2011, 16:17 schrieb Wolfgang Denk:
> > Michael Walle wrote:
> >> >> +#define CONFIG_ETH_ADDR 02:50:43:00:00:01
> >> > 
> >> > NAK to this.  board configs are not allow to hardcode MACs.
> >> 
> >> mh, so why is there a CONFIG_ETH_ADDR macro? is it deprecated?
> >> README.enetaddr explicitly refers to that config option.
> > 
> > There are situations where it cannot be helped, like when programming
> > the U-Boot image into a ROM (yes, a real ROM that cannot be changed
> > any more by software alone).
> > 
> > But in almost all normal situations this must not be used.  In any
> > case, we will not accept it into mainline.
> 
> I have a Buffalo Linkstation Pro LS-XHL for which i plan to submit a BSP.
> On this board, the MAC address is saved within the environment, which is
> in turn stored in flash. Additionally this board does not have a serial
> port, thus the only available console is the network console.
> 
> If someone will mess up the environment, it is possible to clear it by
> holding a button upon powerup. Atm i have the CONFIG_ETH_ADDR set to a
> default MAC address to be able to use the network console with the default
> environment. To restore the original MAC address, a user have to manually
> replace ethaddr with the one printed on the case.
> 
> But now, if i can't use the CONFIG_ETH_ADDR, i think i'm stuck :) Or do i
> miss something? (Yeah there may be the MAC randomization, if accepted).

the "default" MAC address would collide with other Buffalo stations which have 
the "default" MAC address

by default, ethaddr is not writable, so people can't simply "set ethaddr xxx" 
to screw it up.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111108/df8cdb1b/attachment.pgp 

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Michael Walle]

Am Dienstag 08 November 2011, 21:49:49 schrieb Mike Frysinger:
> On Tuesday 08 November 2011 12:34:17 Michael Walle wrote:
> > Am Di, 8.11.2011, 16:17 schrieb Wolfgang Denk:
> > > Michael Walle wrote:
> > >> >> +#define CONFIG_ETH_ADDR 02:50:43:00:00:01
> > >> > 
> > >> > NAK to this.  board configs are not allow to hardcode MACs.
> > >> 
> > >> mh, so why is there a CONFIG_ETH_ADDR macro? is it deprecated?
> > >> README.enetaddr explicitly refers to that config option.
> > > 
> > > There are situations where it cannot be helped, like when programming
> > > the U-Boot image into a ROM (yes, a real ROM that cannot be changed
> > > any more by software alone).
> > > 
> > > But in almost all normal situations this must not be used.  In any
> > > case, we will not accept it into mainline.
> > 
> > I have a Buffalo Linkstation Pro LS-XHL for which i plan to submit a BSP.
> > On this board, the MAC address is saved within the environment, which is
> > in turn stored in flash. Additionally this board does not have a serial
> > port, thus the only available console is the network console.
> > 
> > If someone will mess up the environment, it is possible to clear it by
> > holding a button upon powerup. Atm i have the CONFIG_ETH_ADDR set to a
> > default MAC address to be able to use the network console with the
> > default environment. To restore the original MAC address, a user have to
> > manually replace ethaddr with the one printed on the case.
> > 
> > But now, if i can't use the CONFIG_ETH_ADDR, i think i'm stuck :) Or do i
> > miss something? (Yeah there may be the MAC randomization, if accepted).
> 
> the "default" MAC address would collide with other Buffalo stations which
> have the "default" MAC address
yeah of course, but it is only a recovery mechanism. i would be happy with a 
generated mac address, too.

> by default, ethaddr is not writable, so people can't simply "set ethaddr
> xxx" to screw it up.
well a user could screw it up in many other ways too ;)

there are plenty of boards which has CONFIG_ETHADDR and 
CONFIG_OVERWRITE_ETHADDR_ONCE set. This should exactly be the configuration i 
need, too.

btw only ethaddr is protected and not ethNaddr, which is a bug imho ;)

-- 
Michael

[U-Boot] [PATCH] mvgbe: remove setting of ethaddr within the driver

[Mike Frysinger]

On Tuesday 08 November 2011 17:45:12 Michael Walle wrote:
> btw only ethaddr is protected and not ethNaddr, which is a bug imho ;)

yeah, it is, but i've ignored it as all my boards have only had 1 eth dev.  
feel free to post a patch :).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111109/d37c8cfc/attachment.pgp