Re: [PATCH v4 08/12] PCI: Introduce /sys/bus/pci/devices/.../remove

[Alex Chiang <achiang@hp.com>]

* 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