Date:	Wed, 11 May 2016 15:32:51 +0200
From:	Michał Kępień <kernel@...pniu.pl>
To:	Mario Limonciello <mario_limonciello@...l.com>
Cc:	Matthew Garrett <mjg59@...f.ucam.org>,
	Pali Rohár <pali.rohar@...il.com>,
	dvhart@...radead.org, LKML <linux-kernel@...r.kernel.org>,
	platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v3 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 | 61 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 2c2f02b..7d29690 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,54 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> +/* get_aux_mac

I'm not sure whether repeating the function's name in a comment placed
just above its definition is useful when not using kernel-doc.

CC'ing Matthew and Pali who are the maintainers of dell-laptop as this
boils down to their opinion (and you'll need their ack for the whole
patch anyway).  Please CC them for any upcoming revisions of this patch
series.

> + * returns the auxiliary mac address

get_aux_mac() doesn't return the auxiliary MAC address, it stores it in
a variable and returns an error code.  Please rephrase the comment to
avoid confusion.

> + * 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;
> +	unsigned char *extended_buffer;
> +	size_t length;
> +	int ret;
> +
> +	buffer = dell_smbios_get_buffer();
> +	length = 17;
> +	extended_buffer = dell_smbios_send_extended_request(11, 6, &length);
> +	ret = buffer->output[0];
> +	if (ret != 0 || !extended_buffer) {
> +		pr_debug("get_aux_mac: ext buffer: %p, len: %lu, ret: %d\n",
> +			 extended_buffer, length, ret);
> +		auxiliary_mac_address = NULL;

This is redundant as auxiliary_mac_address is static and thus will be
initialized to NULL.

> +		goto auxout;

I guess the label's name could be changed to "out" for consistency with
other functions in dell-laptop using only one goto label.

> +	}
> +
> +	/* address will be stored in byte 4-> */

This comment is now redundant.

> +	auxiliary_mac_address = kmalloc(length, GFP_KERNEL);
> +	memcpy(auxiliary_mac_address, extended_buffer, length);
> +
> + auxout:
> +	dell_smbios_release_buffer();
> +	return dell_smbios_error(ret);
> +}
> +
> +static ssize_t auxiliary_mac_show(struct device *dev,
> +				  struct device_attribute *attr, char *page)

Could you rename the variable "page" to "buf" for consistency with other
device attributes defined inside dell-laptop?

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

It seems to me that removing this unrelated empty line is something
close to your heart ;)

>  static int dell_rfkill_set(void *data, bool blocked)
>  {
>  	struct calling_interface_buffer *buffer;
> @@ -2003,6 +2051,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);
> +	}

The curly brackets are redundant here.

BTW, while this code will behave correctly when the requested length of
the extended buffer is 17, I cannot shake the premonition that bad
things will happen when someone copy-pastes your code for get_aux_mac()
while changing the requested length of the extended buffer to an invalid
value.  In such a case dell_smbios_send_extended_request() would return
NULL, but the return value from the copy-pasted sibling of get_aux_mac()
would be 0, because dell_smbios_get_buffer() zeroes the SMI buffer and
dell_smbios_send_extended_request() would return so early that it
wouldn't even call dell_smbios_send_request().  Therefore, "if (!ret)"
would evaluate to true, even though the copy-pasted sibling of
get_aux_mac() would have encountered an error.

Perhaps I'm being overly paranoid, but maybe it won't hurt to do the
following instead:

    get_aux_mac();
    if (auxiliary_mac_address)
        sysfs_create_group(&platform_device->dev.kobj,
                           &dell_attr_group);

and make get_aux_mac() return void.  What do you think?

-- 
Best regards,
Michał Kępień

Powered by blists - more mailing lists