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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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.