NVMe: Add nvme subsystem reset support

PATCH RESEND New Keith Busch 4 participants 7 messages Aug 10, 2015 – Aug 18, 2015

Subsystems (2)

Block layer NVMe

lore

Share & tools

lore

Keith Busch

author

2015-08-10 21:20 UTC #0 lore raw

PATCH RESEND 1/2 NVMe: Add nvme subsystem reset support

Controllers part of an NVMe subsystem may be reset by any other controller in the subsystem. If the device is capable of subsystem resets, this patch adds detection for such events and performs appropriate controller initialization upon subsystem reset detection.

The register bit is a RW1C type, so the driver needs to write a 1 to the status bit to clear the subsystem reset occured bit during initialization.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/block/nvme-core.c |   11 ++++++++++-
 include/linux/nvme.h      |    3 +++
 2 files changed, 13 insertions(+), 1 deletion(-)

drivers/block/nvme-core.c +10 -1

index 7e9dd11..4e19c8a 100644
@@ -1696,6 +1696,12 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 		page_shift = dev_page_max;
 	}
 
+	dev->subsystem = readl(&dev->bar->vs) >= NVME_VS(1, 1) ?
+						NVME_CAP_NSSRC(cap) : 0;
+
+	if (dev->subsystem && (readl(&dev->bar->csts) & NVME_CSTS_NSSRO))
+		writel(NVME_CSTS_NSSRO, &dev->bar->csts);
+
 	result = nvme_disable_ctrl(dev, cap);
 	if (result < 0)
 		return result;

