Re: [PATCH v2 03/15] hw/ide: Remove internal DMA qemu_irq

[Mark Cave-Ayland]

On 27/06/2024 14:37, Akihiko Odaki wrote:

> A function pointer is sufficient for internal usage. Replacing qemu_irq
> with one fixes the leak of qemu_irq.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   include/hw/ppc/mac_dbdma.h |  5 +++--
>   hw/ide/macio.c             | 11 +++++++----
>   hw/misc/macio/mac_dbdma.c  | 10 +++++-----
>   3 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
> index 4a3f644516b3..1e1973c39580 100644
> --- a/include/hw/ppc/mac_dbdma.h
> +++ b/include/hw/ppc/mac_dbdma.h
> @@ -32,6 +32,7 @@
>   typedef struct DBDMA_io DBDMA_io;
>   
>   typedef void (*DBDMA_flush)(DBDMA_io *io);
> +typedef void (*DBDMA_irq)(DBDMA_io *io);
>   typedef void (*DBDMA_rw)(DBDMA_io *io);
>   typedef void (*DBDMA_end)(DBDMA_io *io);
>   struct DBDMA_io {
> @@ -154,7 +155,7 @@ typedef struct dbdma_cmd {
>   typedef struct DBDMA_channel {
>       int channel;
>       uint32_t regs[DBDMA_REGS];
> -    qemu_irq irq;
> +    DBDMA_irq irq;
>       DBDMA_io io;
>       DBDMA_rw rw;
>       DBDMA_flush flush;
> @@ -172,7 +173,7 @@ typedef struct DBDMAState DBDMAState;
>   
>   /* Externally callable functions */
>   
> -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
> +void DBDMA_register_channel(void *dbdma, int nchan, DBDMA_irq irq,
>                               DBDMA_rw rw, DBDMA_flush flush,
>                               void *opaque);
>   void DBDMA_kick(DBDMAState *dbdma);
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 9c96a857a7c1..425b670a52a9 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -427,9 +427,8 @@ static void macio_ide_realizefn(DeviceState *dev, Error 
> **errp)
>       s->bus.dma = &s->dma;
>   }
>   
> -static void pmac_irq(void *opaque, int n, int level)
> +static void pmac_irq(MACIOIDEState *s, int n, int level)
>   {
> -    MACIOIDEState *s = opaque;
>       uint32_t mask = 0x80000000u >> n;
>   
>       /* We need to reflect the IRQ state in the irq register */
> @@ -446,6 +445,11 @@ static void pmac_irq(void *opaque, int n, int level)
>       }
>   }
>   
> +static void pmac_dma_irq(DBDMA_io *io)
> +{
> +    pmac_irq(io->opaque, 0, 1);
> +}
> +
>   static void pmac_ide_irq(void *opaque, int n, int level)
>   {
>       pmac_irq(opaque, 1, level);
> @@ -461,7 +465,6 @@ static void macio_ide_initfn(Object *obj)
>       sysbus_init_mmio(d, &s->mem);
>       sysbus_init_irq(d, &s->real_ide_irq);
>       sysbus_init_irq(d, &s->real_dma_irq);
> -    s->dma_irq = qemu_allocate_irq(pmac_irq, s, 0);
>       qdev_init_gpio_in_named_with_opaque(DEVICE(obj), pmac_ide_irq, s, NULL, 
> 1);
>   
>       object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
> @@ -513,7 +516,7 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo 
> **hd_table)
>   
>   void macio_ide_register_dma(MACIOIDEState *s)
>   {
> -    DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
> +    DBDMA_register_channel(s->dbdma, s->channel, pmac_dma_irq,
>                              pmac_ide_transfer, pmac_ide_flush, s);
>   }
>   
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index 2a528ea08caf..3450105ad851 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -114,7 +114,7 @@ static void kill_channel(DBDMA_channel *ch)
>       ch->regs[DBDMA_STATUS] |= DEAD;
>       ch->regs[DBDMA_STATUS] &= ~ACTIVE;
>   
> -    qemu_irq_raise(ch->irq);
> +    ch->irq(&ch->io);
>   }
>   
>   static void conditional_interrupt(DBDMA_channel *ch)
> @@ -133,7 +133,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
>       case INTR_NEVER:  /* don't interrupt */
>           return;
>       case INTR_ALWAYS: /* always interrupt */
> -        qemu_irq_raise(ch->irq);
> +            ch->irq(&ch->io);
>           DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
>           return;
>       }
> @@ -148,13 +148,13 @@ static void conditional_interrupt(DBDMA_channel *ch)
>       switch(intr) {
>       case INTR_IFSET:  /* intr if condition bit is 1 */
>           if (cond) {
> -            qemu_irq_raise(ch->irq);
> +            ch->irq(&ch->io);
>               DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
>           }
>           return;
>       case INTR_IFCLR:  /* intr if condition bit is 0 */
>           if (!cond) {
> -            qemu_irq_raise(ch->irq);
> +            ch->irq(&ch->io);
>               DBDMA_DPRINTFCH(ch, "%s: raise\n", __func__);
>           }
>           return;
> @@ -562,7 +562,7 @@ void DBDMA_kick(DBDMAState *dbdma)
>       qemu_bh_schedule(dbdma->bh);
>   }
>   
> -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
> +void DBDMA_register_channel(void *dbdma, int nchan, DBDMA_irq irq,
>                               DBDMA_rw rw, DBDMA_flush flush,
>                               void *opaque)
>   {

At first glance I can't say I'm keen on this: in general we should be moving towards 
standardising on QEMU irqs or qdev gpios rather than introducing a custom function.

As per my previous email I suspect this is another symptom that something is wrong 
with the modelling, so I will take a look.


ATB,

Mark.