Re: [Qemu-devel] [PATCH v2 09/10] pci/shpc: perform unplug via the hotplug handler

[Igor Mammedov]

On Tue, 20 Nov 2018 11:11:46 +0100
David Hildenbrand <address@hidden> wrote:

> >> diff --git a/hw/pci-bridge/pcie_pci_bridge.c 
> >> b/hw/pci-bridge/pcie_pci_bridge.c
> >> index c634353b06..7c667bc97c 100644
> >> --- a/hw/pci-bridge/pcie_pci_bridge.c
> >> +++ b/hw/pci-bridge/pcie_pci_bridge.c
> >> @@ -150,6 +150,19 @@ static void pcie_pci_bridge_plug_cb(HotplugHandler 
> >> *hotplug_dev,
> >>      shpc_device_plug_cb(hotplug_dev, dev, errp);
> >>  }
> >>  
> >> +static void pcie_pci_bridge_unplug_cb(HotplugHandler *hotplug_dev,
> >> +                                      DeviceState *dev, Error **errp)
> >> +{
> >> +    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
> >> +
> >> +    if (!shpc_present(pci_hotplug_dev)) {
> >> +        error_setg(errp, "standard hotplug controller has been disabled 
> >> for "
> >> +                   "this %s", TYPE_PCIE_PCI_BRIDGE_DEV);  
> > Is it possible to reach here at all?  
> 
> Right, this should right now not be possible. I'll turn this into an
> 
> g_assert(shpc_present(pci_hotplug_dev));
> 
> (if we every support surprise removal, we'll have to rethink either way)
> 
> >   
> >> +        return;
> >> +    }
> >> +    shpc_device_unplug_cb(hotplug_dev, dev, errp);
> >> +}  
> > 
> > you probably can share this function impl. with pci_bridge_dev_unplug_cb()
> > if you use object_get_typename().  
> 
> The same holds for all three functions (+ later pre_plug), however can
> we be sure there won't be differences in the near future?
> 
> If we don't expect these functions to differ, I can add a patch to
> factor the existing two functions (plug/unplug) out.
I'm not sure if they will differ or not in future,
but right now it looks as premature splitting.
It might be better to have a single function now and split it later
when it is necessary.