From: Shivamurthy Shastri <[email protected]>
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
From: Shivamurthy Shastri <[email protected]>
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 <[email protected]>
---
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
From: Shivamurthy Shastri <[email protected]>
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 <[email protected]>
Signed-off-by: Shivamurthy Shastri <[email protected]>
---
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
On Mon, Jul 26, 2021 at 01:22:21PM +0000, [email protected] 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.
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 <[email protected]>
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/[email protected]
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 <[email protected]>
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/[email protected]
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
--
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.
Micron Confidential
Hi,
>
>
> On Mon, Jul 26, 2021 at 01:22:21PM +0000, [email protected] 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