From: "Rafael J. Wysocki" <rjw@rjwysocki.net> To: linux-nvme <linux-nvme@lists.infradead.org> Cc: Keith Busch <kbusch@kernel.org>, Mario Limonciello <Mario.Limonciello@dell.com>, Kai-Heng Feng <kai.heng.feng@canonical.com>, Keith Busch <keith.busch@intel.com>, Christoph Hellwig <hch@lst.de>, Sagi Grimberg <sagi@grimberg.me>, Linux PM <linux-pm@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Rajat Jain <rajatja@google.com>, Linux PCI <linux-pci@vger.kernel.org>, Bjorn Helgaas <helgaas@kernel.org> Subject: [PATCH] nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled Date: Thu, 08 Aug 2019 10:36:37 +0200 [thread overview] Message-ID: <2583975.4sIyE3leJj@kreacher> (raw) In-Reply-To: <20190731221956.GB15795@localhost.localdomain> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> One of the modifications made by commit d916b1be94b6 ("nvme-pci: use host managed power state for suspend") was adding a pci_save_state() call to nvme_suspend() in order to prevent the PCI bus-level PM from being applied to the suspended NVMe devices, but if ASPM is not enabled for the target NVMe device, that causes its PCIe link to stay up and the platform may not be able to get into its optimum low-power state because of that. For example, if ASPM is disabled for the NVMe drive (PC401 NVMe SK hynix 256GB) in my Dell XPS13 9380, leaving it in D0 during suspend-to-idle prevents the SoC from reaching package idle states deeper than PC3, which is way insufficient for system suspend. To address this shortcoming, make nvme_suspend() check if ASPM is enabled for the target device and fall back to full device shutdown and PCI bus-level PM if that is not the case. Fixes: d916b1be94b6 ("nvme-pci: use host managed power state for suspend") Link: https://lore.kernel.org/linux-pm/2763495.NmdaWeg79L@kreacher/T/#t Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- This is an update of the following patch: https://patchwork.kernel.org/patch/11081791/ going with the subject matching the changes in the patch. This also addresses style-related comments from Christoph and follows the Keith's advice to use pci_upstream_bridge() to get to the upstream bridge of the device. Thanks! --- drivers/nvme/host/pci.c | 15 +++++++++++---- drivers/pci/pcie/aspm.c | 20 ++++++++++++++++++++ include/linux/pci.h | 2 ++ 3 files changed, 33 insertions(+), 4 deletions(-) Index: linux-pm/drivers/nvme/host/pci.c =================================================================== --- linux-pm.orig/drivers/nvme/host/pci.c +++ linux-pm/drivers/nvme/host/pci.c @@ -2846,7 +2846,7 @@ static int nvme_resume(struct device *de struct nvme_dev *ndev = pci_get_drvdata(to_pci_dev(dev)); struct nvme_ctrl *ctrl = &ndev->ctrl; - if (pm_resume_via_firmware() || !ctrl->npss || + if (ndev->last_ps == U32_MAX || nvme_set_power_state(ctrl, ndev->last_ps) != 0) nvme_reset_ctrl(ctrl); return 0; @@ -2859,6 +2859,8 @@ static int nvme_suspend(struct device *d struct nvme_ctrl *ctrl = &ndev->ctrl; int ret = -EBUSY; + ndev->last_ps = U32_MAX; + /* * The platform does not remove power for a kernel managed suspend so * use host managed nvme power settings for lowest idle power if @@ -2866,8 +2868,14 @@ static int nvme_suspend(struct device *d * shutdown. But if the firmware is involved after the suspend or the * device does not support any non-default power states, shut down the * device fully. + * + * If ASPM is not enabled for the device, shut down the device and allow + * the PCI bus layer to put it into D3 in order to take the PCIe link + * down, so as to allow the platform to achieve its minimum low-power + * state (which may not be possible if the link is up). */ - if (pm_suspend_via_firmware() || !ctrl->npss) { + if (pm_suspend_via_firmware() || !ctrl->npss || + !pcie_aspm_enabled(pdev)) { nvme_dev_disable(ndev, true); return 0; } @@ -2880,9 +2888,8 @@ static int nvme_suspend(struct device *d ctrl->state != NVME_CTRL_ADMIN_ONLY) goto unfreeze; - ndev->last_ps = 0; ret = nvme_get_power_state(ctrl, &ndev->last_ps); - if (ret < 0) + if (ret < 0 || ndev->last_ps == U32_MAX) goto unfreeze; ret = nvme_set_power_state(ctrl, ctrl->npss); Index: linux-pm/drivers/pci/pcie/aspm.c =================================================================== --- linux-pm.orig/drivers/pci/pcie/aspm.c +++ linux-pm/drivers/pci/pcie/aspm.c @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, NULL, 0644); +/* + * pcie_aspm_enabled - Return the mask of enabled ASPM link states. + * @pci_device: Target device. + */ +u32 pcie_aspm_enabled(struct pci_dev *pci_device) +{ + struct pci_dev *bridge = pci_upstream_bridge(pci_device); + u32 ret; + + if (!bridge) + return 0; + + mutex_lock(&aspm_lock); + ret = bridge->link_state ? bridge->link_state->aspm_enabled : 0; + mutex_unlock(&aspm_lock); + + return ret; +} + + #ifdef CONFIG_PCIEASPM_DEBUG static ssize_t link_state_show(struct device *dev, struct device_attribute *attr, Index: linux-pm/include/linux/pci.h =================================================================== --- linux-pm.orig/include/linux/pci.h +++ linux-pm/include/linux/pci.h @@ -1567,8 +1567,10 @@ extern bool pcie_ports_native; #ifdef CONFIG_PCIEASPM bool pcie_aspm_support_enabled(void); +u32 pcie_aspm_enabled(struct pci_dev *pci_device); #else static inline bool pcie_aspm_support_enabled(void) { return false; } +static inline u32 pcie_aspm_enabled(struct pci_dev *pci_device) { return 0; } #endif #ifdef CONFIG_PCIEAER
WARNING: multiple messages have this Message-ID (diff)
From: rjw@rjwysocki.net (Rafael J. Wysocki) Subject: [PATCH] nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled Date: Thu, 08 Aug 2019 10:36:37 +0200 [thread overview] Message-ID: <2583975.4sIyE3leJj@kreacher> (raw) In-Reply-To: <20190731221956.GB15795@localhost.localdomain> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> One of the modifications made by commit d916b1be94b6 ("nvme-pci: use host managed power state for suspend") was adding a pci_save_state() call to nvme_suspend() in order to prevent the PCI bus-level PM from being applied to the suspended NVMe devices, but if ASPM is not enabled for the target NVMe device, that causes its PCIe link to stay up and the platform may not be able to get into its optimum low-power state because of that. For example, if ASPM is disabled for the NVMe drive (PC401 NVMe SK hynix 256GB) in my Dell XPS13 9380, leaving it in D0 during suspend-to-idle prevents the SoC from reaching package idle states deeper than PC3, which is way insufficient for system suspend. To address this shortcoming, make nvme_suspend() check if ASPM is enabled for the target device and fall back to full device shutdown and PCI bus-level PM if that is not the case. Fixes: d916b1be94b6 ("nvme-pci: use host managed power state for suspend") Link: https://lore.kernel.org/linux-pm/2763495.NmdaWeg79L at kreacher/T/#t Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com> --- This is an update of the following patch: https://patchwork.kernel.org/patch/11081791/ going with the subject matching the changes in the patch. This also addresses style-related comments from Christoph and follows the Keith's advice to use pci_upstream_bridge() to get to the upstream bridge of the device. Thanks! --- drivers/nvme/host/pci.c | 15 +++++++++++---- drivers/pci/pcie/aspm.c | 20 ++++++++++++++++++++ include/linux/pci.h | 2 ++ 3 files changed, 33 insertions(+), 4 deletions(-) Index: linux-pm/drivers/nvme/host/pci.c =================================================================== --- linux-pm.orig/drivers/nvme/host/pci.c +++ linux-pm/drivers/nvme/host/pci.c @@ -2846,7 +2846,7 @@ static int nvme_resume(struct device *de struct nvme_dev *ndev = pci_get_drvdata(to_pci_dev(dev)); struct nvme_ctrl *ctrl = &ndev->ctrl; - if (pm_resume_via_firmware() || !ctrl->npss || + if (ndev->last_ps == U32_MAX || nvme_set_power_state(ctrl, ndev->last_ps) != 0) nvme_reset_ctrl(ctrl); return 0; @@ -2859,6 +2859,8 @@ static int nvme_suspend(struct device *d struct nvme_ctrl *ctrl = &ndev->ctrl; int ret = -EBUSY; + ndev->last_ps = U32_MAX; + /* * The platform does not remove power for a kernel managed suspend so * use host managed nvme power settings for lowest idle power if @@ -2866,8 +2868,14 @@ static int nvme_suspend(struct device *d * shutdown. But if the firmware is involved after the suspend or the * device does not support any non-default power states, shut down the * device fully. + * + * If ASPM is not enabled for the device, shut down the device and allow + * the PCI bus layer to put it into D3 in order to take the PCIe link + * down, so as to allow the platform to achieve its minimum low-power + * state (which may not be possible if the link is up). */ - if (pm_suspend_via_firmware() || !ctrl->npss) { + if (pm_suspend_via_firmware() || !ctrl->npss || + !pcie_aspm_enabled(pdev)) { nvme_dev_disable(ndev, true); return 0; } @@ -2880,9 +2888,8 @@ static int nvme_suspend(struct device *d ctrl->state != NVME_CTRL_ADMIN_ONLY) goto unfreeze; - ndev->last_ps = 0; ret = nvme_get_power_state(ctrl, &ndev->last_ps); - if (ret < 0) + if (ret < 0 || ndev->last_ps == U32_MAX) goto unfreeze; ret = nvme_set_power_state(ctrl, ctrl->npss); Index: linux-pm/drivers/pci/pcie/aspm.c =================================================================== --- linux-pm.orig/drivers/pci/pcie/aspm.c +++ linux-pm/drivers/pci/pcie/aspm.c @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, NULL, 0644); +/* + * pcie_aspm_enabled - Return the mask of enabled ASPM link states. + * @pci_device: Target device. + */ +u32 pcie_aspm_enabled(struct pci_dev *pci_device) +{ + struct pci_dev *bridge = pci_upstream_bridge(pci_device); + u32 ret; + + if (!bridge) + return 0; + + mutex_lock(&aspm_lock); + ret = bridge->link_state ? bridge->link_state->aspm_enabled : 0; + mutex_unlock(&aspm_lock); + + return ret; +} + + #ifdef CONFIG_PCIEASPM_DEBUG static ssize_t link_state_show(struct device *dev, struct device_attribute *attr, Index: linux-pm/include/linux/pci.h =================================================================== --- linux-pm.orig/include/linux/pci.h +++ linux-pm/include/linux/pci.h @@ -1567,8 +1567,10 @@ extern bool pcie_ports_native; #ifdef CONFIG_PCIEASPM bool pcie_aspm_support_enabled(void); +u32 pcie_aspm_enabled(struct pci_dev *pci_device); #else static inline bool pcie_aspm_support_enabled(void) { return false; } +static inline u32 pcie_aspm_enabled(struct pci_dev *pci_device) { return 0; } #endif #ifdef CONFIG_PCIEAER
next prev parent reply other threads:[~2019-08-08 8:36 UTC|newest] Thread overview: 150+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-25 9:51 [Regression] Commit "nvme/pci: Use host managed power state for suspend" has problems Rafael J. Wysocki 2019-07-25 9:51 ` Rafael J. Wysocki 2019-07-25 14:02 ` Kai-Heng Feng 2019-07-25 14:02 ` Kai-Heng Feng 2019-07-25 16:23 ` Mario.Limonciello 2019-07-25 16:23 ` Mario.Limonciello 2019-07-25 17:03 ` Rafael J. Wysocki 2019-07-25 17:03 ` Rafael J. Wysocki 2019-07-25 17:23 ` Mario.Limonciello 2019-07-25 17:23 ` Mario.Limonciello 2019-07-25 18:20 ` Kai-Heng Feng 2019-07-25 18:20 ` Kai-Heng Feng 2019-07-25 19:09 ` Mario.Limonciello 2019-07-25 19:09 ` Mario.Limonciello 2019-07-30 10:45 ` Rafael J. Wysocki 2019-07-30 10:45 ` Rafael J. Wysocki 2019-07-30 14:41 ` Keith Busch 2019-07-30 14:41 ` Keith Busch 2019-07-30 17:14 ` Mario.Limonciello 2019-07-30 17:14 ` Mario.Limonciello 2019-07-30 18:50 ` Kai-Heng Feng 2019-07-30 18:50 ` Kai-Heng Feng 2019-07-30 19:19 ` Keith Busch 2019-07-30 19:19 ` Keith Busch 2019-07-30 21:05 ` Mario.Limonciello 2019-07-30 21:05 ` Mario.Limonciello 2019-07-30 21:31 ` Keith Busch 2019-07-30 21:31 ` Keith Busch 2019-07-31 21:25 ` Rafael J. Wysocki 2019-07-31 21:25 ` Rafael J. Wysocki 2019-07-31 22:19 ` Keith Busch 2019-07-31 22:19 ` Keith Busch 2019-07-31 22:33 ` Rafael J. Wysocki 2019-07-31 22:33 ` Rafael J. Wysocki 2019-08-01 9:05 ` Kai-Heng Feng 2019-08-01 9:05 ` Kai-Heng Feng 2019-08-01 17:29 ` Rafael J. Wysocki 2019-08-01 17:29 ` Rafael J. Wysocki 2019-08-01 19:05 ` Mario.Limonciello 2019-08-01 19:05 ` Mario.Limonciello 2019-08-01 22:26 ` Rafael J. Wysocki 2019-08-01 22:26 ` Rafael J. Wysocki 2019-08-02 10:55 ` Kai-Heng Feng 2019-08-02 10:55 ` Kai-Heng Feng 2019-08-02 11:04 ` Rafael J. Wysocki 2019-08-02 11:04 ` Rafael J. Wysocki 2019-08-05 19:13 ` Kai-Heng Feng 2019-08-05 19:13 ` Kai-Heng Feng 2019-08-05 21:28 ` Rafael J. Wysocki 2019-08-05 21:28 ` Rafael J. Wysocki 2019-08-06 14:02 ` Mario.Limonciello 2019-08-06 14:02 ` Mario.Limonciello 2019-08-06 15:00 ` Rafael J. Wysocki 2019-08-06 15:00 ` Rafael J. Wysocki 2019-08-07 10:29 ` Rafael J. Wysocki 2019-08-07 10:29 ` Rafael J. Wysocki 2019-08-01 20:22 ` Keith Busch 2019-08-01 20:22 ` Keith Busch 2019-08-07 9:48 ` Rafael J. Wysocki 2019-08-07 9:48 ` Rafael J. Wysocki 2019-08-07 10:45 ` Christoph Hellwig 2019-08-07 10:45 ` Christoph Hellwig 2019-08-07 10:54 ` Rafael J. Wysocki 2019-08-07 10:54 ` Rafael J. Wysocki 2019-08-07 9:53 ` [PATCH] nvme-pci: Do not prevent PCI bus-level PM from being used Rafael J. Wysocki 2019-08-07 9:53 ` Rafael J. Wysocki 2019-08-07 10:14 ` Rafael J. Wysocki 2019-08-07 10:14 ` Rafael J. Wysocki 2019-08-07 10:43 ` Christoph Hellwig 2019-08-07 10:43 ` Christoph Hellwig 2019-08-07 14:37 ` Keith Busch 2019-08-07 14:37 ` Keith Busch 2019-08-08 8:36 ` Rafael J. Wysocki [this message] 2019-08-08 8:36 ` [PATCH] nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled Rafael J. Wysocki 2019-08-08 8:48 ` Christoph Hellwig 2019-08-08 8:48 ` Christoph Hellwig 2019-08-08 9:06 ` Rafael J. Wysocki 2019-08-08 9:06 ` Rafael J. Wysocki 2019-08-08 10:03 ` [PATCH v2 0/2] " Rafael J. Wysocki 2019-08-08 10:03 ` Rafael J. Wysocki 2019-08-08 10:06 ` [PATCH v2 1/2] PCI: PCIe: ASPM: Introduce pcie_aspm_enabled_mask() Rafael J. Wysocki 2019-08-08 10:06 ` Rafael J. Wysocki 2019-08-08 13:15 ` Bjorn Helgaas 2019-08-08 13:15 ` Bjorn Helgaas 2019-08-08 14:48 ` Rafael J. Wysocki 2019-08-08 14:48 ` Rafael J. Wysocki 2019-08-08 10:10 ` [PATCH v2 2/2] nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled Rafael J. Wysocki 2019-08-08 10:10 ` Rafael J. Wysocki 2019-08-08 13:43 ` Bjorn Helgaas 2019-08-08 13:43 ` Bjorn Helgaas 2019-08-08 14:47 ` Rafael J. Wysocki 2019-08-08 14:47 ` Rafael J. Wysocki 2019-08-08 17:06 ` Rafael J. Wysocki 2019-08-08 17:06 ` Rafael J. Wysocki 2019-08-08 18:39 ` Bjorn Helgaas 2019-08-08 18:39 ` Bjorn Helgaas 2019-08-08 20:01 ` Keith Busch 2019-08-08 20:01 ` Keith Busch 2019-08-08 20:05 ` Mario.Limonciello 2019-08-08 20:05 ` Mario.Limonciello 2019-08-08 20:41 ` Rafael J. Wysocki 2019-08-08 20:41 ` Rafael J. Wysocki 2019-08-09 4:47 ` Bjorn Helgaas 2019-08-09 4:47 ` Bjorn Helgaas 2019-08-09 8:04 ` Rafael J. Wysocki 2019-08-09 8:04 ` Rafael J. Wysocki 2019-08-08 21:51 ` [PATCH v3 0/2] " Rafael J. Wysocki 2019-08-08 21:51 ` Rafael J. Wysocki 2019-08-08 21:55 ` [PATCH v3 1/2] PCI: PCIe: ASPM: Introduce pcie_aspm_enabled() Rafael J. Wysocki 2019-08-08 21:55 ` Rafael J. Wysocki 2019-08-09 4:50 ` Bjorn Helgaas 2019-08-09 4:50 ` Bjorn Helgaas 2019-08-09 8:00 ` Rafael J. Wysocki 2019-08-09 8:00 ` Rafael J. Wysocki 2019-10-07 22:34 ` Bjorn Helgaas 2019-10-07 22:34 ` Bjorn Helgaas 2019-10-08 9:27 ` Rafael J. Wysocki 2019-10-08 9:27 ` Rafael J. Wysocki 2019-10-08 21:16 ` Bjorn Helgaas 2019-10-08 21:16 ` Bjorn Helgaas 2019-10-08 22:54 ` Rafael J. Wysocki 2019-10-08 22:54 ` Rafael J. Wysocki 2019-10-09 12:49 ` Bjorn Helgaas 2019-10-09 12:49 ` Bjorn Helgaas 2019-08-08 21:58 ` [PATCH v3 2/2] nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled Rafael J. Wysocki 2019-08-08 21:58 ` Rafael J. Wysocki 2019-08-08 22:13 ` [PATCH v3 0/2] " Keith Busch 2019-08-08 22:13 ` Keith Busch 2019-08-09 8:05 ` Rafael J. Wysocki 2019-08-09 8:05 ` Rafael J. Wysocki 2019-08-09 14:52 ` Keith Busch 2019-08-09 14:52 ` Keith Busch 2019-07-25 16:59 ` [Regression] Commit "nvme/pci: Use host managed power state for suspend" has problems Rafael J. Wysocki 2019-07-25 16:59 ` Rafael J. Wysocki 2019-07-25 14:52 ` Keith Busch 2019-07-25 14:52 ` Keith Busch 2019-07-25 19:48 ` Rafael J. Wysocki 2019-07-25 19:48 ` Rafael J. Wysocki 2019-07-25 19:52 ` Keith Busch 2019-07-25 19:52 ` Keith Busch 2019-07-25 20:02 ` Rafael J. Wysocki 2019-07-25 20:02 ` Rafael J. Wysocki 2019-07-26 14:02 ` Kai-Heng Feng 2019-07-26 14:02 ` Kai-Heng Feng 2019-07-27 12:55 ` Rafael J. Wysocki 2019-07-27 12:55 ` Rafael J. Wysocki 2019-07-29 15:51 ` Mario.Limonciello 2019-07-29 15:51 ` Mario.Limonciello 2019-07-29 22:05 ` Rafael J. Wysocki 2019-07-29 22:05 ` Rafael J. Wysocki
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2583975.4sIyE3leJj@kreacher \ --to=rjw@rjwysocki.net \ --cc=Mario.Limonciello@dell.com \ --cc=hch@lst.de \ --cc=helgaas@kernel.org \ --cc=kai.heng.feng@canonical.com \ --cc=kbusch@kernel.org \ --cc=keith.busch@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=rajatja@google.com \ --cc=sagi@grimberg.me \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.