Patchwork [4/6] PCI: acpiphp: check for new devices on enabled host

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

Comments

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

Current acpiphp_check_bridge() implementation is pretty dumb:
 - it enables the slot if it's not enabled and the slot status is
   ACPI_STA_ALL;
 - it disables the slot if it's enabled and slot is not in ACPI_STA_ALL
   state.

This behavior is not enough to handle Thunderbolt chaining case
properly. We need to actually rescan for new devices even if a device
has already in the slot.

The new implementation disables and stops the slot if it's not in
ACPI_STA_ALL state.

For ACPI_STA_ALL state we first trim devices which don't respond and
look for the ones after that. We do that even if slot already enabled
(SLOT_ENABLED).

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 | 56 ++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 29 deletions(-)
Andy Shevchenko - June 25, 2013, 6:04 p.m.
On Tue, Jun 25, 2013 at 7:22 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> Current acpiphp_check_bridge() implementation is pretty dumb:
>  - it enables the slot if it's not enabled and the slot status is
>    ACPI_STA_ALL;
>  - it disables the slot if it's enabled and slot is not in ACPI_STA_ALL
>    state.
>
> This behavior is not enough to handle Thunderbolt chaining case
> properly. We need to actually rescan for new devices even if a device
> has already in the slot.
>
> The new implementation disables and stops the slot if it's not in
> ACPI_STA_ALL state.
>
> For ACPI_STA_ALL state we first trim devices which don't respond and
> look for the ones after that. We do that even if slot already enabled
> (SLOT_ENABLED).

Just a couple of nitpicks below.

