diff mbox

ACPI / hotplug / PCI: Avoid parent bus rescans on spurious device checks

Message ID 2762291.l6lVgcWD3r@vostro.rjw.lan (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Rafael Wysocki Sept. 7, 2013, 10:16 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In the current ACPIPHP notify handler we always go directly for a
rescan of the parent bus if we get a device check notification for
a device that is not a bridge.  However, this obviously is
overzealous if nothing really changes, because this way we may rescan
the whole PCI hierarchy pretty much in vain.

That happens on Alex Williamson's machine whose ACPI tables contain
device objects that are supposed to coresspond to PCIe root ports,
but those ports aren't physically present (or at least they aren't
visible in the PCI config space to us).  The BIOS generates multiple
device check notifies for those objects during boot and for each of
them we go straight for the parent bus rescan, but the parent bus is
the root bus in this particular case.  In consequence, we rescan the
whole PCI bus from the top several times in a row, which is
completely unnecessary, increases boot time by 50% (after previous
fixes) and generates excess dmesg output from the PCI subsystem.

Fix the problem by checking if we can find anything new in the
slot corresponding to the device we've got a device check notify
for and doing nothig if that's not the case.

The spec (ACPI 5.0, Section 5.6.6) appears to mandate this behavior,
as it says:

  Device Check. Used to notify OSPM that the device either appeared
  or disappeared. If the device has appeared, OSPM will re-enumerate
  from the parent. If the device has disappeared, OSPM will
  invalidate the state of the device. OSPM may optimize out
  re-enumeration.

Therefore, according to the spec, we are free to do nothing if
nothing changes.

References: https://bugzilla.kernel.org/show_bug.cgi?id=60865
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

On top of linux-pm.git/linux-next.

Thanks,
Rafael

---
 drivers/pci/hotplug/acpiphp_glue.c |   32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 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

Comments

Alex Williamson Sept. 9, 2013, 4:32 p.m. UTC | #1 
On Sun, 2013-09-08 at 00:16 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In the current ACPIPHP notify handler we always go directly for a
> rescan of the parent bus if we get a device check notification for
> a device that is not a bridge.  However, this obviously is
> overzealous if nothing really changes, because this way we may rescan
> the whole PCI hierarchy pretty much in vain.
> 
> That happens on Alex Williamson's machine whose ACPI tables contain
> device objects that are supposed to coresspond to PCIe root ports,
> but those ports aren't physically present (or at least they aren't
> visible in the PCI config space to us).  The BIOS generates multiple
> device check notifies for those objects during boot and for each of
> them we go straight for the parent bus rescan, but the parent bus is
> the root bus in this particular case.  In consequence, we rescan the
> whole PCI bus from the top several times in a row, which is
> completely unnecessary, increases boot time by 50% (after previous
> fixes) and generates excess dmesg output from the PCI subsystem.
> 
> Fix the problem by checking if we can find anything new in the
> slot corresponding to the device we've got a device check notify
> for and doing nothig if that's not the case.
> 
> The spec (ACPI 5.0, Section 5.6.6) appears to mandate this behavior,
> as it says:
> 
>   Device Check. Used to notify OSPM that the device either appeared
>   or disappeared. If the device has appeared, OSPM will re-enumerate
>   from the parent. If the device has disappeared, OSPM will
>   invalidate the state of the device. OSPM may optimize out
>   re-enumeration.
> 
> Therefore, according to the spec, we are free to do nothing if
> nothing changes.
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=60865
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Works for me.  Thanks!

Tested-by: Alex Williamson <alex.williamson@redhat.com>

