| Message ID | 20240530070656.3841066-1-tommy_huang@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: aspeed: Update the stop sw state when the bus recovry occurs | expand |
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 > 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? > 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? > Fixes: f327c686d3ba ("i2c: aspeed: added driver for Aspeed I2C") > The blank line can be removed. > Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com> > --- > 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
Hi Paul, Thanks for your comments. > -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: Thursday, May 30, 2024 3:48 PM > To: Tommy Huang <tommy_huang@aspeedtech.com> > Cc: brendan.higgins@linux.dev; benh@kernel.crashing.org; joel@jms.id.au; > andi.shyti@kernel.org; andrew@codeconstruct.com.au; wsa@kernel.org; > BMC-SW <BMC-SW@aspeedtech.com>; linux-aspeed@lists.ozlabs.org; > openbmc@lists.ozlabs.org; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org > 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@aspeedtech.com> > > --- > > 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
Hi Tommy, On Thu, May 30, 2024 at 03:06:56PM +0800, Tommy Huang wrote: > When the i2c bus recovey occurs, driver will send i2c stop command > in the scl low condition. In this case the sw state will still keep > original situation. Under multi-master usage, i2c bus recovery will > be called when i2c transfer timeout occurs. Update the stop command > calling with aspeed_i2c_do_stop function to update master_state. > > Fixes: f327c686d3ba ("i2c: aspeed: added driver for Aspeed I2C") > > Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com> Can you please add: Cc: <stable@vger.kernel.org> # v4.13+ > --- > 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); Can you please move aspeed_i2c_do_stop() on top? Doesn't make much sense to add the prototype here as there is no dependencies. It's different the case of aspeed_i2c_reset() because it needs aspeed_i2c_init(). > 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); The patch is good, though! Thanks, Andi
Hi Andi, > -----Original Message----- > From: Andi Shyti <andi.shyti@kernel.org> > Sent: Thursday, June 6, 2024 9:27 AM > To: Tommy Huang <tommy_huang@aspeedtech.com> > Cc: brendan.higgins@linux.dev; benh@kernel.crashing.org; joel@jms.id.au; > andrew@codeconstruct.com.au; wsa@kernel.org; linux-i2c@vger.kernel.org; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; BMC-SW > <BMC-SW@aspeedtech.com> > Subject: Re: [PATCH] i2c: aspeed: Update the stop sw state when the bus > recovry occurs > > Hi Tommy, > > On Thu, May 30, 2024 at 03:06:56PM +0800, Tommy Huang wrote: > > When the i2c bus recovey occurs, driver will send i2c stop command in > > the scl low condition. In this case the sw state will still keep > > original situation. Under multi-master usage, i2c bus recovery will be > > called when i2c transfer timeout occurs. Update the stop command > > calling with aspeed_i2c_do_stop function to update master_state. > > > > Fixes: f327c686d3ba ("i2c: aspeed: added driver for Aspeed I2C") > > > > Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com> > > Can you please add: > > Cc: <stable@vger.kernel.org> # v4.13+ Got it. I will add it. > > > --- > > 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); > > Can you please move aspeed_i2c_do_stop() on top? Doesn't make much sense > to add the prototype here as there is no dependencies. Sure. I will update it. > > It's different the case of aspeed_i2c_reset() because it needs aspeed_i2c_init(). > > > 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); > > The patch is good, though! > Thanks for your commects. > Thanks, > Andi
Hi Andi, There is a problem to move aspeed_i2c_do_stop() on top. This function is like with aspeed_i2c_reset function needs the aspeed_i2c_bus structure definition. BR, By Tommy > -----Original Message----- > From: Tommy Huang <tommy_huang@aspeedtech.com> > Sent: Saturday, June 8, 2024 12:38 PM > To: Andi Shyti <andi.shyti@kernel.org> > Cc: brendan.higgins@linux.dev; benh@kernel.crashing.org; joel@jms.id.au; > andrew@codeconstruct.com.au; wsa@kernel.org; linux-i2c@vger.kernel.org; > openbmc@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; BMC-SW > <BMC-SW@aspeedtech.com> > Subject: RE: [PATCH] i2c: aspeed: Update the stop sw state when the bus > recovry occurs > > Hi Andi, > > > -----Original Message----- > > From: Andi Shyti <andi.shyti@kernel.org> > > Sent: Thursday, June 6, 2024 9:27 AM > > To: Tommy Huang <tommy_huang@aspeedtech.com> > > Cc: brendan.higgins@linux.dev; benh@kernel.crashing.org; > > joel@jms.id.au; andrew@codeconstruct.com.au; wsa@kernel.org; > > linux-i2c@vger.kernel.org; openbmc@lists.ozlabs.org; > > linux-arm-kernel@lists.infradead.org; > > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org; BMC-SW > > <BMC-SW@aspeedtech.com> > > Subject: Re: [PATCH] i2c: aspeed: Update the stop sw state when the > > bus recovry occurs > > > > Hi Tommy, > > > > On Thu, May 30, 2024 at 03:06:56PM +0800, Tommy Huang wrote: > > > When the i2c bus recovey occurs, driver will send i2c stop command > > > in the scl low condition. In this case the sw state will still keep > > > original situation. Under multi-master usage, i2c bus recovery will > > > be called when i2c transfer timeout occurs. Update the stop command > > > calling with aspeed_i2c_do_stop function to update master_state. > > > > > > Fixes: f327c686d3ba ("i2c: aspeed: added driver for Aspeed I2C") > > > > > > Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com> > > > > Can you please add: > > > > Cc: <stable@vger.kernel.org> # v4.13+ > > Got it. I will add it. > > > > > > --- > > > 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); > > > > Can you please move aspeed_i2c_do_stop() on top? Doesn't make much > > sense to add the prototype here as there is no dependencies. > > Sure. I will update it. > > > > > It's different the case of aspeed_i2c_reset() because it needs > aspeed_i2c_init(). > > > > > 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); > > > > The patch is good, though! > > > > Thanks for your commects. > > > Thanks, > > Andi
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(
When the i2c bus recovey occurs, driver will send i2c stop command in the scl low condition. In this case the sw state will still keep original situation. Under multi-master usage, i2c bus recovery will be called when i2c transfer timeout occurs. Update the stop command calling with aspeed_i2c_do_stop function to update master_state. Fixes: f327c686d3ba ("i2c: aspeed: added driver for Aspeed I2C") Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com> --- drivers/i2c/busses/i2c-aspeed.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)