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

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

[Mario Limonciello]

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 avilable 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 <mario_limonciello@dell.com>
---
 drivers/net/usb/Kconfig |  1 +
 drivers/net/usb/r8152.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index cdde590..c320930 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -98,6 +98,7 @@ config USB_RTL8150
 config USB_RTL8152
 	tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
 	select MII
+	depends on ACPI
 	help
 	  This option adds support for Realtek RTL8152 based USB 2.0
 	  10/100 Ethernet adapters and RTL8153 based USB 3.0 10/100/1000
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3f9f6ed..62af3b4 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"
@@ -1030,6 +1031,39 @@ out1:
 	return ret;
 }
 
+static u8 amac_ascii_to_hex(int c)
+{
+	if (c <= 0x39)
+		return (u8)(c - 0x30);
+	else if (c <= 0x46)
+		return (u8)(c - 0x37);
+	return (u8)(c - 0x57);
+}
+
+static void set_auxiliary_addr(struct sockaddr *sa)
+{
+	acpi_status status;
+	acpi_handle handle;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	int i;
+	char *ptr;
+
+	acpi_get_handle(NULL, "\\_SB", &handle);
+	status = acpi_evaluate_object(handle, "AMAC", NULL, &buffer);
+	obj = (union acpi_object *)buffer.pointer;
+	if (ACPI_SUCCESS(status) && (obj->string.length == 0x17)) {
+		/* returns _AUXMAC_#AABBCCDDEEFF#
+		 * this pulls out _AUXMAC# from start and # from end
+		 */
+		ptr = obj->string.pointer + 9;
+		pr_info("r8152: Using system auxiliary MAC address");
+		for (i = 0; i < 6; i++, ptr += 2)
+			sa->sa_data[i] = amac_ascii_to_hex(*ptr) << 4 |
+					 amac_ascii_to_hex(*(ptr + 1));
+	}
+}
+
 static int set_ethernet_addr(struct r8152 *tp)
 {
 	struct net_device *dev = tp->netdev;
@@ -1041,6 +1075,9 @@ 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 */
+	set_auxiliary_addr(&sa);
+
 	if (ret < 0) {
 		netif_err(tp, probe, dev, "Get ether addr fail\n");
 	} else if (!is_valid_ether_addr(sa.sa_data)) {
-- 
2.7.4

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

[Andrew Lunn]

On Wed, Jun 01, 2016 at 04:50:44PM -0500, Mario Limonciello wrote:
> 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 avilable 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 <mario_limonciello@dell.com>
> ---
>  drivers/net/usb/Kconfig |  1 +
>  drivers/net/usb/r8152.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index cdde590..c320930 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -98,6 +98,7 @@ config USB_RTL8150
>  config USB_RTL8152
>  	tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
>  	select MII
> +	depends on ACPI

Hi Mario

That seems a bit heavy handed. What about ARM or MIPS machines which
don't use ACPI but do have USB ports where i could plug in a USB
dongle with this chipset.

I think it would be better to make use of ACPI if it is available, but
don't require it in order the build the driver.

      Andrew

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

[Mario_Limonciello]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Wednesday, June 1, 2016 5:27 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: hayeswang@realtek.com; LKML <linux-kernel@vger.kernel.org>; Netdev
> <netdev@vger.kernel.org>; Linux USB <linux-usb@vger.kernel.org>;
> pali.rohar@gmail.com; anthony.wong@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Wed, Jun 01, 2016 at 04:50:44PM -0500, Mario Limonciello wrote:
> > 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 avilable 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 <mario_limonciello@dell.com>
> > ---
> >  drivers/net/usb/Kconfig |  1 +
> >  drivers/net/usb/r8152.c | 37
> +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig index
> > cdde590..c320930 100644
> > --- a/drivers/net/usb/Kconfig
> > +++ b/drivers/net/usb/Kconfig
> > @@ -98,6 +98,7 @@ config USB_RTL8150
> >  config USB_RTL8152
> >  	tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
> >  	select MII
> > +	depends on ACPI
> 
> Hi Mario
> 
> That seems a bit heavy handed. What about ARM or MIPS machines which
> don't use ACPI but do have USB ports where i could plug in a USB dongle with
> this chipset.
> 
> I think it would be better to make use of ACPI if it is available, but don't
> require it in order the build the driver.
> 
>       Andrew

Hi Andrew,

Thanks for that feedback.  I'll adjust that to look for CONFIG_ACPI in the code instead in V2 after I get some additional comments for the implementation.

Thanks,

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

[Greg KH]

On Wed, Jun 01, 2016 at 10:31:52PM +0000, Mario_Limonciello@Dell.com wrote:
> > -----Original Message-----
> > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > Sent: Wednesday, June 1, 2016 5:27 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: hayeswang@realtek.com; LKML <linux-kernel@vger.kernel.org>; Netdev
> > <netdev@vger.kernel.org>; Linux USB <linux-usb@vger.kernel.org>;
> > pali.rohar@gmail.com; anthony.wong@canonical.com
> > Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> > Auxiliary MAC address
> > 
> > On Wed, Jun 01, 2016 at 04:50:44PM -0500, Mario Limonciello wrote:
> > > 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 avilable 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 <mario_limonciello@dell.com>
> > > ---
> > >  drivers/net/usb/Kconfig |  1 +
> > >  drivers/net/usb/r8152.c | 37
> > +++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 38 insertions(+)
> > >
> > > diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig index
> > > cdde590..c320930 100644
> > > --- a/drivers/net/usb/Kconfig
> > > +++ b/drivers/net/usb/Kconfig
> > > @@ -98,6 +98,7 @@ config USB_RTL8150
> > >  config USB_RTL8152
> > >  	tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
> > >  	select MII
> > > +	depends on ACPI
> > 
> > Hi Mario
> > 
> > That seems a bit heavy handed. What about ARM or MIPS machines which
> > don't use ACPI but do have USB ports where i could plug in a USB dongle with
> > this chipset.
> > 
> > I think it would be better to make use of ACPI if it is available, but don't
> > require it in order the build the driver.
> > 
> >       Andrew
> 
> Hi Andrew,
> 
> Thanks for that feedback.  I'll adjust that to look for CONFIG_ACPI in
> the code instead in V2 after I get some additional comments for the
> implementation.

No need to do that, the acpi functions should be "stubbed out" if that
option is not enabled, right?  You don't want #ifdefs in .c code if at
all possible.

thanks,

greg k-h

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

[Mario_Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, June 1, 2016 6:07 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: andrew@lunn.ch; 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] r8152: Add support for setting MAC to system's Auxiliary
> MAC address
> 
> On Wed, Jun 01, 2016 at 10:31:52PM +0000, Mario_Limonciello@Dell.com
> wrote:
> > > -----Original Message-----
> > > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > > Sent: Wednesday, June 1, 2016 5:27 PM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > > Cc: hayeswang@realtek.com; LKML <linux-kernel@vger.kernel.org>;
> > > Netdev <netdev@vger.kernel.org>; Linux USB
> > > <linux-usb@vger.kernel.org>; pali.rohar@gmail.com;
> > > anthony.wong@canonical.com
> > > Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> > > Auxiliary MAC address
> > >
> > > On Wed, Jun 01, 2016 at 04:50:44PM -0500, Mario Limonciello wrote:
> > > > 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 avilable 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 <mario_limonciello@dell.com>
> > > > ---
> > > >  drivers/net/usb/Kconfig |  1 +
> > > >  drivers/net/usb/r8152.c | 37
> > > +++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 38 insertions(+)
> > > >
> > > > diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> > > > index
> > > > cdde590..c320930 100644
> > > > --- a/drivers/net/usb/Kconfig
> > > > +++ b/drivers/net/usb/Kconfig
> > > > @@ -98,6 +98,7 @@ config USB_RTL8150  config USB_RTL8152
> > > >  	tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
> > > >  	select MII
> > > > +	depends on ACPI
> > >
> > > Hi Mario
> > >
> > > That seems a bit heavy handed. What about ARM or MIPS machines which
> > > don't use ACPI but do have USB ports where i could plug in a USB
> > > dongle with this chipset.
> > >
> > > I think it would be better to make use of ACPI if it is available,
> > > but don't require it in order the build the driver.
> > >
> > >       Andrew
> >
> > Hi Andrew,
> >
> > Thanks for that feedback.  I'll adjust that to look for CONFIG_ACPI in
> > the code instead in V2 after I get some additional comments for the
> > implementation.
> 
> No need to do that, the acpi functions should be "stubbed out" if that option is
> not enabled, right?  You don't want #ifdefs in .c code if at all possible.
> 
> thanks,
> 
> greg k-h

Thanks, yeah I do see them stubbed out, will skip #ifdefs and just drop the Kconfig dependency on ACPI.

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

[Greg KH]

On Wed, Jun 01, 2016 at 04:50:44PM -0500, Mario Limonciello wrote:
> 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 avilable 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 <mario_limonciello@dell.com>
> ---
>  drivers/net/usb/Kconfig |  1 +
>  drivers/net/usb/r8152.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index cdde590..c320930 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -98,6 +98,7 @@ config USB_RTL8150
>  config USB_RTL8152
>  	tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
>  	select MII
> +	depends on ACPI
>  	help
>  	  This option adds support for Realtek RTL8152 based USB 2.0
>  	  10/100 Ethernet adapters and RTL8153 based USB 3.0 10/100/1000
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 3f9f6ed..62af3b4 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"
> @@ -1030,6 +1031,39 @@ out1:
>  	return ret;
>  }
>  
> +static u8 amac_ascii_to_hex(int c)
> +{
> +	if (c <= 0x39)
> +		return (u8)(c - 0x30);
> +	else if (c <= 0x46)
> +		return (u8)(c - 0x37);
> +	return (u8)(c - 0x57);
> +}

We really don't have such a function somewhere in the kernel already?

And why 'int', isn't "c" really a u8?

> +static void set_auxiliary_addr(struct sockaddr *sa)
> +{
> +	acpi_status status;
> +	acpi_handle handle;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	int i;
> +	char *ptr;
> +
> +	acpi_get_handle(NULL, "\\_SB", &handle);
> +	status = acpi_evaluate_object(handle, "AMAC", NULL, &buffer);

Is this field in the ACPI standard, or should this only be "trusted" on
a limited number of machines (i.e. with Dell DMI strings?)

And finally, this seems odd overall given that a MAC address should be
associated with the specific network device, not the overall system.
What if you have 2 types of devices plugged into the same machine, with
this patch you suddenly have the same MAC address for both of them.
That doesn't seem correct...

thanks,

greg k-h

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

[Mario_Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, June 1, 2016 6:06 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: hayeswang@realtek.com; LKML <linux-kernel@vger.kernel.org>; Netdev
> <netdev@vger.kernel.org>; Linux USB <linux-usb@vger.kernel.org>;
> pali.rohar@gmail.com; anthony.wong@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary
> MAC address
> 
> On Wed, Jun 01, 2016 at 04:50:44PM -0500, Mario Limonciello wrote:
> > 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 avilable 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 <mario_limonciello@dell.com>
> > ---
> >  drivers/net/usb/Kconfig |  1 +
> >  drivers/net/usb/r8152.c | 37 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig index
> > cdde590..c320930 100644
> > --- a/drivers/net/usb/Kconfig
> > +++ b/drivers/net/usb/Kconfig
> > @@ -98,6 +98,7 @@ config USB_RTL8150
> >  config USB_RTL8152
> >  	tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
> >  	select MII
> > +	depends on ACPI
> >  	help
> >  	  This option adds support for Realtek RTL8152 based USB 2.0
> >  	  10/100 Ethernet adapters and RTL8153 based USB 3.0 10/100/1000
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 3f9f6ed..62af3b4 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"
> > @@ -1030,6 +1031,39 @@ out1:
> >  	return ret;
> >  }
> >
> > +static u8 amac_ascii_to_hex(int c)
> > +{
> > +	if (c <= 0x39)
> > +		return (u8)(c - 0x30);
> > +	else if (c <= 0x46)
> > +		return (u8)(c - 0x37);
> > +	return (u8)(c - 0x57);
> > +}
> 
> We really don't have such a function somewhere in the kernel already?
> 
> And why 'int', isn't "c" really a u8?
> 
> > +static void set_auxiliary_addr(struct sockaddr *sa) {
> > +	acpi_status status;
> > +	acpi_handle handle;
> > +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +	union acpi_object *obj;
> > +	int i;
> > +	char *ptr;
> > +
> > +	acpi_get_handle(NULL, "\\_SB", &handle);
> > +	status = acpi_evaluate_object(handle, "AMAC", NULL, &buffer);
> 
> Is this field in the ACPI standard, or should this only be "trusted" on a limited
> number of machines (i.e. with Dell DMI strings?)
> 

This isn't something part of ACPI - it's been added specifically for a selection of Dell machines.  
I would rather not hardcode to the specific DMI model strings of those Dell machines as it's certainly going to be a feature that expands to more machines.
Since it is Dell specific though, if you would rather me just match to the sys vendor Dell Inc., that seems like a pretty good compromise to me.

> And finally, this seems odd overall given that a MAC address should be
> associated with the specific network device, not the overall system.

The whole reasoning behind this is to bring the behavior that E-Docks had to TBT docks.  With E-docks they were really just extensions of the pins for the already onboard NIC of the system.  When you docked in an E-dock you inherently would use the same MAC everywhere you went.  If you had two cubes your network admin would see your same MAC in both.

With TBT docks and this patch not present docking in different places will give you the MAC of the dock rather than something persistent that your network admin could identify.  Solving this was something that was explicitly asked for in case studies during conceptualization of these docks and is a feature present in the Realtek Windows driver.

> What if you have 2 types of devices plugged into the same machine, with this
> patch you suddenly have the same MAC address for both of them.
> That doesn't seem correct...
> 

I suppose it is technically possible to have two USB NIC's that use r8152 and that would be a bad behavior indeed to hand the same MAC to both of them.
In addition to limiting to Dell DMI system vendor string how about I do two more things about this:

1) Add a module parameter to disable this behavior altogether in r8152 if it's not desired by the user or admin (but leave it on by default).

2) Track whether this is the first or second USB NIC plugged in.  Only offer it on the first NIC detected by r8152.  When the second NIC is plugged in don't match from ACPI.  
There would be a question of what to do if the first NIC is removed and added back if it should get the persistent system MAC or not.  
I'd say yes, just make sure that only one NIC can have it at a time.

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

[Greg KH]

On Thu, Jun 02, 2016 at 02:10:37AM +0000, Mario_Limonciello@Dell.com wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Wednesday, June 1, 2016 6:06 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: hayeswang@realtek.com; LKML <linux-kernel@vger.kernel.org>; Netdev
> > <netdev@vger.kernel.org>; Linux USB <linux-usb@vger.kernel.org>;
> > pali.rohar@gmail.com; anthony.wong@canonical.com
> > Subject: Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary
> > MAC address
> > 
> > On Wed, Jun 01, 2016 at 04:50:44PM -0500, Mario Limonciello wrote:
> > > 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 avilable 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 <mario_limonciello@dell.com>
> > > ---
> > >  drivers/net/usb/Kconfig |  1 +
> > >  drivers/net/usb/r8152.c | 37 +++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 38 insertions(+)
> > >
> > > diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig index
> > > cdde590..c320930 100644
> > > --- a/drivers/net/usb/Kconfig
> > > +++ b/drivers/net/usb/Kconfig
> > > @@ -98,6 +98,7 @@ config USB_RTL8150
> > >  config USB_RTL8152
> > >  	tristate "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
> > >  	select MII
> > > +	depends on ACPI
> > >  	help
> > >  	  This option adds support for Realtek RTL8152 based USB 2.0
> > >  	  10/100 Ethernet adapters and RTL8153 based USB 3.0 10/100/1000
> > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > > 3f9f6ed..62af3b4 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"
> > > @@ -1030,6 +1031,39 @@ out1:
> > >  	return ret;
> > >  }
> > >
> > > +static u8 amac_ascii_to_hex(int c)
> > > +{
> > > +	if (c <= 0x39)
> > > +		return (u8)(c - 0x30);
> > > +	else if (c <= 0x46)
> > > +		return (u8)(c - 0x37);
> > > +	return (u8)(c - 0x57);
> > > +}
> > 
> > We really don't have such a function somewhere in the kernel already?
> > 
> > And why 'int', isn't "c" really a u8?
> > 
> > > +static void set_auxiliary_addr(struct sockaddr *sa) {
> > > +	acpi_status status;
> > > +	acpi_handle handle;
> > > +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > > +	union acpi_object *obj;
> > > +	int i;
> > > +	char *ptr;
> > > +
> > > +	acpi_get_handle(NULL, "\\_SB", &handle);
> > > +	status = acpi_evaluate_object(handle, "AMAC", NULL, &buffer);
> > 
> > Is this field in the ACPI standard, or should this only be "trusted" on a limited
> > number of machines (i.e. with Dell DMI strings?)
> > 
> 
> This isn't something part of ACPI - it's been added specifically for a
> selection of Dell machines.  

Ah, but isn't ACPI supposed to be a "standard"?  :)

And please wrap your email lines, there is a "standard" for that...

> I would rather not hardcode to the specific DMI model strings of those
> Dell machines as it's certainly going to be a feature that expands to
> more machines.  Since it is Dell specific though, if you would rather
> me just match to the sys vendor Dell Inc., that seems like a pretty
> good compromise to me.

You need to only do this on machines you "know" have this set to a
correct value, otherwise if some other random BIOS happens to set that
field to some random value, you will have problems.

> > And finally, this seems odd overall given that a MAC address should be
> > associated with the specific network device, not the overall system.
> 
> The whole reasoning behind this is to bring the behavior that E-Docks
> had to TBT docks.

What is "TBT"?

> With E-docks they were really just extensions of the pins for the
> already onboard NIC of the system.  When you docked in an E-dock you
> inherently would use the same MAC everywhere you went.  If you had two
> cubes your network admin would see your same MAC in both.
> 
> With TBT docks and this patch not present docking in different places
> will give you the MAC of the dock rather than something persistent
> that your network admin could identify.  Solving this was something
> that was explicitly asked for in case studies during conceptualization
> of these docks and is a feature present in the Realtek Windows driver.

But you are dealing with different "devices" here, thunderbolt is a PCI
device, not a "pin pass-through", and to treat it differently like you
want to is going to cause confusion as well.

> > What if you have 2 types of devices plugged into the same machine, with this
> > patch you suddenly have the same MAC address for both of them.
> > That doesn't seem correct...
> > 
> 
> I suppose it is technically possible to have two USB NIC's that use r8152 and that would be a bad behavior indeed to hand the same MAC to both of them.

Yes, and it will happen.

> In addition to limiting to Dell DMI system vendor string how about I do two more things about this:
> 
> 1) Add a module parameter to disable this behavior altogether in r8152 if it's not desired by the user or admin (but leave it on by default).

