Message ID | 20090106091050.GA19316@localhost |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
>>>>> "Wu" == Wu Fengguang <wfg@linux.intel.com> writes:
Hi,
Wu> Add warnings on invalid mac address to help disclose/debug problems.
Wu> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
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.
On Tue, Jan 06, 2009 at 10:18:17AM +0100, Peter Korsgaard wrote: > >>>>> "Wu" == Wu Fengguang <wfg@linux.intel.com> writes: > > Hi, > > Wu> Add warnings on invalid mac address to help disclose/debug problems. > Wu> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > 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)); OK. > 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. 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 -- 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, 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. 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.
On Tue, 2009-01-06 at 10:18 +0100, Peter Korsgaard wrote: > >>>>> "Wu" == Wu Fengguang <wfg@linux.intel.com> writes: > > Hi, > > Wu> Add warnings on invalid mac address to help disclose/debug problems. > Wu> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > 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)); print_mac() is already obsolete; use %pM in the format string instead. Ben.
From: Ben Hutchings <bhutchings@solarflare.com> Date: Tue, 06 Jan 2009 11:52:35 +0000 > On Tue, 2009-01-06 at 10:18 +0100, Peter Korsgaard wrote: > > >>>>> "Wu" == Wu Fengguang <wfg@linux.intel.com> writes: > > > > Hi, > > > > Wu> Add warnings on invalid mac address to help disclose/debug problems. > > Wu> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> > > 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)); > > print_mac() is already obsolete; use %pM in the format string instead. Yep, please fix this. -- 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 @@ -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);
Add warnings on invalid mac address to help disclose/debug problems. Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> --- drivers/net/usb/dm9601.c | 12 +++++++++++- 1 file changed, 11 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