[PATCH] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[PATCH] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Aaron Ma]

RTL8153-BL is used in Lenovo Thunderbolt4 dock.
Add the support of MAC passthrough.
This is ported from Realtek Outbox driver r8152.53.56-2.15.0.

There are 2 kinds of rules for MAC passthrough of Lenovo products,
1st USB vendor ID belongs to Lenovo, 2nd the chip of RTL8153-BL
is dedicated for Lenovo. Check the ocp data first then set ACPI object
names.

Suggested-by: Hayes Wang <hayeswang@realtek.com>
Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/net/usb/r8152.c | 44 ++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ee41088c5251..df997b330ee4 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -718,6 +718,7 @@ enum spd_duplex {
 #define AD_MASK			0xfee0
 #define BND_MASK		0x0004
 #define BD_MASK			0x0001
+#define BL_MASK                 BIT(3)
 #define EFUSE			0xcfdb
 #define PASS_THRU_MASK		0x1
 
@@ -1606,31 +1607,34 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 	acpi_object_type mac_obj_type;
 	int mac_strlen;
 
+	/* test for -AD variant of RTL8153 */
+	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+	if ((ocp_data & AD_MASK) == 0x1000) {
+		/* test for MAC address pass-through bit */
+		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
+		if ((ocp_data & PASS_THRU_MASK) != 1) {
+			netif_dbg(tp, probe, tp->netdev,
+					"No efuse for RTL8153-AD MAC pass through\n");
+			return -ENODEV;
+		}
+	} else {
+		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
+		if (tp->version == RTL_VER_09 && (ocp_data & BL_MASK)) {
+			/* test for RTL8153BL for Lenovo */
+			tp->lenovo_macpassthru = 1;
+		} else if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK) == 0) {
+			/* test for RTL8153-BND and RTL8153-BD */
+			netif_dbg(tp, probe, tp->netdev,
+					"Invalid variant for MAC pass through\n");
+			return -ENODEV;
+		}
+	}
+
 	if (tp->lenovo_macpassthru) {
 		mac_obj_name = "\\MACA";
 		mac_obj_type = ACPI_TYPE_STRING;
 		mac_strlen = 0x16;
 	} else {
-		/* test for -AD variant of RTL8153 */
-		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
-		if ((ocp_data & AD_MASK) == 0x1000) {
-			/* test for MAC address pass-through bit */
-			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
-			if ((ocp_data & PASS_THRU_MASK) != 1) {
-				netif_dbg(tp, probe, tp->netdev,
-						"No efuse for RTL8153-AD MAC pass through\n");
-				return -ENODEV;
-			}
-		} else {
-			/* test for RTL8153-BND and RTL8153-BD */
-			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
-			if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK) == 0) {
-				netif_dbg(tp, probe, tp->netdev,
-						"Invalid variant for MAC pass through\n");
-				return -ENODEV;
-			}
-		}
-
 		mac_obj_name = "\\_SB.AMAC";
 		mac_obj_type = ACPI_TYPE_BUFFER;
 		mac_strlen = 0x17;
-- 
2.32.0

Re: [PATCH] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Greg KH]

