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-ayl...@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 ;)
