Re: [Qemu-devel] [PATCH] ahci: Fix FLUSH command

["Andreas Färber" <afaerber@suse.de>]

Am 15.07.2013 11:34, schrieb Kevin Wolf:
> AHCI couldn't cope with asynchronous commands that aren't doing DMA, it
> simply wouldn't complete them. Due to the bug fixed in commit f68ec837,
> FLUSH commands would seem to have completed immediately even if they
> were still running on the host. After the commit, they would simply hang
> and never unset the BSY bit, rendering AHCI unusable on any OS sending
> flushes.
> 
> This patch adds another callback for the completion of asynchronous
> commands. This is what AHCI really wants to use for its command
> completion logic rather than an DMA completion callback.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/ide/ahci.c     | 8 +++++++-
>  hw/ide/core.c     | 9 +++++++++
>  hw/ide/internal.h | 1 +
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 97eddec..1d863b5 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1107,9 +1107,14 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
>  
>  static int ahci_dma_set_inactive(IDEDMA *dma)
>  {
> +    return 0;

Is it intentional that this is now no-op rather than calling
dma->ops->set_inactive(dma) like core IDE does below?

Other than that it looks sensible to me.

Andreas

> +}
> +
> +static int ahci_async_cmd_done(IDEDMA *dma)
> +{
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>  
> -    DPRINTF(ad->port_no, "dma done\n");
> +    DPRINTF(ad->port_no, "async cmd done\n");
>  
>      /* update d2h status */
>      ahci_write_fis_d2h(ad, NULL);
> @@ -1144,6 +1149,7 @@ static const IDEDMAOps ahci_dma_ops = {
>      .set_unit = ahci_dma_set_unit,
>      .add_status = ahci_dma_add_status,
>      .set_inactive = ahci_dma_set_inactive,
> +    .async_cmd_done = ahci_async_cmd_done,
>      .restart_cb = ahci_dma_restart_cb,
>      .reset = ahci_dma_reset,
>  };
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 03d1cfa..a73af72 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -568,10 +568,18 @@ static void dma_buf_commit(IDEState *s)
>      qemu_sglist_destroy(&s->sg);
>  }
>  
> +static void ide_async_cmd_done(IDEState *s)
> +{
> +    if (s->bus->dma->ops->async_cmd_done) {
> +        s->bus->dma->ops->async_cmd_done(s->bus->dma);
> +    }
> +}
> +
>  void ide_set_inactive(IDEState *s)
>  {
>      s->bus->dma->aiocb = NULL;
>      s->bus->dma->ops->set_inactive(s->bus->dma);
> +    ide_async_cmd_done(s);
>  }
>  
>  void ide_dma_error(IDEState *s)
> @@ -804,6 +812,7 @@ static void ide_flush_cb(void *opaque, int ret)
>  
>      bdrv_acct_done(s->bs, &s->acct);
>      s->status = READY_STAT | SEEK_STAT;
> +    ide_async_cmd_done(s);
>      ide_set_irq(s->bus);
>  }
>  
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 03f1489..048a052 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -433,6 +433,7 @@ struct IDEDMAOps {
>      DMAIntFunc *set_unit;
>      DMAIntFunc *add_status;
>      DMAFunc *set_inactive;
> +    DMAFunc *async_cmd_done;
>      DMARestartFunc *restart_cb;
>      DMAFunc *reset;
>  };
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg