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

[Alexander Graf]

On 08.12.2010, at 15:26, Stefan Hajnoczi wrote:

> 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.

I was thinking of that too, but then again, why bother? Let's just declare 
BMDMA status bits the standard and be good sounded the easiest :). Less 
conversions are good, no? And so far, no other user really needs those bits.

> 
>> @@ -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.

Hrm, interesting. Maybe I should create one entry for each function type then.

> 
> 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.

Unfortunately, at least reset gets called before the DMA init :(.


Alex