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

[David Hildenbrand]

On 20.11.18 15:13, Igor Mammedov wrote:
> 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.
> 

Indeed, I already went ahead and sent a new version including the
suggested re-factoring.

Thanks!

-- 

Thanks,

David / dhildenb