diff mbox series

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

Message ID 9ab691fa129ceb7a8947fde6f307fbd855d085cf.1538734658.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 Oct. 5, 2018, 10:24 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 | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/usb/aqc111.h |  1 +
 2 files changed, 52 insertions(+)

Comments

Andrew Lunn Oct. 6, 2018, 1:03 a.m. UTC | #1 
On Fri, Oct 05, 2018 at 10:24:58AM +0000, 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 | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/usb/aqc111.h |  1 +
>  2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index 1d366f4a1c51..075f51cd04ab 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>
>  
> @@ -266,11 +267,46 @@ static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed)
>  		aqc111_set_phy_speed_fw_iface(dev, aqc111_data);
>  }
>  
> +static int aqc111_set_mac_addr(struct net_device *net, void *p)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	struct sockaddr *addr = p;
> +
> +	if (netif_running(net))
> +		return -EBUSY;
> +	if (!is_valid_ether_addr(addr->sa_data))
> +		return -EADDRNOTAVAIL;

It is probably better to use eth_mac_addr().

> +
> +	memcpy(net->dev_addr, addr->sa_data, ETH_ALEN);
> +
> +	/* 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_get_mac(struct usbnet *dev, u8 *buf)
> +{
> +	int ret;
> +
> +	ret = aqc111_read_cmd(dev, AQ_FLASH_PARAMETERS, 0, 0, 6, buf);

ETH_ALEN instead of 6?

> +	if (ret < 0)
> +		goto out;
> +
> +	memcpy(dev->net->dev_addr, buf, ETH_ALEN);
> +	memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);

Is this really the permanent address? If i call aqc111_set_mac_addr()
followed by aqc111_get_mac() i still get what is in the OTP EEPROM?

 +
> +	return 0;
> +out:
> +	return ret;
> +}
> +
>  static void aqc111_read_fw_version(struct usbnet *dev,
>  				   struct aqc111_data *aqc111_data)
>  {
> @@ -289,6 +325,7 @@ static void aqc111_read_fw_version(struct usbnet *dev,
>  
>  static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
>  {
> +	u8 buf[6] = { 0 };

ETH_ALEN

>  	int ret;
>  	struct usb_device *udev = interface_to_usbdev(intf);
>  	struct aqc111_data *aqc111_data;
> @@ -316,6 +353,12 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
>  	dev->data[0] = (unsigned long)aqc111_data;
>  	memset(aqc111_data, 0, sizeof(*aqc111_data));
>  
> +	/* Get the MAC address */
> +	memset(buf, 0, ETH_ALEN);

You initialized it above as {0}. You don't need to memset it here.

> +	ret = aqc111_get_mac(dev, buf);

Do you even need to zero it? If aqc111_get_mac() fails, it will be
left undefined, but you fail the bind anyway.

> +	if (ret)
> +		goto out;
> +
>  	dev->net->netdev_ops = &aqc111_netdev_ops;
>  
>  	aqc111_read_fw_version(dev, aqc111_data);
> @@ -324,6 +367,10 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
>  					 SPEED_5000 : SPEED_1000;
>  
>  	return 0;
> +
> +out:
> +	kfree(aqc111_data);
> +	return ret;
>  }

   Andrew
Igor Russkikh Oct. 9, 2018, 2:34 p.m. UTC | #2 
Hi Andrew,

>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	memcpy(dev->net->dev_addr, buf, ETH_ALEN);
>> +	memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
> 
> Is this really the permanent address? If i call aqc111_set_mac_addr()
> followed by aqc111_get_mac() i still get what is in the OTP EEPROM?

Thats actually a confusion with function name here.
Think its better to name it aqc111_init_mac() since it gets called
only once on bind.

It really initializes perm_addr once, thus standard ndev callback will give
the perm mac you want.

> 
> You initialized it above as {0}. You don't need to memset it here.
> 
>> +	ret = aqc111_get_mac(dev, buf);
> 
> Do you even need to zero it? If aqc111_get_mac() fails, it will be
> left undefined, but you fail the bind anyway.

We even don't need this `buf` here at all. We'll move it into
above init_mac function.

BR, Igor
Andrew Lunn Oct. 9, 2018, 2:46 p.m. UTC | #3 
On Tue, Oct 09, 2018 at 02:34:36PM +0000, Igor Russkikh wrote:
> Hi Andrew,
> 
> >> +	if (ret < 0)
> >> +		goto out;
> >> +
> >> +	memcpy(dev->net->dev_addr, buf, ETH_ALEN);
> >> +	memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
> > 
> > Is this really the permanent address? If i call aqc111_set_mac_addr()
> > followed by aqc111_get_mac() i still get what is in the OTP EEPROM?
> 
> Thats actually a confusion with function name here.
> Think its better to name it aqc111_init_mac() since it gets called
> only once on bind.

Hi Igor

Or aqc111_get_otp_mac()?

   Andrew
diff mbox series

Patch

diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
index 1d366f4a1c51..075f51cd04ab 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>
 
@@ -266,11 +267,46 @@  static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed)
 		aqc111_set_phy_speed_fw_iface(dev, aqc111_data);
 }
 
+static int aqc111_set_mac_addr(struct net_device *net, void *p)
+{
+	struct usbnet *dev = netdev_priv(net);
+	struct sockaddr *addr = p;
+
+	if (netif_running(net))
+		return -EBUSY;
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memcpy(net->dev_addr, addr->sa_data, ETH_ALEN);
+
+	/* 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_get_mac(struct usbnet *dev, u8 *buf)
+{
+	int ret;
+
+	ret = aqc111_read_cmd(dev, AQ_FLASH_PARAMETERS, 0, 0, 6, buf);
+	if (ret < 0)
+		goto out;
+
+	memcpy(dev->net->dev_addr, buf, ETH_ALEN);
+	memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
+
+	return 0;
+out:
+	return ret;
+}
+
 static void aqc111_read_fw_version(struct usbnet *dev,
 				   struct aqc111_data *aqc111_data)
 {
@@ -289,6 +325,7 @@  static void aqc111_read_fw_version(struct usbnet *dev,
 
 static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
 {
+	u8 buf[6] = { 0 };
 	int ret;
 	struct usb_device *udev = interface_to_usbdev(intf);
 	struct aqc111_data *aqc111_data;
@@ -316,6 +353,12 @@  static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->data[0] = (unsigned long)aqc111_data;
 	memset(aqc111_data, 0, sizeof(*aqc111_data));
 
+	/* Get the MAC address */
+	memset(buf, 0, ETH_ALEN);
+	ret = aqc111_get_mac(dev, buf);
+	if (ret)
+		goto out;
+
 	dev->net->netdev_ops = &aqc111_netdev_ops;
 
 	aqc111_read_fw_version(dev, aqc111_data);
@@ -324,6 +367,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)
@@ -587,6 +634,10 @@  static int aqc111_reset(struct usbnet *dev)
 				 4, &aqc111_data->phy_ops);
 	}
 
+	/* 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 5c5602e6d236..7ca5c5a6ec82 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_CMD			0x32
 #define AQ_PHY_OPS			0x61