Re: [Qemu-devel] [PATCH 2/6] ide: push end_transfer_func out of start_transfer callback, rename callback

[John Snow]

On 04/17/2018 11:39 AM, Paolo Bonzini wrote:
> Now that end_transfer_func is a tail call in ahci_start_transfer,
> formalize the fact that the callback (of which ahci_start_transfer is
> the sole implementation) takes care of the transfer too: rename it to
> pio_transfer and, if it is present, call the end_transfer_func as soon
> as it returns.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/ide/ahci.c             | 15 +++++++--------
>  hw/ide/core.c             |  8 +++++---
>  hw/ide/trace-events       |  2 +-
>  include/hw/ide/internal.h |  2 +-
>  4 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 45ce195fb8..10bcc65308 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1280,8 +1280,8 @@ out:
>      return 0;
>  }
>  
> -/* DMA dev <-> ram */
> -static void ahci_start_transfer(IDEDMA *dma)
> +/* Transfer PIO data between RAM and device */
> +static void ahci_pio_transfer(IDEDMA *dma)
>  {
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>      IDEState *s = &ad->port.ifs[0];
> @@ -1304,9 +1304,9 @@ static void ahci_start_transfer(IDEDMA *dma)
>          has_sglist = 1;
>      }
>  
> -    trace_ahci_start_transfer(ad->hba, ad->port_no, is_write ? "writ" : 
> "read",
> -                              size, is_atapi ? "atapi" : "ata",
> -                              has_sglist ? "" : "o");
> +    trace_ahci_pio_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read",
> +                            size, is_atapi ? "atapi" : "ata",
> +                            has_sglist ? "" : "o");
>  
>      if (has_sglist && size) {
>          if (is_write) {
> @@ -1319,12 +1319,11 @@ static void ahci_start_transfer(IDEDMA *dma)
>      /* Update number of transferred bytes, destroy sglist */
>      dma_buf_commit(s, size);
>      fis_len = le32_to_cpu(ad->cur_cmd->status);
> +
>  out:
>      /* declare that we processed everything */
>      s->data_ptr = s->data_end;
>      ahci_write_fis_pio(ad, fis_len);
> -
> -    s->end_transfer_func(s);
>  }
>  
>  static void ahci_start_dma(IDEDMA *dma, IDEState *s,
> @@ -1440,7 +1439,7 @@ static const IDEDMAOps ahci_dma_ops = {
>      .start_dma = ahci_start_dma,
>      .restart = ahci_restart,
>      .restart_dma = ahci_restart_dma,
> -    .start_transfer = ahci_start_transfer,
> +    .pio_transfer = ahci_pio_transfer,
>      .prepare_buf = ahci_dma_prepare_buf,
>      .commit_buf = ahci_commit_buf,
>      .rw_buf = ahci_dma_rw_buf,
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 866c659498..7932b7c069 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -527,16 +527,18 @@ static void ide_clear_retry(IDEState *s)
>  void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
>                          EndTransferFunc *end_transfer_func)
>  {
> -    s->end_transfer_func = end_transfer_func;
>      s->data_ptr = buf;
>      s->data_end = buf + size;
>      ide_set_retry(s);
>      if (!(s->status & ERR_STAT)) {
>          s->status |= DRQ_STAT;
>      }
> -    if (s->bus->dma->ops->start_transfer) {
> -        s->bus->dma->ops->start_transfer(s->bus->dma);
> +    if (!s->bus->dma->ops->pio_transfer) {
> +        s->end_transfer_func = end_transfer_func;
> +        return;
>      }
> +    s->bus->dma->ops->pio_transfer(s->bus->dma);
> +    end_transfer_func(s);

Does not setting s->end_transfer_func mess with some of our dumb hacks
in e.g. ide_restart_bh or ide_is_pio_out?