Re: [PATCH v9 3/3] PCI/ACPI: Use device constraints to decide PCI target state fallback policy

[Mario Limonciello <mario.limonciello@amd.com>]

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;
>> +}
>