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

[Mario_Limonciello]

> -----Original Message-----
> From: Pali RohÃr [mailto:pali.rohar@xxxxxxxxx]
> Sent: Wednesday, May 11, 2016 11:42 AM
> To: MichaÅ KÄpieÅ <kernel@xxxxxxxxxx>; Limonciello, Mario
> <Mario_Limonciello@xxxxxxxx>
> Cc: Matthew Garrett <mjg59@xxxxxxxxxxxxx>; dvhart@xxxxxxxxxxxxx; LKML
> <linux-kernel@xxxxxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx
> 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.