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

From: Heiner Kallweit <hkallweit1@gmail.com>
To: Brian Norris <briannorris@chromium.org>,
	Realtek linux nic maintainers <nic_swsd@realtek.com>
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	Chun-Hao Lin <hau@realtek.com>
Subject: Re: [PATCH] [RFC] r8169: check for valid MAC before clobbering
Date: Wed, 13 Nov 2019 21:30:42 +0100	[thread overview]
Message-ID: <32422b2d-6cab-3ea2-aca3-3e74d68599a3@gmail.com> (raw)
In-Reply-To: <20191113005816.37084-1-briannorris@chromium.org>

On 13.11.2019 01:58, Brian Norris wrote:
> 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 application note refers to RTL8105e which is quite different from
RTL8168g. For RTL8168g the BIOS has to write the MAC to the respective
GigaMAC registers, see rtl_read_mac_address for these registers.

If recompiling the BIOS isn't an option, then easiest should be to
change the MAC after boot with "ifconfig" or "ip" command.

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