ide: cleanup warnings

On Wed, May 04, 2011 at 10:08:12AM +0200, Kevin Wolf wrote:
> Isn't it a bug that qemu_aio_flush() doesn't clear aiocb/status? Should
> we move the ide_set_inactive() call from ide_dma_error to ide_dma_cb?

How would that make a difference, it's still running in aio context,
running it a bit earlier won't move the needle? I think it's more
likely an error path currently not covered by ide_set_inactive that
may have to be covered. It doesn't seem fatal but I tend to agree if
we can make that warning go away without putting it under #ifdef like
usptream, we should do that too.

Maybe something like this will make it go away?

Am 04.05.2011 15:41, schrieb Andrea Arcangeli:
> On Wed, May 04, 2011 at 10:08:12AM +0200, Kevin Wolf wrote:
>> Isn't it a bug that qemu_aio_flush() doesn't clear aiocb/status? Should
>> we move the ide_set_inactive() call from ide_dma_error to ide_dma_cb?
> 
> How would that make a difference, it's still running in aio context,
> running it a bit earlier won't move the needle?

Yes, sorry, you're right. I was thinking of the werror=stop case, but
this isn't your case and ide_set_inactive would even be wrong there.

> I think it's more
> likely an error path currently not covered by ide_set_inactive that
> may have to be covered. It doesn't seem fatal but I tend to agree if
> we can make that warning go away without putting it under #ifdef like
> usptream, we should do that too.
> 
> Maybe something like this will make it go away?
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 90f553b..b81f1d7 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -377,6 +377,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num)
>  
>  static void ide_rw_error(IDEState *s) {
>      ide_abort_command(s);
> +    ide_set_inactive(s);
>      ide_set_irq(s->bus);
>  }

No, this looks wrong. ide_rw_error is only used for PIO, and
ide_set_inactive() resets the DMA status.

I can't see how you could leave ide_dma_cb without either scheduling
another AIO request or setting aiocb = NULL in ide_set_inactive. I guess
I need to reproduce this and do some debugging...

Kevin

On Wed, May 04, 2011 at 03:57:28PM +0200, Kevin Wolf wrote:
> I can't see how you could leave ide_dma_cb without either scheduling
> another AIO request or setting aiocb = NULL in ide_set_inactive. I guess
> I need to reproduce this and do some debugging...

That would be nice to be sure. This has been triggering on a older
codebase, it's uncertain if this would happen with current code
too. The \n so far was the only obvious improvement I could make to
the upstream code, until we understand better what's going on...

The good thing is it doesn't seem to cause problems, other than the
printf. The leftover in the dma->aiocb likely gets overwritten when
guest retries without failure.

Am 04.05.2011 16:04, schrieb Andrea Arcangeli:
> On Wed, May 04, 2011 at 03:57:28PM +0200, Kevin Wolf wrote:
>> I can't see how you could leave ide_dma_cb without either scheduling
>> another AIO request or setting aiocb = NULL in ide_set_inactive. I guess
>> I need to reproduce this and do some debugging...
> 
> That would be nice to be sure. This has been triggering on a older
> codebase, it's uncertain if this would happen with current code
> too. The \n so far was the only obvious improvement I could make to
> the upstream code, until we understand better what's going on...
> 
> The good thing is it doesn't seem to cause problems, other than the
> printf. The leftover in the dma->aiocb likely gets overwritten when
> guest retries without failure.

Right. I'm not so much concerned about aiocb (though it would certainly
be cleaner if we cleared it; I think it's checked in more places), but
more about bm->status which is a guest-visible register.

On the other hand we store things in bm->status that are definitely not
meant to be guest visible, so the emulation of this register is broken
anyway. Fixing all of this might turn out to be hard if you don't want
to break migration.

Kevin