RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

[Mario_Limonciello]

Thanks for the check.  Some small comments below, and I'll address in next submission.

> -----Original Message-----
> From: Pali RohÃr [mailto:pali.rohar@xxxxxxxxx]
> Sent: Thursday, June 2, 2016 2:47 AM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: hayeswang@xxxxxxxxxxx; LKML <linux-kernel@xxxxxxxxxxxxxxx>; Netdev
> <netdev@xxxxxxxxxxxxxxx>; Linux USB <linux-usb@xxxxxxxxxxxxxxx>;
> anthony.wong@xxxxxxxxxxxxx
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> Hi! As ACPI bytecode is untrusted for me and also for running kernel, we
> should not expect that it does not contain any bugs or other problems.
> So I would propose these checks to prevent something wrong...

OK

> 
> On Wednesday 01 June 2016 16:50:44 Mario Limonciello wrote:
> > +static void set_auxiliary_addr(struct sockaddr *sa) {
> > +	acpi_status status;
> > +	acpi_handle handle;
> > +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +	union acpi_object *obj;
> > +	int i;
> > +	char *ptr;
> > +
> > +	acpi_get_handle(NULL, "\\_SB", &handle);
> 
> Check return value of acpi_get_handle
> 
> > +	status = acpi_evaluate_object(handle, "AMAC", NULL, &buffer);
> 
> This is question for ACPI devs, it is not possible to call directly?
> 
>   acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);

I tried this and it works for me.  I'll take out the stuff related to making a handle in the next version.

> 
> And what happen if we try to evaluate objects which do not exist? Does not
> it show some warning or error in dmesg about non existent object?
> Such errors should be silent here.
> 

I tried this with an object I knew didn't exist (like \\_SB.AMACx) and there was no warning or error about non existent objects.

> > +	obj = (union acpi_object *)buffer.pointer;
> 
> Check buffer.type

OK.

> 
> > +	if (ACPI_SUCCESS(status) && (obj->string.length == 0x17)) {
> > +		/* returns _AUXMAC_#AABBCCDDEEFF#
> > +		 * this pulls out _AUXMAC# from start and # from end
> > +		 */
> > +		ptr = obj->string.pointer + 9;
> 
> Verify that string really contains that _AUXMAX# prefix. This is really obscure
> and nonstandard format for specifying MAC address and in my opinion it
> should be properly checked. Nonstandard formats can be changed in future
> and we could have problems.

OK.

> 
> > +		pr_info("r8152: Using system auxiliary MAC address");
> 
> It would be great to write also mac address into that pr_info

I was originally doing this before I submitted the patch for debugging but didn't think there was value since it could easily be looked up.  I'll add that back in.

> 
> > +		for (i = 0; i < 6; i++, ptr += 2)
> > +			sa->sa_data[i] = amac_ascii_to_hex(*ptr) << 4 |
> > +					 amac_ascii_to_hex(*(ptr + 1));
> > +	}
> 
> In case of some acpi check fails throw warning (or error).

OK.

> 
> And there is memory leak, you allocated buffer with
> ACPI_ALLOCATE_BUFFER but you did not free it.

Alright I'll make sure to cleanup properly.

> 
> > +}
> 
> And my last question is: Are really all Dell docks comes with this one realtek
> chip? I'm pessimist in this, because I see how other components (like HDD
> vendor, touchpad type, smardcard chips, motherboards, display panels, wifi
> chips) can be different in two laptops of same Dell model.

Indeed some of those things you mention in laptops are multiple sources or multiple options.  I can tell you that all the docks that are enabled with this technology (TB15, WD15, and type-C LAN dongle PN# 96NP5) are only using the r8152 driver.  There is always opportunity to change this in a future generation of docks or dongles.  Another reply had talked about moving this to a platform lookup and then using that platform lookup in r8152.  I think that's a good approach that future proofs choosing another vendor for future docks to prevent extra code duplication.  Of course this can also be bolted onto any other USB Ethernet dongle.  If there was desire to do this with other dongles it could be easily added.