On Thu, Jan 27, 2022 at 06:01:09PM +0800, Aaron Ma wrote:
> RTL8153-BL is used in Lenovo Thunderbolt4 dock.
> Add the support of MAC passthrough.
> This is ported from Realtek Outbox driver r8152.53.56-2.15.0.
> 
> There are 2 kinds of rules for MAC passthrough of Lenovo products,
> 1st USB vendor ID belongs to Lenovo, 2nd the chip of RTL8153-BL
> is dedicated for Lenovo. Check the ocp data first then set ACPI object
> names.
> 
> Suggested-by: Hayes Wang <hayeswang@realtek.com>
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/net/usb/r8152.c | 44 ++++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index ee41088c5251..df997b330ee4 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -718,6 +718,7 @@ enum spd_duplex {
>  #define AD_MASK			0xfee0
>  #define BND_MASK		0x0004
>  #define BD_MASK			0x0001
> +#define BL_MASK                 BIT(3)

No tab?  :(

Re: [PATCH] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Aaron Ma]

On 1/27/22 18:10, Greg KH wrote:
>> +#define BL_MASK                 BIT(3)
> No tab?:(
> 

My bad, vim paste.
Allow me to fix it v2.

Aaron

RE: [PATCH] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Hayes Wang]

Aaron Ma <aaron.ma@canonical.com>
> Sent: Thursday, January 27, 2022 6:01 PM
[...]
> @@ -1606,31 +1607,34 @@ static int vendor_mac_passthru_addr_read(struct
> r8152 *tp, struct sockaddr *sa)
>  	acpi_object_type mac_obj_type;
>  	int mac_strlen;
> 
> +	/* test for -AD variant of RTL8153 */
> +	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> +	if ((ocp_data & AD_MASK) == 0x1000) {
> +		/* test for MAC address pass-through bit */
> +		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
> +		if ((ocp_data & PASS_THRU_MASK) != 1) {
> +			netif_dbg(tp, probe, tp->netdev,
> +					"No efuse for RTL8153-AD MAC pass through\n");
> +			return -ENODEV;
> +		}
> +	} else {
> +		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
> +		if (tp->version == RTL_VER_09 && (ocp_data & BL_MASK)) {
> +			/* test for RTL8153BL for Lenovo */
> +			tp->lenovo_macpassthru = 1;
> +		} else if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK)
> == 0) {
> +			/* test for RTL8153-BND and RTL8153-BD */
> +			netif_dbg(tp, probe, tp->netdev,
> +					"Invalid variant for MAC pass through\n");
> +			return -ENODEV;

I think the devices with the VID/PID of Lenovo, such as 0x17EF/0x3082 and 0x17EF/0xA387,
would always return -ENODEV here. Is it what you want?


Best Regards,
Hayes


> +		}
> +	}
> +
>  	if (tp->lenovo_macpassthru) {
>  		mac_obj_name = "\\MACA";
>  		mac_obj_type = ACPI_TYPE_STRING;
>  		mac_strlen = 0x16;
>  	} else {
> -		/* test for -AD variant of RTL8153 */
> -		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> -		if ((ocp_data & AD_MASK) == 0x1000) {
> -			/* test for MAC address pass-through bit */
> -			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
> -			if ((ocp_data & PASS_THRU_MASK) != 1) {
> -				netif_dbg(tp, probe, tp->netdev,
> -						"No efuse for RTL8153-AD MAC pass through\n");
> -				return -ENODEV;
> -			}
> -		} else {
> -			/* test for RTL8153-BND and RTL8153-BD */
> -			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
> -			if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK)
> == 0) {
> -				netif_dbg(tp, probe, tp->netdev,
> -						"Invalid variant for MAC pass through\n");
> -				return -ENODEV;
> -			}
> -		}
> -
>  		mac_obj_name = "\\_SB.AMAC";
>  		mac_obj_type = ACPI_TYPE_BUFFER;
>  		mac_strlen = 0x17;
> --
> 2.32.0

Re: [PATCH] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Andrew Lunn]

On Thu, Jan 27, 2022 at 06:01:09PM +0800, Aaron Ma wrote:
> RTL8153-BL is used in Lenovo Thunderbolt4 dock.
> Add the support of MAC passthrough.
> This is ported from Realtek Outbox driver r8152.53.56-2.15.0.
> 
> There are 2 kinds of rules for MAC passthrough of Lenovo products,
> 1st USB vendor ID belongs to Lenovo, 2nd the chip of RTL8153-BL
> is dedicated for Lenovo. Check the ocp data first then set ACPI object
> names.
> 
> Suggested-by: Hayes Wang <hayeswang@realtek.com>
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/net/usb/r8152.c | 44 ++++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index ee41088c5251..df997b330ee4 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -718,6 +718,7 @@ enum spd_duplex {
>  #define AD_MASK			0xfee0
>  #define BND_MASK		0x0004
>  #define BD_MASK			0x0001
> +#define BL_MASK                 BIT(3)

Just to be sure, this is defined by Realtek? This is not just Lenovo
just misusing a reserved bit?

     Andrew

Re: [PATCH] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Aaron Ma]

On 1/27/22 22:19, Andrew Lunn wrote:
> On Thu, Jan 27, 2022 at 06:01:09PM +0800, Aaron Ma wrote:
>> RTL8153-BL is used in Lenovo Thunderbolt4 dock.
>> Add the support of MAC passthrough.
>> This is ported from Realtek Outbox driver r8152.53.56-2.15.0.
>>
>> There are 2 kinds of rules for MAC passthrough of Lenovo products,
>> 1st USB vendor ID belongs to Lenovo, 2nd the chip of RTL8153-BL
>> is dedicated for Lenovo. Check the ocp data first then set ACPI object
>> names.
>>
>> Suggested-by: Hayes Wang <hayeswang@realtek.com>
>> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
>> ---
>>   drivers/net/usb/r8152.c | 44 ++++++++++++++++++++++-------------------
>>   1 file changed, 24 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>> index ee41088c5251..df997b330ee4 100644
>> --- a/drivers/net/usb/r8152.c
>> +++ b/drivers/net/usb/r8152.c
>> @@ -718,6 +718,7 @@ enum spd_duplex {
>>   #define AD_MASK			0xfee0
>>   #define BND_MASK		0x0004
>>   #define BD_MASK			0x0001
>> +#define BL_MASK                 BIT(3)
> 
> Just to be sure, this is defined by Realtek? This is not just Lenovo
> just misusing a reserved bit?
> 

 From what I know, It's defined by Realtek to identify customized product.
It shouldn't be misusing.
Also it's used by Realtek outbox driver.

Aaron

>       Andrew

Re: [PATCH] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Aaron Ma]

