diff mbox series

[v3,net-next,07/21] net: usb: aqc111: Add support for getting and setting of MAC address

Message ID 8f92711d8479a3df65849e60fd92c727e1e1f78a.1542794577.git.igor.russkikh@aquantia.com (mailing list archive)
State Superseded
Headers show
Series Add support for Aquantia AQtion USB to 5/2.5GbE devices | expand

Commit Message

Igor Russkikh Nov. 21, 2018, 10:13 a.m. UTC 
From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>

Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 drivers/net/usb/aqc111.c | 47 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/usb/aqc111.h |  1 +
 2 files changed, 48 insertions(+)

Comments

Anant Thazhemadam Nov. 5, 2020, 3:13 a.m. UTC | #1 
Hi,

I only recently browsed through the code, and had some queries regarding
the changes introduced by this commit.

On 21/11/18 3:43 pm, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
>
> Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> ---
>  drivers/net/usb/aqc111.c | 47 ++++++++++++++++++++++++++++++++++++++++
>  drivers/net/usb/aqc111.h |  1 +
>  2 files changed, 48 insertions(+)
>
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index e33be16b506c..390ed6cbc3fd 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -11,6 +11,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/mii.h>
>  #include <linux/usb.h>
> +#include <linux/if_vlan.h>
>  #include <linux/usb/cdc.h>
>  #include <linux/usb/usbnet.h>
>  
> @@ -204,11 +205,43 @@ static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed)
>  	aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &aqc111_data->phy_cfg);
>  }
>  
> +static int aqc111_set_mac_addr(struct net_device *net, void *p)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	int ret = 0;
> +
> +	ret = eth_mac_addr(net, p);
> +	if (ret < 0)
> +		return ret;
> +

When eth_mac_addr() fails, from what I can see, it returns either -EBUSY, or
-EADDRNOTAVAIL.
Wouldn't it be a better idea to set a random MAC address instead, when
-EADDRNOTAVAIL is returned, so that the interface still comes up and is
usable?

I'm only asking because this feels similar to the discussion that can be found here.
    https://lkml.org/lkml/2020/10/2/305

> +	/* Set the MAC address */
> +	return aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_NODE_ID, ETH_ALEN,
> +				ETH_ALEN, net->dev_addr);
> +}
> +
>  static const struct net_device_ops aqc111_netdev_ops = {
>  	.ndo_open		= usbnet_open,
>  	.ndo_stop		= usbnet_stop,
> +	.ndo_set_mac_address	= aqc111_set_mac_addr,
> +	.ndo_validate_addr	= eth_validate_addr,
>  };
>  
> +static int aqc111_read_perm_mac(struct usbnet *dev)
> +{
> +	u8 buf[ETH_ALEN];
> +	int ret;
> +
> +	ret = aqc111_read_cmd(dev, AQ_FLASH_PARAMETERS, 0, 0, ETH_ALEN, buf);
> +	if (ret < 0)
> +		goto out;
> +
> +	ether_addr_copy(dev->net->perm_addr, buf);
> +
> +	return 0;
> +out:
> +	return ret;
> +}
> +
>  static void aqc111_read_fw_version(struct usbnet *dev,
>  				   struct aqc111_data *aqc111_data)
>  {
> @@ -251,6 +284,12 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
>  	/* store aqc111_data pointer in device data field */
>  	dev->driver_priv = aqc111_data;
>  
> +	/* Init the MAC address */
> +	ret = aqc111_read_perm_mac(dev);
> +	if (ret)
> +		goto out;
> +
> +	ether_addr_copy(dev->net->dev_addr, dev->net->perm_addr);
>  	dev->net->netdev_ops = &aqc111_netdev_ops;
>  
>  	aqc111_read_fw_version(dev, aqc111_data);
> @@ -259,6 +298,10 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
>  					 SPEED_5000 : SPEED_1000;
>  
>  	return 0;
> +
> +out:
> +	kfree(aqc111_data);
> +	return ret;
>  }
>  
>  static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf)
> @@ -467,6 +510,10 @@ static int aqc111_reset(struct usbnet *dev)
>  	aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0,
>  			   &aqc111_data->phy_cfg);
>  
> +	/* Set the MAC address */
> +	aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_NODE_ID, ETH_ALEN,
> +			 ETH_ALEN, dev->net->dev_addr);
> +

There's a chance that aqc111_write_cmd() can end up writing only a
part of the required amount of data too.
Wouldn't it be a better idea to enforce a sanity check here, and set a
random mac address instead if this write fails?

>  	reg8 = 0xFF;
>  	aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_BM_INT_MASK, 1, 1, &reg8);
>  
> diff --git a/drivers/net/usb/aqc111.h b/drivers/net/usb/aqc111.h
> index f3b45d8ca4e3..0c8e1ee29893 100644
> --- a/drivers/net/usb/aqc111.h
> +++ b/drivers/net/usb/aqc111.h
> @@ -11,6 +11,7 @@
>  #define __LINUX_USBNET_AQC111_H
>  
>  #define AQ_ACCESS_MAC			0x01
> +#define AQ_FLASH_PARAMETERS		0x20
>  #define AQ_PHY_POWER			0x31
>  #define AQ_PHY_OPS			0x61
>

