lkml.org 
[lkml]   [2009]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 08/12] PCI: Introduce /sys/bus/pci/devices/.../remove
    * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
    > Alex Chiang wrote:
    >> * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
    >>> Alex Chiang wrote:
    >>>> +
    >>>> +static void remove_callback(struct device *dev)
    >>>> +{
    >>>> + int bridge = 0;
    >>>> + struct pci_dev *pdev = to_pci_dev(dev);
    >>>> +
    >>>> + mutex_lock(&pci_remove_rescan_mutex);
    >>>> +
    >>>> + if (pdev->subordinate)
    >>>> + bridge = 1;
    >>>> +
    >>>> + pci_remove_bus_device(pdev);
    >>>> + if (bridge && list_empty(&pdev->bus->devices))
    >>>> + pci_remove_bus(pdev->bus);
    >>> I cannot understand the above two lines. Could you explain
    >>> what it intend?
    >>
    >> If the user says:
    >>
    >> echo 1 > /sys/bus/pci/devices/.../remove
    >>
    >> And that device is a bridge, then we need to specifically call
    >> pci_remove_bus as well, to actually remove the bus itself.
    >> Without it, pci_bus_remove_device() will remove all of its
    >> children (and subordinate buses) in a depth-first manner, but we
    >> will never actually remove the bus that the user specified.
    >>
    >
    > Do you mean user removes bridge device to remove its *primary*
    > bus? It is very strange. I think the bus should be removed
    > when its parent bridge is removed.

    You are correct.

    >> In other words, without it, we will still see the bus in:
    >>
    >> /sys/class/pci_bus/...
    >>
    >
    > What is the problem?
    >
    >> We only want to remove the bus if it has no children left. I
    >> think the check for list_empty(&pdev->bus->devices) might be
    >> overkill... I can try taking that bit out and testing again.
    >>
    >
    > I think we don't need the two lines. But if you do that, you
    > need list_empty(&pdev->bus->devices), doesn't it? On the other
    > hand, we must not check 'bridge' in the if statement. Or bus
    > will never be removed when non-bridge device is removed last
    > on the bus.
    >
    > Again, I think we don't need the two lines. But am I
    > misunderstanding something?

    No, you are correct.

    I think what was happening was that I inserted that code before I
    discovered the double-free in the PCIe port driver, and that
    extra call to pci_remove_bus() helped mask the double-free.

    I re-tested again tonight with the port driver fix, and also
    removing the two lines you mention, and it is behaving correctly.

    As always, thanks for your review.

    /ac



    \
     
     \ /
      Last update: 2009-03-20 04:57    [W:2.712 / U:0.236 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site