On 20/03/2018 22:46, John Snow wrote: >> } >> - if (s->bus->dma->ops->start_transfer) { >> - s->bus->dma->ops->start_transfer(s->bus->dma); >> + if (!s->bus->dma->ops->start_transfer) { >> + s->end_transfer_func = end_transfer_func; >> + return; >> } >> + s->bus->dma->ops->start_transfer(s->bus->dma); >> + end_transfer_func(s); > wow, this makes me really uneasy to look at. The assumption now is: "If > you have a start_transfer method, that method if successful implies that > the transfer is *COMPLETED*." > > It's implicit here, but I think anyone but the two of us would probably > not understand that implication. (Or me in three months.) What can we do > about that? Since AHCI is the only interface that uses this callback, I > wonder if we can't just rename it to something more obvious.
You are completely right, it should be renamed to pio_transfer. > Under normal circumstances this function really does just "start" a > transfer and it's not obvious from context that some adapters have > synchronous methods to finish the transfer without further PIO pingpong > with the guest. Yeah, though it is already doing it---the patch is only unhiding the mutual recursion by pulling it one level up. Paolo