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

[Kevin Wolf <kwolf@redhat.com>]

Am 16.07.2013 um 14:30 hat Andreas Färber geschrieben:
> 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?

This _is_ the dma->ops->set_inactive() callback of AHCI that is called
by the IDE code. For plain IDE commands the same IDEDMAOps function is
implemented by ide_nop().

Kevin

> 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