| Message ID | 1462811099-16897-2-git-send-email-mario_limonciello@dell.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
> 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?
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?
> -----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.
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.
> 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.
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogUGFsaSBSb2jDoXIgW21h aWx0bzpwYWxpLnJvaGFyQGdtYWlsLmNvbV0NCj4gU2VudDogVGh1cnNkYXksIE1heSAxMiwgMjAx NiAzOjQxIEFNDQo+IFRvOiBMaW1vbmNpZWxsbywgTWFyaW8gPE1hcmlvX0xpbW9uY2llbGxvQERl bGwuY29tPg0KPiBDYzoga2VybmVsQGtlbXBuaXUucGw7IG1qZzU5QHNyY2YudWNhbS5vcmc7IGR2 aGFydEBpbmZyYWRlYWQub3JnOyBsaW51eC0NCj4ga2VybmVsQHZnZXIua2VybmVsLm9yZzsgcGxh dGZvcm0tZHJpdmVyLXg4NkB2Z2VyLmtlcm5lbC5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSCB2 MyAyLzJdIGRlbGwtbGFwdG9wOiBFeHBvc2UgYXV4aWxpYXJ5IE1BQyBhZGRyZXNzIGlmDQo+IGF2 YWlsYWJsZQ0KPiANCj4gT24gV2VkbmVzZGF5IDExIE1heSAyMDE2IDIyOjQxOjE2IE1hcmlvX0xp bW9uY2llbGxvQERlbGwuY29tIHdyb3RlOg0KPiA+ID4gPiA+ICtzdGF0aWMgREVWSUNFX0FUVFJf Uk8oYXV4aWxpYXJ5X21hYyk7IHN0YXRpYyBzdHJ1Y3QgYXR0cmlidXRlDQo+ID4gPiA+ID4gKypk ZWxsX2F0dHJpYnV0ZXNbXSA9IHsNCj4gPiA+ID4gPiArCSZkZXZfYXR0cl9hdXhpbGlhcnlfbWFj LmF0dHIsDQo+ID4gPiA+ID4gKwlOVUxMDQo+ID4gPiA+ID4gK307DQo+ID4gPiA+ID4gKw0KPiA+ ID4gPiA+ICtzdGF0aWMgY29uc3Qgc3RydWN0IGF0dHJpYnV0ZV9ncm91cCBkZWxsX2F0dHJfZ3Jv dXAgPSB7DQo+ID4gPiA+ID4gKwkuYXR0cnMgPSBkZWxsX2F0dHJpYnV0ZXMsDQo+ID4gPiA+ID4g K307DQo+ID4gPiA+ID4gKw0KPiA+ID4NCj4gPiA+IEluIG15IG9waW5pb24gdGhpcyBpcyBub3Qg Y29ycmVjdCB3YXkgdG8gZXhwb3J0ICJyYW5kb20iIHN5c2ZzDQo+ID4gPiBhdHRyaWJ1dGVzIHRv IHVzZXJzcGFjZS4gSWYgaXQgaXMgcG9zc2libGUgd2Ugc2hvdWxkIHVzZSBleGlzdGluZw0KPiA+ ID4gQVBJL0FCSSBmb3Iga2VybmVsIGFuZCBub3QgdG8gaW52ZW50IG5ldyBBQkkgZm9yIHVzZXJz cGFjZS4NCj4gPiA+DQo+ID4gPiBEZWxsLWxhcHRvcCBkcml2ZXIgaGFzIGFscmVhZHkgZG9jdW1l bnRlZCBBQkkgZm9yIG5vbiBzdGFuZGFyZA0KPiA+ID4gdGhpbmdzIChsaWtlIGV4dGVuZGVkIHNl dHRpbmdzIGZvciBrZXlib2FyZCBiYWNrbGlnaHQpLCBzZWU6DQo+ID4gPiBodHRwczovL3d3dy5r ZXJuZWwub3JnL2RvYy9Eb2N1bWVudGF0aW9uL0FCSS90ZXN0aW5nL3N5c2ZzLQ0KPiBwbGF0Zm9y bS0NCj4gPiA+IGRlbGwtbGFwdG9wDQo+ID4gPg0KPiA+ID4gQW5kIGV4cG9ydGluZyBNQUMgYWRk cmVzcyBzb3VuZHMgZm9yIG1lIGxpa2UgYmFkIGlkZWEuIEkgdGhpbmsgaXQNCj4gPiA+IHNob3Vs ZCBoYW5kbGUga2VybmVsIGl0c2VsZiAobWF5YmUgc2VuZCBpdCB0byBkcml2ZXIgd2hpY2ggdXNl IGl0PykuDQo+ID4gPg0KPiA+ID4gQW5kIG1vc3QgaW1wb3J0YW50IHF1ZXN0aW9uOiBXaG8gaXMg Z29pbmcgdG8gdXNlIHRoYXQgc3lzZnMgZmlsZT8gSXMNCj4gPiA+IHRoZXJlIGFueSBhcHBsaWNh dGlvbj8gSXQgaXMgbm90IHBvc3NpYmxlIHRvIHF1ZXJ5IGZvciB0aGF0IGFkZHJlc3MgZnJvbQ0K PiB1c2Vyc3BhY2UsIGUuZy4NCj4gPiA+IGZyb20gbGlic21iaW9zIHRvb2xzPw0KPiA+ID4NCj4g PiA+IFdlIGhhdmUgbGlic21iaW9zIGZ1bmN0aW9uYWxpdHkgaW4ga2VybmVsIGp1c3QgZm9yIHRo aW5ncyB3aGljaCBoYXZlDQo+ID4gPiBleGl0aW5nIEFQSS9BQkkvaW50ZXJmYWNlIGluIGtlcm5l bC4gTm90IHRob3NlIHdoaWNoIGRvIG5vdCBoYXZlLi4uDQo+ID4gPg0KPiA+ID4gU28gd2h5IGlz IG5lZWRlZCB0byBoYXZlIHN1Y2ggc3lzZnMgYXR0cmlidXRlIGV4cG9ydGVkIGJ5IGtlcm5lbD8N Cj4gPiA+DQo+ID4NCj4gPiBJIHNraXBwZWQgeW91ciBvdGhlciBjb21tZW50cyBiZWNhdXNlIG9m IHRoaXMgb25lLiAgTXkgb3JpZ2luYWwgcGxhbiBmb3INCj4gdGhpcyB3YXMgdG8gZG8gaXQgaW4g dWRldiBvciBOZXR3b3JrIE1hbmFnZXIgYnV0IHlvdSByYWlzZSBhIGdvb2QgcG9pbnQuDQo+ID4g TWF5YmUgdGhpcyBwYXRjaCBzaG91bGQgYmUgc2NyYXBwZWQgYWxsIHRvZ2V0aGVyLg0KPiA+DQo+ ID4gTWljYWwgLSBpZiB5b3UgdGhpbmsgcGF0Y2ggMS8yIGNvdWxkIHN0aWxsIGJlIHVzZWZ1bCB0 byBoYXZlIGFzIGEgZ2VuZXJhbA0KPiBpbnRlcmZhY2UgSSdsbCB1cGRhdGUgaXQgZm9yIHlvdXIg b3RoZXIgY29tbWVudHMgYW5kIGdldCBpdCByZXN1Ym1pdHRlZC4NCj4gDQo+IFdoYXQgaXMgZmly c3QgcGF0Y2ggZG9pbmc/IENhbiB5b3Ugc2VuZCBtZSBsaW5rIHRvIGl0Pw0KPiANCj4gPiBXZSBk byBtaXJyb3IgdGhlIGluZm9ybWF0aW9uIGluIEFDUEkgdW5kZXIgdGhlIHN5c3RlbSBidXM6DQo+ ID4NCj4gPiAgICAgU2NvcGUgKF9TQikNCj4gPiAgICAgew0KPiA+ICAgICAgICAgTmFtZSAoQU1B QywgQnVmZmVyICgweDE3KQ0KPiA+ICAgICAgICAgew0KPiA+ICAgICAgICAgICAgICJfQVVYTUFD XyM4NDdCRUI1OTkyRDIjIg0KPiA+ICAgICAgICAgfSkNCj4gPiAgICAgfQ0KPiA+DQo+ID4gSSBk b24ndCBrbm93IGhvdyB0byBwcm9wZXJseSBhY2Nlc3MgdGhpcyBmcm9tIHRoZSBrZXJuZWwgc2lk ZS4gIEkgbm90aWNlZA0KPiB0aGF0IG1vc3QgZHJpdmVycyB0aGF0IHJlZmVyZW5jZSBBQ1BJIG5v ZGVzIHJlZmVyIHRvIGRldmljZXMsIG5vdCBzb21ldGhpbmcNCj4gaGFuZ2luZyBvZmYgdGhlIHN5 c3RlbSBidXMuDQo+ID4gSWYgeW91IGNvdWxkIGFkdmlzZSB0aGUgcmlnaHQgd2F5IHRvIGdvIGFi b3V0IHRoYXQsIEkgd291bGQgYXBwcmVjaWF0ZSBpdC4NCj4gDQo+IFNvIHRoZXJlIGFyZSB0d28g d2F5cyBob3cgdG8gcmVhZCB0aGF0IE1BQyBhZGRyZXNzLiBPbmUgaXMgdmlhIFNNTSBhbmQNCj4g b25lIHZpYSBBQ1BJLg0KDQpZZXMsIHRoaXMgaXNuJ3QgYSBnZW5lcmFsIHN0YXRlbWVudCBmb3Ig cmVhZCBvbmx5IHN0YXRpYyBpbmZvcm1hdGlvbiwgYnV0IGluIHRoaXMgY2FzZSBpdCBpcyB0cnVl Lg0KDQo+IFlvdSBjYW4gYWxzbyByZWFkIEFDUEkgYnVmZmVyIChuYW1lIGlzIHByb2JhYmx5IFxf U0IuQU1BQykgd2l0aCBBQ1BJDQo+IGZ1bmN0aW9ucyBpbiBrZXJuZWwuIEFzayBBQ1BJIHBlb3Bs ZSwgZm9yIGNvcnJlY3QgQVBJLiBJJ20gc3VyZSB0aGlzIGlzIHBvc3NpYmxlDQo+IGFsc28gd2l0 aG91dCBjcmVhdGluZyBuZXcgQUNQSSBkcml2ZXIuLi4NCg0KVGhhbmtzIHdpbGwgZG8uDQoNCj4g DQo+ID4gSWYgSSBjYW4gYWNjZXNzIHRoYXQsIG1heWJlIGl0J3MgYmV0dGVyIHRvIGRvIHRoaXMg ZGlyZWN0bHkgYXMgYSBwYXRjaCB0byB0aGUNCj4gRXRoZXJuZXQgZHJpdmVyIGluIHF1ZXN0aW9u IChyODE1MikuDQo+ID4gVGhhdCdzIGFjdHVhbGx5IGhvdyBpdCdzIGhhbmRsZWQgb24gdGhlIE9T IHNpZGUgZm9yIFdpbmRvd3MgdG9vIGZyb20gd2hhdCBJDQo+IHVuZGVyc3RhbmQuDQo+ID4gV2Ug aGF2ZSBzb21lIEZXIGJpdCBzZXQgaW4gdGhlbSB0byBpbmRpY2F0ZSB0aGV5J3JlIERlbGwgUmVh bHRlayBwcm9kdWN0cw0KPiAoZG9uJ3QgaGF2ZSB0aGlzIGRldGFpbCB5ZXQpLg0KPiA+IFdoZW4g dGhleSBzZWUgdGhhdCBiaXQgdGhleSBsb29rIGZvciB0aGF0IEFDUEkgYnVmZmVyIGFuZCB1c2Ug aXQgdG8gc2V0IHRoZQ0KPiBNQUMgYWRkcmVzcyB0aGUgT1Mgc2Vlcy4NCj4gDQo+IE1heWJlIGl0 IHNob3VsZCBiZSBiZXR0ZXIgdG8gY2hvc2Ugc2FtZSB3YXkgYXMgV2luZG93cyBkcml2ZXJzPyBC ZXR0ZXINCj4gYXNrIG9uIG5ldGRldiBtYWlsaW5nIGxpc3QgYW5kIHBpbmcgbWFpbnRhaW5lcnMg b2YgdGhhdCBldGhlcm5ldCBkcml2ZXIgd2hhdA0KPiB0aGV5IHRoaW5rIGFib3V0IGl0Lg0KPiAN Cj4gRm9yIG1lIGl0IHNvdW5kcyBsaWtlIGEgYmV0dGVyIHNvbHV0aW9uIChwYXRjaGluZyB0aGF0 IGV0aGVybmV0IGRyaXZlcikgYXMNCj4gZXhwb3J0aW5nIHNvbWUgbm9uLXN0YW5kYXJkIHN5c2Zz IG5vZGUgZnJvbSBrZXJuZWwgd2l0aCBNQUMgYWRkcmVzcyBhbmQNCj4gdGhlbiB1c2luZyBhbm90 aGVyIHRvb2wgd2hpY2ggc2VuZCB0aGF0IE1BQyBhZGRyZXNzIGJhY2sgdG8ga2VybmVsLg0KPiAN Cg0KR3JlYXQsIHRoYW5rIHlvdSBmb3IgeW91ciBmZWVkYmFjay4gIEknbGwgd2FuZGVyIGRvd24g dGhhdCByYWJiaXQgaG9sZS4NCiANCg0K -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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();
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(-)