[PATCH v2] r8152: Add support for setting MAC to system's Auxiliary MAC address

This adjusts a lot of concerns that have been raised on LKML.

Changes from v1:
 * Lots of error checking around bad ACPI data
 * Only activate on Dell system vendor DMI string
 * Use hex2bin instead of inventing a wheel
 * Copy MAC to both dev_addr and sa_data
 * Track and set only for one NIC at a time
 * If MAC lookup failed (bad ACPI data or bad return) fall back
   to regular r8152 MAC setting routine.

This has been tested with TB15, WD15 docks and Dell P/N 96NP5 dongle.
It's also been tested with two devices that use r8152 simultaneously.

Remaining discussion points:
 * Greg KH had asked this to only on machines that are known to have
   \\_SB.AMAC
 - I would rather avoid doing this because the list will just grow every
   year.  I've added lots of error checking and restricted this to Dell.
 * There was also a request to move this to an x86
   arch_get_platform_mac_address() implementation.  
 - I haven't yet done this.  If this is the right approach.
   I would like to know the proper place in arch/x86 to put this code.
   My initial thought was a new file in arch/x86/platform/intel

Mario Limonciello (1):
  r8152: Add support for setting MAC to system's Auxiliary MAC address

 drivers/net/usb/r8152.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

-- 
2.7.4

Dell systems with Type-C ports have support for a persistent system
specific MAC address when used with Dell Type-C docks and dongles.
This means a dock plugged into two different systems will show different
(but persistent) MAC addresses.  Dell Type-C docks and dongles use the
r8152 driver.

This information for the system's persistent MAC address is burned in when
the HW is built and available under _SB\AMAC in the DSDT at runtime.

More information about the technology is available here:
http://www.dell.com/support/article/us/en/04/SLN301147

Signed-off-by: Mario Limonciello <redacted>
---
 drivers/net/usb/r8152.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3f9f6ed..6dea542 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -26,6 +26,8 @@
 #include <linux/mdio.h>
 #include <linux/usb/cdc.h>
 #include <linux/suspend.h>
+#include <linux/acpi.h>
+#include <linux/dmi.h>
 
 /* Information for net-next */
 #define NETNEXT_VERSION		"08"

@@ -500,6 +502,7 @@ enum rtl8152_flags {
 	SELECTIVE_SUSPEND,
 	PHY_RESET,
 	SCHEDULE_NAPI,
+	MAC_PASSTHRU = 0,
 };
 
 /* Define these values to match your device */

@@ -653,6 +656,7 @@ enum tx_csum_stat {
  */
 static const int multicast_filter_limit = 32;
 static unsigned int agg_buf_sz = 16384;
+static bool mac_passthru_active;
 
 #define RTL_LIMITED_TSO_SIZE	(agg_buf_sz - sizeof(struct tx_desc) - \
 				 VLAN_ETH_HLEN - VLAN_HLEN)

@@ -1030,6 +1034,49 @@ out1:
 	return ret;
 }
 
