Patchwork [2/6] PCI: acpiphp: enable_device(): rescan even if no new devices on slot

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

Comments

Mika Westerberg - June 25, 2013, 4:22 p.m.
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(-)
Bjorn Helgaas - June 26, 2013, 11:37 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>
>
> 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
Yinghai Lu - June 27, 2013, 1:20 a.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>
>
> 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
Mika Westerberg - June 27, 2013, 1:02 p.m.
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
Mika Westerberg - June 27, 2013, 1:04 p.m.
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

Patch

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;
 }