[U-Boot] [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.
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 */
-----Original Message----- From: Michael Walle [mailto:michael@walle.cc] Sent: Tuesday, November 08, 2011 3:38 AM To: u-boot@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 . .
On Tuesday 08 November 2011 02:18:20 Prafulla Wadaskar wrote:
Here are my suggestions-
- commond/random_mac.c should be added
- 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
Dear Mike Frysinger,
In message 201111080858.25678.vapier@gentoo.org you wrote:
- 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
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
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.
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
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
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).
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
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.
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
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.
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
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.
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?
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
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.
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
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
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
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
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
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
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
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
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
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.
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
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 ;)
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
-----Original Message----- From: Wolfgang Denk [mailto:wd@denx.de] Sent: Thursday, November 17, 2011 1:57 AM To: Michael Walle Cc: Prafulla Wadaskar; Valentin Longchamp; Siddarth Gore; Simon Guinot; u-boot@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 . . .
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
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;
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
-----Original Message----- From: Michael Walle [mailto:michael@walle.cc] Sent: Friday, November 18, 2011 4:22 AM To: u-boot@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 */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 . . .
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
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
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
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 ;)
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
participants (7)
-
Holger Brunck -
Michael Walle -
Mike Frysinger -
Prafulla Wadaskar -
Simon Guinot -
Valentin Longchamp -
Wolfgang Denk