| Message ID | 1541778262-11557-1-git-send-email-paolo.pisati@canonical.com |
|---|---|
| Headers | show |
| Series | RaspberryPi 3B+: fix ethernet leds | expand |
Whew, a long read, but seems entirely reasonable ... Acked-by: Kamal Mostafa <kamal@canonical.com> -Kamal On Fri, Nov 09, 2018 at 04:44:18PM +0100, Paolo Pisati wrote: > BugLink: http://bugs.launchpad.net/bugs/1802320 > > Impact: > > Using a xenial/raspi2 kernel on a RaspberryPi3B+ board, ethernet leds don't > blink (though ethernet port works fine). > > Leds not working are due to a missing (actualy reverted) upstream stable patch > in xenial/raspi2 (1111a9 "UBUNTU: SAUCE: Revert "lan78xx: Correctly indicate > invalid OTP""), but reverting that patch uncovers another more severe bug: the > ethernet port stops working since two hardware registers are not updated when, > during bootup, the mac address is set (i explained this part of the bug in > detail here: https://lkml.org/lkml/2018/11/7/860). > > When doing the initial enablement of the rpi3bp+ board in > xenial, i found that the ethernet port was not working and after a bisection i > found an upstream stable commit (the above "lan78xx: Correctly indicate invalid > OTP") that was causing it. > > But it turned out that this patch was legit, and the buggy behaviour of the > lan78xx_read_otp() function (that was being fixed by the above "lan78xx: > Correctly indicate invalid OTP" patch) was actually hiding another bug in the > lan78xx ethernet driver: > > https://lkml.org/lkml/2018/11/7/860 > > " > Upon invocation, lan78xx_init_mac_address() checks that the mac address present > in the RX_ADDRL & RX_ADDRH registers is a valid address, if not, it first tries > to read a new address from an external eeprom or the otp area, and in case both > read fail (or the address read back is invalid), it randomly generates a new > one. > > Unfortunately, due to the way the above logic is laid out, if both > read_eeprom() and read_otp() fail, a new mac address is correctly generated but > is never written back to RX_ADDRL & RX_ADDRH, leaving the chip in an > incosistent state and with an invalid mac address (e.g. the nic appears to be > completely dead, and doesn't receive any packet, etc): > > lan78xx_init_mac_address() > ... > if (lan78xx_read_eeprom(addr ...) || lan78xx_read_otp(addr ...)) { > if (is_valid_ether_addr(addr) { > // nop... > } else { > random_ether_addr(addr); > } > > // correctly writes back the new address > lan78xx_write_reg(RX_ADDRL, addr ...); > lan78xx_write_reg(RX_ADDRH, addr ...); > } else { > // XXX if both eeprom and otp read fail, we land here and skip > // XXX the RX_ADDRL & RX_ADDRH update above > random_ether_addr(addr); > } > > This bug went unnoticed because lan78xx_read_otp() was buggy itself and would > never fail, up until 4bfc338 "lan78xx: Correctly indicate invalid OTP" fixed it > and as a side effect uncovered this bug. > " > > Upstream later decided to take an entire patch from 4.18.y instead of the fix i > proposed, but that is fine, and is one of the patch i'm presenting here (patch > 003 "lan78xx: Read MAC address from DT if present"). > > Fix: > > The whole fix consist in importing a new upstream patch from 4.18.y for the mac > address changing logic (patch 003), reverting two Raspberry BSP patches that > clash (and are obsoleted) by this new patch (patch 001 and 002), and then > reverting a SAUCE patch, or in other words, reapply the lan78xx_read_otp() > upstream fix (patch 004), that is the actual fix for the ethernet leds. > > Chronologically, first the two reverts: > > commit 3f25fbb82f00a80e9eb3be0ce60abebfc263c84a > Author: Paolo Pisati <paolo.pisati@canonical.com> > Date: Thu Nov 8 10:35:09 2018 +0000 > > Revert "lan78xx: Ignore DT MAC address if already valid" > > This reverts commit 17f23a96597810ddd56b0c10584fce77d7c3707f. > > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> > > and > > commit 6c8bdff882656296adc20b6ae0fb727483a73c7c > Author: Paolo Pisati <paolo.pisati@canonical.com> > Date: Thu Nov 8 10:35:11 2018 +0000 > > Revert "lan78xx: Read MAC address from DT if present" > > This reverts commit a23d928781936b51a61a67a0799b77b2a6becfa2. > > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> > > then the upstream cherry pick for the mac changing logic: > > commit 5d9c81e3aa1dd39dafd9a6ea30da05b26b655eca > Author: Phil Elwell <phil@raspberrypi.org> > Date: Thu Apr 19 17:59:38 2018 +0100 > > lan78xx: Read MAC address from DT if present > > There is a standard mechanism for locating and using a MAC address from > the Device Tree. Use this facility in the lan78xx driver to support > applications without programmed EEPROM or OTP. At the same time, > regularise the handling of the different address sources. > > Signed-off-by: Phil Elwell <phil@raspberrypi.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 760db29bdc97b73ff60b091315ad787b1deb5cf5) > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> > > and finally revert the SAUCE patch that fixes lan78xx_read_otp(), and makes the > led code work again: > > commit 24dc8f22b15cc983e13a0ae88b372dd1bfad09f9 > Author: Paolo Pisati <paolo.pisati@canonical.com> > Date: Thu Nov 8 10:37:17 2018 +0000 > > Revert "UBUNTU: SAUCE: Revert "lan78xx: Correctly indicate invalid OTP"" > > This reverts commit 1111a9e0bd2ebab88e736d8a9773df2ec76dc52c. > > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> > > See also the attached patches. > > How to test: > > Boot a patched kernel, connect an ethernet cable to the board and check that > the ethernet leds are blinking when there's traffic > > Regression potential: > > None, we are dropping two raspberry BSP patches in favor of an upstream patch > and re-applying another upstream fix: all patches have been upstream for > awhile, and are clean cherry picks. > > Paolo Pisati (3): > Revert "lan78xx: Ignore DT MAC address if already valid" > Revert "lan78xx: Read MAC address from DT if present" > Revert "UBUNTU: SAUCE: Revert "lan78xx: Correctly indicate invalid > OTP"" > > Phil Elwell (1): > lan78xx: Read MAC address from DT if present > > drivers/net/usb/lan78xx.c | 54 ++++++++++++++++++----------------------------- > 1 file changed, 21 insertions(+), 33 deletions(-) > > -- > 2.7.4 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 2018-11-09 16:44:18 , Paolo Pisati wrote: > BugLink: http://bugs.launchpad.net/bugs/1802320 > > Impact: > > Using a xenial/raspi2 kernel on a RaspberryPi3B+ board, ethernet leds don't > blink (though ethernet port works fine). > > Leds not working are due to a missing (actualy reverted) upstream stable patch > in xenial/raspi2 (1111a9 "UBUNTU: SAUCE: Revert "lan78xx: Correctly indicate > invalid OTP""), but reverting that patch uncovers another more severe bug: the > ethernet port stops working since two hardware registers are not updated when, > during bootup, the mac address is set (i explained this part of the bug in > detail here: https://lkml.org/lkml/2018/11/7/860). > > When doing the initial enablement of the rpi3bp+ board in > xenial, i found that the ethernet port was not working and after a bisection i > found an upstream stable commit (the above "lan78xx: Correctly indicate invalid > OTP") that was causing it. > > But it turned out that this patch was legit, and the buggy behaviour of the > lan78xx_read_otp() function (that was being fixed by the above "lan78xx: > Correctly indicate invalid OTP" patch) was actually hiding another bug in the > lan78xx ethernet driver: > > https://lkml.org/lkml/2018/11/7/860 > > " > Upon invocation, lan78xx_init_mac_address() checks that the mac address present > in the RX_ADDRL & RX_ADDRH registers is a valid address, if not, it first tries > to read a new address from an external eeprom or the otp area, and in case both > read fail (or the address read back is invalid), it randomly generates a new > one. > > Unfortunately, due to the way the above logic is laid out, if both > read_eeprom() and read_otp() fail, a new mac address is correctly generated but > is never written back to RX_ADDRL & RX_ADDRH, leaving the chip in an > incosistent state and with an invalid mac address (e.g. the nic appears to be > completely dead, and doesn't receive any packet, etc): > > lan78xx_init_mac_address() > ... > if (lan78xx_read_eeprom(addr ...) || lan78xx_read_otp(addr ...)) { > if (is_valid_ether_addr(addr) { > // nop... > } else { > random_ether_addr(addr); > } > > // correctly writes back the new address > lan78xx_write_reg(RX_ADDRL, addr ...); > lan78xx_write_reg(RX_ADDRH, addr ...); > } else { > // XXX if both eeprom and otp read fail, we land here and skip > // XXX the RX_ADDRL & RX_ADDRH update above > random_ether_addr(addr); > } > > This bug went unnoticed because lan78xx_read_otp() was buggy itself and would > never fail, up until 4bfc338 "lan78xx: Correctly indicate invalid OTP" fixed it > and as a side effect uncovered this bug. > " > > Upstream later decided to take an entire patch from 4.18.y instead of the fix i > proposed, but that is fine, and is one of the patch i'm presenting here (patch > 003 "lan78xx: Read MAC address from DT if present"). > > Fix: > > The whole fix consist in importing a new upstream patch from 4.18.y for the mac > address changing logic (patch 003), reverting two Raspberry BSP patches that > clash (and are obsoleted) by this new patch (patch 001 and 002), and then > reverting a SAUCE patch, or in other words, reapply the lan78xx_read_otp() > upstream fix (patch 004), that is the actual fix for the ethernet leds. > > Chronologically, first the two reverts: > > commit 3f25fbb82f00a80e9eb3be0ce60abebfc263c84a > Author: Paolo Pisati <paolo.pisati@canonical.com> > Date: Thu Nov 8 10:35:09 2018 +0000 > > Revert "lan78xx: Ignore DT MAC address if already valid" > > This reverts commit 17f23a96597810ddd56b0c10584fce77d7c3707f. > > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> > > and > > commit 6c8bdff882656296adc20b6ae0fb727483a73c7c > Author: Paolo Pisati <paolo.pisati@canonical.com> > Date: Thu Nov 8 10:35:11 2018 +0000 > > Revert "lan78xx: Read MAC address from DT if present" > > This reverts commit a23d928781936b51a61a67a0799b77b2a6becfa2. > > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> > > then the upstream cherry pick for the mac changing logic: > > commit 5d9c81e3aa1dd39dafd9a6ea30da05b26b655eca > Author: Phil Elwell <phil@raspberrypi.org> > Date: Thu Apr 19 17:59:38 2018 +0100 > > lan78xx: Read MAC address from DT if present > > There is a standard mechanism for locating and using a MAC address from > the Device Tree. Use this facility in the lan78xx driver to support > applications without programmed EEPROM or OTP. At the same time, > regularise the handling of the different address sources. > > Signed-off-by: Phil Elwell <phil@raspberrypi.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 760db29bdc97b73ff60b091315ad787b1deb5cf5) > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> > > and finally revert the SAUCE patch that fixes lan78xx_read_otp(), and makes the > led code work again: > > commit 24dc8f22b15cc983e13a0ae88b372dd1bfad09f9 > Author: Paolo Pisati <paolo.pisati@canonical.com> > Date: Thu Nov 8 10:37:17 2018 +0000 > > Revert "UBUNTU: SAUCE: Revert "lan78xx: Correctly indicate invalid OTP"" > > This reverts commit 1111a9e0bd2ebab88e736d8a9773df2ec76dc52c. > > Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> > > See also the attached patches. > > How to test: > > Boot a patched kernel, connect an ethernet cable to the board and check that > the ethernet leds are blinking when there's traffic > > Regression potential: > > None, we are dropping two raspberry BSP patches in favor of an upstream patch > and re-applying another upstream fix: all patches have been upstream for > awhile, and are clean cherry picks. > > Paolo Pisati (3): > Revert "lan78xx: Ignore DT MAC address if already valid" > Revert "lan78xx: Read MAC address from DT if present" > Revert "UBUNTU: SAUCE: Revert "lan78xx: Correctly indicate invalid > OTP"" > > Phil Elwell (1): > lan78xx: Read MAC address from DT if present > > drivers/net/usb/lan78xx.c | 54 ++++++++++++++++++----------------------------- > 1 file changed, 21 insertions(+), 33 deletions(-) > > -- > 2.7.4 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
BugLink: http://bugs.launchpad.net/bugs/1802320 Impact: Using a xenial/raspi2 kernel on a RaspberryPi3B+ board, ethernet leds don't blink (though ethernet port works fine). Leds not working are due to a missing (actualy reverted) upstream stable patch in xenial/raspi2 (1111a9 "UBUNTU: SAUCE: Revert "lan78xx: Correctly indicate invalid OTP""), but reverting that patch uncovers another more severe bug: the ethernet port stops working since two hardware registers are not updated when, during bootup, the mac address is set (i explained this part of the bug in detail here: https://lkml.org/lkml/2018/11/7/860). When doing the initial enablement of the rpi3bp+ board in xenial, i found that the ethernet port was not working and after a bisection i found an upstream stable commit (the above "lan78xx: Correctly indicate invalid OTP") that was causing it. But it turned out that this patch was legit, and the buggy behaviour of the lan78xx_read_otp() function (that was being fixed by the above "lan78xx: Correctly indicate invalid OTP" patch) was actually hiding another bug in the lan78xx ethernet driver: https://lkml.org/lkml/2018/11/7/860 " Upon invocation, lan78xx_init_mac_address() checks that the mac address present in the RX_ADDRL & RX_ADDRH registers is a valid address, if not, it first tries to read a new address from an external eeprom or the otp area, and in case both read fail (or the address read back is invalid), it randomly generates a new one. Unfortunately, due to the way the above logic is laid out, if both read_eeprom() and read_otp() fail, a new mac address is correctly generated but is never written back to RX_ADDRL & RX_ADDRH, leaving the chip in an incosistent state and with an invalid mac address (e.g. the nic appears to be completely dead, and doesn't receive any packet, etc): lan78xx_init_mac_address() ... if (lan78xx_read_eeprom(addr ...) || lan78xx_read_otp(addr ...)) { if (is_valid_ether_addr(addr) { // nop... } else { random_ether_addr(addr); } // correctly writes back the new address lan78xx_write_reg(RX_ADDRL, addr ...); lan78xx_write_reg(RX_ADDRH, addr ...); } else { // XXX if both eeprom and otp read fail, we land here and skip // XXX the RX_ADDRL & RX_ADDRH update above random_ether_addr(addr); } This bug went unnoticed because lan78xx_read_otp() was buggy itself and would never fail, up until 4bfc338 "lan78xx: Correctly indicate invalid OTP" fixed it and as a side effect uncovered this bug. " Upstream later decided to take an entire patch from 4.18.y instead of the fix i proposed, but that is fine, and is one of the patch i'm presenting here (patch 003 "lan78xx: Read MAC address from DT if present"). Fix: The whole fix consist in importing a new upstream patch from 4.18.y for the mac address changing logic (patch 003), reverting two Raspberry BSP patches that clash (and are obsoleted) by this new patch (patch 001 and 002), and then reverting a SAUCE patch, or in other words, reapply the lan78xx_read_otp() upstream fix (patch 004), that is the actual fix for the ethernet leds. Chronologically, first the two reverts: commit 3f25fbb82f00a80e9eb3be0ce60abebfc263c84a Author: Paolo Pisati <paolo.pisati@canonical.com> Date: Thu Nov 8 10:35:09 2018 +0000 Revert "lan78xx: Ignore DT MAC address if already valid" This reverts commit 17f23a96597810ddd56b0c10584fce77d7c3707f. Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> and commit 6c8bdff882656296adc20b6ae0fb727483a73c7c Author: Paolo Pisati <paolo.pisati@canonical.com> Date: Thu Nov 8 10:35:11 2018 +0000 Revert "lan78xx: Read MAC address from DT if present" This reverts commit a23d928781936b51a61a67a0799b77b2a6becfa2. Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> then the upstream cherry pick for the mac changing logic: commit 5d9c81e3aa1dd39dafd9a6ea30da05b26b655eca Author: Phil Elwell <phil@raspberrypi.org> Date: Thu Apr 19 17:59:38 2018 +0100 lan78xx: Read MAC address from DT if present There is a standard mechanism for locating and using a MAC address from the Device Tree. Use this facility in the lan78xx driver to support applications without programmed EEPROM or OTP. At the same time, regularise the handling of the different address sources. Signed-off-by: Phil Elwell <phil@raspberrypi.org> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 760db29bdc97b73ff60b091315ad787b1deb5cf5) Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> and finally revert the SAUCE patch that fixes lan78xx_read_otp(), and makes the led code work again: commit 24dc8f22b15cc983e13a0ae88b372dd1bfad09f9 Author: Paolo Pisati <paolo.pisati@canonical.com> Date: Thu Nov 8 10:37:17 2018 +0000 Revert "UBUNTU: SAUCE: Revert "lan78xx: Correctly indicate invalid OTP"" This reverts commit 1111a9e0bd2ebab88e736d8a9773df2ec76dc52c. Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com> See also the attached patches. How to test: Boot a patched kernel, connect an ethernet cable to the board and check that the ethernet leds are blinking when there's traffic Regression potential: None, we are dropping two raspberry BSP patches in favor of an upstream patch and re-applying another upstream fix: all patches have been upstream for awhile, and are clean cherry picks. Paolo Pisati (3): Revert "lan78xx: Ignore DT MAC address if already valid" Revert "lan78xx: Read MAC address from DT if present" Revert "UBUNTU: SAUCE: Revert "lan78xx: Correctly indicate invalid OTP"" Phil Elwell (1): lan78xx: Read MAC address from DT if present drivers/net/usb/lan78xx.c | 54 ++++++++++++++++++----------------------------- 1 file changed, 21 insertions(+), 33 deletions(-)