| Message ID | 1372177330-28013-5-git-send-email-mika.westerberg@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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
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?
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
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
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)