| Message ID | 20191113005816.37084-1-briannorris@chromium.org |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC] r8169: check for valid MAC before clobbering | expand |
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; > >
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>
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> >
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> >
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
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
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;
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(-)