Skip to contentSkip to conversation

split pci module out of core module

PATCH Superseded Ming Lin 4 participants 18 messages Feb 10, 2016 NVMe

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Ming Lin

author
#0 lore raw

v2 cover split pci module out of core module

From: Ming Lin <[email protected]>

We'll have nvme over fabric dirver soon.
This series split nvme pci module out of nvme core module.

nvme.ko is split into 2 modules:
nvme-core.ko: the core part
nvme.ko: the PCI driver

v2:
- add Sagi's patch to take proper references to the transport driver on open
- add review tags
- don't change the CONFIG name

Ming Lin (4):
nvme: move timeout variables to core.c and export it
nvme: export more functions
nvme: split dev_list_lock
nvme: split pci module out of core module

Sagi Grimberg (1):
nvme/host: reference the fabric module for each bdev open callout

 drivers/nvme/host/Kconfig  |  6 ++++-
 drivers/nvme/host/Makefile | 10 ++++----
 drivers/nvme/host/core.c   | 57 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/nvme/host/nvme.h   |  3 +--
 drivers/nvme/host/pci.c    | 27 +++-------------------
 5 files changed, 67 insertions(+), 36 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH v2 1/5 nvme/host: reference the fabric module for each bdev open callout

From: Sagi Grimberg <[email protected]>

We don't want to be able to unload the fabric driver when we have openened referenced to our namespaces. Thus, for each nvme_open we take a reference on the fabric driver and put it in nvme_release. This behavior is consistent with the scsi model.

This resolves the panic when unloading a fabric module with mpath holders.

Signed-off-by Sagi Grimberg <sagig at mellanox.com>
Reviewed-by Christoph Hellwig <hch at lst.de>
Reviewed-by Ian Bakshan <ianb at mellanox.com>
Signed-off-by Ming Lin <ming.l at ssi.samsung.com>
---
 drivers/nvme/host/core.c | 19 ++++++++++++++++---
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/pci.c  |  1 +
 3 files changed, 18 insertions(+), 3 deletions(-)
drivers/nvme/host/core.c +16 -3
index c5bf001..8adc598 100644
@@ -71,11 +71,21 @@ static struct nvme_ns *nvme_get_ns_from_disk(struct gendisk *disk)
 
 	spin_lock(&dev_list_lock);
 	ns = disk->private_data;
-	if (ns && !kref_get_unless_zero(&ns->kref))
-		ns = NULL;
+	if (ns) {
+		if (!kref_get_unless_zero(&ns->kref))
+			goto fail;
+		if (!try_module_get(ns->ctrl->ops->module))
+			goto fail_put_ns;
+	}
 	spin_unlock(&dev_list_lock);
 
 	return ns;
+
+fail_put_ns:
+	kref_put(&ns->kref, nvme_free_ns);
+fail:
+	spin_unlock(&dev_list_lock);
+	return NULL;
 }
 
 void nvme_requeue_req(struct request *req)

@@ -499,7 +509,10 @@ static int nvme_open(struct block_device *bdev, fmode_t mode)
 
 static void nvme_release(struct gendisk *disk, fmode_t mode)
 {
-	nvme_put_ns(disk->private_data);
+	struct nvme_ns *ns = disk->private_data;
+
+	module_put(ns->ctrl->ops->module);
+	nvme_put_ns(ns);
 }
 
 static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
drivers/nvme/host/nvme.h +1 -0
index 4fb5bb7..9f77386 100644
@@ -117,6 +117,7 @@ struct nvme_ns {
 };
 
 struct nvme_ctrl_ops {
+	struct module *module;
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
 	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
drivers/nvme/host/pci.c +1 -0
index 72ef832..e36ca02 100644
@@ -2029,6 +2029,7 @@ static int nvme_pci_reset_ctrl(struct nvme_ctrl *ctrl)
 }
 
 static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
+	.module			= THIS_MODULE,
 	.reg_read32		= nvme_pci_reg_read32,
 	.reg_write32		= nvme_pci_reg_write32,
 	.reg_read64		= nvme_pci_reg_read64,