No module parameters, this isn't the 1990's anymore, and you aren't
going to be modifying module config files for everyone, are you?

And this seems like a "default" that should be turned off anyway, as it
goes against the model of all of our other network devices in Linux.

> 2) Track whether this is the first or second USB NIC plugged in.  Only offer it on the first NIC detected by r8152.  When the second NIC is plugged in don't match from ACPI.  
> There would be a question of what to do if the first NIC is removed and added back if it should get the persistent system MAC or not.  
> I'd say yes, just make sure that only one NIC can have it at a time.

You are going to get things very complex very quickly if you try to do
this.

What's wrong with a "simple" script to set the mac address from
userspace if the user wants something like this?  Provide it as a system
package and then no kernel changes are needed at all.  Much easier to
support on your end (you don't have to maintain this odd kernel code for
10+ years), the default behavior is as Linux users expect, and your
limited number of people who want this crazy behaviour can install your
script if they want it.

thanks,

greg k-h

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

[Mario_Limonciello-DYMqY+WieiM]

> >
> > This isn't something part of ACPI - it's been added specifically for a
> > selection of Dell machines.
> 
> Ah, but isn't ACPI supposed to be a "standard"?  :)
> 

Heh.
It's also possible to get this from an SMM routine.  Lesser of two evils to fetch the information this way, right? :)

