[PATCH v2 0/2] Abrupt Shutdown for NVMe SSD

[PATCH v2 0/2] Abrupt Shutdown for NVMe SSD

[shiva.linuxworks]

From: Shivamurthy Shastri <sshivamurthy@micron.com>

In the platform with a limited backup power supply, devices like NVMe
SSD does unsafe shutdown.

These two patches address this issue by adding a power loss imminent
flag. The platform will enable the power loss imminent flag if the
platform's power is running on the limited backup power supply. During
the shutdown, the driver checks this information and pwerforms the
abrupt shutdown.

Shivamurthy Shastri (2):
  PM: introduce power loss imminent
  nvme: Add abrupt shutdown support

 drivers/nvme/host/core.c |  7 ++++++-
 drivers/nvme/host/pci.c  |  6 ++++--
 include/linux/suspend.h  | 28 +++++++++++++++++++++++++---
 3 files changed, 35 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] PM: enable support for imminent power loss

[shiva.linuxworks]

From: Shivamurthy Shastri <sshivamurthy@micron.com>

If the shutdown is pwerformed when the platform is running on the
limited backup power supply, some of the devices might not have enough
power to perform a clean shutdown.

It is necessary to inform the driver about the limited backup power
supply, to allow the driver to decide to perform the minimal required
operation for a fast and clean shutdown.

Signed-off-by: Keith Busch <kbush@kernel.org>
Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 include/linux/suspend.h | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 8af13ba60c7e..1898792c10d3 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -210,9 +210,10 @@ extern int suspend_valid_only_mem(suspend_state_t state);
 
 extern unsigned int pm_suspend_global_flags;
 
-#define PM_SUSPEND_FLAG_FW_SUSPEND	BIT(0)
-#define PM_SUSPEND_FLAG_FW_RESUME	BIT(1)
-#define PM_SUSPEND_FLAG_NO_PLATFORM	BIT(2)
+#define PM_SUSPEND_FLAG_FW_SUSPEND		BIT(0)
+#define PM_SUSPEND_FLAG_FW_RESUME		BIT(1)
+#define PM_SUSPEND_FLAG_NO_PLATFORM		BIT(2)
+#define PM_SUSPEND_FLAG_POWER_LOSS_IMMINENT	BIT(3)
 
 static inline void pm_suspend_clear_flags(void)
 {
@@ -234,6 +235,11 @@ static inline void pm_set_suspend_no_platform(void)
 	pm_suspend_global_flags |= PM_SUSPEND_FLAG_NO_PLATFORM;
 }
 
