From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753476AbcFGH7k (ORCPT ); Tue, 7 Jun 2016 03:59:40 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:33149 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750881AbcFGH7j (ORCPT ); Tue, 7 Jun 2016 03:59:39 -0400 Date: Tue, 7 Jun 2016 09:59:34 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= To: Mario Limonciello Cc: hayeswang@realtek.com, LKML , Netdev , Linux USB , anthony.wong@canonical.com, Greg KH Subject: Re: [PATCH v4] r8152: Add support for setting pass through MAC address on RTL8153-AD Message-ID: <20160607075934.GY29844@pali> References: <1465233569-13585-1-git-send-email-mario_limonciello@dell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1465233569-13585-1-git-send-email-mario_limonciello@dell.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi! Below are my comments, all about coding style. On Monday 06 June 2016 12:19:29 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. For consistency it would be great to use same ACPI name in whole patch: \_SB.AMAC > 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 > --- > Changes from v3: > * Add additional comments about functions and what they're doing > * Adjust warning calls to use netif instead > * Rename function > > drivers/net/usb/r8152.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 68 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index 3f9f6ed..b2339d3 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > > /* 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,59 @@ 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 get_vendor_mac_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]; > + > + /* 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); What about? if (!ACPI_SUCCESS(status)) return -ENODEV; You will save one level of indentation. > + obj = (union acpi_object *)buffer.pointer; > + if (ACPI_SUCCESS(status)) { > + if (obj->type != ACPI_TYPE_BUFFER || > + obj->string.length != 0x17) { > + netif_warn(tp, probe, tp->netdev, "Invalid buffer\n"); I would suggest more specific warning messages. These are very general and if I would see them in dmesg log I would have no idea what that means. > + goto amacout; > + } > + if (strncmp(obj->string.pointer, "_AUXMAC_#", 9) != 0) { > + netif_warn(tp, probe, tp->netdev, "Invalid header\n"); > + goto amacout; > + } If your specification state that last character is '#' then I think you should check it too. > + ret = hex2bin(buf, obj->string.pointer + 9, 6); > + if (ret < 0 || !is_valid_ether_addr(buf)) { > + netif_warn(tp, probe, tp->netdev, "Invalid MAC\n"); > + 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-through MAC addr %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 +1097,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 = get_vendor_mac_passthru_addr(tp, &sa); You do not "get" (return) any mac address. Personally I would use "read" word in function name (like above pla_ocp_read). What about? ret = vendor_mac_passthru_addr_read(tp, sa.sa_data); Or something similar... "Get" looks like function "get" something, but instead it set address in &sa structure. > + 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"); -- Pali Rohár pali.rohar@gmail.com