> And please wrap your email lines, there is a "standard" for that...

I'm unfortunately not limited to an evil mail client at my workplace since our mail server migration.   My apologies, I've got it set to wrap at 76 characters and I'm trying to make it as LKML friendly as possible.

> 
> > I would rather not hardcode to the specific DMI model strings of those
> > Dell machines as it's certainly going to be a feature that expands to
> > more machines.  Since it is Dell specific though, if you would rather
> > me just match to the sys vendor Dell Inc., that seems like a pretty
> > good compromise to me.
> 
> You need to only do this on machines you "know" have this set to a correct
> value, otherwise if some other random BIOS happens to set that field to
> some random value, you will have problems.

Pali had recommended in another message to check the buffer header.  I was intending to do this along with check ACPI buffer output type, and output size in the next revision I submitted.  By switching to hex2bin, I'll also validate that the string has correct values (0-F or 0-f).  If somehow all of that fails, the set_ethernet_addr  checks if the address is valid.  If it's invalid it will generate a random one.

> 
> > > And finally, this seems odd overall given that a MAC address should
> > > be associated with the specific network device, not the overall system.
> >
> > The whole reasoning behind this is to bring the behavior that E-Docks
> > had to TBT docks.
> 
> What is "TBT"?

Thunderbolt

> 
> > With E-docks they were really just extensions of the pins for the
> > already onboard NIC of the system.  When you docked in an E-dock you
> > inherently would use the same MAC everywhere you went.  If you had two
> > cubes your network admin would see your same MAC in both.
> >
> > With TBT docks and this patch not present docking in different places
> > will give you the MAC of the dock rather than something persistent
> > that your network admin could identify.  Solving this was something
> > that was explicitly asked for in case studies during conceptualization
> > of these docks and is a feature present in the Realtek Windows driver.
> 
> But you are dealing with different "devices" here, thunderbolt is a PCI
> device, not a "pin pass-through", and to treat it differently like you want to is
> going to cause confusion as well.

Is it not also going to be confusing if someone boots Windows and Linux on the same laptop and has a different behavior with their dock's MAC address?  I'm trying to get parity here for organizations that are supporting both Windows and Linux for their employees.

> > In addition to limiting to Dell DMI system vendor string how about I do two
> more things about this:
> >
> > 1) Add a module parameter to disable this behavior altogether in r8152 if
> it's not desired by the user or admin (but leave it on by default).
> 
> No module parameters, this isn't the 1990's anymore, and you aren't going to
> be modifying module config files for everyone, are you?
> 
> And this seems like a "default" that should be turned off anyway, as it goes
> against the model of all of our other network devices in Linux.
> 
> > 2) Track whether this is the first or second USB NIC plugged in.  Only offer it
> on the first NIC detected by r8152.  When the second NIC is plugged in don't
> match from ACPI.
> > There would be a question of what to do if the first NIC is removed and
> added back if it should get the persistent system MAC or not.
> > I'd say yes, just make sure that only one NIC can have it at a time.
> 
> You are going to get things very complex very quickly if you try to do this.

