lkml.org 
[lkml]   [2023]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v9 3/3] PCI/ACPI: Use device constraints to decide PCI target state fallback policy
From
On 8/3/23 23:30, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 08:02:29PM -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.
>>
>> pci_power_manageable() uses this variable to indicate a PCIe port can
>> enter D3.
>> pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
>> decide whether to try to put a device into its target state for a sleep
>> cycle via pci_prepare_to_sleep().
>>
>> For devices that support D3, the target state is selected by this policy:
>> 1. If platform_pci_power_manageable():
>> Use platform_pci_choose_state()
>> 2. If the device is armed for wakeup:
>> Select the deepest D-state that supports a PME.
>> 3. Else:
>> Use D3hot.
>>
>> Devices are considered power manageable by the platform when they have
>> one or more objects described in the table in section 7.3 of the ACPI 6.5
>> specification.
>>
>> When devices are not considered power manageable; specs are ambiguous as
>> to what should happen. In this situation Windows 11 leaves PCIe
>> ports in D0 while Linux puts them into D3 due to the above mentioned
>> commit.
>>
>> In Windows systems that support Modern Standby specify hardware
>> pre-conditions for the SoC to achieve the lowest power state by device
>> constraints in a SOC specific "Power Engine Plugin" (PEP) [2] [3].
>> They can be marked as disabled or enabled and when enabled can specify
>> the minimum power state required for an ACPI device.
>>
>> When it is ambiguous what should happen, adjust the logic for
>> pci_target_state() to check whether a device constraint is present
>> and enabled.
>> * If power manageable by ACPI use this to get to select target state
>> * If a device constraint is present but disabled then choose D0
>> * If a device constraint is present and enabled then use it
>> * If a device constraint is not present, then continue to existing
>> logic (if marked for wakeup use deepest state that PME works)
>> * If not marked for wakeup choose D3hot
>
> ...
>
>> +/**
>> + * acpi_get_lps0_constraint - get any LPS0 constraint for a device
>> + * @dev: device to get constraint for
>> + *
>> + * If a constraint has been specified in the _DSM method for the device,
>> + * and the constraint is enabled return it. If the constraint is disabled,
>> + * return 0. Otherwise, return -ENODEV.
>> + */
>
> I believe you get a kernel-doc warning. Always test kernel doc with
>
> scripts/kernel-doc -v -none -Wall ...your file...
>

Thanks, will double check these.

> ...
>
>> +/**
>> + * acpi_pci_device_constraint - determine if the platform has a contraint for the device
>> + * @dev: PCI device to check
>> + * @result (out): the constraint specified by the platform
>> + *
>> + * If the platform has specified a constraint for a device, this function will
>> + * return 0 and set @result to the constraint.
>> + * Otherwise, it will return an error code.
>> + */
>
> Ditto.
>
> ...
>
>> +int acpi_pci_device_constraint(struct pci_dev *dev, int *result)
>> +{
>> + int constraint;
>> +
>> + constraint = acpi_get_lps0_constraint(&dev->dev);
>
>> + pci_dbg(dev, "ACPI device constraint: %d\n", constraint);
>
> Does it make sense before the below check? Why can we be interested in the
> _exact_ negative values? (Note that non-printing is already a sign that either
> we don't call this or have negative constraint.)

There are two different negative values that can come up:
-ENODEV or -EINVAL. Both were interesting while coming up with this
series because they mean something different about why a constraint
wasn't selected.

-ENODEV means the constraint wasn't found.
-EINVAL means the constraint was found but something is wrong with the
table parser or the table itself. I found the table parser wasn't
working correctly originaly thanks to this.

Maybe now that I've got it all working you're right and this should go
after the error checking.

>
>> + if (constraint < 0)
>> + return constraint;
>> + *result = constraint;
>> +
>> + return 0;
>> +}
>

\
 
 \ /
  Last update: 2023-08-04 06:38    [W:0.043 / U:0.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site