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

Greg KH
2016-06-01T23:05:35Z

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