Re: [PATCH] PCI: Fix the ACPI power state during runtime resume

From: Rafael J. Wysocki
Date: Wed Apr 06 2022 - 11:11:53 EST

Next message: Kalle Valo: "Re: [PATCH] ipw2x00: Fix potential NULL dereference in libipw_xmit()"
Previous message: duoming: "Re: Re: [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout()"
In reply to: Abhishek Sahu: "Re: [PATCH] PCI: Fix the ACPI power state during runtime resume"
Next in thread: Abhishek Sahu: "Re: [PATCH] PCI: Fix the ACPI power state during runtime resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wednesday, April 6, 2022 7:32:45 AM CEST Abhishek Sahu wrote:
> On 4/5/2022 10:20 PM, Rafael J. Wysocki wrote:
> > 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@xxxxxxxxxx>
> >>>>> ]
> >>>>>
> >>>>> 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@xxxxxxxxxx>
> >>>>>> ---
> >>>>>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Flinux-pm%2Fpatch%2F3623886.MHq7AAxBmi%40kreacher%2F&data=04%7C01%7Cabhsahu%40nvidia.com%7Cae4c8574f5a44973514a08da172471d6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637847743178405297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=aasJ79EICVnlJQ4EbXA2AtZFW0qnRsMkHEZRI8mnDI8%3D&reserved=0
> >
> > Thanks!
> >
> >
> >
>
> Thanks Rafael.
> I have applied both the changes and still the issue which I mentioned is happening.
>
> Following are the prints:
>
> 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
>
> So ACPI state change is still not happening for PCI endpoint devices.
>
> Also, the I checked the code and the pci_power_up() will not be called
> for endpoint devices. For endpoints, pci_restore_standard_config() will
> be called first where the current state will come as D0.

OK, I see.

The problem is that if the PCI device goes to D0 because of the bridge power-up,
it's ACPI companion's power state may not follow, which means that we really
want to do a full power-up in there.

Please test the appended patch with the patch from

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

still applied.

---
drivers/pci/pci-driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -1312,7 +1312,7 @@ static int pci_pm_runtime_resume(struct
* to a driver because although we left it in D0, it may have gone to
* D3cold when the bridge above it runtime suspended.
*/
- pci_restore_standard_config(pci_dev);
+ pci_pm_default_resume_early(pci_dev);

if (!pci_dev->driver)
return 0;

Next message: Kalle Valo: "Re: [PATCH] ipw2x00: Fix potential NULL dereference in libipw_xmit()"
Previous message: duoming: "Re: Re: [PATCH] drivers: net: slip: fix NPD bug in sl_tx_timeout()"
In reply to: Abhishek Sahu: "Re: [PATCH] PCI: Fix the ACPI power state during runtime resume"
Next in thread: Abhishek Sahu: "Re: [PATCH] PCI: Fix the ACPI power state during runtime resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]