Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <[email protected]> ha scritto:
> On 01/02/2024 10:46, Paolo Bonzini wrote: > > > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland > > <[email protected]> wrote: > >> > >> Invert the logic so that the end of DMA transfer check becomes one that > checks > >> for TC == 0 in the from device path in do_dma_pdma_cb(). > >> > >> Signed-off-by: Mark Cave-Ayland <[email protected]> > >> --- > >> hw/scsi/esp.c | 24 +++++++++++------------- > >> 1 file changed, 11 insertions(+), 13 deletions(-) > >> > >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > >> index fecfef7c89..63c828c1b2 100644 > >> --- a/hw/scsi/esp.c > >> +++ b/hw/scsi/esp.c > >> @@ -602,21 +602,19 @@ static void do_dma_pdma_cb(ESPState *s) > >> return; > >> } > >> > >> - if (esp_get_tc(s) != 0) { > >> - /* Copy device data to FIFO */ > >> - len = MIN(s->async_len, esp_get_tc(s)); > >> - len = MIN(len, fifo8_num_free(&s->fifo)); > >> - fifo8_push_all(&s->fifo, s->async_buf, len); > >> - s->async_buf += len; > >> - s->async_len -= len; > >> - s->ti_size -= len; > >> - esp_set_tc(s, esp_get_tc(s) - len); > >> - return; > >> + if (esp_get_tc(s) == 0) { > >> + esp_lower_drq(s); > >> + esp_dma_done(s); > >> } > > > > I'm only doing a cursory review, but shouldn't there be a return here? > > > > Paolo > > (goes and looks) > > Possibly there should, but my guess is that adding the return at that > particular > point in time at the series broke one of my reference images. In > particular MacOS is > notorious for requesting data transfers of X len, and setting the TC > either too high > or too low and let the in-built OS recovery logic handle these cases. > Absolutely, just noticing that there is a functional change but the commit message showed it as a refactoring only. The fact that this is bisectable is quite insane, and I am not asking for any code changes. TBH I wouldn't have blamed you if you just sent a patch to create esp2.c and a patch to delete esp.c... Paolo The tricky part is that as per the cover note, making expected changes at > an earlier > point in the series tends to cause things to break. Another thing to bear > in mind is > that one of the main objectives of the series to completely remove all the > PDMA-specific callbacks including do_dma_pdma_cb(), so you'll see this > function > disappear completely in a later patch. > > It probably helps to compare the existing code at > https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/esp.c against > the version > from this series at > https://gitlab.com/mcayland/qemu/-/blob/esp-rework-v2/hw/scsi/esp.c to > get a feeling > where the series is going, as in order to keep my reference images > bisectable the > journey from start to finish occurs in a fairly roundabout way. > > >> - /* Partially filled a scsi buffer. Complete immediately. */ > >> - esp_lower_drq(s); > >> - esp_dma_done(s); > >> + /* Copy device data to FIFO */ > >> + len = MIN(s->async_len, esp_get_tc(s)); > >> + len = MIN(len, fifo8_num_free(&s->fifo)); > >> + fifo8_push_all(&s->fifo, s->async_buf, len); > >> + s->async_buf += len; > >> + s->async_len -= len; > >> + s->ti_size -= len; > >> + esp_set_tc(s, esp_get_tc(s) - len); > >> } > >> } > >> > >> -- > >> 2.39.2 > > > ATB, > > Mark. > >
