Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Bjorn Helgaas <bhelgaas@google.com>, Len Brown <lenb@kernel.org>,
	Lukas Wunner <lukas@wunner.de>,
	Keith Busch <keith.busch@intel.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Alexandru Gagniuc <mr.nuke.me@gmail.com>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	Linux PCI <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] PCI / ACPI: Use cached ACPI device state to get PCI device power state
Date: Fri, 21 Jun 2019 13:56:49 +0200
Message-ID: <CAJZ5v0huw-n1m3mz2fEk9y2ejQuT4XYeP8_a-iR+epSq5Wu6yQ@mail.gmail.com> (raw)
In-Reply-To: <20190618161858.77834-2-mika.westerberg@linux.intel.com>

On Tue, Jun 18, 2019 at 6:19 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>

Actually, to start with, you can say that the ACPI power state
returned by acpi_device_get_power() may depend on the configuration of
ACPI power resources in the system which may change at any time after
acpi_device_get_power() has returned, unless the reference counters of
the ACPI power resources in question are set to prevent that from
happening.  Thus it is invalid to use acpi_device_get_power() in
acpi_pci_get_power_state() the way it is done now and the value of the
power.state field in the corresponding struct acpi_device object
(which reflects the ACPI power resources reference counting, among
other things) should be used instead.

Then you can describe the particular issue below as an example.

IMO that would explain the rationale better here.

> Intel Ice Lake has an integrated Thunderbolt controller which means that
> the PCIe topology is extended directly from the two root ports (RP0 and
> RP1). Power management is handled by ACPI power resources that are
> shared between the root ports, Thunderbolt controller (NHI) and xHCI
> controller.
>
> The topology with the power resources (marked with []) looks like:
>
>   Host bridge
>     |
>     +- RP0 ---\
>     +- RP1 ---|--+--> [TBT]
>     +- NHI --/   |
>     |            |
>     |            v
>     +- xHCI --> [D3C]
>
> Here TBT and D3C are the shared ACPI power resources. ACPI _PR3() method
> returns either TBT or D3C or both.
>
> Say we runtime suspend first the root ports RP0 and RP1, then NHI. Now
> since the TBT power resource is still on when the root ports are runtime
> suspended their dev->current_state is set to D3hot. When NHI is runtime
> suspended TBT is finally turned off but state of the root ports remain
> to be D3hot.
>
> If the user now runs lspci for instance, the result is all 1's like in
> the below output (07.0 is the first root port, RP0):
>
> 00:07.0 PCI bridge: Intel Corporation Device 8a1d (rev ff) (prog-if ff)
>     !!! Unknown header type 7f
>     Kernel driver in use: pcieport
>
> I short the hardware state is not in sync with the software state
> anymore. The exact same thing happens with the PME polling thread which
> ends up bringing the root ports back into D0 after they are runtime
> suspended.
>
> ACPI core already sets the device state to be D3cold when it drops its
> references to the power resources returned by _PR3 even if these power
> resources are still physically on (other devices still reference them).
> However, in PCI core we call acpi_device_get_power() to figure out the
> power state and that returns the "real" power state based on the state
> of its power resources.
>
> To make it work with the shared power resources modify
> acpi_pci_get_power_state() so that it reads the ACPI device power state
> that was cached by the ACPI core. This makes the PCI device power state
> match the ACPI device power state regardless of state of the shared
> power resources that may still be on at this point.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pci-acpi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 1897847ceb0c..b782acac26c5 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -685,7 +685,8 @@ static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
>         if (!adev || !acpi_device_power_manageable(adev))
>                 return PCI_UNKNOWN;
>
> -       if (acpi_device_get_power(adev, &state) || state == ACPI_STATE_UNKNOWN)
> +       state = adev->power.state;
> +       if (state == ACPI_STATE_UNKNOWN)
>                 return PCI_UNKNOWN;
>
>         return state_conv[state];
> --
> 2.20.1
>

  parent reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-18 16:18 [PATCH v2 0/3] PCI / ACPI: Handle sibling devices sharing power resources Mika Westerberg
2019-06-18 16:18 ` [PATCH v2 1/3] PCI / ACPI: Use cached ACPI device state to get PCI device power state Mika Westerberg
2019-06-19 21:28   ` Bjorn Helgaas
2019-06-20  8:27     ` Mika Westerberg
2019-06-20 13:16       ` Bjorn Helgaas
2019-06-20 13:37         ` Mika Westerberg
2019-06-20 14:15           ` Bjorn Helgaas
2019-06-21 10:32             ` Rafael J. Wysocki
2019-06-21 13:09               ` Bjorn Helgaas
2019-06-22  8:51                 ` Rafael J. Wysocki
2019-06-24 10:57                   ` Mika Westerberg
2019-06-24 11:14               ` Rafael J. Wysocki
2019-06-25  9:45                 ` Mika Westerberg
2019-06-25 10:00                   ` Rafael J. Wysocki
2019-06-25 10:08                     ` Mika Westerberg
2019-06-21 11:56   ` Rafael J. Wysocki [this message]
2019-06-24 10:58     ` Mika Westerberg
2019-06-18 16:18 ` [PATCH v2 2/3] ACPI / PM: Introduce concept of a _PR0 dependent device Mika Westerberg
2019-06-19 13:20   ` Rafael J. Wysocki
2019-06-19 13:34     ` Mika Westerberg
2019-06-18 16:18 ` [PATCH v2 3/3] PCI / ACPI: Add _PR0 dependent devices Mika Westerberg
2019-06-19 13:24 ` [PATCH v2 0/3] PCI / ACPI: Handle sibling devices sharing power resources Rafael J. Wysocki

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJZ5v0huw-n1m3mz2fEk9y2ejQuT4XYeP8_a-iR+epSq5Wu6yQ@mail.gmail.com \
    --to=rafael@kernel.org \
    --cc=alex.williamson@redhat.com \
    --cc=bhelgaas@google.com \
    --cc=keith.busch@intel.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lukas@wunner.de \
    --cc=mika.westerberg@linux.intel.com \
    --cc=mr.nuke.me@gmail.com \
    --cc=rjw@rjwysocki.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org linux-acpi@archiver.kernel.org
	public-inbox-index linux-acpi


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/ public-inbox