[U-Boot] [PATCH] mvgbe: fix network device indices

[U-Boot] [PATCH] mvgbe: fix network device indices

[Michael Walle]

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 and therefore the MAC address is set with the environmen varibale
ethaddr.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/mvgbe.c |   13 +++++++------
 include/net.h       |   12 ++++++++++++
 net/eth.c           |    8 ++++++++
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c
index c701f43..738e8d3 100644
--- a/drivers/net/mvgbe.c
+++ b/drivers/net/mvgbe.c
@@ -645,7 +645,7 @@ int mvgbe_initialize(bd_t *bis)
 	struct mvgbe_device *dmvgbe;
 	struct eth_device *dev;
 	int devnum;
-	char *s;
+	int eth_idx = 0;
 	u8 used_ports[MAX_MVGBE_DEVS] = CONFIG_MVGBE_PORTS;
 
 	for (devnum = 0; devnum < MAX_MVGBE_DEVS; devnum++) {
@@ -700,16 +700,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 +715,9 @@ error1:
 			return -1;
 		}
 
-		while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
+		/* Extract the MAC address from the environment */
+		while (!eth_getenv_enetaddr_by_index("eth", eth_idx,
+					dev->enetaddr)) {
 			/* Generate Private MAC addr if not set */
 			dev->enetaddr[0] = 0x02;
 			dev->enetaddr[1] = 0x50;
@@ -734,7 +733,7 @@ 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", eth_idx, dev->enetaddr);
 		}
 
 		dev->init = (void *)mvgbe_init;
@@ -745,6 +744,8 @@ error1:
 
 		eth_register(dev);
 
+		eth_idx++;
+
 #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 d5d37b6..d378cd2 100644
--- a/include/net.h
+++ b/include/net.h
@@ -103,6 +103,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 4280d6d..a8f68fc 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -62,6 +62,14 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int 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];
+	sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
+	return eth_setenv_enetaddr(enetvar, enetaddr);
+}
+
 static int eth_mac_skip(int index)
 {
 	char enetvar[15];
-- 
1.7.2.5

[U-Boot] [PATCH] mvgbe: fix network device indices

[Prafulla Wadaskar]

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
> On Behalf Of Michael Walle
> Sent: Friday, October 07, 2011 3:53 AM
> To: u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH] mvgbe: fix network device indices
> 
> 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 and therefore the MAC address is set with the environmen
> varibale
> ethaddr.

So if there is only one eth port available, the associated environment variable should be ethaddr, this is understood.
Is there any issue with this?

Then I have anther question in my mind-
Why not we name eth0addr for environment variable associated with egiga0 to maintain consistency with naming and used ports?

Regards..
Prafulla . .

[U-Boot] [PATCH] mvgbe: fix network device indices

[Michael Walle]

Am Fr, 7.10.2011, 10:26 schrieb Prafulla Wadaskar:
>> -----Original Message-----
>> From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
>> On Behalf Of Michael Walle
>> Sent: Friday, October 07, 2011 3:53 AM
>> To: u-boot at lists.denx.de
>> Subject: [U-Boot] [PATCH] mvgbe: fix network device indices
>>
>> 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 and therefore the MAC address is set with the environmen
>> varibale
>> ethaddr.
>
> So if there is only one eth port available, the associated environment
> variable should be ethaddr, this is understood.
> Is there any issue with this?
Are you asking me or the list? (if the question was for me, remember that
there might be _only_ the egiga1 device.)

> Then I have anther question in my mind-
> Why not we name eth0addr for environment variable associated with egiga0
> to maintain consistency with naming and used ports?

If i get it right, there is another problem with setting the environment
variable from the driver itself.

eth_register() and eth_init() using an own index to enumerate the devices.
Eg. if an e1000 is registered before the the associated environment
variable would be eth1addr for the first mvgbe device and eth2addr for the
second.

