| Message ID | 1372177330-28013-3-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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> > > pci_scan_slot() returns number of new devices connected *directly* > connected to the slot. Current enable_device() checks the return value > and stops if it doesn't see a new device. > > In Thunderbolt chaining case the new device can be deeper in hierarchy, so > do the rescan anyway. > > Because of that we must make sure that pcibios_resource_survey_bus() and > check_hotplug_bridge() get called only for a just found bus and not the > ones already added to the system. Failure to do so will lead to resource > conflicts. > > 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 | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > index b983e29..80a6ea1 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -685,18 +685,13 @@ static int __ref enable_device(struct acpiphp_slot *slot) > struct pci_dev *dev; > struct pci_bus *bus = slot->bridge->pci_bus; > struct acpiphp_func *func; > - int num, max, pass; > + int max, pass; > LIST_HEAD(add_list); > > list_for_each_entry(func, &slot->funcs, sibling) > acpiphp_bus_add(func); > > - num = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); > - if (num == 0) { > - /* Maybe only part of funcs are added. */ > - dbg("No new device found\n"); > - goto err_exit; > - } > + pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); > > max = acpiphp_max_busnr(bus); > for (pass = 0; pass < 2; pass++) { I think this is two logical changes: the change below looks like it could by done by itself first, followed by the change above. > @@ -707,8 +702,11 @@ static int __ref enable_device(struct acpiphp_slot *slot) > dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) { > max = pci_scan_bridge(bus, dev, max, pass); > if (pass && dev->subordinate) { > - check_hotplug_bridge(slot, dev); > - pcibios_resource_survey_bus(dev->subordinate); > + if (!dev->subordinate->is_added) { > + check_hotplug_bridge(slot, dev); > + pcibios_resource_survey_bus( > + dev->subordinate); It's a shame that pcibios_resource_survey_bus() can't be called twice. It would be nice if it were smart enough to notice on the second call that "oh, resources have already been assigned, so there's nothing more to be done." Did you look to see whether that's feasible? > + } > __pci_bus_size_bridges(dev->subordinate, > &add_list); > } > @@ -742,8 +740,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) > } > } > > - > - err_exit: > return 0; > } > > -- > 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
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> > > pci_scan_slot() returns number of new devices connected *directly* > connected to the slot. Current enable_device() checks the return value > and stops if it doesn't see a new device. > > In Thunderbolt chaining case the new device can be deeper in hierarchy, so > do the rescan anyway. > > Because of that we must make sure that pcibios_resource_survey_bus() and > check_hotplug_bridge() get called only for a just found bus and not the > ones already added to the system. Failure to do so will lead to resource > conflicts. > > 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 | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > index b983e29..80a6ea1 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -685,18 +685,13 @@ static int __ref enable_device(struct acpiphp_slot *slot) > struct pci_dev *dev; > struct pci_bus *bus = slot->bridge->pci_bus; > struct acpiphp_func *func; > - int num, max, pass; > + int max, pass; > LIST_HEAD(add_list); > > list_for_each_entry(func, &slot->funcs, sibling) > acpiphp_bus_add(func); > > - num = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); > - if (num == 0) { > - /* Maybe only part of funcs are added. */ > - dbg("No new device found\n"); > - goto err_exit; > - } > + pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); > > max = acpiphp_max_busnr(bus); > for (pass = 0; pass < 2; pass++) { > @@ -707,8 +702,11 @@ static int __ref enable_device(struct acpiphp_slot *slot) > dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) { > max = pci_scan_bridge(bus, dev, max, pass); > if (pass && dev->subordinate) { > - check_hotplug_bridge(slot, dev); > - pcibios_resource_survey_bus(dev->subordinate); > + if (!dev->subordinate->is_added) { > + check_hotplug_bridge(slot, dev); > + pcibios_resource_survey_bus( > + dev->subordinate); > + } No, this change will totally disable calling check_hotplug_bridge/pcibios_resource_survey_bus() as pci_scan_bridge/pci_scan_child_bus will set dev->subordinate->is_added... Please note: recently is_added setting is moved early before pci_bus_add_devices... > __pci_bus_size_bridges(dev->subordinate, > &add_list); > } > @@ -742,8 +740,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) > } > } > > - > - err_exit: > return 0; > } > > -- > 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
On Wed, Jun 26, 2013 at 05:37:58PM -0600, 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> > > > > pci_scan_slot() returns number of new devices connected *directly* > > connected to the slot. Current enable_device() checks the return value > > and stops if it doesn't see a new device. > > > > In Thunderbolt chaining case the new device can be deeper in hierarchy, so > > do the rescan anyway. > > > > Because of that we must make sure that pcibios_resource_survey_bus() and > > check_hotplug_bridge() get called only for a just found bus and not the > > ones already added to the system. Failure to do so will lead to resource > > conflicts. > > > > 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 | 18 +++++++----------- > > 1 file changed, 7 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > > index b983e29..80a6ea1 100644 > > --- a/drivers/pci/hotplug/acpiphp_glue.c > > +++ b/drivers/pci/hotplug/acpiphp_glue.c > > @@ -685,18 +685,13 @@ static int __ref enable_device(struct acpiphp_slot *slot) > > struct pci_dev *dev; > > struct pci_bus *bus = slot->bridge->pci_bus; > > struct acpiphp_func *func; > > - int num, max, pass; > > + int max, pass; > > LIST_HEAD(add_list); > > > > list_for_each_entry(func, &slot->funcs, sibling) > > acpiphp_bus_add(func); > > > > - num = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); > > - if (num == 0) { > > - /* Maybe only part of funcs are added. */ > > - dbg("No new device found\n"); > > - goto err_exit; > > - } > > + pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); > > > > max = acpiphp_max_busnr(bus); > > for (pass = 0; pass < 2; pass++) { > > I think this is two logical changes: the change below looks like it > could by done by itself first, followed by the change above. Indeed. We'll going to drop the second change completely. > > > @@ -707,8 +702,11 @@ static int __ref enable_device(struct acpiphp_slot *slot) > > dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) { > > max = pci_scan_bridge(bus, dev, max, pass); > > if (pass && dev->subordinate) { > > - check_hotplug_bridge(slot, dev); > > - pcibios_resource_survey_bus(dev->subordinate); > > + if (!dev->subordinate->is_added) { > > + check_hotplug_bridge(slot, dev); > > + pcibios_resource_survey_bus( > > + dev->subordinate); > > It's a shame that pcibios_resource_survey_bus() can't be called twice. > It would be nice if it were smart enough to notice on the second call > that "oh, resources have already been assigned, so there's nothing > more to be done." Did you look to see whether that's feasible? I didn't but now that you mentioned I went and checked. I'm pretty sure we can handle it there in the next revision and drop this change. -- 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 Wed, Jun 26, 2013 at 06:20:24PM -0700, 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> > > > > pci_scan_slot() returns number of new devices connected *directly* > > connected to the slot. Current enable_device() checks the return value > > and stops if it doesn't see a new device. > > > > In Thunderbolt chaining case the new device can be deeper in hierarchy, so > > do the rescan anyway. > > > > Because of that we must make sure that pcibios_resource_survey_bus() and > > check_hotplug_bridge() get called only for a just found bus and not the > > ones already added to the system. Failure to do so will lead to resource > > conflicts. > > > > 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 | 18 +++++++----------- > > 1 file changed, 7 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > > index b983e29..80a6ea1 100644 > > --- a/drivers/pci/hotplug/acpiphp_glue.c > > +++ b/drivers/pci/hotplug/acpiphp_glue.c > > @@ -685,18 +685,13 @@ static int __ref enable_device(struct acpiphp_slot *slot) > > struct pci_dev *dev; > > struct pci_bus *bus = slot->bridge->pci_bus; > > struct acpiphp_func *func; > > - int num, max, pass; > > + int max, pass; > > LIST_HEAD(add_list); > > > > list_for_each_entry(func, &slot->funcs, sibling) > > acpiphp_bus_add(func); > > > > - num = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); > > - if (num == 0) { > > - /* Maybe only part of funcs are added. */ > > - dbg("No new device found\n"); > > - goto err_exit; > > - } > > + pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); > > > > max = acpiphp_max_busnr(bus); > > for (pass = 0; pass < 2; pass++) { > > @@ -707,8 +702,11 @@ static int __ref enable_device(struct acpiphp_slot *slot) > > dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) { > > max = pci_scan_bridge(bus, dev, max, pass); > > if (pass && dev->subordinate) { > > - check_hotplug_bridge(slot, dev); > > - pcibios_resource_survey_bus(dev->subordinate); > > + if (!dev->subordinate->is_added) { > > + check_hotplug_bridge(slot, dev); > > + pcibios_resource_survey_bus( > > + dev->subordinate); > > + } > > No, > this change will totally disable calling > check_hotplug_bridge/pcibios_resource_survey_bus() > > as pci_scan_bridge/pci_scan_child_bus will set dev->subordinate->is_added... > > Please note: recently is_added setting is moved early before > pci_bus_add_devices... OK, thanks. We will handle this in pcibios_resource_survey_bus() instead as suggested by 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
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index b983e29..80a6ea1 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -685,18 +685,13 @@ static int __ref enable_device(struct acpiphp_slot *slot) struct pci_dev *dev; struct pci_bus *bus = slot->bridge->pci_bus; struct acpiphp_func *func; - int num, max, pass; + int max, pass; LIST_HEAD(add_list); list_for_each_entry(func, &slot->funcs, sibling) acpiphp_bus_add(func); - num = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); - if (num == 0) { - /* Maybe only part of funcs are added. */ - dbg("No new device found\n"); - goto err_exit; - } + pci_scan_slot(bus, PCI_DEVFN(slot->device, 0)); max = acpiphp_max_busnr(bus); for (pass = 0; pass < 2; pass++) { @@ -707,8 +702,11 @@ static int __ref enable_device(struct acpiphp_slot *slot) dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) { max = pci_scan_bridge(bus, dev, max, pass); if (pass && dev->subordinate) { - check_hotplug_bridge(slot, dev); - pcibios_resource_survey_bus(dev->subordinate); + if (!dev->subordinate->is_added) { + check_hotplug_bridge(slot, dev); + pcibios_resource_survey_bus( + dev->subordinate); + } __pci_bus_size_bridges(dev->subordinate, &add_list); } @@ -742,8 +740,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) } } - - err_exit: return 0; }