lkml.org 
[lkml]   [2023]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices
From


On 8/17/2023 2:37 PM, Rafael J. Wysocki wrote:
> On Thu, Aug 17, 2023 at 9:30 PM Limonciello, Mario
> <mario.limonciello@amd.com> wrote:
>>
>>
>>
>> On 8/17/2023 2:25 PM, Bjorn Helgaas wrote:
>>> On Wed, Aug 16, 2023 at 03:41:43PM -0500, Mario Limonciello wrote:
>>>> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>>>> PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
>>>> storing a value to the `bridge_d3` variable in the `struct pci_dev`
>>>> structure.
>>>> ...
>>>
>>>> +static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
>>>> +{
>>>> + pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
>>>> +
>>>> + if (pdev->current_state == target)
>>>> + return;
>>>> +
>>>> + /* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
>>>> + if (target == ACPI_STATE_D3_HOT &&
>>>
>>> ACPI_STATE_D3_HOT is not a valid pci_power_t value.
>>
>> Based on this, kernel robot sparse complaints and your comments on v11's
>> last patch I am going to split off to another function that returns the
>> pci_power_t state based upon the situation and better comment the reason
>> for the D0 when not enabled.
>>
>>>
>>>> + (pdev->current_state == PCI_D3hot ||
>>>> + pdev->current_state == PCI_D3cold))
>>>> + return;
>>>> +
>>>> + if (pm_debug_messages_on)
>>>> + acpi_handle_info(entry->handle,
>>>> + "LPI: PCI device in %s, not in %s\n",
>>>> + acpi_power_state_string(pdev->current_state),
>>>> + acpi_power_state_string(target));
>>>> +
>>>> + /* don't try with things that PCI core hasn't touched */
>>>> + if (pdev->current_state == PCI_UNKNOWN) {
>>>> + entry->handle = NULL;
>>>> + return;
>>>> + }
>>>> +
>>>> + pci_set_power_state(pdev, target);
>>>
>>> It doesn't seem logical for a "check_constraints()" function that
>>> takes no parameters and returns nothing to actively set the PCI power
>>> state.
>>>
>>> lpi_check_constraints() returns nothing, and from the fact that it was
>>> previously only called when "pm_debug_messages_on", I infer that it
>>> should have no side effects.
>>>
>>> IMHO "lpi_check_constraints" is not a great name because "check"
>>> doesn't suggest anything specific about what it does.
>>> "dump_constraints()" -- fine. "log_unmet_constraints()" -- fine
>>> (seems like the original intention of 726fb6b4f2a8 ("ACPI / PM: Check
>>> low power idle constraints for debug only"), which added it.
>>>
>>
>> Great feedback, thanks. I'm thinking to instead change it to:
>>
>> lpi_enforce_constraints()
>
> Don't even try to go this way, please.
>
> Originally, the LPI constraints are there to indicate to Windows
> whether or not it should attempt to enter Connected/Modern Standby.
>
> Because Linux doesn't do Modern Standby, it doesn't use the LPI
> constraints the way Windows does and it really shouldn't do that.
>
> I think that the exercise here is to use the information from the
> constraints list as an indication whether or not a given PCI Root Port
> is supposed to be put into D3hot/cold on suspend-to-idle and this has
> nothing to do with enforcement.

What do you think about me making the changes to pci_prepare_to_sleep()?

Something like this:

@@ -2733,11 +2742,17 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
{
bool wakeup = device_may_wakeup(&dev->dev);
pci_power_t target_state = pci_target_state(dev, wakeup);
+ pci_power_t constraint;
int error;

if (target_state == PCI_POWER_ERROR)
return -EIO;

+ /* if platform indicates device constraint for suspend, use it */
+ constraint = platform_check_constraint(dev, target_state);
+ if (constraint != PCI_POWER_ERROR)
+ target_state = constraint;
+
pci_enable_wake(dev, target_state, wakeup);

error = pci_set_power_state(dev, target_state);

\
 
 \ /
  Last update: 2023-08-17 21:42    [W:0.047 / U:0.656 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site