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@xxxxxxxxxxx>
> > Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>