+static int get_auxiliary_addr(struct r8152 *tp, struct sockaddr *sa)
+{
+	acpi_status status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	int ret = -1;
+	unsigned char buf[6];
+
+	if (!dmi_name_in_vendors("Dell Inc.") || mac_passthru_active)
+		return -1;
+
+	/* returns _AUXMAC_#AABBCCDDEEFF# */
+	status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);
+	obj = (union acpi_object *)buffer.pointer;
+	if (ACPI_SUCCESS(status)) {
+		if (obj->type != ACPI_TYPE_BUFFER ||
+		    obj->string.length != 0x17) {
+			pr_warn("r8152: get_auxiliary_addr: Invalid buffer");
+			goto amacout;
+		}
+		if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {
+			pr_warn("r8152: get_auxiliary_addr: Invalid header");
+			goto amacout;
+		}
+		ret = hex2bin(buf, obj->string.pointer + 9, 6);
+		if (ret < 0) {
+			pr_warn("r8152: get_auxiliary_addr: Invalid MAC");
+			goto amacout;
+		}
+		memcpy(sa->sa_data, buf, 6);
+		ether_addr_copy(tp->netdev->dev_addr, sa->sa_data);
+		netdev_info(tp->netdev, "Using system MAC address %pM\n",
+			    sa->sa_data);
+		set_bit(MAC_PASSTHRU, &tp->flags);
+		mac_passthru_active = true;
+		ret = 1;
+	}
+
+amacout:
+	kfree(obj);
+	return ret;
+}
+
 static int set_ethernet_addr(struct r8152 *tp)
 {
 	struct net_device *dev = tp->netdev;

@@ -1041,6 +1088,10 @@ static int set_ethernet_addr(struct r8152 *tp)
 	else
 		ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
 
+	/* if system provides auxiliary MAC address */
+	if (get_auxiliary_addr(tp, &sa))
+		ret = 0;
+
 	if (ret < 0) {
 		netif_err(tp, probe, dev, "Get ether addr fail\n");
 	} else if (!is_valid_ether_addr(sa.sa_data)) {

@@ -4268,6 +4319,8 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 		if (udev->state == USB_STATE_NOTATTACHED)
 			set_bit(RTL8152_UNPLUG, &tp->flags);
 
+		if (test_bit(MAC_PASSTHRU, &tp->flags))
+			mac_passthru_active = false;
 		netif_napi_del(&tp->napi);
 		unregister_netdev(tp->netdev);
 		tp->rtl_ops.unload(tp);

-- 
2.7.4

On Thu, Jun 02, 2016 at 11:58:07AM -0500, Mario Limonciello wrote:

Does setting that to 0 really work?  You just did this for two enum
values, what is the compiler supposed to do?

very generic name for a platform-specific feature :(

What about the platform mac address api that was pointed out?

+{
+	acpi_status status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	int ret = -1;
+	unsigned char buf[6];
+
+	if (!dmi_name_in_vendors("Dell Inc.") || mac_passthru_active)
+		return -1;

Don't make up random error values, please use "real" ones.

And you want to check this for all Dell devices?  Please be model
specific, I doubt a bunch of Dell servers wants to run this code...

+
+	/* returns _AUXMAC_#AABBCCDDEEFF# */
+	status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);
+	obj = (union acpi_object *)buffer.pointer;
+	if (ACPI_SUCCESS(status)) {
+		if (obj->type != ACPI_TYPE_BUFFER ||
+		    obj->string.length != 0x17) {
+			pr_warn("r8152: get_auxiliary_addr: Invalid buffer");
+			goto amacout;
+		}
+		if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {
+			pr_warn("r8152: get_auxiliary_addr: Invalid header");
+			goto amacout;
+		}
+		ret = hex2bin(buf, obj->string.pointer + 9, 6);
+		if (ret < 0) {
+			pr_warn("r8152: get_auxiliary_addr: Invalid MAC");
+			goto amacout;
+		}
+		memcpy(sa->sa_data, buf, 6);
+		ether_addr_copy(tp->netdev->dev_addr, sa->sa_data);
+		netdev_info(tp->netdev, "Using system MAC address %pM\n",
+			    sa->sa_data);
+		set_bit(MAC_PASSTHRU, &tp->flags);
+		mac_passthru_active = true;
+		ret = 1;

1 is not a "all is good" return value.

	ret = my_dell_specific_function();

But again, I don't like this, but I'm not the network subsystem
maintainer, I'll defer to them as to if this is something they want in
individual drivers...

thanks,

greg k-h

-----Original Message-----
From: Greg KH [mailto:gregkh@linuxfoundation.org]
Sent: Thursday, June 2, 2016 12:48 PM
To: Limonciello, Mario <redacted>
Cc: hayeswang@realtek.com; LKML <redacted>; Netdev
[off-list ref]; Linux USB [off-list ref];
pali.rohar@gmail.com; anthony.wong@canonical.com
Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
Auxiliary MAC address

On Thu, Jun 02, 2016 at 11:58:07AM -0500, Mario Limonciello wrote:

quoted

Dell systems with Type-C ports have support for a persistent system
specific MAC address when used with Dell Type-C docks and dongles.
This means a dock plugged into two different systems will show different
(but persistent) MAC addresses.  Dell Type-C docks and dongles use the
r8152 driver.

This information for the system's persistent MAC address is burned in

when

quoted

the HW is built and available under _SB\AMAC in the DSDT at runtime.

More information about the technology is available here:
http://www.dell.com/support/article/us/en/04/SLN301147

Signed-off-by: Mario Limonciello <redacted>
---
 drivers/net/usb/r8152.c | 53

+++++++++++++++++++++++++++++++++++++++++++++++++

quoted

 1 file changed, 53 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3f9f6ed..6dea542 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -26,6 +26,8 @@
 #include <linux/mdio.h>
 #include <linux/usb/cdc.h>
 #include <linux/suspend.h>
+#include <linux/acpi.h>
+#include <linux/dmi.h>

 /* Information for net-next */
 #define NETNEXT_VERSION		"08"

@@ -500,6 +502,7 @@ enum rtl8152_flags {
 	SELECTIVE_SUSPEND,
 	PHY_RESET,
 	SCHEDULE_NAPI,
+	MAC_PASSTHRU = 0,

Does setting that to 0 really work?  You just did this for two enum
values, what is the compiler supposed to do?

Very silly of me.  I was rushing to send a v2.  
I'm surprised this worked.  Shouldn't be assigned to anything.

quoted

 };

 /* Define these values to match your device */

@@ -653,6 +656,7 @@ enum tx_csum_stat {
  */
 static const int multicast_filter_limit = 32;
 static unsigned int agg_buf_sz = 16384;
+static bool mac_passthru_active;

very generic name for a platform-specific feature :(

Once this is broken up into an x86 platform provided method I'll rename this 
to platform_mac_active (or something similar).

quoted

 #define RTL_LIMITED_TSO_SIZE	(agg_buf_sz - sizeof(struct tx_desc) -

\

quoted

 				 VLAN_ETH_HLEN - VLAN_HLEN)

@@ -1030,6 +1034,49 @@ out1:
 	return ret;
 }

+static int get_auxiliary_addr(struct r8152 *tp, struct sockaddr *sa)

What about the platform mac address api that was pointed out?

I mentioned this in the cover letter - I haven't gotten a chance to move it over there yet.
I sent v2 before I did so that you can see what I've been doing as it was relevant to your
other comments.

quoted

+{
+	acpi_status status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	int ret = -1;
+	unsigned char buf[6];
+
+	if (!dmi_name_in_vendors("Dell Inc.") || mac_passthru_active)
+		return -1;

Don't make up random error values, please use "real" ones.

OK.

And you want to check this for all Dell devices?  Please be model
specific, I doubt a bunch of Dell servers wants to run this code...

Tracking model specific is really going to turn into a giant list never ending list.
To drill down more specifically, I can match on chassis too.

quoted

+
+	/* returns _AUXMAC_#AABBCCDDEEFF# */
+	status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);
+	obj = (union acpi_object *)buffer.pointer;
+	if (ACPI_SUCCESS(status)) {
+		if (obj->type != ACPI_TYPE_BUFFER ||
+		    obj->string.length != 0x17) {
+			pr_warn("r8152: get_auxiliary_addr: Invalid buffer");
+			goto amacout;
+		}
+		if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {
+			pr_warn("r8152: get_auxiliary_addr: Invalid header");
+			goto amacout;
+		}
+		ret = hex2bin(buf, obj->string.pointer + 9, 6);
+		if (ret < 0) {
+			pr_warn("r8152: get_auxiliary_addr: Invalid MAC");
+			goto amacout;
+		}
+		memcpy(sa->sa_data, buf, 6);
+		ether_addr_copy(tp->netdev->dev_addr, sa->sa_data);
+		netdev_info(tp->netdev, "Using system MAC address

%pM\n",

quoted

+			    sa->sa_data);
+		set_bit(MAC_PASSTHRU, &tp->flags);
+		mac_passthru_active = true;
+		ret = 1;

1 is not a "all is good" return value.

OK will switch to 0.

quoted

And you want to check this for all Dell devices?  Please be model
specific, I doubt a bunch of Dell servers wants to run this code...

Tracking model specific is really going to turn into a giant list never ending list.
To drill down more specifically, I can match on chassis too.

Does Dell happen to use its own USB Vendor ID for the USB device in
the dock? You could go at this problem from the other direction if it
does have a unique vendor ID.

     Andrew

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch]
Sent: Thursday, June 2, 2016 2:03 PM
To: Limonciello, Mario <redacted>
Cc: gregkh@linuxfoundation.org; hayeswang@realtek.com; linux-
kernel@vger.kernel.org; netdev@vger.kernel.org; linux-
usb@vger.kernel.org; pali.rohar@gmail.com; anthony.wong@canonical.com
Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
Auxiliary MAC address

quoted

quoted

And you want to check this for all Dell devices?  Please be model
specific, I doubt a bunch of Dell servers wants to run this code...

Tracking model specific is really going to turn into a giant list never ending

list.

quoted

To drill down more specifically, I can match on chassis too.

Does Dell happen to use its own USB Vendor ID for the USB device in
the dock? You could go at this problem from the other direction if it
does have a unique vendor ID.

     Andrew

Unfortunately it's not a Dell specific VID/PID.  I'm asking around to find out
if there is something else identifiable about this dock's NIC (maybe that r8152 
can query).

On Thu, Jun 02, 2016 at 07:04:32PM +0000, Mario_Limonciello@Dell.com wrote:

quoted

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch]
Sent: Thursday, June 2, 2016 2:03 PM
To: Limonciello, Mario <redacted>
Cc: gregkh@linuxfoundation.org; hayeswang@realtek.com; linux-
kernel@vger.kernel.org; netdev@vger.kernel.org; linux-
usb@vger.kernel.org; pali.rohar@gmail.com; anthony.wong@canonical.com
Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
Auxiliary MAC address

quoted

quoted

And you want to check this for all Dell devices?  Please be model
specific, I doubt a bunch of Dell servers wants to run this code...

Tracking model specific is really going to turn into a giant list never ending

list.

quoted

To drill down more specifically, I can match on chassis too.

Does Dell happen to use its own USB Vendor ID for the USB device in
the dock? You could go at this problem from the other direction if it
does have a unique vendor ID.

     Andrew

Unfortunately it's not a Dell specific VID/PID.  I'm asking around to find out
if there is something else identifiable about this dock's NIC (maybe that r8152 
can query).

lsusb -v

I assume there is a USB hub in the dock, maybe that has a Dell VID?
Going one level up the USB tree hierarchy should not be too hard.

      Andrew

On Thu, Jun 02, 2016 at 06:32:42PM +0000, Mario_Limonciello@Dell.com wrote:

quoted

And you want to check this for all Dell devices?  Please be model
specific, I doubt a bunch of Dell servers wants to run this code...

Tracking model specific is really going to turn into a giant list never ending list.
To drill down more specifically, I can match on chassis too.

Yes, as this is a vendor/platform-specific "quirk", you will have to
update it for each and every individual device you want it enabled as it
is so different from what all other drivers do.

thanks,

greg k-h

Mario Limonciello [mailto:mario_limonciello-8PEkshWhKlo@public.gmane.org]

Sent: Friday, June 03, 2016 12:58 AM

[...]

I don't think you have to give this a value.

 };

