Re: [Qemu-devel] [PATCH v1 0/7] pci: hotplug handler reworks

[David Hildenbrand]

On 01.11.18 15:55, Igor Mammedov wrote:
> On Wed, 31 Oct 2018 18:31:30 +0100
> David Hildenbrand <address@hidden> wrote:
> 
>> On 24.10.18 12:19, David Hildenbrand wrote:
>>> This series reworks some pci hotplug handlers (except for s390, that will
>>> require more work but is not required for now).
>>>
>>> 1. Route all unplug calls via the hotplug handler when called from the
>>>    unplug_request handler. This will be required to get multi-stage
>>>    hotplug handlers running, but also makes sense on its own (just like we
>>>    already did for some CPU/memory hotplug handlers).
>>>
>>> 2. Introduce some pre_plug handlers where it makes sense already.
>>>
>>> 3. Call the plug/pre_plug handler also for coldplugged devices. Especially
>>>    pcihp is special as it overwrites hotplug handlers.
>>>
>>> This series will not yet factor out pre_plug/plug/unplug from pci device
>>> realize/unrealize functions, this will require more work but this
>>> series is also required first to get it running.
>>>
>>> David Hildenbrand (7):
>>>   pcihp: perform check for bus capability in pre_plug handler
>>>   pcihp: overwrite hotplug handler recursively from the start
>>>   pcihp: route unplug via the hotplug handler
>>>   pci/pcie: route unplug via the hotplug handler
>>>   pci/shpc: move hotplug checks to preplug handler
>>>   pci/shpc: route unplug via the hotplug handler
>>>   spapr_pci: route unplug via the hotplug handler
>>>
>>>  hw/acpi/pcihp.c                 | 40 +++++++++++++++++++++++-----
>>>  hw/acpi/piix4.c                 | 39 ++++++++++++++-------------
>>>  hw/pci-bridge/pci_bridge_dev.c  | 23 +++++++++++++++-
>>>  hw/pci-bridge/pcie_pci_bridge.c | 23 +++++++++++++++-
>>>  hw/pci/pcie.c                   | 10 ++++++-
>>>  hw/pci/pcie_port.c              |  1 +
>>>  hw/pci/shpc.c                   | 47 ++++++++++++++++++---------------
>>>  hw/ppc/spapr_pci.c              | 33 ++++++++++++++---------
>>>  include/hw/acpi/pcihp.h         |  5 ++++
>>>  include/hw/pci/pcie.h           |  2 ++
>>>  include/hw/pci/shpc.h           |  4 +++
>>>  11 files changed, 165 insertions(+), 62 deletions(-)
>>>   
>>
>> Did some more testing. Can somebody have a look/pick up? Thanks!
> Did a quick pass over series, patches overall looks good here is some other 
> nits
> that apply to series:

Thanks for the review, will address the comments tomorrow!

>  * make more descriptive commit messages (important for history and for 
> whoever comes later to read it)

Yes, I'll add more details. I agree that people without context might
have a harder time figuring out why this is done.

>  * I don't really like a mix of style in callbacks naming
>     for ex: there is hotplug and then for unplug you add hot_unplug instead 
> of hotunplug
>     I'd prefer consistent approach in naming.
>     (either use underscores or drop them or maybe drop 'hot' part as it's not 
> only for hotplug anymore)

Yes, me too. And I guess I will add some cleanup patches first, that
will introduce a common naming scheme, (especially dropping the "hot")


-- 

Thanks,

David / dhildenb