Re: [PATCH 2/8] hw/ide: Get rid of piix4_init function

[BALATON Zoltan]

On Sat, 14 Mar 2020, Philippe Mathieu-Daudé wrote:
> On 3/13/20 10:14 PM, BALATON Zoltan wrote:
> > This removes pci_piix4_ide_init() function similar to clean up done to
> > other ide devices.
> >
> > Signed-off-by: BALATON Zoltan <address@hidden>
> > ---
> >   hw/ide/piix.c    | 12 +-----------
> >   hw/isa/piix4.c   |  5 ++++-
> >   include/hw/ide.h |  1 -
> >   3 files changed, 5 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > index 8bcd6b72c2..3b2de4c312 100644
> > --- a/hw/ide/piix.c
> > +++ b/hw/ide/piix.c
> > @@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
> >       }
> >   }
> >   -/* hd_table must contain 4 block drivers */
> > -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> > -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
> > devfn)
> > -{
> > -    PCIDevice *dev;
> > -
> > -    dev = pci_create_simple(bus, devfn, "piix4-ide");
> > -    pci_ide_create_devs(dev, hd_table);
> > -    return dev;
> > -}
> > -
> >   /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
> >   static void piix3_ide_class_init(ObjectClass *klass, void *data)
> >   {
> > @@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
> >       .class_init    = piix3_ide_class_init,
> >   };
> >   +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> >   static void piix4_ide_class_init(ObjectClass *klass, void *data)
> >   {
> >       DeviceClass *dc = DEVICE_CLASS(klass);
> > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> > index 7edec5e149..0ab4787658 100644
> > --- a/hw/isa/piix4.c
> > +++ b/hw/isa/piix4.c
> > @@ -35,6 +35,7 @@
> >   #include "hw/timer/i8254.h"
> >   #include "hw/rtc/mc146818rtc.h"
> >   #include "hw/ide.h"
> > +#include "hw/ide/pci.h"
> >   #include "migration/vmstate.h"
> >   #include "sysemu/reset.h"
> >   #include "sysemu/runstate.h"
> > @@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
> > **isa_bus,
> >           *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> >       }
> >   +    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
>
> Why are you re-assigning 'pci'?

Need a place to store it to pass to pci_ide_create_devs below and pci is 
unused at this point so it can be reused for this.  (The variable pci 
pointing to a PCIDevice was only used at the beginning of the function to 
cast to dev then it's not needed any more.) Since this is very short func 
and the reassign is right after its previous usage this should not be too 
confusing and avoids needing to define another only once used variable fot 
this. See also patch 6 (http://patchwork.ozlabs.org/patch/1254687/) that 
simplifies it further.

We could also do without this variable and write:

dev = DEVICE(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
             true, TYPE_PIIX4_PCI_DEVICE));

or after patch 6 even

pci_ide_create_devs(pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide"));

but I think those are less readable than reusing variable pci here.

Regards,
BALATON Zoltan

> >       hd = g_new(DriveInfo *, ide_drives);
> >       ide_drive_get(hd, ide_drives);
> > -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
> > +    pci_ide_create_devs(pci, hd);
> >       g_free(hd);
> > +
> >       pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
> >       if (smbus) {
> >           *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
> > diff --git a/include/hw/ide.h b/include/hw/ide.h
> > index 883bbaeb9b..21bd8f23f1 100644
> > --- a/include/hw/ide.h
> > +++ b/include/hw/ide.h
> > @@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int 
> > iobase2, int isairq,
> >                           DriveInfo *hd0, DriveInfo *hd1);
> >     /* ide-pci.c */
> > -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
> > devfn);
> >   int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
> >     /* ide-mmio.c */