[...]

It still has problem when tp->version == RTL_VER_01.
First, you would read the current MAC address (MAC1) to sa.sa_data.
Then sa.sa_data may be modified by MAC2 after get_auxiliary_addr().
However, the MAC2 wouldn't be set to the device, because

		if (tp->version == RTL_VER_01)
			ether_addr_copy(dev->dev_addr, sa.sa_data);

Therefore, you would find that dev_addr is MAC2, and the device
uses MAC1.

Best Regards,
Hayes


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch]
Sent: Thursday, June 2, 2016 2:10 PM
To: Limonciello, Mario <redacted>
Cc: gregkh@linuxfoundation.org; hayeswang@realtek.com; linux-
kernel@vger.kernel.org; netdev@vger.kernel.org; linux-
usb@vger.kernel.org; pali.rohar@gmail.com; anthony.wong@canonical.com
Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
Auxiliary MAC address

On Thu, Jun 02, 2016 at 07:04:32PM +0000, Mario_Limonciello@Dell.com
wrote:

quoted

quoted

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch]
Sent: Thursday, June 2, 2016 2:03 PM
To: Limonciello, Mario <redacted>
Cc: gregkh@linuxfoundation.org; hayeswang@realtek.com; linux-
kernel@vger.kernel.org; netdev@vger.kernel.org; linux-
usb@vger.kernel.org; pali.rohar@gmail.com;

