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

[BALATON Zoltan]

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.
> >
> > Regards,
> > BALATON Zoltan
>
> 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.

Regards,
BALATON Zoltan