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

[Klaus Jensen]

On Nov 17 14:40, Cédric Le Goater wrote:
> On 11/17/22 12:58, Klaus Jensen wrote:
> > On Nov 17 09:01, Cédric Le Goater wrote:
> > > On 11/17/22 08:37, Klaus Jensen wrote:
> > > > On Nov 17 07:56, Cédric Le Goater wrote:
> > > > > On 11/17/22 07:40, Klaus Jensen wrote:
> > > > > > On Nov 16 16:58, Cédric Le Goater wrote:
> > > > > > > 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() ?
> > > > > > > 
> > > > > > 
> > > > > > The reason for having both i2c_bus_release() and
> > > > > > i2c_schedule_pending_master() is that i2c_bus_release() sort of 
> > > > > > pairs
> > > > > > with i2c_bus_master(). They either set or clear the bus->bh member.
> > > > > > 
> > > > > > In the current design, the controller (in this case the Aspeed I2C) 
> > > > > > is
> > > > > > an "implicit" master (it does not have a bottom half driving it), so
> > > > > > there is no bus->bh to clear.
> > > > > > 
> > > > > > I should (and will) write some documentation on the asynchronous 
> > > > > > API.
> > > > > 
> > > > > I found the routine names confusing. Thanks for the clarification.
> > > > > 
> > > > > Maybe we could do this rename  :
> > > > > 
> > > > >     i2c_bus_release()             -> i2c_bus_release_and_clear()
> > > > >     i2c_schedule_pending_master() -> i2c_bus_release()
> > > > > 
> > > > > and keep i2c_schedule_pending_master() internal the I2C core 
> > > > > subsystem.
> > > > > 
> > > > 
> > > > How about renaming i2c_bus_master to i2c_bus_acquire() such that it
> > > > pairs with i2c_bus_release().
> > > 
> > > Looks good to me.
> > > 
> > > > And then add an i2c_bus_yield() to be used by the controller? I think we
> > > > should be able to assert in i2c_bus_yield() that bus->bh is NULL. But
> > > > I'll take a closer look at that.
> > > 
> > > I am using your i2c-echo slave device to track regressions in the Aspeed
> > > machines. May be we could merge it for tests ?
> > > 
> > 
> > Oh, cool.
> > 
> > Sure, I'd be happy to help "maintain" it ;)
> 
> And so, I am seeing errors with the little POC you sent.
> 
> without:
>   console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
>   console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
>   console: [    4.252431] i2c i2c-3: new_device: Instantiated device 
> slave-24c02 at 0x64
>   console: i2cset -y 3 0x42 0x64 0x00 0xaa i
>   /console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
>   console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
>   console: 0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
>   console: poweroff
>   console: 0000010 ffff ffff ffff ffff ffff ffff ffff ffff
>   console: *
>   console: 0000100
> 
> with:
>   console: echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
>   console: # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device
>   console: [    4.413210] i2c i2c-3: new_device: Instantiated device 
> slave-24c02 at 0x64
>   console: i2cset -y 3 0x42 0x64 0x00 0xaa i
>   console: # i2cset -y 3 0x42 0x64 0x00 0xaa i
>   console: # hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom
>   console: 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
>   console: *
>   console: 0000100
> C.

Right.

This is because the i2c-echo device is scheduling the bottom half
initially on its own. What happens is that the bottom half gets queued
up in the pending masters list instead of being scheduling directly. And
since the i2c controller is idle, the bottom half is never scheduled.

Fixing i2c_bus_acquire() to schedulue it directly if the bus is free
seems the proper way to do it. I'll include that in v2.

While it is not directly invalid, the echo device should be fixed to
better align with the api, like so:

--- hw/misc/i2c-echo.c.orig     2022-11-22 09:35:00.478173652 +0100
+++ hw/misc/i2c-echo.c  2022-11-22 09:34:31.428174379 +0100
@@ -9,7 +9,6 @@

 enum i2c_echo_state {
     I2C_ECHO_STATE_IDLE,
-    I2C_ECHO_STATE_REQUEST_MASTER,
     I2C_ECHO_STATE_START_SEND,
     I2C_ECHO_STATE_ACK,
 };
@@ -34,11 +33,6 @@
     case I2C_ECHO_STATE_IDLE:
         return;

-    case I2C_ECHO_STATE_REQUEST_MASTER:
-        i2c_bus_acquire(state->bus, state->bh);
-        state->state = I2C_ECHO_STATE_START_SEND;
-        return;
-
     case I2C_ECHO_STATE_START_SEND:
         if (i2c_start_send_async(state->bus, state->data[0])) {
             goto release_bus;
@@ -85,8 +79,8 @@

     case I2C_FINISH:
         state->pos = 0;
-        state->state = I2C_ECHO_STATE_REQUEST_MASTER;
-        qemu_bh_schedule(state->bh);
+        state->state = I2C_ECHO_STATE_START_SEND;
+        i2c_bus_acquire(state->bus, state->bh);

         break;

signature.asc
Description: PGP signature