On 1/27/22 19:07, Hayes Wang wrote:
> I think the devices with the VID/PID of Lenovo, such as 0x17EF/0x3082 and 0x17EF/0xA387,
> would always return -ENODEV here. Is it what you want?
> 

This was a mistake.
Try to fix it in V3.

Thanks,
Aaron

> 
> Best Regards,
> Hayes

[PATCH v3] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Aaron Ma]

RTL8153-BL is used in Lenovo Thunderbolt4 dock.
Add the support of MAC passthrough.
This is ported from Realtek Outbox driver r8152.53.56-2.15.0.

There are 2 kinds of rules for MAC passthrough of Lenovo products,
1st USB vendor ID belongs to Lenovo, 2nd the chip of RTL8153-BL
is dedicated for Lenovo. Check flag and the ocp data first,
then set ACPI object names.

Suggested-by: Hayes Wang <hayeswang@realtek.com>
Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
v1 -> v2: fix whitespace in definition.
v2 -> v3: check flag of vendor/product ID to avoid it return error
 drivers/net/usb/r8152.c | 45 +++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ee41088c5251..d8350d229f5c 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -718,6 +718,7 @@ enum spd_duplex {
 #define AD_MASK			0xfee0
 #define BND_MASK		0x0004
 #define BD_MASK			0x0001
+#define BL_MASK			BIT(3)
 #define EFUSE			0xcfdb
 #define PASS_THRU_MASK		0x1
 
@@ -1606,31 +1607,35 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 	acpi_object_type mac_obj_type;
 	int mac_strlen;
 
+	/* test for -AD variant of RTL8153 */
+	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
+	if ((ocp_data & AD_MASK) == 0x1000) {
+		/* test for MAC address pass-through bit */
+		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
+		if ((ocp_data & PASS_THRU_MASK) != 1) {
+			netif_dbg(tp, probe, tp->netdev,
+					"No efuse for RTL8153-AD MAC pass through\n");
+			return -ENODEV;
+		}
+	} else {
+		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
+		if (tp->lenovo_macpassthru ||
+				(tp->version == RTL_VER_09 && (ocp_data & BL_MASK))) {
+			/* test for Lenovo vender/product ID or RTL8153BL */
+			tp->lenovo_macpassthru = 1;
+		} else if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK) == 0) {
+			/* test for RTL8153-BND and RTL8153-BD */
+			netif_dbg(tp, probe, tp->netdev,
+					"Invalid variant for MAC pass through\n");
+			return -ENODEV;
+		}
+	}
+
 	if (tp->lenovo_macpassthru) {
 		mac_obj_name = "\\MACA";
 		mac_obj_type = ACPI_TYPE_STRING;
 		mac_strlen = 0x16;
 	} else {
-		/* test for -AD variant of RTL8153 */
-		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
-		if ((ocp_data & AD_MASK) == 0x1000) {
-			/* test for MAC address pass-through bit */
-			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
-			if ((ocp_data & PASS_THRU_MASK) != 1) {
-				netif_dbg(tp, probe, tp->netdev,
-						"No efuse for RTL8153-AD MAC pass through\n");
-				return -ENODEV;
-			}
-		} else {
-			/* test for RTL8153-BND and RTL8153-BD */
-			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
-			if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK) == 0) {
-				netif_dbg(tp, probe, tp->netdev,
-						"Invalid variant for MAC pass through\n");
-				return -ENODEV;
-			}
-		}
-
 		mac_obj_name = "\\_SB.AMAC";
 		mac_obj_type = ACPI_TYPE_BUFFER;
 		mac_strlen = 0x17;
-- 
2.32.0

Re: [PATCH v3] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Henning Schild]

I am still very much against any patches in that direction. The feature
as the vendors envision it does not seem to be really understood or
even explained.
Just narrowing down the device matching caters for vendor lock-in and
confusion when that pass through is happening and when not. And seems
to lead to unmaintainable spaghetti-code. 
People that use this very dock today will see an unexpected mac-change
once they update to a kernel with this patch applied.

But given that some people seem to want that feature in the kernel, i
will stop here and simply disable the feature in the bios. And i will
make sure _not_ get any lenovo gear for my lenovo laptop, not sure that
matches the vendor-vision.

Henning

Am Fri, 28 Jan 2022 12:32:07 +0800
schrieb Aaron Ma <aaron.ma@canonical.com>:

