[Qemu-devel] [PATCH] xlnx_dp: fix iffy xlnx_dp_aux_push_tx_fifo

[Qemu-devel] [PATCH] xlnx_dp: fix iffy xlnx_dp_aux_push_tx_fifo

[Paolo Bonzini]

xlnx_dp_aux_push_tx_fifo takes an immediate uint8_t and a buffer length,
which must be 1 because that is how many uint8_t's fit in a uint8_t.
Sure enough, that is what xlnx_dp_write passes to it, but the function
is just weird.  Therefore, make xlnx_dp_aux_push_tx_fifo look like
xlnx_dp_aux_push_rx_fifo, taking a pointer to the buffer.

Reported by Coverity.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/xlnx_dp.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index be53b75..f43eb09 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -438,10 +438,10 @@ static void xlnx_dp_aux_clear_tx_fifo(XlnxDPState *s)
     fifo8_reset(&s->tx_fifo);
 }
 
-static void xlnx_dp_aux_push_tx_fifo(XlnxDPState *s, uint8_t val, size_t len)
+static void xlnx_dp_aux_push_tx_fifo(XlnxDPState *s, uint8_t *buf, size_t len)
 {
     DPRINTF("Push %u data in tx_fifo\n", (unsigned)len);
-    fifo8_push_all(&s->tx_fifo, &val, len);
+    fifo8_push_all(&s->tx_fifo, buf, len);
 }
 
 static uint8_t xlnx_dp_aux_pop_tx_fifo(XlnxDPState *s)
@@ -806,9 +806,11 @@ static void xlnx_dp_write(void *opaque, hwaddr offset, uint64_t value,
          * TODO: Power down things?
          */
         break;
-    case DP_AUX_WRITE_FIFO:
-        xlnx_dp_aux_push_tx_fifo(s, value, 1);
+    case DP_AUX_WRITE_FIFO: {
+        uint8_t c = value;
+        xlnx_dp_aux_push_tx_fifo(s, &c, 1);
         break;
+    }
     case DP_AUX_CLOCK_DIVIDER:
         break;
     case DP_AUX_REPLY_COUNT:
-- 
2.5.5

[Qemu-devel] [PATCH] aux: fix break that wanted to break two levels out

[Paolo Bonzini]

The last "ret = AUX_I2C_NACK;" is dead, because it is always overridden
by AUX_I2C_ACK.  What really the code wants is to jump out of the switch
statement, and a "return" will not cut it because it would omit a debug
printf.

Change the logic so that we can break out of the while loop.  For clarity,
hoist the bus->last_* assignments up, right after i2c_start_transfer.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/misc/aux.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/misc/aux.c b/hw/misc/aux.c
index 25d7712..06e24ca 100644
--- a/hw/misc/aux.c
+++ b/hw/misc/aux.c
@@ -153,12 +153,12 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
     case WRITE_I2C_MOT:
     case READ_I2C_MOT:
         is_write = cmd == READ_I2C_MOT ? false : true;
+        ret = AUX_I2C_NACK;
         if (!i2c_bus_busy(i2c_bus)) {
             /*
              * No transactions started..
              */
             if (i2c_start_transfer(i2c_bus, address, is_write)) {
-                ret = AUX_I2C_NACK;
                 break;
             }
         } else if ((address != bus->last_i2c_address) ||
@@ -168,22 +168,22 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
              */
             i2c_end_transfer(i2c_bus);
             if (i2c_start_transfer(i2c_bus, address, is_write)) {
-                ret = AUX_I2C_NACK;
                 break;
             }
         }
 
+        bus->last_transaction = cmd;
+        bus->last_i2c_address = address;
         while (len > 0) {
             if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
-                ret = AUX_I2C_NACK;
                 i2c_end_transfer(i2c_bus);
                 break;
             }
             len--;
         }
-        bus->last_transaction = cmd;
-        bus->last_i2c_address = address;
-        ret = AUX_I2C_ACK;
+        if (len == 0) {
+            ret = AUX_I2C_ACK;
+        }
         break;
     default:
         DPRINTF("Not implemented!\n");
-- 
2.5.5

Re: [Qemu-devel] [PATCH] aux: fix break that wanted to break two levels out

[Peter Maydell]

On 21 June 2016 at 14:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The last "ret = AUX_I2C_NACK;" is dead, because it is always overridden
> by AUX_I2C_ACK.  What really the code wants is to jump out of the switch
> statement, and a "return" will not cut it because it would omit a debug
> printf.
>
> Change the logic so that we can break out of the while loop.  For clarity,
> hoist the bus->last_* assignments up, right after i2c_start_transfer.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/misc/aux.c | 12 ++++++------



Applied to target-arm.next, thanks.

-- PMM

Re: [Qemu-devel] [PATCH] xlnx_dp: fix iffy xlnx_dp_aux_push_tx_fifo

[KONRAD Frederic]

