[PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD

[Mario Limonciello]

Since this is a Realtek feature, I feel this shouldn't be moved into a platform
MAC address lookup. The code should only be run when the correct Realtek device
is plugged in.

Changes from v2:
* Only apply to RTL8153-AD w/ eFuse pass through mac address pass thru
bit set.
* Drop matching DMI information on Dell. Although this is implemented on
Dell, this is a Realtek feature that may may be implemented on other
OEMs as well.
* Test that pass through MAC address is valid, fall back to HW address if
invalid.
* Don't track status of which device has MAC pass through activated.
- Expected experience is that if two docks (RTL8153-AD's w/ mac pass thru
bit set) were plugged in both should have MAC pass through activated.

Mario Limonciello (1):
r8152: Add support for setting pass through MAC address on RTL8153-AD

drivers/net/usb/r8152.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 58 insertions(+), 2 deletions(-)

--
2.7.4

[Mario Limonciello]

The RTL8153-AD supports a persistent system specific MAC address.
This means a device plugged into two different systems with host side
support will show different (but persistent) MAC addresses.

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

This technology is currently implemented in the Dell TB15 and WD15 Type-C
docks. More information is available here:
http://www.dell.com/support/article/us/en/04/SLN301147

Signed-off-by: Mario Limonciello <mario_li...@dell.com>
---

drivers/net/usb/r8152.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 58 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3f9f6ed..f4bd46d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -26,6 +26,7 @@
#include <linux/mdio.h>
#include <linux/usb/cdc.h>
#include <linux/suspend.h>
+#include <linux/acpi.h>

/* Information for net-next */
#define NETNEXT_VERSION "08"
@@ -455,6 +456,11 @@
/* SRAM_IMPEDANCE */
#define RX_DRIVING_MASK 0x6000

+/* MAC PASSTHRU */
+#define AD_MASK 0xfee0
+#define EFUSE 0xcfdb
+#define PASS_THRU_MASK 0x1
+
enum rtl_register_content {
_1000bps = 0x10,
_100bps = 0x08,
@@ -1030,6 +1036,53 @@ out1:
return ret;
}

+static int get_passthru_addr(struct r8152 *tp, struct sockaddr *sa)
+{
+ acpi_status status;
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *obj;
+ int ret = -EINVAL;
+ u32 ocp_data;
+ unsigned char buf[6];
+
+ ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+ if ((ocp_data & AD_MASK) != 0x1000)
+ return -ENODEV;
+
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
+ if ((ocp_data & PASS_THRU_MASK) != 1)
+ return -ENODEV;
+
+ /* 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_passthru_addr: Invalid buffer");
+ goto amacout;
+ }
+ if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {
+ pr_warn("r8152: get_passthru_addr: Invalid header");
+ goto amacout;
+ }
+ ret = hex2bin(buf, obj->string.pointer + 9, 6);
+ if (ret < 0 || !is_valid_ether_addr(buf)) {
+ pr_warn("r8152: get_passthru_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 pass-through MAC address %pM\n",
+ sa->sa_data);
+ ret = 0;
+ }
+
+amacout:
+ kfree(obj);
+ return ret;
+}
+
static int set_ethernet_addr(struct r8152 *tp)
{
struct net_device *dev = tp->netdev;
@@ -1038,8 +1091,11 @@ static int set_ethernet_addr(struct r8152 *tp)

if (tp->version == RTL_VER_01)
ret = pla_ocp_read(tp, PLA_IDR, 8, sa.sa_data);
- else
- ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
+ else {
+ ret = get_passthru_addr(tp, &sa);
+ if (ret < 0)
+ ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
+ }

if (ret < 0) {
netif_err(tp, probe, dev, "Get ether addr fail\n");
--
2.7.4

[Greg KH]

I see no "Dell" specific markings or tests here at all, please be
EXPLICIT in the code exactly what this is for, and what it is doing.
Otherwise no one can tell you what machines/devices this will trigger on
at all.

As it is, it's totally vague and unknown what this whole function is
here for.

an "error" here is a "normal" thing, so please document it.

Or again, change the function to be "dell_crazy_hardware_hack_enabled()"
so it'b obvious what is going on.

greg k-h

[Mario_Li...@dell.com]

> -----Original Message-----
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, June 6, 2016 9:40 AM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: haye...@realtek.com; LKML <linux-...@vger.kernel.org>; Netdev
> <net...@vger.kernel.org>; Linux USB <linu...@vger.kernel.org>;
> pali....@gmail.com; anthon...@canonical.com
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>

> On Mon, Jun 06, 2016 at 09:15:20AM -0500, Mario Limonciello wrote:
> > Since this is a Realtek feature, I feel this shouldn't be moved into a platform
> > MAC address lookup. The code should only be run when the correct
> Realtek device
> > is plugged in.
> >
> > Changes from v2:
> > * Only apply to RTL8153-AD w/ eFuse pass through mac address pass thru
> > bit set.
> > * Drop matching DMI information on Dell. Although this is implemented
> on
> > Dell, this is a Realtek feature that may may be implemented on other
> > OEMs as well.
> > * Test that pass through MAC address is valid, fall back to HW address if
> > invalid.
> > * Don't track status of which device has MAC pass through activated.
> > - Expected experience is that if two docks (RTL8153-AD's w/ mac pass thru
> > bit set) were plugged in both should have MAC pass through activated.
>

> cover letters for single-patch submissions is overkill and confusing,
> please don't.

I was trying to convey differences between versions of this patch, I'll avoid
that in the future and let the audience find them themselves.

[Andrew Lunn]

> + /* 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_passthru_addr: Invalid buffer");

Please don't use pr_warn() when you can use the better alternatives
like dev_warn().

Andrew

[Greg KH]

On Mon, Jun 06, 2016 at 09:15:20AM -0500, Mario Limonciello wrote:

> Since this is a Realtek feature, I feel this shouldn't be moved into a platform
> MAC address lookup. The code should only be run when the correct Realtek device
> is plugged in.
>
> Changes from v2:
> * Only apply to RTL8153-AD w/ eFuse pass through mac address pass thru
> bit set.
> * Drop matching DMI information on Dell. Although this is implemented on
> Dell, this is a Realtek feature that may may be implemented on other
> OEMs as well.
> * Test that pass through MAC address is valid, fall back to HW address if
> invalid.
> * Don't track status of which device has MAC pass through activated.
> - Expected experience is that if two docks (RTL8153-AD's w/ mac pass thru
> bit set) were plugged in both should have MAC pass through activated.

[Mario_Li...@dell.com]

> -----Original Message-----
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Monday, June 6, 2016 9:50 AM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: haye...@realtek.com; linux-...@vger.kernel.org;
> net...@vger.kernel.org; linu...@vger.kernel.org; pali....@gmail.com;
> anthon...@canonical.com
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>

> No, put them in the patch itself, under the --- line, like is documented
> to do so. Don't make people "find them themselves", if you do that,
> your patch will just be ignored.
>
> greg k-h

Sorry I was not aware of that. I'll do that in the next patch.

[Mario_Li...@dell.com]

> -----Original Message-----
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Monday, June 6, 2016 9:41 AM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: haye...@realtek.com; LKML <linux-...@vger.kernel.org>; Netdev
> <net...@vger.kernel.org>; Linux USB <linu...@vger.kernel.org>;

> pali....@gmail.com; anthon...@canonical.com; Greg KH
> <gre...@linuxfoundation.org>
> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>

OK thanks, I'll adjust that to use a different warn.

[Mario_Li...@dell.com]

> -----Original Message-----
> From: Greg KH [mailto:gre...@linuxfoundation.org]

> Sent: Monday, June 6, 2016 9:40 AM
> To: Limonciello, Mario <Mario_Li...@Dell.com>
> Cc: haye...@realtek.com; LKML <linux-...@vger.kernel.org>; Netdev
> <net...@vger.kernel.org>; Linux USB <linu...@vger.kernel.org>;
> pali....@gmail.com; anthon...@canonical.com

> Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>

I intentionally removed the Dell tests. I tried to go into this in the cover
Letter as I knew there would be discussion about it.

This is intended because the better approach (as mentioned by this ML)
is to look for the exact chip that supports this feature, and query the bit
that can be set on that device's eFuse to indicate it supports this feature.

That device is an RTL8153-AD and there is a bit on the eFuse for MAC
pass through.

The ACPI code is intended to run on any device that has one of these
Devices plugged in. From what I learned discussing with others is that
is the approach the Realtek driver takes on the Windows side as well.

If the Dell TB15 (containin RTL8153-AD with this pass through bit set)
is plugged into for example an HP machine with the latest Realtek driver
installed this ACPI query will be run on Windows too. It will fail and fall
back to the address burned into the HW.

If the netdev maintainers still want a DMI check put in place again
here I'll put it back in, but I really think this is overkill.

OK.

>
> Or again, change the function to be "dell_crazy_hardware_hack_enabled()"
> so it'b obvious what is going on.

I'm really trying to be a good citizen here, just because it is implemented in
Dell HW first doesn't mean it won't be implemented by our competitors too
with their docks and dongles that get provided by Realtek.

Realtek has this in their Windows driver that all OEM's will be taking.
Another OEM would just need to burn the right information into the SPI at
manufacturing and expose it to the DSDT.

If considering all my above comments you still want it to be Dell specific I'll
return the DMI system vendor match.
I really don't think the netdev maintainers are going to want a list of every
specific DMI product string that Dell machine has this though. Maybe
they can speak up here.

[Greg KH]

On Mon, Jun 06, 2016 at 02:43:37PM +0000, Mario_Li...@Dell.com wrote:

[Greg KH]

I'm not asking for a DMI check, I'm asking for the ability to look at
this code, no changelog text at all, and know what is going on and what

this whole function is here for.

Right now I can't do that. No one can. You have documented what the
code does in it, but not described _why_ it is doing that at all.

That's fine, then call it "crazy_vendor_mac_passthrough() or something
like that. "get_passthru_addr()" makes this look like it is a "normal"
thing to have happen, which really, it isn't at all (hence these long
email threads.)

> Realtek has this in their Windows driver that all OEM's will be taking.
> Another OEM would just need to burn the right information into the SPI at
> manufacturing and expose it to the DSDT.

Where it the match up for the Realtek bit to corrispond with this
specific ACPI field? If it's not in the ACPI spec, then vendors _WILL_
do this in different ways.

Again, document it, in the code, what is going on here, that's all I'm
asking. I'm not asking you to change the logic at all!

> If considering all my above comments you still want it to be Dell specific I'll
> return the DMI system vendor match.
> I really don't think the netdev maintainers are going to want a list of every
> specific DMI product string that Dell machine has this though. Maybe
> they can speak up here.

If I were them, I sure would :)

good luck!

greg k-h

[Mario Limonciello]

The RTL8153-AD supports a persistent system specific MAC address.
This means a device plugged into two different systems with host side
support will show different (but persistent) MAC addresses.

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

This technology is currently implemented in the Dell TB15 and WD15 Type-C
docks. More information is available here:
http://www.dell.com/support/article/us/en/04/SLN301147

Signed-off-by: Mario Limonciello <mario_li...@dell.com>
---

Changes from v3:
* Add additional comments about functions and what they're doing
* Adjust warning calls to use netif instead
* Rename function

drivers/net/usb/r8152.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 68 insertions(+), 2 deletions(-)

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

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -26,6 +26,7 @@
#include <linux/mdio.h>
#include <linux/usb/cdc.h>
#include <linux/suspend.h>
+#include <linux/acpi.h>

/* Information for net-next */
#define NETNEXT_VERSION "08"
@@ -455,6 +456,11 @@
/* SRAM_IMPEDANCE */
#define RX_DRIVING_MASK 0x6000

+/* MAC PASSTHRU */
+#define AD_MASK 0xfee0
+#define EFUSE 0xcfdb
+#define PASS_THRU_MASK 0x1
+
enum rtl_register_content {
_1000bps = 0x10,
_100bps = 0x08,

@@ -1030,6 +1036,59 @@ out1:
return ret;
}

+/* Devices containing RTL8153-AD can support a persistent
+ * host system provided MAC address.
+ * Examples of this are Dell TB15 and Dell WD15 docks
+ */
+static int get_vendor_mac_passthru_addr(struct r8152 *tp, struct sockaddr *sa)

+{
+ acpi_status status;
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *obj;
+ int ret = -EINVAL;
+ u32 ocp_data;
+ unsigned char buf[6];
+

+ /* test for -AD variant of RTL8153 */

+ ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+ if ((ocp_data & AD_MASK) != 0x1000)
+ return -ENODEV;
+

+ /* test for MAC address pass-through bit */

+ ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
+ if ((ocp_data & PASS_THRU_MASK) != 1)
+ return -ENODEV;
+
+ /* 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) {

+ netif_warn(tp, probe, tp->netdev, "Invalid buffer\n");

+ goto amacout;
+ }
+ if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {

+ netif_warn(tp, probe, tp->netdev, "Invalid header\n");

+ goto amacout;
+ }
+ ret = hex2bin(buf, obj->string.pointer + 9, 6);
+ if (ret < 0 || !is_valid_ether_addr(buf)) {

+ netif_warn(tp, probe, tp->netdev, "Invalid MAC\n");

+ goto amacout;
+ }
+ memcpy(sa->sa_data, buf, 6);
+ ether_addr_copy(tp->netdev->dev_addr, sa->sa_data);

+ netif_info(tp, probe, tp->netdev,
+ "Using pass-through MAC addr %pM\n", sa->sa_data);

+ ret = 0;
+ }
+
+amacout:
+ kfree(obj);
+ return ret;
+}
+
static int set_ethernet_addr(struct r8152 *tp)
{
struct net_device *dev = tp->netdev;

@@ -1038,8 +1097,15 @@ static int set_ethernet_addr(struct r8152 *tp)

if (tp->version == RTL_VER_01)
ret = pla_ocp_read(tp, PLA_IDR, 8, sa.sa_data);
- else
- ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
+ else {

+ /* if this is not an RTL8153-AD, no eFuse mac pass thru set,
+ * or system doesn't provide valid _SB.AMAC this will be
+ * be expected to non-zero
+ */
+ ret = get_vendor_mac_passthru_addr(tp, &sa);
+ if (ret < 0)
+ ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
+ }

[Mario_Li...@dell.com]

> > Realtek has this in their Windows driver that all OEM's will be taking.
> > Another OEM would just need to burn the right information into the SPI at
> > manufacturing and expose it to the DSDT.
>
> Where it the match up for the Realtek bit to corrispond with this
> specific ACPI field? If it's not in the ACPI spec, then vendors _WILL_
> do this in different ways.
>
> Again, document it, in the code, what is going on here, that's all I'm
> asking. I'm not asking you to change the logic at all!

I've added additional comments for v4. I strongly believe that even if
another vendor does do this differently for their implementation of
\\_SB.AMAC this code will be safe to run.

All of the output from the field are tested for exactly what the field
should look like.

That said, I would be highly surprised if Realtek decided to implement
with another OEM differently. It would increase their code complexity
on Windows as well since this is part of the generic driver.

[Greg KH]

On Mon, Jun 06, 2016 at 05:24:57PM +0000, Mario_Li...@Dell.com wrote:
> That said, I would be highly surprised if Realtek decided to implement
> with another OEM differently. It would increase their code complexity
> on Windows as well since this is part of the generic driver.

Ah, it's refreshing to see people who haven't dealt with BIOS and system
vendors for very long, your good attitude is a wonderful sign :)

Seriously, if there is ANY chance that it could be broken or changed, it
will be. I place the odds that your next hardware product will do just
this for no obvious reason at all and am willing to buy the beer if it
doesn't happen.

thanks,

greg k-h

[Mario_Li...@dell.com]

> -----Original Message-----
> From: Konstantin Shkolnyy [mailto:Konstanti...@silabs.com]
> Sent: Monday, June 6, 2016 12:43 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>;
> haye...@realtek.com
> Cc: LKML <linux-...@vger.kernel.org>; Netdev
> <net...@vger.kernel.org>; Linux USB <linu...@vger.kernel.org>;
> pali....@gmail.com; anthon...@canonical.com; Greg KH
> <gre...@linuxfoundation.org>
> Subject: RE: [EXT] [PATCH v4] r8152: Add support for setting pass through
> MAC address on RTL8153-AD
>

> > -----Original Message-----
> > From: linux-u...@vger.kernel.org [mailto:linux-usb-
> > ow...@vger.kernel.org] On Behalf Of Mario Limonciello
> > Sent: Monday, June 06, 2016 12:19
> > To: haye...@realtek.com
> > Cc: LKML; Netdev; Linux USB; pali....@gmail.com;
> > anthon...@canonical.com; Greg KH; Mario Limonciello
> > Subject: [EXT] [PATCH v4] r8152: Add support for setting pass through MAC
> > address on RTL8153-AD
> >

> > The RTL8153-AD supports a persistent system specific MAC address.
> > This means a device plugged into two different systems with host side
> > support will show different (but persistent) MAC addresses.
> >
> > This information for the system's persistent MAC address is burned in
> when
> > the system HW is built and available under _SB\AMAC in the DSDT at
> > runtime.
> >
> > This technology is currently implemented in the Dell TB15 and WD15 Type-C
> > docks. More information is available here:
> > http://www.dell.com/support/article/us/en/04/SLN301147
>

> What is going to happen if I connect multiple dongles? Will they all get the
> same address?

If you connect a dongle without a RTL8153-AD or without that bit they'll
get the MAC that was burned into the dongle.

If you connect multiple docks that have this Realtek chip with this bit set,
yes they'll all get the same address.
I confirmed that's what happens on Windows too.

[Konstantin Shkolnyy]

> -----Original Message-----
> From: linux-u...@vger.kernel.org [mailto:linux-usb-
> ow...@vger.kernel.org] On Behalf Of Mario Limonciello
> Sent: Monday, June 06, 2016 12:19
> To: haye...@realtek.com
> Cc: LKML; Netdev; Linux USB; pali....@gmail.com;
> anthon...@canonical.com; Greg KH; Mario Limonciello
> Subject: [EXT] [PATCH v4] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>

> The RTL8153-AD supports a persistent system specific MAC address.
> This means a device plugged into two different systems with host side
> support will show different (but persistent) MAC addresses.
>
> This information for the system's persistent MAC address is burned in when
> the system HW is built and available under _SB\AMAC in the DSDT at
> runtime.
>
> This technology is currently implemented in the Dell TB15 and WD15 Type-C
> docks. More information is available here:
> http://www.dell.com/support/article/us/en/04/SLN301147

[Pali Rohár]

Hi! Below are my comments, all about coding style.

On Monday 06 June 2016 12:19:29 Mario Limonciello wrote:
> The RTL8153-AD supports a persistent system specific MAC address.
> This means a device plugged into two different systems with host side
> support will show different (but persistent) MAC addresses.
>
> This information for the system's persistent MAC address is burned in when
> the system HW is built and available under _SB\AMAC in the DSDT at runtime.

For consistency it would be great to use same ACPI name in whole patch: \_SB.AMAC

What about?

if (!ACPI_SUCCESS(status))
return -ENODEV;

You will save one level of indentation.

> + obj = (union acpi_object *)buffer.pointer;
> + if (ACPI_SUCCESS(status)) {
> + if (obj->type != ACPI_TYPE_BUFFER ||
> + obj->string.length != 0x17) {
> + netif_warn(tp, probe, tp->netdev, "Invalid buffer\n");

I would suggest more specific warning messages. These are very general
and if I would see them in dmesg log I would have no idea what that
means.

> + goto amacout;
> + }
> + if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) {
> + netif_warn(tp, probe, tp->netdev, "Invalid header\n");
> + goto amacout;
> + }

If your specification state that last character is '#' then I think you
should check it too.

You do not "get" (return) any mac address. Personally I would use "read"
word in function name (like above pla_ocp_read). What about?

ret = vendor_mac_passthru_addr_read(tp, sa.sa_data);

Or something similar... "Get" looks like function "get" something, but
instead it set address in &sa structure.

> + if (ret < 0)
> + ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
> + }
>
> if (ret < 0) {
> netif_err(tp, probe, dev, "Get ether addr fail\n");

--

Pali Rohár
pali....@gmail.com

[Hayes Wang]

Mario Limonciello [mailto:mario_li...@dell.com]
[...]

> + ret = hex2bin(buf, obj->string.pointer + 9, 6);
> + if (ret < 0 || !is_valid_ether_addr(buf)) {
> + netif_warn(tp, probe, tp->netdev, "Invalid MAC\n");
> + goto amacout;
> + }

If hex2bin() is success, the ret would be 0.
And you would return 0 when !is_valid_ether_addr(buf) occurs.
I don't think it is what you wants.

Best Regards,
Hayes

[Mario Limonciello]

The RTL8153-AD supports a persistent system specific MAC address.
This means a device plugged into two different systems with host side
support will show different (but persistent) MAC addresses.

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

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

This technology is currently implemented in the Dell TB15 and WD15 Type-C
docks. More information is available here:
http://www.dell.com/support/article/us/en/04/SLN301147

Signed-off-by: Mario Limonciello <mario_li...@dell.com>
---

Changes from v4:
* Correct style for all of Pali's comments
* Add extra check for trailing # in ACPI request
* Correct logic test for invalid MAC address to cover hex2bin ret 0,
but valid_is_ether being invalid

drivers/net/usb/r8152.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3f9f6ed..5a04c59 100644

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -26,6 +26,7 @@
#include <linux/mdio.h>
#include <linux/usb/cdc.h>
#include <linux/suspend.h>
+#include <linux/acpi.h>

/* Information for net-next */
#define NETNEXT_VERSION "08"
@@ -455,6 +456,11 @@
/* SRAM_IMPEDANCE */
#define RX_DRIVING_MASK 0x6000

+/* MAC PASSTHRU */
+#define AD_MASK 0xfee0
+#define EFUSE 0xcfdb
+#define PASS_THRU_MASK 0x1
+
enum rtl_register_content {
_1000bps = 0x10,
_100bps = 0x08,

@@ -1030,6 +1036,66 @@ out1:

return ret;
}

+/* Devices containing RTL8153-AD can support a persistent
+ * host system provided MAC address.
+ * Examples of this are Dell TB15 and Dell WD15 docks
+ */

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

+{
+ acpi_status status;
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *obj;
+ int ret = -EINVAL;
+ u32 ocp_data;
+ unsigned char buf[6];
+
+ /* test for -AD variant of RTL8153 */
+ ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+ if ((ocp_data & AD_MASK) != 0x1000)
+ return -ENODEV;
+
+ /* test for MAC address pass-through bit */
+ ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
+ if ((ocp_data & PASS_THRU_MASK) != 1)
+ return -ENODEV;
+
+ /* returns _AUXMAC_#AABBCCDDEEFF# */
+ status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);

+ obj = (union acpi_object *)buffer.pointer;

+ if (!ACPI_SUCCESS(status))
+ return -ENODEV;
+ if (obj->type != ACPI_TYPE_BUFFER || obj->string.length != 0x17) {

+ netif_warn(tp, probe, tp->netdev,

+ "Invalid buffer when reading pass-thru MAC addr: "
+ "(%d, %d)\n",
+ obj->type, obj->string.length);

+ goto amacout;
+ }
+ if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0 ||

+ strncmp(obj->string.pointer + 0x15, "#", 1) != 0) {

+ netif_warn(tp, probe, tp->netdev,

+ "Invalid header when reading pass-thru MAC addr\n");
+ goto amacout;
+ }