>         list_for_each_entry(slot, &bridge->slots, node) {
> +               struct pci_bus *bus = slot->bridge->pci_bus;
> +               struct pci_dev *dev, *tmp;
> +               int retval;
> +
> +               mutex_lock(&slot->crit_sect);

Does it make sense to introduce a helper let's say
__acpiphp_check_slot() and put there all lines inside this mutex?

> +               if (get_slot_status(slot) == ACPI_STA_ALL) {
> +                       /* remove stale devices if any */
> +                       list_for_each_entry_safe(dev, tmp,
> +                                       &bus->devices, bus_list) {
> +                               if (PCI_SLOT(dev->devfn) != slot->device)
> +                                       continue;
> +                               pci_trim_stale_devices(dev);

Perhaps

 list_for_each_entry_safe(dev, tmp, &bus->devices, bus_list) {
      if (PCI_SLOT(dev->devfn) == slot->device)
                pci_trim_stale_devices(dev);
 }

--
With Best Regards,
Andy Shevchenko
--
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 26, 2013, 9:39 a.m.
Andy Shevchenko wrote:
> On Tue, Jun 25, 2013 at 7:22 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > Current acpiphp_check_bridge() implementation is pretty dumb:
> >  - it enables the slot if it's not enabled and the slot status is
> >    ACPI_STA_ALL;
> >  - it disables the slot if it's enabled and slot is not in ACPI_STA_ALL
> >    state.
> >
> > This behavior is not enough to handle Thunderbolt chaining case
> > properly. We need to actually rescan for new devices even if a device
> > has already in the slot.
> >
> > The new implementation disables and stops the slot if it's not in
> > ACPI_STA_ALL state.
> >
> > For ACPI_STA_ALL state we first trim devices which don't respond and
> > look for the ones after that. We do that even if slot already enabled
> > (SLOT_ENABLED).
> 
> Just a couple of nitpicks below.
> 
> >         list_for_each_entry(slot, &bridge->slots, node) {
> > +               struct pci_bus *bus = slot->bridge->pci_bus;
> > +               struct pci_dev *dev, *tmp;
> > +               int retval;
> > +
> > +               mutex_lock(&slot->crit_sect);
> 
> Does it make sense to introduce a helper let's say
> __acpiphp_check_slot() and put there all lines inside this mutex?

No. Why?

> > +               if (get_slot_status(slot) == ACPI_STA_ALL) {
> > +                       /* remove stale devices if any */
> > +                       list_for_each_entry_safe(dev, tmp,
> > +                                       &bus->devices, bus_list) {
> > +                               if (PCI_SLOT(dev->devfn) != slot->device)
> > +                                       continue;
> > +                               pci_trim_stale_devices(dev);
> 
> Perhaps
> 
>  list_for_each_entry_safe(dev, tmp, &bus->devices, bus_list) {
>       if (PCI_SLOT(dev->devfn) == slot->device)
>                 pci_trim_stale_devices(dev);
>  }

Makes sense, thanks.
Yinghai Lu - June 27, 2013, 7:05 p.m.
On Tue, Jun 25, 2013 at 9:22 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> Current acpiphp_check_bridge() implementation is pretty dumb:
>  - it enables the slot if it's not enabled and the slot status is
>    ACPI_STA_ALL;
>  - it disables the slot if it's enabled and slot is not in ACPI_STA_ALL
>    state.
>
> This behavior is not enough to handle Thunderbolt chaining case
> properly. We need to actually rescan for new devices even if a device
> has already in the slot.
>
> The new implementation disables and stops the slot if it's not in
> ACPI_STA_ALL state.
>
> For ACPI_STA_ALL state we first trim devices which don't respond and
> look for the ones after that. We do that even if slot already enabled
> (SLOT_ENABLED).

that is not right, some time BUS_CHECK is even sent root bus.
in that case, stop all devices in slots and load driver again.

like you put one card in one slots, but all devices in other slots get stop
and enable again.

Thanks

Yinghai
--
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 28, 2013, 9:33 a.m.
Yinghai Lu wrote:
> On Tue, Jun 25, 2013 at 9:22 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > Current acpiphp_check_bridge() implementation is pretty dumb:
> >  - it enables the slot if it's not enabled and the slot status is
> >    ACPI_STA_ALL;
> >  - it disables the slot if it's enabled and slot is not in ACPI_STA_ALL
> >    state.
> >
> > This behavior is not enough to handle Thunderbolt chaining case
> > properly. We need to actually rescan for new devices even if a device
> > has already in the slot.
> >
> > The new implementation disables and stops the slot if it's not in
> > ACPI_STA_ALL state.
> >
> > For ACPI_STA_ALL state we first trim devices which don't respond and
> > look for the ones after that. We do that even if slot already enabled
> > (SLOT_ENABLED).
> 
> that is not right, some time BUS_CHECK is even sent root bus.
> in that case, stop all devices in slots and load driver again.
> 
> like you put one card in one slots, but all devices in other slots get stop
> and enable again.

We don't stop enabled devices, we only stop and remove devices which don't
respond.  See patch 3/6.

I don't see how it's harmful. Do you?
Yinghai Lu - June 28, 2013, 4:22 p.m.
On Fri, Jun 28, 2013 at 2:33 AM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> Yinghai Lu wrote:
>> On Tue, Jun 25, 2013 at 9:22 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> >
>> > Current acpiphp_check_bridge() implementation is pretty dumb:
>> >  - it enables the slot if it's not enabled and the slot status is
>> >    ACPI_STA_ALL;
>> >  - it disables the slot if it's enabled and slot is not in ACPI_STA_ALL
>> >    state.
>> >
>> > This behavior is not enough to handle Thunderbolt chaining case
>> > properly. We need to actually rescan for new devices even if a device
>> > has already in the slot.
>> >
>> > The new implementation disables and stops the slot if it's not in
>> > ACPI_STA_ALL state.
>> >
>> > For ACPI_STA_ALL state we first trim devices which don't respond and
>> > look for the ones after that. We do that even if slot already enabled
>> > (SLOT_ENABLED).
>>
>> that is not right, some time BUS_CHECK is even sent root bus.
>> in that case, stop all devices in slots and load driver again.
>>
>> like you put one card in one slots, but all devices in other slots get stop
>> and enable again.
>
> We don't stop enabled devices, we only stop and remove devices which don't
> respond.  See patch 3/6.
>
> I don't see how it's harmful. Do you?

then please check with disable_device to put back pci_dev ref,
also may need to trim corresponding acpi devices.

so this patch is helping: multiple plug-in and remove?

Yinghai
--
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, 8:04 p.m.
On Friday, June 28, 2013 09:22:32 AM Yinghai Lu wrote:
> On Fri, Jun 28, 2013 at 2:33 AM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> > Yinghai Lu wrote:
> >> On Tue, Jun 25, 2013 at 9:22 AM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >> >
> >> > Current acpiphp_check_bridge() implementation is pretty dumb:
> >> >  - it enables the slot if it's not enabled and the slot status is
> >> >    ACPI_STA_ALL;
> >> >  - it disables the slot if it's enabled and slot is not in ACPI_STA_ALL
> >> >    state.
> >> >
> >> > This behavior is not enough to handle Thunderbolt chaining case
> >> > properly. We need to actually rescan for new devices even if a device
> >> > has already in the slot.
> >> >
> >> > The new implementation disables and stops the slot if it's not in
> >> > ACPI_STA_ALL state.
> >> >
> >> > For ACPI_STA_ALL state we first trim devices which don't respond and
> >> > look for the ones after that. We do that even if slot already enabled
> >> > (SLOT_ENABLED).
> >>
> >> that is not right, some time BUS_CHECK is even sent root bus.
> >> in that case, stop all devices in slots and load driver again.
> >>
> >> like you put one card in one slots, but all devices in other slots get stop
> >> and enable again.
> >
> > We don't stop enabled devices, we only stop and remove devices which don't
> > respond.  See patch 3/6.
> >
> > I don't see how it's harmful. Do you?
> 
> then please check with disable_device to put back pci_dev ref,
> also may need to trim corresponding acpi devices.

That's correct.  Thunderbolt may not need that, but ACPI device objects need
to be trimmed too in general.

> so this patch is helping: multiple plug-in and remove?

I think it helps the case when a Thunderbolt device is removed and we only
get a bus check notify for the controller after the fact.

Thanks,
Rafael

Patch

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 80a6ea1..82a4ec9 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -868,43 +868,41 @@  int acpiphp_eject_slot(struct acpiphp_slot *slot)
  * Iterate over all slots under this bridge and make sure that if a
  * card is present they are enabled, and if not they are disabled.
  */
-static int acpiphp_check_bridge(struct acpiphp_bridge *bridge)
+static void acpiphp_check_bridge(struct acpiphp_bridge *bridge)
 {
 	struct acpiphp_slot *slot;
-	int retval = 0;
-	int enabled, disabled;
-
-	enabled = disabled = 0;
 
 	list_for_each_entry(slot, &bridge->slots, node) {
-		unsigned int status = get_slot_status(slot);
-		if (slot->flags & SLOT_ENABLED) {
-			if (status == ACPI_STA_ALL)
-				continue;
-			retval = acpiphp_disable_slot(slot);
-			if (retval) {
-				err("Error occurred in disabling\n");
-				goto err_exit;
-			} else {
-				acpiphp_eject_slot(slot);
+		struct pci_bus *bus = slot->bridge->pci_bus;
+		struct pci_dev *dev, *tmp;
+		int retval;
+
+		mutex_lock(&slot->crit_sect);
+		/* wake up all functions */
+		retval = power_on_slot(slot);
+		if (retval)
+			goto unlock;
+
+		if (get_slot_status(slot) == ACPI_STA_ALL) {
+			/* remove stale devices if any */
+			list_for_each_entry_safe(dev, tmp,
+					&bus->devices, bus_list) {
+				if (PCI_SLOT(dev->devfn) != slot->device)
+					continue;
+				pci_trim_stale_devices(dev);
 			}
-			disabled++;
+
+			/* configure all functions */
+			retval = enable_device(slot);
+			if (retval)
+				power_off_slot(slot);
 		} else {
-			if (status != ACPI_STA_ALL)
-				continue;
-			retval = acpiphp_enable_slot(slot);
-			if (retval) {
-				err("Error occurred in enabling\n");
-				goto err_exit;
-			}
-			enabled++;
+			disable_device(slot);
+			power_off_slot(slot);
 		}
+unlock:
+		mutex_unlock(&slot->crit_sect);
 	}
-
-	dbg("%s: %d enabled, %d disabled\n", __func__, enabled, disabled);
-
- err_exit:
-	return retval;
 }
 
 static void acpiphp_set_hpp_values(struct pci_bus *bus)