Re: [Qemu-devel] [PATCH] hw/i2c/bitbang_i2c: Handle NACKs from devices

[Peter Maydell]

Ping? This is a bugfix and a coverity warning fix so it
would be nice if it could go into 2.8.

thanks
-- PMM

On 24 October 2016 at 19:12, Peter Maydell <address@hidden> wrote:
> If the guest attempts to talk to a nonexistent device over i2c,
> the i2c_start_transfer() function will return non-zero, indicating
> that the bus is signalling a NACK. Similarly, if the i2c_send()
> function returns nonzero then the target device returned a NACK.
> Handle this possibility in the bitbang_i2c code, by returning
> the state machine to the STOPPED state and returning the NACK
> bit to the guest.
>
> This bit of missing functionality was spotted by Coverity
> (it noticed that we weren't checking the return value from
> i2c_start_transfer()).
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> Lightly tested using the musicpal board, which is the only one
> using the bitbang_i2c code. If you make the board put the
> Wolfson 8750 at the wrong I2C address then the guest will
> retry the transaction a few times and give up, as expected.
> Without this patch it will send a bunch of data out into
> the void without realizing there's a problem.
> ---
>  hw/i2c/bitbang_i2c.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i2c/bitbang_i2c.c b/hw/i2c/bitbang_i2c.c
> index d3a2989..8be88ee 100644
> --- a/hw/i2c/bitbang_i2c.c
> +++ b/hw/i2c/bitbang_i2c.c
> @@ -130,14 +130,25 @@ int bitbang_i2c_set(bitbang_i2c_interface *i2c, int 
> line, int level)
>          return bitbang_i2c_ret(i2c, 1);
>
>      case WAITING_FOR_ACK:
> +    {
> +        int ret;
> +
>          if (i2c->current_addr < 0) {
>              i2c->current_addr = i2c->buffer;
>              DPRINTF("Address 0x%02x\n", i2c->current_addr);
> -            i2c_start_transfer(i2c->bus, i2c->current_addr >> 1,
> -                               i2c->current_addr & 1);
> +            ret = i2c_start_transfer(i2c->bus, i2c->current_addr >> 1,
> +                                     i2c->current_addr & 1);
>          } else {
>              DPRINTF("Sent 0x%02x\n", i2c->buffer);
> -            i2c_send(i2c->bus, i2c->buffer);
> +            ret = i2c_send(i2c->bus, i2c->buffer);
> +        }
> +        if (ret) {
> +            /* NACK (either addressing a nonexistent device, or the
> +             * device we were sending to decided to NACK us).
> +             */
> +            DPRINTF("Got NACK\n");
> +            bitbang_i2c_enter_stop(i2c);
> +            return bitbang_i2c_ret(i2c, 1);
>          }
>          if (i2c->current_addr & 1) {
>              i2c->state = RECEIVING_BIT7;
> @@ -145,7 +156,7 @@ int bitbang_i2c_set(bitbang_i2c_interface *i2c, int line, 
> int level)
>              i2c->state = SENDING_BIT7;
>          }
>          return bitbang_i2c_ret(i2c, 0);
> -
> +    }
>      case RECEIVING_BIT7:
>          i2c->buffer = i2c_recv(i2c->bus);
>          DPRINTF("RX byte 0x%02x\n", i2c->buffer);
> --
> 2.7.4
>
>