Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   "Rafael J. Wysocki" <rjw@rjwysocki.net>
To:     Abhishek Sahu <abhsahu@nvidia.com>
Cc:     Bjorn Helgaas <helgaas@kernel.org>,
        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:   Tue, 05 Apr 2022 18:50:48 +0200
Message-ID: <2632919.mvXUDI8C0e@kreacher>
In-Reply-To: <469a9db5-9868-cb17-dd5c-96ef29193fda@nvidia.com>
References: <20220204233219.GA228585@bhelgaas> <8f89131c-41a2-02e4-1427-8a5b1704e0ea@nvidia.com> <469a9db5-9868-cb17-dd5c-96ef29193fda@nvidia.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tuesday, April 5, 2022 6:36:34 PM CEST Abhishek Sahu wrote:
> On 2/8/2022 4:00 PM, Abhishek Sahu wrote:
> > On 2/8/2022 12:28 AM, Rafael J. Wysocki wrote:
> >> 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?
> >>
> > 
> >  Yes. It should be PCI_D3hot here. 
> > 
> >>>> 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.
> >>
> > 
> >  Yes. This is applicable for endpoint devices which was previously put
> >  into D3hot.
> > 
> >> 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.
> >>
> > 
> >  The pci_restore_standard_config() for endpoint devices are being called
> >  internally during wake-up of upstream bridge. 
> > 
> >  pci_power_up(struct pci_dev *dev)
> >  {
> >       ...
> >       if (dev->runtime_d3cold) {
> >         /*
> >          * When powering on a bridge from D3cold, the whole hierarchy
> >          * may be powered on into D0uninitialized state, resume them to
> >          * give them a chance to suspend again
> >          */
> >         pci_resume_bus(dev->subordinate);
> >     }
> >     ...
> >  }
> > 
> >  For the upstream bridge, the above code will trigger the wake-up of
> >  endpoint devices and then following code will be executed for the
> >  endpoint devices:
> > 
> >  pci_update_current_state(struct pci_dev *dev, pci_power_t state)
> >  {
> >     if (platform_pci_get_power_state(dev) == PCI_D3cold ||
> >         !pci_device_is_present(dev)) {
> >         dev->current_state = PCI_D3cold;
> >     } else if (dev->pm_cap) {
> >         u16 pmcsr;
> > 
> >         pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> >         dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> >     } else {
> >         dev->current_state = state;
> >     }
> >  }
> > 
> >  In the above code, the current_state will be set to D0 for the
> >  endpoint devices since it will go into second block where
> >  it will read the PM_CTRL register.
> >  
> >>>> 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.
> >>
> > 
> >  The state setting to D0 is happening due to the current logic present in
> >  pci_update_current_state(). If we can fix the logic in
> >  pci_update_current_state() to detect this condition and return state D3hot,
> >  then it should also fix the issue. 
> > 
> >  Thanks,
> >  Abhishek
> > 
> 
>  Hi Rafael/Mika,
> 
>  Could you please help regarding the correct way to fix this issue.
>  I can update the patch accordingly.

I think you can try one of the patches posted recently:

https://patchwork.kernel.org/project/linux-pm/patch/3623886.MHq7AAxBmi@kreacher/

Thanks!