a small problem about pci_stop_and_remove_bus_device() function

a small problem about pci_stop_and_remove_bus_device() function

[Yijing Wang]

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

Re: a small problem about pci_stop_and_remove_bus_device() function

[Bjorn Helgaas]

On Wed, Aug 29, 2012 at 2:58 AM, Yijing Wang <wangyijing@huawei.com> wrote:
> 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.

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 :)

> 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
>

Re: a small problem about pci_stop_and_remove_bus_device() function

[Jiang Liu]

On 09/14/2012 12:43 AM, Bjorn Helgaas wrote:
> 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