Re: [PATCH 1/6] PCI: acpiphp: do not check for SLOT_ENABLED in enable_device()

[Bjorn Helgaas]

On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov
<kirill.shutemov@xxxxxxxxxxxxxxx> wrote:
> Bjorn Helgaas wrote:
>> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg
>> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>> > From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
>> >
>> > With Thunderbolt you can chain devices: connect a new devices to plugged
>> > one. In this case the slot is already enabled, but we still want to look
>> > for new devices behind it.
>> >
>> > We're going to reuse enable_device() for rescan for new devices on the
>> > enabled slot. Let's push the check up by stack.
>> >
>> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
>> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>> > ---
>> >  drivers/pci/hotplug/acpiphp_glue.c | 5 ++---
>> >  1 file changed, 2 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> > index 59df857..b983e29 100644
>> > --- a/drivers/pci/hotplug/acpiphp_glue.c
>> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> > @@ -688,9 +688,6 @@ static int __ref enable_device(struct acpiphp_slot *slot)
>> >         int num, max, pass;
>> >         LIST_HEAD(add_list);
>> >
>> > -       if (slot->flags & SLOT_ENABLED)
>> > -               goto err_exit;
>> > -
>> >         list_for_each_entry(func, &slot->funcs, sibling)
>> >                 acpiphp_bus_add(func);
>> >
>> > @@ -1242,6 +1239,8 @@ int acpiphp_enable_slot(struct acpiphp_slot *slot)
>> >                 goto err_exit;
>> >
>> >         if (get_slot_status(slot) == ACPI_STA_ALL) {
>> > +               if (slot->flags & SLOT_ENABLED)
>> > +                       goto err_exit;
>>
>> Why do we check for SLOT_ENABLED at all?  I think we're handling a Bus
>> Check notification, which means "re-enumerate on the device tree
>> starting from the notification point."  It doesn't say anything about
>> skipping the re-enumeration if we find a device that's already
>> enabled.
>>
>> It seems like we ought to just re-enumerate all the way down in case a
>> device was added farther down in the tree (which is what it sounds
>> like Thunderbolt is doing).
>
> Currently (with patchset applied), we have two users of
> acpiphp_enable_slot():
>
> - /sys/bus/pci/slots/*/power
> - ACPI_NOTIFY_BUS_CHECK in _handle_hotplug_event_func().
>
> Both are not related to Thunderbolt.
>
> Although, I think remove the check is good idea, I prefer to keep it
> separate from Thunderbolt enabling patchset, since it will change sysfs
> ABI a bit and can potentially affect othe ACPI PCI hotplug
> implementations.

I'll think about this some more, but if we can make a change that
simplifies things and makes them more spec-compliant, and also happens
to make Thunderbolt work, that sounds better than fixing Thunderbolt
while leaving the wart there.

If we only fix Thunderbolt, it just feels like we're adding to an
ever-growing "deferred maintenance" list.

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/