lkml.org 
[lkml]   [2016]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v4] r8152: Add support for setting pass through MAC address on RTL8153-AD
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 <mario_limonciello@dell.com>
> ---
> 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 <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,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

\
 
 \ /
  Last update: 2016-06-07 10:21    [W:0.124 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site