> On top of linux-pm.git/linux-next.
> 
> Thanks,
> Rafael
> 
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 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
> @@ -528,6 +528,16 @@ static void check_hotplug_bridge(struct
>  	}
>  }
>  
> +static int acpiphp_rescan_slot(struct acpiphp_slot *slot)
> +{
> +	struct acpiphp_func *func;
> +
> +	list_for_each_entry(func, &slot->funcs, sibling)
> +		acpiphp_bus_add(func_to_handle(func));
> +
> +	return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
> +}
> +
>  /**
>   * enable_slot - enable, configure a slot
>   * @slot: slot to be enabled
> @@ -544,10 +554,7 @@ static void __ref enable_slot(struct acp
>  	LIST_HEAD(add_list);
>  	int nr_found;
>  
> -	list_for_each_entry(func, &slot->funcs, sibling)
> -		acpiphp_bus_add(func_to_handle(func));
> -
> -	nr_found = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
> +	nr_found = acpiphp_rescan_slot(slot);
>  	max = acpiphp_max_busnr(bus);
>  	for (pass = 0; pass < 2; pass++) {
>  		list_for_each_entry(dev, &bus->devices, bus_list) {
> @@ -840,11 +847,22 @@ static void hotplug_event(acpi_handle ha
>  	case ACPI_NOTIFY_DEVICE_CHECK:
>  		/* device check */
>  		dbg("%s: Device check notify on %s\n", __func__, objname);
> -		if (bridge)
> +		if (bridge) {
>  			acpiphp_check_bridge(bridge);
> -		else
> -			acpiphp_check_bridge(func->parent);
> +		} else {
> +			struct acpiphp_slot *slot = func->slot;
> +			int ret;
>  
> +			/*
> +			 * Check if anything has changed in the slot and rescan
> +			 * from the parent if that's the case.
> +			 */
> +			mutex_lock(&slot->crit_sect);
> +			ret = acpiphp_rescan_slot(slot);
> +			mutex_unlock(&slot->crit_sect);
> +			if (ret)
> +				acpiphp_check_bridge(func->parent);
> +		}
>  		break;
>  
>  	case ACPI_NOTIFY_EJECT_REQUEST:


--
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 Wysocki Sept. 9, 2013, 8:02 p.m. UTC | #2 
On Monday, September 09, 2013 10:32:57 AM Alex Williamson wrote:
> On Sun, 2013-09-08 at 00:16 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > In the current ACPIPHP notify handler we always go directly for a
> > rescan of the parent bus if we get a device check notification for
> > a device that is not a bridge.  However, this obviously is
> > overzealous if nothing really changes, because this way we may rescan
> > the whole PCI hierarchy pretty much in vain.
> > 
> > That happens on Alex Williamson's machine whose ACPI tables contain
> > device objects that are supposed to coresspond to PCIe root ports,
> > but those ports aren't physically present (or at least they aren't
> > visible in the PCI config space to us).  The BIOS generates multiple
> > device check notifies for those objects during boot and for each of
> > them we go straight for the parent bus rescan, but the parent bus is
> > the root bus in this particular case.  In consequence, we rescan the
> > whole PCI bus from the top several times in a row, which is
> > completely unnecessary, increases boot time by 50% (after previous
> > fixes) and generates excess dmesg output from the PCI subsystem.
> > 
> > Fix the problem by checking if we can find anything new in the
> > slot corresponding to the device we've got a device check notify
> > for and doing nothig if that's not the case.
> > 
> > The spec (ACPI 5.0, Section 5.6.6) appears to mandate this behavior,
> > as it says:
> > 
> >   Device Check. Used to notify OSPM that the device either appeared
> >   or disappeared. If the device has appeared, OSPM will re-enumerate
> >   from the parent. If the device has disappeared, OSPM will
> >   invalidate the state of the device. OSPM may optimize out
> >   re-enumeration.
> > 
> > Therefore, according to the spec, we are free to do nothing if
> > nothing changes.
> > 
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=60865
> > Reported-by: Alex Williamson <alex.williamson@redhat.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> 
> Works for me.  Thanks!
> 
> Tested-by: Alex Williamson <alex.williamson@redhat.com>

Great, thanks for testing.


