RE: [PATCH v2 2/2] dell-laptop: Expose auxiliary MAC address if available

[<Mario_Limonciello@Dell.com>]

> -----Original Message-----
> From: Michał Kępień [mailto:kernel@kempniu.pl]
> Sent: Monday, May 9, 2016 7:13 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; LKML <linux-kernel@vger.kernel.org>; platform-
> driver-x86@vger.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@dell.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.
> 

This proposal sounds great, thanks.  I'll adjust it in the next revision.

> 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?

Yes it's referring to something in internal documentation.  I'm fine with just having it be something like "extended request".  I'll make that noted in the next change set.

> 
> > +	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?
> 

The extended buffer address is only used as an argument to the calling_interface_buffer, so in order to prevent problems from other calls having unexpected data it should be cleared out.  

I guess your argument is dell_smbios_get_buffer () already calls dell_smbios_clear_buffer() everywhere so this is pointless.

> > +
> > +	/* 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.

OK.

> 
> > +
> > + 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.

OK.

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

OK.


> 
> >  /*
> >   * 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?

No, it was probably a mistake on my part.  I'll double check that none of that is in v3.

> 
> >  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.

OK.

> 
> 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...
> 

Yes, it's used for any request that needs to provide larger amounts of data.  Some of them aren't too important because they can be obtained other methods (for example Service tag can be obtained via DMI's serial # field or via an SMM call).
I can't release the full spec, but if you have some pet requests you'd like to know if it's possible to get information I can check through and let you know the appropriate call that matches up to them.

Thanks,