+static inline void pm_set_power_loss_imminent(void)
+{
+	pm_suspend_global_flags |= PM_SUSPEND_FLAG_POWER_LOSS_IMMINENT;
+}
+
 /**
  * pm_suspend_via_firmware - Check if platform firmware will suspend the system.
  *
@@ -291,6 +297,22 @@ static inline bool pm_suspend_no_platform(void)
 	return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_NO_PLATFORM);
 }
 
+/**
+ * pm_power_loss_imminent - Check if platform is running on limited backup power
+ * source
+ *
+ * To be called during system-wide power management transitions to sleep states.
+ *
+ * Return 'true' if power loss may be imminent due to platform running on
+ * limited backup supply. If set during a shutdown, drivers should use any
+ * available shortcuts to prepare their device for abrupt power loss.
+ */
+static inline bool pm_power_loss_imminent(void)
+{
+	return !!(pm_suspend_global_flags &
+		  PM_SUSPEND_FLAG_POWER_LOSS_IMMINENT);
+}
+
 /* Suspend-to-idle state machnine. */
 enum s2idle_states {
 	S2IDLE_STATE_NONE,      /* Not suspended/suspending. */
-- 
2.25.1

[PATCH v2 2/2] nvme: Add abrupt shutdown support

[shiva.linuxworks]

From: Shivamurthy Shastri <sshivamurthy@micron.com>

Enabling the abrupt shutdown support. In this shutdown type, the host does
not need to send Delete I/O Submission Queue and Delete I/O Completion
Queue commands to the device.

Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
---
 drivers/nvme/host/core.c | 7 ++++++-
 drivers/nvme/host/pci.c  | 6 ++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 11779be42186..760ffb071c1b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -20,6 +20,7 @@
 #include <linux/ptrace.h>
 #include <linux/nvme_ioctl.h>
 #include <linux/pm_qos.h>
+#include <linux/suspend.h>
 #include <asm/unaligned.h>
 
 #include "nvme.h"
@@ -2159,7 +2160,11 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 	int ret;
 
 	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
-	ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
+
+	if (pm_power_loss_imminent())
+		ctrl->ctrl_config |= NVME_CC_SHN_ABRUPT;
+	else
+		ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
 
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 320051f5a3dd..07be319f63d9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/suspend.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -2524,13 +2525,14 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	 * Give the controller a chance to complete all entered requests if
 	 * doing a safe shutdown.
 	 */
-	if (!dead && shutdown && freeze)
+	if (!dead && shutdown && !pm_power_loss_imminent() && freeze)
 		nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
 
 	nvme_stop_queues(&dev->ctrl);
 
 	if (!dead && dev->ctrl.queue_count > 0) {
-		nvme_disable_io_queues(dev);
+		if (!pm_power_loss_imminent())
+			nvme_disable_io_queues(dev);
 		nvme_disable_admin_queue(dev, shutdown);
 	}
 	nvme_suspend_io_queues(dev);
-- 
2.25.1

Re: [PATCH v2 0/2] Abrupt Shutdown for NVMe SSD

[Keith Busch]

On Mon, Jul 26, 2021 at 01:22:21PM +0000, shiva.linuxworks@gmail.com wrote:
> In the platform with a limited backup power supply, devices like NVMe
> SSD does unsafe shutdown.
> 
> These two patches address this issue by adding a power loss imminent
> flag. The platform will enable the power loss imminent flag if the
> platform's power is running on the limited backup power supply. During
> the shutdown, the driver checks this information and pwerforms the
> abrupt shutdown.

I think the pm framework and nvme usage are ok, but you need a platform
specific caller to set the new power loss flag before this should be
considered, otherwise this is just unreachable code.

Re: [PATCH v2 2/2] nvme: Add abrupt shutdown support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4171 bytes --]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-linux-leds/for-next]
[also build test ERROR on linus/master v5.14-rc3 next-20210723]
[cannot apply to linux-nvme/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/shiva-linuxworks-gmail-com/Abrupt-Shutdown-for-NVMe-SSD/20210726-212459
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: ia64-randconfig-r012-20210726 (attached as .config)
compiler: ia64-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/929817804ad19d2760e156c539dbec82638c35c3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review shiva-linuxworks-gmail-com/Abrupt-Shutdown-for-NVMe-SSD/20210726-212459
        git checkout 929817804ad19d2760e156c539dbec82638c35c3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/nvme/host/pci.c: In function 'nvme_dev_disable':
>> drivers/nvme/host/pci.c:2478:28: error: implicit declaration of function 'pm_power_loss_imminent' [-Werror=implicit-function-declaration]
    2478 |  if (!dead && shutdown && !pm_power_loss_imminent() && freeze)
         |                            ^~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/pm_power_loss_imminent +2478 drivers/nvme/host/pci.c

  2455	
  2456	static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
  2457	{
  2458		bool dead = true, freeze = false;
  2459		struct pci_dev *pdev = to_pci_dev(dev->dev);
  2460	
  2461		mutex_lock(&dev->shutdown_lock);
  2462		if (pci_is_enabled(pdev)) {
  2463			u32 csts = readl(dev->bar + NVME_REG_CSTS);
  2464	
  2465			if (dev->ctrl.state == NVME_CTRL_LIVE ||
  2466			    dev->ctrl.state == NVME_CTRL_RESETTING) {
  2467				freeze = true;
  2468				nvme_start_freeze(&dev->ctrl);
  2469			}
  2470			dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
  2471				pdev->error_state  != pci_channel_io_normal);
  2472		}
  2473	
  2474		/*
  2475		 * Give the controller a chance to complete all entered requests if
  2476		 * doing a safe shutdown.
  2477		 */
> 2478		if (!dead && shutdown && !pm_power_loss_imminent() && freeze)
  2479			nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
  2480	
  2481		nvme_stop_queues(&dev->ctrl);
  2482	
  2483		if (!dead && dev->ctrl.queue_count > 0) {
  2484			if (!pm_power_loss_imminent())
  2485				nvme_disable_io_queues(dev);
  2486			nvme_disable_admin_queue(dev, shutdown);
  2487		}
  2488		nvme_suspend_io_queues(dev);
  2489		nvme_suspend_queue(&dev->queues[0]);
  2490		nvme_pci_disable(dev);
  2491		nvme_reap_pending_cqes(dev);
  2492	
  2493		blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
  2494		blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_request, &dev->ctrl);
  2495		blk_mq_tagset_wait_completed_request(&dev->tagset);
  2496		blk_mq_tagset_wait_completed_request(&dev->admin_tagset);
  2497	
  2498		/*
  2499		 * The driver will not be starting up queues again if shutting down so
  2500		 * must flush all entered requests to their failed completion to avoid
  2501		 * deadlocking blk-mq hot-cpu notifier.
  2502		 */
  2503		if (shutdown) {
  2504			nvme_start_queues(&dev->ctrl);
  2505			if (dev->ctrl.admin_q && !blk_queue_dying(dev->ctrl.admin_q))
  2506				blk_mq_unquiesce_queue(dev->ctrl.admin_q);
  2507		}
  2508		mutex_unlock(&dev->shutdown_lock);
  2509	}
  2510	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26226 bytes --]

