Patchwork [1/2] ACPI / hotplug / PCI: Avoid doing too much for spurious notifies

login
register
mail settings
Submitter Rafael J. Wysocki
Date Sept. 6, 2013, 1:43 p.m.
Message ID <1831138.AvDtjG8RWn@vostro.rjw.lan>
Download mbox | patch
Permalink /patch/273200/
State Not Applicable
Headers show

Comments

Rafael J. Wysocki - Sept. 6, 2013, 1:43 p.m.
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Sometimes we may get a spurious device check or bus check notify for
a hotplug device and in those cases we should avoid doing all of the
configuration work needed when something actually changes.  To that
end, check the return value of pci_scan_slot() in enable_slot() and
bail out early if it is 0.

This turns out to help reduce the amount of diagnostic output from
the ACPIPHP subsystem and speed up boot on at least one system that
generates multiple device check notifies for PCIe ports during
boot.

Reported-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)


--
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 - Sept. 6, 2013, 3:46 p.m.
On Fri, Sep 6, 2013 at 6:43 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Sometimes we may get a spurious device check or bus check notify for
> a hotplug device and in those cases we should avoid doing all of the
> configuration work needed when something actually changes.  To that
> end, check the return value of pci_scan_slot() in enable_slot() and
> bail out early if it is 0.
>
> This turns out to help reduce the amount of diagnostic output from
> the ACPIPHP subsystem and speed up boot on at least one system that
> generates multiple device check notifies for PCIe ports during
> boot.
>
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   35 +++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
>
> Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
> +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> @@ -542,12 +542,12 @@ static void __ref enable_slot(struct acp
>         struct acpiphp_func *func;
>         int max, pass;
>         LIST_HEAD(add_list);
> +       int nr_found;
>
>         list_for_each_entry(func, &slot->funcs, sibling)
>                 acpiphp_bus_add(func_to_handle(func));
>
> -       pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
> -
> +       nr_found = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
>         max = acpiphp_max_busnr(bus);
>         for (pass = 0; pass < 2; pass++) {
>                 list_for_each_entry(dev, &bus->devices, bus_list) {
> @@ -566,8 +566,11 @@ static void __ref enable_slot(struct acp
>                         }
>                 }
>         }
> -
>         __pci_bus_assign_resources(bus, &add_list, NULL);
> +       /* Nothing more to do here if there are no new devices on this bus. */
> +       if (!nr_found && (slot->flags & SLOT_ENABLED))
> +               return;
> +
>         acpiphp_sanitize_bus(bus);
>         acpiphp_set_hpp_values(bus);
>         acpiphp_set_acpi_region(slot);
>

why not just returning early before size bridges and assign unassign resources?

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 - Sept. 6, 2013, 11:45 p.m.
On Friday, September 06, 2013 08:46:28 AM Yinghai Lu wrote:
> On Fri, Sep 6, 2013 at 6:43 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Sometimes we may get a spurious device check or bus check notify for
> > a hotplug device and in those cases we should avoid doing all of the
> > configuration work needed when something actually changes.  To that
> > end, check the return value of pci_scan_slot() in enable_slot() and
> > bail out early if it is 0.
> >
> > This turns out to help reduce the amount of diagnostic output from
> > the ACPIPHP subsystem and speed up boot on at least one system that
> > generates multiple device check notifies for PCIe ports during
> > boot.
> >
> > Reported-by: Alex Williamson <alex.williamson@redhat.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/pci/hotplug/acpiphp_glue.c |   35 +++++++++++++++++++++++++++--------
> >  1 file changed, 27 insertions(+), 8 deletions(-)
> >
> > Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
> > +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -542,12 +542,12 @@ static void __ref enable_slot(struct acp
> >         struct acpiphp_func *func;
> >         int max, pass;
> >         LIST_HEAD(add_list);
> > +       int nr_found;
> >
> >         list_for_each_entry(func, &slot->funcs, sibling)
> >                 acpiphp_bus_add(func_to_handle(func));
> >
> > -       pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
> > -
> > +       nr_found = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
> >         max = acpiphp_max_busnr(bus);
> >         for (pass = 0; pass < 2; pass++) {
> >                 list_for_each_entry(dev, &bus->devices, bus_list) {
> > @@ -566,8 +566,11 @@ static void __ref enable_slot(struct acp
> >                         }
> >                 }
> >         }
> > -
> >         __pci_bus_assign_resources(bus, &add_list, NULL);
> > +       /* Nothing more to do here if there are no new devices on this bus. */
> > +       if (!nr_found && (slot->flags & SLOT_ENABLED))
> > +               return;
> > +
> >         acpiphp_sanitize_bus(bus);
> >         acpiphp_set_hpp_values(bus);
> >         acpiphp_set_acpi_region(slot);
> >
> 
> why not just returning early before size bridges and assign unassign resources?

Because we still need to do that even if pci_scan_slot() returns 0.

There may be new devices down in the hierarchy and we need to look for them.

Thanks,
Rafael

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

Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -542,12 +542,12 @@  static void __ref enable_slot(struct acp
 	struct acpiphp_func *func;
 	int max, pass;
 	LIST_HEAD(add_list);
+	int nr_found;
 
 	list_for_each_entry(func, &slot->funcs, sibling)
 		acpiphp_bus_add(func_to_handle(func));
 
-	pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
-
+	nr_found = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
 	max = acpiphp_max_busnr(bus);
 	for (pass = 0; pass < 2; pass++) {
 		list_for_each_entry(dev, &bus->devices, bus_list) {
@@ -566,8 +566,11 @@  static void __ref enable_slot(struct acp
 			}
 		}
 	}
-
 	__pci_bus_assign_resources(bus, &add_list, NULL);
+	/* Nothing more to do here if there are no new devices on this bus. */
+	if (!nr_found && (slot->flags & SLOT_ENABLED))
+		return;
+
 	acpiphp_sanitize_bus(bus);
 	acpiphp_set_hpp_values(bus);
 	acpiphp_set_acpi_region(slot);