On Thu, Feb 8, 2024 at 10:46 AM Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> wrote: > > On 01/02/2024 11:36, Paolo Bonzini wrote: > > > Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk > > <mailto:mark.cave-ayl...@ilande.co.uk>> ha scritto: > > > > On 01/02/2024 10:46, Paolo Bonzini wrote: > > > > > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland > > > <mark.cave-ayl...@ilande.co.uk > > <mailto:mark.cave-ayl...@ilande.co.uk>> 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 <mark.cave-ayl...@ilande.co.uk > > <mailto:mark.cave-ayl...@ilande.co.uk>> > > >> --- > > >> 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. > > Would you like me to come up with a reworded commit message here?
If you want to change it, it's okay because len is zero at this point and the code after the "if" therefore does nothing. And the "if" will become esp_dma_ti_check() so adding a return is kind of messy here. > > 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... > > Heh... spending a chunk of time rewriting the emulation of a 30 year-old SCSI > controller is probably an odd choice, but the result is something that is > considerably more maintainable than the current implementation. I absolutely agree, you went above and beyond and sorry if it wasn't clear from the joke. > Anything else that you'd like me to change before the series can be > considered for merge? No, go ahead! Paolo
