[PATCH v2 1/2] dell-smbios: Add a helper function for complex SMI requests

[PATCH v2 1/2] dell-smbios: Add a helper function for complex SMI requests

[Mario Limonciello]

More complex SMI requests can return data that exceeds the 4 32 byte
arguments that are traditionally part of a request.

To support more complex requests, the first input argument can be a
32 bit physical address with a buffer properly built in advance.

This helper function prepares the buffer as the BIOS will look for
it in these requests.

Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
---
 drivers/platform/x86/dell-smbios.c | 28 ++++++++++++++++++++++++++++
 drivers/platform/x86/dell-smbios.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index d2412ab..00b29df 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -92,6 +92,34 @@ void dell_smbios_send_request(int class, int select)
 }
 EXPORT_SYMBOL_GPL(dell_smbios_send_request);
 
+/* More complex requests are served by sending
+ * a pointer to a pre-allocated buffer
+ * Bytes 0:3 are the size of the return value
+ * Bytes 4:length are the returned value
+ *
+ * This helper function prepares it properly
+ * with the size length to be provided
+ *
+ * caller still needs to pre-allocate and clear
+ * the input buffer
+ */
+void dell_smbios_prepare_v2_call(u8 *input, size_t length)
+{
+	u32 i;
+	u32 *data = (u32 *) input;
+
+	data[0] = length - 4;
+	for (i = 4; i < length; i += 4) {
+		if (length - i > 4) {
+			input[i]   = 'D';
+			input[i+1] = 'S';
+			input[i+2] = 'C';
+			input[i+3] = 'I';
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(dell_smbios_prepare_v2_call);
+
 struct calling_interface_token *dell_smbios_find_token(int tokenid)
 {
 	int i;
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index ec7d40a..5d4184c 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -41,6 +41,7 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void);
 void dell_smbios_clear_buffer(void);
 void dell_smbios_release_buffer(void);
 void dell_smbios_send_request(int class, int select);
+void dell_smbios_prepare_v2_call(u8 *input, size_t length);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid);
 #endif
-- 
2.7.4

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

[Mario Limonciello]

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);
+	ret = buffer->output[0];
+
+	if (ret != 0) {
+		auxiliary_mac_address = NULL;
+		goto auxout;
+	}
+	dell_smbios_clear_buffer();
+
+	/* address will be stored in byte 4-> */
+	auxiliary_mac_address = kmalloc(13, GFP_KERNEL);
+	memcpy(auxiliary_mac_address, &address[4], 13);
+
+ 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,
+};
+
+
 /*
  * 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)
  */
-
 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);
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
-- 
2.7.4

Re: [PATCH v2 1/2] dell-smbios: Add a helper function for complex SMI requests

[Michał Kępień]

> More complex SMI requests can return data that exceeds the 4 32 byte

32-bit

