On 2/2/25 21:31, Andrew Jeffery wrote:
On Fri, 2025-01-31 at 16:29 -0600, Eddie James wrote:
For multimaster case, it's conceivable that an interrupt comes
in after the transfer times out and attempts to use bus messages
that have already been freed by the i2c core.
This description seems a little vague. Did you hit this case in
practice?
Yes. I no longer have the back trace but it's a null pointer access in
the interrupt handler. We had a certain system that would hit this under
certain conditions and this patch fixed it.
I can update the commit message with some more detail.
Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>
---
drivers/i2c/busses/i2c-aspeed.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/i2c/busses/i2c-aspeed.c
b/drivers/i2c/busses/i2c-aspeed.c
index 1550d3d552aed..e344dcc2233fe 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -731,6 +731,7 @@ static int aspeed_i2c_master_xfer(struct
i2c_adapter *adap,
* master command.
*/
spin_lock_irqsave(&bus->lock, flags);
+ bus->msgs = NULL;
It feels like there's buggy code elsewhere in the driver that this is
protecting (broken state machine)? I've had a look at the
aspeed_i2c_master_irq() implementation and can't see what though, as we
take an early-exit (before indexing into bus->msgs) if bus-
master_state is INACTIVE or PENDING.
Can you be a bit more specific about where the issue lies?
I'm sure the state machine isn't perfect, yea. The bad access can happen
like this: if a transfer times out while waiting for an interrupt, the
aspeed_i2c_master_xfer function will either reset the engine or recover
the bus, and exit ETIMEDOUT. Resetting the engine will turn off
interrupts, so we're safe. But recovering the bus doesn't turn off
interrupts, so after the function exits ETIMEDOUT, I assume what happens
is we get the interrupt for the previous transfer and try and access the
messages pointer, which the i2c core has already freed.
Thanks for looking!
Eddie
Andrew
if (bus->master_state == ASPEED_I2C_MASTER_PENDING)
bus->master_state =
ASPEED_I2C_MASTER_INACTIVE;
spin_unlock_irqrestore(&bus->lock, flags);
