Re: [PATCH v2 20/37] PCI: Add pci bus removal through /sys/.../pci_bus/.../remove

[Bjorn Helgaas]

On Mon, Mar 12, 2012 at 11:54 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> On Mon, Mar 12, 2012 at 8:10 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
>> On Sat, Mar 10, 2012 at 12:00 AM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>>> it supports both pci root bus and pci bus under pci bridge.
>>>
>>> -v2: Change to three returns way in dev_bus_remove_store.
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
>>> Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxx>
>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> Cc: linux-doc@xxxxxxxxxxxxxxx
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-pci |    8 ++++++++
>>>  drivers/pci/pci-sysfs.c                 |   30 ++++++++++++++++++++++++++++++
>>>  2 files changed, 38 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
>>> index 95f0f37..22392de 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-pci
>>> +++ b/Documentation/ABI/testing/sysfs-bus-pci
>>> @@ -92,6 +92,14 @@ Description:
>>>                hot-remove the PCI device and any of its children.
>>>                Depends on CONFIG_HOTPLUG.
>>>
>>> +What:          /sys/bus/pci/devices/.../pci_bus/.../remove
>>> +Date:          March 2012
>>> +Contact:       Linux PCI developers <linux-pci@xxxxxxxxxxxxxxx>
>>> +Description:
>>> +               Writing a non-zero value to this attribute will
>>> +               hot-remove the PCI bus and any of its children.
>>> +               Depends on CONFIG_HOTPLUG.
>>
>> I don't think this interface makes sense.  This stops every device on
>> the bus, removes the bus, unregisters the host bridge device if we're
>> removing a root bus.
>
> it is some kind of handy. otherwise if there is several devices on the
> bus, will need to
> remove the devices one by one.

It might be handy, but that's not enough.  You're doing a sysfs
operation in the PCI namespace.  It should not have effects outside
that namespace.  Host bridges are by definition outside the PCI
namespace.

>> I think we should just have a "remove *bridge*" interface.  It makes
>> no sense to me to remove a bus but leave the bridge, and it makes the
>> code quite complicated.
>
> I want to remove all devices under the bridge and then rescan the
> bridge. then if the bridge get removed.
> then we have to rescan bridge's parent bus.
>
>>
>> The point of hotplug is to add and remove devices.  We'll get
>> interrupts saying "please remove this device" or "please rescan the
>> bus behind this bridge because something might have changed."
>>
>> If we're removing a host bridge, the platform, e.g., ACPI, will tell
>> us about the bridge that is going away.  The notification is attached
>> to the *bridge*, not the bus it leads to.
>>
>> The expected mechanism on x86 is an SCI from the platform, but if we
>> need a sysfs way to start host bridge removal, I think it should be
>> something like this:
>>
>>    echo 1 > /sys/devices/LNXSYSTM:00/device:00/PNP0A08:00/remove
>
> for that,  let just use
> echo "\_SB.PCI0 3" > /proc/acpi/sci/notify

That's a totally broken user interface.  There's no way for the user
to look at the sysfs tree and figure out how to do that.  The ACPI
namespace names like "PCI0" are platform-dependent internal things and
should not be part of any user interface.

I don't object to /proc/acpi/sci/notify.  That looks like a useful way
to inject notification events that would normally come from the
hardware/BIOS.  But it's a development tool, not something that can be
part of the normal user interface.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/