It's really not that hard, track a module wide static variable whether the feature is in use.  Track in each device whether the feature was in use.  If it in use, don't assign the next device plugged in via the ACPI string.  If a device is removed that has the feature activated, change the module wide static variable.

> 
> What's wrong with a "simple" script to set the mac address from userspace if
> the user wants something like this?  Provide it as a system package and then
> no kernel changes are needed at all.  Much easier to support on your end
> (you don't have to maintain this odd kernel code for
> 10+ years), the default behavior is as Linux users expect, and your
> limited number of people who want this crazy behaviour can install your
> script if they want it.
> 

This was my original approach.  It involved a network manager script, network manager code changes to support this, and exposing this somewhere in a platform module (like dell-laptop).  I was told I'm better off doing it directly in the network module, so here I am.
--
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

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

[Greg KH]

On Thu, Jun 02, 2016 at 03:46:41PM +0000, Mario_Limonciello@Dell.com wrote:
> > >
> > > This isn't something part of ACPI - it's been added specifically for a
> > > selection of Dell machines.
> > 
> > Ah, but isn't ACPI supposed to be a "standard"?  :)
> > 
> 
> Heh.
> It's also possible to get this from an SMM routine.  Lesser of two evils to fetch the information this way, right? :)

Yes, but again, please only do this for machines you _know_ this value
will be present on.  Otherwise you will end up with problems.

> > And please wrap your email lines, there is a "standard" for that...
> 
> I'm unfortunately not limited to an evil mail client at my workplace since our mail server migration.   My apologies, I've got it set to wrap at 76 characters and I'm trying to make it as LKML friendly as possible.

It's not working as you can see here :(

> > > I would rather not hardcode to the specific DMI model strings of those
> > > Dell machines as it's certainly going to be a feature that expands to
> > > more machines.  Since it is Dell specific though, if you would rather
> > > me just match to the sys vendor Dell Inc., that seems like a pretty
> > > good compromise to me.
> > 
> > You need to only do this on machines you "know" have this set to a correct
> > value, otherwise if some other random BIOS happens to set that field to
> > some random value, you will have problems.
> 
> Pali had recommended in another message to check the buffer header.  I was intending to do this along with check ACPI buffer output type, and output size in the next revision I submitted.  By switching to hex2bin, I'll also validate that the string has correct values (0-F or 0-f).  If somehow all of that fails, the set_ethernet_addr  checks if the address is valid.  If it's invalid it will generate a random one.

Why generate a random one and not just use the one that the network
controler already provides?

And yes, you better error check the heck out of this before you set it.

> > > With E-docks they were really just extensions of the pins for the
> > > already onboard NIC of the system.  When you docked in an E-dock you
> > > inherently would use the same MAC everywhere you went.  If you had two
> > > cubes your network admin would see your same MAC in both.
> > >
> > > With TBT docks and this patch not present docking in different places
> > > will give you the MAC of the dock rather than something persistent
> > > that your network admin could identify.  Solving this was something
> > > that was explicitly asked for in case studies during conceptualization
> > > of these docks and is a feature present in the Realtek Windows driver.
> > 
> > But you are dealing with different "devices" here, thunderbolt is a PCI
> > device, not a "pin pass-through", and to treat it differently like you want to is
> > going to cause confusion as well.
> 
> Is it not also going to be confusing if someone boots Windows and Linux on the same laptop and has a different behavior with their dock's MAC address?  I'm trying to get parity here for organizations that are supporting both Windows and Linux for their employees.

Those few orginizations can use a userspace script for this :)

> > > In addition to limiting to Dell DMI system vendor string how about I do two
> > more things about this:
> > >
> > > 1) Add a module parameter to disable this behavior altogether in r8152 if
> > it's not desired by the user or admin (but leave it on by default).
> > 
> > No module parameters, this isn't the 1990's anymore, and you aren't going to
> > be modifying module config files for everyone, are you?
> > 
> > And this seems like a "default" that should be turned off anyway, as it goes
> > against the model of all of our other network devices in Linux.
> > 
> > > 2) Track whether this is the first or second USB NIC plugged in.  Only offer it
> > on the first NIC detected by r8152.  When the second NIC is plugged in don't
> > match from ACPI.
> > > There would be a question of what to do if the first NIC is removed and
> > added back if it should get the persistent system MAC or not.
> > > I'd say yes, just make sure that only one NIC can have it at a time.
> > 
> > You are going to get things very complex very quickly if you try to do this.
> 
> It's really not that hard, track a module wide static variable whether the feature is in use.  Track in each device whether the feature was in use.  If it in use, don't assign the next device plugged in via the ACPI string.  If a device is removed that has the feature activated, change the module wide static variable.

Ok, let's see the code before I say anymore about this.

> > What's wrong with a "simple" script to set the mac address from userspace if
> > the user wants something like this?  Provide it as a system package and then
> > no kernel changes are needed at all.  Much easier to support on your end
> > (you don't have to maintain this odd kernel code for
> > 10+ years), the default behavior is as Linux users expect, and your
> > limited number of people who want this crazy behaviour can install your
> > script if they want it.
> > 
> 
> This was my original approach.  It involved a network manager script, network manager code changes to support this, and exposing this somewhere in a platform module (like dell-laptop).  I was told I'm better off doing it directly in the network module, so here I am.

Why not a small systemd unit file for this that sets things up when the
device is found in the system?  Why mess with network manager and a
platform kernel driver at all?  That seems very complex for such a
simple operation where the kernel doesn't need to be involved at all,
especially for such a "niche" product.

See this link:
	https://wiki.archlinux.org/index.php/MAC_address_spoofing#Automatically

for an example of how to do this in a simple manner.

thanks,

greg k-h

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

[Mario_Limonciello]

Some of my comments are getting stale with what I've done in response to all these emails.
Let me send a v2 that we can better iterate on, a few comments below though.

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, June 2, 2016 11:09 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> 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] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> On Thu, Jun 02, 2016 at 03:46:41PM +0000, Mario_Limonciello@Dell.com
> wrote:
> > > >
> > > > This isn't something part of ACPI - it's been added specifically for a
> > > > selection of Dell machines.
> > >
> > > Ah, but isn't ACPI supposed to be a "standard"?  :)
> > >
> >
> > Heh.
> > It's also possible to get this from an SMM routine.  Lesser of two evils to
> fetch the information this way, right? :)
> 
> Yes, but again, please only do this for machines you _know_ this value
> will be present on.  Otherwise you will end up with problems.

I'm going to send a V2, I'd like to know where and how this could still break. 
I am having a hard time grasping this.  

> 
> > > And please wrap your email lines, there is a "standard" for that...
> >
> > I'm unfortunately not limited to an evil mail client at my workplace since our
> mail server migration.   My apologies, I've got it set to wrap at 76 characters
> and I'm trying to make it as LKML friendly as possible.
> 
> It's not working as you can see here :(

Ugh, sorry.  Stupid outlook.  It seems to only be doing it on replies.
I'll manually just chop the lines when they're around that size until I've got a
better solution.

