Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3601.0.10\))
Subject: Re: [PATCH v2] r8152: Add macpassthru support for ThinkPad
 Thunderbolt 3 Dock Gen 2
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB2F18F4E9E@RTITMBSVM03.realtek.com.tw>
Date:   Tue, 5 Nov 2019 19:18:04 +0800
Cc:     "davem@davemloft.net" <davem@davemloft.net>,
        "oliver@neukum.org" <oliver@neukum.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: 8BIT
Message-Id: <193EF03A-1EF7-4604-BF3A-61201A78D724@canonical.com>
References: <20191105081526.4206-1-kai.heng.feng@canonical.com>
 <0835B3720019904CB8F7AA43166CEEB2F18F4E9E@RTITMBSVM03.realtek.com.tw>
To:     Hayes Wang <hayeswang@realtek.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Nov 5, 2019, at 16:55, Hayes Wang <hayeswang@realtek.com> wrote:
> 
> Kai-Heng Feng [mailto:kai.heng.feng@canonical.com]
>> Sent: Tuesday, November 05, 2019 4:15 PM
>> To: davem@davemloft.net; oliver@neukum.org
> [...]
>> +	if (test_bit(LENOVO_MACPASSTHRU, &tp->flags)) {
>> +		bypass_test = true;
>> +		mac_obj_name = "\\MACA";
>> +		mac_obj_type = ACPI_TYPE_STRING;
>> +		mac_strlen = 0x16;
>> 	} 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;
>> +		bypass_test = false;
>> +		mac_obj_name = "\\_SB.AMAC";
>> +		mac_obj_type = ACPI_TYPE_BUFFER;
>> +		mac_strlen = 0x17;
>> +	}
>> +
>> +	if (!bypass_test) {
> 
> Maybe you could combine this with the "else" above.
> Then, the variable "bypass_test" could be removed.

Ok, will do in V3.

> And the declaration of "ocp_data" could be moved after the "else".

Isn't putting declarations at the top of the function the preferred way?

Kai-Heng

> 
>> +		/* 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;
>> +			}
>> 		}
>> 	}
>> 
>> 	/* returns _AUXMAC_#AABBCCDDEEFF# */
>> -	status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);
>> +	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 != ACPI_TYPE_BUFFER || obj->string.length != 0x17) {
>> +	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,
>> @@ -6629,6 +6649,10 @@ static int rtl8152_probe(struct usb_interface *intf,
>> 		netdev->hw_features &= ~NETIF_F_RXCSUM;
>> 	}
>> 
>> +	if (le16_to_cpu(udev->descriptor.idVendor) == VENDOR_ID_LENOVO &&
>> +	    le16_to_cpu(udev->descriptor.idProduct) == 0x3082)
>> +		set_bit(LENOVO_MACPASSTHRU, &tp->flags);
>> +
>> 	if (le16_to_cpu(udev->descriptor.bcdDevice) == 0x3011 && udev->serial
>> &&
>> 	    (!strcmp(udev->serial, "000001000000") ||
>> 	     !strcmp(udev->serial, "000002000000"))) {
>> @@ -6755,6 +6779,7 @@ static const struct usb_device_id rtl8152_table[] = {
>> 	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x304f)},
>> 	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x3062)},
>> 	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x3069)},
>> +	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x3082)},
>> 	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7205)},
>> 	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x720c)},
>> 	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7214)},
>> --
>> 2.17.1
> 
> 
> Best Regards,
> Hayes