mbox series

[v3,0/3] PCI / ACPI: Handle sibling devices sharing power resources

Message ID 20190625102942.27740-1-mika.westerberg@linux.intel.com (mailing list archive)
Headers show
Series PCI / ACPI: Handle sibling devices sharing power resources | expand

Message

Mika Westerberg June 25, 2019, 10:29 a.m. UTC
Hi all,

This is third iteration of the patch series addressing issues around
sibling PCI devices sharing ACPI power resources.

As a concrete example in Intel Ice Lake the Thunderbolt controller, PCIe
root ports and xHCI all share the same ACPI power resources. When they are
all in D3hot power resources (returned by _PR3) can be turned off powering
off the whole block. However, there are two issues around this.

Firstly the PCI core sets the device power state by asking what the real
ACPI power state is. This results that all but last device sharing the
power resources are in D3hot when the power resources are turned off. This
causes issues if user runs for example 'lspci' because the device is really
in D3cold so what user gets back is all ones (0xffffffff).

Secondly if any of the device is runtime resumed the power resources are
turned on bringing all other devices sharing the resources to
D0uninitialized losing their wakeup configuration.

This series aims to fix the two issues by:

  1. Using the ACPI cached power state when PCI devices are transitioned
     into low power states instead of reading back the "real" power state.

  2. Introducing concept of "_PR0 dependent devices" that get runtime
     resumed whenever their power resource (which they might share with
     other sibling devices) gets turned on.

The series is based on the idea of Rafael J. Wysocki <rafael@kernel.org>.

Previous version of the series can be found here:

  v2: https://lore.kernel.org/linux-pci/20190618161858.77834-1-mika.westerberg@linux.intel.com/T/#m7a41d0b745400054543324ce84125040dbfed912
  v1: https://www.spinics.net/lists/linux-pci/msg83583.html

Changes from v2:

  * Updated changelog of patch [1/3] according to comments I got. I left
    the D3C power resource and xHCI there because it shows that we can have
    multiple shared power resources.

  * Added link to the discussion around v2.

  * Use adev->flags.power_manageable in patch [2/3].

Mika Westerberg (3):
  PCI / ACPI: Use cached ACPI device state to get PCI device power state
  ACPI / PM: Introduce concept of a _PR0 dependent device
  PCI / ACPI: Add _PR0 dependent devices

 drivers/acpi/power.c    | 135 ++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-acpi.c  |   5 +-
 include/acpi/acpi_bus.h |   4 ++
 3 files changed, 143 insertions(+), 1 deletion(-)

Comments

Rafael J. Wysocki June 25, 2019, 10:35 a.m. UTC | #1
On Tue, Jun 25, 2019 at 12:30 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi all,
>
> This is third iteration of the patch series addressing issues around
> sibling PCI devices sharing ACPI power resources.
>
> As a concrete example in Intel Ice Lake the Thunderbolt controller, PCIe
> root ports and xHCI all share the same ACPI power resources. When they are
> all in D3hot power resources (returned by _PR3) can be turned off powering
> off the whole block. However, there are two issues around this.
>
> Firstly the PCI core sets the device power state by asking what the real
> ACPI power state is. This results that all but last device sharing the
> power resources are in D3hot when the power resources are turned off. This
> causes issues if user runs for example 'lspci' because the device is really
> in D3cold so what user gets back is all ones (0xffffffff).
>
> Secondly if any of the device is runtime resumed the power resources are
> turned on bringing all other devices sharing the resources to
> D0uninitialized losing their wakeup configuration.
>
> This series aims to fix the two issues by:
>
>   1. Using the ACPI cached power state when PCI devices are transitioned
>      into low power states instead of reading back the "real" power state.
>
>   2. Introducing concept of "_PR0 dependent devices" that get runtime
>      resumed whenever their power resource (which they might share with
>      other sibling devices) gets turned on.
>
> The series is based on the idea of Rafael J. Wysocki <rafael@kernel.org>.
>
> Previous version of the series can be found here:
>
>   v2: https://lore.kernel.org/linux-pci/20190618161858.77834-1-mika.westerberg@linux.intel.com/T/#m7a41d0b745400054543324ce84125040dbfed912
>   v1: https://www.spinics.net/lists/linux-pci/msg83583.html
>
> Changes from v2:
>
>   * Updated changelog of patch [1/3] according to comments I got. I left
>     the D3C power resource and xHCI there because it shows that we can have
>     multiple shared power resources.
>
>   * Added link to the discussion around v2.
>
>   * Use adev->flags.power_manageable in patch [2/3].
>
> Mika Westerberg (3):
>   PCI / ACPI: Use cached ACPI device state to get PCI device power state
>   ACPI / PM: Introduce concept of a _PR0 dependent device
>   PCI / ACPI: Add _PR0 dependent devices
>
>  drivers/acpi/power.c    | 135 ++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci-acpi.c  |   5 +-
>  include/acpi/acpi_bus.h |   4 ++
>  3 files changed, 143 insertions(+), 1 deletion(-)
>

