diff mbox series

drivers/net/usb/r8152: Enable MAC address passthru support

Message ID 20220628015325.1204234-1-franklin_lin@wistron.corp-partner.google.com (mailing list archive)
State New, archived
Headers show
Series drivers/net/usb/r8152: Enable MAC address passthru support | expand

Commit Message

Franklin Lin June 28, 2022, 1:53 a.m. UTC 
From: franklin_lin <franklin_lin@wistron.corp-partner.google.com>

Enable the support for providing a MAC address
for a dock to use based on the VPD values set in the platform.

Signed-off-by: franklin_lin <franklin_lin@wistron.corp-partner.google.com>
---
 drivers/net/usb/r8152.c | 49 ++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 13 deletions(-)

Comments

Greg KH June 28, 2022, 5:41 a.m. UTC | #1 
On Tue, Jun 28, 2022 at 09:53:25AM +0800, Franklin Lin wrote:
> From: franklin_lin <franklin_lin@wistron.corp-partner.google.com>
> 
> Enable the support for providing a MAC address
> for a dock to use based on the VPD values set in the platform.
> 
> Signed-off-by: franklin_lin <franklin_lin@wistron.corp-partner.google.com>

Please use your name "Franklin Lin", and the corp-partner.google.com
email addresses are not "real" addresses.  Please use your normal
corporate one instead.

thanks,

greg k-h
Greg KH June 28, 2022, 5:42 a.m. UTC | #2 
On Tue, Jun 28, 2022 at 09:53:25AM +0800, Franklin Lin wrote:
> From: franklin_lin <franklin_lin@wistron.corp-partner.google.com>
> 
> Enable the support for providing a MAC address
> for a dock to use based on the VPD values set in the platform.
> 
> Signed-off-by: franklin_lin <franklin_lin@wistron.corp-partner.google.com>
> ---
>  drivers/net/usb/r8152.c | 49 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 7389d6ef8..732e48d99 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -3,6 +3,7 @@
>   *  Copyright (c) 2014 Realtek Semiconductor Corp. All rights reserved.
>   */
>  
> +#include <linux/fs.h>
>  #include <linux/signal.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> @@ -1608,6 +1609,11 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
>  	acpi_object_type mac_obj_type;
>  	int mac_strlen;
>  
> +	struct file *fp;
> +	unsigned char read_buf[32];
> +	loff_t f_pos = 0;
> +	int i, j, len;
> +
>  	if (tp->lenovo_macpassthru) {
>  		mac_obj_name = "\\MACA";
>  		mac_obj_type = ACPI_TYPE_STRING;
> @@ -1641,22 +1647,39 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
>  	/* returns _AUXMAC_#AABBCCDDEEFF# */
>  	status = acpi_evaluate_object(NULL, mac_obj_name, NULL, &buffer);
>  	obj = (union acpi_object *)buffer.pointer;
> -	if (!ACPI_SUCCESS(status))
> -		return -ENODEV;
> -	if (obj->type != mac_obj_type || obj->string.length != mac_strlen) {
> -		netif_warn(tp, probe, tp->netdev,
> +	if (ACPI_SUCCESS(status)) {
> +		if (obj->type != mac_obj_type || obj->string.length != mac_strlen) {
> +			netif_warn(tp, probe, tp->netdev,
>  			   "Invalid buffer for 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;
> +			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);
> +	} else {
> +		/* read from "/sys/firmware/vpd/ro/dock_mac" */
> +		fp = filp_open("/sys/firmware/vpd/ro/dock_mac", O_RDONLY, 0);

Woah, what?  No, that's not how firmware works at all, sorry.  Please
use the correct firmware interface if this really is firmware.  If it is
not, please use the correct networking interface instead.

you should NEVER read from a file from a driver, that is a sure sign
something is really really wrong.

thanks,

greg k-h
Andrew Lunn June 28, 2022, 10:17 a.m. UTC | #3 
On Tue, Jun 28, 2022 at 09:53:25AM +0800, Franklin Lin wrote:
> From: franklin_lin <franklin_lin@wistron.corp-partner.google.com>
> 
> Enable the support for providing a MAC address
> for a dock to use based on the VPD values set in the platform.

Maybe i'm missing it, but i don't see any code which verifies this
r8152 is actually in the dock, and not a USB ethernet dongle plugged
into some port of either the laptop or the dock itself.

     Andrew
Oliver Neukum June 28, 2022, 11:26 a.m. UTC | #4 
On 28.06.22 12:17, Andrew Lunn wrote:
> On Tue, Jun 28, 2022 at 09:53:25AM +0800, Franklin Lin wrote:
>> From: franklin_lin <franklin_lin@wistron.corp-partner.google.com>
>>
>> Enable the support for providing a MAC address
>> for a dock to use based on the VPD values set in the platform.
> 
> Maybe i'm missing it, but i don't see any code which verifies this
> r8152 is actually in the dock, and not a USB ethernet dongle plugged
> into some port of either the laptop or the dock itself.

And it will happily assign the same MAC to multiple devices.
I am afraid this patch needs some basic redesign.

	Regards
		Oliver
diff mbox series

Patch

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 7389d6ef8..732e48d99 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3,6 +3,7 @@ 
  *  Copyright (c) 2014 Realtek Semiconductor Corp. All rights reserved.
  */
 
+#include <linux/fs.h>
 #include <linux/signal.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -1608,6 +1609,11 @@  static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 	acpi_object_type mac_obj_type;
 	int mac_strlen;
 
+	struct file *fp;
+	unsigned char read_buf[32];
+	loff_t f_pos = 0;
+	int i, j, len;
+
 	if (tp->lenovo_macpassthru) {
 		mac_obj_name = "\\MACA";
 		mac_obj_type = ACPI_TYPE_STRING;
@@ -1641,22 +1647,39 @@  static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
 	/* returns _AUXMAC_#AABBCCDDEEFF# */
 	status = acpi_evaluate_object(NULL, mac_obj_name, NULL, &buffer);
 	obj = (union acpi_object *)buffer.pointer;
-	if (!ACPI_SUCCESS(status))
-		return -ENODEV;
-	if (obj->type != mac_obj_type || obj->string.length != mac_strlen) {
-		netif_warn(tp, probe, tp->netdev,
+	if (ACPI_SUCCESS(status)) {
+		if (obj->type != mac_obj_type || obj->string.length != mac_strlen) {
+			netif_warn(tp, probe, tp->netdev,
 			   "Invalid buffer for 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;
+			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);
+	} else {
+		/* read from "/sys/firmware/vpd/ro/dock_mac" */
+		fp = filp_open("/sys/firmware/vpd/ro/dock_mac", O_RDONLY, 0);
+		if (IS_ERR(fp))
+			return -ENOENT;
+		kernel_read(fp, read_buf, 32, &f_pos);
+		len = strlen(read_buf);
+		/* remove ':' form mac address string */
+		for (i = 0; i < len; i++) {
+			if (read_buf[i] == ':') {
+				for (j = i; j < len; j++)
+					read_buf[j] = read_buf[j+1];
+				len--;
+				i--;
+			}
+		}
+		filp_close(fp, NULL);
+		ret = hex2bin(buf, read_buf, 6);
 	}
-	ret = hex2bin(buf, obj->string.pointer + 9, 6);
 	if (!(ret == 0 && is_valid_ether_addr(buf))) {
 		netif_warn(tp, probe, tp->netdev,
 			   "Invalid MAC for pass-thru MAC addr: %d, %pM\n",