> RTL8153-BL is used in Lenovo Thunderbolt4 dock.
> Add the support of MAC passthrough.
> This is ported from Realtek Outbox driver r8152.53.56-2.15.0.
> 
> There are 2 kinds of rules for MAC passthrough of Lenovo products,
> 1st USB vendor ID belongs to Lenovo, 2nd the chip of RTL8153-BL
> is dedicated for Lenovo. Check flag and the ocp data first,
> then set ACPI object names.
> 
> Suggested-by: Hayes Wang <hayeswang@realtek.com>
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
> v1 -> v2: fix whitespace in definition.
> v2 -> v3: check flag of vendor/product ID to avoid it return error
>  drivers/net/usb/r8152.c | 45
> +++++++++++++++++++++++------------------ 1 file changed, 25
> insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index ee41088c5251..d8350d229f5c 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -718,6 +718,7 @@ enum spd_duplex {
>  #define AD_MASK			0xfee0
>  #define BND_MASK		0x0004
>  #define BD_MASK			0x0001
> +#define BL_MASK			BIT(3)
>  #define EFUSE			0xcfdb
>  #define PASS_THRU_MASK		0x1
>  
> @@ -1606,31 +1607,35 @@ static int
> vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
> acpi_object_type mac_obj_type; int mac_strlen;
>  
> +	/* test for -AD variant of RTL8153 */
> +	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> +	if ((ocp_data & AD_MASK) == 0x1000) {
> +		/* test for MAC address pass-through bit */
> +		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
> +		if ((ocp_data & PASS_THRU_MASK) != 1) {
> +			netif_dbg(tp, probe, tp->netdev,
> +					"No efuse for RTL8153-AD MAC
> pass through\n");
> +			return -ENODEV;
> +		}
> +	} else {
> +		ocp_data = ocp_read_byte(tp, MCU_TYPE_USB,
> USB_MISC_1);
> +		if (tp->lenovo_macpassthru ||
> +				(tp->version == RTL_VER_09 &&
> (ocp_data & BL_MASK))) {
> +			/* test for Lenovo vender/product ID or
> RTL8153BL */
> +			tp->lenovo_macpassthru = 1;
> +		} else if ((ocp_data & BND_MASK) == 0 && (ocp_data &
> BD_MASK) == 0) {
> +			/* test for RTL8153-BND and RTL8153-BD */
> +			netif_dbg(tp, probe, tp->netdev,
> +					"Invalid variant for MAC
> pass through\n");
> +			return -ENODEV;
> +		}
> +	}
> +
>  	if (tp->lenovo_macpassthru) {
>  		mac_obj_name = "\\MACA";
>  		mac_obj_type = ACPI_TYPE_STRING;
>  		mac_strlen = 0x16;
>  	} else {
> -		/* test for -AD variant of RTL8153 */
> -		ocp_data = ocp_read_word(tp, MCU_TYPE_USB,
> USB_MISC_0);
> -		if ((ocp_data & AD_MASK) == 0x1000) {
> -			/* test for MAC address pass-through bit */
> -			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB,
> EFUSE);
> -			if ((ocp_data & PASS_THRU_MASK) != 1) {
> -				netif_dbg(tp, probe, tp->netdev,
> -						"No efuse for
> RTL8153-AD MAC pass through\n");
> -				return -ENODEV;
> -			}
> -		} else {
> -			/* test for RTL8153-BND and RTL8153-BD */
> -			ocp_data = ocp_read_byte(tp, MCU_TYPE_USB,
> USB_MISC_1);
> -			if ((ocp_data & BND_MASK) == 0 && (ocp_data
> & BD_MASK) == 0) {
> -				netif_dbg(tp, probe, tp->netdev,
> -						"Invalid variant for
> MAC pass through\n");
> -				return -ENODEV;
> -			}
> -		}
> -
>  		mac_obj_name = "\\_SB.AMAC";
>  		mac_obj_type = ACPI_TYPE_BUFFER;
>  		mac_strlen = 0x17;

Re: [PATCH v3] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Andrew Lunn]

On Fri, Jan 28, 2022 at 09:21:03AM +0100, Henning Schild wrote:
> I am still very much against any patches in that direction. The feature
> as the vendors envision it does not seem to be really understood or
> even explained.
> Just narrowing down the device matching caters for vendor lock-in and
> confusion when that pass through is happening and when not. And seems
> to lead to unmaintainable spaghetti-code. 
> People that use this very dock today will see an unexpected mac-change
> once they update to a kernel with this patch applied.

I've not yet been convinced by replies that the proposed code really
does only match the given dock, and not random USB dongles. To be
convinced i would probably like to see code which positively
identifies the dock, and that the USB device is on the correct port of
the USB hub within the dock. I doubt you can actually do that in a
sane way inside an Ethernet driver. As you say, it will likely lead to
unmaintainable spaghetti-code.

I also don't really think the vendor would be keen on adding code
which they know will get reverted as soon as it is shown to cause a
regression.

So i would prefer to NACK this, and push it to udev rules where you
have a complete picture of the hardware and really can identify with
100% certainty it really is the docks NIC.

   Andrew

RE: [PATCH v3] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, January 28, 2022 12:07
> To: Henning Schild <henning.schild@siemens.com>
> Cc: Aaron Ma <aaron.ma@canonical.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; kuba@kernel.org; linux-usb@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> gregkh@linuxfoundation.org; davem@davemloft.net;
> hayeswang@realtek.com; tiwai@suse.de
> Subject: Re: [PATCH v3] net: usb: r8152: Add MAC passthrough support for
> RTL8153BL
> 
> On Fri, Jan 28, 2022 at 09:21:03AM +0100, Henning Schild wrote:
> > I am still very much against any patches in that direction. The feature
> > as the vendors envision it does not seem to be really understood or
> > even explained.
> > Just narrowing down the device matching caters for vendor lock-in and
> > confusion when that pass through is happening and when not. And seems
> > to lead to unmaintainable spaghetti-code.
> > People that use this very dock today will see an unexpected mac-change
> > once they update to a kernel with this patch applied.
> 
> I've not yet been convinced by replies that the proposed code really
> does only match the given dock, and not random USB dongles. 

Didn't Realtek confirm this bit is used to identify the Lenovo devices?

> To be
> convinced i would probably like to see code which positively
> identifies the dock, and that the USB device is on the correct port of
> the USB hub within the dock. I doubt you can actually do that in a
> sane way inside an Ethernet driver. As you say, it will likely lead to
> unmaintainable spaghetti-code.
> 
> I also don't really think the vendor would be keen on adding code
> which they know will get reverted as soon as it is shown to cause a
> regression.
> 
> So i would prefer to NACK this, and push it to udev rules where you
> have a complete picture of the hardware and really can identify with
> 100% certainty it really is the docks NIC.

I remember when I did the Dell implementation I tried userspace first.

Pushing this out to udev has a few other implications I remember hitting:
1) You need to also get the value you're supposed to use from ACPI BIOS
     exported some way in userland too.
2) You can run into race conditions with other device or MAC renaming rules.
    My first try I did it with NM and hit that continually.  So you would probably
    need to land this in systemd or so.

> 
>    Andrew

Re: [PATCH v3] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Henning Schild]

Am Fri, 28 Jan 2022 18:41:16 +0000
schrieb "Limonciello, Mario" <Mario.Limonciello@amd.com>:

> [Public]
> 
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Friday, January 28, 2022 12:07
> > To: Henning Schild <henning.schild@siemens.com>
> > Cc: Aaron Ma <aaron.ma@canonical.com>; Limonciello, Mario
> > <Mario.Limonciello@amd.com>; kuba@kernel.org;
> > linux-usb@vger.kernel.org; netdev@vger.kernel.org;
> > linux-kernel@vger.kernel.org; gregkh@linuxfoundation.org;
> > davem@davemloft.net; hayeswang@realtek.com; tiwai@suse.de
> > Subject: Re: [PATCH v3] net: usb: r8152: Add MAC passthrough
> > support for RTL8153BL
> > 
> > On Fri, Jan 28, 2022 at 09:21:03AM +0100, Henning Schild wrote:  
> > > I am still very much against any patches in that direction. The
> > > feature as the vendors envision it does not seem to be really
> > > understood or even explained.
> > > Just narrowing down the device matching caters for vendor lock-in
> > > and confusion when that pass through is happening and when not.
> > > And seems to lead to unmaintainable spaghetti-code.
> > > People that use this very dock today will see an unexpected
> > > mac-change once they update to a kernel with this patch applied.  
> > 
> > I've not yet been convinced by replies that the proposed code really
> > does only match the given dock, and not random USB dongles.   
> 
> Didn't Realtek confirm this bit is used to identify the Lenovo
> devices?

The question is really why we do all that and that answer really seems
missing. Lenovo might have a very different answer than Dell. All i saw
was Lenovo docks that suggest PXE. While that could be a good case,
bootloaders and OSs should then inherit the MAC of a NIC that is
already up. But sure not try and guess what to do and spoof on their
own. (based on weird bits and topology guesses) I am willing to bet that
grub/pxelinux/uboot/systemd-boot would all fail a PXE run even if the
OS to boot would spoof/inherit.

PXE uses spoofed MAC and bootloader will dhcp again and not spoof. dhcp
would time out or fall back to somewhere (MAC not known), Linux kernel
would not come, PXE boot failed.

In fact i have seen first hand how PXE Windows install failed on a
Lenovo dock and the real NIC worked. That was within my company and i
sure do not know the BIOS settings or what bootloader was involved and
if that Windows installer did active spoofing. But i would say that the
PXE story probably does not really "work" for Windows either.
In fact i am willing to bet the BIOS setting for spoofing was turned
on, because it seems to be the factory default.

And all stories beyond PXE-bootstrap should probably be answered with
"a MAC does not identify a machine". So people that care to ident a
machine should use something like x509, or allow network access in any
other way not relying on a MAC. If "Linux" cares to spoof for the lazy
ones, udev is a better place than the kernel.

> > To be
> > convinced i would probably like to see code which positively
> > identifies the dock, and that the USB device is on the correct port
> > of the USB hub within the dock. I doubt you can actually do that in
> > a sane way inside an Ethernet driver. As you say, it will likely
> > lead to unmaintainable spaghetti-code.
> > 
> > I also don't really think the vendor would be keen on adding code
> > which they know will get reverted as soon as it is shown to cause a
> > regression.
> > 
> > So i would prefer to NACK this, and push it to udev rules where you
> > have a complete picture of the hardware and really can identify with
> > 100% certainty it really is the docks NIC.  
> 
> I remember when I did the Dell implementation I tried userspace first.
> 
> Pushing this out to udev has a few other implications I remember
> hitting: 1) You need to also get the value you're supposed to use
> from ACPI BIOS exported some way in userland too.

