Re: [PATCH maybe-7.2 1/3] hw/i2c: only schedule pending master when bus is idle

[Cédric Le Goater]

On 11/16/22 09:43, Klaus Jensen wrote:

From: Klaus Jensen <k.jen...@samsung.com>

It is not given that the current master will release the bus after a
transfer ends. Only schedule a pending master if the bus is idle.

Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
---
  hw/i2c/aspeed_i2c.c  |  2 ++
  hw/i2c/core.c        | 37 ++++++++++++++++++++++---------------
  include/hw/i2c/i2c.h |  2 ++
  3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c166fd20fa11..1f071a3811f7 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, 
uint64_t value)
          }
          SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
          aspeed_i2c_set_state(bus, I2CD_IDLE);
+
+        i2c_schedule_pending_master(bus->bus);

Shouldn't it be i2c_bus_release() ?

Thanks,

C.

      }

if (aspeed_i2c_bus_pkt_mode_en(bus)) {

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d4ba8146bffb..bed594fe599b 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -185,22 +185,39 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool 
is_recv)

void i2c_bus_master(I2CBus *bus, QEMUBH *bh)

  {
+    I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
+    node->bh = bh;
+
+    QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
+}
+
+void i2c_schedule_pending_master(I2CBus *bus)
+{
+    I2CPendingMaster *node;
+
      if (i2c_bus_busy(bus)) {
-        I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
-        node->bh = bh;
-
-        QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
+        /* someone is already controlling the bus; wait for it to release it */
+        return;
+    }

+ if (QSIMPLEQ_EMPTY(&bus->pending_masters)) {

          return;
      }

- bus->bh = bh;

+    node = QSIMPLEQ_FIRST(&bus->pending_masters);
+    bus->bh = node->bh;
+
+    QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
+    g_free(node);
+
      qemu_bh_schedule(bus->bh);
  }

void i2c_bus_release(I2CBus *bus)

  {
      bus->bh = NULL;
+
+    i2c_schedule_pending_master(bus);
  }

int i2c_start_recv(I2CBus *bus, uint8_t address)

@@ -234,16 +251,6 @@ void i2c_end_transfer(I2CBus *bus)
          g_free(node);
      }
      bus->broadcast = false;
-
-    if (!QSIMPLEQ_EMPTY(&bus->pending_masters)) {
-        I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters);
-        bus->bh = node->bh;
-
-        QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
-        g_free(node);
-
-        qemu_bh_schedule(bus->bh);
-    }
  }

int i2c_send(I2CBus *bus, uint8_t data)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 9b9581d23097..2a3abacd1ba6 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -141,6 +141,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
   */
  int i2c_start_send_async(I2CBus *bus, uint8_t address);

+void i2c_schedule_pending_master(I2CBus *bus);

+
  void i2c_end_transfer(I2CBus *bus);
  void i2c_nack(I2CBus *bus);
  void i2c_ack(I2CBus *bus);