| Message ID | 20240313085810.2655062-2-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | esp: avoid explicit setting of DRQ within ESP state machine | expand |
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) {
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.
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. >
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) {
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(-)