diff mbox series

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

Message ID 20221116084312.35808-2-its@irrelevant.dk
State New
Headers show
Series hw/{i2c, nvme}: mctp endpoint, nvme management interface model | expand

Commit Message

Klaus Jensen Nov. 16, 2022, 8:43 a.m. UTC
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(-)

Comments

Peter Maydell Nov. 16, 2022, 1:16 p.m. UTC | #1
On Wed, 16 Nov 2022 at 08:43, Klaus Jensen <its@irrelevant.dk> 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>

If this is a bug fix we should consider for 7.2, you should
send it as a separate patch with a commit message that
describes the consequences of the bug (e.g. whether it
affects common workloads or if it's just an odd corner case).
As one patch in an otherwise RFC series it's going to get lost
otherwise.

thanks
-- PMM
Corey Minyard Nov. 16, 2022, 1:43 p.m. UTC | #2
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
> 
>
Cédric Le Goater Nov. 16, 2022, 3:58 p.m. UTC | #3
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);
Klaus Jensen Nov. 17, 2022, 6:40 a.m. UTC | #4
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.
Cédric Le Goater Nov. 17, 2022, 6:56 a.m. UTC | #5
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.

C.
Klaus Jensen Nov. 17, 2022, 7:37 a.m. UTC | #6
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().

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.
Cédric Le Goater Nov. 17, 2022, 8:01 a.m. UTC | #7
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 ?

Thanks,

C.
Klaus Jensen Nov. 17, 2022, 11:58 a.m. UTC | #8
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 ;)
Cédric Le Goater Nov. 17, 2022, 1:40 p.m. UTC | #9
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.
Klaus Jensen Nov. 18, 2022, 6:59 a.m. UTC | #10
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.

Interesting,

I'll check it out.
Klaus Jensen Nov. 22, 2022, 8:45 a.m. UTC | #11
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;
diff mbox series

Patch

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