[PATCH v1 0/3] ACPI: PCI: PM: Power up PCI devices with ACPI companions upfront

[PATCH v1 0/3] ACPI: PCI: PM: Power up PCI devices with ACPI companions upfront

[Rafael J. Wysocki]

Hi All,

There are cases in which the power state of a PCI device depends on an ACPI
power resource (or more of them) in such a way that when the given power
resource is in the "off" state, the PCI device depending on it is in D3cold.

On some systems, the initial state of these power resources is "off", so the
kernel should not access the config space of PCI devices depending on them,
until the power resources in question are turned "on", but currently that is
not respected during PCI device enumeration.  Namely, the PCI device
enumeration code walks the entire bus and enumerates all of the devices it
can find, including the ones whose initial power state in principle depends on
the ACPI power resources in the "off" state.

Apparently, most of the time, the config space of such devices is accessible
regardless of the state of the ACPI power resource associated with the PCI
device, so the device enumeration is successful, but there are two potential
issues related to this behavior.  First off, even if the given PCI device
is accessible when the ACPI power resource depended on by it is "off",
changing its configuration may confuse the platform firmware and lead to
problems when the ACPI power resource in question is turned "on".  Second,
the PCI device may not be actually accessible at all when the ACPI power
resource depended on by it is "off", in which case it won't be found during
the PCI enumeration of devices.

This patch series addresses that problem by turning "on" all ACPI power
resources depended on by PCI devices before attempting to access the config
space of those devices for the first time.

The first two patches introduce the requisite machinery and the actual change
of behavior is done in the last patch.

Thanks!

[PATCH v1 1/3] ACPI: bus: Introduce acpi_dev_for_each_child()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a wrapper around device_for_each_child() to iterate over
the children of a given ACPI device object.

