| Message ID | 1465323757-7249-1-git-send-email-mario_limonciello@dell.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Mario Limonciello <mario_limonciello@dell.com> Date: Tue, 7 Jun 2016 13:22:37 -0500 > The RTL8153-AD supports a persistent system specific MAC address. > This means a device plugged into two different systems with host side > support will show different (but persistent) MAC addresses. > > This information for the system's persistent MAC address is burned in when > the system HW is built and available under \_SB.AMAC in the DSDT at runtime. > > This technology is currently implemented in the Dell TB15 and WD15 Type-C > docks. More information is available here: > http://www.dell.com/support/article/us/en/04/SLN301147 > > Signed-off-by: Mario Limonciello <mario_limonciello@dell.com> > --- > Changes from v5: > * Correct return value if hex2bin succesful but invalid ether addr Have things calmed down enough now that I can apply this? Let's see some ACKs.
On Fri, Jun 10, 2016 at 10:51:56PM -0700, David Miller wrote: > From: Mario Limonciello <mario_limonciello@dell.com> > Date: Tue, 7 Jun 2016 13:22:37 -0500 > > > The RTL8153-AD supports a persistent system specific MAC address. > > This means a device plugged into two different systems with host side > > support will show different (but persistent) MAC addresses. > > > > This information for the system's persistent MAC address is burned in when > > the system HW is built and available under \_SB.AMAC in the DSDT at runtime. > > > > This technology is currently implemented in the Dell TB15 and WD15 Type-C > > docks. More information is available here: > > http://www.dell.com/support/article/us/en/04/SLN301147 > > > > Signed-off-by: Mario Limonciello <mario_limonciello@dell.com> > > --- > > Changes from v5: > > * Correct return value if hex2bin succesful but invalid ether addr > > Have things calmed down enough now that I can apply this? Hi David I think the code has reaching the level of maturity needed for acceptance. So for the code quality: Reviewed-by: Andrew Lunn <andrew@lunn.ch> What is still open is do we want to accept it at all? Do we accept the concept of putting the same MAC address on multiple interfaces at hotplug time? Do we trust BIOS vendors to not keep changing DSDT property name, since it is not standardised? Do we want this at all should be decided by somebody more senior then those passing comments on the code. Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: Sat, 11 Jun 2016 17:39:21 +0200 > What is still open is do we want to accept it at all? Do we accept the > concept of putting the same MAC address on multiple interfaces at > hotplug time? Do we trust BIOS vendors to not keep changing DSDT > property name, since it is not standardised? > > Do we want this at all should be decided by somebody more senior then > those passing comments on the code. Indeed, I think the behavior of using the same MAC address on multiple interfaces if we plug several of these in at once is not good. We shouldn't behave this way just because the Microsoft driver does.
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Saturday, June 11, 2016 12:42 PM > To: andrew@lunn.ch > Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; > 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; gregkh@linuxfoundation.org > Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC > address on RTL8153-AD > > From: Andrew Lunn <andrew@lunn.ch> > Date: Sat, 11 Jun 2016 17:39:21 +0200 > > > What is still open is do we want to accept it at all? Do we accept the > > concept of putting the same MAC address on multiple interfaces at > > hotplug time? Do we trust BIOS vendors to not keep changing DSDT > > property name, since it is not standardised? > > It's worth saying - standardized a property name doesn't indemnify it. Properties change all the time from one version of a spec to another. I can only speak for Dell, but it's in our best interest to keep the BIOS side of the codebase around this simpler too. > > Do we want this at all should be decided by somebody more senior then > > those passing comments on the code. > > Indeed, I think the behavior of using the same MAC address on multiple > interfaces if we plug several of these in at once is not good. > > We shouldn't behave this way just because the Microsoft driver does. This is really grasping at an extreme corner case scenario. Dell TB15 and WD15 docks are currently only ones on the market with RTL8135-AD and MAC address pass through efuse bit set. These docks are not inexpensive. If someone really wants multiple USB NIC's plugged in, they can pick up a second USB NIC from the web for far cheaper than buying a second dock. Also for what it's worth, the docks don't allow daisy chaining. The dock EC's will reject the second dock from functioning through the downstream connection. The only way that two docks could be hooked up and functional is on a machine with multiple type C ports. If you still think it's worth solving, what would you like done as an alternative? I would really like to have some implementation of this that you guys are comfortable with upstream. There was already discussion and an implementation in this thread about tracking if the aux MAC was assigned to something and only allowing one device to get that assignment. There was a limitation that you won't be able to know which device gets the auxiliary MAC address. It will be based solely upon hotplug order. There was also in one version of the patch a way to turn off this behavior In a module parameter. Greg KH wasn't fond of that, so it's not present in the current version. I can add either of those back in if they would help the case. Another option I wanted to offer was turning this behavior on via kernel configuration option and let the distros decide if they want to turn it on for users.
On Saturday 11 June 2016 19:42:26 David Miller wrote: > From: Andrew Lunn <andrew@lunn.ch> > Date: Sat, 11 Jun 2016 17:39:21 +0200 > > > What is still open is do we want to accept it at all? Do we accept > > the concept of putting the same MAC address on multiple interfaces > > at hotplug time? Do we trust BIOS vendors to not keep changing > > DSDT property name, since it is not standardised? > > > > Do we want this at all should be decided by somebody more senior > > then those passing comments on the code. > > Indeed, I think the behavior of using the same MAC address on > multiple interfaces if we plug several of these in at once is not > good. > > We shouldn't behave this way just because the Microsoft driver does. I agree, but in some cases it is night mare for local admins when booting different OS cause changing MAC address on local network. Another similar situation: Imagine that you have two USB network cards and both have "burned" into their registers same MAC address. If you connect both those USB network cards, linux kernel bind appropriate driver which read MAC address for both those cards. But those addresses are same. What will linux kernel do in this case? This is very similar situation as those Dell usb network cards told us "hey, use address which is in ACPI DSDT table". Either we should trust what network card what told us, or not and then generate MAC addresses in better way. Just my opinion...
On Tue, Jun 14, 2016 at 06:28:10PM +0200, Pali Rohár wrote: > On Saturday 11 June 2016 19:42:26 David Miller wrote: > > From: Andrew Lunn <andrew@lunn.ch> > > Date: Sat, 11 Jun 2016 17:39:21 +0200 > > > > > What is still open is do we want to accept it at all? Do we accept > > > the concept of putting the same MAC address on multiple interfaces > > > at hotplug time? Do we trust BIOS vendors to not keep changing > > > DSDT property name, since it is not standardised? > > > > > > Do we want this at all should be decided by somebody more senior > > > then those passing comments on the code. > > > > Indeed, I think the behavior of using the same MAC address on > > multiple interfaces if we plug several of these in at once is not > > good. > > > > We shouldn't behave this way just because the Microsoft driver does. > > I agree, but in some cases it is night mare for local admins when > booting different OS cause changing MAC address on local network. > > Another similar situation: Imagine that you have two USB network cards > and both have "burned" into their registers same MAC address. If you > connect both those USB network cards, linux kernel bind appropriate > driver which read MAC address for both those cards. But those addresses > are same. What will linux kernel do in this case? If you can find such a broken USB device, try it and see :) (hint, might be hard to find, I've never seen such a device before.) I don't see how that pertains to this issue, sorry, how does broken USB hardware compare to a working Dell device? thanks, greg k-h
On Tuesday 14 June 2016 18:40:17 Greg KH wrote: > On Tue, Jun 14, 2016 at 06:28:10PM +0200, Pali Rohár wrote: > > On Saturday 11 June 2016 19:42:26 David Miller wrote: > > > From: Andrew Lunn <andrew@lunn.ch> > > > Date: Sat, 11 Jun 2016 17:39:21 +0200 > > > > > > > What is still open is do we want to accept it at all? Do we > > > > accept the concept of putting the same MAC address on multiple > > > > interfaces at hotplug time? Do we trust BIOS vendors to not > > > > keep changing DSDT property name, since it is not > > > > standardised? > > > > > > > > Do we want this at all should be decided by somebody more > > > > senior then those passing comments on the code. > > > > > > Indeed, I think the behavior of using the same MAC address on > > > multiple interfaces if we plug several of these in at once is not > > > good. > > > > > > We shouldn't behave this way just because the Microsoft driver > > > does. > > > > I agree, but in some cases it is night mare for local admins when > > booting different OS cause changing MAC address on local network. > > > > Another similar situation: Imagine that you have two USB network > > cards and both have "burned" into their registers same MAC > > address. If you connect both those USB network cards, linux kernel > > bind appropriate driver which read MAC address for both those > > cards. But those addresses are same. What will linux kernel do in > > this case? > > If you can find such a broken USB device, try it and see :) What do you mean by broken USB device? You have never seen two ethernet cards with same MAC addresses? Right I have not seen two USB, but there is non zero chance that could happen. Specially now when more and more people starts using USB network cards. > (hint, might be hard to find, I've never seen such a device before.) > > I don't see how that pertains to this issue, sorry, how does broken > USB hardware compare to a working Dell device? It is same, how to handle two network cards which tell us, that they have same MAC addresses.
> -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Tuesday, June 14, 2016 11:48 AM > To: Greg KH <gregkh@linuxfoundation.org> > Cc: David Miller <davem@davemloft.net>; andrew@lunn.ch; Limonciello, > Mario <Mario_Limonciello@Dell.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 v6] r8152: Add support for setting pass through MAC > address on RTL8153-AD > > On Tuesday 14 June 2016 18:40:17 Greg KH wrote: > > On Tue, Jun 14, 2016 at 06:28:10PM +0200, Pali Rohár wrote: > > > On Saturday 11 June 2016 19:42:26 David Miller wrote: > > > > From: Andrew Lunn <andrew@lunn.ch> > > > > Date: Sat, 11 Jun 2016 17:39:21 +0200 > > > > > > > > > What is still open is do we want to accept it at all? Do we > > > > > accept the concept of putting the same MAC address on multiple > > > > > interfaces at hotplug time? Do we trust BIOS vendors to not > > > > > keep changing DSDT property name, since it is not > > > > > standardised? > > > > > > > > > > Do we want this at all should be decided by somebody more > > > > > senior then those passing comments on the code. > > > > > > > > Indeed, I think the behavior of using the same MAC address on > > > > multiple interfaces if we plug several of these in at once is not > > > > good. > > > > > > > > We shouldn't behave this way just because the Microsoft driver > > > > does. > > > > > > I agree, but in some cases it is night mare for local admins when > > > booting different OS cause changing MAC address on local network. > > > > > > Another similar situation: Imagine that you have two USB network > > > cards and both have "burned" into their registers same MAC > > > address. If you connect both those USB network cards, linux kernel > > > bind appropriate driver which read MAC address for both those > > > cards. But those addresses are same. What will linux kernel do in > > > this case? > > > > If you can find such a broken USB device, try it and see :) > > What do you mean by broken USB device? > > You have never seen two ethernet cards with same MAC addresses? Right I > have not seen two USB, but there is non zero chance that could happen. > Specially now when more and more people starts using USB network cards. > > > (hint, might be hard to find, I've never seen such a device before.) > > > > I don't see how that pertains to this issue, sorry, how does broken > > USB hardware compare to a working Dell device? > > It is same, how to handle two network cards which tell us, that they > have same MAC addresses. > The kernel handles this just fine. In doing this patch I checked to see what it does in that scenario. Two devices are made. systemd doesn't rename the second device via the MAC name (eg enxAABBCCDDEEFF).
> > It is same, how to handle two network cards which tell us, that they > > have same MAC addresses. > > > > The kernel handles this just fine. In doing this patch I checked to see > what it does in that scenario. Two devices are made. systemd doesn't > rename the second device via the MAC name (eg enxAABBCCDDEEFF). What does you dhcp server do? Does it gives out the same IP address? You then have two interfaces on the same network, with the same MAC address and IP address. Then what happens? Andrew
> -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Tuesday, June 14, 2016 12:23 PM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: pali.rohar@gmail.com; gregkh@linuxfoundation.org; > davem@davemloft.net; hayeswang@realtek.com; linux- > kernel@vger.kernel.org; netdev@vger.kernel.org; linux- > usb@vger.kernel.org; anthony.wong@canonical.com > Subject: Re: [PATCH v6] r8152: Add support for setting pass through MAC > address on RTL8153-AD > > > > It is same, how to handle two network cards which tell us, that they > > > have same MAC addresses. > > > > > > > The kernel handles this just fine. In doing this patch I checked to see > > what it does in that scenario. Two devices are made. systemd doesn't > > rename the second device via the MAC name (eg enxAABBCCDDEEFF). > > What does you dhcp server do? Does it gives out the same IP address? > You then have two interfaces on the same network, with the same MAC > address and IP address. Then what happens? > > Andrew I didn't test it on the same network, I used two separate networks. I expect that the DHCP server would be awfully confused and you'd run down an interesting problem path if it got the same MAC twice.
From: Pali Rohár <pali.rohar@gmail.com> Date: Tue, 14 Jun 2016 18:47:36 +0200 > You have never seen two ethernet cards with same MAC addresses? Right I > have not seen two USB, but there is non zero chance that could happen. It would be an error scenerio, and something to be avoided. It is a valid and correct assumption that one is able to put several devices at the same time on the same physical network and expect it to work. The behavior added by the change in question invalidates that. I'm trying to consider the long term aspects of this, which is that if more devices adopt this scheme we're in trouble if we blindly interpret the MAC address in this way. This firmware MAC property facility seems to be designed with only an extremely narrow use case being considered.
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Tuesday, June 14, 2016 1:35 PM > To: pali.rohar@gmail.com > Cc: gregkh@linuxfoundation.org; andrew@lunn.ch; Limonciello, Mario > <Mario_Limonciello@Dell.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 v6] r8152: Add support for setting pass through MAC > address on RTL8153-AD > > From: Pali Rohár <pali.rohar@gmail.com> > Date: Tue, 14 Jun 2016 18:47:36 +0200 > > > You have never seen two ethernet cards with same MAC addresses? Right > I > > have not seen two USB, but there is non zero chance that could happen. > > It would be an error scenerio, and something to be avoided. > > It is a valid and correct assumption that one is able to put > several devices at the same time on the same physical network > and expect it to work. > > The behavior added by the change in question invalidates that. > > I'm trying to consider the long term aspects of this, which is that if > more devices adopt this scheme we're in trouble if we blindly > interpret the MAC address in this way. > Do you mean if other manufacturers start to ship devices with RTL8135-AD's w/ this pass through bit set and people start to try to mix and match? > This firmware MAC property facility seems to be designed with only an > extremely narrow use case being considered. Yes, as I understand it this is the reason that it's only on such specific devices that the mac address pass through bit is actually set on the efuse.
> -----Original Message----- > From: Limonciello, Mario > Sent: Tuesday, June 14, 2016 5:27 PM > To: 'David Miller' <davem@davemloft.net>; pali.rohar@gmail.com > Cc: gregkh@linuxfoundation.org; andrew@lunn.ch; > hayeswang@realtek.com; linux-kernel@vger.kernel.org; > netdev@vger.kernel.org; linux-usb@vger.kernel.org; > anthony.wong@canonical.com > Subject: RE: [PATCH v6] r8152: Add support for setting pass through MAC > address on RTL8153-AD > > > -----Original Message----- > > From: David Miller [mailto:davem@davemloft.net] > > Sent: Tuesday, June 14, 2016 1:35 PM > > To: pali.rohar@gmail.com > > Cc: gregkh@linuxfoundation.org; andrew@lunn.ch; Limonciello, Mario > > <Mario_Limonciello@Dell.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 v6] r8152: Add support for setting pass through MAC > > address on RTL8153-AD > > > > From: Pali Rohár <pali.rohar@gmail.com> > > Date: Tue, 14 Jun 2016 18:47:36 +0200 > > > > > You have never seen two ethernet cards with same MAC addresses? > Right > > I > > > have not seen two USB, but there is non zero chance that could happen. > > > > It would be an error scenerio, and something to be avoided. > > > > It is a valid and correct assumption that one is able to put > > several devices at the same time on the same physical network > > and expect it to work. > > > > The behavior added by the change in question invalidates that. > > > > I'm trying to consider the long term aspects of this, which is that if > > more devices adopt this scheme we're in trouble if we blindly > > interpret the MAC address in this way. > > > > Do you mean if other manufacturers start to ship devices with > RTL8135-AD's w/ this pass through bit set and people start to try to > mix and match? > > > This firmware MAC property facility seems to be designed with only an > > extremely narrow use case being considered. > > Yes, as I understand it this is the reason that it's only on such specific devices > that the mac address pass through bit is actually set on the efuse. David, Did you have any more thoughts about this? I'm happy to make some other adjustments to the patch, if you have some recommendations.
> David, > > Did you have any more thoughts about this? I'm happy to make some other > adjustments to the patch, if you have some recommendations. Hi, I just wanted to share that the maintenance BIOSes released for the Dell platforms with Type-C this past week enables the MAC address pass through feature in UEFI, so any network booted machines will offer the auxiliary MAC to the DHCP server. In Windows a network booted machine will always use auxiliary MAC now. XPS 9350 (BIOS 1.4.4): http://goo.gl/7Sw2DZ Please let me know what else can be done for this patch to make it acceptable so we can have parity for Linux. Thanks,
From: <Mario_Limonciello@Dell.com> Date: Mon, 11 Jul 2016 21:54:07 +0000 > Please let me know what else can be done for this patch to make it > acceptable so we can have parity for Linux. Just resubmit it and I'll apply it, I'm so tired of hearing about this...
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 3f9f6ed..fc58669 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -26,6 +26,7 @@ #include <linux/mdio.h> #include <linux/usb/cdc.h> #include <linux/suspend.h> +#include <linux/acpi.h> /* Information for net-next */ #define NETNEXT_VERSION "08" @@ -455,6 +456,11 @@ /* SRAM_IMPEDANCE */ #define RX_DRIVING_MASK 0x6000 +/* MAC PASSTHRU */ +#define AD_MASK 0xfee0 +#define EFUSE 0xcfdb +#define PASS_THRU_MASK 0x1 + enum rtl_register_content { _1000bps = 0x10, _100bps = 0x08, @@ -1030,6 +1036,65 @@ out1: return ret; } +/* Devices containing RTL8153-AD can support a persistent + * host system provided MAC address. + * Examples of this are Dell TB15 and Dell WD15 docks + */ +static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa) +{ + acpi_status status; + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; + union acpi_object *obj; + int ret = -EINVAL; + u32 ocp_data; + unsigned char buf[6]; + + /* test for -AD variant of RTL8153 */ + ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0); + if ((ocp_data & AD_MASK) != 0x1000) + return -ENODEV; + + /* test for MAC address pass-through bit */ + ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE); + if ((ocp_data & PASS_THRU_MASK) != 1) + return -ENODEV; + + /* returns _AUXMAC_#AABBCCDDEEFF# */ + status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer); + obj = (union acpi_object *)buffer.pointer; + if (!ACPI_SUCCESS(status)) + return -ENODEV; + if (obj->type != ACPI_TYPE_BUFFER || obj->string.length != 0x17) { + netif_warn(tp, probe, tp->netdev, + "Invalid buffer when reading pass-thru MAC addr: " + "(%d, %d)\n", + obj->type, obj->string.length); + goto amacout; + } + if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0 || + strncmp(obj->string.pointer + 0x15, "#", 1) != 0) { + netif_warn(tp, probe, tp->netdev, + "Invalid header when reading pass-thru MAC addr\n"); + goto amacout; + } + ret = hex2bin(buf, obj->string.pointer + 9, 6); + if (!(ret == 0 && is_valid_ether_addr(buf))) { + netif_warn(tp, probe, tp->netdev, + "Invalid MAC when reading pass-thru MAC addr: " + "%d, %pM\n", ret, buf); + ret = -EINVAL; + goto amacout; + } + memcpy(sa->sa_data, buf, 6); + ether_addr_copy(tp->netdev->dev_addr, sa->sa_data); + netif_info(tp, probe, tp->netdev, + "Using pass-thru MAC addr %pM\n", sa->sa_data); + +amacout: + kfree(obj); + return ret; +} + static int set_ethernet_addr(struct r8152 *tp) { struct net_device *dev = tp->netdev; @@ -1038,8 +1103,15 @@ static int set_ethernet_addr(struct r8152 *tp) if (tp->version == RTL_VER_01) ret = pla_ocp_read(tp, PLA_IDR, 8, sa.sa_data); - else - ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data); + else { + /* if this is not an RTL8153-AD, no eFuse mac pass thru set, + * or system doesn't provide valid _SB.AMAC this will be + * be expected to non-zero + */ + ret = vendor_mac_passthru_addr_read(tp, &sa); + if (ret < 0) + ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data); + } if (ret < 0) { netif_err(tp, probe, dev, "Get ether addr fail\n");
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> --- Changes from v5: * Correct return value if hex2bin succesful but invalid ether addr drivers/net/usb/r8152.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 74 insertions(+), 2 deletions(-)