> 
> > > > I would rather not hardcode to the specific DMI model strings of those
> > > > Dell machines as it's certainly going to be a feature that expands to
> > > > more machines.  Since it is Dell specific though, if you would rather
> > > > me just match to the sys vendor Dell Inc., that seems like a pretty
> > > > good compromise to me.
> > >
> > > You need to only do this on machines you "know" have this set to a
> correct
> > > value, otherwise if some other random BIOS happens to set that field to
> > > some random value, you will have problems.
> >
> > Pali had recommended in another message to check the buffer header.  I
> was intending to do this along with check ACPI buffer output type, and
> output size in the next revision I submitted.  By switching to hex2bin, I'll also
> validate that the string has correct values (0-F or 0-f).  If somehow all of that
> fails, the set_ethernet_addr  checks if the address is valid.  If it's invalid it will
> generate a random one.
> 
> Why generate a random one and not just use the one that the network
> controler already provides?

That's how the flow works in r8152 already and I'm not overriding it.
Again, I'll send V2 and you'll see what I did.

> >
> > It's really not that hard, track a module wide static variable whether the
> feature is in use.  Track in each device whether the feature was in use.  If it in
> use, don't assign the next device plugged in via the ACPI string.  If a device is
> removed that has the feature activated, change the module wide static
> variable.
> 
> Ok, let's see the code before I say anymore about this.
> 
> > > What's wrong with a "simple" script to set the mac address from
> userspace if
> > > the user wants something like this?  Provide it as a system package and
> then
> > > no kernel changes are needed at all.  Much easier to support on your end
> > > (you don't have to maintain this odd kernel code for
> > > 10+ years), the default behavior is as Linux users expect, and your
> > > limited number of people who want this crazy behaviour can install your
> > > script if they want it.
> > >
> >
> > This was my original approach.  It involved a network manager script,
> network manager code changes to support this, and exposing this
> somewhere in a platform module (like dell-laptop).  I was told I'm better off
> doing it directly in the network module, so here I am.
> 
> Why not a small systemd unit file for this that sets things up when the
> device is found in the system?  Why mess with network manager and a
> platform kernel driver at all?  That seems very complex for such a
> simple operation where the kernel doesn't need to be involved at all,
> especially for such a "niche" product.
> 
> See this link:
> 	https://wiki.archlinux.org/index.php/MAC_address_spoofing#Auto
> matically
> 

The ACPI subsystem doesn't create a sysfs node for a random buffer under _SB.
I don't think the ACPI guys would be crazy about this either.

So you need a platform kernel driver to pull this out of ACPI (or SMM) and expose
into userspace somewhere in the first place.  I was putting it into a random sysfs
attribute when I did my first attempts at a userspace approach.

So I saw that wiki too, and tried that approach.  Network manager stomps all over
anything that systemd does in unit files and resets to the MAC address it declares 
was the HW MAC address from sysfs.

The other problem I encountered doing this in userspace was that because of the
renaming magic that exists now, you can't have a userspace script key off the device
as you move to to different places.  Dock in location A will rename eth0 to 
ethaabbccddeeff and dock in location B will rename to ethffeeddccbbaa.  

So my next approach was use a system unit file to rename the device to 
"delldock%x" and then use network manager to key off that.

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

[Hayes Wang]

Mario_Limonciello@Dell.com [mailto:Mario_Limonciello@Dell.com]
> Sent: Friday, June 03, 2016 12:58 AM
[...]
> > Why generate a random one and not just use the one that the network
> > controler already provides?
> 
> That's how the flow works in r8152 already and I'm not overriding it.
> Again, I'll send V2 and you'll see what I did.

The original flows are
1. Read MAC address 1 from device.
2. Check if the MAC address 1 is valid.
3. Use random MAC address if MAC address 1 is invalid.

However, your flow would be
1. Read MAC address 1 from device.
2. Read MAC address 2 from ACPI.
3. Check if the MAC address 2 is valid.
4. Use random MAC address if MAC address 2 is invalid.

Although MAC address 2 may be invalid, MAC address 1 may be valid.
For this situation, you have to use MAC address 1, not random one.

Best Regards,
Hayes

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

[Mario_Limonciello-DYMqY+WieiM]

> -----Original Message-----
> From: Hayes Wang [mailto:hayeswang-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org]
> Sent: Friday, June 3, 2016 4:23 AM
> To: Limonciello, Mario <Mario_Limonciello-DYMqY+WieiM@public.gmane.org>;
> gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; anthony.wong-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org
> Subject: RE: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> Mario_Limonciello-DYMqY+WieiM@public.gmane.org [mailto:Mario_Limonciello-DYMqY+WieiM@public.gmane.org]
> > Sent: Friday, June 03, 2016 12:58 AM
> [...]
> > > Why generate a random one and not just use the one that the network
> > > controler already provides?
> >
> > That's how the flow works in r8152 already and I'm not overriding it.
> > Again, I'll send V2 and you'll see what I did.
> 
> The original flows are
> 1. Read MAC address 1 from device.
> 2. Check if the MAC address 1 is valid.
> 3. Use random MAC address if MAC address 1 is invalid.
> 
> However, your flow would be
> 1. Read MAC address 1 from device.
> 2. Read MAC address 2 from ACPI.
> 3. Check if the MAC address 2 is valid.
> 4. Use random MAC address if MAC address 2 is invalid.
> 
> Although MAC address 2 may be invalid, MAC address 1 may be valid.
> For this situation, you have to use MAC address 1, not random one.
> 
> Best Regards,
> Hayes

Hi Hayes,

Once I get information about how to read docking bit, I'll adjust that for v3
to ensure that it falls back to MAC address 1 if #2 is invalid.

Thanks
--
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

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

[Bjørn Mork]

<Mario_Limonciello@Dell.com> writes:

>> > 2) Track whether this is the first or second USB NIC plugged in.  Only offer it
>> on the first NIC detected by r8152.  When the second NIC is plugged in don't
>> match from ACPI.
>> > There would be a question of what to do if the first NIC is removed and
>> added back if it should get the persistent system MAC or not.
>> > I'd say yes, just make sure that only one NIC can have it at a time.
>> 
>> You are going to get things very complex very quickly if you try to do this.
>
> It's really not that hard, track a module wide static variable whether
> the feature is in use.  Track in each device whether the feature was
> in use.  If it in use, don't assign the next device plugged in via the
> ACPI string.  If a device is removed that has the feature activated,
> change the module wide static variable.

Having the mac address jump around in an arbitrary way like this is
going to confuse the hell out of your users.  Consider what happens if
the user docks a laptop with an r8152 usb dongle already plugged in...
How are you going to explain that the dock gets some other mac address
in this case? How are you going to explain the difference between using
an r8152 based dongle and some other ethernet usb dongle with your
systems?

Make it behave consistently if you're going to add this.  Which can be
done by specifically matching the Dell dock (doesn't it have an unique
Dell device ID?) and ignoring any other r8152 device.  You could also
choose to set the same mac for all r8152 devices.  Which is fine, but
will probably confuse many users.

What you definitely should not do is to change the mac for some
arbitrary "first" device.  Then you are better off with the userspace
proposal where you and your users have some chance to implement a
sensible policy based on e.g. usb port numbers.



Bjørn

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

[Mario_Limonciello]

> -----Original Message-----
> From: Bjørn Mork [mailto:bjorn@mork.no]
> Sent: Thursday, June 2, 2016 1:04 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> 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] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> <Mario_Limonciello@Dell.com> writes:
> 
> >> > 2) Track whether this is the first or second USB NIC plugged in.  Only
> offer it
> >> on the first NIC detected by r8152.  When the second NIC is plugged in
> don't
> >> match from ACPI.
> >> > There would be a question of what to do if the first NIC is removed and
> >> added back if it should get the persistent system MAC or not.
> >> > I'd say yes, just make sure that only one NIC can have it at a time.
> >>
> >> You are going to get things very complex very quickly if you try to do this.
> >
> > It's really not that hard, track a module wide static variable whether
> > the feature is in use.  Track in each device whether the feature was
> > in use.  If it in use, don't assign the next device plugged in via the
> > ACPI string.  If a device is removed that has the feature activated,
> > change the module wide static variable.
> 
> Having the mac address jump around in an arbitrary way like this is
> going to confuse the hell out of your users.  Consider what happens if
> the user docks a laptop with an r8152 usb dongle already plugged in...
> How are you going to explain that the dock gets some other mac address
> in this case? How are you going to explain the difference between using
> an r8152 based dongle and some other ethernet usb dongle with your
> systems?