Sounds like a problem with ACPI in userspace. So the kernel could
expose ACPI in a better shape. Or you will simply need to see what
systemd thinks about a funny "sed | grep | awk | bc" patch to parse
binary. DMI might contain bits, but without clear vendor instructions
we would be guessing (like on ACPI?).

> 2) You can run into race conditions with other device or MAC renaming
> rules. My first try I did it with NM and hit that continually.  So
> you would probably need to land this in systemd or so.

For sure you would end up in systemd (udev). NM is just one of many
options and would be the wrong place. You might quickly find yourself
in mdev (busybox) as well because of that PXE case or because of an
initrd.

If it was my call i would revert all mac passthough patches and request
Lenovo/Dell and others to present their case first hand. (no
canonical/amd/suse proxies)
The "feature" causes MAC collisions and is not well understood/argued by
anyone.

https://en.wikipedia.org/wiki/MAC_address

> A media access control address (MAC address) is a unique identifier
> assigned to a network interface controller (NIC) ...

unique and NIC, as opposed to colliding and machine

Henning
 
> > 
> >    Andre  

RE: [PATCH v3] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Limonciello, Mario]

[Public]

> > > I've not yet been convinced by replies that the proposed code really
> > > does only match the given dock, and not random USB dongles.
> >
> > Didn't Realtek confirm this bit is used to identify the Lenovo
> > devices?
> 
> The question is really why we do all that and that answer really seems
> missing. Lenovo might have a very different answer than Dell. All i saw
> was Lenovo docks that suggest PXE. While that could be a good case,
> bootloaders and OSs should then inherit the MAC of a NIC that is
> already up. But sure not try and guess what to do and spoof on their
> own. (based on weird bits and topology guesses) I am willing to bet that
> grub/pxelinux/uboot/systemd-boot would all fail a PXE run even if the
> OS to boot would spoof/inherit.
> 
> PXE uses spoofed MAC and bootloader will dhcp again and not spoof. dhcp
> would time out or fall back to somewhere (MAC not known), Linux kernel
> would not come, PXE boot failed.
> 
> In fact i have seen first hand how PXE Windows install failed on a
> Lenovo dock and the real NIC worked. That was within my company and i
> sure do not know the BIOS settings or what bootloader was involved and
> if that Windows installer did active spoofing. But i would say that the
> PXE story probably does not really "work" for Windows either.
> In fact i am willing to bet the BIOS setting for spoofing was turned
> on, because it seems to be the factory default.
> 
> And all stories beyond PXE-bootstrap should probably be answered with
> "a MAC does not identify a machine". So people that care to ident a
> machine should use something like x509, or allow network access in any
> other way not relying on a MAC. If "Linux" cares to spoof for the lazy
> ones, udev is a better place than the kernel.
> 
> > > To be
> > > convinced i would probably like to see code which positively
> > > identifies the dock, and that the USB device is on the correct port
> > > of the USB hub within the dock. I doubt you can actually do that in
> > > a sane way inside an Ethernet driver. As you say, it will likely
> > > lead to unmaintainable spaghetti-code.
> > >
> > > I also don't really think the vendor would be keen on adding code
> > > which they know will get reverted as soon as it is shown to cause a
> > > regression.
> > >
> > > So i would prefer to NACK this, and push it to udev rules where you
> > > have a complete picture of the hardware and really can identify with
> > > 100% certainty it really is the docks NIC.
> >
> > I remember when I did the Dell implementation I tried userspace first.
> >
> > Pushing this out to udev has a few other implications I remember
> > hitting: 1) You need to also get the value you're supposed to use
> > from ACPI BIOS exported some way in userland too.
> 
> Sounds like a problem with ACPI in userspace. So the kernel could
> expose ACPI in a better shape. Or you will simply need to see what
> systemd thinks about a funny "sed | grep | awk | bc" patch to parse
> binary. DMI might contain bits, but without clear vendor instructions
> we would be guessing (like on ACPI?).

