[Qemu-devel] Re: [PATCH 03/13] ide: Split out BMDMA code from ATA core

[Stefan Hajnoczi]

On Wed, Dec 8, 2010 at 12:13 PM, Alexander Graf <address@hidden> wrote:
> @@ -486,8 +440,8 @@ void ide_dma_error(IDEState *s)
>     ide_transfer_stop(s);
>     s->error = ABRT_ERR;
>     s->status = READY_STAT | ERR_STAT;
> -    ide_dma_set_inactive(s->bus->bmdma);
> -    s->bus->bmdma->status |= BM_STATUS_INT;
> +    ide_set_inactive(s);
> +    s->bus->dma.ops->set_status(s->bus->dma.opaque, BM_STATUS_INT);

Is BM_STATUS_INT constant naming appropriate for a general DMA
abstraction?  Perhaps DMA_STATUS_INT.

> @@ -2717,6 +2586,29 @@ static void ide_init1(IDEBus *bus, int unit)
>                                            ide_sector_write_timer_cb, s);
>  }
>
> +static int ide_nop_start_irq(void *opaque)
> +{
> +    return 1;
> +}
> +
> +static int ide_nop(void *opaque)
> +{
> +    return 0;
> +}
> +
> +static const IDEDMAOps ide_dma_nop = {
> +    .start_irq      = ide_nop_start_irq,
> +    .start_dma      = (void*)ide_nop,
> +    .start_transfer = (void*)ide_nop,
> +    .prepare_buf    = (void*)ide_nop,
> +    .rw_buf         = (void*)ide_nop,
> +    .set_unit       = (void*)ide_nop,
> +    .set_status     = (void*)ide_nop,
> +    .set_inactive   = (void*)ide_nop,
> +    .restart_cb     = (void*)ide_nop,
> +    .reset          = (void*)ide_nop,

Creative use of void* :).  This looks unportable.

ppc and other architectures use function descriptors.  There, a
function pointer is not sizeof(void*) so the (void*) cast is
questionable.

Also, casting to a function with a different signature is unportable.
You're relying on the calling convention to make this work.

Instead of fleshing out these functions, how about initializing
dma.ops to NULL?  The program crashes should anyone try to do DMA
before setting a real IDEDMAOps pointer.  That's not as robust as
limping along with non-working IDE, but should be straightforward to
debug if it ever happens.  It also requires less code.

Stefan