This patch only fixes the case where no other network device is
registered. (The current behaviour just works with egiga0 being the only
or the first and egiga1 being the second device).

-- 
Michael

[U-Boot] [PATCH] mvgbe: fix network device indices

[Mike Frysinger]

On Thursday 06 October 2011 18:23:22 Michael Walle wrote:
> --- a/net/eth.c
> +++ b/net/eth.c
> 
> +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> +				 const uchar *enetaddr)
> +{
> +	char enetvar[32];
> +	sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> +	return eth_setenv_enetaddr(enetvar, enetaddr);
> +}

please unify the duplicate logic coming from eth_getenv_enetaddr_by_index in a 
new local static function
-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/20111007/9420fe8d/attachment.pgp 

[U-Boot] [PATCH] mvgbe: fix network device indices

[Michael Walle]

Am Freitag 07 Oktober 2011, 12:48:40 schrieb Michael Walle:
> Am Fr, 7.10.2011, 10:26 schrieb Prafulla Wadaskar:
> >> -----Original Message-----
> >> From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
> >> On Behalf Of Michael Walle
> >> Sent: Friday, October 07, 2011 3:53 AM
> >> To: u-boot at lists.denx.de
> >> Subject: [U-Boot] [PATCH] mvgbe: fix network device indices
> >> 
> >> 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 and therefore the MAC address is set with the environmen
> >> varibale
> >> ethaddr.
> > 
> > So if there is only one eth port available, the associated environment
> > variable should be ethaddr, this is understood.
> > Is there any issue with this?
> 
> Are you asking me or the list? (if the question was for me, remember that
> there might be _only_ the egiga1 device.)
> 
> > Then I have anther question in my mind-
> > Why not we name eth0addr for environment variable associated with egiga0
> > to maintain consistency with naming and used ports?
> 
> If i get it right, there is another problem with setting the environment
> variable from the driver itself.
> 
> eth_register() and eth_init() using an own index to enumerate the devices.
> Eg. if an e1000 is registered before the the associated environment
> variable would be eth1addr for the first mvgbe device and eth2addr for the
> second.
> 
> This patch only fixes the case where no other network device is
> registered. (The current behaviour just works with egiga0 being the only
> or the first and egiga1 being the second device).

ping :)

-- 
Michael

[U-Boot] [PATCH] mvgbe: fix network device indices

[Prafulla Wadaskar]

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
> On Behalf Of Michael Walle
> Sent: Friday, October 07, 2011 3:53 AM
> To: u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH] mvgbe: fix network device indices
> 
> 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 and therefore the MAC address is set with the environment
> varibale
> ethaddr.

Hi

If I understood it correctly,
In current implementation, if only egiga1 device is enabled on the board, then it will assign MAC address associated with environment variable "ethaddr".

This patch will make environment variable "egiga1" available in such case. Right?

If so, then this is not the case with only mvgbe, it is applicable for all network drivers.

I think this is okay, are there any issues following this standard structure at your end?

Regards..
Prafulla . . .

[U-Boot] [PATCH] mvgbe: fix network device indices

[Michael Walle]

Am Freitag 21 Oktober 2011, 10:09:15 schrieben Sie:
> > -----Original Message-----
> > From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
> > On Behalf Of Michael Walle
> > Sent: Friday, October 07, 2011 3:53 AM
> > To: u-boot at lists.denx.de
> > Subject: [U-Boot] [PATCH] mvgbe: fix network device indices
> > 
> > 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 and therefore the MAC address is set with the environment
> > varibale
> > ethaddr.
> 
> Hi
> 
> If I understood it correctly,
> In current implementation, if only egiga1 device is enabled on the board,
> then it will assign MAC address associated with environment variable
> "ethaddr".
yes but the current mvgbe driver will check eth1addr and set it to a random 
value if not set.

