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

[<Mario_Limonciello@Dell.com>]

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