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

[Philippe Mathieu-Daudé]

On 4/1/21 10:50 AM, Mark Cave-Ayland wrote:
> 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() :)

Sorry, I should have been more explicit by precising this was not
a comment directed to your patch, but I was thinking loudly about
the Fifo8 API; and as you mentioned the other models do that so your
kludge is acceptable.
>>> 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.

Actually it reappears in patch 05/11 ;)