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

login
register
mail settings
Submitter Mika Westerberg
Date June 25, 2013, 4:22 p.m.
Message ID <1372177330-28013-2-git-send-email-mika.westerberg@linux.intel.com>
Download mbox | patch
Permalink /patch/254230/
State Superseded
Headers show

Comments

Mika Westerberg - June 25, 2013, 4:22 p.m.
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

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@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Bjorn Helgaas - June 26, 2013, 11:28 p.m.
On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> 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@linux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  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).

>                 /* configure all functions */
>                 retval = enable_device(slot);
>                 if (retval)
> --
> 1.8.3.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kirill A. Shutemov - June 27, 2013, 1:25 p.m.
Bjorn Helgaas wrote:
> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > 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@linux.intel.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  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).

Okay, we will do more cleanup here.

The tricky part here is that acpiphp_enable_slot() is exposed to userspace
directly: /sys/bus/pci/slots/*/power files.

I wounder if complete drop of the check is an ABI change.
Kirill A. Shutemov - June 28, 2013, 9:51 a.m.
Bjorn Helgaas wrote:
> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > 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@linux.intel.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  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.
Bjorn Helgaas - June 28, 2013, 5 p.m.
On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> Bjorn Helgaas wrote:
>> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> >
>> > 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@linux.intel.com>
>> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> > ---
>> >  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-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki - June 28, 2013, 6:54 p.m.
On Friday, June 28, 2013 11:00:31 AM Bjorn Helgaas wrote:
> On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> > Bjorn Helgaas wrote:
> >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >> >
> >> > 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@linux.intel.com>
> >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >> > ---
> >> >  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.

I agree.

That change may be done in a separate patch, but it should be included in the
series.

Thanks,
Rafael
Mika Westerberg - July 1, 2013, 9:32 a.m.
On Fri, Jun 28, 2013 at 08:54:45PM +0200, Rafael J. Wysocki wrote:
> On Friday, June 28, 2013 11:00:31 AM Bjorn Helgaas wrote:
> > On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov
> > <kirill.shutemov@linux.intel.com> wrote:
> > > Bjorn Helgaas wrote:
> > >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg
> > >> <mika.westerberg@linux.intel.com> wrote:
> > >> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > >> >
> > >> > 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@linux.intel.com>
> > >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > >> > ---
> > >> >  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.
> 
> I agree.
> 
> That change may be done in a separate patch, but it should be included in the
> series.

Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot()
(after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON
anyway, should we remove the whole flag?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki - July 1, 2013, 2:01 p.m.
On Monday, July 01, 2013 12:32:17 PM Mika Westerberg wrote:
> On Fri, Jun 28, 2013 at 08:54:45PM +0200, Rafael J. Wysocki wrote:
> > On Friday, June 28, 2013 11:00:31 AM Bjorn Helgaas wrote:
> > > On Fri, Jun 28, 2013 at 3:51 AM, Kirill A. Shutemov
> > > <kirill.shutemov@linux.intel.com> wrote:
> > > > Bjorn Helgaas wrote:
> > > >> On Tue, Jun 25, 2013 at 10:22 AM, Mika Westerberg
> > > >> <mika.westerberg@linux.intel.com> wrote:
> > > >> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > >> >
> > > >> > 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@linux.intel.com>
> > > >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > >> > ---
> > > >> >  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.
> > 
> > I agree.
> > 
> > That change may be done in a separate patch, but it should be included in the
> > series.
> 
> Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot()
> (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON
> anyway, should we remove the whole flag?

Sure, if it is not necessary any more, we should remove it.

Thanks,
Rafael
Mika Westerberg - July 1, 2013, 6:36 p.m.
On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote:
> > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot()
> > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON
> > anyway, should we remove the whole flag?
> 
> Sure, if it is not necessary any more, we should remove it.

Well, there is one thing that changes due that. Once the flag is gone
userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and
the slot is always re-enumerated.

If that is not acceptable we should probably move the SLOT_ENABLED check
closer to acpiphp_core:enable_device() and drop it from here, so that we
always re-enumerate on Bus Check event but userspace can only do enable
once (we still re-enumerate on Bus Check).
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki - July 2, 2013, 1:29 a.m.
On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote:
> On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote:
> > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot()
> > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON
> > > anyway, should we remove the whole flag?
> > 
> > Sure, if it is not necessary any more, we should remove it.
> 
> Well, there is one thing that changes due that. Once the flag is gone
> userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and
> the slot is always re-enumerated.
> 
> If that is not acceptable we should probably move the SLOT_ENABLED check
> closer to acpiphp_core:enable_device() and drop it from here, so that we
> always re-enumerate on Bus Check event but userspace can only do enable
> once (we still re-enumerate on Bus Check).

Yes, that sounds like the right thing to do.

Thanks,
Rafael
Bjorn Helgaas - July 2, 2013, 4:40 p.m.
On Mon, Jul 1, 2013 at 7:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote:
>> On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote:
>> > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot()
>> > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON
>> > > anyway, should we remove the whole flag?
>> >
>> > Sure, if it is not necessary any more, we should remove it.
>>
>> Well, there is one thing that changes due that. Once the flag is gone
>> userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and
>> the slot is always re-enumerated.
>>
>> If that is not acceptable we should probably move the SLOT_ENABLED check
>> closer to acpiphp_core:enable_device() and drop it from here, so that we
>> always re-enumerate on Bus Check event but userspace can only do enable
>> once (we still re-enumerate on Bus Check).
>
> Yes, that sounds like the right thing to do.

Is it actually a problem if we re-enumerate every time userspace does
'echo 1 > /sys/bus/pci/slots/*/power'?  I assume re-enumeration is a
no-op if nothing has changed.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki - July 2, 2013, 8:29 p.m.
On Tuesday, July 02, 2013 10:40:39 AM Bjorn Helgaas wrote:
> On Mon, Jul 1, 2013 at 7:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote:
> >> On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote:
> >> > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot()
> >> > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON
> >> > > anyway, should we remove the whole flag?
> >> >
> >> > Sure, if it is not necessary any more, we should remove it.
> >>
> >> Well, there is one thing that changes due that. Once the flag is gone
> >> userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and
> >> the slot is always re-enumerated.
> >>
> >> If that is not acceptable we should probably move the SLOT_ENABLED check
> >> closer to acpiphp_core:enable_device() and drop it from here, so that we
> >> always re-enumerate on Bus Check event but userspace can only do enable
> >> once (we still re-enumerate on Bus Check).
> >
> > Yes, that sounds like the right thing to do.
> 
> Is it actually a problem if we re-enumerate every time userspace does
> 'echo 1 > /sys/bus/pci/slots/*/power'?  I assume re-enumeration is a
> no-op if nothing has changed.

