Re: [Qemu-devel] [PATCH v2 1/6] s390x/pci: Use hotplug_dev instead of looking up the host bridge

[Cornelia Huck]

On Tue, 15 Jan 2019 15:57:38 -0500
Collin Walling <address@hidden> wrote:

> On 1/14/19 5:31 AM, David Hildenbrand wrote:
> > We directly have it in our hands.
> > 
> > Signed-off-by: David Hildenbrand <address@hidden>
> > ---
> >   hw/s390x/s390-pci-bus.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 7f911b216a..86dda831f9 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -826,9 +826,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, 
> > S390PCIBusDevice *pbdev)
> >   static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState 
> > *dev,
> >                                 Error **errp)
> >   {
> > +    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
> >       PCIDevice *pdev = NULL;
> >       S390PCIBusDevice *pbdev = NULL;
> > -    S390pciState *s = s390_get_phb();
> >   
> >       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> >           BusState *bus;
> > @@ -935,11 +935,11 @@ static void s390_pcihost_timer_cb(void *opaque)
> >   static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState 
> > *dev,
> >                                   Error **errp)
> >   {
> > +    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
> >       PCIDevice *pci_dev = NULL;
> >       PCIBus *bus;
> >       int32_t devfn;
> >       S390PCIBusDevice *pbdev = NULL;
> > -    S390pciState *s = s390_get_phb();
> >   
> >       if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> >           error_setg(errp, "PCI bridge hot unplug currently not supported");
> >   
> 
> Looks like the macro will do the same thing as the function does? I 
> wonder if it makes sense to one day replace all function calls with the 
> macro.

The idea behind the function was to do some caching. Opinions on that
differ :)

> 
> Reviewed-by: Collin Walling <address@hidden>
> 

If you like this patch, I don't really object to merging it :)