The whole series looks good to me, thank you!
Rafael J. Wysocki July 5, 2019, 9:51 a.m. UTC | #2
On Tuesday, June 25, 2019 12:35:12 PM CEST Rafael J. Wysocki wrote:
> On Tue, Jun 25, 2019 at 12:30 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Hi all,
> >
> > This is third iteration of the patch series addressing issues around
> > sibling PCI devices sharing ACPI power resources.
> >
> > As a concrete example in Intel Ice Lake the Thunderbolt controller, PCIe
> > root ports and xHCI all share the same ACPI power resources. When they are
> > all in D3hot power resources (returned by _PR3) can be turned off powering
> > off the whole block. However, there are two issues around this.
> >
> > Firstly the PCI core sets the device power state by asking what the real
> > ACPI power state is. This results that all but last device sharing the
> > power resources are in D3hot when the power resources are turned off. This
> > causes issues if user runs for example 'lspci' because the device is really
> > in D3cold so what user gets back is all ones (0xffffffff).
> >
> > Secondly if any of the device is runtime resumed the power resources are
> > turned on bringing all other devices sharing the resources to
> > D0uninitialized losing their wakeup configuration.
> >
> > This series aims to fix the two issues by:
> >
> >   1. Using the ACPI cached power state when PCI devices are transitioned
> >      into low power states instead of reading back the "real" power state.
> >
> >   2. Introducing concept of "_PR0 dependent devices" that get runtime
> >      resumed whenever their power resource (which they might share with
> >      other sibling devices) gets turned on.
> >
> > The series is based on the idea of Rafael J. Wysocki <rafael@kernel.org>.
> >
> > Previous version of the series can be found here:
> >
> >   v2: https://lore.kernel.org/linux-pci/20190618161858.77834-1-mika.westerberg@linux.intel.com/T/#m7a41d0b745400054543324ce84125040dbfed912
> >   v1: https://www.spinics.net/lists/linux-pci/msg83583.html
> >
> > Changes from v2:
> >
> >   * Updated changelog of patch [1/3] according to comments I got. I left
> >     the D3C power resource and xHCI there because it shows that we can have
> >     multiple shared power resources.
> >
> >   * Added link to the discussion around v2.
> >
> >   * Use adev->flags.power_manageable in patch [2/3].
> >
> > Mika Westerberg (3):
> >   PCI / ACPI: Use cached ACPI device state to get PCI device power state
> >   ACPI / PM: Introduce concept of a _PR0 dependent device
> >   PCI / ACPI: Add _PR0 dependent devices
> >
> >  drivers/acpi/power.c    | 135 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/pci-acpi.c  |   5 +-
> >  include/acpi/acpi_bus.h |   4 ++
> >  3 files changed, 143 insertions(+), 1 deletion(-)
> >
> 
> The whole series looks good to me, thank you!
> 

And so it has been applied and queued for 5.3, thanks!