Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1411591 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address Mario Limonciello <mario_limonciello@dell.com> - 2016-06-02 00:00 +0200
    Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address Andrew Lunn <andrew@lunn.ch> - 2016-06-02 00:30 +0200
      RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 00:40 +0200
        Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address Greg KH <gregkh@linuxfoundation.org> - 2016-06-02 01:10 +0200
          RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 04:00 +0200
    Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address Greg KH <gregkh@linuxfoundation.org> - 2016-06-02 01:10 +0200
      RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 04:20 +0200
        Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address Greg KH <gregkh@linuxfoundation.org> - 2016-06-02 17:30 +0200
          RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 17:50 +0200
            Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address Greg KH <gregkh@linuxfoundation.org> - 2016-06-02 18:10 +0200
              RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 19:00 +0200
                RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address Hayes Wang <hayeswang@realtek.com> - 2016-06-03 11:30 +0200
                  RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-03 17:00 +0200
            Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address Bjørn Mork <bjorn@mork.no> - 2016-06-02 20:10 +0200
              RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 20:30 +0200
                Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address Pali Rohár <pali.rohar@gmail.com> - 2016-06-02 21:10 +0200
                  RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 21:20 +0200
              Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address Pali Rohár <pali.rohar@gmail.com> - 2016-06-02 20:50 +0200
      RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 05:00 +0200
        Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address Bjørn Mork <bjorn@mork.no> - 2016-06-02 10:20 +0200
          RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 16:30 +0200
      Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address Bjørn Mork <bjorn@mork.no> - 2016-06-02 09:30 +0200
    RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address Hayes Wang <hayeswang@realtek.com> - 2016-06-02 08:20 +0200
    Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address Pali Rohár <pali.rohar@gmail.com> - 2016-06-02 09:50 +0200
      RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 16:50 +0200
        RE: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address <Mario_Limonciello@Dell.com> - 2016-06-02 17:10 +0200
        Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary  MAC address Andrew Lunn <andrew@lunn.ch> - 2016-06-02 17:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

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

> -----Original Message-----
> From: Bjørn Mork [mailto:bjorn@mork.no]
> Sent: Thursday, June 2, 2016 3:11 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: gregkh@linuxfoundation.org; hayeswang@realtek.com; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; linux-
> usb@vger.kernel.org; pali.rohar@gmail.com; anthony.wong@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> <Mario_Limonciello@Dell.com> writes:
> 
> >> > +static u8 amac_ascii_to_hex(int c) {
> >> > +	if (c <= 0x39)
> >> > +		return (u8)(c - 0x30);
> >> > +	else if (c <= 0x46)
> >> > +		return (u8)(c - 0x37);
> >> > +	return (u8)(c - 0x57);
> >> > +}
> >>
> >
> > Sorry forgot to address this.
> >
> >> We really don't have such a function somewhere in the kernel already?
> >
> > There is a function in acpi/acpica/uthex.c that does this, but it
> > doesn't seem to be used by anything outside of acpica so far.  Would it be
> OK style wise to #include " ../../acpi/acpica/acutils.h" from r8152.c?
> 
> Makes me wonder where you looked....  You have hex_to_bin() and
> hex2bin() in include/linux/kernel.h
> 


Thank you, I completely missed that.  I wasn't looking for literals in my grepping.  I'll use this instead.

> You could look at usbnet_get_ethernet_addr() for an example of how to do
> this properly.  It's pretty close to this driver in the tree, and should be a
> natural starting point before reinventing the wheel...
> 

OK will do.

[toc] | [prev] | [next] | [standalone]

#1411936

Greg KH <gregkh@linuxfoundation.org> writes:

> And finally, this seems odd overall given that a MAC address should be
> associated with the specific network device, not the overall system.

Definitely.

I wonder if this isn't a perfect candidate for an x86
arch_get_platform_mac_address() implementation?  Then you could just use
the eth_platform_get_mac_address() helper in the driver and avoid any
platform specific code there.  Which will automagically make it work as
expected on a Sparc too :)

This will also make your job next year much easier, when the hardeware
guys decided to replace the chip and you need to implement the exact
same code in some other driver...

See https://patchwork.ozlabs.org/patch/564100/ for a detailed
discussion of this interface.


Bjørn

[toc] | [prev] | [next] | [standalone]

#1411864

Mario Limonciello [mailto:mario_limonciello@dell.com]
[...]
>  static int set_ethernet_addr(struct r8152 *tp)
>  {
>  	struct net_device *dev = tp->netdev;
> @@ -1041,6 +1075,9 @@ static int set_ethernet_addr(struct r8152 *tp)
>  	else
>  		ret = pla_ocp_read(tp, PLA_BACKUP, 8, sa.sa_data);
> 
> +	/* if system provides auxiliary MAC address */
> +	set_auxiliary_addr(&sa);
> +
>  	if (ret < 0) {
>  		netif_err(tp, probe, dev, "Get ether addr fail\n");
>  	} else if (!is_valid_ether_addr(sa.sa_data)) {

When tp->version == RTL_VER_01, you would have different MAC address
between SW and HW. You may use the MAC address from ACPI for dev_addr.
However, the device uses another one, because you don't set it to the device.

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1411950 — 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...

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);

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.

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

Check buffer.type

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

> +		pr_info("r8152: Using system auxiliary MAC address");

It would be great to write also mac address into that pr_info

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

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

> +}

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.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

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

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

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Thursday, June 2, 2016 2:47 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: hayeswang@realtek.com; LKML <linux-kernel@vger.kernel.org>; Netdev
> <netdev@vger.kernel.org>; Linux USB <linux-usb@vger.kernel.org>;
> anthony.wong@canonical.com
> 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.

[toc] | [prev] | [next] | [standalone]

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

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Thursday, June 2, 2016 10:01 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: pali.rohar@gmail.com; hayeswang@realtek.com; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; linux-
> usb@vger.kernel.org; anthony.wong@canonical.com
> Subject: Re: [PATCH] r8152: Add support for setting MAC to system's
> Auxiliary MAC address
> 
> > >
> > > > +		pr_info("r8152: Using system auxiliary MAC address");
> > >
> > > It would be great to write also mac address into that pr_info
> 
> And since there could be multiple r8152 in the system, it would be good to
> indicate which of them is having its MAC changed. So
> netdev_info() or dev_info().
> 
>       Andrew

Thanks, will do that in next submission too.

[toc] | [prev] | [next] | [standalone]

#1412335 — Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

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

And since there could be multiple r8152 in the system, it would be
good to indicate which of them is having its MAC changed. So
netdev_info() or dev_info().

      Andrew

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web