Re: [Qemu-devel] [PATCH v9 07/13] xilinx_spips: Add support for RX discard and RX drain

[Peter Maydell]

On 26 November 2017 at 23:16, Francisco Iglesias
<address@hidden> 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 <address@hidden>
> Reviewed-by: Edgar E. Iglesias <address@hidden>
> Tested-by: Edgar E. Iglesias <address@hidden>

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