@@ -2020,7 +2026,10 @@ static int nvme_kthread(void *data)
 		spin_lock(&dev_list_lock);
 		list_for_each_entry_safe(dev, next, &dev_list, node) {
 			int i;
-			if (readl(&dev->bar->csts) & NVME_CSTS_CFS) {
+			u32 csts = readl(&dev->bar->csts);
+
+			if ((dev->subsystem && (csts & NVME_CSTS_NSSRO)) ||
+							csts & NVME_CSTS_CFS) {
 				if (work_busy(&dev->reset_work))
 					continue;
 				list_del_init(&dev->node);

include/linux/nvme.h +3 -0

index c0d94ed..a566b41 100644
@@ -37,6 +37,7 @@ struct nvme_bar {
 #define NVME_CAP_MQES(cap)	((cap) & 0xffff)
 #define NVME_CAP_TIMEOUT(cap)	(((cap) >> 24) & 0xff)
 #define NVME_CAP_STRIDE(cap)	(((cap) >> 32) & 0xf)
+#define NVME_CAP_NSSRC(cap)	(((cap) >> 36) & 0x1)
 #define NVME_CAP_MPSMIN(cap)	(((cap) >> 48) & 0xf)
 #define NVME_CAP_MPSMAX(cap)	(((cap) >> 52) & 0xf)

@@ -55,6 +56,7 @@ enum {
 	NVME_CC_IOCQES		= 4 << 20,
 	NVME_CSTS_RDY		= 1 << 0,
 	NVME_CSTS_CFS		= 1 << 1,
+	NVME_CSTS_NSSRO		= 1 << 4,
 	NVME_CSTS_SHST_NORMAL	= 0 << 2,
 	NVME_CSTS_SHST_OCCUR	= 1 << 2,
 	NVME_CSTS_SHST_CMPLT	= 2 << 2,

@@ -97,6 +99,7 @@ struct nvme_dev {
 	char serial[20];
 	char model[40];
 	char firmware_rev[8];
+	bool subsystem;
 	u32 max_hw_sectors;
 	u32 stripe_size;
 	u32 page_size;

Signature · 1 line

1.7.10.4

Keith Busch

author

2015-08-10 21:20 UTC #1 lore raw

PATCH RESEND 2/2 NVMe: Add nvme subsystem reset IOCTL

From: Jon Derrick <[email protected]>

Controllers can perform optional subsystem resets as introduced in NVMe 1.1. This patch adds an IOCTL to trigger the subsystem reset by writing "NVMe" to the NSSR register.

Signed-off-by Jon Derrick <jonathan.derrick at intel.com>

Acked-by Keith Busch <keith.busch at intel.com>

---
 drivers/block/nvme-core.c |   11 +++++++++++
 include/linux/nvme.h      |    2 +-
 include/uapi/linux/nvme.h |    1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

drivers/block/nvme-core.c +11 -0

index 4e19c8a..613c1bc 100644
@@ -1862,6 +1862,15 @@ static int nvme_user_cmd(struct nvme_dev *dev, struct nvme_ns *ns,
 	return status;
 }
 
+static int nvme_subsys_reset(struct nvme_dev *dev)
+{
+	if (!dev->subsystem)
+		return -ENOTTY;
+
+	writel(0x4E564D65, &dev->bar->nssr); /* "NVMe" */
+	return 0;
+}
+
 static int nvme_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
 							unsigned long arg)
 {

@@ -2829,6 +2838,8 @@ static long nvme_dev_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
 	case NVME_IOCTL_RESET:
 		dev_warn(dev->dev, "resetting controller\n");
 		return nvme_reset(dev);
+	case NVME_IOCTL_SUBSYS_RESET:
+		return nvme_subsys_reset(dev);
 	default:
 		return -ENOTTY;
 	}

include/linux/nvme.h +1 -1

index a566b41..4d08354 100644
@@ -28,7 +28,7 @@ struct nvme_bar {
 	__u32			cc;	/* Controller Configuration */
 	__u32			rsvd1;	/* Reserved */
 	__u32			csts;	/* Controller Status */
-	__u32			rsvd2;	/* Reserved */
+	__u32			nssr;	/* Subsystem Reset */
 	__u32			aqa;	/* Admin Queue Attributes */
 	__u64			asq;	/* Admin SQ Base Address */
 	__u64			acq;	/* Admin CQ Base Address */

include/uapi/linux/nvme.h +1 -0

index 732b32e..8864194 100644
@@ -584,5 +584,6 @@ struct nvme_passthru_cmd {
 #define NVME_IOCTL_SUBMIT_IO	_IOW('N', 0x42, struct nvme_user_io)
 #define NVME_IOCTL_IO_CMD	_IOWR('N', 0x43, struct nvme_passthru_cmd)
 #define NVME_IOCTL_RESET	_IO('N', 0x44)
+#define NVME_IOCTL_SUBSYS_RESET	_IO('N', 0x45)
 
 #endif /* _UAPI_LINUX_NVME_H */

Signature · 1 line

1.7.10.4

J Freyensee

2015-08-11 16:11 UTC #2 lore raw

PATCH RESEND 2/2 NVMe: Add nvme subsystem reset IOCTL

Quoted 7 lines · Keith Busch

From: Jon Derrick <jonathan.derrick at intel.com>
Controllers can perform optional subsystem resets as introduced in NVMe
1.1. This patch adds an IOCTL to trigger the subsystem reset by writing
"NVMe" to the NSSR register.

I just took a Linux Foundation training class and one of the points the Foundation made was the Linux community wanting to move off of IOCTLs to other userspace-to-kernelspace mechanisms (like sysfs for example).

I was wondering if it's a good thing to be continuing to add IOCTLs if it is true that the Linux community would like to move off the reliance of IOCTLs?

Quoted 68 lines

Signed-off-by Jon Derrick <jonathan.derrick at intel.com>

Acked-by Keith Busch <keith.busch at intel.com>

---
 drivers/block/nvme-core.c |   11 +++++++++++
 include/linux/nvme.h      |    2 +-
 include/uapi/linux/nvme.h |    1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

drivers/block/nvme-core.c +9 -0

index 4e19c8a..613c1bc 100644
@@ -1862,6 +1862,15 @@ static int nvme_user_cmd(struct nvme_dev *dev, 
 struct nvme_ns *ns,
 	return status;
 }
 
+static int nvme_subsys_reset(struct nvme_dev *dev)
+{
+	if (!dev->subsystem)
+		return -ENOTTY;
+
+	writel(0x4E564D65, &dev->bar->nssr); /* "NVMe" */
+	return 0;
+}
+
 static int nvme_ioctl(struct block_device *bdev, fmode_t mode, 
 unsigned int cmd,

unsigned
long arg)
{

drivers/block/nvme-core.c +2 -0

@@ -2829,6 +2838,8 @@ static long nvme_dev_ioctl(struct file *f, 
 unsigned int cmd, unsigned long arg)
 	case NVME_IOCTL_RESET:
 		dev_warn(dev->dev, "resetting controller\n");
 		return nvme_reset(dev);
+	case NVME_IOCTL_SUBSYS_RESET:
+		return nvme_subsys_reset(dev);
 	default:
 		return -ENOTTY;

drivers/block/nvme-core.c +0 -0

include/linux/nvme.h +1 -1

index a566b41..4d08354 100644
@@ -28,7 +28,7 @@ struct nvme_bar {
 	__u32			cc;	/* Controller Configuration 
 */
 	__u32			rsvd1;	/* Reserved */
 	__u32			csts;	/* Controller Status */
-	__u32			rsvd2;	/* Reserved */
+	__u32			nssr;	/* Subsystem Reset */
 	__u32			aqa;	/* Admin Queue Attributes */
 	__u64			asq;	/* Admin SQ Base Address */

include/linux/nvme.h +0 -0

 	__u64			acq;	/* Admin CQ Base Address */

include/uapi/linux/nvme.h +1 -0

index 732b32e..8864194 100644
@@ -584,5 +584,6 @@ struct nvme_passthru_cmd {
 #define NVME_IOCTL_SUBMIT_IO	_IOW('N', 0x42, struct nvme_user_io)
 #define NVME_IOCTL_IO_CMD	_IOWR('N', 0x43, struct 
 nvme_passthru_cmd)
 #define NVME_IOCTL_RESET	_IO('N', 0x44)
+#define NVME_IOCTL_SUBSYS_RESET	_IO('N', 0x45)

#endif /* _UAPI_LINUX_NVME_H */

Keith Busch

author

2015-08-11 17:21 UTC #3 lore raw

PATCH RESEND 2/2 NVMe: Add nvme subsystem reset IOCTL

Quoted 7 lines · J Freyensee

Quoted 5 lines · Keith Busch
From: Jon Derrick <jonathan.derrick at intel.com>
Controllers can perform optional subsystem resets as introduced in NVMe 1.1. This patch adds an IOCTL to trigger the subsystem reset by writing "NVMe" to the NSSR register.

Quoted 7 lines

I just took a Linux Foundation training class and one of the points the Foundation made was the Linux community wanting to move off of IOCTLs to other userspace-to-kernelspace mechanisms (like sysfs for example).
I was wondering if it's a good thing to be continuing to add IOCTLs if it is true that the Linux community would like to move off the reliance of IOCTLs?

Maybe some developers have stronger opinions on adding ioctls. I don't see a big problem with it. The proposed ioctl doesn't transfer data with userspace, so we're not committing to maintain a new ABI.

We have sysfs and IOCTL for NVMe controller reset, so maybe we should have both for subsystem resets too. I like sysfs for shell scripting, but ioctl seems easier/cleaner to use within applications. I can add a sysfs entry if this series gets applied.

Christoph Hellwig

2015-08-11 18:13 UTC #4 lore raw

PATCH RESEND 2/2 NVMe: Add nvme subsystem reset IOCTL

In reply to J Freyensee

Quoted 3 lines · J Freyensee

I just took a Linux Foundation training class and one of the points the Foundation made was the Linux community wanting to move off of IOCTLs to other userspace-to-kernelspace mechanisms (like sysfs for example).

That's incorrect, and I don't think Linux Foundation trainers should claim to speak for the community. If that's their standard of training I'd ask for a refund.

J Freyensee

2015-08-11 21:23 UTC #5 lore raw

PATCH RESEND 2/2 NVMe: Add nvme subsystem reset IOCTL

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines · J Freyensee
I just took a Linux Foundation training class and one of the points the
Foundation made was the Linux community wanting to move off of IOCTLs
to other userspace-to-kernelspace mechanisms (like sysfs for example).

Quoted 4 lines

That's incorrect, and I don't think Linux Foundation trainers should claim to speak for the community. If that's their standard of training
I'd ask for a refund.

Well, from old email threads (like this one:
http://yarchive.net/comp/linux/ioctl.html), it appears Linus had a preference of not using ioctl's, so maybe the Foundation bases its info off of those old emails. But I understand, it totally depends on the implementation solution.

Jens Axboe

maintainer

2015-08-18 17:56 UTC #6 lore raw

PATCH RESEND 1/2 NVMe: Add nvme subsystem reset support

In reply to Keith Busch

Quoted 7 lines · Keith Busch

Controllers part of an NVMe subsystem may be reset by any other controller in the subsystem. If the device is capable of subsystem resets, this patch adds detection for such events and performs appropriate controller initialization upon subsystem reset detection.
The register bit is a RW1C type, so the driver needs to write a 1 to the status bit to clear the subsystem reset occured bit during initialization.

Applied 1+2.

Signature · 1 line

Jens Axboe