On Mon, Sep 18, 2023 at 02:48:01PM +0200, Lukas Wunner wrote:
> struct pci_dev contains two flags which govern whether the device may
> suspend to D3cold:
>
> * no_d3cold provides an opt-out for drivers (e.g. if a device is known
> to not wake from D3cold)
>
> * d3cold_allowed provides an opt-out for user space (default is true,
> user space may set to false)
>
> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend"),
> the user space setting overwrites the driver setting. Essentially user
> space is trusted to know better than the driver whether D3cold is
> working.
>
> That feels unsafe and wrong. Assume that the change was introduced
> inadvertently and do not overwrite no_d3cold when d3cold_allowed is
> modified. Instead, consider d3cold_allowed in addition to no_d3cold
> when choosing a suspend state for the device.
>
> That way, user space may opt out of D3cold if the driver hasn't, but it
> may no longer force an opt in if the driver has opted out.
>
> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # v4.8+
> Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
Applied with Reviewed-by from Mika and Mario to pci/pm for v6.7,
thanks!
> ---
> drivers/pci/pci-acpi.c | 2 +-
> drivers/pci/pci-sysfs.c | 5 +----
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 7aa1c20..2f5eddf 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -911,7 +911,7 @@ pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
> {
> int acpi_state, d_max;
>
> - if (pdev->no_d3cold)
> + if (pdev->no_d3cold || !pdev->d3cold_allowed)
> d_max = ACPI_STATE_D3_HOT;
> else
> d_max = ACPI_STATE_D3_COLD;
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index ba38fc4..e18ccd2 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -530,10 +530,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
> return -EINVAL;
>
> pdev->d3cold_allowed = !!val;
> - if (pdev->d3cold_allowed)
> - pci_d3cold_enable(pdev);
> - else
> - pci_d3cold_disable(pdev);
> + pci_bridge_d3_update(pdev);
>
> pm_runtime_resume(dev);
>
> --
> 2.39.2
>
