[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f864f6ef-29ae-8cb6-b46c-ee0f32c32fe7@amd.com>
Date: Thu, 3 Aug 2023 23:37:10 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
Bjorn Helgaas <helgaas@...nel.org>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
Iain Lane <iain@...ngesquash.org.uk>,
Shyam-sundar S-k <Shyam-sundar.S-k@....com>
Subject: Re: [PATCH v9 3/3] PCI/ACPI: Use device constraints to decide PCI
target state fallback policy
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;
>> +}
>
Powered by blists - more mailing lists