[PATCH] dm9601: handle corrupt mac address

[PATCH] dm9601: handle corrupt mac address

[Wu Fengguang]

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

Re: [PATCH] dm9601: handle corrupt mac address

[Peter Korsgaard]

>>>>> "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

Re: [PATCH] dm9601: handle corrupt mac address

[Wu Fengguang]

Hi Peter,

On Tue, Jan 06, 2009 at 09:04:58AM +0100, Peter Korsgaard wrote:
> >>>>> "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.

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


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

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.

> Other than that,
> 
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>

Thanks,
Fengguang

Re: [PATCH] dm9601: handle corrupt mac address

[Peter Korsgaard]

>>>>> "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

Re: [PATCH] dm9601: handle corrupt mac address

[Wu Fengguang]

On Tue, Jan 06, 2009 at 09:42:01AM +0100, Peter Korsgaard wrote:
> >>>>> "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.

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

Re: [PATCH] dm9601: handle corrupt mac address

[David Brownell]

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.

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

Re: [PATCH] dm9601: handle corrupt mac address

[Peter Korsgaard]

>>>>> "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

Re: [PATCH] dm9601: handle corrupt mac address

[David Miller]

From: Peter Korsgaard <jacmet@sunsite.dk>
Date: Tue, 06 Jan 2009 09:04:58 +0100

> >>>>> "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>

Applied, thanks everyone.