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

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

[Brian Norris]

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

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

[Heiner Kallweit]

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

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

[Brian Norris]

Hi Heiner,

Thanks for the response, and sorry for some delay. I've been busy in the
last week.

On Wed, Nov 13, 2019 at 09:30:42PM +0100, Heiner Kallweit wrote:
> 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.

Understood. But the register mapping for this part does appear to be the
same, and I'm really having trouble finding any other documentation, so
I can't really blame whoever was writing the Coreboot code in the first
place.

> For RTL8168g the BIOS has to write the MAC to the respective
> GigaMAC registers, see rtl_read_mac_address for these registers.

I already see the code, but do you have any reference docs? For example,
how am I to determine "has to"? I've totally failed at finding any good
documentation.

To the contrary, I did find an alleged RTL8169 document (no clue if it's
legit), and it appears to describe the IDR0-5 registers (i.e., offset
0000h) as:

  ID Register 0: The ID registers 0-5 are only permitted to write by
  4-byte access. Read access can be byte, word, or double word access.
  The initial value is autoloaded from EEPROM EthernetID field. 

If that implies anything, it seems to imply that any EEPROM settings
should be automatically applied, and that register 0-5h are the correct
source of truth.

Or it doesn't really imply anything, except that some other similar IP
doesn't specifically mention this "backup register."

> If recompiling the BIOS isn't an option,

It's not 100% impossible, but it seems highly unlikely to happen. To me
(and likely the folks responsible for this BIOS), this looks like a
kernel regression (this driver worked just fine for me before commit
89cceb2729c7).

> then easiest should be to
> change the MAC after boot with "ifconfig" or "ip" command.

No, I think the easiest option is to apply my patch, which I'll probably
do if I can't find anything else.

I'm curious: do you see any problem with my patch? In your
understanding, what's the purpose of the "backup registers" (as they
were called in commit 89cceb2729c7)? To be the primary source of MAC
address information? Or to only be a source if the primary registers are
empty? If the latter, then my patch should be a fine substitute.

Brian

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

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

[Heiner Kallweit]

On 23.11.2019 01:51, Brian Norris wrote:
> Hi Heiner,
> 
> Thanks for the response, and sorry for some delay. I've been busy in the
> last week.
> 
> On Wed, Nov 13, 2019 at 09:30:42PM +0100, Heiner Kallweit wrote:
>> 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.
> 
> Understood. But the register mapping for this part does appear to be the
> same, and I'm really having trouble finding any other documentation, so
> I can't really blame whoever was writing the Coreboot code in the first
> place.
> 
>> For RTL8168g the BIOS has to write the MAC to the respective
>> GigaMAC registers, see rtl_read_mac_address for these registers.
> 
> I already see the code, but do you have any reference docs? For example,
> how am I to determine "has to"? I've totally failed at finding any good
> documentation.
> 
Realtek doesn't provide any public datasheets, only very few leaked
old datasheets are available. Only public source of information is
the vendor drivers: r8168/r8101/r8125.
Check the vendor drivers for where they read the MAC from.

> To the contrary, I did find an alleged RTL8169 document (no clue if it's
> legit), and it appears to describe the IDR0-5 registers (i.e., offset
> 0000h) as:
> 
>   ID Register 0: The ID registers 0-5 are only permitted to write by
>   4-byte access. Read access can be byte, word, or double word access.
>   The initial value is autoloaded from EEPROM EthernetID field. 
> 
> If that implies anything, it seems to imply that any EEPROM settings
> should be automatically applied, and that register 0-5h are the correct
> source of truth.
> 
RTL8169 is a very old chip version, and although an EEPROM interface
is specified, basically no design uses it.

> Or it doesn't really imply anything, except that some other similar IP
> doesn't specifically mention this "backup register."
> 
>> If recompiling the BIOS isn't an option,
> 
> It's not 100% impossible, but it seems highly unlikely to happen. To me
> (and likely the folks responsible for this BIOS), this looks like a
> kernel regression (this driver worked just fine for me before commit
> 89cceb2729c7).
> 
>> then easiest should be to
>> change the MAC after boot with "ifconfig" or "ip" command.
> 
> No, I think the easiest option is to apply my patch, which I'll probably
> do if I can't find anything else.
> 
> I'm curious: do you see any problem with my patch? In your
> understanding, what's the purpose of the "backup registers" (as they
> were called in commit 89cceb2729c7)? To be the primary source of MAC
> address information? Or to only be a source if the primary registers are
> empty? If the latter, then my patch should be a fine substitute.
> 
There are two types of MAC registers, first one for the currently active
MAC, and the second one for the permanent MAC.
Old chip versions don't support permanent MAC, newer ones use different
registers for it. For RTL8168g it's the mentioned GigaMAC register.
And BIOS code should set the permanent MAC.

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

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

