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