ACK: [PATCH 0/4] [SRU] [X/raspi2] RaspberryPi 3B+: fix ethernet leds

Tue Nov 13 17:26:20 UTC 2018

Whew, a long read, but seems entirely reasonable ...

Acked-by: Kamal Mostafa <kamal at 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 at 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 at canonical.com>
> 
> and
> 
> commit 6c8bdff882656296adc20b6ae0fb727483a73c7c
> Author: Paolo Pisati <paolo.pisati at 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 at canonical.com>
> 
> then the upstream cherry pick for the mac changing logic:
> 
> commit 5d9c81e3aa1dd39dafd9a6ea30da05b26b655eca
> Author: Phil Elwell <phil at 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 at raspberrypi.org>
>     Signed-off-by: David S. Miller <davem at davemloft.net>
>     (cherry picked from commit 760db29bdc97b73ff60b091315ad787b1deb5cf5)
>     Signed-off-by: Paolo Pisati <paolo.pisati at 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 at 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 at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team