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