linux-kernel - RE: [PATCH] nvme-pci: Use non-operational power state instead of D3 on Suspend-to-Idle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7a002851c435481593f8629ec9193e40@AUSX13MPC101.AMER.DELL.COM>
Date:   Thu, 9 May 2019 21:37:58 +0000
From:   <Mario.Limonciello@...l.com>
To:     <kbusch@...nel.org>
CC:     <kai.heng.feng@...onical.com>, <hch@....de>, <axboe@...com>,
        <sagi@...mberg.me>, <rafael@...nel.org>,
        <linux-pm@...r.kernel.org>, <rafael.j.wysocki@...el.com>,
        <linux-kernel@...r.kernel.org>, <linux-nvme@...ts.infradead.org>,
        <keith.busch@...el.com>
Subject: RE: [PATCH] nvme-pci: Use non-operational power state instead of D3
 on Suspend-to-Idle

> On Thu, May 09, 2019 at 06:57:34PM +0000, Mario.Limonciello@...l.com wrote:
> > No, current Windows versions don't transition to D3 with inbox NVME driver.
> > You're correct, it's explicit state transitions even if APST was enabled
> > (as this patch is currently doing as well).
> 
> The proposed patch does too much, and your resume latency will be worse
> off for doing an unnecessary controller reset.
> 
> The following should be all that's needed if the device is spec
> compliant. The resume part isn't necessary if npss is non-operational, but
> we're not saving that info, and it shouldn't hurt to be explicit anyway.
> 
> I don't have any PS capable devices, so this is just compile tested.
> 
> ---
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 6265d9225ec8..ce8b9bc949b9 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1132,6 +1132,22 @@ static int nvme_set_features(struct nvme_ctrl *dev,
> unsigned fid, unsigned dword
>  	return ret;
>  }
> 
> +int nvme_set_power(struct nvme_ctrl *ctrl, unsigned npss)
> +{
> +	int ret;
> +
> +	mutex_lock(&ctrl->scan_lock);
> +	nvme_start_freeze(ctrl);
> +	nvme_wait_freeze(ctrl);
> +	ret = nvme_set_features(ctrl, NVME_FEAT_POWER_MGMT, npss, NULL, 0,
> +				NULL);
> +	nvme_unfreeze(ctrl);
> +	mutex_unlock(&ctrl->scan_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(nvme_set_power);

I believe without memory barriers at the end disks with HMB this will
still kernel panic (Such as Toshiba BG3).

This still allows D3 which we found at least failed to go into deepest state and blocked
platform s0ix for the following SSDs (maybe others):
Hynix PC601
LiteOn CL1

> +
>  int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
>  {
>  	u32 q_count = (*count - 1) | ((*count - 1) << 16);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 527d64545023..f2be6aad9804 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -459,6 +459,7 @@ int __nvme_submit_sync_cmd(struct request_queue *q,
> struct nvme_command *cmd,
>  		unsigned timeout, int qid, int at_head,
>  		blk_mq_req_flags_t flags, bool poll);
>  int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count);
> +int nvme_set_power(struct nvme_ctrl *ctrl, unsigned npss);
>  void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);
>  int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
>  int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl);
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index a90cf5d63aac..2c4154cb4e79 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -18,6 +18,7 @@
>  #include <linux/mutex.h>
>  #include <linux/once.h>
>  #include <linux/pci.h>
> +#include <linux/suspend.h>
>  #include <linux/t10-pi.h>
>  #include <linux/types.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
> @@ -2851,6 +2852,8 @@ static int nvme_suspend(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct nvme_dev *ndev = pci_get_drvdata(pdev);
> 
> +	if (!pm_suspend_via_firmware())
> +		return nvme_set_power(&ndev->ctrl, ndev->ctrl.npss);
>  	nvme_dev_disable(ndev, true);
>  	return 0;
>  }
> @@ -2860,6 +2863,8 @@ static int nvme_resume(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct nvme_dev *ndev = pci_get_drvdata(pdev);
> 
> +	if (!pm_suspend_via_firmware())
> +		return nvme_set_power(&ndev->ctrl, 0);
>  	nvme_reset_ctrl(&ndev->ctrl);
>  	return 0;
>  }
> --