a small problem about pci_stop_and_remove_bus_device() function

Hi Bjorn,
The drivers/pci/remove.c now is so clean after applied series "[PATCH v2 00/16] Clean up drivers/pci/remove.c"(very good ^_^).
I have a small problem about pci_stop_and_remove_bus_device() function. Traditionally, pci_bus is removed after its pci bridge stop. But now pci_bus is remove before its pci brdige. So If I use pci_bus_type notifier, I can't identify pci_dev whether is bridge(by pci_dev->subordinate) when notifier arrives.
What about move pci_remove_bus after stop pci bridge?

82 if (bus) {
83 list_for_each_entry_safe_reverse(child, tmp,
84 &bus->devices, bus_list)
85 pci_stop_and_remove_bus_device(child);
86
87 }
88
89 pci_stop_dev(dev);
90 if (bus) {
91 pci_remove_bus(bus);
92 dev->subordinate = NULL;
93 }
94 pci_destroy_dev(dev);

Thanks!
Yijing

Let me restate this to make sure I understand the problem. Prior to my cleanups, the flow when removing a bridge device was like this:

    pci_stop_and_remove_bus_device(bridge)
        pci_stop_bus_device(bridge)
            pci_stop_bus_devices(bridge->subordinate)
            pci_stop_dev(bridge)
                device_unregister(bridge->dev)
                    device_del(bridge->dev)
                        blocking_notifier_call_chain(pci_bus_type, ...)
                            <notifier>
                                if (dev->bus == &pci_bus_type)
                                    pdev = to_pci_dev(dev)
                                    if (pdev->subordinate)
                                        ...
        __pci_remove_bus_device(bridge)
            if (bridge->subordinate)
                pci_remove_bus(bridge->subordinate)
                bridge->subordinate = NULL

After my changes, the flow is like this:

    pci_stop_and_remove_bus_device(bridge)
        pci_remove_bus(bridge->subordinate)
        bridge->subordinate = NULL
        pci_stop_dev(bridge)
            device_unregister(bridge->dev)
                device_del(bridge->dev)
                    blocking_notifier_call_chain(pci_bus_type, ...)
                        <notifier>
                            if (dev->bus == &pci_bus_type)
                                pdev = to_pci_dev(dev)
                                if (pdev->subordinate)
                                    ** broken because

pdev->subordinate is always NULL **

The problem is that before, a notifier could check "dev->subordinate" to identify a bridge device, but now, "dev->subordinate" has already been cleared by the time the notifier runs, so that test no longer identifies bridges.

I didn't intend this change in behavior, so in that sense, this is a regression. We could restore the previous behavior by changing pci_stop_and_remove_bus_device() so the "pci_remove_bus(); dev->subordinate = NULL;" code is after the pci_stop_dev() call, as you suggest.

But I'm not 100% sure that's the correct fix. I think it's possible to have a bridge device that has no secondary bus. For example, I don't think a bridge configured with "secondary > subordinate", e.g., "[bus ff-00]", will forward any config transactions downstream. In that case, we'll have a struct pci_dev for the bridge device, but there won't be a struct pci_bus for the secondary bus, so dev->subordinate will be NULL. There are actually quite a few existing tests for this situation in pnv_ioda_setup_bus_PE(), eeh_add_device_tree_late(), yenta_probe(), etc.

When we enumerate bridges, we build the bridge pci_dev before building the downstream pci_bus, so symmetry suggests that we should tear down the pci_bus before tearing down the pci_dev.

So I wonder if a better fix is remove the assumption that "dev->subordinate != NULL means this is a bridge device." There are many places where we test "dev->subordinate" to iterate through downstream devices or something similar; those should be fine. We'd only have to change the places that care about actual type of the device, e.g., the config space differences between header types.

Where did you trip over this? If you just found this by inspection, my congratulations, it's a pretty subtle issue :)

HI Bjorn,
This issue was disclosed when developing the patch set "[PATCH v2 0/9]
enhance PCI related drivers to handle hotplug events". BTW, I have sent this patch set to you just now.

	We are not assume that dev->subordinate is not NULL for a bridge device,

but that dev->subordinate is consistent when PCI bus notification callbacks are called for BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE.
Thanks!
Gerry