[PATCH v3 1/2] dell-smbios: Add a method for more complex SMI requests

[PATCH v3 1/2] dell-smbios: Add a method for more complex SMI requests

[Mario Limonciello]

More complex SMI requests can return data that exceeds the 4 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 new method 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 | 43 ++++++++++++++++++++++++++++++++++++++
 drivers/platform/x86/dell-smbios.h |  2 ++
 2 files changed, 45 insertions(+)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index d2412ab..e3bbbb3 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -33,12 +33,14 @@ struct calling_interface_structure {
 } __packed;
 
 static struct calling_interface_buffer *buffer;
+static unsigned char *extended_buffer;
 static DEFINE_MUTEX(buffer_mutex);
 
 static int da_command_address;
 static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
+static const char extended_key[4] = {'D', 'S', 'C', 'I'};
 
 int dell_smbios_error(int value)
 {
@@ -92,6 +94,38 @@ 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
+ *
+ * The return value is the data of the extended buffer request
+ * The value of length will be updated to the length of the actual
+ * buffer content
+ *
+ */
+unsigned char *dell_smbios_send_extended_request(int class, int select,
+						 size_t *length)
+{
+	u32 i;
+	u32 *buffer_length = (u32 *)extended_buffer;
+
+	if (*length < 5 || *length - 4 > PAGE_SIZE)
+		return NULL;
+
+	*buffer_length = *length - 4;
+	for (i = 4; i < *length; i += 4)
+		if (*length - i > 4)
+			memcpy(&extended_buffer[i], &extended_key, 4);
+
+	*length = buffer_length[0];
+	buffer->input[0] = virt_to_phys(extended_buffer);
+	dell_smbios_send_request(class, select);
+
+	return &extended_buffer[4];
+}
+EXPORT_SYMBOL_GPL(dell_smbios_send_extended_request);
+
 struct calling_interface_token *dell_smbios_find_token(int tokenid)
 {
 	int i;
@@ -170,8 +204,16 @@ static int __init dell_smbios_init(void)
 		goto fail_buffer;
 	}
 
+	extended_buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
+	if (!extended_buffer) {
+		ret = -ENOMEM;
+		goto fail_extended_buffer;
+	}
+
 	return 0;
 
+fail_extended_buffer:
+	kfree(buffer);
 fail_buffer:
 	kfree(da_tokens);
 	return ret;
@@ -181,6 +223,7 @@ static void __exit dell_smbios_exit(void)
 {
 	kfree(da_tokens);
 	free_page((unsigned long)buffer);
+	free_page((unsigned long)extended_buffer);
 }
 
 subsys_initcall(dell_smbios_init);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index ec7d40a..c0f4395 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -41,6 +41,8 @@ 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);
+unsigned char *dell_smbios_send_extended_request(int class, int select,
+						 size_t *length);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid);
 #endif
-- 
2.7.4

[PATCH v3 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 | 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
+ * 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;
+	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;
+		goto auxout;
+	}
+
+	/* address will be stored in byte 4-> */
+	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)
+{
+	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)
  */
-
 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);
+	}
+
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_init(&platform_device->dev);
 
@@ -2064,6 +2118,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 v3 1/2] dell-smbios: Add a method for more complex SMI requests

[Michał Kępień]

> More complex SMI requests can return data that exceeds the 4 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 new method prepares the buffer as the BIOS will look for
> it in these requests.

I guess rephrasing this sentence might improve clarity, e.g.:

    This patch adds a new method which prepares the buffer as required
    by the BIOS and then issues the specified SMI request.

> Signed-off-by: Mario Limonciello <mario_limonciello@dell.com>
> ---
>  drivers/platform/x86/dell-smbios.c | 43 ++++++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/dell-smbios.h |  2 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index d2412ab..e3bbbb3 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -33,12 +33,14 @@ struct calling_interface_structure {
>  } __packed;
>  
>  static struct calling_interface_buffer *buffer;
> +static unsigned char *extended_buffer;
>  static DEFINE_MUTEX(buffer_mutex);
>  
>  static int da_command_address;
>  static int da_command_code;
>  static int da_num_tokens;
>  static struct calling_interface_token *da_tokens;
> +static const char extended_key[4] = {'D', 'S', 'C', 'I'};