> This patch will make environment variable "egiga1" available in such case.
> Right?
mh? This patch will use the same enumeration as the net/eth.c code in 
eth_initialize(). At least if there is no other ethernet driver than mvgbe.
So ethaddr is set to a random value instead of eth1addr, which 
eth_initialize() then use the set the mac address.

> If so, then this is not the case with only mvgbe, it is applicable for all
> network drivers.
yeah other drivers also set eth(N)addr sometimes, which suffers from the same 
problem. maybe a driver could provide some callback to initialize a macaddress 
or eth_register returns the device index which in turn could be used to get 
the proper ethNaddr environment variable.

-- 
Michael

[U-Boot] [PATCH] mvgbe: fix network device indices

[Prafulla Wadaskar]

> -----Original Message-----
> From: Michael Walle [mailto:michael at walle.cc]
> Sent: Wednesday, October 26, 2011 2:41 AM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Mike Frysinger
> Subject: Re: [U-Boot] [PATCH] mvgbe: fix network device indices
> 
> Am Freitag 21 Oktober 2011, 10:09:15 schrieben Sie:
> > > -----Original Message-----
> > > From: u-boot-bounces at lists.denx.de [mailto:u-boot-
> bounces at lists.denx.de]
> > > On Behalf Of Michael Walle
> > > Sent: Friday, October 07, 2011 3:53 AM
> > > To: u-boot at lists.denx.de
> > > Subject: [U-Boot] [PATCH] mvgbe: fix network device indices
> > >
> > > 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 and therefore the MAC address is set with the
> environment
> > > varibale
> > > ethaddr.
> >
> > Hi
> >
> > If I understood it correctly,
> > In current implementation, if only egiga1 device is enabled
> on the board,
> > then it will assign MAC address associated with environment
> variable
> > "ethaddr".
> yes but the current mvgbe driver will check eth1addr and set it
> to a random
> value if not set.

Yes, but this may be avoided by defining  CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION as done for edminiv2 board.

> 
> > This patch will make environment variable "egiga1" available
> in such case.
> > Right?
> mh? This patch will use the same enumeration as the net/eth.c
> code in
> eth_initialize(). At least if there is no other ethernet driver
> than mvgbe.
> So ethaddr is set to a random value instead of eth1addr, which
> eth_initialize() then use the set the mac address.
> 
> > If so, then this is not the case with only mvgbe, it is
> applicable for all
> > network drivers.
> yeah other drivers also set eth(N)addr sometimes, which suffers
> from the same
> problem. maybe a driver could provide some callback to
> initialize a macaddress
> or eth_register returns the device index which in turn could be
> used to get
> the proper ethNaddr environment variable.

I think right place to provide solution for this problem is net/eth.c.
The suggested change is logical but doing this will affect all u-boot users. May be some more opinion on this will be helpful.

Regards..
Prafulla . . .

> 
> --
> Michael

[U-Boot] [PATCH] mvgbe: fix network device indices

[Michael Walle]

Am Do, 27.10.2011, 11:12 schrieb Prafulla Wadaskar:
>> > If I understood it correctly,
>> > In current implementation, if only egiga1 device is enabled
>> on the board,
>> > then it will assign MAC address associated with environment
>> variable
>> > "ethaddr".
>> yes but the current mvgbe driver will check eth1addr and set it
>> to a random
>> value if not set.
>
> Yes, but this may be avoided by defining
> CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION as done for edminiv2 board.
Ok, but that is not my problem ;) And even if this macro is set, the wrong
environment variable will still be set (in this case not a randomized mac
address, but :00:00:<devnum> suffix).

