Hi Paul, Thanks for your comments. > -----Original Message----- > From: Paul Menzel <pmenzel@xxxxxxxxxxxxx> > Sent: Thursday, May 30, 2024 3:48 PM > To: Tommy Huang <tommy_huang@xxxxxxxxxxxxxx> > Cc: brendan.higgins@xxxxxxxxx; benh@xxxxxxxxxxxxxxxxxxx; joel@xxxxxxxxx; > andi.shyti@xxxxxxxxxx; andrew@xxxxxxxxxxxxxxxxxxxx; wsa@xxxxxxxxxx; > BMC-SW <BMC-SW@xxxxxxxxxxxxxx>; linux-aspeed@xxxxxxxxxxxxxxxx; > openbmc@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > linux-i2c@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] i2c: aspeed: Update the stop sw state when the bus > recovry occurs > > Dear Tommy, > > > Thank you for your patch. > > Am 30.05.24 um 09:06 schrieb Tommy Huang: > > When the i2c bus recovey occurs, driver will send i2c stop command > > recove*r*y I will fix this spelling typo on next patch. Thanks. > > > in the scl low condition. In this case the sw state will still keep > > original situation. Under multi-master usage, i2c bus recovery will > > What is the user visible problem? > In the multi-master case, the i2c transfer timeout will execute the aspeed_i2c_recover_bus. What we noticed when timeout appears, and kernel panic is observed: - xfer goes through aspeed_i2c_recovery_bus - inside recovery we can see “SCL hung (state ec0b0000), attempting recovery”. - STOP_CMD is performed but time_left = 0 what causes that aspeed_i2c_reset is called (and returns 0) - Going out of timeout handling, release memory, irq handler is invoked and rise interrupt causing that released memory is used. > > be called when i2c transfer timeout occurs. Update the stop command > > calling with aspeed_i2c_do_stop function to update master_state. > > How can this be tested? It just updates master_state for usage safety. The original stop command still be triggered. > > > Fixes: f327c686d3ba ("i2c: aspeed: added driver for Aspeed I2C") > > > > The blank line can be removed. > > > Signed-off-by: Tommy Huang <tommy_huang@xxxxxxxxxxxxxx> > > --- > > drivers/i2c/busses/i2c-aspeed.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/i2c-aspeed.c > > b/drivers/i2c/busses/i2c-aspeed.c index ce8c4846b7fa..32f8b0c1c174 > > 100644 > > --- a/drivers/i2c/busses/i2c-aspeed.c > > +++ b/drivers/i2c/busses/i2c-aspeed.c > > @@ -169,6 +169,7 @@ struct aspeed_i2c_bus { > > }; > > > > static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus); > > +static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus); > > > > static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus) > > { > > @@ -187,7 +188,7 @@ static int aspeed_i2c_recover_bus(struct > aspeed_i2c_bus *bus) > > command); > > > > reinit_completion(&bus->cmd_complete); > > - writel(ASPEED_I2CD_M_STOP_CMD, bus->base + > ASPEED_I2C_CMD_REG); > > + aspeed_i2c_do_stop(bus); > > spin_unlock_irqrestore(&bus->lock, flags); > > > > time_left = wait_for_completion_timeout( > > > Kind regards, > > Paul BR, Tommy