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

[Mark Cave-Ayland]

On 01/04/2021 09:15, Philippe Mathieu-Daudé wrote:

> 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.

I guess it depends what you consider to be public and private to Fifo8: what is 
arguably missing is something like:

int fifo8_capacity(Fifo8 *fifo)
{
    return fifo->capacity;
}

But given that most other users access fifo->capacity directly then I might as well 
do the same and save myself requiring 2 separate implementations of esp_fifo_pop_buf() :)

> > 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?

Ooops yes. I'm not too worried about this, but if Paolo has any other changes then I 
can roll this into a v4.

> >   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>


ATB,

Mark.