anthony.wong@canonical.com

quoted

quoted

Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
Auxiliary MAC address

quoted

quoted

And you want to check this for all Dell devices?  Please be model
specific, I doubt a bunch of Dell servers wants to run this code...

Tracking model specific is really going to turn into a giant list never

ending

quoted

quoted

list.

quoted

To drill down more specifically, I can match on chassis too.

Does Dell happen to use its own USB Vendor ID for the USB device in
the dock? You could go at this problem from the other direction if it
does have a unique vendor ID.

     Andrew

Unfortunately it's not a Dell specific VID/PID.  I'm asking around to find out
if there is something else identifiable about this dock's NIC (maybe that

r8152

quoted

can query).

lsusb -v

I assume there is a USB hub in the dock, maybe that has a Dell VID?
Going one level up the USB tree hierarchy should not be too hard.

      Andrew

I confirmed with some internal contacts a few extra bits about questions 
about this from throughout the conversation.

1) This feature IS in the general Realtek driver for Windows, not a special Dell driver.
2) The feature only offered to the Realtek 8153-AD, no other Realtek chip will do it 
(even Realtek 8153-VB won't).
3) The feature is activated when a special "Docking" bit is set on the 8153-AD.
4) The feature is activated on all Realtek 8153-AD's on the host system with the
docking bit set.  So on Windows if two Dell docks were hooked up they would both 
have the same AUX MAC. 

