Hi Peter,
I looked at the coverity output and below are my comments. Please do
correct me if I misunderstood something!
----
CID 1383841 (#1 of 4): Uninitialized scalar variable (UNINIT)26.
uninit_use_in_call: Using uninitialized element of array tx_rx when calling
stripe8.
'num_effective_busses' can only return 1 or 2 so the foor loop initializing
the array should always be executed, meaning that above shouldn't happen as
I see it.
----
CID 1383841 (#2 of 4): Uninitialized scalar variable (UNINIT)29.
uninit_use_in_call: Using uninitialized value (uint32_t)tx_rx[0] when
calling
ssi_transfer.
This is correct, tx_rx is used uninitialized but since we are transmitting
dummy cycles the transmitted value (tx_rx[0] in this case) is not used (by
the flashes), this the reason the code looks like that. Would you like me
to create a patch for quieting coverity here anyway?
----
CID 1383841 (#3 of 4): Uninitialized scalar variable (UNINIT)29.
uninit_use_in_call: Using uninitialized value (uint32_t)tx_rx[i] when
calling
ssi_transfer.
This occurs if the last 'else' of the first 'if' ladder is executed and
dummy_cycles is zero, something that shouldn't happen either. 's->link
state' is 1, 2 or 4 so dummy_cycles will always be greater than zero if the
'else' is executed.
----
CID 1383841 (#4 of 4): Uninitialized scalar variable (UNINIT)34.
uninit_use_in_call: Using uninitialized value (uint8_t)tx_rx[0] when calling
fifo8_push.
Since num_effective_busses is always greater than 0, the foor loop in the
middle (with the ssi_transfer calls) always initializes tx_rx[0] before
this access. So this one can't happen either.
----
All together #1, #3, #4 seem to be false positives what I can see, #2 was
known and left intentionaly like that (since tx_rx value doesn't matter)
but can be remade for quieting coverity if wished.
Best regards,
Francisco Iglesias
On 11 January 2018 at 14:16, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 26 November 2017 at 23:16, Francisco Iglesias
> <frasse.igles...@gmail.com> wrote:
> > Add support for the RX discard and RX drain functionality. Also transmit
> > one byte per dummy cycle (to the flash memories) with commands that
> require
> > these.
> >
> > Signed-off-by: Francisco Iglesias <frasse.igles...@gmail.com>
> > Reviewed-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
> > Tested-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
>
> Hi. Coverity (CID 1383841) complains that this change introduces
> a use of an uninitialized local variable:
>
>
> > static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
> > {
> > int debug_level = 0;
> > + XilinxQSPIPS *q = (XilinxQSPIPS *) object_dynamic_cast(OBJECT(s),
> > +
> TYPE_XILINX_QSPIPS);
> >
> > for (;;) {
> > int i;
> > uint8_t tx = 0;
> > uint8_t tx_rx[num_effective_busses(s)];
>
> tx_rx[] is the variable in question.
>
> > + uint8_t dummy_cycles = 0;
> > + uint8_t addr_length;
> >
> > if (fifo8_is_empty(&s->tx_fifo)) {
> > if (!(s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE)) {
> > @@ -258,54 +336,102 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS
> *s)
> > tx_rx[i] = fifo8_pop(&s->tx_fifo);
> > }
> > stripe8(tx_rx, num_effective_busses(s), false);
> > - } else {
> > + } else if (s->snoop_state >= SNOOP_ADDR) {
> > tx = fifo8_pop(&s->tx_fifo);
> > for (i = 0; i < num_effective_busses(s); ++i) {
> > tx_rx[i] = tx;
> > }
> > + } else {
> > + /* Extract a dummy byte and generate dummy cycles according
> to the
> > + * link state */
> > + tx = fifo8_pop(&s->tx_fifo);
> > + dummy_cycles = 8 / s->link_state;
> > }
>
> Before this if...else ladder was changed, each branch of it
> filled in the whole tx_rx[] array. But the new else {} case
> does not do that...
>
> > for (i = 0; i < num_effective_busses(s); ++i) {
> > int bus = num_effective_busses(s) - 1 - i;
> > - DB_PRINT_L(debug_level, "tx = %02x\n", tx_rx[i]);
> > - tx_rx[i] = ssi_transfer(s->spi[bus], (uint32_t)tx_rx[i]);
> > - DB_PRINT_L(debug_level, "rx = %02x\n", tx_rx[i]);
> > + if (dummy_cycles) {
> > + int d;
> > + for (d = 0; d < dummy_cycles; ++d) {
> > + tx_rx[0] = ssi_transfer(s->spi[bus],
> (uint32_t)tx_rx[0]);
> > + }
> > + } else {
> > + DB_PRINT_L(debug_level, "tx = %02x\n", tx_rx[i]);
> > + tx_rx[i] = ssi_transfer(s->spi[bus],
> (uint32_t)tx_rx[i]);
> > + DB_PRINT_L(debug_level, "rx = %02x\n", tx_rx[i]);
> > + }
>
> ...and subsequent code, both introduced in this patch and code
> already present, will read the tx_rx[] array.
>
> Could one of you have a look at this and determine what the right
> fix is, please?
>
> thanks
> -- PMM
>