Le 21/06/2016 à 15:16, Paolo Bonzini a écrit :
> xlnx_dp_aux_push_tx_fifo takes an immediate uint8_t and a buffer length,
> which must be 1 because that is how many uint8_t's fit in a uint8_t.
> Sure enough, that is what xlnx_dp_write passes to it, but the function
> is just weird.  Therefore, make xlnx_dp_aux_push_tx_fifo look like
> xlnx_dp_aux_push_rx_fifo, taking a pointer to the buffer.
>
> Reported by Coverity.

Hi Paolo,

Seems ok to me.

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/display/xlnx_dp.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index be53b75..f43eb09 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -438,10 +438,10 @@ static void xlnx_dp_aux_clear_tx_fifo(XlnxDPState *s)
>      fifo8_reset(&s->tx_fifo);
>  }
>
> -static void xlnx_dp_aux_push_tx_fifo(XlnxDPState *s, uint8_t val, size_t len)
> +static void xlnx_dp_aux_push_tx_fifo(XlnxDPState *s, uint8_t *buf, size_t len)
>  {
>      DPRINTF("Push %u data in tx_fifo\n", (unsigned)len);
> -    fifo8_push_all(&s->tx_fifo, &val, len);
> +    fifo8_push_all(&s->tx_fifo, buf, len);
>  }
>
>  static uint8_t xlnx_dp_aux_pop_tx_fifo(XlnxDPState *s)
> @@ -806,9 +806,11 @@ static void xlnx_dp_write(void *opaque, hwaddr offset, uint64_t value,
>           * TODO: Power down things?
>           */
>          break;
> -    case DP_AUX_WRITE_FIFO:
> -        xlnx_dp_aux_push_tx_fifo(s, value, 1);
> +    case DP_AUX_WRITE_FIFO: {
> +        uint8_t c = value;
> +        xlnx_dp_aux_push_tx_fifo(s, &c, 1);
>          break;
> +    }

BTW do you need those braces here?

Thanks,
Fred

>      case DP_AUX_CLOCK_DIVIDER:
>          break;
>      case DP_AUX_REPLY_COUNT:
>

Re: [Qemu-devel] [PATCH] xlnx_dp: fix iffy xlnx_dp_aux_push_tx_fifo

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On 06/21/2016 08:09 AM, KONRAD Frederic wrote:
> 
> 
> Le 21/06/2016 à 15:16, Paolo Bonzini a écrit :
>> xlnx_dp_aux_push_tx_fifo takes an immediate uint8_t and a buffer length,
>> which must be 1 because that is how many uint8_t's fit in a uint8_t.
>> Sure enough, that is what xlnx_dp_write passes to it, but the function
>> is just weird.  Therefore, make xlnx_dp_aux_push_tx_fifo look like
>> xlnx_dp_aux_push_rx_fifo, taking a pointer to the buffer.
>>
>> Reported by Coverity.
> 

>> +    case DP_AUX_WRITE_FIFO: {
>> +        uint8_t c = value;
>> +        xlnx_dp_aux_push_tx_fifo(s, &c, 1);
>>          break;
>> +    }
> 
> BTW do you need those braces here?

Yes. The declaration of 'c' inside a case label causes (at least some
versions of) gcc to gripe, if it is not in a {} scope.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] xlnx_dp: fix iffy xlnx_dp_aux_push_tx_fifo

[KONRAD Frederic]

Le 22/06/2016 à 00:14, Eric Blake a écrit :
> On 06/21/2016 08:09 AM, KONRAD Frederic wrote:
>>
>>
>> Le 21/06/2016 à 15:16, Paolo Bonzini a écrit :
>>> xlnx_dp_aux_push_tx_fifo takes an immediate uint8_t and a buffer length,
>>> which must be 1 because that is how many uint8_t's fit in a uint8_t.
>>> Sure enough, that is what xlnx_dp_write passes to it, but the function
>>> is just weird.  Therefore, make xlnx_dp_aux_push_tx_fifo look like
>>> xlnx_dp_aux_push_rx_fifo, taking a pointer to the buffer.
>>>
>>> Reported by Coverity.
>>
>
>>> +    case DP_AUX_WRITE_FIFO: {
>>> +        uint8_t c = value;
>>> +        xlnx_dp_aux_push_tx_fifo(s, &c, 1);
>>>          break;
>>> +    }
>>
>> BTW do you need those braces here?
>
> Yes. The declaration of 'c' inside a case label causes (at least some
> versions of) gcc to gripe, if it is not in a {} scope.
>

Hi Eric,

Makes sense!

Thanks,
Fred

Re: [Qemu-devel] [PATCH] xlnx_dp: fix iffy xlnx_dp_aux_push_tx_fifo

[Peter Maydell]

On 21 June 2016 at 14:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> xlnx_dp_aux_push_tx_fifo takes an immediate uint8_t and a buffer length,
> which must be 1 because that is how many uint8_t's fit in a uint8_t.
> Sure enough, that is what xlnx_dp_write passes to it, but the function
> is just weird.  Therefore, make xlnx_dp_aux_push_tx_fifo look like
> xlnx_dp_aux_push_rx_fifo, taking a pointer to the buffer.
>
> Reported by Coverity.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/display/xlnx_dp.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)



Applied to target-arm.next, thanks.

-- PMM