This function will be used in subsequent change sets.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c      |    6 ++++++
 include/acpi/acpi_bus.h |    2 ++
 2 files changed, 8 insertions(+)

Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -1070,6 +1070,12 @@ int acpi_bus_for_each_dev(int (*fn)(stru
 }
 EXPORT_SYMBOL_GPL(acpi_bus_for_each_dev);
 
+int acpi_dev_for_each_child(struct acpi_device *adev,
+			    int (*fn)(struct device *, void *), void *data)
+{
+	return device_for_each_child(&adev->dev, data, fn);
+}
+
 /* --------------------------------------------------------------------------
                              Initialization/Cleanup
    -------------------------------------------------------------------------- */
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -481,6 +481,8 @@ void acpi_initialize_hp_context(struct a
 extern struct bus_type acpi_bus_type;
 
 int acpi_bus_for_each_dev(int (*fn)(struct device *, void *), void *data);
+int acpi_dev_for_each_child(struct acpi_device *adev,
+			    int (*fn)(struct device *, void *), void *data);
 
 /*
  * Events

[PATCH v1 2/3] ACPI: PM: Introduce acpi_dev_power_up_children_with_adr()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a function powering up all of the children of a given ACPI
device object that are power-manageable and hold valid _ADR ACPI
objects so as to make it possible to prepare the corresponding
"physical" devices for enumeration carried out by a bus type driver,
like PCI.

This function will be used in a subsequent change set.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   30 ++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h  |    1 +
 2 files changed, 31 insertions(+)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -427,6 +427,36 @@ bool acpi_bus_power_manageable(acpi_hand
 }
 EXPORT_SYMBOL(acpi_bus_power_manageable);
 
+static int acpi_power_up_if_adr_present(struct device *dev, void *not_used)
+{
+	struct acpi_device *adev;
+
+	adev = to_acpi_device(dev);
+	if (!(adev->flags.power_manageable && adev->pnp.type.bus_address))
+		return 0;
+
+	acpi_handle_debug(adev->handle, "Power state: %s\n",
+			  acpi_power_state_string(adev->power.state));
+
+	if (adev->power.state == ACPI_STATE_D3_COLD)
+		return acpi_device_set_power(adev, ACPI_STATE_D0);
+
+	return 0;
+}
+
+/**
+ * acpi_dev_power_up_children_with_adr - Power up childres with valid _ADR
+ * @adev: Parent ACPI device object.
+ *
+ * Change the power states of the direct children of @adev that are in D3cold
+ * and hold valid _ADR objects to D0 in order to allow bus (e.g. PCI)
+ * enumeration code to access them.
+ */
+void acpi_dev_power_up_children_with_adr(struct acpi_device *adev)
+{
+	acpi_dev_for_each_child(adev, acpi_power_up_if_adr_present, NULL);
+}
+
 #ifdef CONFIG_PM
 static DEFINE_MUTEX(acpi_pm_notifier_lock);
 static DEFINE_MUTEX(acpi_pm_notifier_install_lock);
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -525,6 +525,7 @@ int acpi_device_fix_up_power(struct acpi
 int acpi_bus_update_power(acpi_handle handle, int *state_p);
 int acpi_device_update_power(struct acpi_device *device, int *state_p);
 bool acpi_bus_power_manageable(acpi_handle handle);
+void acpi_dev_power_up_children_with_adr(struct acpi_device *adev);
 int acpi_device_power_add_dependent(struct acpi_device *adev,
 				    struct device *dev);
 void acpi_device_power_remove_dependent(struct acpi_device *adev,

[PATCH v1 3/3] PCI: ACPI: PM: Power up devices in D3cold before scanning them

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The initial configuration of ACPI power resources on some systems
implies that some PCI devices on them are initially in D3cold.

In some cases, especially for PCIe Root Ports, this is a "logical"
D3cold, meaning that the configuration space of the device is
accessible, but some of its functionality may be missing, but it
very well may be real D3cold, in which case the device will not
be accessible at all.  However, the PCI bus type driver will need
to access its configuration space in order to enumerate it.

To prevent possible device enumeration failures that may ensue as
a result of ACPI power resources being initially in the "off"
state, power up all children of the host bridge ACPI device object
that hold valid _ADR objects (which indicates that they will be
enumerated by the PCI bus type driver) and do that to all children
of the ACPI device objects corresponding to PCI bridges (including
PCIe ports).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/pci_root.c |    2 ++
 drivers/pci/pci-acpi.c  |    3 +++
 2 files changed, 5 insertions(+)

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
--- linux-pm.orig/drivers/acpi/pci_root.c
+++ linux-pm/drivers/acpi/pci_root.c
@@ -927,6 +927,8 @@ struct pci_bus *acpi_pci_root_create(str
 		host_bridge->preserve_config = 1;
 	ACPI_FREE(obj);
 
+	acpi_dev_power_up_children_with_adr(device);
+
 	pci_scan_child_bus(bus);
 	pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
 				    info);
Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -1374,6 +1374,9 @@ void pci_acpi_setup(struct device *dev,
 
 	acpi_pci_wakeup(pci_dev, false);
 	acpi_device_power_add_dependent(adev, dev);
+
+	if (pci_is_bridge(pci_dev))
+		acpi_dev_power_up_children_with_adr(adev);
 }
 
 void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)

Re: [PATCH v1 0/3] ACPI: PCI: PM: Power up PCI devices with ACPI companions upfront

[Mika Westerberg]

Hi Rafael,

On Mon, Apr 04, 2022 at 05:20:30PM +0200, Rafael J. Wysocki wrote:
> Hi All,
> 
> There are cases in which the power state of a PCI device depends on an ACPI
> power resource (or more of them) in such a way that when the given power
> resource is in the "off" state, the PCI device depending on it is in D3cold.
> 
> On some systems, the initial state of these power resources is "off", so the
> kernel should not access the config space of PCI devices depending on them,
> until the power resources in question are turned "on", but currently that is
> not respected during PCI device enumeration.  Namely, the PCI device
> enumeration code walks the entire bus and enumerates all of the devices it
> can find, including the ones whose initial power state in principle depends on
> the ACPI power resources in the "off" state.

I guess these devices do not have _PRE() method either.

> Apparently, most of the time, the config space of such devices is accessible
> regardless of the state of the ACPI power resource associated with the PCI
> device, so the device enumeration is successful, but there are two potential
> issues related to this behavior.  First off, even if the given PCI device
> is accessible when the ACPI power resource depended on by it is "off",
> changing its configuration may confuse the platform firmware and lead to
> problems when the ACPI power resource in question is turned "on".  Second,
> the PCI device may not be actually accessible at all when the ACPI power
> resource depended on by it is "off", in which case it won't be found during
> the PCI enumeration of devices.
> 
> This patch series addresses that problem by turning "on" all ACPI power
> resources depended on by PCI devices before attempting to access the config
> space of those devices for the first time.

Makes sense.

For the series,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 0/3] ACPI: PCI: PM: Power up PCI devices with ACPI companions upfront

[Rafael J. Wysocki]

On Tue, Apr 5, 2022 at 1:45 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, Apr 04, 2022 at 05:20:30PM +0200, Rafael J. Wysocki wrote:
> > Hi All,
> >
> > There are cases in which the power state of a PCI device depends on an ACPI
> > power resource (or more of them) in such a way that when the given power
> > resource is in the "off" state, the PCI device depending on it is in D3cold.
> >
> > On some systems, the initial state of these power resources is "off", so the
> > kernel should not access the config space of PCI devices depending on them,
> > until the power resources in question are turned "on", but currently that is
> > not respected during PCI device enumeration.  Namely, the PCI device
> > enumeration code walks the entire bus and enumerates all of the devices it
> > can find, including the ones whose initial power state in principle depends on
> > the ACPI power resources in the "off" state.
>
> I guess these devices do not have _PRE() method either.

Personally, I haven't seen any ACPI tables containing any _PRE yet.

> > Apparently, most of the time, the config space of such devices is accessible
> > regardless of the state of the ACPI power resource associated with the PCI
> > device, so the device enumeration is successful, but there are two potential
> > issues related to this behavior.  First off, even if the given PCI device
> > is accessible when the ACPI power resource depended on by it is "off",
> > changing its configuration may confuse the platform firmware and lead to
> > problems when the ACPI power resource in question is turned "on".  Second,
> > the PCI device may not be actually accessible at all when the ACPI power
> > resource depended on by it is "off", in which case it won't be found during
> > the PCI enumeration of devices.
> >
> > This patch series addresses that problem by turning "on" all ACPI power
> > resources depended on by PCI devices before attempting to access the config
> > space of those devices for the first time.
>
> Makes sense.
>
> For the series,
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks!

Re: [PATCH v1 3/3] PCI: ACPI: PM: Power up devices in D3cold before scanning them

[Bjorn Helgaas]

On Mon, Apr 04, 2022 at 05:25:04PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The initial configuration of ACPI power resources on some systems
> implies that some PCI devices on them are initially in D3cold.
> 
> In some cases, especially for PCIe Root Ports, this is a "logical"
> D3cold, meaning that the configuration space of the device is
> accessible, but some of its functionality may be missing, but it
> very well may be real D3cold, in which case the device will not
> be accessible at all.  However, the PCI bus type driver will need
> to access its configuration space in order to enumerate it.
> 
> To prevent possible device enumeration failures that may ensue as
> a result of ACPI power resources being initially in the "off"
> state, power up all children of the host bridge ACPI device object
> that hold valid _ADR objects (which indicates that they will be
> enumerated by the PCI bus type driver) and do that to all children
> of the ACPI device objects corresponding to PCI bridges (including
> PCIe ports).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/acpi/pci_root.c |    2 ++
>  drivers/pci/pci-acpi.c  |    3 +++
>  2 files changed, 5 insertions(+)
> 
> Index: linux-pm/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/pci_root.c
> +++ linux-pm/drivers/acpi/pci_root.c
> @@ -927,6 +927,8 @@ struct pci_bus *acpi_pci_root_create(str
>  		host_bridge->preserve_config = 1;
>  	ACPI_FREE(obj);
>  
> +	acpi_dev_power_up_children_with_adr(device);
> +
>  	pci_scan_child_bus(bus);
>  	pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
>  				    info);
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -1374,6 +1374,9 @@ void pci_acpi_setup(struct device *dev,
>  
>  	acpi_pci_wakeup(pci_dev, false);
>  	acpi_device_power_add_dependent(adev, dev);
> +
> +	if (pci_is_bridge(pci_dev))
> +		acpi_dev_power_up_children_with_adr(adev);
>  }
>  
>  void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)
> 
> 
>