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

[Corey Minyard <minyard@acm.org>]

On Wed, Nov 16, 2022 at 09:43:10AM +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@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.
> 

Yes, I think this is correct.

Acked-by: Corey Minyard <cminyard@mvista.com>

Is there a reason you are thinking this is needed for 7.2?  There's no
code in qemu proper that uses this yet.  I had assumed that was coming
soon after the patch.

-corey

> Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
> Signed-off-by: Klaus Jensen <k.jensen@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);
>      }
>  
>      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);
> -- 
> 2.38.1
> 
>