Re: [PATCH] PCI: Fix the ACPI power state during runtime resume
From: Abhishek Sahu
Date: Wed Apr 06 2022 - 03:22:11 EST
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.
Thanks,
Abhishek