linux-kernel - Re: [PATCH v8 2/2] PCI/ACPI: Use device constraints instead of dates to opt devices into D3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZMuUQb4AcZpMB2Gq@smile.fi.intel.com>
Date:   Thu, 3 Aug 2023 14:49:21 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Mario Limonciello <mario.limonciello@....com>
Cc:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Bjorn Helgaas <helgaas@...nel.org>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        Iain Lane <iain@...ngesquash.org.uk>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
Subject: Re: [PATCH v8 2/2] PCI/ACPI: Use device constraints instead of dates
 to opt devices into D3

On Wed, Aug 02, 2023 at 03:10:13PM -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 seems to leave PCIe
> root 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.
> 
> Instead of using a time based heuristic to decide if a port should go
> into D3 use device constraints to decide.
> * If the constraint is not present or disabled then choose D0.
> * If the constraint is enabled, then enable D3 if the constraint is set
>   to 3 or greater.

...

> +/*
> + * acpi_get_lps0_constraint - get any LPS0 constraint for an acpi device
> + * @handle: ACPI handle of the device
> + *
> + * If a constraint has been specified in the _DSM method for the device,
> + * return it.  Otherwise, return -ENODEV.
> + */
> +int acpi_get_lps0_constraint(struct device *dev)
> +{

> +	acpi_handle handle = ACPI_HANDLE(dev);

(see below)

> +	int i;
> +
> +	if (!handle)
> +		return -ENODEV;
> +
> +	for (i = 0; i < lpi_constraints_table_size; ++i) {

> +		if (lpi_constraints_table[i].handle != handle)

Maybe

	device_match_acpi_handle()

?

> +			continue;
> +		return lpi_constraints_table[i].min_dstate;
> +	}
> +
> +	return -ENODEV;
> +}

...

> +/*

Is it deliberately non-kernel-doc while mimicking it?

> + * acpi_pci_device_constraint_d3 - determine if device constraints require D3
> + * @dev: PCI device to check
> + *
> + * Returns true if the PEP constraints for the device is enabled and
> + * requires D3.
> + */
> +bool acpi_pci_device_constraint_d3(struct pci_dev *dev)
> +{

> +	int constraint = acpi_get_lps0_constraint(&dev->dev);
> +
> +	if (constraint < 0) {

I slightly prefer

	int constraint;

	constraint = acpi_get_lps0_constraint(&dev->dev);
	if (constraint < 0) {

> +		pci_dbg(dev, "ACPI device constraint not present\n");
> +		return false;
> +	}
> +
> +	return constraint >= 3;
> +}

-- 
With Best Regards,
Andy Shevchenko