+ ret = hex2bin(buf, obj->string.pointer + 9, 6);

+ if (!(ret == 0 && is_valid_ether_addr(buf))) {

+ netif_warn(tp, probe, tp->netdev,

+ "Invalid MAC when reading pass-thru MAC addr: "
+ "%pM\n",
+ buf);

+ goto amacout;
+ }
+ memcpy(sa->sa_data, buf, 6);
+ ether_addr_copy(tp->netdev->dev_addr, sa->sa_data);
+ netif_info(tp, probe, tp->netdev,

+ "Using pass-thru MAC addr %pM\n", sa->sa_data);

+ ret = 0;
+

+amacout:
+ kfree(obj);
+ return ret;
+}
+
static int set_ethernet_addr(struct r8152 *tp)
{
struct net_device *dev = tp->netdev;

@@ -1038,8 +1104,15 @@ static int set_ethernet_addr(struct r8152 *tp)

if (tp->version == RTL_VER_01)
ret = pla_ocp_read(tp, PLA_IDR, 8, sa.sa_data);
- else
- ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
+ else {
+ /* if this is not an RTL8153-AD, no eFuse mac pass thru set,
+ * or system doesn't provide valid _SB.AMAC this will be
+ * be expected to non-zero
+ */

+ ret = vendor_mac_passthru_addr_read(tp, &sa);

+ if (ret < 0)
+ ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
+ }

if (ret < 0) {
netif_err(tp, probe, dev, "Get ether addr fail\n");
--

2.7.4

[Pali Rohár]

Hi! Another problem which I found now:

On Tuesday 07 June 2016 10:33:47 Mario Limonciello wrote:
> + ret = hex2bin(buf, obj->string.pointer + 9, 6);
> + if (!(ret == 0 && is_valid_ether_addr(buf))) {
> + netif_warn(tp, probe, tp->netdev,
> + "Invalid MAC when reading pass-thru MAC addr: "
> + "%pM\n",
> + buf);
> + goto amacout;
> + }

In case when hex2bin returns zero, but is_valid_ether_addr returns
false, this function returns also zero. And thats wrong, because error
occur.

--
Pali Rohár
pali....@gmail.com

[Mario Limonciello]

The RTL8153-AD supports a persistent system specific MAC address.
This means a device plugged into two different systems with host side
support will show different (but persistent) MAC addresses.

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

This technology is currently implemented in the Dell TB15 and WD15 Type-C
docks. More information is available here:
http://www.dell.com/support/article/us/en/04/SLN301147

Signed-off-by: Mario Limonciello <mario_li...@dell.com>
---

Changes from v5:
* Correct return value if hex2bin succesful but invalid ether addr

drivers/net/usb/r8152.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 74 insertions(+), 2 deletions(-)

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

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -26,6 +26,7 @@
#include <linux/mdio.h>
#include <linux/usb/cdc.h>
#include <linux/suspend.h>
+#include <linux/acpi.h>

/* Information for net-next */
#define NETNEXT_VERSION "08"
@@ -455,6 +456,11 @@
/* SRAM_IMPEDANCE */
#define RX_DRIVING_MASK 0x6000

+/* MAC PASSTHRU */
+#define AD_MASK 0xfee0
+#define EFUSE 0xcfdb
+#define PASS_THRU_MASK 0x1
+
enum rtl_register_content {
_1000bps = 0x10,
_100bps = 0x08,

@@ -1030,6 +1036,65 @@ out1:

+ netif_warn(tp, probe, tp->netdev,

+ "Invalid buffer when reading pass-thru MAC addr: "
+ "(%d, %d)\n",
+ obj->type, obj->string.length);
+ goto amacout;
+ }
+ if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0 ||
+ strncmp(obj->string.pointer + 0x15, "#", 1) != 0) {

+ netif_warn(tp, probe, tp->netdev,

+ "Invalid header when reading pass-thru MAC addr\n");
+ goto amacout;
+ }

+ ret = hex2bin(buf, obj->string.pointer + 9, 6);
+ if (!(ret == 0 && is_valid_ether_addr(buf))) {
+ netif_warn(tp, probe, tp->netdev,
+ "Invalid MAC when reading pass-thru MAC addr: "

+ "%d, %pM\n", ret, buf);
+ ret = -EINVAL;

+ goto amacout;
+ }
+ memcpy(sa->sa_data, buf, 6);
+ ether_addr_copy(tp->netdev->dev_addr, sa->sa_data);
+ netif_info(tp, probe, tp->netdev,
+ "Using pass-thru MAC addr %pM\n", sa->sa_data);
+

+amacout:
+ kfree(obj);
+ return ret;
+}
+
static int set_ethernet_addr(struct r8152 *tp)
{
struct net_device *dev = tp->netdev;

@@ -1038,8 +1103,15 @@ static int set_ethernet_addr(struct r8152 *tp)

[David Miller]

From: Mario Limonciello <mario_li...@dell.com>
Date: Tue, 7 Jun 2016 13:22:37 -0500

> The RTL8153-AD supports a persistent system specific MAC address.
> This means a device plugged into two different systems with host side
> support will show different (but persistent) MAC addresses.
>
> This information for the system's persistent MAC address is burned in when
> the system HW is built and available under \_SB.AMAC in the DSDT at runtime.
>
> This technology is currently implemented in the Dell TB15 and WD15 Type-C
> docks. More information is available here:
> http://www.dell.com/support/article/us/en/04/SLN301147
>
> Signed-off-by: Mario Limonciello <mario_li...@dell.com>
> ---
> Changes from v5:
> * Correct return value if hex2bin succesful but invalid ether addr

Have things calmed down enough now that I can apply this?

Let's see some ACKs.

[Andrew Lunn]

Hi David

I think the code has reaching the level of maturity needed for
acceptance. So for the code quality:

Reviewed-by: Andrew Lunn <and...@lunn.ch>

What is still open is do we want to accept it at all? Do we accept the
concept of putting the same MAC address on multiple interfaces at
hotplug time? Do we trust BIOS vendors to not keep changing DSDT
property name, since it is not standardised?

Do we want this at all should be decided by somebody more senior then
those passing comments on the code.

Andrew

[David Miller]

From: Andrew Lunn <and...@lunn.ch>
Date: Sat, 11 Jun 2016 17:39:21 +0200

> What is still open is do we want to accept it at all? Do we accept the
> concept of putting the same MAC address on multiple interfaces at
> hotplug time? Do we trust BIOS vendors to not keep changing DSDT
> property name, since it is not standardised?
>
> Do we want this at all should be decided by somebody more senior then
> those passing comments on the code.

Indeed, I think the behavior of using the same MAC address on multiple
interfaces if we plug several of these in at once is not good.

We shouldn't behave this way just because the Microsoft driver does.

[Mario_Li...@dell.com]

> -----Original Message-----
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Saturday, June 11, 2016 12:42 PM
> To: and...@lunn.ch
> Cc: Limonciello, Mario <Mario_Li...@Dell.com>;
> haye...@realtek.com; linux-...@vger.kernel.org;
> net...@vger.kernel.org; linu...@vger.kernel.org; pali....@gmail.com;

> anthon...@canonical.com; gre...@linuxfoundation.org
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>

> From: Andrew Lunn <and...@lunn.ch>
> Date: Sat, 11 Jun 2016 17:39:21 +0200
>
> > What is still open is do we want to accept it at all? Do we accept the
> > concept of putting the same MAC address on multiple interfaces at
> > hotplug time? Do we trust BIOS vendors to not keep changing DSDT
> > property name, since it is not standardised?
> >

It's worth saying - standardized a property name doesn't indemnify it.
Properties change all the time from one version of a spec to another.

I can only speak for Dell, but it's in our best interest to keep the BIOS
side of the codebase around this simpler too.

> > Do we want this at all should be decided by somebody more senior then
> > those passing comments on the code.
>
> Indeed, I think the behavior of using the same MAC address on multiple
> interfaces if we plug several of these in at once is not good.
>
> We shouldn't behave this way just because the Microsoft driver does.

This is really grasping at an extreme corner case scenario.

Dell TB15 and WD15 docks are currently only ones on the market with
RTL8135-AD and MAC address pass through efuse bit set. These docks
are not inexpensive. If someone really wants multiple USB NIC's plugged
in, they can pick up a second USB NIC from the web for far cheaper than
buying a second dock.

Also for what it's worth, the docks don't allow daisy chaining. The dock EC's
will reject the second dock from functioning through the downstream
connection. The only way that two docks could be hooked up and
functional is on a machine with multiple type C ports.

If you still think it's worth solving, what would you like done as an
alternative? I would really like to have some implementation of this
that you guys are comfortable with upstream.

There was already discussion and an implementation in this
thread about tracking if the aux MAC was assigned to something and only
allowing one device to get that assignment. There was a limitation that
you won't be able to know which device gets the auxiliary MAC address.
It will be based solely upon hotplug order.

There was also in one version of the patch a way to turn off this behavior
In a module parameter. Greg KH wasn't fond of that, so it's not present
in the current version.

I can add either of those back in if they would help the case.

Another option I wanted to offer was turning this behavior on via kernel
configuration option and let the distros decide if they want to turn it on
for users.

[Pali Rohár]

I agree, but in some cases it is night mare for local admins when
booting different OS cause changing MAC address on local network.

Another similar situation: Imagine that you have two USB network cards
and both have "burned" into their registers same MAC address. If you
connect both those USB network cards, linux kernel bind appropriate
driver which read MAC address for both those cards. But those addresses
are same. What will linux kernel do in this case?

This is very similar situation as those Dell usb network cards told us
"hey, use address which is in ACPI DSDT table".

Either we should trust what network card what told us, or not and then
generate MAC addresses in better way.

Just my opinion...

--
Pali Rohár
pali....@gmail.com

[Greg KH]

On Tue, Jun 14, 2016 at 06:28:10PM +0200, Pali Rohár wrote:
> On Saturday 11 June 2016 19:42:26 David Miller wrote:
> > From: Andrew Lunn <and...@lunn.ch>
> > Date: Sat, 11 Jun 2016 17:39:21 +0200
> >
> > > What is still open is do we want to accept it at all? Do we accept
> > > the concept of putting the same MAC address on multiple interfaces
> > > at hotplug time? Do we trust BIOS vendors to not keep changing
> > > DSDT property name, since it is not standardised?
> > >
> > > Do we want this at all should be decided by somebody more senior
> > > then those passing comments on the code.
> >
> > Indeed, I think the behavior of using the same MAC address on
> > multiple interfaces if we plug several of these in at once is not
> > good.
> >
> > We shouldn't behave this way just because the Microsoft driver does.
>
> I agree, but in some cases it is night mare for local admins when
> booting different OS cause changing MAC address on local network.
>
> Another similar situation: Imagine that you have two USB network cards
> and both have "burned" into their registers same MAC address. If you
> connect both those USB network cards, linux kernel bind appropriate
> driver which read MAC address for both those cards. But those addresses
> are same. What will linux kernel do in this case?

If you can find such a broken USB device, try it and see :)

