| Message ID | 1466514977-12670-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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: >
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.
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
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
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:
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(-)