Message-ID: <20160509121318.GB2875@eudyptula.hq.kempniu.pl>
Date:	Mon, 9 May 2016 14:13:18 +0200
From:	Michał Kępień <kernel@...pniu.pl>
To:	Mario Limonciello <mario_limonciello@...l.com>
Cc:	dvhart@...radead.org, LKML <linux-kernel@...r.kernel.org>,
	platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v2 2/2] dell-laptop: Expose auxiliary MAC address if
 available

> System with Type-C ports have a feature to expose an auxiliary
> persistent MAC address.  This address is burned in at the
> factory.
> 
> The intention of this address is to update the MAC address on
> Type-C docks containing an ethernet adapter to match the
> auxiliary address of the system connected to them.
> 
> Signed-off-by: Mario Limonciello <mario_limonciello@...l.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 66 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 2c2f02b..7a1fe08 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -87,6 +87,7 @@ static struct rfkill *wifi_rfkill;
>  static struct rfkill *bluetooth_rfkill;
>  static struct rfkill *wwan_rfkill;
>  static bool force_rfkill;
> +static char *auxiliary_mac_address;
>  
>  module_param(force_rfkill, bool, 0444);
>  MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> @@ -273,6 +274,59 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> +/* get_aux_mac
> + * returns the auxiliary mac address
> + * for assigning to a Type-C ethernet device
> + * such as that found in the Dell TB15 dock
> + */
> +static int get_aux_mac(void)
> +{
> +	struct calling_interface_buffer *buffer;
> +	int ret;
> +	unsigned char *address =
> +		(unsigned char *) __get_free_page(GFP_KERNEL | GFP_DMA32);
> +
> +	buffer = dell_smbios_get_buffer();
> +
> +	/* prepare a 17 byte buffer */
> +	dell_smbios_prepare_v2_call(address, 17);
> +	buffer->input[0] = virt_to_phys(address);
> +	dell_smbios_send_request(11, 6);

I guess this code could be made reusable by:

  * moving the pre-allocation to dell-smbios (so it would pre-allocate
    two pages, one for "normal" requests and one more for "complex"
    requests),
    
  * defining a new function, e.g. dell_smbios_send_v2_request(), taking
    an additional parameter for specifying the desired length of the
    "complex request" buffer, which would:
        
      o prepare the "complex request" buffer,
        
      o fill the SMI buffer with proper values (physical address of the
        "complex request" buffer as the first input argument),
        
      o perform the SMI request,
        
      o return a structure containing the length of the returned
        "complex" data and a pointer to that data (or perhaps even
        length immediately followed by data, so that the "complex
        request" buffer can simply be cast to that structure after the
        SMI is performed).

Such a function could then be used like this:

    struct calling_interface_extended_buffer *extbuffer;
    struct calling_interface_buffer *buffer;
    int ret;

    buffer = dell_smbios_get_buffer();
    extbuffer = dell_smbios_send_v2_request(11, 6, 17);
    ret = buffer->output[0];
    if (ret != 0) {
        ...
    }
    auxiliary_mac_address = kmalloc(extbuffer->length, GFP_KERNEL);
    memcpy(auxiliary_mac_address, extbuffer->data, extbuffer->length);
    dell_smbios_release_buffer();

This "complex request" buffer could then be freed in dell_smbios_exit(),
making caller cleanup a bit simpler.

I am also not sure about the "v2" part of the naming scheme, perhaps it
should be "extended" or "complex" instead?  Is it referred to as "v2" in
some internal Dell documentation?

> +	ret = buffer->output[0];
> +
> +	if (ret != 0) {

Nit: I would remove the empty line here, perhaps moving it one line
higher, i.e. below dell_smbios_send_request().

> +		auxiliary_mac_address = NULL;
> +		goto auxout;
> +	}
> +	dell_smbios_clear_buffer();

Why is this needed?

> +
> +	/* address will be stored in byte 4-> */
> +	auxiliary_mac_address = kmalloc(13, GFP_KERNEL);
> +	memcpy(auxiliary_mac_address, &address[4], 13);

As pointed out above, instead of hardcoding a constant value here, the
value returned in the first four bytes of the "complex request" buffer
should probably be used or at least checked.

> +
> + auxout:
> +	free_page((unsigned long)address);
> +	dell_smbios_release_buffer();
> +	return dell_smbios_error(ret);
> +
> +}
> +
> +static ssize_t auxiliary_mac_show(struct device *dev,
> +				  struct device_attribute *attr, char *page)
> +{
> +	return sprintf(page, "%s\n", auxiliary_mac_address);
> +}
> +
> +static DEVICE_ATTR_RO(auxiliary_mac);
> +static struct attribute *dell_attributes[] = {
> +	&dev_attr_auxiliary_mac.attr,
> +	NULL
> +};
> +static const struct attribute_group dell_attr_group = {
> +.attrs = dell_attributes,

While checkpatch doesn't complain here, an indent would ensure
consistency with the rest of dell-laptop.

> +};
> +
> +

If you run checkpatch with the --strict option, you will find that it
has some whitespace-related remarks here and in several other places.

>  /*
>   * Derived from information in smbios-wireless-ctl:
>   *
> @@ -392,7 +446,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>   *     cbArg1, byte0 = 0x13
>   *     cbRes1 Standard return codes (0, -1, -2)
>   */
> -

Is there any reason why this patch touches an unrelated empty line?

>  static int dell_rfkill_set(void *data, bool blocked)
>  {
>  	struct calling_interface_buffer *buffer;
> @@ -2003,6 +2056,12 @@ static int __init dell_init(void)
>  		goto fail_rfkill;
>  	}
>  
> +	ret = get_aux_mac();
> +	if (!ret) {
> +		sysfs_create_group(&platform_device->dev.kobj,
> +				   &dell_attr_group);
> +	}
> +
>  	if (quirks && quirks->touchpad_led)
>  		touchpad_led_init(&platform_device->dev);
>  
> @@ -2064,6 +2123,11 @@ fail_platform_driver:
>  
>  static void __exit dell_exit(void)
>  {
> +	if (auxiliary_mac_address)
> +		sysfs_remove_group(&platform_device->dev.kobj,
> +				  &dell_attr_group);
> +
> +	kfree(auxiliary_mac_address);

Nit, but it might be cleaner to only call kfree() if
auxiliary_mac_address is not NULL.

BTW, I sincerely hope this whole "complex request" thingy is also used
for something more useful than encoding a 6-byte MAC into a 13-byte
null-terminated string...

-- 
Best regards,
Michał Kępień

Powered by blists - more mailing lists