[Brian Norris]

On Sat, Nov 23, 2019 at 1:59 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Realtek doesn't provide any public datasheets, only very few leaked
> old datasheets are available. Only public source of information is
> the vendor drivers: r8168/r8101/r8125.
> Check the vendor drivers for where they read the MAC from.

Thanks, I looked it up, and IIUC the chips I'm using would fall under
the vendor driver's 'CFG_METHOD_21', which does indeed check the GMAC
registers as a priority. (It's also even worse than the upstream
driver here: although it reads out the active MAC register first, it
doesn't end up using the value and instead just clobbers it, even if
the GMAC value is empty/garbage.)

So I guess the vendor driver "always" failed me in the same way, and
it's just the Coreboot authors who were misinformed. :(

Thanks for the pointers,
Brian

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

[Heiner Kallweit]

On 23.11.2019 01:51, Brian Norris wrote:
> Hi Heiner,
> 
> Thanks for the response, and sorry for some delay. I've been busy in the
> last week.
> 
> On Wed, Nov 13, 2019 at 09:30:42PM +0100, Heiner Kallweit wrote:
>> 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.
> 
> Understood. But the register mapping for this part does appear to be the
> same, and I'm really having trouble finding any other documentation, so
> I can't really blame whoever was writing the Coreboot code in the first
> place.
> 
>> For RTL8168g the BIOS has to write the MAC to the respective
>> GigaMAC registers, see rtl_read_mac_address for these registers.
> 
> I already see the code, but do you have any reference docs? For example,
> how am I to determine "has to"? I've totally failed at finding any good
> documentation.
> 
> To the contrary, I did find an alleged RTL8169 document (no clue if it's
> legit), and it appears to describe the IDR0-5 registers (i.e., offset
> 0000h) as:
> 
>   ID Register 0: The ID registers 0-5 are only permitted to write by
>   4-byte access. Read access can be byte, word, or double word access.
>   The initial value is autoloaded from EEPROM EthernetID field. 
> 
> If that implies anything, it seems to imply that any EEPROM settings
> should be automatically applied, and that register 0-5h are the correct
> source of truth.
> 
> Or it doesn't really imply anything, except that some other similar IP
> doesn't specifically mention this "backup register."
> 
>> If recompiling the BIOS isn't an option,
> 
> It's not 100% impossible, but it seems highly unlikely to happen. To me
> (and likely the folks responsible for this BIOS), this looks like a
> kernel regression (this driver worked just fine for me before commit
> 89cceb2729c7).
> 
On an additional note:
The referenced coreboot driver is part of the Google JECHT baseboard
support. Most likely the driver is just meant to support the Realtek
chip version found on this board. I doubt the driver authors intended
to support each and every Realtek NIC chip version.

>> then easiest should be to
>> change the MAC after boot with "ifconfig" or "ip" command.
> 
> No, I think the easiest option is to apply my patch, which I'll probably
> do if I can't find anything else.
> 
> I'm curious: do you see any problem with my patch? In your
> understanding, what's the purpose of the "backup registers" (as they
> were called in commit 89cceb2729c7)? To be the primary source of MAC
> address information? Or to only be a source if the primary registers are
> empty? If the latter, then my patch should be a fine substitute.
> 
> Brian
> 
>>> 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>
> 

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

[Brian Norris]

On Sat, Nov 23, 2019 at 2:46 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> On 23.11.2019 01:51, Brian Norris wrote:
> > On Wed, Nov 13, 2019 at 09:30:42PM +0100, Heiner Kallweit wrote:
> >> If recompiling the BIOS isn't an option,
> >
> > It's not 100% impossible, but it seems highly unlikely to happen. To me
> > (and likely the folks responsible for this BIOS), this looks like a
> > kernel regression (this driver worked just fine for me before commit
> > 89cceb2729c7).
> >
> On an additional note:
> The referenced coreboot driver is part of the Google JECHT baseboard
> support. Most likely the driver is just meant to support the Realtek
> chip version found on this board. I doubt the driver authors intended
> to support each and every Realtek NIC chip version.

I understand that -- I'm specifically seeing problems on the Jecht
family of devices (Jecht was the reference board), which is why I
pointed you there :) All devices in that family use a Realtek chipset
that appears to be RTL8168G, and they all only program the registers I
pointed at in the first place.

One side note: I'm not quite sure how (again, no documentation...) but
some devices appear to have a different valid MAC address in the
GigaMAC register, which is why I see this problem. If they all just
left it 0x00, then I'd be in OK shape.

Brian