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.
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);
}
-
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);
}
-static void esp_cmdfifo_push(ESPState *s, uint8_t val)
-{
- if (fifo8_num_used(&s->cmdfifo) == ESP_CMDFIFO_SZ) {
- trace_esp_error_fifo_overrun();
- return;
- }
-
- fifo8_push(&s->cmdfifo, val);
-}
-
static uint8_t esp_cmdfifo_pop(ESPState *s)
{
if (fifo8_is_empty(&s->cmdfifo)) {
@@ -187,9 +176,9 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
}
if (s->do_cmd) {
- esp_cmdfifo_push(s, val);
+ esp_fifo_push(&s->cmdfifo, val);
} else {
- esp_fifo_push(s, val);
+ esp_fifo_push(&s->fifo, val);
}
dmalen--;
@@ -645,7 +634,7 @@ static void esp_do_dma(ESPState *s)
*/
if (len < esp_get_tc(s) && esp_get_tc(s) <= ESP_FIFO_SZ) {
while (fifo8_num_used(&s->fifo) < ESP_FIFO_SZ) {
- esp_fifo_push(s, 0);
+ esp_fifo_push(&s->fifo, 0);
len++;
}
}
@@ -947,9 +936,9 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
break;
case ESP_FIFO:
if (s->do_cmd) {
- esp_cmdfifo_push(s, val);
+ esp_fifo_push(&s->cmdfifo, val);
} else {
- esp_fifo_push(s, val);
+ esp_fifo_push(&s->fifo, val);
}
/* Non-DMA transfers raise an interrupt after every byte */
--
2.20.1
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>
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.
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 ;)
© 2016 - 2026 Red Hat, Inc.