Message-Id: <201605111841.54411@pali>
Date:	Wed, 11 May 2016 18:41:54 +0200
From:	Pali Rohár <pali.rohar@...il.com>
To:	Michał Kępień <kernel@...pniu.pl>,
	Mario Limonciello <mario_limonciello@...l.com>
Cc:	Matthew Garrett <mjg59@...f.ucam.org>, dvhart@...radead.org,
	LKML <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

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@...il.com

Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)

Powered by blists - more mailing lists