Date:	Thu, 12 May 2016 10:40:33 +0200
From:	Pali Rohár <pali.rohar@...il.com>
To:	Mario_Limonciello@...l.com
Cc:	kernel@...pniu.pl, mjg59@...f.ucam.org, dvhart@...radead.org,
	linux-kernel@...r.kernel.org, platform-driver-x86@...r.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@...l.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@...il.com

Powered by blists - more mailing lists