On 08.12.2010, at 15:26, Stefan Hajnoczi wrote:
> On Wed, Dec 8, 2010 at 12:13 PM, Alexander Graf <ag...@suse.de> 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
