dm9601: handle corrupt mac address

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.

---
 drivers/net/usb/dm9601.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

@@ -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);

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 <[email protected]>

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,

Bye, Peter Korsgaard

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

1195 status = info->bind (dev, udev);

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 this bug.

Thanks,
Fengguang

Hi,

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.

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

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.

---
 drivers/net/usb/usbnet.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

@@ -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);

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

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

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

- Dave

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

From: Peter Korsgaard <[email protected]>
Date: Tue, 06 Jan 2009 09:04:58 +0100

Applied, thanks everyone.