diff mbox

[v2,2/7] hw/mips/mips_jazz.c: Store irq array in MachineState to fix memory leak

Message ID 1432972477-13504-3-git-send-email-zhaoshenglong@huawei.com
State New
Headers show

Commit Message

Shannon Zhao May 30, 2015, 7:54 a.m. UTC 
From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/mips/mips_jazz.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Comments

Shannon Zhao June 4, 2015, 2:30 p.m. UTC | #1 
On 2015/6/4 22:18, Michael Tokarev wrote:
> 30.05.2015 10:54, Shannon Zhao пишет:
>> >From: Shannon Zhao<shannon.zhao@linaro.org>
>> >
>> >Signed-off-by: Shannon Zhao<zhaoshenglong@huawei.com>
>> >Signed-off-by: Shannon Zhao<shannon.zhao@linaro.org>
>> >---
>> >  hw/mips/mips_jazz.c | 28 ++++++++++++++--------------
>> >  1 file changed, 14 insertions(+), 14 deletions(-)
>> >
>> >diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
>> >index 2c153e0..259458b 100644
>> >--- a/hw/mips/mips_jazz.c
>> >+++ b/hw/mips/mips_jazz.c
>> >@@ -135,7 +135,7 @@ static void mips_jazz_init(MachineState *machine,
>> >      MIPSCPU *cpu;
>> >      CPUClass *cc;
>> >      CPUMIPSState *env;
>> >-    qemu_irq *rc4030, *i8259;
>> >+    qemu_irq *i8259;
> Hm. Why do you only cover rc4030, not i8259?
>

As i8259 is stored in ISABus->irqs by function isa_bus_irqs.

void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
{
     if (!bus) {
         hw_error("Can't set isa irqs with no isa bus present.");
     }
     bus->irqs = irqs;
}

> Besides, in order to keep the changes smaller, I think it is okay to
> keep the variables like that, here and in the rest of the function,
> and only add assignment of it to machine->irqs.  This way, we also
> keep semantic names of the variables, rc4030[i] is easier to understand
> than machine->irqs[i], the former's more specific.
>
Agree.

> BTW, there's also cpu_exit_irq in this function whose allocation also
> suffers from qemu_allocate_irqs(..., 1) API abuse.

Yeah, missed this one.
Shannon Zhao June 4, 2015, 2:51 p.m. UTC | #2 
On 2015/6/4 22:32, Michael Tokarev wrote:
> 04.06.2015 17:30, Shannon Zhao wrote:
>
>>> >>BTW, there's also cpu_exit_irq in this function whose allocation also
>>> >>suffers from qemu_allocate_irqs(..., 1) API abuse.
>> >
>> >Yeah, missed this one.
> This one isn't that simple, because it passes the result to DMA_init
> which expects a pointer, even if it uses only one element of the
> array.

Yeah, but I think something like below would work.

     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
     DMA_init(0, cpu_exit_irq);
Shannon Zhao June 4, 2015, 3:09 p.m. UTC | #3 
On 2015/6/4 22:57, Michael Tokarev wrote:
> 04.06.2015 17:51, Shannon Zhao wrote:
>> >Yeah, but I think something like below would work.
>> >
>> >     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
>> >     DMA_init(0, cpu_exit_irq);
> NO!!!:)

Hmm...really?

This is used by DMA_schedule.

void DMA_schedule(int nchan)
{
     struct dma_cont *d = &dma_controllers[nchan > 3];

     qemu_irq_pulse(*d->cpu_request_exit);
}
Shannon Zhao June 4, 2015, 3:17 p.m. UTC | #4 
On 2015/6/4 22:57, Michael Tokarev wrote:
> 04.06.2015 17:51, Shannon Zhao wrote:
>> >Yeah, but I think something like below would work.
>> >
>> >     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
>> >     DMA_init(0, cpu_exit_irq);
> NO!!!:)

Oh, will rethink about it. But I think maybe it's unnecessary to fix it 
as it actually requires a pointer which stores qemu_irq.
Shannon Zhao June 4, 2015, 3:23 p.m. UTC | #5 
On 2015/6/4 23:17, Shannon Zhao wrote:
>
>
> On 2015/6/4 22:57, Michael Tokarev wrote:
>> 04.06.2015 17:51, Shannon Zhao wrote:
>>> >Yeah, but I think something like below would work.
>>> >
>>> >     cpu_exit_irq = &qemu_allocate_irq(cpu_request_exit, NULL, 0);
>>> >     DMA_init(0, cpu_exit_irq);
>> NO!!!:)
>
> Oh, will rethink about it. But I think maybe it's unnecessary to fix it
> as it actually requires a pointer which stores qemu_irq.

And if we want to use qemu_allocate_irq here, it will be something like:

cpu_exit_irq = g_new(qemu_irq, 1);
cpu_exit_irq[0] = qemu_allocate_irq(cpu_request_exit, NULL, 0);
DMA_init(0, cpu_exit_irq);

This is what exactly qemu_allocate_irqs does.

Or we modify the DMA_init to make it take qemu_irq as parameter not 
qemu_irq *
diff mbox

Patch

diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index 2c153e0..259458b 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -135,7 +135,7 @@  static void mips_jazz_init(MachineState *machine,
     MIPSCPU *cpu;
     CPUClass *cc;
     CPUMIPSState *env;
-    qemu_irq *rc4030, *i8259;
+    qemu_irq *i8259;
     rc4030_dma *dmas;
     void* rc4030_opaque;
     MemoryRegion *isa_mem = g_new(MemoryRegion, 1);
@@ -218,7 +218,7 @@  static void mips_jazz_init(MachineState *machine,
     cpu_mips_clock_init(env);
 
     /* Chipset */
-    rc4030_opaque = rc4030_init(env->irq[6], env->irq[3], &rc4030, &dmas,
+    rc4030_opaque = rc4030_init(env->irq[6], env->irq[3], &machine->irqs, &dmas,
                                 address_space);
     memory_region_init_io(dma_dummy, NULL, &dma_dummy_ops, NULL, "dummy_dma", 0x1000);
     memory_region_add_subregion(address_space, 0x8000d000, dma_dummy);
@@ -246,7 +246,7 @@  static void mips_jazz_init(MachineState *machine,
         sysbus = SYS_BUS_DEVICE(dev);
         sysbus_mmio_map(sysbus, 0, 0x60080000);
         sysbus_mmio_map(sysbus, 1, 0x40000000);
-        sysbus_connect_irq(sysbus, 0, rc4030[3]);
+        sysbus_connect_irq(sysbus, 0, machine->irqs[3]);
         {
             /* Simple ROM, so user doesn't have to provide one */
             MemoryRegion *rom_mr = g_new(MemoryRegion, 1);
@@ -272,8 +272,8 @@  static void mips_jazz_init(MachineState *machine,
         if (!nd->model)
             nd->model = g_strdup("dp83932");
         if (strcmp(nd->model, "dp83932") == 0) {
-            dp83932_init(nd, 0x80001000, 2, get_system_memory(), rc4030[4],
-                         rc4030_opaque, rc4030_dma_memory_rw);
+            dp83932_init(nd, 0x80001000, 2, get_system_memory(),
+                         machine->irqs[4], rc4030_opaque, rc4030_dma_memory_rw);
             break;
         } else if (is_help_option(nd->model)) {
             fprintf(stderr, "qemu: Supported NICs: dp83932\n");
@@ -287,7 +287,7 @@  static void mips_jazz_init(MachineState *machine,
     /* SCSI adapter */
     esp_init(0x80002000, 0,
              rc4030_dma_read, rc4030_dma_write, dmas[0],
-             rc4030[5], &esp_reset, &dma_enable);
+             machine->irqs[5], &esp_reset, &dma_enable);
 
     /* Floppy */
     if (drive_get_max_bus(IF_FLOPPY) >= MAX_FD) {
@@ -297,7 +297,7 @@  static void mips_jazz_init(MachineState *machine,
     for (n = 0; n < MAX_FD; n++) {
         fds[n] = drive_get(IF_FLOPPY, 0, n);
     }
-    fdctrl_init_sysbus(rc4030[1], 0, 0x80003000, fds);
+    fdctrl_init_sysbus(machine->irqs[1], 0, 0x80003000, fds);
 
     /* Real time clock */
     rtc_init(isa_bus, 1980, NULL);
@@ -305,25 +305,25 @@  static void mips_jazz_init(MachineState *machine,
     memory_region_add_subregion(address_space, 0x80004000, rtc);
 
     /* Keyboard (i8042) */
-    i8042_mm_init(rc4030[6], rc4030[7], i8042, 0x1000, 0x1);
+    i8042_mm_init(machine->irqs[6], machine->irqs[7], i8042, 0x1000, 0x1);
     memory_region_add_subregion(address_space, 0x80005000, i8042);
 
     /* Serial ports */
     if (serial_hds[0]) {
-        serial_mm_init(address_space, 0x80006000, 0, rc4030[8], 8000000/16,
-                       serial_hds[0], DEVICE_NATIVE_ENDIAN);
+        serial_mm_init(address_space, 0x80006000, 0, machine->irqs[8],
+                       8000000/16, serial_hds[0], DEVICE_NATIVE_ENDIAN);
     }
     if (serial_hds[1]) {
-        serial_mm_init(address_space, 0x80007000, 0, rc4030[9], 8000000/16,
-                       serial_hds[1], DEVICE_NATIVE_ENDIAN);
+        serial_mm_init(address_space, 0x80007000, 0, machine->irqs[9],
+                       8000000/16, serial_hds[1], DEVICE_NATIVE_ENDIAN);
     }
 
     /* Parallel port */
     if (parallel_hds[0])
-        parallel_mm_init(address_space, 0x80008000, 0, rc4030[0],
+        parallel_mm_init(address_space, 0x80008000, 0, machine->irqs[0],
                          parallel_hds[0]);
 
-    /* FIXME: missing Jazz sound at 0x8000c000, rc4030[2] */
+    /* FIXME: missing Jazz sound at 0x8000c000, machine->irqs[2] */
 
     /* NVRAM */
     dev = qdev_create(NULL, "ds1225y");