Yeah I understand the concern.  I agree that would be very confusing
to a user.  This does need to match only on Dell docks then.

> 
> Make it behave consistently if you're going to add this.  Which can be
> done by specifically matching the Dell dock (doesn't it have an unique
> Dell device ID?) and ignoring any other r8152 device.  You could also
> choose to set the same mac for all r8152 devices.  Which is fine, but
> will probably confuse many users.

Unfortunately there is no Dell specific VID/PID.  I checked a no-name dongle
that used r8152 and it was the same (0bda:8153).  Maybe Hayes Wang can 
check with his Windows driver colleagues if there was anything else to key
off when this was implemented on the Windows Realtek driver.  If there 
is something else to key off of, I'm not aware what it is.  I'll check with 
some of my colleagues too.

I do have a way to query if a dock is plugged in via SMM, but I doubt that's
what Realtek is using on the Windows side.  I'd leave that as a second to
last resort (last resort being move back to userspace again).

> 
> What you definitely should not do is to change the mac for some
> arbitrary "first" device.  Then you are better off with the userspace
> proposal where you and your users have some chance to implement a
> sensible policy based on e.g. usb port numbers.

OK, if I can't come up with a way to key on the device being a Dell dock 
I'll scrap this entirely kernel approach.

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 5249 bytes --]

On Thursday 02 June 2016 20:28:33 Mario_Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Bjørn Mork [mailto:bjorn@mork.no]
> > Sent: Thursday, June 2, 2016 1:04 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > 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] r8152: Add support
> > for setting MAC to system's Auxiliary MAC address
> > 
> > <Mario_Limonciello@Dell.com> writes:
> > >> > 2) Track whether this is the first or second USB NIC plugged
> > >> > in.  Only
> > 
> > offer it
> > 
> > >> on the first NIC detected by r8152.  When the second NIC is
> > >> plugged in
> > 
> > don't
> > 
> > >> match from ACPI.
> > >> 
> > >> > There would be a question of what to do if the first NIC is
> > >> > removed and
> > >> 
> > >> added back if it should get the persistent system MAC or not.
> > >> 
> > >> > I'd say yes, just make sure that only one NIC can have it at a
> > >> > time.
> > >> 
> > >> You are going to get things very complex very quickly if you try
> > >> to do this.
> > > 
> > > It's really not that hard, track a module wide static variable
> > > whether the feature is in use.  Track in each device whether the
> > > feature was in use.  If it in use, don't assign the next device
> > > plugged in via the ACPI string.  If a device is removed that has
> > > the feature activated, change the module wide static variable.
> > 
> > Having the mac address jump around in an arbitrary way like this is
> > going to confuse the hell out of your users.  Consider what happens
> > if the user docks a laptop with an r8152 usb dongle already
> > plugged in... How are you going to explain that the dock gets some
> > other mac address in this case? How are you going to explain the
> > difference between using an r8152 based dongle and some other
> > ethernet usb dongle with your systems?
> 
> Yeah I understand the concern.  I agree that would be very confusing
> to a user.  This does need to match only on Dell docks then.
> 
> > Make it behave consistently if you're going to add this.  Which can
> > be done by specifically matching the Dell dock (doesn't it have an
> > unique Dell device ID?) and ignoring any other r8152 device.  You
> > could also choose to set the same mac for all r8152 devices. 
> > Which is fine, but will probably confuse many users.
> 
> Unfortunately there is no Dell specific VID/PID.  I checked a no-name
> dongle that used r8152 and it was the same (0bda:8153).  Maybe Hayes
> Wang can check with his Windows driver colleagues if there was
> anything else to key off when this was implemented on the Windows
> Realtek driver.  If there is something else to key off of, I'm not
> aware what it is.  I'll check with some of my colleagues too.

I have some other questions which answers should we know:

1) Is that AUX MAC address implemented only in customized windows Dell 
driver? Or also in "upstream" windows Realtek driver and all users of 
Realtek hw can install it (or update via next driver update)?

2) Can you share pseudo code or description of algorithm which decide 
MAC address for newly connected r8152 device on windows? This could help 
us to decide if something similar/same cannot be implemented also on 
linux (either in kernel or userspace). What I would like to know are 
those situations when you connect more r8152 devices (some Dell and some 
non-Dell).

> I do have a way to query if a dock is plugged in via SMM, but I doubt
> that's what Realtek is using on the Windows side.

So there is some way to check if Dell dock is plugged, right? But what 
happen if you connect Dell dock and also non-Dell r8152 device? Can you 
distinguish which device is Dell and which non-Dell?

Anyway, I think that by SMM you mean dell smbios API call. Cannot you 
guys in Dell release documentation of all smbios calls to community? 
Time to time you release some small parts in libsmbios project which 
then we can use for implementing useful parts in kernel (e.g. LED driver 
for controlling keyboard backlight). But there are couple of 
undocumented APIs and maybe some can also help with this problem... 

> I'd leave that as
> a second to last resort (last resort being move back to userspace
> again).
> 
> > What you definitely should not do is to change the mac for some
> > arbitrary "first" device.  Then you are better off with the
> > userspace proposal where you and your users have some chance to
> > implement a sensible policy based on e.g. usb port numbers.
> 
> OK, if I can't come up with a way to key on the device being a Dell
> dock I'll scrap this entirely kernel approach.

E.g. PCI devices have ordinary PCI device & vendor IDs, but have Dell 
specific subsystem IDs. And via subsystem IDs we can distinguish between 
Intel graphics card on Dell laptop and on non-Dell laptop.

Does not you have some special/modified firmware in those Dell realtek 
docks (and ability to check from OS some registers)?

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Mario_Limonciello]

> I have some other questions which answers should we know:
> 
> 1) Is that AUX MAC address implemented only in customized windows Dell
> driver? Or also in "upstream" windows Realtek driver and all users of
> Realtek hw can install it (or update via next driver update)?
> 

I don't have the information on this.  Realtek will have to comment here as this
part is a black box to me.  I'm asking my internal colleagues about this too.

> 2) Can you share pseudo code or description of algorithm which decide
> MAC address for newly connected r8152 device on windows? This could help
> us to decide if something similar/same cannot be implemented also on
> linux (either in kernel or userspace). What I would like to know are
> those situations when you connect more r8152 devices (some Dell and some
> non-Dell).
> 

This is another thing I don't have the information for right now.
I can install Windows on a laptop, install the Realtek driver and experiment,
but it would be better to get this directly from Realtek if at all possible.

> > I do have a way to query if a dock is plugged in via SMM, but I doubt
> > that's what Realtek is using on the Windows side.
> 
> So there is some way to check if Dell dock is plugged, right? But what
> happen if you connect Dell dock and also non-Dell r8152 device? Can you
> distinguish which device is Dell and which non-Dell?

Yes, when querying if a Dell dock is plugged in, a "location" and "count" 
parameter is returned.  I'd have to figure out how to translate that into
what the Linux kernel sees.  Actually the information for how to do this 
is already public too.  It's in a pull request for Dock FW updating in the 
fwupd project.

https://github.com/hughsie/fwupd/pull/49/files#diff-81b55c87ce1542a18b0a4b2b228b9129R189

> 
> Anyway, I think that by SMM you mean dell smbios API call. Cannot you
> guys in Dell release documentation of all smbios calls to community?