So considering that I think it would make sense to only activate in those same
special circumstances (RTL8153-AD, docking bit set, system has \\_SB.AMAC).  
I also feel this should stay in the Realtek driver as this as this implementation is
very Realtek specific.

-----Original Message-----
From: Hayes Wang [mailto:hayeswang@realtek.com]
Sent: Friday, June 3, 2016 4:44 AM
To: Limonciello, Mario <redacted>
Cc: LKML <redacted>; Netdev
[off-list ref]; Linux USB [off-list ref];
pali.rohar@gmail.com; anthony.wong@canonical.com; Greg KH
[off-list ref]
Subject: RE: [PATCH v2] r8152: Add support for setting MAC to system's
Auxiliary MAC address

Mario Limonciello [mailto:mario_limonciello@dell.com]

quoted

Sent: Friday, June 03, 2016 12:58 AM

[...]

quoted

@@ -500,6 +502,7 @@ enum rtl8152_flags {
 	SELECTIVE_SUSPEND,
 	PHY_RESET,
 	SCHEDULE_NAPI,
+	MAC_PASSTHRU = 0,

I don't think you have to give this a value.

quoted

 };

[...]

quoted

 static int set_ethernet_addr(struct r8152 *tp)
 {
 	struct net_device *dev = tp->netdev;

@@ -1041,6 +1088,10 @@ static int set_ethernet_addr(struct r8152 *tp)
 	else
 		ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);

+	/* if system provides auxiliary MAC address */
+	if (get_auxiliary_addr(tp, &sa))
+		ret = 0;

It still has problem when tp->version == RTL_VER_01.
First, you would read the current MAC address (MAC1) to sa.sa_data.
Then sa.sa_data may be modified by MAC2 after get_auxiliary_addr().
However, the MAC2 wouldn't be set to the device, because

		if (tp->version == RTL_VER_01)
			ether_addr_copy(dev->dev_addr, sa.sa_data);

Therefore, you would find that dev_addr is MAC2, and the device
uses MAC1.

Best Regards,
Hayes

Hayes,

From the information that I got that this is only a valid thing to do on

RTL-8153-AD (when dock bit set), should this only match with 
tp->version  >= RTL_VER_03? 

How can I confirm that the chip is 8153-AD specifically?  I didn't see
something obvious in driver to tell this.

Thanks,

-----Original Message-----
From: Greg KH [mailto:gregkh@linuxfoundation.org]
Sent: Thursday, June 2, 2016 9:02 PM
To: Limonciello, Mario <redacted>
Cc: hayeswang@realtek.com; linux-kernel@vger.kernel.org;
netdev@vger.kernel.org; linux-usb@vger.kernel.org; pali.rohar@gmail.com;
anthony.wong@canonical.com
Subject: Re: [PATCH v2] r8152: Add support for setting MAC to system's
Auxiliary MAC address

On Thu, Jun 02, 2016 at 06:32:42PM +0000, Mario_Limonciello@Dell.com
wrote:

quoted

quoted

And you want to check this for all Dell devices?  Please be model
specific, I doubt a bunch of Dell servers wants to run this code...

Tracking model specific is really going to turn into a giant list never ending

list.

quoted

To drill down more specifically, I can match on chassis too.

Yes, as this is a vendor/platform-specific "quirk", you will have to
update it for each and every individual device you want it enabled as it
is so different from what all other drivers do.

thanks,

greg k-h

After finding out that there is a characteristic specific to Realtek chip in this USB
device, I think it would be best to only run the \\_SB.AMAC lookup when this 
specific USB device is connected.  

Realtek is not maintaining a whitelist of systems in their driver for the other OS,
they approach it from the other end and only lookup auxiliary MAC if the right
device is plugged in.