>> > This patch will make environment variable "egiga1" available
>> in such case.
>> > Right?
>> mh? This patch will use the same enumeration as the net/eth.c
>> code in
>> eth_initialize(). At least if there is no other ethernet driver
>> than mvgbe.
>> So ethaddr is set to a random value instead of eth1addr, which
>> eth_initialize() then use the set the mac address.
>>
>> > If so, then this is not the case with only mvgbe, it is
>> applicable for all
>> > network drivers.
>> yeah other drivers also set eth(N)addr sometimes, which suffers
>> from the same
>> problem. maybe a driver could provide some callback to
>> initialize a macaddress
>> or eth_register returns the device index which in turn could be
>> used to get
>> the proper ethNaddr environment variable.
>
> I think right place to provide solution for this problem is net/eth.c.
> The suggested change is logical but doing this will affect all u-boot
> users. May be some more opinion on this will be helpful.

ok, so the second option still applies, that is eth_register() returns a
device index. the current eth_register always returns zero, in fact all
drivers in the uboot source don't check the return code. so here we could
return the device index.

@wolfgang: any opinion on that? or any other idea how to pass the ethernet
device number to a network driver?

-- 
Michael

[U-Boot] [PATCH 0/2] improve ethernet device index handling

[Michael Walle]

This patchset introduces a new element "index" within the eth_device
structure, which is populated by eth_register(). The second patch uses this
element to get the right name of the environment variable for the ethernet
hardware address.

[U-Boot] [PATCH 1/2] net: introduce per device index

[Michael Walle]

Instead of counting the device index everytime a functions needs it, store
it in the eth_device struct. eth_register() keeps track of the indices and
updates the device's index number. This simplifies some functions in
net/eth.c.

Additionally, a network driver can now query its index, eg. to get the
correct environment ethaddr name.

Signed-off-by: Michael Walle <michael@walle.cc>
Cc: Prafulla Wadaskar <prafulla@marvell.com>
Cc: Mike Frysinger <vapier@gentoo.com>
Cc: Wolfgang Denk <wd@denx.de>
---
 include/net.h |    1 +
 net/eth.c     |   41 ++++++++++++-----------------------------
 2 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/include/net.h b/include/net.h
index b408dea..7f9b1d1 100644
--- a/include/net.h
+++ b/include/net.h
@@ -89,6 +89,7 @@ struct eth_device {
 #endif
 	int  (*write_hwaddr) (struct eth_device*);
 	struct eth_device *next;
+	int index;
 	void *priv;
 };
 