Signature · 1 line
1.9.1
#2 lore raw

PATCH v2 1/5 nvme/host: reference the fabric module for each bdev open callout

Quoted 7 lines · Ming Lin

From: Sagi Grimberg <sagig at mellanox.com>

We don't want to be able to unload the fabric driver when we have openened referenced to our namespaces. Thus, for each nvme_open we take a reference on the fabric driver and put it in nvme_release. This behavior is consistent with the scsi model.

Quoted 7 lines

This resolves the panic when unloading a fabric module with mpath holders.

Signed-off-by Sagi Grimberg <sagig at mellanox.com>
Reviewed-by Christoph Hellwig <hch at lst.de>
Reviewed-by Ian Bakshan <ianb at mellanox.com>
Signed-off-by Ming Lin <ming.l at ssi.samsung.com>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
author
#3 lore raw

PATCH v2 2/5 nvme: move timeout variables to core.c and export it

In reply to Ming Lin

From: Ming Lin <[email protected]>

These variables are used by PCI driver and will also be used in the forthcoming NVMe over Fabric drivers.

Reviewed-by Christoph Hellwig <hch at lst.de>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Signed-off-by Ming Lin <ming.l at ssi.samsung.com>
---
 drivers/nvme/host/core.c | 14 ++++++++++++++
 drivers/nvme/host/pci.c  | 12 ------------
 2 files changed, 14 insertions(+), 12 deletions(-)
drivers/nvme/host/core.c +14 -0
index 8adc598..165ef54 100644
@@ -33,6 +33,20 @@
 
 #define NVME_MINORS		(1U << MINORBITS)
 
+unsigned char admin_timeout = 60;
+module_param(admin_timeout, byte, 0644);
+MODULE_PARM_DESC(admin_timeout, "timeout in seconds for admin commands");
+EXPORT_SYMBOL_GPL(admin_timeout);
+
+unsigned char nvme_io_timeout = 30;
+module_param_named(io_timeout, nvme_io_timeout, byte, 0644);
+MODULE_PARM_DESC(io_timeout, "timeout in seconds for I/O");
+EXPORT_SYMBOL_GPL(nvme_io_timeout);
+
+unsigned char shutdown_timeout = 5;
+module_param(shutdown_timeout, byte, 0644);
+MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for controller shutdown");
+
 static int nvme_major;
 module_param(nvme_major, int, 0);
drivers/nvme/host/pci.c +0 -12
index e36ca02..e3552d1 100644
@@ -57,18 +57,6 @@
 #define NVME_NR_AEN_COMMANDS	1
 #define NVME_AQ_BLKMQ_DEPTH	(NVME_AQ_DEPTH - NVME_NR_AEN_COMMANDS)
 
-unsigned char admin_timeout = 60;
-module_param(admin_timeout, byte, 0644);
-MODULE_PARM_DESC(admin_timeout, "timeout in seconds for admin commands");
-
-unsigned char nvme_io_timeout = 30;
-module_param_named(io_timeout, nvme_io_timeout, byte, 0644);
-MODULE_PARM_DESC(io_timeout, "timeout in seconds for I/O");
-
-unsigned char shutdown_timeout = 5;
-module_param(shutdown_timeout, byte, 0644);
-MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for controller shutdown");
-
 static int use_threaded_interrupts;
 module_param(use_threaded_interrupts, int, 0);
Signature · 1 line
1.9.1
#4 lore raw

PATCH v2 2/5 nvme: move timeout variables to core.c and export it

Looks good,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
author
#5 lore raw

PATCH v2 3/5 nvme: export more functions

In reply to Ming Lin

From: Ming Lin <[email protected]>

These functions are used by PCI driver and will also be used in the forthcoming NVMe over Fabric drivers.

Reviewed-by Christoph Hellwig <hch at lst.de>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Signed-off-by Ming Lin <ming.l at ssi.samsung.com>
---
 drivers/nvme/host/core.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
drivers/nvme/host/core.c +16 -1
index 165ef54..1bba21f 100644
@@ -112,6 +112,7 @@ void nvme_requeue_req(struct request *req)
 		blk_mq_kick_requeue_list(req->q);
 	spin_unlock_irqrestore(req->q->queue_lock, flags);
 }
