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.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.
>
> 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);

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);