[PATCH] smsc75xx: only set mac address once on bind

[PATCH] smsc75xx: only set mac address once on bind

[Steve Glendinning]

This patch changes when we decide what the device's MAC address
is from per ifconfig up to once when the device is connected.

Without this patch, a manually forced device MAC is overwritten
on ifconfig down/up.  Also devices that have no EEPROM are
assigned a new random address on ifconfig down/up instead of
persisting the same one.

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
Reported-by: Robert Cunningham <rcunningham@nsmsurveillance.com>
Cc: Bjorn Mork <bjorn@mork.no>
Cc: Dan Williams <dcbw@redhat.com>
---
 drivers/net/usb/smsc75xx.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 1cbd936..251a335 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -1054,8 +1054,6 @@ static int smsc75xx_reset(struct usbnet *dev)
 
 	netif_dbg(dev, ifup, dev->net, "PHY reset complete\n");
 
-	smsc75xx_init_mac_address(dev);
-
 	ret = smsc75xx_set_mac_address(dev);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to set mac address\n");
@@ -1422,6 +1420,14 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->net->hw_features = NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		NETIF_F_SG | NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_RXCSUM;
 
+	ret = smsc75xx_wait_ready(dev, 0);
+	if (ret < 0) {
+		netdev_warn(dev->net, "device not ready in smsc75xx_bind\n");
+		return ret;
+	}
+
+	smsc75xx_init_mac_address(dev);
+
 	/* Init all registers */
 	ret = smsc75xx_reset(dev);
 	if (ret < 0) {
-- 
1.7.10.4

Re: [PATCH] smsc75xx: only set mac address once on bind

[David Miller]

From: Steve Glendinning <steve.glendinning@shawell.net>
Date: Mon, 10 Dec 2012 11:01:19 +0000

> This patch changes when we decide what the device's MAC address
> is from per ifconfig up to once when the device is connected.
> 
> Without this patch, a manually forced device MAC is overwritten
> on ifconfig down/up.  Also devices that have no EEPROM are
> assigned a new random address on ifconfig down/up instead of
> persisting the same one.
> 
> Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
> Reported-by: Robert Cunningham <rcunningham@nsmsurveillance.com>

Applied.

Re: [PATCH] smsc75xx: only set mac address once on bind

[Dan Williams]

On Mon, 2012-12-10 at 11:01 +0000, Steve Glendinning wrote:
> This patch changes when we decide what the device's MAC address
> is from per ifconfig up to once when the device is connected.
> 
> Without this patch, a manually forced device MAC is overwritten
> on ifconfig down/up.  Also devices that have no EEPROM are
> assigned a new random address on ifconfig down/up instead of
> persisting the same one.

Does this mean that on devices without EEPROM, ifconfig XXX
down/ifconfig XXX up will generate a *new* random address?  That seems a
bit odd; why wouldn't the first random address generated for the device
persist until either (a) changed by ifconfig or (b) device was
disconnected?

Dan

> Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
> Reported-by: Robert Cunningham <rcunningham@nsmsurveillance.com>
> Cc: Bjorn Mork <bjorn@mork.no>
> Cc: Dan Williams <dcbw@redhat.com>
> ---
>  drivers/net/usb/smsc75xx.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
> index 1cbd936..251a335 100644
> --- a/drivers/net/usb/smsc75xx.c
> +++ b/drivers/net/usb/smsc75xx.c
> @@ -1054,8 +1054,6 @@ static int smsc75xx_reset(struct usbnet *dev)
>  
>  	netif_dbg(dev, ifup, dev->net, "PHY reset complete\n");
>  
> -	smsc75xx_init_mac_address(dev);
> -
>  	ret = smsc75xx_set_mac_address(dev);
>  	if (ret < 0) {
>  		netdev_warn(dev->net, "Failed to set mac address\n");
> @@ -1422,6 +1420,14 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf)
>  	dev->net->hw_features = NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  		NETIF_F_SG | NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_RXCSUM;
>  
> +	ret = smsc75xx_wait_ready(dev, 0);
> +	if (ret < 0) {
> +		netdev_warn(dev->net, "device not ready in smsc75xx_bind\n");
> +		return ret;
> +	}
> +
> +	smsc75xx_init_mac_address(dev);
> +
>  	/* Init all registers */
>  	ret = smsc75xx_reset(dev);
>  	if (ret < 0) {

Re: [PATCH] smsc75xx: only set mac address once on bind

[Steve Glendinning]

Hi Dan,

On 10 December 2012 23:00, Dan Williams <dcbw@redhat.com> wrote:
> On Mon, 2012-12-10 at 11:01 +0000, Steve Glendinning wrote:
>> This patch changes when we decide what the device's MAC address
>> is from per ifconfig up to once when the device is connected.
>>
>> Without this patch, a manually forced device MAC is overwritten
>> on ifconfig down/up.  Also devices that have no EEPROM are
>> assigned a new random address on ifconfig down/up instead of
>> persisting the same one.
>
> Does this mean that on devices without EEPROM, ifconfig XXX
> down/ifconfig XXX up will generate a *new* random address?  That seems a
> bit odd; why wouldn't the first random address generated for the device
> persist until either (a) changed by ifconfig or (b) device was
> disconnected?

Sorry if my commit message wasn't clear enough.  That is indeed a bit
odd, and it describes the (buggy) behaviour BEFORE this patch was
applied.

With this patch applied, devices without EEPROM (and without a
manually specified MAC address) will get a randomly generated address
assigned once at bind time, so this MAC will persist for the lifetime
the USB device is connected.  We also now won't trash a manually
specified MAC, for cases where userland sets the MAC before bringing
up the interface.

-- 
Steve Glendinning

Re: [PATCH] smsc75xx: only set mac address once on bind

[Dan Williams]

On Tue, 2012-12-11 at 14:35 +0000, Steve Glendinning wrote:
> Hi Dan,
> 
> On 10 December 2012 23:00, Dan Williams <dcbw@redhat.com> wrote:
> > On Mon, 2012-12-10 at 11:01 +0000, Steve Glendinning wrote:
> >> This patch changes when we decide what the device's MAC address
> >> is from per ifconfig up to once when the device is connected.
> >>
> >> Without this patch, a manually forced device MAC is overwritten
> >> on ifconfig down/up.  Also devices that have no EEPROM are
> >> assigned a new random address on ifconfig down/up instead of
> >> persisting the same one.
> >
> > Does this mean that on devices without EEPROM, ifconfig XXX
> > down/ifconfig XXX up will generate a *new* random address?  That seems a
> > bit odd; why wouldn't the first random address generated for the device
> > persist until either (a) changed by ifconfig or (b) device was
> > disconnected?
> 
> Sorry if my commit message wasn't clear enough.  That is indeed a bit
> odd, and it describes the (buggy) behaviour BEFORE this patch was
> applied.
> 
> With this patch applied, devices without EEPROM (and without a
> manually specified MAC address) will get a randomly generated address
> assigned once at bind time, so this MAC will persist for the lifetime
> the USB device is connected.  We also now won't trash a manually
> specified MAC, for cases where userland sets the MAC before bringing
> up the interface.

Excellent, thanks for the clarification.

Dan