Received-SPF: pass (google.com: domain of linux-kernel+bounces-14974-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <20cd835e-f84c-4c43-812e-6706f7266150@linux.intel.com>
Date: Tue, 2 Jan 2024 15:33:51 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Revert "PCI/ASPM: Remove pcie_aspm_pm_state_change()"
Content-Language: en-US
To: Bjorn Helgaas <helgaas@kernel.org>, Michael Schaller
 <michael@5challer.de>, Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
 regressions@lists.linux.dev, "Maciej W . Rozycki" <macro@orcam.me.uk>,
 Ajay Agarwal <ajayagarwal@google.com>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 Heiner Kallweit <hkallweit1@gmail.com>,
 Johan Hovold <johan+linaro@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
 stable@vger.kernel.org
References: <76c61361-b8b4-435f-a9f1-32b716763d62@5challer.de>
 <20240102232550.1751655-1-helgaas@kernel.org>
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
In-Reply-To: <20240102232550.1751655-1-helgaas@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 1/2/2024 3:25 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This reverts commit 08d0cc5f34265d1a1e3031f319f594bd1970976c.
> 
> Michael reported that when attempting to resume from suspend to RAM on ASUS
> mini PC PN51-BB757MDE1 (DMI model: MINIPC PN51-E1), 08d0cc5f3426
> ("PCI/ASPM: Remove pcie_aspm_pm_state_change()") caused a 12-second delay
> with no output, followed by a reboot.
> 
> Workarounds include:
> 
>   - Reverting 08d0cc5f3426 ("PCI/ASPM: Remove pcie_aspm_pm_state_change()")
>   - Booting with "pcie_aspm=off"
>   - Booting with "pcie_aspm.policy=performance"
>   - "echo 0 | sudo tee /sys/bus/pci/devices/0000:03:00.0/link/l1_aspm"
>     before suspending
>   - Connecting a USB flash drive
> 

Did you find the root cause? Is this issue specific to that particular
device? If yes, can we do a quirk?

> Fixes: 08d0cc5f3426 ("PCI/ASPM: Remove pcie_aspm_pm_state_change()")
> Reported-by: Michael Schaller <michael@5challer.de>
> Link: https://lore.kernel.org/r/76c61361-b8b4-435f-a9f1-32b716763d62@5challer.de
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/pci/pci.c       |  6 ++++++
>  drivers/pci/pci.h       |  2 ++
>  drivers/pci/pcie/aspm.c | 19 +++++++++++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 55bc3576a985..bdbf8a94b4d0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1335,6 +1335,9 @@ static int pci_set_full_power_state(struct pci_dev *dev)
>  		pci_restore_bars(dev);
>  	}
>  
> +	if (dev->bus->self)
> +		pcie_aspm_pm_state_change(dev->bus->self);
> +
>  	return 0;
>  }
>  
> @@ -1429,6 +1432,9 @@ static int pci_set_low_power_state(struct pci_dev *dev, pci_power_t state)
>  				     pci_power_name(dev->current_state),
>  				     pci_power_name(state));
>  
> +	if (dev->bus->self)
> +		pcie_aspm_pm_state_change(dev->bus->self);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5ecbcf041179..f43873049d52 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -569,10 +569,12 @@ int pcie_retrain_link(struct pci_dev *pdev, bool use_lt);
>  #ifdef CONFIG_PCIEASPM
>  void pcie_aspm_init_link_state(struct pci_dev *pdev);
>  void pcie_aspm_exit_link_state(struct pci_dev *pdev);
> +void pcie_aspm_pm_state_change(struct pci_dev *pdev);
>  void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
>  #else
>  static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
> +static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
>  #endif
>  
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 50b04ae5c394..8715e951c491 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1008,6 +1008,25 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>  	up_read(&pci_bus_sem);
>  }
>  
> +/* @pdev: the root port or switch downstream port */
> +void pcie_aspm_pm_state_change(struct pci_dev *pdev)
> +{
> +	struct pcie_link_state *link = pdev->link_state;
> +
> +	if (aspm_disabled || !link)
> +		return;
> +	/*
> +	 * Devices changed PM state, we should recheck if latency
> +	 * meets all functions' requirement
> +	 */
> +	down_read(&pci_bus_sem);
> +	mutex_lock(&aspm_lock);
> +	pcie_update_aspm_capable(link->root);
> +	pcie_config_aspm_path(link);
> +	mutex_unlock(&aspm_lock);
> +	up_read(&pci_bus_sem);
> +}
> +
>  void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
>  {
>  	struct pcie_link_state *link = pdev->link_state;

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer