* [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD @ 2016-06-06 14:15 Mario Limonciello 2016-06-06 14:15 ` Mario Limonciello 2016-06-06 14:40 ` Greg KH 0 siblings, 2 replies; 13+ messages in thread From: Mario Limonciello @ 2016-06-06 14:15 UTC (permalink / raw) To: hayeswang Cc: LKML, Netdev, Linux USB, pali.rohar, anthony.wong, Greg KH, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:15 [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD Mario Limonciello @ 2016-06-06 14:15 ` Mario Limonciello 2016-06-06 14:39 ` Greg KH 2016-06-06 14:40 ` Andrew Lunn 2016-06-06 14:40 ` Greg KH 1 sibling, 2 replies; 13+ messages in thread From: Mario Limonciello @ 2016-06-06 14:15 UTC (permalink / raw) To: hayeswang Cc: LKML, Netdev, Linux USB, pali.rohar, anthony.wong, Greg KH, 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_limonciello@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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:15 ` Mario Limonciello @ 2016-06-06 14:39 ` Greg KH 2016-06-06 14:56 ` Mario_Limonciello 2016-06-06 14:40 ` Andrew Lunn 1 sibling, 1 reply; 13+ messages in thread From: Greg KH @ 2016-06-06 14:39 UTC (permalink / raw) To: Mario Limonciello Cc: hayeswang, LKML, Netdev, Linux USB, pali.rohar, anthony.wong On Mon, Jun 06, 2016 at 09:15:21AM -0500, 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. > > 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_limonciello@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) 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. > +{ > + 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) 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:39 ` Greg KH @ 2016-06-06 14:56 ` Mario_Limonciello 2016-06-06 16:12 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Mario_Limonciello @ 2016-06-06 14:56 UTC (permalink / raw) To: gregkh Cc: hayeswang, linux-kernel, netdev, linux-usb, pali.rohar, anthony.wong > -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Monday, June 6, 2016 9:40 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>; > pali.rohar@gmail.com; anthony.wong@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:21AM -0500, 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. > > > > 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_limonciello@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) > > 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. > 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. > > > > +{ > > + 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) > > an "error" here is a "normal" thing, so please document it. 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:56 ` Mario_Limonciello @ 2016-06-06 16:12 ` Greg KH 2016-06-06 17:24 ` Mario_Limonciello 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2016-06-06 16:12 UTC (permalink / raw) To: Mario_Limonciello Cc: hayeswang, linux-kernel, netdev, linux-usb, pali.rohar, anthony.wong On Mon, Jun 06, 2016 at 02:56:32PM +0000, Mario_Limonciello@Dell.com wrote: > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Monday, June 6, 2016 9:40 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>; > > pali.rohar@gmail.com; anthony.wong@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:21AM -0500, 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. > > > > > > 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_limonciello@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) > > > > 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. > > > > 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. 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. > > > +{ > > > + 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) > > > > an "error" here is a "normal" thing, so please document it. > > 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. 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 16:12 ` Greg KH @ 2016-06-06 17:24 ` Mario_Limonciello 2016-06-06 17:38 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Mario_Limonciello @ 2016-06-06 17:24 UTC (permalink / raw) To: gregkh Cc: hayeswang, linux-kernel, netdev, linux-usb, pali.rohar, anthony.wong > > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 17:24 ` Mario_Limonciello @ 2016-06-06 17:38 ` Greg KH 0 siblings, 0 replies; 13+ messages in thread From: Greg KH @ 2016-06-06 17:38 UTC (permalink / raw) To: Mario_Limonciello Cc: hayeswang, linux-kernel, netdev, linux-usb, pali.rohar, anthony.wong On Mon, Jun 06, 2016 at 05:24:57PM +0000, Mario_Limonciello@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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:15 ` Mario Limonciello 2016-06-06 14:39 ` Greg KH @ 2016-06-06 14:40 ` Andrew Lunn 2016-06-06 14:57 ` Mario_Limonciello 1 sibling, 1 reply; 13+ messages in thread From: Andrew Lunn @ 2016-06-06 14:40 UTC (permalink / raw) To: Mario Limonciello Cc: hayeswang, LKML, Netdev, Linux USB, pali.rohar, anthony.wong, Greg KH > + /* 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:40 ` Andrew Lunn @ 2016-06-06 14:57 ` Mario_Limonciello 0 siblings, 0 replies; 13+ messages in thread From: Mario_Limonciello @ 2016-06-06 14:57 UTC (permalink / raw) To: andrew Cc: hayeswang, linux-kernel, netdev, linux-usb, pali.rohar, anthony.wong, gregkh > -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Monday, June 6, 2016 9:41 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>; > pali.rohar@gmail.com; anthony.wong@canonical.com; Greg KH > <gregkh@linuxfoundation.org> > Subject: Re: [PATCH v3] r8152: Add support for setting pass through MAC > address on RTL8153-AD > > > + /* 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 OK thanks, I'll adjust that to use a different warn. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:15 [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD Mario Limonciello 2016-06-06 14:15 ` Mario Limonciello @ 2016-06-06 14:40 ` Greg KH 2016-06-06 14:43 ` Mario_Limonciello 1 sibling, 1 reply; 13+ messages in thread From: Greg KH @ 2016-06-06 14:40 UTC (permalink / raw) To: Mario Limonciello Cc: hayeswang, LKML, Netdev, Linux USB, pali.rohar, anthony.wong 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:40 ` Greg KH @ 2016-06-06 14:43 ` Mario_Limonciello 2016-06-06 14:50 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Mario_Limonciello @ 2016-06-06 14:43 UTC (permalink / raw) To: gregkh Cc: hayeswang, linux-kernel, netdev, linux-usb, pali.rohar, anthony.wong > -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Monday, June 6, 2016 9:40 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>; > pali.rohar@gmail.com; anthony.wong@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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:43 ` Mario_Limonciello @ 2016-06-06 14:50 ` Greg KH 2016-06-06 14:51 ` Mario_Limonciello 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2016-06-06 14:50 UTC (permalink / raw) To: Mario_Limonciello Cc: hayeswang, linux-kernel, netdev, linux-usb, pali.rohar, anthony.wong On Mon, Jun 06, 2016 at 02:43:37PM +0000, Mario_Limonciello@Dell.com wrote: > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Monday, June 6, 2016 9:40 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>; > > pali.rohar@gmail.com; anthony.wong@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. 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD 2016-06-06 14:50 ` Greg KH @ 2016-06-06 14:51 ` Mario_Limonciello 0 siblings, 0 replies; 13+ messages in thread From: Mario_Limonciello @ 2016-06-06 14:51 UTC (permalink / raw) To: gregkh Cc: hayeswang, linux-kernel, netdev, linux-usb, pali.rohar, anthony.wong > -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Monday, June 6, 2016 9:50 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 v3] r8152: Add support for setting pass through MAC > address on RTL8153-AD > > On Mon, Jun 06, 2016 at 02:43:37PM +0000, Mario_Limonciello@Dell.com > wrote: > > > -----Original Message----- > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > Sent: Monday, June 6, 2016 9:40 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>; > > > pali.rohar@gmail.com; anthony.wong@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. > > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-06-06 17:39 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-06 14:15 [PATCH v3] r8152: Add support for setting pass through MAC address on RTL8153-AD Mario Limonciello 2016-06-06 14:15 ` Mario Limonciello 2016-06-06 14:39 ` Greg KH 2016-06-06 14:56 ` Mario_Limonciello 2016-06-06 16:12 ` Greg KH 2016-06-06 17:24 ` Mario_Limonciello 2016-06-06 17:38 ` Greg KH 2016-06-06 14:40 ` Andrew Lunn 2016-06-06 14:57 ` Mario_Limonciello 2016-06-06 14:40 ` Greg KH 2016-06-06 14:43 ` Mario_Limonciello 2016-06-06 14:50 ` Greg KH 2016-06-06 14:51 ` Mario_Limonciello
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.