| Message ID | 20090106061414.GA3677@localhost |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
>>>>> "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>
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "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.
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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);
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(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html