Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Aug 2023 14:49:21 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     Mika Westerberg <mika.westerberg@linux.intel.com>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Bjorn Helgaas <helgaas@kernel.org>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
        Iain Lane <iain@orangesquash.org.uk>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>
Subject: Re: [PATCH v8 2/2] PCI/ACPI: Use device constraints instead of dates
 to opt devices into D3
Message-ID: <ZMuUQb4AcZpMB2Gq@smile.fi.intel.com>
References: <20230802201013.910-1-mario.limonciello@amd.com>
 <20230802201013.910-3-mario.limonciello@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230802201013.910-3-mario.limonciello@amd.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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