[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs

From: BALATON Zoltan
Subject: Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
Date: Mon, 23 Mar 2020 20:52:48 +0100 (CET)
User-agent: Alpine 2.22 (BSF 395 2020-01-19)

On Mon, 23 Mar 2020, John Snow wrote:
On 3/23/20 1:04 PM, BALATON Zoltan wrote:
On Mon, 23 Mar 2020, Peter Maydell wrote:
Coverity points out (CID 1421984) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Signed-off-by: Peter Maydell <address@hidden>
This is how the 'use qdev gpio' approach to fixing the leak looks.
Disclaimer: I have only tested this with "make check", nothing more.

hw/ide/sii3112.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2b..2ae6f5d9df6 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev,
Error **errp)
    SiI3112PCIState *d = SII3112_PCI(dev);
    PCIIDEState *s = PCI_IDE(dev);
+    DeviceState *ds = DEVICE(dev);
    MemoryRegion *mr;
-    qemu_irq *irq;
    int i;

    pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev,
Error **errp)
    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio,
0, 16);
    pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);

-    irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+    qdev_init_gpio_in(ds, sii3112_set_irq, 2);
    for (i = 0; i < 2; i++) {
        ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-        ide_init2(&s->bus[i], irq[i]);
+        ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));

Maybe we could just use DEVICE(dev) here and above as well just like in
ide_bus_new above just to keep it consistent and avoid the confusion
caused by having dev, d, s and now also ds. DEVICE(dev) is short and
clear enough I think.


Reviewed-by: John Snow <address@hidden>

The named temporary is fine. We probably should be using a named
temporary in the other locations, too.

Yes that's fine too if using cast more than once is less efficient. Could you change the existing DEVICE(dev) also to ds when applying please? Probably no need for a v2 for that.

I will run my usual tests, but admit I don't really test the non-x86
boards directly. Do you want to give a tested-by on this, if it matters
to you? Otherwise, I'm fairly content to trust Peter's judgment here.

I've tried that AmigaOS still boots on sam460ex so:

Tested-by: BALATON Zoltan <address@hidden>

The sii3112 should also work on x86 or other platforms with Linux's sata_sil driver but only as a 2nd non-bootable controller because we don't have option ROM and BIOS probably has no driver. Apart from that it's a common PCI SATA controller so likely a lot of guests have driver for it.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]