(hint, might be hard to find, I've never seen such a device before.)

I don't see how that pertains to this issue, sorry, how does broken USB
hardware compare to a working Dell device?

thanks,

greg k-h

[Pali Rohár]

What do you mean by broken USB device?

You have never seen two ethernet cards with same MAC addresses? Right I
have not seen two USB, but there is non zero chance that could happen.
Specially now when more and more people starts using USB network cards.

> (hint, might be hard to find, I've never seen such a device before.)
>
> I don't see how that pertains to this issue, sorry, how does broken
> USB hardware compare to a working Dell device?

It is same, how to handle two network cards which tell us, that they
have same MAC addresses.

--
Pali Rohár
pali....@gmail.com

[Mario_Li...@dell.com]

> -----Original Message-----
> From: Pali Rohár [mailto:pali....@gmail.com]
> Sent: Tuesday, June 14, 2016 11:48 AM
> To: Greg KH <gre...@linuxfoundation.org>
> Cc: David Miller <da...@davemloft.net>; and...@lunn.ch; Limonciello,
> Mario <Mario_Li...@Dell.com>; haye...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthon...@canonical.com
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>

The kernel handles this just fine. In doing this patch I checked to see
what it does in that scenario. Two devices are made. systemd doesn't
rename the second device via the MAC name (eg enxAABBCCDDEEFF).

[Andrew Lunn]

> > It is same, how to handle two network cards which tell us, that they
> > have same MAC addresses.
> >
>
> The kernel handles this just fine. In doing this patch I checked to see
> what it does in that scenario. Two devices are made. systemd doesn't
> rename the second device via the MAC name (eg enxAABBCCDDEEFF).

What does you dhcp server do? Does it gives out the same IP address?
You then have two interfaces on the same network, with the same MAC
address and IP address. Then what happens?

Andrew

[Mario_Li...@dell.com]

> -----Original Message-----
> From: Andrew Lunn [mailto:and...@lunn.ch]

> Sent: Tuesday, June 14, 2016 12:23 PM
> To: Limonciello, Mario <Mario_Li...@Dell.com>

> Cc: pali....@gmail.com; gre...@linuxfoundation.org;
> da...@davemloft.net; haye...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthon...@canonical.com
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>

I didn't test it on the same network, I used two separate networks.

I expect that the DHCP server would be awfully confused and you'd
run down an interesting problem path if it got the same MAC twice.

[David Miller]

From: Pali Rohár <pali....@gmail.com>
Date: Tue, 14 Jun 2016 18:47:36 +0200

> You have never seen two ethernet cards with same MAC addresses? Right I
> have not seen two USB, but there is non zero chance that could happen.

It would be an error scenerio, and something to be avoided.

It is a valid and correct assumption that one is able to put
several devices at the same time on the same physical network
and expect it to work.

The behavior added by the change in question invalidates that.

I'm trying to consider the long term aspects of this, which is that if
more devices adopt this scheme we're in trouble if we blindly
interpret the MAC address in this way.

This firmware MAC property facility seems to be designed with only an
extremely narrow use case being considered.

[Mario_Li...@dell.com]

> -----Original Message-----
> From: David Miller [mailto:da...@davemloft.net]

> Sent: Tuesday, June 14, 2016 1:35 PM
> To: pali....@gmail.com
> Cc: gre...@linuxfoundation.org; and...@lunn.ch; Limonciello, Mario
> <Mario_Li...@Dell.com>; haye...@realtek.com; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthon...@canonical.com
> Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>

> From: Pali Rohár <pali....@gmail.com>
> Date: Tue, 14 Jun 2016 18:47:36 +0200
>
> > You have never seen two ethernet cards with same MAC addresses? Right
> I
> > have not seen two USB, but there is non zero chance that could happen.
>
> It would be an error scenerio, and something to be avoided.
>
> It is a valid and correct assumption that one is able to put
> several devices at the same time on the same physical network
> and expect it to work.
>
> The behavior added by the change in question invalidates that.
>
> I'm trying to consider the long term aspects of this, which is that if
> more devices adopt this scheme we're in trouble if we blindly
> interpret the MAC address in this way.
>

Do you mean if other manufacturers start to ship devices with
RTL8135-AD's w/ this pass through bit set and people start to try to
mix and match?

> This firmware MAC property facility seems to be designed with only an
> extremely narrow use case being considered.

Yes, as I understand it this is the reason that it's only on such specific devices
that the mac address pass through bit is actually set on the efuse.

[Mario_Li...@dell.com]

> -----Original Message-----
> From: Limonciello, Mario
> Sent: Tuesday, June 14, 2016 5:27 PM
> To: 'David Miller' <da...@davemloft.net>; pali....@gmail.com
> Cc: gre...@linuxfoundation.org; and...@lunn.ch;
> haye...@realtek.com; linux-...@vger.kernel.org;
> net...@vger.kernel.org; linu...@vger.kernel.org;
> anthon...@canonical.com
> Subject: RE: [PATCH v6] r8152: Add support for setting pass through MAC
> address on RTL8153-AD
>
> > -----Original Message-----
> > From: David Miller [mailto:da...@davemloft.net]
> > Sent: Tuesday, June 14, 2016 1:35 PM
> > To: pali....@gmail.com
> > Cc: gre...@linuxfoundation.org; and...@lunn.ch; Limonciello, Mario
> > <Mario_Li...@Dell.com>; haye...@realtek.com; linux-
> > ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> > u...@vger.kernel.org; anthon...@canonical.com
> > Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC
> > address on RTL8153-AD
> >

> > From: Pali Rohár <pali....@gmail.com>
> > Date: Tue, 14 Jun 2016 18:47:36 +0200
> >
> > > You have never seen two ethernet cards with same MAC addresses?
> Right
> > I
> > > have not seen two USB, but there is non zero chance that could happen.
> >
> > It would be an error scenerio, and something to be avoided.
> >
> > It is a valid and correct assumption that one is able to put
> > several devices at the same time on the same physical network
> > and expect it to work.
> >
> > The behavior added by the change in question invalidates that.
> >
> > I'm trying to consider the long term aspects of this, which is that if
> > more devices adopt this scheme we're in trouble if we blindly
> > interpret the MAC address in this way.
> >
>

> Do you mean if other manufacturers start to ship devices with
> RTL8135-AD's w/ this pass through bit set and people start to try to
> mix and match?
>

> > This firmware MAC property facility seems to be designed with only an
> > extremely narrow use case being considered.
>

> Yes, as I understand it this is the reason that it's only on such specific devices
> that the mac address pass through bit is actually set on the efuse.

David,

Did you have any more thoughts about this? I'm happy to make some other
adjustments to the patch, if you have some recommendations.

[Mario_Li...@dell.com]

> David,
>
> Did you have any more thoughts about this? I'm happy to make some other
> adjustments to the patch, if you have some recommendations.

Hi,

I just wanted to share that the maintenance BIOSes released for the Dell
platforms with Type-C this past week enables the MAC address pass
through feature in UEFI, so any network booted machines will offer
the auxiliary MAC to the DHCP server. In Windows a network booted
machine will always use auxiliary MAC now.

XPS 9350 (BIOS 1.4.4): http://goo.gl/7Sw2DZ

Please let me know what else can be done for this patch to make it
acceptable so we can have parity for Linux.

Thanks,

[David Miller]

From: <Mario_Li...@Dell.com>
Date: Mon, 11 Jul 2016 21:54:07 +0000

> Please let me know what else can be done for this patch to make it
> acceptable so we can have parity for Linux.

Just resubmit it and I'll apply it, I'm so tired of hearing about this...

[Mario Limonciello]

The RTL8153-AD supports a persistent system specific MAC address.
This means a device plugged into two different systems with host side
support will show different (but persistent) MAC addresses.

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

This technology is currently implemented in the Dell TB15 and WD15 Type-C
docks. More information is available here:
http://www.dell.com/support/article/us/en/04/SLN301147

Signed-off-by: Mario Limonciello <mario_li...@dell.com>
---

drivers/net/usb/r8152.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c

index 0da72d3..2298f26 100644

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -26,6 +26,7 @@
#include <linux/mdio.h>
#include <linux/usb/cdc.h>
#include <linux/suspend.h>
+#include <linux/acpi.h>

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

@@ -460,6 +461,11 @@

/* SRAM_IMPEDANCE */
#define RX_DRIVING_MASK 0x6000

+/* MAC PASSTHRU */
+#define AD_MASK 0xfee0
+#define EFUSE 0xcfdb
+#define PASS_THRU_MASK 0x1
+
enum rtl_register_content {
_1000bps = 0x10,
_100bps = 0x08,

@@ -1036,6 +1042,65 @@ out1:

@@ -1044,8 +1109,15 @@ static int set_ethernet_addr(struct r8152 *tp)

[David Miller]

From: Mario Limonciello <mario_li...@dell.com>
Date: Mon, 11 Jul 2016 19:58:04 -0500

> The RTL8153-AD supports a persistent system specific MAC address.
> This means a device plugged into two different systems with host side
> support will show different (but persistent) MAC addresses.
>
> This information for the system's persistent MAC address is burned in when
> the system HW is built and available under \_SB.AMAC in the DSDT at runtime.
>
> This technology is currently implemented in the Dell TB15 and WD15 Type-C
> docks. More information is available here:
> http://www.dell.com/support/article/us/en/04/SLN301147
>
> Signed-off-by: Mario Limonciello <mario_li...@dell.com>

Applied.

[Michał Pecio]

Hi,

Isn't it possible that the same ACPI name could be used for other
vendor-specific extensions on other laptops and that r8152 will now
wreak havoc there?

Regards,
MP

[Mario_Li...@dell.com]

> -----Original Message-----
> From: Michał Pecio [mailto:michal...@gmail.com]
> Sent: Tuesday, July 12, 2016 2:03 AM
> To: David Miller <da...@davemloft.net>
> Cc: Limonciello, Mario <Mario_Li...@Dell.com>; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; linux-
> u...@vger.kernel.org; anthon...@canonical.com
> Subject: Re: [PATCH v6 RESEND] r8152: Add support for setting pass through
> MAC address on RTL8153-AD
>

This has been discussed a bit previously in earlier submissions.

In short, this is an extreme corner case. Some changes were made
to diminish potential impact. The ACPI code is resolved only when
the specific variant of RTL8135 (-AD) has a bit set on the efuse.

The patch also explicitly checks the return type and contents of the
ACPI object and won't proceed if it's invalid.

The Type-C devices that provide this are currently only sold by Dell.
This of course may change one day if other OEM's add this
feature, but it just shows that device scope is limited.

For now the way this is implemented if Realtek does choose to
work with another OEM on this feature, there should be no
kernel code change necessary for interoperability of peripherals
on other OEM machines.

So in order to hit this hypothetical corner case today you would
need to be using a real world type-C device something such as a
Dell WD15 on another OEM's machine that has type-C and the exact
same ACPI object name that does $BADSTUFF other than return a
buffer.

If that situation arises please alert me and I'll send a follow up
patch that whitelists this to match DMI vendor of only Dell
systems.

[Michał Pecio]

> So in order to hit this hypothetical corner case today you would
> need to be using a real world type-C device something such as a
> Dell WD15 on another OEM's machine that has type-C and the exact
> same ACPI object name that does $BADSTUFF other than return a
> buffer.

Exactly what I meant.

> If that situation arises please alert me and I'll send a follow up
> patch that whitelists this to match DMI vendor of only Dell
> systems.

I'm not sure if it's the most responsible way to approach this problem.
There were cases of laptops bricking (as in - they would never power on
ever again) just because Linux made some bad BIOS/ACPI calls. Also, see
the famous Samsung UEFI NVRAM fiasco or the recent pains with EFIVARS.

Regards,
MP