Message ID | 20200323151715.29454-1-peter.maydell@linaro.org |
---|---|
State | New |
Headers | show |
Series | hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() | expand |
On 3/23/20 4:17 PM, 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 <peter.maydell@linaro.org> > --- > 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)); > > bmdma_init(&s->bus[i], &s->bmdma[i], s); > s->bmdma[i].bus = &s->bus[i]; > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
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 <peter.maydell@linaro.org> > --- > 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 > > bmdma_init(&s->bus[i], &s->bmdma[i], s); > s->bmdma[i].bus = &s->bus[i]; >
On Mon, 23 Mar 2020 at 17:04, BALATON Zoltan <balaton@eik.bme.hu> 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 <peter.maydell@linaro.org> > > --- > > 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. Usual style in these methods is to have local variables if it's going to be used more than once or twice -- QOM casts aren't free the way C casts are. We already do that here for the SII3112_PCI() and the PCI_IDE(). In this case things are a bit confused by the function having used "dev" for the PCIDevice* and 's' for the PCIIDEState. QEMU is far from consistent in this matter, but usually 's' is the pointer to the device's own state (ie SiI3112PCIState* in this case) and the DeviceState* is 'dev'. The PCIDevice * is often 'pci_dev'. I would call the PCIIDEState* 'pci_ide'. I hadn't noticed the use of DEVICE() in ide_bus_new(); you're right that we should be consistent about whether we use the cast macro or the local variable. thanks -- PMM
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 <peter.maydell@linaro.org> >> --- >> 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 <jsnow@redhat.com> The named temporary is fine. We probably should be using a named temporary in the other locations, too. 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. --js
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 <peter.maydell@linaro.org> >>> --- >>> 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 <jsnow@redhat.com> > > > 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 <balaton@eik.bme.hu> 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
On 23/03/2020 15:17, 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 <peter.maydell@linaro.org> > --- > 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)); > > bmdma_init(&s->bus[i], &s->bmdma[i], s); > s->bmdma[i].bus = &s->bus[i]; Looks like there is similar use of qemu_allocate_irqs() in via-ide and cmd646-ide, and also reviewing my latest via-ide changes I spotted a silly mistake which was obviously left in from a previous experimental version. I'm not sure why Coverity doesn't pick up these other occurrences, however I'll send along a patchset for this shortly. ATB, Mark.
On 3/24/20 4:43 PM, Mark Cave-Ayland wrote: > On 23/03/2020 15:17, 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 <peter.maydell@linaro.org> >> --- >> 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)); >> >> bmdma_init(&s->bus[i], &s->bmdma[i], s); >> s->bmdma[i].bus = &s->bus[i]; > > Looks like there is similar use of qemu_allocate_irqs() in via-ide and cmd646-ide, > and also reviewing my latest via-ide changes I spotted a silly mistake which was > obviously left in from a previous experimental version. > > I'm not sure why Coverity doesn't pick up these other occurrences, however I'll send > along a patchset for this shortly. > OK; I will rescind my PR and will re-send it with your patches included. --js
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)); bmdma_init(&s->bus[i], &s->bmdma[i], s); s->bmdma[i].bus = &s->bus[i];
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 <peter.maydell@linaro.org> --- 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(-)