I hit a similar problem that has a slightly different backtrace on a malfunctioning device. https://pastebin.com/TiWdkdrG With this patch, the kernel panic is gone and it gets below logs instead: aspeed-i2c-bus 1e78a180.i2c-bus: bus in unknown state. irq_status: 0x1 aspeed-i2c-bus 1e78a180.i2c-bus: irq handled != irq. expected 0x00000001, but was 0x00000000 aspeed-i2c-bus 1e78a180.i2c-bus: bus in unknown state. irq_status: 0x10 aspeed-i2c-bus 1e78a180.i2c-bus: irq handled != irq. expected 0x00000010, but was 0x00000000 So I think this patch is good in that it prevents the kernel panic. On Wed, Jan 19, 2022 at 11:00 AM Heyi Guo <guoheyi@xxxxxxxxxxxxxxxxx> wrote: > > > 在 2022/1/17 下午2:38, Joel Stanley 写道: > > On Fri, 14 Jan 2022 at 14:01, Heyi Guo <guoheyi@xxxxxxxxxxxxxxxxx> wrote: > >> Hi Joel, > >> > >> > >> 在 2022/1/11 下午6:51, Joel Stanley 写道: > >>> On Tue, 11 Jan 2022 at 07:52, Heyi Guo <guoheyi@xxxxxxxxxxxxxxxxx> wrote: > >>>> Hi all, > >>>> > >>>> Any comments? > >>>> > >>>> Thanks, > >>>> > >>>> Heyi > >>>> > >>>> 在 2022/1/9 下午9:26, Heyi Guo 写道: > >>>>> The memory will be freed by the caller if transfer timeout occurs, > >>>>> then it would trigger kernel panic if the peer device responds with > >>>>> something after timeout and triggers the interrupt handler of aspeed > >>>>> i2c driver. > >>>>> > >>>>> Set the msgs pointer to NULL to avoid invalid memory reference after > >>>>> timeout to fix this potential kernel panic. > >>> Thanks for the patch. How did you discover this issue? Do you have a > >>> test I can run to reproduce the crash? > >> We are using one i2c channel to communicate with another MCU by > >> implementing user space SSIF protocol, and the MCU may not respond in > >> time if it is busy. If it responds after timeout occurs, it will trigger > >> below kernel panic: > >> > > Thanks for the details. It looks like you've done some testing of > > this, which is good. > > > >> After applying this patch, we'll get below warning instead: > >> > >> "bus in unknown state. irq_status: 0x%x\n" > > Given we get to here in the irq handler, we've done these two tests: > > > > - aspeed_i2c_is_irq_error() > > - the state is not INACTIVE or PENDING > > > > but there's no buffer ready for us to use. So what has triggered the > > IRQ in this case? Do you have a record of the irq status bits? > > > > I am wondering if the driver should know that the transaction has > > timed out, instead of detecting this unknown state. > > Yes, some drivers will try to abort the transaction before returning to > the caller, if timeout happens. > > The irq status bits are not always the same; searching from the history > logs, I found some messages like below: > > [ 495.289499] aspeed-i2c-bus 1e78a680.i2c-bus: bus in unknown state. > irq_status: 0x2 > [ 495.298003] aspeed-i2c-bus 1e78a680.i2c-bus: bus in unknown state. > irq_status: 0x10 > > [ 65.176761] aspeed-i2c-bus 1e78a680.i2c-bus: bus in unknown state. > irq_status: 0x15 > > Thanks, > > Heyi > > > > > > >>> Can you provide a Fixes tag? > >> I think the bug was introduced by the first commit of this file :( > >> > >> f327c686d3ba44eda79a2d9e02a6a242e0b75787 > >> > >> > >>> Do other i2c master drivers do this? I took a quick look at the meson > >>> driver and it doesn't appear to clear it's pointer to msgs. > >> It is hard to say. It seems other drivers have some recover scheme like > >> aborting the transfer, or loop each messages in process context and > >> don't do much in IRQ handler, which may disable interrupts or not retain > >> the buffer pointer before returning timeout. > > I think your change is okay to go in as it fixes the crash, but first > > I want to work out if there's some missing handling of a timeout > > condition that we should add as well. > > > > > >> Thanks, > >> > >> Heyi > >> > >> > >>>>> Signed-off-by: Heyi Guo <guoheyi@xxxxxxxxxxxxxxxxx> > >>>>> > >>>>> ------- > >>>>> > >>>>> Cc: Brendan Higgins <brendanhiggins@xxxxxxxxxx> > >>>>> Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> > >>>>> Cc: Joel Stanley <joel@xxxxxxxxx> > >>>>> Cc: Andrew Jeffery <andrew@xxxxxxxx> > >>>>> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > >>>>> Cc: linux-i2c@xxxxxxxxxxxxxxx > >>>>> Cc: openbmc@xxxxxxxxxxxxxxxx > >>>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > >>>>> Cc: linux-aspeed@xxxxxxxxxxxxxxxx > >>>>> --- > >>>>> drivers/i2c/busses/i2c-aspeed.c | 5 +++++ > >>>>> 1 file changed, 5 insertions(+) > >>>>> > >>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > >>>>> index 67e8b97c0c950..3ab0396168680 100644 > >>>>> --- a/drivers/i2c/busses/i2c-aspeed.c > >>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c > >>>>> @@ -708,6 +708,11 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, > >>>>> spin_lock_irqsave(&bus->lock, flags); > >>>>> if (bus->master_state == ASPEED_I2C_MASTER_PENDING) > >>>>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE; > >>>>> + /* > >>>>> + * All the buffers may be freed after returning to caller, so > >>>>> + * set msgs to NULL to avoid memory reference after freeing. > >>>>> + */ > >>>>> + bus->msgs = NULL; > >>>>> spin_unlock_irqrestore(&bus->lock, flags); > >>>>> > >>>>> return -ETIMEDOUT; -- BRs, Lei YU