> arguments that are traditionally part of a request.
> 
> To support more complex requests, the first input argument can be a
> 32 bit physical address with a buffer properly built in advance.
> 
> This helper function prepares the buffer as the BIOS will look for
> it in these requests.
> 
> Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
> ---
>  drivers/platform/x86/dell-smbios.c | 28 ++++++++++++++++++++++++++++
>  drivers/platform/x86/dell-smbios.h |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index d2412ab..00b29df 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -92,6 +92,34 @@ void dell_smbios_send_request(int class, int select)
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_send_request);
>  
> +/* More complex requests are served by sending
> + * a pointer to a pre-allocated buffer
> + * Bytes 0:3 are the size of the return value
> + * Bytes 4:length are the returned value
> + *
> + * This helper function prepares it properly
> + * with the size length to be provided
> + *
> + * caller still needs to pre-allocate and clear
> + * the input buffer

Which input buffer, the original one (struct calling_interface_buffer)
or the one for "complex requests"?

If the former, pre-allocation is done by dell_smbios_init() while
dell_smbios_get_buffer() clears the buffer before use, so the comment
above may be misleading.

If the latter, then the caller in patch 2/2 doesn't clear the input
buffer, it just allocates it using __get_free_page().

> + */
> +void dell_smbios_prepare_v2_call(u8 *input, size_t length)
> +{
> +	u32 i;
> +	u32 *data = (u32 *) input;
> +
> +	data[0] = length - 4;
> +	for (i = 4; i < length; i += 4) {
> +		if (length - i > 4) {
> +			input[i]   = 'D';
> +			input[i+1] = 'S';
> +			input[i+2] = 'C';
> +			input[i+3] = 'I';

memcpy(&input[i], "DSCI", 4) or something along those lines?

Anyway, I'm confused about filling the buffer with repeating "DSCI"
strings.  The buffer's length is provided explicitly in its first four
bytes.  Some bytes at the end will always be left uninitialized.  So why
bother initializing anything besides length at all?

> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(dell_smbios_prepare_v2_call);
> +
>  struct calling_interface_token *dell_smbios_find_token(int tokenid)
>  {
>  	int i;
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> index ec7d40a..5d4184c 100644
> --- a/drivers/platform/x86/dell-smbios.h
> +++ b/drivers/platform/x86/dell-smbios.h
> @@ -41,6 +41,7 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void);
>  void dell_smbios_clear_buffer(void);
>  void dell_smbios_release_buffer(void);
>  void dell_smbios_send_request(int class, int select);
> +void dell_smbios_prepare_v2_call(u8 *input, size_t length);

I will send my remarks regarding the proposed changes to dell-smbios API
in response to patch 2/2.

-- 
Best regards,
Michał Kępień

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

[Michał Kępień]

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

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ń

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

[Mario_Limonciello]

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

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

[Mario_Limonciello]

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

RE: [PATCH v2 1/2] dell-smbios: Add a helper function for complex SMI requests

[Mario_Limonciello]

> -----Original Message-----
> From: Michał Kępień [mailto:kernel@kempniu.pl]
> Sent: Monday, May 9, 2016 7:02 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 1/2] dell-smbios: Add a helper function for complex
> SMI requests
> 
> > More complex SMI requests can return data that exceeds the 4 32 byte
> 
> 32-bit

Ack, thanks.

> 
> > arguments that are traditionally part of a request.
> >
> > To support more complex requests, the first input argument can be a
> > 32 bit physical address with a buffer properly built in advance.
> >
> > This helper function prepares the buffer as the BIOS will look for it
> > in these requests.
> >
> > Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
> > ---
> >  drivers/platform/x86/dell-smbios.c | 28
> ++++++++++++++++++++++++++++
> > drivers/platform/x86/dell-smbios.h |  1 +
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/platform/x86/dell-smbios.c
> > b/drivers/platform/x86/dell-smbios.c
> > index d2412ab..00b29df 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > @@ -92,6 +92,34 @@ void dell_smbios_send_request(int class, int
> > select)  }  EXPORT_SYMBOL_GPL(dell_smbios_send_request);
> >
> > +/* More complex requests are served by sending
> > + * a pointer to a pre-allocated buffer
> > + * Bytes 0:3 are the size of the return value
> > + * Bytes 4:length are the returned value
> > + *
> > + * This helper function prepares it properly
> > + * with the size length to be provided
> > + *
> > + * caller still needs to pre-allocate and clear
> > + * the input buffer
> 
> Which input buffer, the original one (struct calling_interface_buffer) or the
> one for "complex requests"?
> 
> If the former, pre-allocation is done by dell_smbios_init() while
> dell_smbios_get_buffer() clears the buffer before use, so the comment
> above may be misleading.
> 
> If the latter, then the caller in patch 2/2 doesn't clear the input buffer, it just
> allocates it using __get_free_page().

Yeah this is a comment mistake.  I was meaning for  the caller to just allocated it, but as your other comment noted it's better for code re-use to have this happen in dell-smbios, so I'll try to work towards that goal in next patch attempt.

> 
> > + */
> > +void dell_smbios_prepare_v2_call(u8 *input, size_t length) {
> > +	u32 i;
> > +	u32 *data = (u32 *) input;
> > +
> > +	data[0] = length - 4;
> > +	for (i = 4; i < length; i += 4) {
> > +		if (length - i > 4) {
> > +			input[i]   = 'D';
> > +			input[i+1] = 'S';
> > +			input[i+2] = 'C';
> > +			input[i+3] = 'I';
> 
> memcpy(&input[i], "DSCI", 4) or something along those lines?
> 

Yeah that's cleaner, I'll use that, thanks.

> Anyway, I'm confused about filling the buffer with repeating "DSCI"
> strings.  The buffer's length is provided explicitly in its first four bytes.  Some
> bytes at the end will always be left uninitialized.  So why bother initializing
> anything besides length at all?
> 

The reason for any of this in the first place was some situations where memory was getting clobbered because it wasn't properly allocated by users of this interface.  So the determination was to have a the 32 bit unsigned length the first 4 bytes and then repeat that string on 4 byte alignment for the rest of the buffer.  Anything not aligned on 4 bytes isn't checked by the SMM, so it's not important to initialize.

The SMM will check the buffer for that repeating string and reject (error -5) if it's not built properly.  

> > +		}
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(dell_smbios_prepare_v2_call);
> > +
> >  struct calling_interface_token *dell_smbios_find_token(int tokenid)
> > {
> >  	int i;
> > diff --git a/drivers/platform/x86/dell-smbios.h
> > b/drivers/platform/x86/dell-smbios.h
> > index ec7d40a..5d4184c 100644
> > --- a/drivers/platform/x86/dell-smbios.h
> > +++ b/drivers/platform/x86/dell-smbios.h
> > @@ -41,6 +41,7 @@ struct calling_interface_buffer
> > *dell_smbios_get_buffer(void);  void dell_smbios_clear_buffer(void);
> > void dell_smbios_release_buffer(void);  void
> > dell_smbios_send_request(int class, int select);
> > +void dell_smbios_prepare_v2_call(u8 *input, size_t length);
> 
> I will send my remarks regarding the proposed changes to dell-smbios API in
> response to patch 2/2.
> 
> --
> Best regards,
> Michał Kępień

RE: [PATCH v2 1/2] dell-smbios: Add a helper function for complex SMI requests

[Mario_Limonciello]

> -----Original Message-----
> From: Michał Kępień [mailto:kernel@kempniu.pl]
> Sent: Monday, May 9, 2016 7:02 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 1/2] dell-smbios: Add a helper function for complex
> SMI requests
> 
> > More complex SMI requests can return data that exceeds the 4 32 byte
> 
> 32-bit

Ack, thanks.

> 
> > arguments that are traditionally part of a request.
> >
> > To support more complex requests, the first input argument can be a
> > 32 bit physical address with a buffer properly built in advance.
> >
> > This helper function prepares the buffer as the BIOS will look for it
> > in these requests.
> >
> > Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
> > ---
> >  drivers/platform/x86/dell-smbios.c | 28
> ++++++++++++++++++++++++++++
> > drivers/platform/x86/dell-smbios.h |  1 +
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/platform/x86/dell-smbios.c
> > b/drivers/platform/x86/dell-smbios.c
> > index d2412ab..00b29df 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > @@ -92,6 +92,34 @@ void dell_smbios_send_request(int class, int
> > select)  }  EXPORT_SYMBOL_GPL(dell_smbios_send_request);
> >
> > +/* More complex requests are served by sending
> > + * a pointer to a pre-allocated buffer
> > + * Bytes 0:3 are the size of the return value
> > + * Bytes 4:length are the returned value
> > + *
> > + * This helper function prepares it properly
> > + * with the size length to be provided
> > + *
> > + * caller still needs to pre-allocate and clear
> > + * the input buffer
> 
> Which input buffer, the original one (struct calling_interface_buffer) or the
> one for "complex requests"?
> 
> If the former, pre-allocation is done by dell_smbios_init() while
> dell_smbios_get_buffer() clears the buffer before use, so the comment
> above may be misleading.
> 
> If the latter, then the caller in patch 2/2 doesn't clear the input buffer, it just
> allocates it using __get_free_page().

Yeah this is a comment mistake.  I was meaning for  the caller to just allocated it, but as your other comment noted it's better for code re-use to have this happen in dell-smbios, so I'll try to work towards that goal in next patch attempt.

> 
> > + */
> > +void dell_smbios_prepare_v2_call(u8 *input, size_t length) {
> > +	u32 i;
> > +	u32 *data = (u32 *) input;
> > +
> > +	data[0] = length - 4;
> > +	for (i = 4; i < length; i += 4) {
> > +		if (length - i > 4) {
> > +			input[i]   = 'D';
> > +			input[i+1] = 'S';
> > +			input[i+2] = 'C';
> > +			input[i+3] = 'I';
> 
> memcpy(&input[i], "DSCI", 4) or something along those lines?
> 

Yeah that's cleaner, I'll use that, thanks.

> Anyway, I'm confused about filling the buffer with repeating "DSCI"
> strings.  The buffer's length is provided explicitly in its first four bytes.  Some
> bytes at the end will always be left uninitialized.  So why bother initializing
> anything besides length at all?
> 

The reason for any of this in the first place was some situations where memory was getting clobbered because it wasn't properly allocated by users of this interface.  So the determination was to have a the 32 bit unsigned length the first 4 bytes and then repeat that string on 4 byte alignment for the rest of the buffer.  Anything not aligned on 4 bytes isn't checked by the SMM, so it's not important to initialize.

The SMM will check the buffer for that repeating string and reject (error -5) if it's not built properly.  

> > +		}
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(dell_smbios_prepare_v2_call);
> > +
> >  struct calling_interface_token *dell_smbios_find_token(int tokenid)
> > {
> >  	int i;
> > diff --git a/drivers/platform/x86/dell-smbios.h
> > b/drivers/platform/x86/dell-smbios.h
> > index ec7d40a..5d4184c 100644
> > --- a/drivers/platform/x86/dell-smbios.h
> > +++ b/drivers/platform/x86/dell-smbios.h
> > @@ -41,6 +41,7 @@ struct calling_interface_buffer
> > *dell_smbios_get_buffer(void);  void dell_smbios_clear_buffer(void);
> > void dell_smbios_release_buffer(void);  void
> > dell_smbios_send_request(int class, int select);
> > +void dell_smbios_prepare_v2_call(u8 *input, size_t length);
> 
> I will send my remarks regarding the proposed changes to dell-smbios API in
> response to patch 2/2.
> 
> --
> Best regards,
> Michał Kępień