+EXPORT_SYMBOL_GPL(nvme_requeue_req);
 
 struct request *nvme_alloc_request(struct request_queue *q,
 		struct nvme_command *cmd, unsigned int flags)

@@ -135,6 +136,7 @@ struct request *nvme_alloc_request(struct request_queue *q,
 
 	return req;
 }
+EXPORT_SYMBOL_GPL(nvme_alloc_request);
 
 /*
  * Returns 0 on success.  If the result is negative, it's a Linux error code;

@@ -172,6 +174,7 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 {
 	return __nvme_submit_sync_cmd(q, cmd, buffer, bufflen, NULL, 0);
 }
+EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);
 
 int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void __user *ubuffer, unsigned bufflen,

@@ -387,6 +390,7 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
 	*count = min(*count, nr_io_queues);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(nvme_set_queue_count);
 
 static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 {

@@ -796,6 +800,7 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
 		return ret;
 	return nvme_wait_ready(ctrl, cap, false);
 }
+EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
 
 int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
 {

@@ -827,6 +832,7 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
 		return ret;
 	return nvme_wait_ready(ctrl, cap, true);
 }
+EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {

@@ -857,6 +863,7 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(nvme_shutdown_ctrl);
 
 /*
  * Initialize the cached copies of the Identify data and various controller

@@ -918,6 +925,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	kfree(id);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(nvme_init_identify);
 
 static int nvme_dev_open(struct inode *inode, struct file *file)
 {

@@ -1323,6 +1331,7 @@ void nvme_scan_namespaces(struct nvme_ctrl *ctrl)
 	mutex_unlock(&ctrl->namespaces_mutex);
 	kfree(id);
 }
+EXPORT_SYMBOL_GPL(nvme_scan_namespaces);
 
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 {

@@ -1333,6 +1342,7 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 		nvme_ns_remove(ns);
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
+EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
 
 static DEFINE_IDA(nvme_instance_ida);
 

@@ -1364,13 +1374,14 @@ static void nvme_release_instance(struct nvme_ctrl *ctrl)
 }
 
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
- {
+{
 	device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));
 
 	spin_lock(&dev_list_lock);
 	list_del(&ctrl->node);
 	spin_unlock(&dev_list_lock);
 }
+EXPORT_SYMBOL_GPL(nvme_uninit_ctrl);
 
 static void nvme_free_ctrl(struct kref *kref)
 {

@@ -1386,6 +1397,7 @@ void nvme_put_ctrl(struct nvme_ctrl *ctrl)
 {
 	kref_put(&ctrl->kref, nvme_free_ctrl);
 }
+EXPORT_SYMBOL_GPL(nvme_put_ctrl);
 
 /*
  * Initialize a NVMe controller structures.  This needs to be called during

@@ -1429,6 +1441,7 @@ out_release_instance:
 out:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(nvme_init_ctrl);
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl)
 {

@@ -1445,6 +1458,7 @@ void nvme_stop_queues(struct nvme_ctrl *ctrl)
 	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
+EXPORT_SYMBOL_GPL(nvme_stop_queues);
 
 void nvme_start_queues(struct nvme_ctrl *ctrl)
 {

@@ -1458,6 +1472,7 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
 	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
+EXPORT_SYMBOL_GPL(nvme_start_queues);
 
 int __init nvme_core_init(void)
 {
Signature · 1 line
1.9.1
#6 lore raw

PATCH v2 3/5 nvme: export more functions

Looks good,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
author
#7 lore raw

PATCH v2 4/5 nvme: split dev_list_lock

In reply to Ming Lin

From: Ming Lin <[email protected]>

Split dev_list_lock into one in the core and one in the PCI driver.

Reviewed-by Christoph Hellwig <hch at lst.de>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Signed-off-by Ming Lin <ming.l at ssi.samsung.com>
---
 drivers/nvme/host/core.c | 2 +-
 drivers/nvme/host/nvme.h | 2 --
 drivers/nvme/host/pci.c  | 1 +
 3 files changed, 2 insertions(+), 3 deletions(-)
drivers/nvme/host/core.c +1 -1
index 1bba21f..740615b 100644
@@ -54,7 +54,7 @@ static int nvme_char_major;
 module_param(nvme_char_major, int, 0);
 
 static LIST_HEAD(nvme_ctrl_list);
-DEFINE_SPINLOCK(dev_list_lock);
+static DEFINE_SPINLOCK(dev_list_lock);
 
 static struct class *nvme_class;
drivers/nvme/host/nvme.h +0 -2
index 9f77386..63ba8a5 100644
@@ -266,8 +266,6 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 			dma_addr_t dma_addr, u32 *result);
 int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count);
 
-extern spinlock_t dev_list_lock;
-
 struct sg_io_hdr;
 
 int nvme_sg_io(struct nvme_ns *ns, struct sg_io_hdr __user *u_hdr);
drivers/nvme/host/pci.c +1 -0
index e3552d1..f7266cc 100644
@@ -65,6 +65,7 @@ module_param(use_cmb_sqes, bool, 0644);
 MODULE_PARM_DESC(use_cmb_sqes, "use controller's memory buffer for I/O SQes");
 
 static LIST_HEAD(dev_list);
+static DEFINE_SPINLOCK(dev_list_lock);
 static struct task_struct *nvme_thread;
 static struct workqueue_struct *nvme_workq;
 static wait_queue_head_t nvme_kthread_wait;
Signature · 1 line
1.9.1
#8 lore raw

PATCH v2 4/5 nvme: split dev_list_lock

Looks good,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
author
#9 lore raw

PATCH v2 5/5 nvme: split pci module out of core module

In reply to Ming Lin

From: Ming Lin <[email protected]>

NVMe over Fabrics drivers are going to reuse the core, so splits nvme.ko into 2 modules:

nvme-core.ko: the core part
nvme.ko: the PCI driver

Signed-off-by Ming Lin <ming.l at ssi.samsung.com>
---
 drivers/nvme/host/Kconfig  |  6 +++++-
 drivers/nvme/host/Makefile | 10 ++++++----
 drivers/nvme/host/core.c   |  5 +++++
 drivers/nvme/host/pci.c    | 13 +------------
 4 files changed, 17 insertions(+), 17 deletions(-)
drivers/nvme/host/Kconfig +5 -1
index 5d62373..2ed30f0 100644
@@ -1,6 +1,10 @@
+config NVME_CORE
+	tristate
+
 config BLK_DEV_NVME
 	tristate "NVM Express block device"
 	depends on PCI && BLOCK
+	select NVME_CORE
 	---help---
 	  The NVM Express driver is for solid state drives directly
 	  connected to the PCI or PCI Express bus.  If you know you

@@ -11,7 +15,7 @@ config BLK_DEV_NVME
 
 config BLK_DEV_NVME_SCSI
 	bool "SCSI emulation for NVMe device nodes"
-	depends on BLK_DEV_NVME
+	depends on NVME_CORE
 	---help---
 	  This adds support for the SG_IO ioctl on the NVMe character
 	  and block devices nodes, as well a a translation for a small
drivers/nvme/host/Makefile +6 -4
index 51bf908..9a3ca89 100644
@@ -1,6 +1,8 @@
+obj-$(CONFIG_NVME_CORE)			+= nvme-core.o
+obj-$(CONFIG_BLK_DEV_NVME)		+= nvme.o
 
-obj-$(CONFIG_BLK_DEV_NVME)     += nvme.o
+nvme-core-y				:= core.o
+nvme-core-$(CONFIG_BLK_DEV_NVME_SCSI)	+= scsi.o
+nvme-core-$(CONFIG_NVM)			+= lightnvm.o
 
-lightnvm-$(CONFIG_NVM)			:= lightnvm.o
-nvme-y					+= core.o pci.o $(lightnvm-y)
-nvme-$(CONFIG_BLK_DEV_NVME_SCSI)        += scsi.o
+nvme-y					+= pci.o
drivers/nvme/host/core.c +5 -0
index 740615b..c7df0ab 100644
@@ -1512,3 +1512,8 @@ void nvme_core_exit(void)
 	class_destroy(nvme_class);
 	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme");
 }
+
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");
+module_init(nvme_core_init);
+module_exit(nvme_core_exit);
drivers/nvme/host/pci.c +1 -12
index f7266cc..a5a4567 100644
@@ -2221,26 +2221,15 @@ static int __init nvme_init(void)
 	if (!nvme_workq)
 		return -ENOMEM;
 
-	result = nvme_core_init();
-	if (result < 0)
-		goto kill_workq;
-
 	result = pci_register_driver(&nvme_driver);
 	if (result)
-		goto core_exit;
-	return 0;
-
- core_exit:
-	nvme_core_exit();
- kill_workq:
-	destroy_workqueue(nvme_workq);
+		destroy_workqueue(nvme_workq);
 	return result;
 }
 
 static void __exit nvme_exit(void)
 {
 	pci_unregister_driver(&nvme_driver);
-	nvme_core_exit();
 	destroy_workqueue(nvme_workq);
 	BUG_ON(nvme_thread && !IS_ERR(nvme_thread));
 	_nvme_check_size();
Signature · 1 line
1.9.1
#10 lore raw

PATCH v2 5/5 nvme: split pci module out of core module

Looks good,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>
maintainer
#12 lore raw

v2 cover split pci module out of core module

In reply to Ming Lin

Patch 3 still says "export more symbols" while it exports the first symbols. Maybe it's better to just gold patch 3 into patch 5 where the exports are use?

Otherwise looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
author
#13 lore raw

v2 cover split pci module out of core module

Quoted 3 lines · Christoph Hellwig

Patch 3 still says "export more symbols" while it exports the first symbols. Maybe it's better to just gold patch 3 into patch 5 where the exports are use?

Patch 2 exports the first symbols.

EXPORT_SYMBOL_GPL(admin_timeout);
EXPORT_SYMBOL_GPL(nvme_io_timeout);

Quoted 4 lines

Otherwise looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
maintainer
#14 lore raw

v2 cover split pci module out of core module

Quoted 9 lines · Ming Lin
Quoted 3 lines · Christoph Hellwig

Patch 3 still says "export more symbols" while it exports the first symbols. Maybe it's better to just gold patch 3 into patch 5 where the exports are use?

Patch 2 exports the first symbols.

EXPORT_SYMBOL_GPL(admin_timeout);
EXPORT_SYMBOL_GPL(nvme_io_timeout);

Ok, let's move all the exports into a single patch then.

author
#15 lore raw

v2 cover split pci module out of core module

Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Ming Lin
Quoted 3 lines · Christoph Hellwig

Patch 3 still says "export more symbols" while it exports the first symbols. Maybe it's better to just gold patch 3 into patch 5 where the exports are use?

Quoted 6 lines
Quoted 4 lines

Patch 2 exports the first symbols.

EXPORT_SYMBOL_GPL(admin_timeout);
EXPORT_SYMBOL_GPL(nvme_io_timeout);

Ok, let's move all the exports into a single patch then.

OK. I'll merge patch 2 & 3.

maintainer
#16 lore raw

v2 cover split pci module out of core module

Quoted 3 lines · Ming Lin
Quoted 1 line

Ok, let's move all the exports into a single patch then.

OK. I'll merge patch 2 & 3.

Can I suggest the following:

- patch 1: as-is
- patch 2: drop the exports, just do the move
- patch 3: fold into patch 5
- patch 4: as-is
- patch 5: all existing changes, plus all exports

author
#17 lore raw

v2 cover split pci module out of core module

Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Ming Lin
Quoted 1 line

Ok, let's move all the exports into a single patch then.

OK. I'll merge patch 2 & 3.

Quoted 7 lines

Can I suggest the following:

- patch 1: as-is
- patch 2: drop the exports, just do the move
- patch 3: fold into patch 5
- patch 4: as-is
- patch 5: all existing changes, plus all exports

OK, will send v3.