Bug 215087

Summary: r8169 driver returns incorrect MAC address
Product: Drivers Reporter: Richard Herbert (rherbert)
Component: NetworkAssignee: drivers_network (drivers_network)
Status: RESOLVED CODE_FIX    
Severity: normal CC: ajchapman, hkallweit1, rherbert
Priority: P1    
Hardware: x86-64   
OS: Linux   
Kernel Version: 5.16.0-rc1 Subsystem:
Regression: No Bisected commit-id:
Attachments: r8169_main.c patch for 5.16.0-rc1

Description Richard Herbert 2021-11-20 22:00:04 UTC
Created attachment 299661 [details]
r8169_main.c patch for 5.16.0-rc1

Compared to the correct behavior in recent kernels (eg. 5.15.3), the 5.16.0-rc1 kernel doesn't assign the correct MAC address to the Realtek Ethernet adapter.

RTL8211B Gigabit Ethernet r8169-0-300:00: attached PHY driver (mii_bus:phy_addr=r8169-0-300:00, irq=MAC) is assigned the following MAC address:  00:00:00:00:02:02.

The attached patch reverts a couple of the changes in /drivers/net/ethernet/realtek/r8169_main.c, and corrects the problem.

Thanks.
Comment 1 Richard Herbert 2021-11-21 17:47:47 UTC
Confirming that only the first patch is required to fix the problem.

