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

[Philippe Mathieu-Daudé]

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.