Some cheap devices ship with dangling EEPROM pins!
They always return invalid address ff:ff:ff:ff:ff:ff.

Inherit the auto-generated address in this case,
so that these products can work with zero configuration.

Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 drivers/net/usb/dm9601.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--- linux-2.6.orig/drivers/net/usb/dm9601.c
+++ linux-2.6/drivers/net/usb/dm9601.c
@@ -413,6 +413,7 @@ static int dm9601_set_mac_address(struct
 static int dm9601_bind(struct usbnet *dev, struct usb_interface *intf)
 {
 	int ret;
+	u8 mac[ETH_ALEN];
 
 	ret = usbnet_get_endpoints(dev, intf);
 	if (ret)
@@ -437,12 +438,18 @@ static int dm9601_bind(struct usbnet *de
 	udelay(20);
 
 	/* read MAC */
-	if (dm_read(dev, DM_PHY_ADDR, ETH_ALEN, dev->net->dev_addr) < 0) {
+	if (dm_read(dev, DM_PHY_ADDR, ETH_ALEN, mac) < 0) {
 		printk(KERN_ERR "Error reading MAC address\n");
 		ret = -ENODEV;
 		goto out;
 	}
 
+	/*
+	 * Overwrite the auto-generated address only with good ones.
+	 */
+	if (is_valid_ether_addr(mac))
+		memcpy(dev->net->dev_addr, mac, ETH_ALEN);
+
 	/* power up phy */
 	dm_write_reg(dev, DM_GPR_CTRL, 1);
 	dm_write_reg(dev, DM_GPR_DATA, 0);
--

[ message continues ]

>>>>> "Wu" == Wu Fengguang <wfg@linux.intel.com> writes:

 Wu> Some cheap devices ship with dangling EEPROM pins!
 Wu> They always return invalid address ff:ff:ff:ff:ff:ff.

 Wu> Inherit the auto-generated address in this case,
 Wu> so that these products can work with zero configuration.

 Wu> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>

 Wu> +	/*
 Wu> +	 * Overwrite the auto-generated address only with good ones.
 Wu> +	 */
 Wu> +	if (is_valid_ether_addr(mac))
 Wu> +		memcpy(dev->net->dev_addr, mac, ETH_ALEN);
 Wu> +

Do we automatically get a random address in netdev nowadays without
calling random_ether_addr? I didn't know that.

Anyway, I would prefer to add a dev_warn mentioning the fact that
we're using a random address (and which one).

Other than that,

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

-- 
Bye, Peter Korsgaard
--

[ message continues ]

Hi Peter,


I confirmed based on both code review and tests.
The logic goes like this in usbnet.c:

  80 // randomly generated ethernet address
  81 static u8       node_id [ETH_ALEN];
...
1118 int
1119 usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
1120 {
...
1168         dev->net = net;
1169         strcpy (net->name, "usb%d");
1170         memcpy (net->dev_addr, node_id, sizeof node_id);
...
1192         // allow device-specific bind/init procedures
1193         // NOTE net->name still not usable ...
1194         if (info->bind) {

I'll do that in another patch. Since I'd like to add one more similar
warning to dm9601_set_mac_address(). That warning helped me identify

Thanks,
Fengguang
--

[ message continues ]

>>>>> "Wu" == Wu Fengguang <wfg@linux.intel.com> writes:

Hi,

 >> Do we automatically get a random address in netdev nowadays without
 >> calling random_ether_addr? I didn't know that.

 Wu> I confirmed based on both code review and tests.
 Wu> The logic goes like this in usbnet.c:

 Wu>   80 // randomly generated ethernet address
 Wu>   81 static u8       node_id [ETH_ALEN];
 Wu> ...
 Wu> 1118 int
 Wu> 1119 usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 Wu> 1120 {
 Wu> ...
 Wu> 1168         dev->net = net;
 Wu> 1169         strcpy (net->name, "usb%d");
 Wu> 1170         memcpy (net->dev_addr, node_id, sizeof node_id);
 Wu> ...

So what happens if you plug in 2 devices without an EEPROM? Do they
get the same MAC address? That seems broken.

 >> Anyway, I would prefer to add a dev_warn mentioning the fact that
 >> we're using a random address (and which one).

 Wu> I'll do that in another patch. Since I'd like to add one more similar
 Wu> warning to dm9601_set_mac_address(). That warning helped me identify
 Wu> this bug.

Ok.

-- 
Bye, Peter Korsgaard
--

[ message continues ]

Good catch. The following patch for this case was tested OK.

But this simple fix will make the mac address change between
ifup/ifdowns, is this acceptable?

Thanks,
Fengguang
---
usbnet: use different random mac addresses for multiple usbnet devices

It is a bug to use the same mac address for possibly 2+ connected
usbnet devices.  Fix this concern raised by Peter Korsgaard.

Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 drivers/net/usb/usbnet.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

--- linux-2.6.orig/drivers/net/usb/usbnet.c
+++ linux-2.6/drivers/net/usb/usbnet.c
@@ -77,9 +77,6 @@
 
 /*-------------------------------------------------------------------------*/
 
-// randomly generated ethernet address
-static u8	node_id [ETH_ALEN];
-
 static const char driver_name [] = "usbnet";
 
 /* use ethtool to change the level for any given device */
@@ -1167,7 +1164,7 @@ usbnet_probe (struct usb_interface *udev
 
 	dev->net = net;
 	strcpy (net->name, "usb%d");
-	memcpy (net->dev_addr, node_id, sizeof node_id);
+	random_ether_addr(net->dev_addr);
 
 	/* rx and tx sides can use different message sizes;
 	 * bind() should set rx_urb_size in that case.
@@ -1310,7 +1307,6 @@ static int __init usbnet_init(void)
 	BUILD_BUG_ON (sizeof (((struct sk_buff *)0)->cb)
 			< sizeof (struct skb_data));
 
-	random_ether_addr(node_id);
 	return 0;
 }
 module_init(usbnet_init);
--

[ message continues ]

>>>>> "David" == David Brownell <david-b@pacbell.net> writes:

 David> On Tuesday 06 January 2009, Peter Korsgaard wrote:
 >> So what happens if you plug in 2 devices without an EEPROM? Do they
 >> get the same MAC address? That seems broken.

 David> What happens when you unplug one then re-plug it?  Maybe
 David> someone trips over the USB cable, or it gets an electrical
 David> glitch that evalutes to disconnect/reconnect... It gets
 David> the same address again.  Not particularly broken.

 David> Note that Ethernet was *designed* around using a single
 David> address per host ... I still have XNS docs sitting around
 David> somewhere, that was a fairly significant thing.  One of the
 David> original reasons Ethernet adapter addresses could change
 David> was to make sure that all Ethernet interfaces on a host
 David> would use the same address.

 David> That said ... if it bothers you, that's easy to change
 David> in userspace.  This code has worked this way for around
 David> nine years now; I don't recall any previous complaints.

Ok, I don't feel particular strongly about it, just found it odd.

-- 
Bye, Peter Korsgaard
--

[ message continues ]

From: Peter Korsgaard <jacmet@sunsite.dk>

Applied, thanks everyone.
--

[ message continues ]