Well, if it's a no-op in that case, then re-enumerating shouldn't be a problem,
but is it a no-op?

Rafael (who's not really sure)
Mika Westerberg - July 2, 2013, 8:31 p.m.
On Tue, Jul 02, 2013 at 10:29:12PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 02, 2013 10:40:39 AM Bjorn Helgaas wrote:
> > On Mon, Jul 1, 2013 at 7:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote:
> > >> On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote:
> > >> > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot()
> > >> > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON
> > >> > > anyway, should we remove the whole flag?
> > >> >
> > >> > Sure, if it is not necessary any more, we should remove it.
> > >>
> > >> Well, there is one thing that changes due that. Once the flag is gone
> > >> userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and
> > >> the slot is always re-enumerated.
> > >>
> > >> If that is not acceptable we should probably move the SLOT_ENABLED check
> > >> closer to acpiphp_core:enable_device() and drop it from here, so that we
> > >> always re-enumerate on Bus Check event but userspace can only do enable
> > >> once (we still re-enumerate on Bus Check).
> > >
> > > Yes, that sounds like the right thing to do.
> > 
> > Is it actually a problem if we re-enumerate every time userspace does
> > 'echo 1 > /sys/bus/pci/slots/*/power'?  I assume re-enumeration is a
> > no-op if nothing has changed.
> 
> Well, if it's a no-op in that case, then re-enumerating shouldn't be a problem,
> but is it a no-op?

I can confirm that it's a no-op (at least for the Thunderbolt case).
Basically we just scan for new devices and nothing is to be found.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki - July 2, 2013, 8:49 p.m.
On Tuesday, July 02, 2013 11:31:17 PM Mika Westerberg wrote:
> On Tue, Jul 02, 2013 at 10:29:12PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, July 02, 2013 10:40:39 AM Bjorn Helgaas wrote:
> > > On Mon, Jul 1, 2013 at 7:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > On Monday, July 01, 2013 09:36:13 PM Mika Westerberg wrote:
> > > >> On Mon, Jul 01, 2013 at 04:01:37PM +0200, Rafael J. Wysocki wrote:
> > > >> > > Given the fact that SLOT_ENABLED is only checked in acpiphp_enable_slot()
> > > >> > > (after this patch) and that /sys/bus/pci/slots/*/power uses SLOT_POWEREDON
> > > >> > > anyway, should we remove the whole flag?
> > > >> >
> > > >> > Sure, if it is not necessary any more, we should remove it.
> > > >>
> > > >> Well, there is one thing that changes due that. Once the flag is gone
> > > >> userspace can do 'echo 1 > /sys/bus/pci/slots/*/power' several times and
> > > >> the slot is always re-enumerated.
> > > >>
> > > >> If that is not acceptable we should probably move the SLOT_ENABLED check
> > > >> closer to acpiphp_core:enable_device() and drop it from here, so that we
> > > >> always re-enumerate on Bus Check event but userspace can only do enable
> > > >> once (we still re-enumerate on Bus Check).
> > > >
> > > > Yes, that sounds like the right thing to do.
> > > 
> > > Is it actually a problem if we re-enumerate every time userspace does
> > > 'echo 1 > /sys/bus/pci/slots/*/power'?  I assume re-enumeration is a
> > > no-op if nothing has changed.
> > 
> > Well, if it's a no-op in that case, then re-enumerating shouldn't be a problem,
> > but is it a no-op?
> 
> I can confirm that it's a no-op (at least for the Thunderbolt case).
> Basically we just scan for new devices and nothing is to be found.

We can try to drop SLOT_ENABLED entirely then I suppose.

Thanks,
Rafael

Patch

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;
 		/* configure all functions */
 		retval = enable_device(slot);
 		if (retval)