diff --git a/net/eth.c b/net/eth.c
index 4280d6d..b4b9b43 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -127,7 +127,6 @@ struct eth_device *eth_get_dev_by_name(const char *devname)
 struct eth_device *eth_get_dev_by_index(int index)
 {
 	struct eth_device *dev, *target_dev;
-	int idx = 0;
 
 	if (!eth_devices)
 		return NULL;
@@ -135,12 +134,11 @@ struct eth_device *eth_get_dev_by_index(int index)
 	dev = eth_devices;
 	target_dev = NULL;
 	do {
-		if (idx == index) {
+		if (dev->index == index) {
 			target_dev = dev;
 			break;
 		}
 		dev = dev->next;
-		idx++;
 	} while (dev != eth_devices);
 
 	return target_dev;
@@ -148,24 +146,11 @@ struct eth_device *eth_get_dev_by_index(int index)
 
 int eth_get_dev_index (void)
 {
-	struct eth_device *dev;
-	int num = 0;
-
-	if (!eth_devices) {
-		return (-1);
-	}
-
-	for (dev = eth_devices; dev; dev = dev->next) {
-		if (dev == eth_current)
-			break;
-		++num;
-	}
-
-	if (dev) {
-		return (num);
+	if (!eth_current) {
+		return -1;
 	}
 
-	return (0);
+	return eth_current->index;
 }
 
 static void eth_current_changed(void)
@@ -219,6 +204,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 int eth_register(struct eth_device *dev)
 {
 	struct eth_device *d;
+	static int index = 0;
 
 	assert(strlen(dev->name) < NAMESIZE);
 
@@ -233,14 +219,14 @@ int eth_register(struct eth_device *dev)
 
 	dev->state = ETH_STATE_INIT;
 	dev->next  = eth_devices;
+	dev->index = index++;
 
 	return 0;
 }
 
 int eth_initialize(bd_t *bis)
 {
-	int eth_number = 0;
-
+	int num_devices = 0;
 	eth_devices = NULL;
 	eth_current = NULL;
 
@@ -281,7 +267,7 @@ int eth_initialize(bd_t *bis)
 
 		show_boot_progress (65);
 		do {
-			if (eth_number)
+			if (dev->index)
 				puts (", ");
 
 			printf("%s", dev->name);
@@ -294,18 +280,18 @@ int eth_initialize(bd_t *bis)
 			if (strchr(dev->name, ' '))
 				puts("\nWarning: eth device name has a space!\n");
 
-			if (eth_write_hwaddr(dev, "eth", eth_number))
+			if (eth_write_hwaddr(dev, "eth", dev->index))
 				puts("\nWarning: failed to set MAC address\n");
 
-			eth_number++;
 			dev = dev->next;
+			num_devices++;
 		} while(dev != eth_devices);
 
 		eth_current_changed();
 		putc ('\n');
 	}
 
-	return eth_number;
+	return num_devices;
 }
 
 #ifdef CONFIG_MCAST_TFTP
@@ -356,7 +342,6 @@ u32 ether_crc (size_t len, unsigned char const *p)
 
 int eth_init(bd_t *bis)
 {
-	int eth_number;
 	struct eth_device *old_current, *dev;
 
 	if (!eth_current) {
@@ -365,16 +350,14 @@ int eth_init(bd_t *bis)
 	}
 
 	/* Sync environment with network devices */
-	eth_number = 0;
 	dev = eth_devices;
 	do {
 		uchar env_enetaddr[6];
 
-		if (eth_getenv_enetaddr_by_index("eth", eth_number,
+		if (eth_getenv_enetaddr_by_index("eth", dev->index,
 						 env_enetaddr))
 			memcpy(dev->enetaddr, env_enetaddr, 6);
 
-		++eth_number;
 		dev = dev->next;
 	} while (dev != eth_devices);
 
-- 
1.7.2.5

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Michael Walle]

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(-)

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];
-- 
1.7.2.5

[U-Boot] [PATCH 1/2] net: introduce per device index

[Michael Walle]

Am Donnerstag 27 Oktober 2011, 23:31:35 schrieb Michael Walle:
> Instead of counting the device index everytime a functions needs it, store
> it in the eth_device struct. eth_register() keeps track of the indices and
> updates the device's index number. This simplifies some functions in
> net/eth.c.
> 
> Additionally, a network driver can now query its index, eg. to get the
> correct environment ethaddr name.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>
> Cc: Mike Frysinger <vapier@gentoo.com>
sorry mike, i'll fix this for v2 ;)

-- 
Michael

[U-Boot] [PATCH 1/2] net: introduce per device index

[Michael Walle]

Am Do, 27.10.2011, 22:31 schrieb Michael Walle:
> Instead of counting the device index everytime a functions needs it, store
> it in the eth_device struct. eth_register() keeps track of the indices and
> updates the device's index number. This simplifies some functions in
> net/eth.c.
>
> Additionally, a network driver can now query its index, eg. to get the
> correct environment ethaddr name.

Ping, anyone?

> Signed-off-by: Michael Walle <michael@walle.cc>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>
> Cc: Mike Frysinger <vapier@gentoo.com>
> Cc: Wolfgang Denk <wd@denx.de>
> ---
>  include/net.h |    1 +
>  net/eth.c     |   41 ++++++++++++-----------------------------
>  2 files changed, 13 insertions(+), 29 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index b408dea..7f9b1d1 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -89,6 +89,7 @@ struct eth_device {
>  #endif
>  	int  (*write_hwaddr) (struct eth_device*);
>  	struct eth_device *next;
> +	int index;
>  	void *priv;
>  };
>
> diff --git a/net/eth.c b/net/eth.c
> index 4280d6d..b4b9b43 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -127,7 +127,6 @@ struct eth_device *eth_get_dev_by_name(const char
> *devname)
>  struct eth_device *eth_get_dev_by_index(int index)
>  {
>  	struct eth_device *dev, *target_dev;
> -	int idx = 0;
>
>  	if (!eth_devices)
>  		return NULL;
> @@ -135,12 +134,11 @@ struct eth_device *eth_get_dev_by_index(int index)
>  	dev = eth_devices;
>  	target_dev = NULL;
>  	do {
> -		if (idx == index) {
> +		if (dev->index == index) {
>  			target_dev = dev;
>  			break;
>  		}
>  		dev = dev->next;
> -		idx++;
>  	} while (dev != eth_devices);
>
>  	return target_dev;
> @@ -148,24 +146,11 @@ struct eth_device *eth_get_dev_by_index(int index)
>
>  int eth_get_dev_index (void)
>  {
> -	struct eth_device *dev;
> -	int num = 0;
> -
> -	if (!eth_devices) {
> -		return (-1);
> -	}
> -
> -	for (dev = eth_devices; dev; dev = dev->next) {
> -		if (dev == eth_current)
> -			break;
> -		++num;
> -	}
> -
> -	if (dev) {
> -		return (num);
> +	if (!eth_current) {
> +		return -1;
>  	}
>
> -	return (0);
> +	return eth_current->index;
>  }
>
>  static void eth_current_changed(void)
> @@ -219,6 +204,7 @@ int eth_write_hwaddr(struct eth_device *dev, const
> char *base_name,
>  int eth_register(struct eth_device *dev)
>  {
>  	struct eth_device *d;
> +	static int index = 0;
>
>  	assert(strlen(dev->name) < NAMESIZE);
>
> @@ -233,14 +219,14 @@ int eth_register(struct eth_device *dev)
>
>  	dev->state = ETH_STATE_INIT;
>  	dev->next  = eth_devices;
> +	dev->index = index++;
>
>  	return 0;
>  }
>
>  int eth_initialize(bd_t *bis)
>  {
> -	int eth_number = 0;
> -
> +	int num_devices = 0;
>  	eth_devices = NULL;
>  	eth_current = NULL;
>
> @@ -281,7 +267,7 @@ int eth_initialize(bd_t *bis)
>
>  		show_boot_progress (65);
>  		do {
> -			if (eth_number)
> +			if (dev->index)
>  				puts (", ");
>
>  			printf("%s", dev->name);
> @@ -294,18 +280,18 @@ int eth_initialize(bd_t *bis)
>  			if (strchr(dev->name, ' '))
>  				puts("\nWarning: eth device name has a space!\n");
>
> -			if (eth_write_hwaddr(dev, "eth", eth_number))
> +			if (eth_write_hwaddr(dev, "eth", dev->index))
>  				puts("\nWarning: failed to set MAC address\n");
>
> -			eth_number++;
>  			dev = dev->next;
> +			num_devices++;
>  		} while(dev != eth_devices);
>
>  		eth_current_changed();
>  		putc ('\n');
>  	}
>
> -	return eth_number;
> +	return num_devices;
>  }
>
>  #ifdef CONFIG_MCAST_TFTP
> @@ -356,7 +342,6 @@ u32 ether_crc (size_t len, unsigned char const *p)
>
>  int eth_init(bd_t *bis)
>  {
> -	int eth_number;
>  	struct eth_device *old_current, *dev;
>
>  	if (!eth_current) {
> @@ -365,16 +350,14 @@ int eth_init(bd_t *bis)
>  	}
>
>  	/* Sync environment with network devices */
> -	eth_number = 0;
>  	dev = eth_devices;
>  	do {
>  		uchar env_enetaddr[6];
>
> -		if (eth_getenv_enetaddr_by_index("eth", eth_number,
> +		if (eth_getenv_enetaddr_by_index("eth", dev->index,
>  						 env_enetaddr))
>  			memcpy(dev->enetaddr, env_enetaddr, 6);
>
> -		++eth_number;
>  		dev = dev->next;
>  	} while (dev != eth_devices);
>
> --
> 1.7.2.5
>
>

[U-Boot] [PATCH 1/2] net: introduce per device index

[Prafulla Wadaskar]

> -----Original Message-----
> From: Michael Walle [mailto:michael at walle.cc]
> Sent: Thursday, November 03, 2011 4:54 PM
> To: Michael Walle
> Cc: u-boot at lists.denx.de; Prafulla Wadaskar; Mike Frysinger;
> Wolfgang Denk; Michael Walle
> Subject: Re: [PATCH 1/2] net: introduce per device index
> 
> Am Do, 27.10.2011, 22:31 schrieb Michael Walle:
> > Instead of counting the device index everytime a functions
> needs it, store
> > it in the eth_device struct. eth_register() keeps track of
> the indices and
> > updates the device's index number. This simplifies some
> functions in
> > net/eth.c.
> >
> > Additionally, a network driver can now query its index, eg.
> to get the
> > correct environment ethaddr name.
> 
> Ping, anyone?
> 
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > Cc: Prafulla Wadaskar <prafulla@marvell.com>
> > Cc: Mike Frysinger <vapier@gentoo.com>
> > Cc: Wolfgang Denk <wd@denx.de>
> > ---
> >  include/net.h |    1 +
> >  net/eth.c     |   41 ++++++++++++---------------------------

These patches are for u-boot-net.git.
Copying Ben on this.

Regards..
Prafulla . .

[U-Boot] [PATCH 1/2] net: introduce per device index

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <F766E4F80769BD478052FB6533FA745D1A14DD9299@SC-VEXCH4.marvell.com> you wrote:
> 
> These patches are for u-boot-net.git.
> Copying Ben on this.

Ben has resigned long ago.  We're currently without Net custodian (any
volunteers out there??) so I will have to pick that up - when time
comes.

The patch was submitted after close of trhe merge window, so it is low
priority for now.

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
Minds are like parachutes - they only function when open.

[U-Boot] [PATCH 1/2] net: introduce per device index

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-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/20111103/4203f9b2/attachment.pgp 

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[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.
-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/20111103/2bd724db/attachment.pgp 

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Michael Walle]

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.

-- 
Michael

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[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
-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/20111103/dbdf1c96/attachment.pgp 

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Prafulla Wadaskar]

> -----Original Message-----
> From: Mike Frysinger [mailto:vapier at gentoo.org]
> Sent: Friday, November 04, 2011 4:42 AM
> To: Michael Walle
> Cc: u-boot at 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 . .

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Mike Frysinger]

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
-------------- 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/20111104/dfabf98a/attachment.pgp 

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Albert ARIBAUD]

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

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Wolfgang Denk]

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

-- 
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
No journaling file system can recover your data if the disk dies.
                                 - Steve Rago in <D4Cw1p.L9E@plc.com>

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Albert ARIBAUD]

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

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Wolfgang Denk]

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

-- 
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
Perfection is reached, not when there is no longer anything  to  add,
but when there is no longer anything to take away.
                                           - Antoine de Saint-Exupery

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Prafulla Wadaskar]

> -----Original Message-----
> From: Mike Frysinger [mailto:vapier at gentoo.org]
> Sent: Saturday, November 05, 2011 4:37 AM
> To: Prafulla Wadaskar
> Cc: Michael Walle; u-boot at 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 . .

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Prafulla Wadaskar]

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> Sent: Saturday, November 05, 2011 3:24 PM
> To: Mike Frysinger
> Cc: Prafulla Wadaskar; u-boot at 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 . .

[U-Boot] [PATCH 2/2] mvgbe: fix network device indices

[Mike Frysinger]

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
-------------- 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/93a1f23c/attachment.pgp