@@ -5218,7 +5255,7 @@ 
 static void rtl_init_mac_address(struct rtl8169_private *tp)
 {
 	struct net_device *dev = tp->dev;
-	u8 mac_addr[ETH_ALEN];
+	u8 *mac_addr = dev->dev_addr;
 	int rc;

Thanks.
Comment 2 Heiner Kallweit 2021-11-22 11:11:52 UTC
What you propose is simply a revert of 1c5d09d58748.
I think that the issue you see can only happen when the random MAC path is hit.
Do you see warning "can't read MAC address, setting random one" in your dmesg under 5.15? Would also be helpful to see the MAC address you have under 5.15.
Comment 3 Richard Herbert 2021-11-22 16:56:18 UTC
No, I don't see the warning you asked me about in dmesg.  Below you can see the difference in dmesg between an unpatched 5.16-rc2 and 5.15.4. The MAC addresses for each case are also included.  The MAC address with kernel 5.15.4 is the correct one.


5.16-rc2 unpatched:

[    5.586473] libphy: r8169: probed
[    5.589623] r8169 0000:03:00.0 eth0: RTL8168d/8111d, 00:00:00:00:02:02, XID 281, IRQ 26
[    5.589659] r8169 0000:03:00.0 eth0: jumbo features [frames: 9194 bytes, tx checksumming: ko]
[    5.650556] r8169 0000:03:00.0 eth125: renamed from eth0
[    5.654242] r8169 0000:03:00.0 eth1: renamed from eth125
[   12.164515] RTL8211B Gigabit Ethernet r8169-0-300:00: attached PHY driver (mii_bus:phy_addr=r8169-0-300:00, irq=MAC)
[   12.303551] r8169 0000:03:00.0 eth1: No native access to PCI extended config space, falling back to CSI
[   12.309402] r8169 0000:03:00.0 eth1: Link is Down
[   15.021408] r8169 0000:03:00.0 eth1: Link is Up - 1Gbps/Full - flow control rx/tx

5.15.4 no patch required:

[    5.498558] libphy: r8169: probed
[    5.505377] r8169 0000:03:00.0 eth0: RTL8168d/8111d, 00:27:0e:0b:8c:b4, XID 281, IRQ 26
[    5.505437] r8169 0000:03:00.0 eth0: jumbo features [frames: 9194 bytes, tx checksumming: ko]
[   12.130025] RTL8211B Gigabit Ethernet r8169-0-300:00: attached PHY driver (mii_bus:phy_addr=r8169-0-300:00, irq=MAC)
[   12.291220] r8169 0000:03:00.0 eth0: No native access to PCI extended config space, falling back to CSI
[   12.293793] r8169 0000:03:00.0 eth0: Link is Down
[   15.045637] r8169 0000:03:00.0 eth0: Link is Up - 1Gbps/Full - flow control rx/tx
Comment 4 Heiner Kallweit 2021-11-22 20:14:29 UTC
Thanks for the details! This makes me think that the following happens:
On your system eth_platform_get_mac_address() and rtl_read_mac_address() both are no-ops. Variable mac_addr isn't initialized and if by chance its random value passes is_valid_ether_addr(), then this value is set as MAC address.
The following patch fixes this and two other issues with the recent introduction of eth_hw_addr_set().
Could you please test whether the following fixes the issue for you?


diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index e896e5eca..e9b560051 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5226,8 +5226,8 @@ static int rtl_get_ether_clk(struct rtl8169_private *tp)
 
 static void rtl_init_mac_address(struct rtl8169_private *tp)
 {
+	u8 mac_addr[ETH_ALEN] __aligned(2) = {};
 	struct net_device *dev = tp->dev;
-	u8 mac_addr[ETH_ALEN];
 	int rc;
 
 	rc = eth_platform_get_mac_address(tp_to_dev(tp), mac_addr);
@@ -5242,7 +5242,8 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
 	if (is_valid_ether_addr(mac_addr))
 		goto done;
 
-	eth_hw_addr_random(dev);
+	eth_random_addr(mac_addr);
+	dev->addr_assign_type = NET_ADDR_RANDOM;
 	dev_warn(tp_to_dev(tp), "can't read MAC address, setting random one\n");
 done:
 	eth_hw_addr_set(dev, mac_addr);
-- 
2.34.0
Comment 5 Richard Herbert 2021-11-22 20:50:37 UTC
(In reply to Heiner Kallweit from comment #4)
> Thanks for the details! This makes me think that the following happens:
> On your system eth_platform_get_mac_address() and rtl_read_mac_address()
> both are no-ops. Variable mac_addr isn't initialized and if by chance its
> random value passes is_valid_ether_addr(), then this value is set as MAC
> address.
> The following patch fixes this and two other issues with the recent
> introduction of eth_hw_addr_set().
> Could you please test whether the following fixes the issue for you?
> 
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> b/drivers/net/ethernet/realtek/r8169_main.c
> index e896e5eca..e9b560051 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -5226,8 +5226,8 @@ static int rtl_get_ether_clk(struct rtl8169_private
> *tp)
>  
>  static void rtl_init_mac_address(struct rtl8169_private *tp)
>  {
> +     u8 mac_addr[ETH_ALEN] __aligned(2) = {};
>       struct net_device *dev = tp->dev;
> -     u8 mac_addr[ETH_ALEN];
>       int rc;
>  
>       rc = eth_platform_get_mac_address(tp_to_dev(tp), mac_addr);
> @@ -5242,7 +5242,8 @@ static void rtl_init_mac_address(struct
> rtl8169_private *tp)
>       if (is_valid_ether_addr(mac_addr))
>               goto done;
>  
> -     eth_hw_addr_random(dev);
> +     eth_random_addr(mac_addr);
> +     dev->addr_assign_type = NET_ADDR_RANDOM;
>       dev_warn(tp_to_dev(tp), "can't read MAC address, setting random
>  one\n");
>  done:
>       eth_hw_addr_set(dev, mac_addr);
> -- 
> 2.34.0

I confirm that your patch does indeed do the trick on my system. Here is the result from 'dmesg | grep -i r8169 (kernel 5.16.0-rc2).

rherbert@starbug:~$ dmesg | grep -i r8169
[    5.518161] libphy: r8169: probed
[    5.522013] r8169 0000:03:00.0 eth0: RTL8168d/8111d, 00:27:0e:0b:8c:b4, XID 281, IRQ 26
[    5.522047] r8169 0000:03:00.0 eth0: jumbo features [frames: 9194 bytes, tx checksumming: ko]
[   11.863166] RTL8211B Gigabit Ethernet r8169-0-300:00: attached PHY driver (mii_bus:phy_addr=r8169-0-300:00, irq=MAC)
[   12.022321] r8169 0000:03:00.0 eth0: No native access to PCI extended config space, falling back to CSI
[   12.024257] r8169 0000:03:00.0 eth0: Link is Down
[   14.982880] r8169 0000:03:00.0 eth0: Link is Up - 1Gbps/Full - flow control rx/tx

Thanks
Comment 6 Heiner Kallweit 2021-11-24 09:27:26 UTC
Fixed with c75a9ad43691 ("r8169: fix incorrect mac address assignment").
Comment 7 Richard Herbert 2021-11-28 23:21:28 UTC
Closing. Added to kernel 5.16-rc3. Thanks!