| Message ID | 1372555629-17976-6-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
Am 30.06.2013 03:26, schrieb Alexander Graf: > The macio code is basically undebuggable as it stands today, with no > debug prints anywhere whatsoever. DBDMA was better, but I needed a > few more to create reasonable logs that tell me where breakage is. > > Add a DPRINTF macro in the macio source file and add a bunch of debug > prints that are all disabled by default of course. > > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > hw/ide/macio.c | 39 ++++++++++++++++++++++++++++++++++++++- > hw/misc/macio/mac_dbdma.c | 12 ++++++++++-- > 2 files changed, 48 insertions(+), 3 deletions(-) > > diff --git a/hw/ide/macio.c b/hw/ide/macio.c > index 82409dc..5cbc923 100644 > --- a/hw/ide/macio.c > +++ b/hw/ide/macio.c > @@ -30,6 +30,17 @@ > > #include <hw/ide/internal.h> > > +/* debug MACIO */ > +// #define DEBUG_MACIO > + > +#ifdef DEBUG_MACIO > +#define MACIO_DPRINTF(fmt, ...) \ > + do { printf("MACIO: %s: " fmt , __func__, ## __VA_ARGS__); } while (0) > +#else > +#define MACIO_DPRINTF(fmt, ...) > +#endif Please use the pattern you suggested yourself of having an if (DEBUG_MACIO_ENABLED) {...} inside the macro rather than a second MACIO_DPRINTF(), so that the newly added debug output doesn'T bitrot. Andreas > + > + > /***********************************************************/ > /* MacIO based PowerPC IDE */ > > @@ -48,6 +59,8 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) > goto done; > } > > + MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size); > + > if (s->io_buffer_size > 0) { > m->aiocb = NULL; > qemu_sglist_destroy(&s->sg); > @@ -59,15 +72,22 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) > s->io_buffer_index &= 0x7ff; > } > > - if (s->packet_transfer_size <= 0) > + /* end of transfer ? */ > + if (s->packet_transfer_size <= 0) { > + MACIO_DPRINTF("end of transfer\n"); > ide_atapi_cmd_ok(s); > + } > > + /* end of DMA ? */ > if (io->len == 0) { > + MACIO_DPRINTF("end of DMA\n"); > goto done; > } Both comments duplicate your debug output module question mark. :) > > /* launch next transfer */ > > + MACIO_DPRINTF("io->len = %#x\n", io->len); > + > s->io_buffer_size = io->len; > > qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1, > @@ -76,12 +96,17 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) > io->addr += io->len; > io->len = 0; > > + MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n", > + (s->lba << 2) + (s->io_buffer_index >> 9), > + s->packet_transfer_size, s->dma_cmd); > + > m->aiocb = dma_bdrv_read(s->bs, &s->sg, > (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9), > pmac_ide_atapi_transfer_cb, io); > return; > > done: > + MACIO_DPRINTF("done DMA\n"); > bdrv_acct_done(s->bs, &s->acct); > io->dma_end(opaque); > } > @@ -95,6 +120,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) > int64_t sector_num; > > if (ret < 0) { > + MACIO_DPRINTF("DMA error\n"); > m->aiocb = NULL; > qemu_sglist_destroy(&s->sg); > ide_dma_error(s); > @@ -102,6 +128,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) > } > > sector_num = ide_get_sector(s); > + MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size); > if (s->io_buffer_size > 0) { > m->aiocb = NULL; > qemu_sglist_destroy(&s->sg); > @@ -113,12 +140,14 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) > > /* end of transfer ? */ > if (s->nsector == 0) { > + MACIO_DPRINTF("end of transfer\n"); > s->status = READY_STAT | SEEK_STAT; > ide_set_irq(s->bus); > } > > /* end of DMA ? */ > if (io->len == 0) { > + MACIO_DPRINTF("end of DMA\n"); > goto done; > } > > @@ -127,12 +156,18 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) > s->io_buffer_index = 0; > s->io_buffer_size = io->len; > > + Intentionally two white lines? > + MACIO_DPRINTF("io->len = %#x\n", io->len); > + > qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1, > &address_space_memory); > qemu_sglist_add(&s->sg, io->addr, io->len); > io->addr += io->len; > io->len = 0; > > + MACIO_DPRINTF("sector_num=%" PRId64 " n=%d, nsector=%d, cmd_cmd=%d\n", > + sector_num, n, s->nsector, s->dma_cmd); > + > switch (s->dma_cmd) { > case IDE_DMA_READ: > m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num, > @@ -162,6 +197,8 @@ static void pmac_ide_transfer(DBDMA_io *io) > MACIOIDEState *m = io->opaque; > IDEState *s = idebus_active_if(&m->bus); > > + MACIO_DPRINTF("\n", __LINE__); The argument is unused. > + > s->io_buffer_size = 0; > if (s->drive_kind == IDE_CD) { > bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ); > diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c > index ab174f5..903604d 100644 > --- a/hw/misc/macio/mac_dbdma.c > +++ b/hw/misc/macio/mac_dbdma.c > @@ -233,6 +233,7 @@ static void conditional_interrupt(DBDMA_channel *ch) > return; > case INTR_ALWAYS: /* always interrupt */ > qemu_irq_raise(ch->irq); > + DBDMA_DPRINTF("conditional_interrupt: raise\n"); Use %s and __func__ in case we ever rename it? More instances below. For dbdma_end() you did. > return; > } > > @@ -245,12 +246,16 @@ static void conditional_interrupt(DBDMA_channel *ch) > > switch(intr) { > case INTR_IFSET: /* intr if condition bit is 1 */ > - if (cond) > + if (cond) { > qemu_irq_raise(ch->irq); > + DBDMA_DPRINTF("conditional_interrupt: raise\n"); > + } > return; > case INTR_IFCLR: /* intr if condition bit is 0 */ > - if (!cond) > + if (!cond) { > qemu_irq_raise(ch->irq); > + DBDMA_DPRINTF("conditional_interrupt: raise\n"); > + } > return; > } > } > @@ -368,6 +373,8 @@ static void dbdma_end(DBDMA_io *io) > DBDMA_channel *ch = io->channel; > dbdma_cmd *current = &ch->current; > > + DBDMA_DPRINTF("%s\n", __func__, __LINE__); Unused argument. > + > if (conditional_wait(ch)) > goto wait; > > @@ -422,6 +429,7 @@ static void start_input(DBDMA_channel *ch, int key, uint32_t addr, > * are not implemented in the mac-io chip > */ > > + DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key); PRIx32 for addr > if (!addr || key > KEY_STREAM3) { > kill_channel(ch); > return; Andreas
On 30.06.2013, at 08:42, Andreas Färber wrote: > Am 30.06.2013 03:26, schrieb Alexander Graf: >> The macio code is basically undebuggable as it stands today, with no >> debug prints anywhere whatsoever. DBDMA was better, but I needed a >> few more to create reasonable logs that tell me where breakage is. >> >> Add a DPRINTF macro in the macio source file and add a bunch of debug >> prints that are all disabled by default of course. >> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> --- >> hw/ide/macio.c | 39 ++++++++++++++++++++++++++++++++++++++- >> hw/misc/macio/mac_dbdma.c | 12 ++++++++++-- >> 2 files changed, 48 insertions(+), 3 deletions(-) >> >> diff --git a/hw/ide/macio.c b/hw/ide/macio.c >> index 82409dc..5cbc923 100644 >> --- a/hw/ide/macio.c >> +++ b/hw/ide/macio.c >> @@ -30,6 +30,17 @@ >> >> #include <hw/ide/internal.h> >> >> +/* debug MACIO */ >> +// #define DEBUG_MACIO >> + >> +#ifdef DEBUG_MACIO >> +#define MACIO_DPRINTF(fmt, ...) \ >> + do { printf("MACIO: %s: " fmt , __func__, ## __VA_ARGS__); } while (0) >> +#else >> +#define MACIO_DPRINTF(fmt, ...) >> +#endif > > Please use the pattern you suggested yourself of having an if > (DEBUG_MACIO_ENABLED) {...} inside the macro rather than a second > MACIO_DPRINTF(), so that the newly added debug output doesn'T bitrot. Very good point. Thanks a lot! > >> + >> + >> /***********************************************************/ >> /* MacIO based PowerPC IDE */ >> >> @@ -48,6 +59,8 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) >> goto done; >> } >> >> + MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size); >> + >> if (s->io_buffer_size > 0) { >> m->aiocb = NULL; >> qemu_sglist_destroy(&s->sg); >> @@ -59,15 +72,22 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) >> s->io_buffer_index &= 0x7ff; >> } >> >> - if (s->packet_transfer_size <= 0) >> + /* end of transfer ? */ >> + if (s->packet_transfer_size <= 0) { >> + MACIO_DPRINTF("end of transfer\n"); >> ide_atapi_cmd_ok(s); >> + } >> >> + /* end of DMA ? */ >> if (io->len == 0) { >> + MACIO_DPRINTF("end of DMA\n"); >> goto done; >> } > > Both comments duplicate your debug output module question mark. :) Yeah, I've just synced the comments between ATAPI and ATA here. But you're right - I should just remove them altogether. > >> >> /* launch next transfer */ >> >> + MACIO_DPRINTF("io->len = %#x\n", io->len); >> + >> s->io_buffer_size = io->len; >> >> qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1, >> @@ -76,12 +96,17 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) >> io->addr += io->len; >> io->len = 0; >> >> + MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n", >> + (s->lba << 2) + (s->io_buffer_index >> 9), >> + s->packet_transfer_size, s->dma_cmd); >> + >> m->aiocb = dma_bdrv_read(s->bs, &s->sg, >> (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9), >> pmac_ide_atapi_transfer_cb, io); >> return; >> >> done: >> + MACIO_DPRINTF("done DMA\n"); >> bdrv_acct_done(s->bs, &s->acct); >> io->dma_end(opaque); >> } >> @@ -95,6 +120,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) >> int64_t sector_num; >> >> if (ret < 0) { >> + MACIO_DPRINTF("DMA error\n"); >> m->aiocb = NULL; >> qemu_sglist_destroy(&s->sg); >> ide_dma_error(s); >> @@ -102,6 +128,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) >> } >> >> sector_num = ide_get_sector(s); >> + MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size); >> if (s->io_buffer_size > 0) { >> m->aiocb = NULL; >> qemu_sglist_destroy(&s->sg); >> @@ -113,12 +140,14 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) >> >> /* end of transfer ? */ >> if (s->nsector == 0) { >> + MACIO_DPRINTF("end of transfer\n"); >> s->status = READY_STAT | SEEK_STAT; >> ide_set_irq(s->bus); >> } >> >> /* end of DMA ? */ >> if (io->len == 0) { >> + MACIO_DPRINTF("end of DMA\n"); >> goto done; >> } >> >> @@ -127,12 +156,18 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) >> s->io_buffer_index = 0; >> s->io_buffer_size = io->len; >> >> + > > Intentionally two white lines? Nope, I'm sure that gets resolved somewhere later in the patch series. But I'll fix it up here too ;). > >> + MACIO_DPRINTF("io->len = %#x\n", io->len); >> + >> qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1, >> &address_space_memory); >> qemu_sglist_add(&s->sg, io->addr, io->len); >> io->addr += io->len; >> io->len = 0; >> >> + MACIO_DPRINTF("sector_num=%" PRId64 " n=%d, nsector=%d, cmd_cmd=%d\n", >> + sector_num, n, s->nsector, s->dma_cmd); >> + >> switch (s->dma_cmd) { >> case IDE_DMA_READ: >> m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num, >> @@ -162,6 +197,8 @@ static void pmac_ide_transfer(DBDMA_io *io) >> MACIOIDEState *m = io->opaque; >> IDEState *s = idebus_active_if(&m->bus); >> >> + MACIO_DPRINTF("\n", __LINE__); > > The argument is unused. which proves the point that DPRINTF code shouldn't be left to bitrot ;). > >> + >> s->io_buffer_size = 0; >> if (s->drive_kind == IDE_CD) { >> bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ); >> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c >> index ab174f5..903604d 100644 >> --- a/hw/misc/macio/mac_dbdma.c >> +++ b/hw/misc/macio/mac_dbdma.c >> @@ -233,6 +233,7 @@ static void conditional_interrupt(DBDMA_channel *ch) >> return; >> case INTR_ALWAYS: /* always interrupt */ >> qemu_irq_raise(ch->irq); >> + DBDMA_DPRINTF("conditional_interrupt: raise\n"); > > Use %s and __func__ in case we ever rename it? More instances below. For > dbdma_end() you did. Ah, out of scope of this hunk is another debug print that already hardcodes the name. But I'll just change to __func__ ;). > >> return; >> } >> >> @@ -245,12 +246,16 @@ static void conditional_interrupt(DBDMA_channel *ch) >> >> switch(intr) { >> case INTR_IFSET: /* intr if condition bit is 1 */ >> - if (cond) >> + if (cond) { >> qemu_irq_raise(ch->irq); >> + DBDMA_DPRINTF("conditional_interrupt: raise\n"); >> + } >> return; >> case INTR_IFCLR: /* intr if condition bit is 0 */ >> - if (!cond) >> + if (!cond) { >> qemu_irq_raise(ch->irq); >> + DBDMA_DPRINTF("conditional_interrupt: raise\n"); >> + } >> return; >> } >> } >> @@ -368,6 +373,8 @@ static void dbdma_end(DBDMA_io *io) >> DBDMA_channel *ch = io->channel; >> dbdma_cmd *current = &ch->current; >> >> + DBDMA_DPRINTF("%s\n", __func__, __LINE__); > > Unused argument. > >> + >> if (conditional_wait(ch)) >> goto wait; >> >> @@ -422,6 +429,7 @@ static void start_input(DBDMA_channel *ch, int key, uint32_t addr, >> * are not implemented in the mac-io chip >> */ >> >> + DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key); > > PRIx32 for addr Oh? Is there any system out there which does not work with %x and uint32_t? Alex
diff --git a/hw/ide/macio.c b/hw/ide/macio.c index 82409dc..5cbc923 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -30,6 +30,17 @@ #include <hw/ide/internal.h> +/* debug MACIO */ +// #define DEBUG_MACIO + +#ifdef DEBUG_MACIO +#define MACIO_DPRINTF(fmt, ...) \ + do { printf("MACIO: %s: " fmt , __func__, ## __VA_ARGS__); } while (0) +#else +#define MACIO_DPRINTF(fmt, ...) +#endif + + /***********************************************************/ /* MacIO based PowerPC IDE */ @@ -48,6 +59,8 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) goto done; } + MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size); + if (s->io_buffer_size > 0) { m->aiocb = NULL; qemu_sglist_destroy(&s->sg); @@ -59,15 +72,22 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) s->io_buffer_index &= 0x7ff; } - if (s->packet_transfer_size <= 0) + /* end of transfer ? */ + if (s->packet_transfer_size <= 0) { + MACIO_DPRINTF("end of transfer\n"); ide_atapi_cmd_ok(s); + } + /* end of DMA ? */ if (io->len == 0) { + MACIO_DPRINTF("end of DMA\n"); goto done; } /* launch next transfer */ + MACIO_DPRINTF("io->len = %#x\n", io->len); + s->io_buffer_size = io->len; qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1, @@ -76,12 +96,17 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) io->addr += io->len; io->len = 0; + MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n", + (s->lba << 2) + (s->io_buffer_index >> 9), + s->packet_transfer_size, s->dma_cmd); + m->aiocb = dma_bdrv_read(s->bs, &s->sg, (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9), pmac_ide_atapi_transfer_cb, io); return; done: + MACIO_DPRINTF("done DMA\n"); bdrv_acct_done(s->bs, &s->acct); io->dma_end(opaque); } @@ -95,6 +120,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) int64_t sector_num; if (ret < 0) { + MACIO_DPRINTF("DMA error\n"); m->aiocb = NULL; qemu_sglist_destroy(&s->sg); ide_dma_error(s); @@ -102,6 +128,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) } sector_num = ide_get_sector(s); + MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size); if (s->io_buffer_size > 0) { m->aiocb = NULL; qemu_sglist_destroy(&s->sg); @@ -113,12 +140,14 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) /* end of transfer ? */ if (s->nsector == 0) { + MACIO_DPRINTF("end of transfer\n"); s->status = READY_STAT | SEEK_STAT; ide_set_irq(s->bus); } /* end of DMA ? */ if (io->len == 0) { + MACIO_DPRINTF("end of DMA\n"); goto done; } @@ -127,12 +156,18 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) s->io_buffer_index = 0; s->io_buffer_size = io->len; + + MACIO_DPRINTF("io->len = %#x\n", io->len); + qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1, &address_space_memory); qemu_sglist_add(&s->sg, io->addr, io->len); io->addr += io->len; io->len = 0; + MACIO_DPRINTF("sector_num=%" PRId64 " n=%d, nsector=%d, cmd_cmd=%d\n", + sector_num, n, s->nsector, s->dma_cmd); + switch (s->dma_cmd) { case IDE_DMA_READ: m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num, @@ -162,6 +197,8 @@ static void pmac_ide_transfer(DBDMA_io *io) MACIOIDEState *m = io->opaque; IDEState *s = idebus_active_if(&m->bus); + MACIO_DPRINTF("\n", __LINE__); + s->io_buffer_size = 0; if (s->drive_kind == IDE_CD) { bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ); diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c index ab174f5..903604d 100644 --- a/hw/misc/macio/mac_dbdma.c +++ b/hw/misc/macio/mac_dbdma.c @@ -233,6 +233,7 @@ static void conditional_interrupt(DBDMA_channel *ch) return; case INTR_ALWAYS: /* always interrupt */ qemu_irq_raise(ch->irq); + DBDMA_DPRINTF("conditional_interrupt: raise\n"); return; } @@ -245,12 +246,16 @@ static void conditional_interrupt(DBDMA_channel *ch) switch(intr) { case INTR_IFSET: /* intr if condition bit is 1 */ - if (cond) + if (cond) { qemu_irq_raise(ch->irq); + DBDMA_DPRINTF("conditional_interrupt: raise\n"); + } return; case INTR_IFCLR: /* intr if condition bit is 0 */ - if (!cond) + if (!cond) { qemu_irq_raise(ch->irq); + DBDMA_DPRINTF("conditional_interrupt: raise\n"); + } return; } } @@ -368,6 +373,8 @@ static void dbdma_end(DBDMA_io *io) DBDMA_channel *ch = io->channel; dbdma_cmd *current = &ch->current; + DBDMA_DPRINTF("%s\n", __func__, __LINE__); + if (conditional_wait(ch)) goto wait; @@ -422,6 +429,7 @@ static void start_input(DBDMA_channel *ch, int key, uint32_t addr, * are not implemented in the mac-io chip */ + DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key); if (!addr || key > KEY_STREAM3) { kill_channel(ch); return;
The macio code is basically undebuggable as it stands today, with no debug prints anywhere whatsoever. DBDMA was better, but I needed a few more to create reasonable logs that tell me where breakage is. Add a DPRINTF macro in the macio source file and add a bunch of debug prints that are all disabled by default of course. Signed-off-by: Alexander Graf <agraf@suse.de> --- hw/ide/macio.c | 39 ++++++++++++++++++++++++++++++++++++++- hw/misc/macio/mac_dbdma.c | 12 ++++++++++-- 2 files changed, 48 insertions(+), 3 deletions(-)