Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Rafael J. Wysocki" <rjw@rjwysocki.net>
To:     Abhishek Sahu <abhsahu@nvidia.com>,
        Bjorn Helgaas <helgaas@kernel.org>
Cc:     Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, Linux PM <linux-pm@vger.kernel.org>,
        Mika Westerberg <mika.westerberg@linux.intel.com>
Subject: Re: [PATCH] PCI: Fix the ACPI power state during runtime resume
Date:   Mon, 07 Feb 2022 19:58:13 +0100
Message-ID: <2249802.ElGaqSPkdT@kreacher>
In-Reply-To: <20220204233219.GA228585@bhelgaas>
References: <20220204233219.GA228585@bhelgaas>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Saturday, February 5, 2022 12:32:19 AM CET Bjorn Helgaas wrote:
> [+cc Rafael, hoping for your review :)

+Mika

> Wonder if we should add something like this to MAINTAINERS so you get
> cc'd on power-related things:
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ea3e6c914384..3d9a211cad5d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15422,6 +15422,7 @@ F:	include/linux/pm.h
>  F:	include/linux/pm_*
>  F:	include/linux/powercap.h
>  F:	kernel/configs/nopm.config
> +K:	pci_[a-z_]*power[a-z_]*\(

It seems so, but generally PM patches should be CCed to linux-pm anyway.

>  
>  DYNAMIC THERMAL POWER MANAGEMENT (DTPM)
>  M:	Daniel Lezcano <daniel.lezcano@kernel.org>
> ]
> 
> On Mon, Jan 24, 2022 at 05:51:07PM +0530, Abhishek Sahu wrote:
> > Consider the following sequence during PCI device runtime
> > suspend/resume:
> > 
> > 1. PCI device goes into runtime suspended state. The PCI state
> >    will be changed to PCI_D0 and then pci_platform_power_transition()
> >    will be called which changes the ACPI state to ACPI_STATE_D3_HOT.

You mean PCI_D3hot I suppose?

> > 2. Parent bridge goes into runtime suspended state. If parent
> >    bridge supports D3cold, then it will change the power state of all its
> >    children to D3cold state and the power will be removed.
> > 
> > 3. During wake-up time, the bridge will be runtime resumed first
> >    and pci_power_up() will be called for the bridge. Now, the power
> >    supply will be resumed.
> > 
> > 4. pci_resume_bus() will be called which will internally invoke
> >    pci_restore_standard_config(). pci_update_current_state()
> >    will read PCI_PM_CTRL register and the current_state will be
> >    updated to D0.
> > 
> > In the above process, at step 4, the ACPI device state will still be
> > ACPI_STATE_D3_HOT since pci_platform_power_transition() is not being
> > invoked.

I'm not quite following.

I'm assuming that this description applies to the endpoint device that was
previously put into D3_hot.

Since its current state is D3_hot, it is not D0 (in particular) and the
pci_set_power_state() in pci_restore_standard_config() should put int into
D0 proper, including the platform firmware part.

> > We need call the pci_platform_power_transition() with state
> > D0 to change the ACPI state to ACPI_STATE_D0.
> > 
> > This patch calls pci_power_up() if current power state is D0 inside
> > pci_restore_standard_config(). This pci_power_up() will change the
> > ACPI state to ACPI_STATE_D0.
> > 
> > Following are the steps to confirm:
> > 
> > Enable the debug prints in acpi_pci_set_power_state()
> > 
> > 0000:01:00.0 is PCI device and 0000:00:01.0 is parent bridge device
> > 
> > Before:
> > 
> > 0000:01:00.0: power state changed by ACPI to D3hot
> > 0000:00:01.0: power state changed by ACPI to D3cold
> > 0000:00:01.0: power state changed by ACPI to D0
> > 
> > After:
> > 
> > 0000:01:00.0: power state changed by ACPI to D3hot
> > 0000:00:01.0: power state changed by ACPI to D3cold
> > 0000:00:01.0: power state changed by ACPI to D0
> > 0000:01:00.0: power state changed by ACPI to D0
> > 
> > So with this patch, the PCI device ACPI state is also being
> > changed to D0.
> > 
> > Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
> > ---
> >  drivers/pci/pci-driver.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 588588cfda48..64e0cca12f16 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -521,14 +521,22 @@ static void pci_device_shutdown(struct device *dev)
> >   */
> >  static int pci_restore_standard_config(struct pci_dev *pci_dev)
> >  {
> > +	int error = 0;
> >  	pci_update_current_state(pci_dev, PCI_UNKNOWN);
> >  
> >  	if (pci_dev->current_state != PCI_D0) {
> > -		int error = pci_set_power_state(pci_dev, PCI_D0);
> > -		if (error)
> > -			return error;
> > +		error = pci_set_power_state(pci_dev, PCI_D0);
> > +	} else {
> > +		/*
> > +		 * The platform power state can still be non-D0, so this is
> > +		 * required to change the platform power state to D0.
> > +		 */

This really isn't expected to happen.

If the device's power state has been changed to D3hot by ACPI, it is not in D0.

It looks like the state tracking is not working here.

> > +		error = pci_power_up(pci_dev);
> >  	}
> >  
> > +	if (error)
> > +		return error;
> > +
> >  	pci_restore_state(pci_dev);
> >  	pci_pme_restore(pci_dev);
> >  	return 0;
>