Re: [PATCH v2 2/2] nvme: Add abrupt shutdown support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3073 bytes --]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-linux-leds/for-next]
[also build test ERROR on linus/master v5.14-rc3 next-20210723]
[cannot apply to linux-nvme/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/shiva-linuxworks-gmail-com/Abrupt-Shutdown-for-NVMe-SSD/20210726-212459
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: nios2-randconfig-p002-20210726 (attached as .config)
compiler: nios2-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/929817804ad19d2760e156c539dbec82638c35c3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review shiva-linuxworks-gmail-com/Abrupt-Shutdown-for-NVMe-SSD/20210726-212459
        git checkout 929817804ad19d2760e156c539dbec82638c35c3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=nios2 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/nvme/host/core.c: In function 'nvme_shutdown_ctrl':
>> drivers/nvme/host/core.c:2164:6: error: implicit declaration of function 'pm_power_loss_imminent' [-Werror=implicit-function-declaration]
    2164 |  if (pm_power_loss_imminent())
         |      ^~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/pm_power_loss_imminent +2164 drivers/nvme/host/core.c

  2155	
  2156	int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
  2157	{
  2158		unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
  2159		u32 csts;
  2160		int ret;
  2161	
  2162		ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
  2163	
> 2164		if (pm_power_loss_imminent())
  2165			ctrl->ctrl_config |= NVME_CC_SHN_ABRUPT;
  2166		else
  2167			ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
  2168	
  2169		ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
  2170		if (ret)
  2171			return ret;
  2172	
  2173		while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
  2174			if ((csts & NVME_CSTS_SHST_MASK) == NVME_CSTS_SHST_CMPLT)
  2175				break;
  2176	
  2177			msleep(100);
  2178			if (fatal_signal_pending(current))
  2179				return -EINTR;
  2180			if (time_after(jiffies, timeout)) {
  2181				dev_err(ctrl->device,
  2182					"Device shutdown incomplete; abort shutdown\n");
  2183				return -ENODEV;
  2184			}
  2185		}
  2186	
  2187		return ret;
  2188	}
  2189	EXPORT_SYMBOL_GPL(nvme_shutdown_ctrl);
  2190	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26902 bytes --]

Re: [PATCH v2 1/2] PM: enable support for imminent power loss

[Pavel Machek]

Hi!

> If the shutdown is pwerformed when the platform is running on the
> limited backup power supply, some of the devices might not have enough
> power to perform a clean shutdown.
> 
> It is necessary to inform the driver about the limited backup power
> supply, to allow the driver to decide to perform the minimal required
> operation for a fast and clean shutdown.

If you can do shutdown that is fast & clean, why not do it always?

How fast is normal shutdown vs. fast shutdown?

> +#define PM_SUSPEND_FLAG_POWER_LOSS_IMMINENT	BIT(3)

I believe we should be more concrete here. Like explaining use (did
UPS say battery is low? Or does it mean 10 seconds remaining? Or...?)

Plus, who sets this flag? Userland?

Best regards,
								Pavel


-- 

Re: [PATCH v2 1/2] PM: enable support for imminent power loss

[Keith Busch]

On Mon, Jul 26, 2021 at 09:41:46PM +0200, Pavel Machek wrote:
> > If the shutdown is pwerformed when the platform is running on the
> > limited backup power supply, some of the devices might not have enough
> > power to perform a clean shutdown.
> > 
> > It is necessary to inform the driver about the limited backup power
> > supply, to allow the driver to decide to perform the minimal required
> > operation for a fast and clean shutdown.
> 
> If you can do shutdown that is fast & clean, why not do it always?

At least for nvme, I don't think this faster shutdown qualifies as
"clean". It's just a little better than removing power without telling
the device. Typical implementations take longer to become ready on their
next power-on following the abrupt shutdown sequence.

RE: [EXT] Re: [PATCH v2 0/2] Abrupt Shutdown for NVMe SSD

[Shivamurthy Shastri (sshivamurthy)]

Micron Confidential

Hi,

> 
> 
> On Mon, Jul 26, 2021 at 01:22:21PM +0000, shiva.linuxworks@gmail.com wrote:
> > In the platform with a limited backup power supply, devices like NVMe
> > SSD does unsafe shutdown.
> >
> > These two patches address this issue by adding a power loss imminent
> > flag. The platform will enable the power loss imminent flag if the
> > platform's power is running on the limited backup power supply. During
> > the shutdown, the driver checks this information and pwerforms the
> > abrupt shutdown.
> 
> I think the pm framework and nvme usage are ok, but you need a platform
> specific caller to set the new power loss flag before this should be
> considered, otherwise this is just unreachable code.

Sure, I will send platform specific caller along with V3.

Thanks,
Shiva

Micron Confidential