Well dell SMBIOS API call really means to use dcdbas kernel module which
does SMM..

> Time to time you release some small parts in libsmbios project which
> then we can use for implementing useful parts in kernel (e.g. LED driver
> for controlling keyboard backlight). But there are couple of
> undocumented APIs and maybe some can also help with this problem...
> 

Releasing different bits of our SMBIOS document requires approvals.
We can't just release the whole thing as there are lots of interfaces that
aren't intended for the OS to be using.  They're used only by Dell tools.

For example we just had approval for information about querying TPM
and dock information and those are present in the fwupd pull request
for dock and TPM FW updates you see above.

If you have some API's in particular you would like more information on,
I'm happy to have internal discussion to see if we can release information
on those.

> > I'd leave that as
> > a second to last resort (last resort being move back to userspace
> > again).
> >
> > > What you definitely should not do is to change the mac for some
> > > arbitrary "first" device.  Then you are better off with the
> > > userspace proposal where you and your users have some chance to
> > > implement a sensible policy based on e.g. usb port numbers.
> >
> > OK, if I can't come up with a way to key on the device being a Dell
> > dock I'll scrap this entirely kernel approach.
> 
> E.g. PCI devices have ordinary PCI device & vendor IDs, but have Dell
> specific subsystem IDs. And via subsystem IDs we can distinguish between
> Intel graphics card on Dell laptop and on non-Dell laptop.
> 
> Does not you have some special/modified firmware in those Dell realtek
> docks (and ability to check from OS some registers)?

I think so.  Otherwise there would be all the same concerns you have outlined
with generic devices.  Like I said this part is currently a black box to me.  
I hope Realtek can publicly comment on this, or I can get some information 
from my colleagues.

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2882 bytes --]

On Thursday 02 June 2016 20:04:02 Bjørn Mork wrote:
> <Mario_Limonciello@Dell.com> writes:
> >> > 2) Track whether this is the first or second USB NIC plugged in.
> >> >  Only offer it
> >> 
> >> on the first NIC detected by r8152.  When the second NIC is
> >> plugged in don't match from ACPI.
> >> 
> >> > There would be a question of what to do if the first NIC is
> >> > removed and
> >> 
> >> added back if it should get the persistent system MAC or not.
> >> 
> >> > I'd say yes, just make sure that only one NIC can have it at a
> >> > time.
> >> 
> >> You are going to get things very complex very quickly if you try
> >> to do this.
> > 
> > It's really not that hard, track a module wide static variable
> > whether the feature is in use.  Track in each device whether the
> > feature was in use.  If it in use, don't assign the next device
> > plugged in via the ACPI string.  If a device is removed that has
> > the feature activated, change the module wide static variable.
> 
> Having the mac address jump around in an arbitrary way like this is
> going to confuse the hell out of your users.  Consider what happens
> if the user docks a laptop with an r8152 usb dongle already plugged
> in... How are you going to explain that the dock gets some other mac
> address in this case? How are you going to explain the difference
> between using an r8152 based dongle and some other ethernet usb
> dongle with your systems?
> 
> Make it behave consistently if you're going to add this.  Which can
> be done by specifically matching the Dell dock (doesn't it have an
> unique Dell device ID?) and ignoring any other r8152 device.  You
> could also choose to set the same mac for all r8152 devices.  Which
> is fine, but will probably confuse many users.
> 
> What you definitely should not do is to change the mac for some
> arbitrary "first" device.  Then you are better off with the userspace
> proposal where you and your users have some chance to implement a
> sensible policy based on e.g. usb port numbers.

This is exactly what I wanted to write, but you were faster :-)

You can connect more Dell docks (with r8152 devices) and more non-Dell 
r8152 devices in random order into Dell laptop. In any case dependent on 
connect and disconnect order, devices always must have exactly same MAC 
addresses. Otherwise there will be problems! It confuse users and also 
admins of networks...

So if kernel approach is chosen then I think there are only two solution 
those satisfy above conditions:

First one is:
* all non-Dell devices have own MAC address
* all Dell devices have (one, same) AUX MAC address

Second one is:
* all devices (Dell and also non-Dell) have own address
* AUX MAC address is never used

So what do you (netdev maintainers) think about it?

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Mario_Limonciello]

> > +static u8 amac_ascii_to_hex(int c)
> > +{
> > +	if (c <= 0x39)
> > +		return (u8)(c - 0x30);
> > +	else if (c <= 0x46)
> > +		return (u8)(c - 0x37);
> > +	return (u8)(c - 0x57);
> > +}
> 

Sorry forgot to address this.  

> We really don't have such a function somewhere in the kernel already?

There is a function in acpi/acpica/uthex.c that does this, but it doesn't seem to be used by anything outside of acpica so far.  Would it be OK style wise to 
#include " ../../acpi/acpica/acutils.h" from r8152.c?  

If not, then what is the proper thing to do here to re-use it from there?

> And why 'int', isn't "c" really a u8?

Yeah I guess u8 should be fine there, and avoid the casting then too.

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

[Bjørn Mork]

<Mario_Limonciello@Dell.com> writes:

>> > +static u8 amac_ascii_to_hex(int c)
>> > +{
>> > +	if (c <= 0x39)
>> > +		return (u8)(c - 0x30);
>> > +	else if (c <= 0x46)
>> > +		return (u8)(c - 0x37);
>> > +	return (u8)(c - 0x57);
>> > +}
>> 
>
> Sorry forgot to address this.  
>
>> We really don't have such a function somewhere in the kernel already?
>
> There is a function in acpi/acpica/uthex.c that does this, but it doesn't seem to be used by anything outside of acpica so far.  Would it be OK style wise to 
> #include " ../../acpi/acpica/acutils.h" from r8152.c?  

Makes me wonder where you looked....  You have hex_to_bin() and
hex2bin() in include/linux/kernel.h

You could look at usbnet_get_ethernet_addr() for an example of how to do
this properly.  It's pretty close to this driver in the tree, and should
be a natural starting point before reinventing the wheel...



Bjørn

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

[Mario_Limonciello]

> -----Original Message-----
> From: Bjørn Mork [mailto:bjorn@mork.no]
> Sent: Thursday, June 2, 2016 3:11 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> 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] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> <Mario_Limonciello@Dell.com> writes:
> 
> >> > +static u8 amac_ascii_to_hex(int c) {
> >> > +	if (c <= 0x39)
> >> > +		return (u8)(c - 0x30);
> >> > +	else if (c <= 0x46)
> >> > +		return (u8)(c - 0x37);
> >> > +	return (u8)(c - 0x57);
> >> > +}
> >>
> >
> > Sorry forgot to address this.
> >
> >> We really don't have such a function somewhere in the kernel already?
> >
> > There is a function in acpi/acpica/uthex.c that does this, but it
> > doesn't seem to be used by anything outside of acpica so far.  Would it be
> OK style wise to #include " ../../acpi/acpica/acutils.h" from r8152.c?
> 
> Makes me wonder where you looked....  You have hex_to_bin() and
> hex2bin() in include/linux/kernel.h
> 


Thank you, I completely missed that.  I wasn't looking for literals in my grepping.  I'll use this instead.

> You could look at usbnet_get_ethernet_addr() for an example of how to do
> this properly.  It's pretty close to this driver in the tree, and should be a
> natural starting point before reinventing the wheel...
> 

OK will do.

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

[Bjørn Mork]

Greg KH <gregkh@linuxfoundation.org> writes:

> And finally, this seems odd overall given that a MAC address should be
> associated with the specific network device, not the overall system.

Definitely.

I wonder if this isn't a perfect candidate for an x86
arch_get_platform_mac_address() implementation?  Then you could just use
the eth_platform_get_mac_address() helper in the driver and avoid any
platform specific code there.  Which will automagically make it work as
expected on a Sparc too :)