Is there any reason why you preferred declaring a char array over using
a string literal in the memcpy() call below?

>  
>  int dell_smbios_error(int value)
>  {
> @@ -92,6 +94,38 @@ 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
> + *
> + * The return value is the data of the extended buffer request
> + * The value of length will be updated to the length of the actual
> + * buffer content
> + *
> + */

Nit: could you please format this comment so that it resembles the rest
of the kernel in terms of whitespace placement, i.e.:

 -> /*
     * More complex requests are served by sending
    ...
     * The value of length will be updated to the length of the actual
     * buffer content
 ->  */

> +unsigned char *dell_smbios_send_extended_request(int class, int select,
> +						 size_t *length)

Nice touch with length being an in/out parameter, I like it.

> +{
> +	u32 i;
> +	u32 *buffer_length = (u32 *)extended_buffer;

Please swap these two lines so that local variable declarations are in
"Reverse Christmas Tree" order.

> +
> +	if (*length < 5 || *length - 4 > PAGE_SIZE)
> +		return NULL;
> +
> +	*buffer_length = *length - 4;
> +	for (i = 4; i < *length; i += 4)
> +		if (*length - i > 4)
> +			memcpy(&extended_buffer[i], &extended_key, 4);

I guess a comment just before the for loop might be useful for
documenting the format expected by the BIOS, e.g.:

    /*
     * BIOS will reject the buffer with error code -5 if it does not
     * contain repeating "DSCI" strings from byte 4 onward
     */

> +
> +	*length = buffer_length[0];

This assignment should be moved below the dell_smbios_send_request()
call as at this point the BIOS hasn't yet had a chance to update the
buffer.

By the way, I think it would be nice to use the dereference operator
here instead of the subscript operator for consistency with a similar
assignment 4 lines earlier.

> +	buffer->input[0] = virt_to_phys(extended_buffer);
> +	dell_smbios_send_request(class, select);
> +
> +	return &extended_buffer[4];
> +}
> +EXPORT_SYMBOL_GPL(dell_smbios_send_extended_request);
> +
>  struct calling_interface_token *dell_smbios_find_token(int tokenid)
>  {
>  	int i;
> @@ -170,8 +204,16 @@ static int __init dell_smbios_init(void)
>  		goto fail_buffer;
>  	}
>  
> +	extended_buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
> +	if (!extended_buffer) {
> +		ret = -ENOMEM;
> +		goto fail_extended_buffer;
> +	}
> +
>  	return 0;
>  
> +fail_extended_buffer:
> +	kfree(buffer);

Please use free_page() instead, as in dell_smbios_exit().

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 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 | 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ń

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 6501 bytes --]

Hi Mario & Michał!

On Wednesday 11 May 2016 15:32:51 Michał Kępień wrote:
> > 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.

So... if I understand patch description correctly, it means that new 
notebooks could have reserved MAC address used by dock, right?

And USB Type-C docks with ethernet port act like USB ethernet card, 
right?

So notebook has burned MAC address which should be used for any 
connected dock (usb ethernet) into C port, instead MAC address burned 
into ethernet card?

If I'm not right, please describe me how it should work, as I'm not sure 
if I understand it correctly...

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

Yes, that comment does not bring any value for reader.

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

I'm not on platform-driver-x86 ML, so please CC me explicitly if you 
want from me to comment patches.

> > + * 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,
> > +};
> > +

In my opinion this is not correct way to export "random" sysfs 
attributes to userspace. If it is possible we should use existing 
API/ABI for kernel and not to invent new ABI for userspace.

Dell-laptop driver has already documented ABI for non standard things 
(like extended settings for keyboard backlight), see: 
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-platform-dell-laptop

And exporting MAC address sounds for me like bad idea. I think it should 
handle kernel itself (maybe send it to driver which use it?).

And most important question: Who is going to use that sysfs file? Is 
there any application? It is not possible to query for that address from 
userspace, e.g. from libsmbios tools?

