[PATCH] [RFC] r8169: check for valid MAC before clobbering

From: Brian Norris <briannorris@chromium.org>
To: Realtek linux nic maintainers <nic_swsd@realtek.com>,
	Heiner Kallweit <hkallweit1@gmail.com>
Cc: <linux-kernel@vger.kernel.org>,
	netdev@vger.kernel.org, Brian Norris <briannorris@chromium.org>,
	Chun-Hao Lin <hau@realtek.com>
Subject: [PATCH] [RFC] r8169: check for valid MAC before clobbering
Date: Tue, 12 Nov 2019 16:58:16 -0800	[thread overview]
Message-ID: <20191113005816.37084-1-briannorris@chromium.org> (raw)

I have some old systems with RTL8168g Ethernet, where the BIOS (based on
Coreboot) programs the MAC address into the MAC0 registers (at offset
0x0 and 0x4). The relevant Coreboot source is publicly available here:

https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/jecht/lan.c?h=4.10#n139

(The BIOS is built off a much older branch, but the code is effectively
the same.)

Note that this was apparently the recommended solution in an application
note at the time (I have a copy, but it's not marked for redistribution
:( ), with no mention of the method used in rtl_read_mac_address().

The result is that ever since commit 89cceb2729c7 ("r8169:add support
more chips to get mac address from backup mac address register"), my MAC
address changes to use an address I never intended.

Unfortunately, these commits don't really provide any documentation, and
I'm not sure when the recommendation actually changed. So I'm sending
this as RFC, in case I can get any tips from Realtek on how to avoid
breaking compatibility like this.

I'll freely admit that the devices in question are currently pinned to
an ancient kernel. We're only recently testing newer kernels on these
devices, which brings me here.

I'll also admit that I don't have much means to test this widely, and
I'm not sure what implicit behaviors other systems were depending on
along the way.

Fixes: 89cceb2729c7 ("r8169:add support more chips to get mac address from backup mac address register")
Fixes: 6e1d0b898818 ("r8169:add support for RTL8168H and RTL8107E")
Cc: Chun-Hao Lin <hau@realtek.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/ethernet/realtek/r8169_main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index c4e961ea44d5..94067cf30514 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -7031,11 +7031,14 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
 	if (!rc)
 		goto done;
 
-	rtl_read_mac_address(tp, mac_addr);
+	/* Check first to see if (e.g.) bootloader already programmed
+	 * something.
+	 */
+	rtl_read_mac_from_reg(tp, mac_addr, MAC0);
 	if (is_valid_ether_addr(mac_addr))
 		goto done;
 
-	rtl_read_mac_from_reg(tp, mac_addr, MAC0);
+	rtl_read_mac_address(tp, mac_addr);
 	if (is_valid_ether_addr(mac_addr))
 		goto done;
 
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

