| Message ID | 1319751096-24404-3-git-send-email-michael@walle.cc |
|---|---|
| State | Rejected |
| Headers | show |
On Thursday 27 October 2011 17:31:36 Michael Walle wrote: > --- a/drivers/net/mvgbe.c > +++ b/drivers/net/mvgbe.c > > + /* Extract the MAC address from the environment */ > + while (!eth_getenv_enetaddr_by_index("eth", dev->index, > + dev->enetaddr)) { > /* Generate Private MAC addr if not set */ > dev->enetaddr[0] = 0x02; > dev->enetaddr[1] = 0x50; this is wrong. net drivers should not be touching the env at all. please fix your driver to not do that first. -mike
Am Donnerstag 03 November 2011, 19:10:57 schrieb Mike Frysinger: > On Thursday 27 October 2011 17:31:36 Michael Walle wrote: > > --- a/drivers/net/mvgbe.c > > +++ b/drivers/net/mvgbe.c > > > > + /* Extract the MAC address from the environment */ > > + while (!eth_getenv_enetaddr_by_index("eth", dev->index, > > + dev->enetaddr)) { > > > > /* Generate Private MAC addr if not set */ > > dev->enetaddr[0] = 0x02; > > dev->enetaddr[1] = 0x50; > > this is wrong. net drivers should not be touching the env at all. please > fix your driver to not do that first. i guess this whole mac randomization/generation code belongs to board specific files.
On Thursday 03 November 2011 19:02:26 Michael Walle wrote: > Am Donnerstag 03 November 2011, 19:10:57 schrieb Mike Frysinger: > > On Thursday 27 October 2011 17:31:36 Michael Walle wrote: > > > --- a/drivers/net/mvgbe.c > > > +++ b/drivers/net/mvgbe.c > > > > > > + /* Extract the MAC address from the environment */ > > > + while (!eth_getenv_enetaddr_by_index("eth", dev->index, > > > + dev->enetaddr)) { > > > > > > /* Generate Private MAC addr if not set */ > > > dev->enetaddr[0] = 0x02; > > > dev->enetaddr[1] = 0x50; > > > > this is wrong. net drivers should not be touching the env at all. > > please fix your driver to not do that first. > > i guess this whole mac randomization/generation code belongs to board > specific files. correct -mike
> -----Original Message----- > From: Mike Frysinger [mailto:vapier@gentoo.org] > Sent: Friday, November 04, 2011 4:42 AM > To: Michael Walle > Cc: u-boot@lists.denx.de; Prafulla Wadaskar; Wolfgang Denk > Subject: Re: [PATCH 2/2] mvgbe: fix network device indices > > On Thursday 03 November 2011 19:02:26 Michael Walle wrote: > > Am Donnerstag 03 November 2011, 19:10:57 schrieb Mike > Frysinger: > > > On Thursday 27 October 2011 17:31:36 Michael Walle wrote: > > > > --- a/drivers/net/mvgbe.c > > > > +++ b/drivers/net/mvgbe.c > > > > > > > > + /* Extract the MAC address from the environment */ > > > > + while (!eth_getenv_enetaddr_by_index("eth", dev- > >index, > > > > + dev->enetaddr)) { > > > > > > > > /* Generate Private MAC addr if not set */ > > > > dev->enetaddr[0] = 0x02; > > > > dev->enetaddr[1] = 0x50; > > > > > > this is wrong. net drivers should not be touching the env > at all. > > > please fix your driver to not do that first. > > > > i guess this whole mac randomization/generation code belongs > to board > > specific files. > > correct We can move mac randomization code to the board specific files, but it will be needed for each board and there will be code duplication. How about supporting standalone mac randomization feature? Regards.. Prafulla . .
On Friday 04 November 2011 02:29:24 Prafulla Wadaskar wrote: > Mike Frysinger: > > On Thursday 03 November 2011 19:02:26 Michael Walle wrote: > > > Am Donnerstag 03 November 2011, 19:10:57 schrieb Mike Frysinger: > > > > On Thursday 27 October 2011 17:31:36 Michael Walle wrote: > > > > > --- a/drivers/net/mvgbe.c > > > > > +++ b/drivers/net/mvgbe.c > > > > > > > > > > + /* Extract the MAC address from the environment */ > > > > > + while (!eth_getenv_enetaddr_by_index("eth", dev-index, > > > > > + dev->enetaddr)) { > > > > > > > > > > /* Generate Private MAC addr if not set */ > > > > > dev->enetaddr[0] = 0x02; > > > > > dev->enetaddr[1] = 0x50; > > > > > > > > this is wrong. net drivers should not be touching the env > > > > at all. please fix your driver to not do that first. > > > > > > i guess this whole mac randomization/generation code belongs to board > > > specific files. > > > > correct > > We can move mac randomization code to the board specific files, but it will > be needed for each board and there will be code duplication. there's two issues here. (1) no net driver should touch the env. this is why we have the dev->enetaddr field in the first place. (2) drivers should be seeding dev->enetaddr with values from storage directly related to it. so for parts that have dedicated EEPROM interfaces, reading the MAC out of that storage makes sense. if no storage like that exists, then it is up to the board to figure out where to find the address. that means this could should be moved to the boards file. > How about supporting standalone mac randomization feature? i think Wolfgang would be opposed to that. mac randomization should not be the first line of defense. your board is supposed to be managing this sanely. from the mvgbe code, it seems that this is not the case and these boards are a bit insane. -mike
Le 05/11/2011 00:06, Mike Frysinger a écrit : > On Friday 04 November 2011 02:29:24 Prafulla Wadaskar wrote: >> Mike Frysinger: >>> On Thursday 03 November 2011 19:02:26 Michael Walle wrote: >>>> Am Donnerstag 03 November 2011, 19:10:57 schrieb Mike Frysinger: >>>>> On Thursday 27 October 2011 17:31:36 Michael Walle wrote: >>>>>> --- a/drivers/net/mvgbe.c >>>>>> +++ b/drivers/net/mvgbe.c >>>>>> >>>>>> + /* Extract the MAC address from the environment */ >>>>>> + while (!eth_getenv_enetaddr_by_index("eth", dev-index, >>>>>> + dev->enetaddr)) { >>>>>> >>>>>> /* Generate Private MAC addr if not set */ >>>>>> dev->enetaddr[0] = 0x02; >>>>>> dev->enetaddr[1] = 0x50; >>>>> >>>>> this is wrong. net drivers should not be touching the env >>>>> at all. please fix your driver to not do that first. >>>> >>>> i guess this whole mac randomization/generation code belongs to board >>>> specific files. >>> >>> correct >> >> We can move mac randomization code to the board specific files, but it will >> be needed for each board and there will be code duplication. > > there's two issues here. (1) no net driver should touch the env. this is why > we have the dev->enetaddr field in the first place. (2) drivers should be > seeding dev->enetaddr with values from storage directly related to it. so for > parts that have dedicated EEPROM interfaces, reading the MAC out of that > storage makes sense. if no storage like that exists, then it is up to the > board to figure out where to find the address. > > that means this could should be moved to the boards file. > >> How about supporting standalone mac randomization feature? > > i think Wolfgang would be opposed to that. mac randomization should not be > the first line of defense. your board is supposed to be managing this sanely. > from the mvgbe code, it seems that this is not the case and these boards are a > bit insane. Granted, MAC randomization as a feature -- i.e., choosing to use a random MAC *instead* of any other way -- would be Bad(tm). But what about MAC randomization as a function provided by the SoC level to board MAC init code that wants to use it? For instance, a weak MAC setup function provided by the SoC level, and the board level would use it or provide its own. > -mike Amicalement,
Dear Albert ARIBAUD, In message <4EB507B7.9090907@aribaud.net> you wrote: > > But what about MAC randomization as a function provided by the SoC level > to board MAC init code that wants to use it? For instance, a weak MAC > setup function provided by the SoC level, and the board level would use > it or provide its own. What would be the result? A bord that comes up with a new MAC address each time you reset it? Best regards, Wolfgang Denk
Hi Wolfgang, Le 05/11/2011 14:21, Wolfgang Denk a écrit : > Dear Albert ARIBAUD, > > In message<4EB507B7.9090907@aribaud.net> you wrote: >> >> But what about MAC randomization as a function provided by the SoC level >> to board MAC init code that wants to use it? For instance, a weak MAC >> setup function provided by the SoC level, and the board level would use >> it or provide its own. > > What would be the result? A bord that comes up with a new MAC address > each time you reset it? No -- the goal of the randomization code was, is, and will be to allow the board to use the network when no correct MAC address can be found anywhere (env vars, EEPROM, e-fuses, whatever). When a correct address is available, that address will be used. Typically, this happens when the board has not been provisioned yet, at a point where the MAC address it uses is not relevant yet. Notes: 1. This code would only be available to kirkwood-based boards anyway. 2. Although the code incorrectly describes it as "private", the random address is actually a locally administered address (bit 1 of first octet is set), which eliminates the risk of clashing against any 'normal' (universally administered) address; and its last three octets are randomized in order to limit the risk of clashing against other locally administered addresses if we're unlucky enough to have any on the network segment. > Best regards, > > Wolfgang Denk Amicalement,
Dear Albert ARIBAUD, In message <4EB54978.5020301@aribaud.net> you wrote: > > > What would be the result? A bord that comes up with a new MAC address > > each time you reset it? > > No -- the goal of the randomization code was, is, and will be to allow > the board to use the network when no correct MAC address can be found > anywhere (env vars, EEPROM, e-fuses, whatever). When a correct address And if this is the case, then the board will come up with a new MAC address each time you reset it, right? > is available, that address will be used. Typically, this happens when > the board has not been provisioned yet, at a point where the MAC address > it uses is not relevant yet. I've done provisioning stuff a couple of times before, and I'm just doing is again. Random MAC addresses are a broken concept, and anybody who considers using it should reassess his concepts. Where is the real MAC address coming from, and how does it get assigned to this specific board? And how do you make sure not to make mistakes when all you see is some board with a random MAC address? [The systems I know usually either have the MAC address pre-programmed in some storage device on the board, or printed on a barcode label, so you can use ca barcode reader in combination with "askenv" and very little U-Boot scripting as part of your production test / initialization procedures.] > 1. This code would only be available to kirkwood-based boards anyway. That doe snot make things any better. > 2. Although the code incorrectly describes it as "private", the random > address is actually a locally administered address (bit 1 of first octet > is set), which eliminates the risk of clashing against any 'normal' > (universally administered) address; and its last three octets are > randomized in order to limit the risk of clashing against other locally > administered addresses if we're unlucky enough to have any on the > network segment. I consider the whole approach broken and object against it. Best regards, Wolfgang Denk
> -----Original Message----- > From: Mike Frysinger [mailto:vapier@gentoo.org] > Sent: Saturday, November 05, 2011 4:37 AM > To: Prafulla Wadaskar > Cc: Michael Walle; u-boot@lists.denx.de; Wolfgang Denk > Subject: Re: [PATCH 2/2] mvgbe: fix network device indices > > On Friday 04 November 2011 02:29:24 Prafulla Wadaskar wrote: > > Mike Frysinger: > > > On Thursday 03 November 2011 19:02:26 Michael Walle wrote: > > > > Am Donnerstag 03 November 2011, 19:10:57 schrieb Mike > Frysinger: > > > > > On Thursday 27 October 2011 17:31:36 Michael Walle > wrote: > > > > > > --- a/drivers/net/mvgbe.c > > > > > > +++ b/drivers/net/mvgbe.c > > > > > > > > > > > > + /* Extract the MAC address from the environment > */ > > > > > > + while (!eth_getenv_enetaddr_by_index("eth", dev- > index, > > > > > > + dev->enetaddr)) { > > > > > > > > > > > > /* Generate Private MAC addr if not set */ > > > > > > dev->enetaddr[0] = 0x02; > > > > > > dev->enetaddr[1] = 0x50; > > > > > > > > > > this is wrong. net drivers should not be touching the > env > > > > > at all. please fix your driver to not do that first. > > > > > > > > i guess this whole mac randomization/generation code > belongs to board > > > > specific files. > > > > > > correct > > > > We can move mac randomization code to the board specific > files, but it will > > be needed for each board and there will be code duplication. > > there's two issues here. (1) no net driver should touch the > env. this is why > we have the dev->enetaddr field in the first place. (2) > drivers should be > seeding dev->enetaddr with values from storage directly related > to it. so for > parts that have dedicated EEPROM interfaces, reading the MAC > out of that > storage makes sense. if no storage like that exists, then it > is up to the > board to figure out where to find the address. > > that means this could should be moved to the boards file. > > > How about supporting standalone mac randomization feature? > > i think Wolfgang would be opposed to that. mac randomization > should not be > the first line of defense. your board is supposed to be > managing this sanely. > from the mvgbe code, it seems that this is not the case and > these boards are a > bit insane. Hi Mike I had posted feedback for your patch first and seen this email latter. On some boards, using mvbge random mac generation is needed since there is no eeprom to hold this. Further practically it is not possible to hardcode mac address and recompile, nor it is suggested to have any mac/ip/server address definition in board config files. Random mac address helps to resolve dhcp issues with more similar boards in the same network in such cases and will be applicable for any board. Your patch is clean to abstract out mac randomization, on the other hand you should not remove the support for other boards which are already using it. May be some more patches on the top to move support in board/arch specific files are needed. (if framework can not be entertained) Regards.. Prafulla . .
> -----Original Message----- > From: Albert ARIBAUD [mailto:albert.u.boot@aribaud.net] > Sent: Saturday, November 05, 2011 3:24 PM > To: Mike Frysinger > Cc: Prafulla Wadaskar; u-boot@lists.denx.de > Subject: Re: [U-Boot] [PATCH 2/2] mvgbe: fix network device > indices > > Le 05/11/2011 00:06, Mike Frysinger a écrit : > > On Friday 04 November 2011 02:29:24 Prafulla Wadaskar wrote: > >> Mike Frysinger: > >>> On Thursday 03 November 2011 19:02:26 Michael Walle wrote: > >>>> Am Donnerstag 03 November 2011, 19:10:57 schrieb Mike > Frysinger: > >>>>> On Thursday 27 October 2011 17:31:36 Michael Walle wrote: > >>>>>> --- a/drivers/net/mvgbe.c > >>>>>> +++ b/drivers/net/mvgbe.c > >>>>>> > >>>>>> + /* Extract the MAC address from the environment > */ > >>>>>> + while (!eth_getenv_enetaddr_by_index("eth", dev- > index, > >>>>>> + dev->enetaddr)) { > >>>>>> > >>>>>> /* Generate Private MAC addr if not set */ > >>>>>> dev->enetaddr[0] = 0x02; > >>>>>> dev->enetaddr[1] = 0x50; > >>>>> > >>>>> this is wrong. net drivers should not be touching the > env > >>>>> at all. please fix your driver to not do that first. > >>>> > >>>> i guess this whole mac randomization/generation code > belongs to board > >>>> specific files. > >>> > >>> correct > >> > >> We can move mac randomization code to the board specific > files, but it will > >> be needed for each board and there will be code duplication. > > > > there's two issues here. (1) no net driver should touch the > env. this is why > > we have the dev->enetaddr field in the first place. (2) > drivers should be > > seeding dev->enetaddr with values from storage directly > related to it. so for > > parts that have dedicated EEPROM interfaces, reading the MAC > out of that > > storage makes sense. if no storage like that exists, then it > is up to the > > board to figure out where to find the address. > > > > that means this could should be moved to the boards file. > > > >> How about supporting standalone mac randomization feature? > > > > i think Wolfgang would be opposed to that. mac randomization > should not be > > the first line of defense. your board is supposed to be > managing this sanely. > > from the mvgbe code, it seems that this is not the case and > these boards are a > > bit insane. > > Granted, MAC randomization as a feature -- i.e., choosing to > use a > random MAC *instead* of any other way -- would be Bad(tm). > > But what about MAC randomization as a function provided by the > SoC level > to board MAC init code that wants to use it? For instance, a > weak MAC > setup function provided by the SoC level, and the board level > would use > it or provide its own. I ack for this method Regards.. Prafulla . .
On Tuesday 08 November 2011 02:32:59 Prafulla Wadaskar wrote: > On some boards, using mvbge random mac generation is needed since there is > no eeprom to hold this. Further practically it is not possible to hardcode > mac address and recompile, nor it is suggested to have any mac/ip/server > address definition in board config files. > > Random mac address helps to resolve dhcp issues with more similar boards in > the same network in such cases and will be applicable for any board. > > Your patch is clean to abstract out mac randomization, on the other hand > you should not remove the support for other boards which are already using > it. > > May be some more patches on the top to move support in board/arch specific > files are needed. (if framework can not be entertained) i'm not the one to convince. i think Wolfgang addressed these already and still said no. -mike
diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c index c701f43..80314ec 100644 --- a/drivers/net/mvgbe.c +++ b/drivers/net/mvgbe.c @@ -645,7 +645,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++) { @@ -700,16 +699,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 */ @@ -718,7 +714,17 @@ error1: return -1; } - while (!eth_getenv_enetaddr(s, dev->enetaddr)) { + dev->init = (void *)mvgbe_init; + dev->halt = (void *)mvgbe_halt; + dev->send = (void *)mvgbe_send; + dev->recv = (void *)mvgbe_recv; + dev->write_hwaddr = (void *)mvgbe_write_hwaddr; + + eth_register(dev); + + /* Extract the MAC address from the environment */ + while (!eth_getenv_enetaddr_by_index("eth", dev->index, + dev->enetaddr)) { /* Generate Private MAC addr if not set */ dev->enetaddr[0] = 0x02; dev->enetaddr[1] = 0x50; @@ -734,17 +740,10 @@ error1: dev->enetaddr[4] = get_random_hex(); dev->enetaddr[5] = get_random_hex(); #endif - eth_setenv_enetaddr(s, dev->enetaddr); + eth_setenv_enetaddr_by_index("eth", dev->index, + dev->enetaddr); } - dev->init = (void *)mvgbe_init; - dev->halt = (void *)mvgbe_halt; - dev->send = (void *)mvgbe_send; - dev->recv = (void *)mvgbe_recv; - dev->write_hwaddr = (void *)mvgbe_write_hwaddr; - - eth_register(dev); - #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) miiphy_register(dev->name, smi_reg_read, smi_reg_write); /* Set phy address of the port */ diff --git a/include/net.h b/include/net.h index 7f9b1d1..b841f85 100644 --- a/include/net.h +++ b/include/net.h @@ -117,6 +117,18 @@ extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); extern int eth_getenv_enetaddr_by_index(const char *base_name, int index, uchar *enetaddr); +/* + * Set the hardware address for an ethernet interface. + * Args: + * base_name - base name for device (normally "eth") + * index - device index number (0 for first) + * enetaddr - returns 6 byte hardware address + * Returns: + * Return true if the environment varibable was set successfully. + */ +extern int eth_setenv_enetaddr_by_index(const char *base_name, int index, + const uchar *enetaddr); + extern int usb_eth_initialize(bd_t *bi); extern int eth_init(bd_t *bis); /* Initialize the device */ extern int eth_send(volatile void *packet, int length); /* Send a packet */ diff --git a/net/eth.c b/net/eth.c index b4b9b43..baa6ded 100644 --- a/net/eth.c +++ b/net/eth.c @@ -54,14 +54,29 @@ int eth_setenv_enetaddr(char *name, const uchar *enetaddr) return setenv(name, buf); } +static void eth_get_enetaddr_env_name(char *buf, const char *base_name, + int index) +{ + sprintf(buf, index ? "%s%daddr" : "%saddr", base_name, index); + return buf; +} + int eth_getenv_enetaddr_by_index(const char *base_name, int index, uchar *enetaddr) { char enetvar[32]; - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + eth_get_enetaddr_env_name(enetvar, base_name, index); return eth_getenv_enetaddr(enetvar, enetaddr); } +int eth_setenv_enetaddr_by_index(const char *base_name, int index, + const uchar *enetaddr) +{ + char enetvar[32]; + eth_get_enetaddr_env_name(enetvar, base_name, index); + return eth_setenv_enetaddr(enetvar, enetaddr); +} + static int eth_mac_skip(int index) { char enetvar[15];
Don't assume that the MAC address of egiga0 rsp. egiga1 is ethaddr rsp. eth1addr. If there is only a egiga1 device, u-boot will enumerate it as device 0. Instead use the correct index number stored within the eth_device structure. Signed-off-by: Michael Walle <michael@walle.cc> Cc: Prafulla Wadaskar <prafulla@marvell.com> --- drivers/net/mvgbe.c | 27 +++++++++++++-------------- include/net.h | 12 ++++++++++++ net/eth.c | 17 ++++++++++++++++- 3 files changed, 41 insertions(+), 15 deletions(-)