p8-i2c: Fix race between OCC interrupt and recovery timer

Message ID 20171012070309.23714-1-oohall@gmail.com
State Superseded
Headers show
Series
  • p8-i2c: Fix race between OCC interrupt and recovery timer
Related show

Commit Message

Oliver O'Halloran Oct. 12, 2017, 7:03 a.m.
When we request ownership of an I2C bus from the OCC we wait until the
OCC notifies us the bus is available with an interrupt. The wait time is
limited by the recovery timer duration which will cancel the pending
transaction if the OCC takes too long to hand ownership of the bus to
OPAL.

There is a race between the timeout function and the OCC interrupt
handler which can result in a deadlock when the two are run
concurrently. If both are running and the OCC interrupt aquires
the I2C master lock first it will wait attempt to cancel the recovery
timer which waits for the timeout handler function to finish if it's
already running. The problem is that the recovery handler will also
attempt to aquire the I2C master lock which results in both threads
waiting for each other to finish.

Fix this by using cancel_timer_async() which doesn not wait for the
timeout handle function to finish running.

Reported-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 hw/p8-i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stewart Smith Oct. 16, 2017, 8:04 a.m. | #1
Oliver O'Halloran <oohall@gmail.com> writes:
> When we request ownership of an I2C bus from the OCC we wait until the
> OCC notifies us the bus is available with an interrupt. The wait time is
> limited by the recovery timer duration which will cancel the pending
> transaction if the OCC takes too long to hand ownership of the bus to
> OPAL.
>
> There is a race between the timeout function and the OCC interrupt
> handler which can result in a deadlock when the two are run
> concurrently. If both are running and the OCC interrupt aquires
> the I2C master lock first it will wait attempt to cancel the recovery
> timer which waits for the timeout handler function to finish if it's
> already running. The problem is that the recovery handler will also
> attempt to aquire the I2C master lock which results in both threads
> waiting for each other to finish.
>
> Fix this by using cancel_timer_async() which doesn not wait for the
> timeout handle function to finish running.
>
> Reported-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
>  hw/p8-i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged the one Anton sent (exactly the same patch) as of
4466a6ec89bf39afeb98b92d03bbf7fc30c55afe

Patch

diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c
index ffbace936a9f..0defe7b2a2d2 100644
--- a/hw/p8-i2c.c
+++ b/hw/p8-i2c.c
@@ -1307,7 +1307,7 @@  void p9_i2c_bus_owner_change(u32 chip_id)
 			goto done;
 
 		/* clear the existing wait timer */
-		cancel_timer(&master->recovery);
+		cancel_timer_async(&master->recovery);
 
 		/* re-start the request now that we own the master */
 		master->state = state_idle;