Yeah I think if this is to be reverted, step 1 is going to be to export that
data into sysfs from some Dell and Lenovo drivers so userspace can get it.
No funny sed/grep/awk to parse binary.

DMI doesn't contain it (at least for Dell).

> 
> > 2) You can run into race conditions with other device or MAC renaming
> > rules. My first try I did it with NM and hit that continually.  So
> > you would probably need to land this in systemd or so.
> 
> For sure you would end up in systemd (udev). NM is just one of many
> options and would be the wrong place. You might quickly find yourself
> in mdev (busybox) as well because of that PXE case or because of an
> initrd.
> 
> If it was my call i would revert all mac passthough patches and request
> Lenovo/Dell and others to present their case first hand. (no
> canonical/amd/suse proxies)
> The "feature" causes MAC collisions and is not well understood/argued by
> anyone.

Reverting it has the possibility to really mess up machines people have in the
field that have behavior built around it.  I think a clear set of rules for what is allow
to use it is the only safe way forward.  You need to "clearly identify the device" or
something.

Just FYI I'm not intentionally acting as a proxy to anyone at Dell on behalf of AMD,
my only involvement here is because I did the original implementation for Dell
when I was there and so I can speak the how things were and thought processes at
that time.

If the consensus is to revert this then someone from Dell probably does need to
speak up.

> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikip
> edia.org%2Fwiki%2FMAC_address&data=04%7C01%7CMario.Limonciello%
> 40amd.com%7C385b6dd02672490749d208d9e29ba192%7C3dd8961fe4884e60
> 8e11a82d994e183d%7C0%7C0%7C637789980357612909%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000&sdata=9vmPi1%2FF%2BxrkqpjoYoLmmupAJ1vBMckLLo
> 5hDMqTuZA%3D&reserved=0
> 
> > A media access control address (MAC address) is a unique identifier
> > assigned to a network interface controller (NIC) ...
> 
> unique and NIC, as opposed to colliding and machine
> 
> Henning
> 
> > >
> > >    Andre

Re: [PATCH v3] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Henning Schild]

Am Fri, 28 Jan 2022 20:29:58 +0000
schrieb "Limonciello, Mario" <Mario.Limonciello@amd.com>:

> [Public]
> 
> > > > I've not yet been convinced by replies that the proposed code
> > > > really does only match the given dock, and not random USB
> > > > dongles.  
> > >
> > > Didn't Realtek confirm this bit is used to identify the Lenovo
> > > devices?  
> > 
> > The question is really why we do all that and that answer really
> > seems missing. Lenovo might have a very different answer than Dell.
> > All i saw was Lenovo docks that suggest PXE. While that could be a
> > good case, bootloaders and OSs should then inherit the MAC of a NIC
> > that is already up. But sure not try and guess what to do and spoof
> > on their own. (based on weird bits and topology guesses) I am
> > willing to bet that grub/pxelinux/uboot/systemd-boot would all fail
> > a PXE run even if the OS to boot would spoof/inherit.
> > 
> > PXE uses spoofed MAC and bootloader will dhcp again and not spoof.
> > dhcp would time out or fall back to somewhere (MAC not known),
> > Linux kernel would not come, PXE boot failed.
> > 
> > In fact i have seen first hand how PXE Windows install failed on a
> > Lenovo dock and the real NIC worked. That was within my company and
> > i sure do not know the BIOS settings or what bootloader was
> > involved and if that Windows installer did active spoofing. But i
> > would say that the PXE story probably does not really "work" for
> > Windows either. In fact i am willing to bet the BIOS setting for
> > spoofing was turned on, because it seems to be the factory default.
> > 
> > And all stories beyond PXE-bootstrap should probably be answered
> > with "a MAC does not identify a machine". So people that care to
> > ident a machine should use something like x509, or allow network
> > access in any other way not relying on a MAC. If "Linux" cares to
> > spoof for the lazy ones, udev is a better place than the kernel.
> >   
> > > > To be
> > > > convinced i would probably like to see code which positively
> > > > identifies the dock, and that the USB device is on the correct
> > > > port of the USB hub within the dock. I doubt you can actually
> > > > do that in a sane way inside an Ethernet driver. As you say, it
> > > > will likely lead to unmaintainable spaghetti-code.
> > > >
> > > > I also don't really think the vendor would be keen on adding
> > > > code which they know will get reverted as soon as it is shown
> > > > to cause a regression.
> > > >
> > > > So i would prefer to NACK this, and push it to udev rules where
> > > > you have a complete picture of the hardware and really can
> > > > identify with 100% certainty it really is the docks NIC.  
> > >
> > > I remember when I did the Dell implementation I tried userspace
> > > first.
> > >
> > > Pushing this out to udev has a few other implications I remember
> > > hitting: 1) You need to also get the value you're supposed to use
> > > from ACPI BIOS exported some way in userland too.  
> > 
> > Sounds like a problem with ACPI in userspace. So the kernel could
> > expose ACPI in a better shape. Or you will simply need to see what
> > systemd thinks about a funny "sed | grep | awk | bc" patch to parse
> > binary. DMI might contain bits, but without clear vendor
> > instructions we would be guessing (like on ACPI?).  
> 
> Yeah I think if this is to be reverted, step 1 is going to be to
> export that data into sysfs from some Dell and Lenovo drivers so
> userspace can get it. No funny sed/grep/awk to parse binary.
> 
> DMI doesn't contain it (at least for Dell).