If any of these changes sound like a good idea, I'd be happy to write a
patch implementing them. :)

Thanks,
Anant
Igor Russkikh Nov. 12, 2020, 7:31 a.m. UTC | #2 
Hi Anant,

>>
>> +static int aqc111_set_mac_addr(struct net_device *net, void *p)
>> +{
>> +	struct usbnet *dev = netdev_priv(net);
>> +	int ret = 0;
>> +
>> +	ret = eth_mac_addr(net, p);
>> +	if (ret < 0)
>> +		return ret;
>> +
> 
> When eth_mac_addr() fails, from what I can see, it returns either -EBUSY, or
> -EADDRNOTAVAIL.
> Wouldn't it be a better idea to set a random MAC address instead, when
> -EADDRNOTAVAIL is returned, so that the interface still comes up and is
> usable?
> 
> I'm only asking because this feels similar to the discussion that can be 
> found here.
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_10_2_305&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=3kUjVPjrPMvlbd3rzgP63W0eewvCq4D-kzQRqaXHOqU&m=aJdSTt0SmQpKGqrsMm9TQ2mCWDH1Vc7arUp0xq-v6Ac&s=n-tXDyWIR5tPvrQ4DUasDgrocxKU3e_A-mh3Nv5JC5A&e=

Here in set_mac_addr driver is being asked to SET the specified mac.
If it fails - device will most probably drop the packets designated for it.

So I think no, you can't put here some random MAC.

Regards,
  Igor
diff mbox series

Patch

diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
index e33be16b506c..390ed6cbc3fd 100644
--- a/drivers/net/usb/aqc111.c
+++ b/drivers/net/usb/aqc111.c
@@ -11,6 +11,7 @@ 
 #include <linux/netdevice.h>
 #include <linux/mii.h>
 #include <linux/usb.h>
+#include <linux/if_vlan.h>
 #include <linux/usb/cdc.h>
 #include <linux/usb/usbnet.h>
 
@@ -204,11 +205,43 @@  static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed)
 	aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &aqc111_data->phy_cfg);
 }
 
+static int aqc111_set_mac_addr(struct net_device *net, void *p)
+{
+	struct usbnet *dev = netdev_priv(net);
+	int ret = 0;
+
+	ret = eth_mac_addr(net, p);
+	if (ret < 0)
+		return ret;
+
+	/* Set the MAC address */
+	return aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_NODE_ID, ETH_ALEN,
+				ETH_ALEN, net->dev_addr);
+}
+
 static const struct net_device_ops aqc111_netdev_ops = {
 	.ndo_open		= usbnet_open,
 	.ndo_stop		= usbnet_stop,
+	.ndo_set_mac_address	= aqc111_set_mac_addr,
+	.ndo_validate_addr	= eth_validate_addr,
 };
 
+static int aqc111_read_perm_mac(struct usbnet *dev)
+{
+	u8 buf[ETH_ALEN];
+	int ret;
+
+	ret = aqc111_read_cmd(dev, AQ_FLASH_PARAMETERS, 0, 0, ETH_ALEN, buf);
+	if (ret < 0)
+		goto out;
+
+	ether_addr_copy(dev->net->perm_addr, buf);
+
+	return 0;
+out:
+	return ret;
+}
+
 static void aqc111_read_fw_version(struct usbnet *dev,
 				   struct aqc111_data *aqc111_data)
 {
@@ -251,6 +284,12 @@  static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
 	/* store aqc111_data pointer in device data field */
 	dev->driver_priv = aqc111_data;
 
+	/* Init the MAC address */
+	ret = aqc111_read_perm_mac(dev);
+	if (ret)
+		goto out;
+
+	ether_addr_copy(dev->net->dev_addr, dev->net->perm_addr);
 	dev->net->netdev_ops = &aqc111_netdev_ops;
 
 	aqc111_read_fw_version(dev, aqc111_data);
@@ -259,6 +298,10 @@  static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
 					 SPEED_5000 : SPEED_1000;
 
 	return 0;
+
+out:
+	kfree(aqc111_data);
+	return ret;
 }
 
 static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf)
@@ -467,6 +510,10 @@  static int aqc111_reset(struct usbnet *dev)
 	aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0,
 			   &aqc111_data->phy_cfg);
 
+	/* Set the MAC address */
+	aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_NODE_ID, ETH_ALEN,
+			 ETH_ALEN, dev->net->dev_addr);
+
 	reg8 = 0xFF;
 	aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_BM_INT_MASK, 1, 1, &reg8);
 
diff --git a/drivers/net/usb/aqc111.h b/drivers/net/usb/aqc111.h
index f3b45d8ca4e3..0c8e1ee29893 100644
--- a/drivers/net/usb/aqc111.h
+++ b/drivers/net/usb/aqc111.h
@@ -11,6 +11,7 @@ 
 #define __LINUX_USBNET_AQC111_H
 
 #define AQ_ACCESS_MAC			0x01
+#define AQ_FLASH_PARAMETERS		0x20
 #define AQ_PHY_POWER			0x31
 #define AQ_PHY_OPS			0x61