Skip to contentSkip to conversation

dm9601: warn on invalid mac address

PATCH Applied Wu Fengguang 4 participants 12 messages Jan 6, 2009 – Jan 8, 2009

Subsystems (4)
Networking Network drivers USB networking drivers USB davicom DM9601 driver
lore
Share & tools
lore

Wu Fengguang

author
#0 lore raw

PATCH 1/1 dm9601: warn on invalid mac address

Add warnings on invalid mac address to help disclose/debug problems.

Signed-off-by Wu Fengguang <[email protected]>
---
 drivers/net/usb/dm9601.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/net/usb/dm9601.c +11 -1
@@ -401,8 +401,12 @@ static int dm9601_set_mac_address(struct
 	struct sockaddr *addr = p;
 	struct usbnet *dev = netdev_priv(net);
 
-	if (!is_valid_ether_addr(addr->sa_data))
+	if (!is_valid_ether_addr(addr->sa_data)) {
+		DECLARE_MAC_BUF(mac_buf);
+		print_mac(mac_buf, addr->sa_data);
+		dev_warn(&net->dev, "not setting invalid mac address %s\n", mac_buf);
 		return -EINVAL;
+	}
 
 	memcpy(net->dev_addr, addr->sa_data, net->addr_len);
 	dm_write_async(dev, DM_PHY_ADDR, net->addr_len, net->dev_addr);

@@ -449,6 +453,12 @@ static int dm9601_bind(struct usbnet *de
 	 */
 	if (is_valid_ether_addr(mac))
 		memcpy(dev->net->dev_addr, mac, ETH_ALEN);
+	else {
+		DECLARE_MAC_BUF(mac_buf);
+		print_mac(mac_buf, mac);
+		devdbg(dev, "EEPROM reported mac address %s is invalid,"
+				" use the randomly generated one.", mac_buf);
+	}
 
 	/* power up phy */
 	dm_write_reg(dev, DM_GPR_CTRL, 1);
#1 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Hi,

Wu> Add warnings on invalid mac address to help disclose/debug problems. Wu> Signed-off-by: Wu Fengguang <[email protected]>
Wu> ---

 Wu>  drivers/net/usb/dm9601.c |   12 +++++++++++-

Wu> 1 file changed, 11 insertions(+), 1 deletion(-)

Wu> --- linux-2.6.orig/drivers/net/usb/dm9601.c
Wu> +++ linux-2.6/drivers/net/usb/dm9601.c
Wu> @@ -401,8 +401,12 @@ static int dm9601_set_mac_address(struct
Wu> struct sockaddr *addr = p;
Wu> struct usbnet *dev = netdev_priv(net);

Wu> - if (!is_valid_ether_addr(addr->sa_data))
Wu> + if (!is_valid_ether_addr(addr->sa_data)) {
Wu> + DECLARE_MAC_BUF(mac_buf);
Wu> + print_mac(mac_buf, addr->sa_data);
Wu> + dev_warn(&net->dev, "not setting invalid mac address %s\n", mac_buf);

This should be an error and not a warning. Notice that print_mac returns the string, so you can do:

dev_err(&net->dev, "... %s", print_mac(mac_buf, addr->sa_data));

Wu> memcpy(net->dev_addr, addr->sa_data, net->addr_len);
Wu> dm_write_async(dev, DM_PHY_ADDR, net->addr_len, net->dev_addr);
Wu> @@ -449,6 +453,12 @@ static int dm9601_bind(struct usbnet *de

 Wu>  	 */
 Wu>  	if (is_valid_ether_addr(mac))
 Wu>  		memcpy(dev->net->dev_addr, mac, ETH_ALEN);

Wu> + else {
Wu> + DECLARE_MAC_BUF(mac_buf);
Wu> + print_mac(mac_buf, mac);
Wu> + devdbg(dev, "EEPROM reported mac address %s is invalid,"
Wu> + " use the randomly generated one.", mac_buf);

And this should be a warning.

Signature · 1 line
Bye, Peter Korsgaard
author
#2 lore raw
Quoted 22 lines · Peter Korsgaard
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Hi,

Wu> Add warnings on invalid mac address to help disclose/debug problems. Wu> Signed-off-by: Wu Fengguang <[email protected]>
Wu> ---

 Wu>  drivers/net/usb/dm9601.c |   12 +++++++++++-

Wu> 1 file changed, 11 insertions(+), 1 deletion(-)

Wu> --- linux-2.6.orig/drivers/net/usb/dm9601.c
Wu> +++ linux-2.6/drivers/net/usb/dm9601.c
Wu> @@ -401,8 +401,12 @@ static int dm9601_set_mac_address(struct
Wu> struct sockaddr *addr = p;
Wu> struct usbnet *dev = netdev_priv(net);

Wu> - if (!is_valid_ether_addr(addr->sa_data))
Wu> + if (!is_valid_ether_addr(addr->sa_data)) {
Wu> + DECLARE_MAC_BUF(mac_buf);
Wu> + print_mac(mac_buf, addr->sa_data);
Wu> + dev_warn(&net->dev, "not setting invalid mac address %s\n", mac_buf);

Quoted 4 lines

This should be an error and not a warning. Notice that print_mac returns the string, so you can do:

dev_err(&net->dev, "... %s", print_mac(mac_buf, addr->sa_data));

OK.

Quoted 6 lines

Wu> memcpy(net->dev_addr, addr->sa_data, net->addr_len);
Wu> dm_write_async(dev, DM_PHY_ADDR, net->addr_len, net->dev_addr);
Wu> @@ -449,6 +453,12 @@ static int dm9601_bind(struct usbnet *de

 Wu>  	 */
 Wu>  	if (is_valid_ether_addr(mac))
 Wu>  		memcpy(dev->net->dev_addr, mac, ETH_ALEN);
Quoted 7 lines

Wu> + else {
Wu> + DECLARE_MAC_BUF(mac_buf);
Wu> + print_mac(mac_buf, mac);
Wu> + devdbg(dev, "EEPROM reported mac address %s is invalid,"
Wu> + " use the randomly generated one.", mac_buf);

And this should be a warning.

Then let the warning message appear repeatedly for some devices?

Also dev_warn() won't be able to show the device name at that time, like this:

[28489.062180]  : EEPROM reported mac address ff:ff:ff:ff:ff:ff is invalid, use the randomly generated one.

Thanks,
Fengguang

#3 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Hi,

Wu> if (is_valid_ether_addr(mac))
Wu> memcpy(dev->net->dev_addr, mac, ETH_ALEN);
Wu> + else {
Wu> + DECLARE_MAC_BUF(mac_buf);
Wu> + print_mac(mac_buf, mac);
Wu> + devdbg(dev, "EEPROM reported mac address %s is invalid,"
Wu> + " use the randomly generated one.", mac_buf);

Quoted 2 lines
Quoted 2 lines

And this should be a warning.

Wu> Then let the warning message appear repeatedly for some devices?

This is called at probe time - But yes, I think it makes sense to print it.

We should print the random address instead of the ff's though.

Wu> Also dev_warn() won't be able to show the device name at that time, Wu> like this:

Ah yes, that's presumably why I used a raw printk just above.

Wu> [28489.062180] : EEPROM reported mac address ff:ff:ff:ff:ff:ff is Wu> invalid, use the randomly generated one.

I would prefer something like:

printk(KERN_WARNING "dm9601: No valid MAC address in EEPROM, using %s\n",

       print_mac(..));

Also, it seems like you're not writing the random address to the hardware registers, so you won't be able to receive any unicast - You'll need to add a call to dm9601_set_mac_address() or similar.

Signature · 1 line
Bye, Peter Korsgaard
author
#4 lore raw
Quoted 26 lines · Peter Korsgaard
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Hi,

Wu> if (is_valid_ether_addr(mac))
Wu> memcpy(dev->net->dev_addr, mac, ETH_ALEN);
Wu> + else {
Wu> + DECLARE_MAC_BUF(mac_buf);
Wu> + print_mac(mac_buf, mac);
Wu> + devdbg(dev, "EEPROM reported mac address %s is invalid,"
Wu> + " use the randomly generated one.", mac_buf);

Quoted 2 lines
Quoted 2 lines

And this should be a warning.

Wu> Then let the warning message appear repeatedly for some devices?

This is called at probe time - But yes, I think it makes sense to print it.

We should print the random address instead of the ff's though.

Wu> Also dev_warn() won't be able to show the device name at that time, Wu> like this:

Ah yes, that's presumably why I used a raw printk just above.

Quoted 7 lines

Wu> [28489.062180] : EEPROM reported mac address ff:ff:ff:ff:ff:ff is Wu> invalid, use the randomly generated one.

I would prefer something like:

printk(KERN_WARNING "dm9601: No valid MAC address in EEPROM, using %s\n",

       print_mac(..));

This looks better :-)

Quoted 3 lines

Also, it seems like you're not writing the random address to the hardware registers, so you won't be able to receive any unicast - You'll need to add a call to dm9601_set_mac_address() or similar.

Good catch. I had wanted to ask why it only works with promisc mode ;-)

Here is the tested patch.

Thanks,
Fengguang
---
dm9601: tell HW about random generated mac address

Otherwise unicast RX will only work in promisc mode.

Signed-off-by Peter Korsgaard <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>
---
 drivers/net/usb/dm9601.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/net/usb/dm9601.c +8 -1
@@ -396,6 +396,11 @@ static void dm9601_set_multicast(struct 
 	dm_write_reg_async(dev, DM_RX_CTRL, rx_ctl);
 }
 
+static void __dm9601_set_mac_address(struct usbnet *dev)
+{
+	dm_write_async(dev, DM_PHY_ADDR, ETH_ALEN, dev->net->dev_addr);
+}
+
 static int dm9601_set_mac_address(struct net_device *net, void *p)
 {
 	struct sockaddr *addr = p;

@@ -405,7 +410,7 @@ static int dm9601_set_mac_address(struct
 		return -EINVAL;
 
 	memcpy(net->dev_addr, addr->sa_data, net->addr_len);
-	dm_write_async(dev, DM_PHY_ADDR, net->addr_len, net->dev_addr);
+	__dm9601_set_mac_address(dev);
 
 	return 0;
 }

@@ -449,6 +454,8 @@ static int dm9601_bind(struct usbnet *de
 	 */
 	if (is_valid_ether_addr(mac))
 		memcpy(dev->net->dev_addr, mac, ETH_ALEN);
+	else
+		__dm9601_set_mac_address(dev);
 
 	/* power up phy */
 	dm_write_reg(dev, DM_GPR_CTRL, 1);
Acked-by
#5 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Hi,

Quoted 3 lines
Quoted 3 lines

Also, it seems like you're not writing the random address to the hardware registers, so you won't be able to receive any unicast - You'll need to add a call to dm9601_set_mac_address() or similar.

Wu> Good catch. I had wanted to ask why it only works with promisc Wu> mode ;-)

;)

Wu> Here is the tested patch.

Wu> Thanks,
Wu> Fengguang
Wu> ---
Wu> dm9601: tell HW about random generated mac address

Wu> Otherwise unicast RX will only work in promisc mode.

Wu> Signed-off-by: Peter Korsgaard <[email protected]> Wu> Signed-off-by: Wu Fengguang <[email protected]>

Acked-by Peter Korsgaard <[email protected]>
Signature · 1 line
Bye, Peter Korsgaard
maintainer Applied
#6 lore raw

From: Peter Korsgaard <[email protected]>
Date: Wed, 07 Jan 2009 09:24:43 +0100

Quoted 10 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Wu> dm9601: tell HW about random generated mac address

Wu> Otherwise unicast RX will only work in promisc mode.

Wu> Signed-off-by: Peter Korsgaard <[email protected]> Wu> Signed-off-by: Wu Fengguang <[email protected]>

Acked-by Peter Korsgaard <[email protected]>

Applied, thanks everyone.

#7 lore raw
In reply to Peter Korsgaard
Quoted 22 lines · Peter Korsgaard
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Hi,

Wu> Add warnings on invalid mac address to help disclose/debug problems. Wu> Signed-off-by: Wu Fengguang <[email protected]>
Wu> ---

 Wu>  drivers/net/usb/dm9601.c |   12 +++++++++++-

Wu> 1 file changed, 11 insertions(+), 1 deletion(-)

Wu> --- linux-2.6.orig/drivers/net/usb/dm9601.c
Wu> +++ linux-2.6/drivers/net/usb/dm9601.c
Wu> @@ -401,8 +401,12 @@ static int dm9601_set_mac_address(struct
Wu> struct sockaddr *addr = p;
Wu> struct usbnet *dev = netdev_priv(net);

Wu> - if (!is_valid_ether_addr(addr->sa_data))
Wu> + if (!is_valid_ether_addr(addr->sa_data)) {
Wu> + DECLARE_MAC_BUF(mac_buf);
Wu> + print_mac(mac_buf, addr->sa_data);
Wu> + dev_warn(&net->dev, "not setting invalid mac address %s\n", mac_buf);

Quoted 4 lines

This should be an error and not a warning. Notice that print_mac returns the string, so you can do:

dev_err(&net->dev, "... %s", print_mac(mac_buf, addr->sa_data));

print_mac() is already obsolete; use %pM in the format string instead.

Ben.

Signature · 3 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
maintainer
#8 lore raw

From: Ben Hutchings <[email protected]>
Date: Tue, 06 Jan 2009 11:52:35 +0000

Quoted 23 lines · Peter Korsgaard
Quoted 22 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Hi,

Wu> Add warnings on invalid mac address to help disclose/debug problems. Wu> Signed-off-by: Wu Fengguang <[email protected]>
Wu> ---

 Wu>  drivers/net/usb/dm9601.c |   12 +++++++++++-

Wu> 1 file changed, 11 insertions(+), 1 deletion(-)

Wu> --- linux-2.6.orig/drivers/net/usb/dm9601.c
Wu> +++ linux-2.6/drivers/net/usb/dm9601.c
Wu> @@ -401,8 +401,12 @@ static int dm9601_set_mac_address(struct
Wu> struct sockaddr *addr = p;
Wu> struct usbnet *dev = netdev_priv(net);

Wu> - if (!is_valid_ether_addr(addr->sa_data))
Wu> + if (!is_valid_ether_addr(addr->sa_data)) {
Wu> + DECLARE_MAC_BUF(mac_buf);
Wu> + print_mac(mac_buf, addr->sa_data);
Wu> + dev_warn(&net->dev, "not setting invalid mac address %s\n", mac_buf);

Quoted 6 lines
Quoted 4 lines

This should be an error and not a warning. Notice that print_mac returns the string, so you can do:

dev_err(&net->dev, "... %s", print_mac(mac_buf, addr->sa_data));

print_mac() is already obsolete; use %pM in the format string instead.

Yep, please fix this.

author
#9 lore raw
Quoted 26 lines · David Miller

From: Ben Hutchings <[email protected]>
Date: Tue, 06 Jan 2009 11:52:35 +0000

Quoted 23 lines · Peter Korsgaard
Quoted 22 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Hi,

Wu> Add warnings on invalid mac address to help disclose/debug problems. Wu> Signed-off-by: Wu Fengguang <[email protected]>
Wu> ---

 Wu>  drivers/net/usb/dm9601.c |   12 +++++++++++-

Wu> 1 file changed, 11 insertions(+), 1 deletion(-)

Wu> --- linux-2.6.orig/drivers/net/usb/dm9601.c
Wu> +++ linux-2.6/drivers/net/usb/dm9601.c
Wu> @@ -401,8 +401,12 @@ static int dm9601_set_mac_address(struct
Wu> struct sockaddr *addr = p;
Wu> struct usbnet *dev = netdev_priv(net);

Wu> - if (!is_valid_ether_addr(addr->sa_data))
Wu> + if (!is_valid_ether_addr(addr->sa_data)) {
Wu> + DECLARE_MAC_BUF(mac_buf);
Wu> + print_mac(mac_buf, addr->sa_data);
Wu> + dev_warn(&net->dev, "not setting invalid mac address %s\n", mac_buf);

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

This should be an error and not a warning. Notice that print_mac returns the string, so you can do:

dev_err(&net->dev, "... %s", print_mac(mac_buf, addr->sa_data));

print_mac() is already obsolete; use %pM in the format string instead.

Yep, please fix this.

Thanks for the tip. Here is the updated patch.

Thanks,
Fengguang
---
dm9601: warn on invalid mac address

Add warnings on invalid mac address to help disclose/debug problems.

Signed-off-by Wu Fengguang <[email protected]>
---
 drivers/net/usb/dm9601.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/net/usb/dm9601.c +9 -2
@@ -406,8 +406,11 @@ static int dm9601_set_mac_address(struct
 	struct sockaddr *addr = p;
 	struct usbnet *dev = netdev_priv(net);
 
-	if (!is_valid_ether_addr(addr->sa_data))
+	if (!is_valid_ether_addr(addr->sa_data)) {
+		dev_err(&net->dev, "not setting invalid mac address %pM\n",
+								addr->sa_data);
 		return -EINVAL;
+	}
 
 	memcpy(net->dev_addr, addr->sa_data, net->addr_len);
 	__dm9601_set_mac_address(dev);

@@ -454,8 +457,12 @@ static int dm9601_bind(struct usbnet *de
 	 */
 	if (is_valid_ether_addr(mac))
 		memcpy(dev->net->dev_addr, mac, ETH_ALEN);
-	else
+	else {
+		printk(KERN_WARNING
+			"dm9601: No valid MAC address in EEPROM, using %pM\n",
+			dev->net->dev_addr);
 		__dm9601_set_mac_address(dev);
+	}
 
 	/* power up phy */
 	dm_write_reg(dev, DM_GPR_CTRL, 1);
Acked-by
#10 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Wu> Thanks for the tip. Here is the updated patch.

Wu> Thanks,
Wu> Fengguang
Wu> ---
Wu> dm9601: warn on invalid mac address

Wu> Add warnings on invalid mac address to help disclose/debug problems.

Wu> Signed-off-by: Wu Fengguang <[email protected]>

Acked-by Peter Korsgaard <[email protected]>
Signature · 1 line
Bye, Peter Korsgaard
maintainer
#11 lore raw

From: Peter Korsgaard <[email protected]>
Date: Wed, 07 Jan 2009 09:25:03 +0100

Quoted 9 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Wu" == Wu Fengguang <[email protected]> writes:

Wu> Thanks for the tip. Here is the updated patch.

Wu> Thanks,
Wu> Fengguang
Wu> ---
Wu> dm9601: warn on invalid mac address

Quoted 5 lines

Wu> Add warnings on invalid mac address to help disclose/debug problems.

Wu> Signed-off-by: Wu Fengguang <[email protected]>

Acked-by Peter Korsgaard <[email protected]>

Also applied, thanks.