Re: [PATCH v2 38/42] esp: convert ti_buf from array to Fifo8

[Philippe Mathieu-Daudé]

On 2/9/21 8:30 PM, Mark Cave-Ayland wrote:
> Rename TI_BUFSZ to ESP_FIFO_SZ since this constant is really describing the 
> size
> of the FIFO and is not directly related to the TI size.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c         | 117 ++++++++++++++++++++++++++----------------
>  include/hw/scsi/esp.h |   8 +--
>  2 files changed, 79 insertions(+), 46 deletions(-)

> @@ -806,11 +818,9 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t 
> val)
>              } else {
>                  trace_esp_error_fifo_overrun();
>              }
> -        } else if (s->ti_wptr == TI_BUFSZ - 1) {
> -            trace_esp_error_fifo_overrun();
>          } else {
>              s->ti_size++;
> -            s->ti_buf[s->ti_wptr++] = val & 0xff;
> +            esp_fifo_push(s, val & 0xff);

Personally I'd drop the '& 0xff' part.

>          }
>  
>          /* Non-DMA transfers raise an interrupt after every byte */
> @@ -839,8 +849,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t 
> val)
>          case CMD_FLUSH:
>              trace_esp_mem_writeb_cmd_flush(val);
>              /*s->ti_size = 0;*/

Is this comment still meaningful?

> -            s->ti_wptr = 0;
> -            s->ti_rptr = 0;
> +            fifo8_reset(&s->fifo);
>              break;
>          case CMD_RESET:
>              trace_esp_mem_writeb_cmd_reset(val);
> @@ -958,11 +967,18 @@ static int esp_pre_save(void *opaque)
>  static int esp_post_load(void *opaque, int version_id)
>  {
>      ESPState *s = ESP(opaque);
> +    int len, i;
>  
>      version_id = MIN(version_id, s->mig_version_id);
>  
>      if (version_id < 5) {
>          esp_set_tc(s, s->mig_dma_left);
> +
> +        /* Migrate ti_buf to fifo */
> +        len = s->mig_ti_wptr - s->mig_ti_rptr;
> +        for (i = 0; i < len; i++) {
> +            fifo8_push(&s->fifo, s->mig_ti_buf[i]);

Again I dare to add:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +        }
>      }