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

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

[Peter Maydell]

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];
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

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>

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

[BALATON Zoltan]

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];
>

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

[Peter Maydell]

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

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

[John Snow]

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

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

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

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

[Mark Cave-Ayland]

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.

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

[John Snow]

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