This will also make your job next year much easier, when the hardeware
guys decided to replace the chip and you need to implement the exact
same code in some other driver...

See https://patchwork.ozlabs.org/patch/564100/ for a detailed
discussion of this interface.


Bjørn

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

[Hayes Wang]

Mario Limonciello [mailto:mario_limonciello@dell.com]
[...]
>  static int set_ethernet_addr(struct r8152 *tp)
>  {
>  	struct net_device *dev = tp->netdev;
> @@ -1041,6 +1075,9 @@ 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 */
> +	set_auxiliary_addr(&sa);
> +
>  	if (ret < 0) {
>  		netif_err(tp, probe, dev, "Get ether addr fail\n");
>  	} else if (!is_valid_ether_addr(sa.sa_data)) {

When tp->version == RTL_VER_01, you would have different MAC address
between SW and HW. You may use the MAC address from ACPI for dev_addr.
However, the device uses another one, because you don't set it to the device.

Best Regards,
Hayes

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

[Pali Rohár]

Hi! As ACPI bytecode is untrusted for me and also for running kernel, we
should not expect that it does not contain any bugs or other problems.
So I would propose these checks to prevent something wrong...

On Wednesday 01 June 2016 16:50:44 Mario Limonciello wrote:
> +static void set_auxiliary_addr(struct sockaddr *sa)
> +{
> +	acpi_status status;
> +	acpi_handle handle;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	int i;
> +	char *ptr;
> +
> +	acpi_get_handle(NULL, "\\_SB", &handle);

Check return value of acpi_get_handle

> +	status = acpi_evaluate_object(handle, "AMAC", NULL, &buffer);

This is question for ACPI devs, it is not possible to call directly?

  acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);

And what happen if we try to evaluate objects which do not exist? Does
not it show some warning or error in dmesg about non existent object?
Such errors should be silent here.

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

Check buffer.type

> +	if (ACPI_SUCCESS(status) && (obj->string.length == 0x17)) {
> +		/* returns _AUXMAC_#AABBCCDDEEFF#
> +		 * this pulls out _AUXMAC# from start and # from end
> +		 */
> +		ptr = obj->string.pointer + 9;

Verify that string really contains that _AUXMAX# prefix. This is really
obscure and nonstandard format for specifying MAC address and in my
opinion it should be properly checked. Nonstandard formats can be
changed in future and we could have problems.

> +		pr_info("r8152: Using system auxiliary MAC address");

It would be great to write also mac address into that pr_info

> +		for (i = 0; i < 6; i++, ptr += 2)
> +			sa->sa_data[i] = amac_ascii_to_hex(*ptr) << 4 |
> +					 amac_ascii_to_hex(*(ptr + 1));
> +	}

In case of some acpi check fails throw warning (or error).

And there is memory leak, you allocated buffer with ACPI_ALLOCATE_BUFFER
but you did not free it.

> +}

And my last question is: Are really all Dell docks comes with this one
realtek chip? I'm pessimist in this, because I see how other components
(like HDD vendor, touchpad type, smardcard chips, motherboards, display
panels, wifi chips) can be different in two laptops of same Dell model.

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

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

[Mario_Limonciello]

Thanks for the check.  Some small comments below, and I'll address in next submission.

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Thursday, June 2, 2016 2:47 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: hayeswang@realtek.com; LKML <linux-kernel@vger.kernel.org>; Netdev
> <netdev@vger.kernel.org>; Linux USB <linux-usb@vger.kernel.org>;
> anthony.wong@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> Hi! As ACPI bytecode is untrusted for me and also for running kernel, we
> should not expect that it does not contain any bugs or other problems.
> So I would propose these checks to prevent something wrong...

OK

> 
> On Wednesday 01 June 2016 16:50:44 Mario Limonciello wrote:
> > +static void set_auxiliary_addr(struct sockaddr *sa) {
> > +	acpi_status status;
> > +	acpi_handle handle;
> > +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +	union acpi_object *obj;
> > +	int i;
> > +	char *ptr;
> > +
> > +	acpi_get_handle(NULL, "\\_SB", &handle);
> 
> Check return value of acpi_get_handle
> 
> > +	status = acpi_evaluate_object(handle, "AMAC", NULL, &buffer);
> 
> This is question for ACPI devs, it is not possible to call directly?
> 
>   acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);

I tried this and it works for me.  I'll take out the stuff related to making a handle in the next version.

> 
> And what happen if we try to evaluate objects which do not exist? Does not
> it show some warning or error in dmesg about non existent object?
> Such errors should be silent here.
> 

I tried this with an object I knew didn't exist (like \\_SB.AMACx) and there was no warning or error about non existent objects.

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

OK.

> 
> > +	if (ACPI_SUCCESS(status) && (obj->string.length == 0x17)) {
> > +		/* returns _AUXMAC_#AABBCCDDEEFF#
> > +		 * this pulls out _AUXMAC# from start and # from end
> > +		 */
> > +		ptr = obj->string.pointer + 9;
> 
> Verify that string really contains that _AUXMAX# prefix. This is really obscure
> and nonstandard format for specifying MAC address and in my opinion it
> should be properly checked. Nonstandard formats can be changed in future
> and we could have problems.

OK.

> 
> > +		pr_info("r8152: Using system auxiliary MAC address");
> 
> It would be great to write also mac address into that pr_info

I was originally doing this before I submitted the patch for debugging but didn't think there was value since it could easily be looked up.  I'll add that back in.

> 
> > +		for (i = 0; i < 6; i++, ptr += 2)
> > +			sa->sa_data[i] = amac_ascii_to_hex(*ptr) << 4 |
> > +					 amac_ascii_to_hex(*(ptr + 1));
> > +	}
> 
> In case of some acpi check fails throw warning (or error).

OK.

> 
> And there is memory leak, you allocated buffer with
> ACPI_ALLOCATE_BUFFER but you did not free it.

Alright I'll make sure to cleanup properly.

> 
> > +}
> 
> And my last question is: Are really all Dell docks comes with this one realtek
> chip? I'm pessimist in this, because I see how other components (like HDD
> vendor, touchpad type, smardcard chips, motherboards, display panels, wifi
> chips) can be different in two laptops of same Dell model.

Indeed some of those things you mention in laptops are multiple sources or multiple options.  I can tell you that all the docks that are enabled with this technology (TB15, WD15, and type-C LAN dongle PN# 96NP5) are only using the r8152 driver.  There is always opportunity to change this in a future generation of docks or dongles.  Another reply had talked about moving this to a platform lookup and then using that platform lookup in r8152.  I think that's a good approach that future proofs choosing another vendor for future docks to prevent extra code duplication.  Of course this can also be bolted onto any other USB Ethernet dongle.  If there was desire to do this with other dongles it could be easily added.

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

[Andrew Lunn]

> > 
> > > +		pr_info("r8152: Using system auxiliary MAC address");
> > 
> > It would be great to write also mac address into that pr_info

And since there could be multiple r8152 in the system, it would be
good to indicate which of them is having its MAC changed. So
netdev_info() or dev_info().

      Andrew

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

[Mario_Limonciello]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Thursday, June 2, 2016 10:01 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: pali.rohar@gmail.com; hayeswang@realtek.com; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; linux-
> usb@vger.kernel.org; anthony.wong@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> > >
> > > > +		pr_info("r8152: Using system auxiliary MAC address");
> > >
> > > It would be great to write also mac address into that pr_info
> 
> And since there could be multiple r8152 in the system, it would be good to
> indicate which of them is having its MAC changed. So
> netdev_info() or dev_info().
> 
>       Andrew

Thanks, will do that in next submission too.