We have libsmbios functionality in kernel just for things which have 
exiting API/ABI/interface in kernel. Not those which do not have...

So why is needed to have such sysfs attribute exported by kernel?

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

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Mario_Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, May 11, 2016 11:42 AM
> To: Michał Kępień <kernel@kempniu.pl>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>; dvhart@infradead.org; LKML
> <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if
> available
> 
> Hi Mario & Michał!
> 
> On Wednesday 11 May 2016 15:32:51 Michał Kępień wrote:
> > > 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.
> 
> So... if I understand patch description correctly, it means that new notebooks
> could have reserved MAC address used by dock, right?
> 
> And USB Type-C docks with ethernet port act like USB ethernet card, right?
> 
> So notebook has burned MAC address which should be used for any
> connected dock (usb ethernet) into C port, instead MAC address burned into
> ethernet card?
> 
> If I'm not right, please describe me how it should work, as I'm not sure if I
> understand it correctly...

Yep, that's spot on.

> > > +/* 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.
> 
> Yes, that comment does not bring any value for reader.
> 
> > 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.
> 
> I'm not on platform-driver-x86 ML, so please CC me explicitly if you want
> from me to comment patches.
> 
> > > + * 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,
> > > +};
> > > +
> 
> In my opinion this is not correct way to export "random" sysfs attributes to
> userspace. If it is possible we should use existing API/ABI for kernel and not
> to invent new ABI for userspace.
> 
> Dell-laptop driver has already documented ABI for non standard things (like
> extended settings for keyboard backlight), see:
> https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-platform-
> dell-laptop
> 
> And exporting MAC address sounds for me like bad idea. I think it should
> handle kernel itself (maybe send it to driver which use it?).
> 
> And most important question: Who is going to use that sysfs file? Is there any
> application? It is not possible to query for that address from userspace, e.g.
> from libsmbios tools?
> 
> We have libsmbios functionality in kernel just for things which have exiting
> API/ABI/interface in kernel. Not those which do not have...
> 
> So why is needed to have such sysfs attribute exported by kernel?
> 

I skipped your other comments because of this one.  My original plan for this was to do it in udev or Network Manager but you raise a good point. 
Maybe this patch should be scrapped all together.  

Mical - if you think patch 1/2 could still be useful to have as a general interface I'll update it for your other comments and get it resubmitted.

We do mirror the information in ACPI under the system bus:

    Scope (_SB)
    {
        Name (AMAC, Buffer (0x17)
        {
            "_AUXMAC_#847BEB5992D2#"
        })
    }

I don't know how to properly access this from the kernel side.  I noticed that most drivers that reference ACPI nodes refer to devices, not something hanging off the system bus. 
If you could advise the right way to go about that, I would appreciate it.

If I can access that, maybe it's better to do this directly as a patch to the Ethernet driver in question (r8152).  
That's actually how it's handled on the OS side for Windows too from what I understand.
We have some FW bit set in them to indicate they're Dell Realtek products (don't have this detail yet).
When they see that bit they look for that ACPI buffer and use it to set the MAC address the OS sees.

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

[Pali Rohár]

On Wednesday 11 May 2016 22:41:16 Mario_Limonciello@Dell.com wrote:
> > > > +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,
> > > > +};
> > > > +
> > 
> > In my opinion this is not correct way to export "random" sysfs attributes to
> > userspace. If it is possible we should use existing API/ABI for kernel and not
> > to invent new ABI for userspace.
> > 
> > Dell-laptop driver has already documented ABI for non standard things (like
> > extended settings for keyboard backlight), see:
> > https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-platform-
> > dell-laptop
> > 
> > And exporting MAC address sounds for me like bad idea. I think it should
> > handle kernel itself (maybe send it to driver which use it?).
> > 
> > And most important question: Who is going to use that sysfs file? Is there any
> > application? It is not possible to query for that address from userspace, e.g.
> > from libsmbios tools?
> > 
> > We have libsmbios functionality in kernel just for things which have exiting
> > API/ABI/interface in kernel. Not those which do not have...
> > 
> > So why is needed to have such sysfs attribute exported by kernel?
> > 
> 
> I skipped your other comments because of this one.  My original plan for this was to do it in udev or Network Manager but you raise a good point. 
> Maybe this patch should be scrapped all together.  
> 
> Mical - if you think patch 1/2 could still be useful to have as a general interface I'll update it for your other comments and get it resubmitted.

What is first patch doing? Can you send me link to it?

> We do mirror the information in ACPI under the system bus:
> 
>     Scope (_SB)
>     {
>         Name (AMAC, Buffer (0x17)
>         {
>             "_AUXMAC_#847BEB5992D2#"
>         })
>     }
> 
> I don't know how to properly access this from the kernel side.  I noticed that most drivers that reference ACPI nodes refer to devices, not something hanging off the system bus. 
> If you could advise the right way to go about that, I would appreciate it.

So there are two ways how to read that MAC address. One is via SMM and
one via ACPI.

You can also read ACPI buffer (name is probably \_SB.AMAC) with ACPI
functions in kernel. Ask ACPI people, for correct API. I'm sure this is
possible also without creating new ACPI driver...

> If I can access that, maybe it's better to do this directly as a patch to the Ethernet driver in question (r8152).  
> That's actually how it's handled on the OS side for Windows too from what I understand.
> We have some FW bit set in them to indicate they're Dell Realtek products (don't have this detail yet).
> When they see that bit they look for that ACPI buffer and use it to set the MAC address the OS sees.

Maybe it should be better to chose same way as Windows drivers? Better
ask on netdev mailing list and ping maintainers of that ethernet driver
what they think about it.

For me it sounds like a better solution (patching that ethernet driver)
as exporting some non-standard sysfs node from kernel with MAC address
and then using another tool which send that MAC address back to kernel.

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Michał Kępień]

> Mical - if you think patch 1/2 could still be useful to have as a general interface I'll update it for your other comments and get it resubmitted.

If there is no practical use for it today, then let's call it quits for
now.  Once the need emerges, you can simply pick up where you left off.

> We do mirror the information in ACPI under the system bus:
> 
>     Scope (_SB)
>     {
>         Name (AMAC, Buffer (0x17)
>         {
>             "_AUXMAC_#847BEB5992D2#"
>         })
>     }
> 
> I don't know how to properly access this from the kernel side.  I noticed that most drivers that reference ACPI nodes refer to devices, not something hanging off the system bus. 
> If you could advise the right way to go about that, I would appreciate it.
> 
> If I can access that, maybe it's better to do this directly as a patch to the Ethernet driver in question (r8152).  

As Pali said, this sounds like the cleaner option.

-- 
Best regards,
Michał Kępień

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

[Mario_Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Thursday, May 12, 2016 3:41 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: kernel@kempniu.pl; mjg59@srcf.ucam.org; dvhart@infradead.org; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if
> available
> 
> On Wednesday 11 May 2016 22:41:16 Mario_Limonciello@Dell.com wrote:
> > > > > +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,
> > > > > +};
> > > > > +
> > >
> > > In my opinion this is not correct way to export "random" sysfs
> > > attributes to userspace. If it is possible we should use existing
> > > API/ABI for kernel and not to invent new ABI for userspace.
> > >
> > > Dell-laptop driver has already documented ABI for non standard
> > > things (like extended settings for keyboard backlight), see:
> > > https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-
> platform-
> > > dell-laptop
> > >
> > > And exporting MAC address sounds for me like bad idea. I think it
> > > should handle kernel itself (maybe send it to driver which use it?).
> > >
> > > And most important question: Who is going to use that sysfs file? Is
> > > there any application? It is not possible to query for that address from
> userspace, e.g.
> > > from libsmbios tools?
> > >
> > > We have libsmbios functionality in kernel just for things which have
> > > exiting API/ABI/interface in kernel. Not those which do not have...
> > >
> > > So why is needed to have such sysfs attribute exported by kernel?
> > >
> >
> > I skipped your other comments because of this one.  My original plan for
> this was to do it in udev or Network Manager but you raise a good point.
> > Maybe this patch should be scrapped all together.
> >
> > Mical - if you think patch 1/2 could still be useful to have as a general
> interface I'll update it for your other comments and get it resubmitted.
> 
> What is first patch doing? Can you send me link to it?
> 
> > We do mirror the information in ACPI under the system bus:
> >
> >     Scope (_SB)
> >     {
> >         Name (AMAC, Buffer (0x17)
> >         {
> >             "_AUXMAC_#847BEB5992D2#"
> >         })
> >     }
> >
> > I don't know how to properly access this from the kernel side.  I noticed
> that most drivers that reference ACPI nodes refer to devices, not something
> hanging off the system bus.
> > If you could advise the right way to go about that, I would appreciate it.
> 
> So there are two ways how to read that MAC address. One is via SMM and
> one via ACPI.

Yes, this isn't a general statement for read only static information, but in this case it is true.

> You can also read ACPI buffer (name is probably \_SB.AMAC) with ACPI
> functions in kernel. Ask ACPI people, for correct API. I'm sure this is possible
> also without creating new ACPI driver...

Thanks will do.

> 
> > If I can access that, maybe it's better to do this directly as a patch to the
> Ethernet driver in question (r8152).
> > That's actually how it's handled on the OS side for Windows too from what I
> understand.
> > We have some FW bit set in them to indicate they're Dell Realtek products
> (don't have this detail yet).
> > When they see that bit they look for that ACPI buffer and use it to set the
> MAC address the OS sees.
> 
> Maybe it should be better to chose same way as Windows drivers? Better
> ask on netdev mailing list and ping maintainers of that ethernet driver what
> they think about it.
> 
> For me it sounds like a better solution (patching that ethernet driver) as
> exporting some non-standard sysfs node from kernel with MAC address and
> then using another tool which send that MAC address back to kernel.
> 

Great, thank you for your feedback.  I'll wander down that rabbit hole.
 

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

[Pali Rohár]

On Thursday 12 May 2016 19:08:30 Mario_Limonciello@Dell.com wrote:
> > > We do mirror the information in ACPI under the system bus:
> > >
> > >     Scope (_SB)
> > >     {
> > >         Name (AMAC, Buffer (0x17)
> > >         {
> > >             "_AUXMAC_#847BEB5992D2#"
> > >         })
> > >     }
> > >
> > > I don't know how to properly access this from the kernel side.  I noticed
> > that most drivers that reference ACPI nodes refer to devices, not something
> > hanging off the system bus.
> > > If you could advise the right way to go about that, I would appreciate it.
> > 
> > So there are two ways how to read that MAC address. One is via SMM and
> > one via ACPI.
> 
> Yes, this isn't a general statement for read only static information, but in this case it is true.
> 
> > You can also read ACPI buffer (name is probably \_SB.AMAC) with ACPI
> > functions in kernel. Ask ACPI people, for correct API. I'm sure this is possible
> > also without creating new ACPI driver...
> 
> Thanks will do.

I think that acpi_get_handle() and acpi_evaluate_object() methods are
those which you want to use.

> > > If I can access that, maybe it's better to do this directly as a patch to the
> > Ethernet driver in question (r8152).
> > > That's actually how it's handled on the OS side for Windows too from what I
> > understand.
> > > We have some FW bit set in them to indicate they're Dell Realtek products
> > (don't have this detail yet).
> > > When they see that bit they look for that ACPI buffer and use it to set the
> > MAC address the OS sees.
> > 
> > Maybe it should be better to chose same way as Windows drivers? Better
> > ask on netdev mailing list and ping maintainers of that ethernet driver what
> > they think about it.
> > 
> > For me it sounds like a better solution (patching that ethernet driver) as
> > exporting some non-standard sysfs node from kernel with MAC address and
> > then using another tool which send that MAC address back to kernel.
> > 
> 
> Great, thank you for your feedback.  I'll wander down that rabbit hole.

Ok, CC me next discussion.

-- 
Pali Rohár
pali.rohar@gmail.com