[PATCH v2 net] net: dsa: mv88e6xxx: Wait for EEPROM done before HW reset

[PATCH v2 net] net: dsa: mv88e6xxx: Wait for EEPROM done before HW reset

[Alfred Lee]

If the switch is reset during active EEPROM transactions, as in
just after an SoC reset after power up, the I2C bus transaction
may be cut short leaving the EEPROM internal I2C state machine
in the wrong state.  When the switch is reset again, the bad
state machine state may result in data being read from the wrong
memory location causing the switch to enter unexpect mode
rendering it inoperational.

Fixes: 8abbffd27ced ("net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset")
Signed-off-by: Alfred Lee <l00g33k@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c7d51a539451..7af2f08a62f1 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3034,6 +3034,14 @@ static void mv88e6xxx_hardware_reset(struct mv88e6xxx_chip *chip)
 
 	/* If there is a GPIO connected to the reset pin, toggle it */
 	if (gpiod) {
+		/* If the switch has just been reset and not yet completed
+		 * loading EEPROM, the reset may interrupt the I2C transaction
+		 * mid-byte, causing the first EEPROM read after the reset
+		 * from the wrong location resulting in the switch booting
+		 * to wrong mode and inoperable.
+		 */
+		mv88e6xxx_g1_wait_eeprom_done(chip);
+
 		gpiod_set_value_cansleep(gpiod, 1);
 		usleep_range(10000, 20000);
 		gpiod_set_value_cansleep(gpiod, 0);
-- 
2.41.0

Re: [PATCH v2 net] net: dsa: mv88e6xxx: Wait for EEPROM done before HW reset

[Vladimir Oltean]

Hi Alfred,

On Fri, Aug 11, 2023 at 04:28:32PM -0700, Alfred Lee wrote:
> If the switch is reset during active EEPROM transactions, as in
> just after an SoC reset after power up, the I2C bus transaction
> may be cut short leaving the EEPROM internal I2C state machine
> in the wrong state.  When the switch is reset again, the bad
> state machine state may result in data being read from the wrong
> memory location causing the switch to enter unexpect mode
> rendering it inoperational.
> 
> Fixes: 8abbffd27ced ("net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset")
> Signed-off-by: Alfred Lee <l00g33k@gmail.com>
> ---

I don't think you understand the meaning of the Fixes: tag. It is
supposed to point to the first commit where the issue being fixed
started being visible. But git show 8abbffd27ced shows:

commit 8abbffd27cedd0f89f69e5ee2ff6841ea01511eb
Author: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
Date:   Tue Jan 10 10:18:32 2023 +0000

    test/vsock: vsock_perf utility

    This adds utility to check vsock rx/tx performance.

    Usage as sender:
    ./vsock_perf --sender <cid> --port <port> --bytes <bytes to send>
    Usage as receiver:
    ./vsock_perf --port <port> --rcvlowat <SO_RCVLOWAT>

    Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
    Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
    Signed-off-by: Paolo Abeni <pabeni@redhat.com>

So:
1 - that has nothing to do with mv88e6xxx, so it cannot have introduced
    the issue here
2 - there is a mismatch between the blamed commit sha1sum and the blamed
    commit message. In fact, the blamed commit message is _your_ commit
    message, which is not correct.

Re: [PATCH v2 net] net: dsa: mv88e6xxx: Wait for EEPROM done before HW reset

[Andrew Lunn]

On Sun, Aug 13, 2023 at 01:58:04PM +0300, Vladimir Oltean wrote:
> Hi Alfred,
> 
> On Fri, Aug 11, 2023 at 04:28:32PM -0700, Alfred Lee wrote:
> > If the switch is reset during active EEPROM transactions, as in
> > just after an SoC reset after power up, the I2C bus transaction
> > may be cut short leaving the EEPROM internal I2C state machine
> > in the wrong state.  When the switch is reset again, the bad
> > state machine state may result in data being read from the wrong
> > memory location causing the switch to enter unexpect mode
> > rendering it inoperational.
> > 
> > Fixes: 8abbffd27ced ("net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset")
> > Signed-off-by: Alfred Lee <l00g33k@gmail.com>
> > ---
> 
> I don't think you understand the meaning of the Fixes: tag.

The subject looks correct, but the hash is wrong. The correct hash is
a3dcb3e7e70c72a68a79b30fc3a3adad5612731c.

Fixes: a3dcb3e7e70c ("net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset")

       Andrew

Re: [PATCH v2 net] net: dsa: mv88e6xxx: Wait for EEPROM done before HW reset

[Vladimir Oltean]

On Sun, Aug 13, 2023 at 05:09:14PM +0200, Andrew Lunn wrote:
> On Sun, Aug 13, 2023 at 01:58:04PM +0300, Vladimir Oltean wrote:
> > Hi Alfred,
> > 
> > On Fri, Aug 11, 2023 at 04:28:32PM -0700, Alfred Lee wrote:
> > > If the switch is reset during active EEPROM transactions, as in
> > > just after an SoC reset after power up, the I2C bus transaction
> > > may be cut short leaving the EEPROM internal I2C state machine
> > > in the wrong state.  When the switch is reset again, the bad
> > > state machine state may result in data being read from the wrong
> > > memory location causing the switch to enter unexpect mode
> > > rendering it inoperational.
> > > 
> > > Fixes: 8abbffd27ced ("net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset")
> > > Signed-off-by: Alfred Lee <l00g33k@gmail.com>
> > > ---
> > 
> > I don't think you understand the meaning of the Fixes: tag.
> 
> The subject looks correct, but the hash is wrong. The correct hash is
> a3dcb3e7e70c72a68a79b30fc3a3adad5612731c.
> 
> Fixes: a3dcb3e7e70c ("net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset")
> 
>        Andrew

Ok. I didn't notice the "before" vs "after". But the patch still needs
to be resubmitted with the right Fixes: tag.

Re: [PATCH v2 net] net: dsa: mv88e6xxx: Wait for EEPROM done before HW reset

[Simon Horman]

On Fri, Aug 11, 2023 at 04:28:32PM -0700, Alfred Lee wrote:
> If the switch is reset during active EEPROM transactions, as in
> just after an SoC reset after power up, the I2C bus transaction
> may be cut short leaving the EEPROM internal I2C state machine
> in the wrong state.  When the switch is reset again, the bad
> state machine state may result in data being read from the wrong
> memory location causing the switch to enter unexpect mode

nit: unexpect -> unexpected

> rendering it inoperational.
> 
> Fixes: 8abbffd27ced ("net: dsa: mv88e6xxx: Wait for EEPROM done after HW reset")
> Signed-off-by: Alfred Lee <l00g33k@gmail.com>