> > ---
> >  drivers/pci/hotplug/acpiphp_glue.c |   32 +++++++++++++++++++++++++-------
> >  1 file changed, 25 insertions(+), 7 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
> > @@ -528,6 +528,16 @@ static void check_hotplug_bridge(struct
> >  	}
> >  }
> >  
> > +static int acpiphp_rescan_slot(struct acpiphp_slot *slot)
> > +{
> > +	struct acpiphp_func *func;
> > +
> > +	list_for_each_entry(func, &slot->funcs, sibling)
> > +		acpiphp_bus_add(func_to_handle(func));
> > +
> > +	return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
> > +}
> > +
> >  /**
> >   * enable_slot - enable, configure a slot
> >   * @slot: slot to be enabled
> > @@ -544,10 +554,7 @@ static void __ref enable_slot(struct acp
> >  	LIST_HEAD(add_list);
> >  	int nr_found;
> >  
> > -	list_for_each_entry(func, &slot->funcs, sibling)
> > -		acpiphp_bus_add(func_to_handle(func));
> > -
> > -	nr_found = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
> > +	nr_found = acpiphp_rescan_slot(slot);
> >  	max = acpiphp_max_busnr(bus);
> >  	for (pass = 0; pass < 2; pass++) {
> >  		list_for_each_entry(dev, &bus->devices, bus_list) {
> > @@ -840,11 +847,22 @@ static void hotplug_event(acpi_handle ha
> >  	case ACPI_NOTIFY_DEVICE_CHECK:
> >  		/* device check */
> >  		dbg("%s: Device check notify on %s\n", __func__, objname);
> > -		if (bridge)
> > +		if (bridge) {
> >  			acpiphp_check_bridge(bridge);
> > -		else
> > -			acpiphp_check_bridge(func->parent);
> > +		} else {
> > +			struct acpiphp_slot *slot = func->slot;
> > +			int ret;
> >  
> > +			/*
> > +			 * Check if anything has changed in the slot and rescan
> > +			 * from the parent if that's the case.
> > +			 */
> > +			mutex_lock(&slot->crit_sect);
> > +			ret = acpiphp_rescan_slot(slot);
> > +			mutex_unlock(&slot->crit_sect);
> > +			if (ret)
> > +				acpiphp_check_bridge(func->parent);
> > +		}
> >  		break;
> >  
> >  	case ACPI_NOTIFY_EJECT_REQUEST:
> 
>
diff mbox

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
@@ -528,6 +528,16 @@  static void check_hotplug_bridge(struct
 	}
 }
 
+static int acpiphp_rescan_slot(struct acpiphp_slot *slot)
+{
+	struct acpiphp_func *func;
+
+	list_for_each_entry(func, &slot->funcs, sibling)
+		acpiphp_bus_add(func_to_handle(func));
+
+	return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
+}
+
 /**
  * enable_slot - enable, configure a slot
  * @slot: slot to be enabled
@@ -544,10 +554,7 @@  static void __ref enable_slot(struct acp
 	LIST_HEAD(add_list);
 	int nr_found;
 
-	list_for_each_entry(func, &slot->funcs, sibling)
-		acpiphp_bus_add(func_to_handle(func));
-
-	nr_found = pci_scan_slot(bus, PCI_DEVFN(slot->device, 0));
+	nr_found = acpiphp_rescan_slot(slot);
 	max = acpiphp_max_busnr(bus);
 	for (pass = 0; pass < 2; pass++) {
 		list_for_each_entry(dev, &bus->devices, bus_list) {
@@ -840,11 +847,22 @@  static void hotplug_event(acpi_handle ha
 	case ACPI_NOTIFY_DEVICE_CHECK:
 		/* device check */
 		dbg("%s: Device check notify on %s\n", __func__, objname);
-		if (bridge)
+		if (bridge) {
 			acpiphp_check_bridge(bridge);
-		else
-			acpiphp_check_bridge(func->parent);
+		} else {
+			struct acpiphp_slot *slot = func->slot;
+			int ret;
 
+			/*
+			 * Check if anything has changed in the slot and rescan
+			 * from the parent if that's the case.
+			 */
+			mutex_lock(&slot->crit_sect);
+			ret = acpiphp_rescan_slot(slot);
+			mutex_unlock(&slot->crit_sect);
+			if (ret)
+				acpiphp_check_bridge(func->parent);
+		}
 		break;
 
 	case ACPI_NOTIFY_EJECT_REQUEST: