[PATCH] r8169: Load MAC address from device tree if present

[PATCH] r8169: Load MAC address from device tree if present

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

If the system was booted using a device tree and if the device tree
contains a MAC address, use it instead of reading one from the EEPROM.
This is useful in situations where the EEPROM isn't properly programmed
or where the firmware wants to override the existing MAC address.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Based on net-next.

 drivers/net/ethernet/realtek/r8169.c | 35 +++++++++++++++++-----------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f574b6b557f9..fd9edd643ca5 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6957,6 +6957,21 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
 	return pci_alloc_irq_vectors(tp->pci_dev, 1, 1, flags);
 }
 
+static void rtl_read_mac_address(struct rtl8169_private *tp,
+				 u8 mac_addr[ETH_ALEN])
+{
+	/* Get MAC address */
+	switch (tp->mac_version) {
+	case RTL_GIGA_MAC_VER_35 ... RTL_GIGA_MAC_VER_38:
+	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
+		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
+		*(u16 *)&mac_addr[4] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
+		break;
+	default:
+		break;
+	}
+}
+
 DECLARE_RTL_COND(rtl_link_list_ready_cond)
 {
 	return RTL_R8(tp, MCU) & LINK_LIST_RDY;
@@ -7148,6 +7163,7 @@ static int rtl_get_ether_clk(struct rtl8169_private *tp)
 static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
+	u8 mac_addr[ETH_ALEN] __aligned(4);
 	struct rtl8169_private *tp;
 	struct net_device *dev;
 	int chipset, region, i;
@@ -7252,20 +7268,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	u64_stats_init(&tp->rx_stats.syncp);
 	u64_stats_init(&tp->tx_stats.syncp);
 
-	/* Get MAC address */
-	switch (tp->mac_version) {
-		u8 mac_addr[ETH_ALEN] __aligned(4);
-	case RTL_GIGA_MAC_VER_35 ... RTL_GIGA_MAC_VER_38:
-	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
-		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
-		*(u16 *)&mac_addr[4] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
+	/* get MAC address */
+	if (eth_platform_get_mac_address(&pdev->dev, mac_addr))
+		rtl_read_mac_address(tp, mac_addr);
+
+	if (is_valid_ether_addr(mac_addr))
+		rtl_rar_set(tp, mac_addr);
 
-		if (is_valid_ether_addr(mac_addr))
-			rtl_rar_set(tp, mac_addr);
-		break;
-	default:
-		break;
-	}
 	for (i = 0; i < ETH_ALEN; i++)
 		dev->dev_addr[i] = RTL_R8(tp, MAC0 + i);
 
-- 
2.19.1

Re: [PATCH] r8169: Load MAC address from device tree if present

[Andrew Lunn]

On Fri, Jan 25, 2019 at 11:18:14AM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> If the system was booted using a device tree and if the device tree
> contains a MAC address, use it instead of reading one from the EEPROM.
> This is useful in situations where the EEPROM isn't properly programmed
> or where the firmware wants to override the existing MAC address.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Based on net-next.
> 
>  drivers/net/ethernet/realtek/r8169.c | 35 +++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index f574b6b557f9..fd9edd643ca5 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -6957,6 +6957,21 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
>  	return pci_alloc_irq_vectors(tp->pci_dev, 1, 1, flags);
>  }
>  
> +static void rtl_read_mac_address(struct rtl8169_private *tp,
> +				 u8 mac_addr[ETH_ALEN])
> +{
> +	/* Get MAC address */
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_35 ... RTL_GIGA_MAC_VER_38:
> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
> +		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
> +		*(u16 *)&mac_addr[4] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>  DECLARE_RTL_COND(rtl_link_list_ready_cond)
>  {
>  	return RTL_R8(tp, MCU) & LINK_LIST_RDY;
> @@ -7148,6 +7163,7 @@ static int rtl_get_ether_clk(struct rtl8169_private *tp)
>  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
> +	u8 mac_addr[ETH_ALEN] __aligned(4);

Hi Thierry

Maybe now would be a good time to cleanup this __aligned(4), pointer
aliasing, etc?

> +	/* get MAC address */
> +	if (eth_platform_get_mac_address(&pdev->dev, mac_addr))
> +		rtl_read_mac_address(tp, mac_addr);
> +

Maybe that could be made more readable with:

      err = eth_platform_get_mac_address(&pdev->dev, mac_addr);
      if (err)
      	 	rtl_read_mac_address(tp, mac_addr);

Thanks
	Andrew

Re: [PATCH] r8169: Load MAC address from device tree if present

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3102 bytes --]

On Fri, Jan 25, 2019 at 03:57:11PM +0100, Andrew Lunn wrote:
> On Fri, Jan 25, 2019 at 11:18:14AM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > If the system was booted using a device tree and if the device tree
> > contains a MAC address, use it instead of reading one from the EEPROM.
> > This is useful in situations where the EEPROM isn't properly programmed
> > or where the firmware wants to override the existing MAC address.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Based on net-next.
> > 
> >  drivers/net/ethernet/realtek/r8169.c | 35 +++++++++++++++++-----------
> >  1 file changed, 22 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> > index f574b6b557f9..fd9edd643ca5 100644
> > --- a/drivers/net/ethernet/realtek/r8169.c
> > +++ b/drivers/net/ethernet/realtek/r8169.c
> > @@ -6957,6 +6957,21 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
> >  	return pci_alloc_irq_vectors(tp->pci_dev, 1, 1, flags);
> >  }
> >  
> > +static void rtl_read_mac_address(struct rtl8169_private *tp,
> > +				 u8 mac_addr[ETH_ALEN])
> > +{
> > +	/* Get MAC address */
> > +	switch (tp->mac_version) {
> > +	case RTL_GIGA_MAC_VER_35 ... RTL_GIGA_MAC_VER_38:
> > +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
> > +		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
> > +		*(u16 *)&mac_addr[4] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> >  DECLARE_RTL_COND(rtl_link_list_ready_cond)
> >  {
> >  	return RTL_R8(tp, MCU) & LINK_LIST_RDY;
> > @@ -7148,6 +7163,7 @@ static int rtl_get_ether_clk(struct rtl8169_private *tp)
> >  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  {
> >  	const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
> > +	u8 mac_addr[ETH_ALEN] __aligned(4);
> 
> Hi Thierry
> 
> Maybe now would be a good time to cleanup this __aligned(4), pointer
> aliasing, etc?

Are you proposing that I rewrite rtl_read_mac_address() to manually
extract bytes from each register read? Something along these lines:

	value = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
	mac_addr[0] = (value >>  0) & 0xff;
	mac_addr[1] = (value >>  8) & 0xff;
	mac_addr[2] = (value >> 16) & 0xff;
	mac_addr[3] = (value >> 24) & 0xff;

	value = rtl_eri_read(tp, 0xe4 ERIAR_EXGMAC);
	mac_addr[4] = (value >>  0) & 0xff;
	mac_addr[5] = (value >>  8) & 0xff;

Looks like maybe that should be a separate patch?

> 
> > +	/* get MAC address */
> > +	if (eth_platform_get_mac_address(&pdev->dev, mac_addr))
> > +		rtl_read_mac_address(tp, mac_addr);
> > +
> 
> Maybe that could be made more readable with:
> 
>       err = eth_platform_get_mac_address(&pdev->dev, mac_addr);
>       if (err)
>       	 	rtl_read_mac_address(tp, mac_addr);

I was following the same pattern that other drivers were, but I can
change to the above if you prefer.

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] r8169: Load MAC address from device tree if present

[Heiner Kallweit]

On 25.01.2019 15:57, Andrew Lunn wrote:
> On Fri, Jan 25, 2019 at 11:18:14AM +0100, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> If the system was booted using a device tree and if the device tree
>> contains a MAC address, use it instead of reading one from the EEPROM.
>> This is useful in situations where the EEPROM isn't properly programmed
>> or where the firmware wants to override the existing MAC address.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> ---
>> Based on net-next.
>>
>>  drivers/net/ethernet/realtek/r8169.c | 35 +++++++++++++++++-----------
>>  1 file changed, 22 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>> index f574b6b557f9..fd9edd643ca5 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -6957,6 +6957,21 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
>>  	return pci_alloc_irq_vectors(tp->pci_dev, 1, 1, flags);
>>  }
>>  
>> +static void rtl_read_mac_address(struct rtl8169_private *tp,
>> +				 u8 mac_addr[ETH_ALEN])
>> +{
>> +	/* Get MAC address */
>> +	switch (tp->mac_version) {
>> +	case RTL_GIGA_MAC_VER_35 ... RTL_GIGA_MAC_VER_38:
>> +	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
>> +		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
>> +		*(u16 *)&mac_addr[4] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>>  DECLARE_RTL_COND(rtl_link_list_ready_cond)
>>  {
>>  	return RTL_R8(tp, MCU) & LINK_LIST_RDY;
>> @@ -7148,6 +7163,7 @@ static int rtl_get_ether_clk(struct rtl8169_private *tp)
>>  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  {
>>  	const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
>> +	u8 mac_addr[ETH_ALEN] __aligned(4);
> 
> Hi Thierry
> 
> Maybe now would be a good time to cleanup this __aligned(4), pointer
> aliasing, etc?
> 
Andrew, for my understanding: What do you think is wrong with the 
alignment requirement? It was introduced because we do a 32 bit access
to the start address of the array and want to avoid an unaligned access.

>> +	/* get MAC address */
>> +	if (eth_platform_get_mac_address(&pdev->dev, mac_addr))
>> +		rtl_read_mac_address(tp, mac_addr);
>> +
> 
> Maybe that could be made more readable with:
> 
>       err = eth_platform_get_mac_address(&pdev->dev, mac_addr);
>       if (err)
>       	 	rtl_read_mac_address(tp, mac_addr);
> 
> Thanks
> 	Andrew
> .
> 

Re: [PATCH] r8169: Load MAC address from device tree if present

[Heiner Kallweit]

On 25.01.2019 11:18, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> If the system was booted using a device tree and if the device tree
> contains a MAC address, use it instead of reading one from the EEPROM.
> This is useful in situations where the EEPROM isn't properly programmed
> or where the firmware wants to override the existing MAC address.
> 
I rarely see DT-configured boards with RTL8168 network. Do you add this
patch because of a specific board?
And you state "if EEPROM isn't properly programmed": Did you come across
such a case?

In general the patch is fine with me, I just want to understand the
motivation. One further comment see inline.
As of today we already have the option to set a MAC from userspace
via ethtool.

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Based on net-next.
> 
>  drivers/net/ethernet/realtek/r8169.c | 35 +++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index f574b6b557f9..fd9edd643ca5 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -6957,6 +6957,21 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
>  	return pci_alloc_irq_vectors(tp->pci_dev, 1, 1, flags);
>  }
>  
[...]
> @@ -7252,20 +7268,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	u64_stats_init(&tp->rx_stats.syncp);
>  	u64_stats_init(&tp->tx_stats.syncp);
>  
> -	/* Get MAC address */
> -	switch (tp->mac_version) {
> -		u8 mac_addr[ETH_ALEN] __aligned(4);
> -	case RTL_GIGA_MAC_VER_35 ... RTL_GIGA_MAC_VER_38:
> -	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
> -		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
> -		*(u16 *)&mac_addr[4] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
> +	/* get MAC address */
> +	if (eth_platform_get_mac_address(&pdev->dev, mac_addr))
> +		rtl_read_mac_address(tp, mac_addr);
> +
> +	if (is_valid_ether_addr(mac_addr))

Here array mac_addr may be uninitialized (if platform defines no MAC
and chip version is not covered by the switch statement).

> +		rtl_rar_set(tp, mac_addr);
>  
> -		if (is_valid_ether_addr(mac_addr))
> -			rtl_rar_set(tp, mac_addr);
> -		break;
> -	default:
> -		break;
> -	}
>  	for (i = 0; i < ETH_ALEN; i++)
>  		dev->dev_addr[i] = RTL_R8(tp, MAC0 + i);
>  
> 

Re: [PATCH] r8169: Load MAC address from device tree if present

[Andrew Lunn]

> Andrew, for my understanding: What do you think is wrong with the 
> alignment requirement? It was introduced because we do a 32 bit access
> to the start address of the array and want to avoid an unaligned access.

Hi Heiner

Because you are doing pointer aliasing, the compiler will by default
generate bad code, doing unaligned access. Adding the attribute works
around this. But it is just a work around. Since this is very slow
path code, i would just avoid the pointer aliasing, write a bit more C
code as Thierry suggested, and the optimiser will probably figure out
what is going on and produce reasonable code.

Also, in general, by avoiding pointer aliasing, you allow static code
checkers to work better. They are more likely to discover buffer
overruns, etc.

    Andrew

Re: [PATCH] r8169: Load MAC address from device tree if present

[Heiner Kallweit]

On 25.01.2019 20:07, Andrew Lunn wrote:
>> Andrew, for my understanding: What do you think is wrong with the 
>> alignment requirement? It was introduced because we do a 32 bit access
>> to the start address of the array and want to avoid an unaligned access.
> 
> Hi Heiner
> 
> Because you are doing pointer aliasing, the compiler will by default
> generate bad code, doing unaligned access. Adding the attribute works
> around this. But it is just a work around. Since this is very slow
> path code, i would just avoid the pointer aliasing, write a bit more C
> code as Thierry suggested, and the optimiser will probably figure out
> what is going on and produce reasonable code.
> 
> Also, in general, by avoiding pointer aliasing, you allow static code
> checkers to work better. They are more likely to discover buffer
> overruns, etc.
> 
>     Andrew
> 

Thanks, good to know.

The following doesn't hurt us here, but things like this have to be
considered too. According to chip spec:

"The ID registers 0-5 are only permitted to write by 4-byte access.
Read access can be byte, word, or double word access."

Heiner

Re: [PATCH] r8169: Load MAC address from device tree if present

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3459 bytes --]

On Fri, Jan 25, 2019 at 07:34:31PM +0100, Heiner Kallweit wrote:
> On 25.01.2019 11:18, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > If the system was booted using a device tree and if the device tree
> > contains a MAC address, use it instead of reading one from the EEPROM.
> > This is useful in situations where the EEPROM isn't properly programmed
> > or where the firmware wants to override the existing MAC address.
> > 
> I rarely see DT-configured boards with RTL8168 network. Do you add this
> patch because of a specific board?
> And you state "if EEPROM isn't properly programmed": Did you come across
> such a case?

We use these Realtek chips on some of our boards that customers can
either purchase individually and integrate into their own designs or
they can get the module as part of a product.

In order to easily allow customers to reprogram the device (they may
want to do that if integrating the module into their own products), a
so-called ID EEPROM is part of the module that stores various bits of
information. The ethernet MAC address is part of that EEPROM.

Typically the ID EEPROM will contain a valid MAC address if the module
is part of a product, but if customers purchase the module individually,
it is expected that they use a MAC address from their own pool.

Typically early boot firmware will load the MAC address from the EEPROM
and store it in the ethernet device's device tree node so that the MAC
address programmed into the ID EEPROM will be used by the ethernet
device at runtime.

> In general the patch is fine with me, I just want to understand the
> motivation. One further comment see inline.
> As of today we already have the option to set a MAC from userspace
> via ethtool.
> 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Based on net-next.
> > 
> >  drivers/net/ethernet/realtek/r8169.c | 35 +++++++++++++++++-----------
> >  1 file changed, 22 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> > index f574b6b557f9..fd9edd643ca5 100644
> > --- a/drivers/net/ethernet/realtek/r8169.c
> > +++ b/drivers/net/ethernet/realtek/r8169.c
> > @@ -6957,6 +6957,21 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
> >  	return pci_alloc_irq_vectors(tp->pci_dev, 1, 1, flags);
> >  }
> >  
> [...]
> > @@ -7252,20 +7268,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	u64_stats_init(&tp->rx_stats.syncp);
> >  	u64_stats_init(&tp->tx_stats.syncp);
> >  
> > -	/* Get MAC address */
> > -	switch (tp->mac_version) {
> > -		u8 mac_addr[ETH_ALEN] __aligned(4);
> > -	case RTL_GIGA_MAC_VER_35 ... RTL_GIGA_MAC_VER_38:
> > -	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
> > -		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
> > -		*(u16 *)&mac_addr[4] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
> > +	/* get MAC address */
> > +	if (eth_platform_get_mac_address(&pdev->dev, mac_addr))
> > +		rtl_read_mac_address(tp, mac_addr);
> > +
> > +	if (is_valid_ether_addr(mac_addr))
> 
> Here array mac_addr may be uninitialized (if platform defines no MAC
> and chip version is not covered by the switch statement).

Good point. I can memset() mac_addr to make sure it is invalid, rather
than undefined, for chip versions that are not covered.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] r8169: Load MAC address from device tree if present

[Heiner Kallweit]

On 29.01.2019 18:40, Thierry Reding wrote:
> On Fri, Jan 25, 2019 at 07:34:31PM +0100, Heiner Kallweit wrote:
>> On 25.01.2019 11:18, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> If the system was booted using a device tree and if the device tree
>>> contains a MAC address, use it instead of reading one from the EEPROM.
>>> This is useful in situations where the EEPROM isn't properly programmed
>>> or where the firmware wants to override the existing MAC address.
>>>
>> I rarely see DT-configured boards with RTL8168 network. Do you add this
>> patch because of a specific board?
>> And you state "if EEPROM isn't properly programmed": Did you come across
>> such a case?
> 
> We use these Realtek chips on some of our boards that customers can
> either purchase individually and integrate into their own designs or
> they can get the module as part of a product.
> 
> In order to easily allow customers to reprogram the device (they may
> want to do that if integrating the module into their own products), a
> so-called ID EEPROM is part of the module that stores various bits of
> information. The ethernet MAC address is part of that EEPROM.
> 
> Typically the ID EEPROM will contain a valid MAC address if the module
> is part of a product, but if customers purchase the module individually,
> it is expected that they use a MAC address from their own pool.
> 
> Typically early boot firmware will load the MAC address from the EEPROM
> and store it in the ethernet device's device tree node so that the MAC
> address programmed into the ID EEPROM will be used by the ethernet
> device at runtime.
> 
Thanks for the explanation.

>> In general the patch is fine with me, I just want to understand the
>> motivation. One further comment see inline.
>> As of today we already have the option to set a MAC from userspace
>> via ethtool.
>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>> Based on net-next.
>>>
>>>  drivers/net/ethernet/realtek/r8169.c | 35 +++++++++++++++++-----------
>>>  1 file changed, 22 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>>> index f574b6b557f9..fd9edd643ca5 100644
>>> --- a/drivers/net/ethernet/realtek/r8169.c
>>> +++ b/drivers/net/ethernet/realtek/r8169.c
>>> @@ -6957,6 +6957,21 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
>>>  	return pci_alloc_irq_vectors(tp->pci_dev, 1, 1, flags);
>>>  }
>>>  
>> [...]
>>> @@ -7252,20 +7268,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>  	u64_stats_init(&tp->rx_stats.syncp);
>>>  	u64_stats_init(&tp->tx_stats.syncp);
>>>  
>>> -	/* Get MAC address */
>>> -	switch (tp->mac_version) {
>>> -		u8 mac_addr[ETH_ALEN] __aligned(4);
>>> -	case RTL_GIGA_MAC_VER_35 ... RTL_GIGA_MAC_VER_38:
>>> -	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51:
>>> -		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
>>> -		*(u16 *)&mac_addr[4] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
>>> +	/* get MAC address */
>>> +	if (eth_platform_get_mac_address(&pdev->dev, mac_addr))
>>> +		rtl_read_mac_address(tp, mac_addr);
>>> +
>>> +	if (is_valid_ether_addr(mac_addr))
>>
>> Here array mac_addr may be uninitialized (if platform defines no MAC
>> and chip version is not covered by the switch statement).
> 
> Good point. I can memset() mac_addr to make sure it is invalid, rather
> than undefined, for chip versions that are not covered.
> 
An empty initializer would work as well.

> Thierry
> 
Heiner