Messages in this thread | | | From | Chaitanya Kulkarni <> | Subject | Re: [PATCH] nvme-pci: Use non-operational power state instead of D3 on Suspend-to-Idle | Date | Wed, 8 May 2019 19:15:29 +0000 |
| |
Did you get a chance to run this patch through checkpatch.pl ?
On 05/08/2019 12:00 PM, Kai-Heng Feng wrote: > Several NVMes consume lots of power during Suspend-to-Idle. > > According to the NVMe vendors, APST should be used and there are two > things that should be prevented: > - Controller shutdown (CC.SHN) > - PCI D3 > > I think that's because the Windows Modern Standby design document [1] > states below as a requirement: > " > Akin to AHCI PCIe SSDs, NVMe SSDs need to provide the host with a > non-operational power state that is comparable to DEVSLP (<5mW draw, > <100ms exit latency) in order to allow the host to perform appropriate > transitions into Modern Standby. Should the NVMe SSD not expose such a > non-operational power state, autonomous power state transitions (APST) > is the only other option to enter Modern Standby successfully. > " > > D3 wasn't mentioned at all, though for some vendors D3 still put the > device into a low power state, others disable APST after trasition to > D3. > > So instead of disabling NVMe controller in suspend callback we do the > following instead: > - Make sure all IRQs are quiesced > - Stop all queues > - Prevent the device entering D3 > - Use memory barrier to make sure DMA operations are flushed on HMB devices > > This patch has been verified on several different NVMes: > - Intel > - Hynix > - LiteOn > - Micron > - WDC > - Samsung > - Tohiba > > With the new suspend routine, the laptop I tested use roughly 0.8W > under Suspend-to-Idle, and resume time is faster than the original > D3 scheme. > > The only one exception so far is Toshiba XG5, which works with the old > suspend routine, so introduce a new quirk for XG5. > We are working with Toshiba to root cause the issue on XG5. > > [1] https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/part-selection#ssd-storage > > Tested-by: Lucien Kao <Lucien_kao@compal.com> > Tested-by: Mice Lin <mice_lin@wistron.com> > Tested-by: Jason Tan <LiangChuan.Tan@wdc.com> > Tested-by: Cody Liu (codyliu) <codyliu@micron.com> > Tested-by: Theodore Ts'o <tytso@mit.edu> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/nvme/host/core.c | 8 +++ > drivers/nvme/host/nvme.h | 8 +++ > drivers/nvme/host/pci.c | 102 +++++++++++++++++++++++++++++++++++++-- > 3 files changed, 115 insertions(+), 3 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index a6644a2c3ef7..929db749da98 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -3903,6 +3903,14 @@ static inline void _nvme_check_size(void) > BUILD_BUG_ON(sizeof(struct nvme_directive_cmd) != 64); > } > > +void nvme_enter_deepest(struct nvme_ctrl *ctrl) > +{ > + int ret = nvme_set_features(ctrl, NVME_FEAT_POWER_MGMT, ctrl->npss, > + NULL, 0, NULL); > + if (ret) > + dev_warn(ctrl->device, "failed to set deepest non-operational state (%d)\n", ret); > +} > +EXPORT_SYMBOL_GPL(nvme_enter_deepest); > > static int __init nvme_core_init(void) > { > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > index 5ee75b5ff83f..ea40a83314ae 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -92,6 +92,11 @@ enum nvme_quirks { > * Broken Write Zeroes. > */ > NVME_QUIRK_DISABLE_WRITE_ZEROES = (1 << 9), > + > + /* > + * Use D3 on S2I regardless of NPSS. > + */ > + NVME_QUIRK_USE_D3_ON_S2I = (1 << 10), > }; > > /* > @@ -229,6 +234,7 @@ struct nvme_ctrl { > /* Power saving configuration */ > u64 ps_max_latency_us; > bool apst_enabled; > + bool suspend_to_idle; > > /* PCIe only: */ > u32 hmpre; > @@ -467,6 +473,8 @@ int nvme_delete_ctrl(struct nvme_ctrl *ctrl); > int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp, > void *log, size_t size, u64 offset); > > +void nvme_enter_deepest(struct nvme_ctrl *ctrl); > + > extern const struct attribute_group *nvme_ns_id_attr_groups[]; > extern const struct block_device_operations nvme_ns_head_ops; > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index 3e4fb891a95a..dea42b41f9a8 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -23,6 +23,7 @@ > #include <linux/io-64-nonatomic-lo-hi.h> > #include <linux/sed-opal.h> > #include <linux/pci-p2pdma.h> > +#include <linux/suspend.h> > > #include "trace.h" > #include "nvme.h" > @@ -2828,12 +2829,98 @@ static void nvme_remove(struct pci_dev *pdev) > } > > #ifdef CONFIG_PM_SLEEP > +static void nvme_do_suspend_to_idle(struct pci_dev *pdev) > +{ > + struct nvme_dev *ndev = pci_get_drvdata(pdev); > + > + pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3; > + ndev->ctrl.suspend_to_idle = true; > + > + nvme_start_freeze(&ndev->ctrl); > + nvme_wait_freeze_timeout(&ndev->ctrl, NVME_IO_TIMEOUT); > + nvme_stop_queues(&ndev->ctrl); > + > + nvme_enter_deepest(&ndev->ctrl); > + > + if (ndev->ctrl.queue_count > 0) { > + nvme_disable_io_queues(ndev); > + nvme_poll_irqdisable(&ndev->queues[0], -1); > + } > + > + nvme_suspend_io_queues(ndev); > + nvme_suspend_queue(&ndev->queues[0]); > + pci_free_irq_vectors(pdev); > + > + blk_mq_tagset_busy_iter(&ndev->tagset, nvme_cancel_request, &ndev->ctrl); > + blk_mq_tagset_busy_iter(&ndev->admin_tagset, nvme_cancel_request, &ndev->ctrl); > + > + /* Make sure all HMB DMA operations are done */ > + mb(); > +} > + > +static int nvme_do_resume_from_idle(struct pci_dev *pdev) > +{ > + struct nvme_dev *ndev = pci_get_drvdata(pdev); > + int result; > + > + pdev->dev_flags &= ~PCI_DEV_FLAGS_NO_D3; > + ndev->ctrl.suspend_to_idle = false; > + > + result = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES); > + if (result < 0) > + goto out; > + > + result = nvme_pci_configure_admin_queue(ndev); > + if (result) > + goto out; > + > + result = nvme_alloc_admin_tags(ndev); > + if (result) > + goto out; > + > + ndev->ctrl.max_hw_sectors = NVME_MAX_KB_SZ << 1; > + ndev->ctrl.max_segments = NVME_MAX_SEGS; > + mutex_unlock(&ndev->shutdown_lock); > + > + result = nvme_init_identify(&ndev->ctrl); > + if (result) > + goto out; > + > + result = nvme_setup_io_queues(ndev); > + if (result) > + goto out; > + > + if (ndev->online_queues < 2) { > + dev_warn(ndev->ctrl.device, "IO queues not created\n"); > + nvme_kill_queues(&ndev->ctrl); > + nvme_remove_namespaces(&ndev->ctrl); > + } else { > + nvme_start_queues(&ndev->ctrl); > + nvme_wait_freeze(&ndev->ctrl); > + nvme_dev_add(ndev); > + nvme_unfreeze(&ndev->ctrl); > + } > + > + nvme_start_ctrl(&ndev->ctrl); > + > + return 0; > + > + out: > + nvme_remove_dead_ctrl(ndev, result); > + return result; > +} > + > static int nvme_suspend(struct device *dev) > { > struct pci_dev *pdev = to_pci_dev(dev); > struct nvme_dev *ndev = pci_get_drvdata(pdev); > > - nvme_dev_disable(ndev, true); > + if (IS_ENABLED(CONFIG_ACPI) && pm_suspend_via_s2idle() && > + ndev->ctrl.npss && !(ndev->ctrl.quirks & NVME_QUIRK_USE_D3_ON_S2I)) > + nvme_do_suspend_to_idle(pdev); > + else > + nvme_dev_disable(ndev, true); > + > return 0; > } > > @@ -2841,9 +2928,16 @@ static int nvme_resume(struct device *dev) > { > struct pci_dev *pdev = to_pci_dev(dev); > struct nvme_dev *ndev = pci_get_drvdata(pdev); > + int result = 0; > > - nvme_reset_ctrl(&ndev->ctrl); > - return 0; > + if (ndev->ctrl.suspend_to_idle) { > + result = nvme_do_resume_from_idle(pdev); > + if (result) > + dev_warn(dev, "failed to resume from idle state (%d)\n", result); > + } else > + nvme_reset_ctrl(&ndev->ctrl); > + > + return result; > } > #endif > > @@ -2921,6 +3015,8 @@ static const struct pci_device_id nvme_id_table[] = { > { PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */ > .driver_data = NVME_QUIRK_IDENTIFY_CNS | > NVME_QUIRK_DISABLE_WRITE_ZEROES, }, > + { PCI_DEVICE(0x1179, 0x0116), /* Toshiba XG5 */ > + .driver_data = NVME_QUIRK_USE_D3_ON_S2I, }, > { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ > .driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, }, > { PCI_DEVICE(0x1c58, 0x0003), /* HGST adapter */ >
| |