1. Patchew
  2. QEMU
  3. [PATCH v2 00/16] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine
  4. View patch

[PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()

Mark Cave-Ayland posted 16 patches 1 year, 11 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>
There is a newer version of this series
[PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()
Posted by Mark Cave-Ayland 1 year, 11 months ago 
The aim is to restrict the esp_fifo_*() functions so that they only operate on
the hardware FIFO. When reading from cmdfifo in do_command_phase() use the
underlying Fifo8 functions directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 590ff99744..f8230c74b3 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -265,7 +265,7 @@ static void esp_do_nodma(ESPState *s);
 
 static void do_command_phase(ESPState *s)
 {
-    uint32_t cmdlen;
+    uint32_t cmdlen, n;
     int32_t datalen;
     SCSIDevice *current_lun;
     uint8_t buf[ESP_CMDFIFO_SZ];
@@ -275,7 +275,7 @@ static void do_command_phase(ESPState *s)
     if (!cmdlen || !s->current_dev) {
         return;
     }
-    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
+    memcpy(buf, fifo8_pop_buf(&s->cmdfifo, cmdlen, &n), cmdlen);
 
     current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun);
     if (!current_lun) {
-- 
2.39.2
Re: [PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()
Posted by Philippe Mathieu-Daudé 1 year, 11 months ago 
On 13/3/24 09:57, Mark Cave-Ayland wrote:
> The aim is to restrict the esp_fifo_*() functions so that they only operate on
> the hardware FIFO. When reading from cmdfifo in do_command_phase() use the
> underlying Fifo8 functions directly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 590ff99744..f8230c74b3 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -265,7 +265,7 @@ static void esp_do_nodma(ESPState *s);
>   
>   static void do_command_phase(ESPState *s)
>   {
> -    uint32_t cmdlen;
> +    uint32_t cmdlen, n;
>       int32_t datalen;
>       SCSIDevice *current_lun;
>       uint8_t buf[ESP_CMDFIFO_SZ];
> @@ -275,7 +275,7 @@ static void do_command_phase(ESPState *s)
>       if (!cmdlen || !s->current_dev) {
>           return;
>       }
> -    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
> +    memcpy(buf, fifo8_pop_buf(&s->cmdfifo, cmdlen, &n), cmdlen);

'n' is unused, use NULL?

>   
>       current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun);
>       if (!current_lun) {
Re: [PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()
Posted by Mark Cave-Ayland 1 year, 11 months ago 
On 13/03/2024 11:03, Philippe Mathieu-Daudé wrote:

> On 13/3/24 09:57, Mark Cave-Ayland wrote:
>> The aim is to restrict the esp_fifo_*() functions so that they only operate on
>> the hardware FIFO. When reading from cmdfifo in do_command_phase() use the
>> underlying Fifo8 functions directly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 590ff99744..f8230c74b3 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -265,7 +265,7 @@ static void esp_do_nodma(ESPState *s);
>>   static void do_command_phase(ESPState *s)
>>   {
>> -    uint32_t cmdlen;
>> +    uint32_t cmdlen, n;
>>       int32_t datalen;
>>       SCSIDevice *current_lun;
>>       uint8_t buf[ESP_CMDFIFO_SZ];
>> @@ -275,7 +275,7 @@ static void do_command_phase(ESPState *s)
>>       if (!cmdlen || !s->current_dev) {
>>           return;
>>       }
>> -    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
>> +    memcpy(buf, fifo8_pop_buf(&s->cmdfifo, cmdlen, &n), cmdlen);
> 
> 'n' is unused, use NULL?

I was sure I had tried that before and it had failed, but I see that you made it work 
with NULL in commit cd04033dbe ("util/fifo8: Allow fifo8_pop_buf() to not populate 
popped length") - thanks!

I'll make the change for v3, but I'll wait a couple of days first to see if there are 
any further comments, in particular R-B tags for patches 10 and 11.

>>       current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun);
>>       if (!current_lun) {


ATB,

Mark.
Re: [PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()
Posted by Philippe Mathieu-Daudé 1 year, 11 months ago 
On 13/3/24 22:08, Mark Cave-Ayland wrote:
> On 13/03/2024 11:03, Philippe Mathieu-Daudé wrote:
> 
>> On 13/3/24 09:57, Mark Cave-Ayland wrote:
>>> The aim is to restrict the esp_fifo_*() functions so that they only 
>>> operate on
>>> the hardware FIFO. When reading from cmdfifo in do_command_phase() 
>>> use the
>>> underlying Fifo8 functions directly.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/scsi/esp.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index 590ff99744..f8230c74b3 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -265,7 +265,7 @@ static void esp_do_nodma(ESPState *s);
>>>   static void do_command_phase(ESPState *s)
>>>   {
>>> -    uint32_t cmdlen;
>>> +    uint32_t cmdlen, n;
>>>       int32_t datalen;
>>>       SCSIDevice *current_lun;
>>>       uint8_t buf[ESP_CMDFIFO_SZ];
>>> @@ -275,7 +275,7 @@ static void do_command_phase(ESPState *s)
>>>       if (!cmdlen || !s->current_dev) {
>>>           return;
>>>       }
>>> -    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
>>> +    memcpy(buf, fifo8_pop_buf(&s->cmdfifo, cmdlen, &n), cmdlen);
>>
>> 'n' is unused, use NULL?
> 
> I was sure I had tried that before and it had failed, but I see that you 
> made it work with NULL in commit cd04033dbe ("util/fifo8: Allow 
> fifo8_pop_buf() to not populate popped length") - thanks!

Ah!

So using NULL in patches 1 and 2:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> I'll make the change for v3, but I'll wait a couple of days first to see 
> if there are any further comments, in particular R-B tags for patches 10 
> and 11.

I still have them in my TOREVIEW queue and need to digest them.

> 
>>>       current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, 
>>> s->lun);
>>>       if (!current_lun) {
> 
> 
> ATB,
> 
> Mark.
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.