Would still cause a nasty vendor-lockin where passthrough would only
happen if peripherals match the laptop. In the end users would end up
very confused or Linux would cater for vendors selling their docks.

> >   
> > > 2) You can run into race conditions with other device or MAC
> > > renaming rules. My first try I did it with NM and hit that
> > > continually.  So you would probably need to land this in systemd
> > > or so.  
> > 
> > For sure you would end up in systemd (udev). NM is just one of many
> > options and would be the wrong place. You might quickly find
> > yourself in mdev (busybox) as well because of that PXE case or
> > because of an initrd.
> > 
> > If it was my call i would revert all mac passthough patches and
> > request Lenovo/Dell and others to present their case first hand. (no
> > canonical/amd/suse proxies)
> > The "feature" causes MAC collisions and is not well
> > understood/argued by anyone.  
> 
> Reverting it has the possibility to really mess up machines people
> have in the field that have behavior built around it.  I think a
> clear set of rules for what is allow to use it is the only safe way
> forward.  You need to "clearly identify the device" or something.

Very true, i was kind of hardlining it. But IMHO the clear rule is that
a MAC does not ident a machine. And while that might break already
deployed setups, it will make clear that passthrough is a clear no-go.
If you do not draw that line, many more patches might follow. Claiming
their rightful place next to Dell hacks.

The revert that did already happen actually has potential to "break"
already deployed setups. It was still done because i brought up the
case and people agreed. I can not speak for the Dell patches, they are
in there for much longer and will be more widely deployed. So a revert
will potentially affect for people. But they sure seem equally
questionable and not well explained.

> Just FYI I'm not intentionally acting as a proxy to anyone at Dell on
> behalf of AMD, my only involvement here is because I did the original
> implementation for Dell when I was there and so I can speak the how
> things were and thought processes at that time.

I bet nobody wanted to act as a proxy, it just happened somehow. If
that proxy can explain in detail why and how that is fine.

Since you have been at Dell and started that, do you happen to know the
real story? Like real ... not "i had to solve jira ticket 42"

> If the consensus is to revert this then someone from Dell probably
> does need to speak up.

If you do not cater for Dell any longer, maybe you would be just the
person to revert and tell them to reveal what "jira 42" was about.
Merge windows are wide open and distros will be slow to follow, so
enough time to react after a revert.

regards,
Henning

> > 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikip
> > edia.org%2Fwiki%2FMAC_address&amp;data=04%7C01%7CMario.Limonciello%
> > 40amd.com%7C385b6dd02672490749d208d9e29ba192%7C3dd8961fe4884e60
> > 8e11a82d994e183d%7C0%7C0%7C637789980357612909%7CUnknown%7CTWF
> > pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> > Mn0%3D%7C3000&amp;sdata=9vmPi1%2FF%2BxrkqpjoYoLmmupAJ1vBMckLLo
> > 5hDMqTuZA%3D&amp;reserved=0
> >   
> > > A media access control address (MAC address) is a unique
> > > identifier assigned to a network interface controller (NIC) ...  
> > 
> > unique and NIC, as opposed to colliding and machine
> > 
> > Henning
> >   
> > > >
> > > >    Andre  

RE: [PATCH v3] net: usb: r8152: Add MAC passthrough support for RTL8153BL

[Hayes Wang]

Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Saturday, January 29, 2022 2:41 AM
[...]
> > I've not yet been convinced by replies that the proposed code really
> > does only match the given dock, and not random USB dongles.
> 
> Didn't Realtek confirm this bit is used to identify the Lenovo devices?

Excuse me. Last week is our vacation of Chinese New Year.
Realtek confirms that bit is used to identify the Lenovo devices.
We use different bits for specific customers.
For RTL8153B, bit 0 and 2 of USB OCP 0xD81F are for Dell. Bit 3 is for Lenovo.
However, Realtek couldn't answer if the Lenovo devices are used on docks only.

Best Regards,
Hayes

> > To be
> > convinced i would probably like to see code which positively
> > identifies the dock, and that the USB device is on the correct port of
> > the USB hub within the dock. I doubt you can actually do that in a
> > sane way inside an Ethernet driver. As you say, it will likely lead to
> > unmaintainable spaghetti-code.
> >
> > I also don't really think the vendor would be keen on adding code
> > which they know will get reverted as soon as it is shown to cause a
> > regression.
> >
> > So i would prefer to NACK this, and push it to udev rules where you
> > have a complete picture of the hardware and really can identify with
> > 100% certainty it really is the docks NIC.
> 
> I remember when I did the Dell implementation I tried userspace first.
> 
> Pushing this out to udev has a few other implications I remember hitting:
> 1) You need to also get the value you're supposed to use from ACPI BIOS
>      exported some way in userland too.
> 2) You can run into race conditions with other device or MAC renaming rules.
>     My first try I did it with NM and hit that continually.  So you would
> probably
>     need to land this in systemd or so.
> 
> >
> >    Andrew------Please consider the environment before printing this e-mail.