Re: [PATCH v3 03/11] esp: consolidate esp_cmdfifo_push() into esp_fifo_push()

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
> Each FIFO currently has its own push functions with the only difference being
> the capacity check. The original reason for this was that the fifo8
> implementation doesn't have a formal API for retrieving the FIFO capacity,
> however there are multiple examples within QEMU where the capacity field is
> accessed directly.

So the Fifo8 API is not complete / practical.

> Change esp_fifo_push() to access the FIFO capacity directly and then consolidate
> esp_cmdfifo_push() into esp_fifo_push().
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 26fe1dcb9d..16aaf8be93 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -98,16 +98,15 @@ void esp_request_cancelled(SCSIRequest *req)
>      }
>  }
>  
> -static void esp_fifo_push(ESPState *s, uint8_t val)
> +static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
>  {
> -    if (fifo8_num_used(&s->fifo) == ESP_FIFO_SZ) {
> +    if (fifo8_num_used(fifo) == fifo->capacity) {
>          trace_esp_error_fifo_overrun();
>          return;
>      }
>  
> -    fifo8_push(&s->fifo, val);
> +    fifo8_push(fifo, val);
>  }
> -

Spurious line removal?

>  static uint8_t esp_fifo_pop(ESPState *s)
>  {
>      if (fifo8_is_empty(&s->fifo)) {
> @@ -117,16 +116,6 @@ static uint8_t esp_fifo_pop(ESPState *s)
>      return fifo8_pop(&s->fifo);
>  }

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>