Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1397165 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if available Mario Limonciello <mario_limonciello@dell.com> - 2016-05-09 18:30 +0200
    Re: [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if  available Michał Kępień <kernel@kempniu.pl> - 2016-05-11 15:40 +0200
      Re: [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if available Pali Rohár <pali.rohar@gmail.com> - 2016-05-11 18:50 +0200
        RE: [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if  available <Mario_Limonciello@Dell.com> - 2016-05-12 00:50 +0200
          Re: [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if  available Pali Rohár <pali.rohar@gmail.com> - 2016-05-12 10:50 +0200
            RE: [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if  available <Mario_Limonciello@Dell.com> - 2016-05-12 21:10 +0200
              Re: [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if  available Pali Rohár <pali.rohar@gmail.com> - 2016-05-20 16:30 +0200
          Re: [PATCH v3 2/2] dell-laptop: Expose auxiliary MAC address if  available Michał Kępień <kernel@kempniu.pl> - 2016-05-12 13:40 +0200

#1397165 — [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@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

[toc] | [next] | [standalone]

#1399082 — 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@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ń

[toc] | [prev] | [next] | [standalone]

#1399353

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1399761 — 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.

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web