[PATCH 0/4] nvme: split pci module out of core module

[PATCH 0/4] nvme: split pci module out of core module

[Ming Lin]

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

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

 drivers/nvme/host/Kconfig  | 10 +++++++---
 drivers/nvme/host/Makefile | 10 ++++++----
 drivers/nvme/host/core.c   | 40 +++++++++++++++++++++++++++++++++++++---
 drivers/nvme/host/nvme.h   |  2 --
 drivers/nvme/host/pci.c    | 26 ++------------------------
 5 files changed, 52 insertions(+), 36 deletions(-)

-- 
1.9.1

[PATCH 1/4] nvme: move timeout variables to core.c and export it

[Ming Lin]

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(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c5bf001..3faed20 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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);
 
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 72ef832..deba7ac 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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);
 
-- 
1.9.1

[PATCH 1/4] nvme: move timeout variables to core.c and export it

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 1/4] nvme: move timeout variables to core.c and export it

[Johannes Thumshirn]

On Mon, Feb 08, 2016@02:24:41PM -0800, Ming Lin wrote:
> Signed-off-by: Ming Lin <ming.l at ssi.samsung.com>
> ---

I'd really like to have one or two sentences more in the log but anyways

Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

>  drivers/nvme/host/core.c | 14 ++++++++++++++
>  drivers/nvme/host/pci.c  | 12 ------------
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index c5bf001..3faed20 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -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);
>  
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 72ef832..deba7ac 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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);
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

-- 
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

[PATCH 1/4] nvme: move timeout variables to core.c and export it

[Matias Bjørling]

On 02/08/2016 11:24 PM, Ming Lin wrote:
> 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(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index c5bf001..3faed20 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -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);


Can these be exported without _GPL? There might be a couple of drivers
out of tree which would like to access these variables.

[PATCH 2/4] nvme: export more symbols

[Ming Lin]

Signed-off-by: Ming Lin <ming.l at ssi.samsung.com>
---
 drivers/nvme/host/core.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3faed20..8e4b8ac 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -102,6 +102,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)
@@ -125,6 +126,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;
@@ -162,6 +164,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,
@@ -377,6 +380,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)
 {
@@ -783,6 +787,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)
 {
@@ -814,6 +819,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)
 {
@@ -844,6 +850,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
@@ -905,6 +912,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)
 {
@@ -1310,6 +1318,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)
 {
@@ -1320,6 +1329,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);
 
@@ -1351,13 +1361,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)
 {
@@ -1373,6 +1384,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
@@ -1416,6 +1428,7 @@ out_release_instance:
 out:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(nvme_init_ctrl);
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl)
 {
@@ -1432,6 +1445,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)
 {
@@ -1445,6 +1459,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)
 {
-- 
1.9.1

[PATCH 2/4] nvme: export more symbols

[Christoph Hellwig]

These are the first symbols exports from the nvme module :)

Also a little explanation on why might be useful, a single sentence
should be enough.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 2/4] nvme: export more symbols

[Johannes Thumshirn]

On Mon, Feb 08, 2016@02:24:42PM -0800, Ming Lin wrote:
> Signed-off-by: Ming Lin <ming.l at ssi.samsung.com>
> ---

Like Christoph already said, please be a bit more verbose with your commit
messages.


Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
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

[PATCH 2/4] nvme: export more symbols

[Matias Bjørling]

On 02/08/2016 11:24 PM, Ming Lin wrote:
> Signed-off-by: Ming Lin <ming.l at ssi.samsung.com>
> ---
>  drivers/nvme/host/core.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 3faed20..8e4b8ac 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -102,6 +102,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)
> @@ -125,6 +126,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;
> @@ -162,6 +164,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,
> @@ -377,6 +380,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)
>  {
> @@ -783,6 +787,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)
>  {
> @@ -814,6 +819,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)
>  {
> @@ -844,6 +850,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
> @@ -905,6 +912,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)
>  {
> @@ -1310,6 +1318,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)
>  {
> @@ -1320,6 +1329,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);
>  
> @@ -1351,13 +1361,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)
>  {
> @@ -1373,6 +1384,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
> @@ -1416,6 +1428,7 @@ out_release_instance:
>  out:
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(nvme_init_ctrl);
>  
>  void nvme_stop_queues(struct nvme_ctrl *ctrl)
>  {
> @@ -1432,6 +1445,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)
>  {
> @@ -1445,6 +1459,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)
>  {
> 

Hi Ming,

Can these be exported without _GPL?

[PATCH 2/4] nvme: export more symbols

[Christoph Hellwig]

> Can these be exported without _GPL?

No way.  I consider every Linux module a derivative work, but anything
poking into the NVMe internals surely is much as it can.  We better
keep this clear warning sign.

[PATCH 3/4] nvme: split dev_list_lock

[Ming Lin]

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

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(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8e4b8ac..3928366 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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;
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 4fb5bb7..3cea0ed 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -265,8 +265,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);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index deba7ac..9797595 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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;
-- 
1.9.1

[PATCH 3/4] nvme: split dev_list_lock

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 3/4] nvme: split dev_list_lock

[Johannes Thumshirn]

On Mon, Feb 08, 2016@02:24:43PM -0800, Ming Lin wrote:
> Split dev_list_lock into one in the core and one in the PCI driver.

The dev_list_lock in core.c doesn't really protect the dev_list, like it does
in pci.c. Wouldn't it be better to rename it to something more appropriate? I
just can't come up with a name, as it seems to be some kind of catch all lock.

Otherwise

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(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 8e4b8ac..3928366 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -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;
>  
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 4fb5bb7..3cea0ed 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -265,8 +265,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);
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index deba7ac..9797595 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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;
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

-- 
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

[PATCH 3/4] nvme: split dev_list_lock

[Christoph Hellwig]

On Tue, Feb 09, 2016@01:41:34PM +0100, Johannes Thumshirn wrote:
> On Mon, Feb 08, 2016@02:24:43PM -0800, Ming Lin wrote:
> > Split dev_list_lock into one in the core and one in the PCI driver.
> 
> The dev_list_lock in core.c doesn't really protect the dev_list, like it does
> in pci.c. Wouldn't it be better to rename it to something more appropriate? I
> just can't come up with a name, as it seems to be some kind of catch all lock.

I plans to remove the core.c dev_list_lock by rewriting the character
device interface code.  Let's keep the name for now, it'll go away.

(and the pci.c might be gone even sooner, but at least it has the right name
:))

[PATCH 3/4] nvme: split dev_list_lock

[Johannes Thumshirn]

On Tue, Feb 09, 2016@02:14:08PM +0100, Christoph Hellwig wrote:
> On Tue, Feb 09, 2016@01:41:34PM +0100, Johannes Thumshirn wrote:
> > On Mon, Feb 08, 2016@02:24:43PM -0800, Ming Lin wrote:
> > > Split dev_list_lock into one in the core and one in the PCI driver.
> > 
> > The dev_list_lock in core.c doesn't really protect the dev_list, like it does
> > in pci.c. Wouldn't it be better to rename it to something more appropriate? I
> > just can't come up with a name, as it seems to be some kind of catch all lock.
> 
> I plans to remove the core.c dev_list_lock by rewriting the character
> device interface code.  Let's keep the name for now, it'll go away.
> 
> (and the pci.c might be gone even sooner, but at least it has the right name
> :))

OK, then I'm fine with it.

-- 
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

[PATCH 4/4] nvme: split pci module out of core module

[Ming Lin]

This splits nvme.ko into 2 modules:
nvme-core.ko: the core part
nvme.ko: the PCI driver

Also changes config name:
s/CONFIG_BLK_DEV_NVME/CONFIG_NVME_PCI
s/CONFIG_BLK_DEV_NVME_SCSI/CONFIG_NVME_SCSI

Signed-off-by: Ming Lin <ming.l at ssi.samsung.com>
---
 drivers/nvme/host/Kconfig  | 10 +++++++---
 drivers/nvme/host/Makefile | 10 ++++++----
 drivers/nvme/host/core.c   |  7 ++++++-
 drivers/nvme/host/pci.c    | 13 +------------
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index 5d62373..883dcd6 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -1,6 +1,10 @@
-config BLK_DEV_NVME
+config NVME_CORE
+	tristate
+
+config NVME_PCI
 	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
@@ -9,9 +13,9 @@ config BLK_DEV_NVME
 	  To compile this driver as a module, choose M here: the
 	  module will be called nvme.
 
-config BLK_DEV_NVME_SCSI
+config 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
diff --git a/drivers/nvme/host/Makefile b/drivers/nvme/host/Makefile
index 51bf908..98b057e 100644
--- a/drivers/nvme/host/Makefile
+++ b/drivers/nvme/host/Makefile
@@ -1,6 +1,8 @@
+obj-$(CONFIG_NVME_CORE)		+= nvme-core.o
+obj-$(CONFIG_NVME_PCI)		+= nvme.o
 
-obj-$(CONFIG_BLK_DEV_NVME)     += nvme.o
+nvme-core-y				:= core.o
+nvme-core-$(CONFIG_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
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3928366..eaf9d34 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -485,7 +485,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 		return nvme_user_cmd(ns->ctrl, ns, (void __user *)arg);
 	case NVME_IOCTL_SUBMIT_IO:
 		return nvme_submit_io(ns, (void __user *)arg);
-#ifdef CONFIG_BLK_DEV_NVME_SCSI
+#ifdef CONFIG_NVME_SCSI
 	case SG_GET_VERSION_NUM:
 		return nvme_sg_get_version_num((void __user *)arg);
 	case SG_IO:
@@ -1499,3 +1499,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);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 9797595..4cc32fa 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2220,26 +2220,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();
-- 
1.9.1

[PATCH 4/4] nvme: split pci module out of core module

[Johannes Thumshirn]

On Mon, Feb 08, 2016@02:24:44PM -0800, Ming Lin wrote:
> This splits nvme.ko into 2 modules:
> nvme-core.ko: the core part
> nvme.ko: the PCI driver
> 
> Also changes config name:
> s/CONFIG_BLK_DEV_NVME/CONFIG_NVME_PCI
> s/CONFIG_BLK_DEV_NVME_SCSI/CONFIG_NVME_SCSI

Is there any auto-migration for those who already have CONFIG_BLK_DEV_NVME set
in their kernel configs? If not, this could lead to a bunch of non-booting
systems.

-- 
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

[PATCH 4/4] nvme: split pci module out of core module

[Ming Lin]

On Tue, 2016-02-09@09:56 +0100, Johannes Thumshirn wrote:
> On Mon, Feb 08, 2016@02:24:44PM -0800, Ming Lin wrote:
> > This splits nvme.ko into 2 modules:
> > nvme-core.ko: the core part
> > nvme.ko: the PCI driver
> > 
> > Also changes config name:
> > s/CONFIG_BLK_DEV_NVME/CONFIG_NVME_PCI
> > s/CONFIG_BLK_DEV_NVME_SCSI/CONFIG_NVME_SCSI
> 
> Is there any auto-migration for those who already have CONFIG_BLK_DEV_NVME set
> in their kernel configs? If not, this could lead to a bunch of non-booting
> systems.

Hi Michal(CCed),

As you saw above, I'm changing:
CONFIG_BLK_DEV_NVME to CONFIG_NVME_PCI
and 
CONFIG_BLK_DEV_NVME_SCSI to CONFIG_NVME_SCSI

Is there Kconfig magic to keep existing config working?
ie, if CONFIG_BLK_DEV_NVME was set in .config, then CONFIG_NVME_PCI
should be set automatically?

Thanks,
Ming

[PATCH 4/4] nvme: split pci module out of core module

[Michal Marek]

On 2016-02-09 19:26, Ming Lin wrote:
> On Tue, 2016-02-09@09:56 +0100, Johannes Thumshirn wrote:
>> On Mon, Feb 08, 2016@02:24:44PM -0800, Ming Lin wrote:
>>> This splits nvme.ko into 2 modules:
>>> nvme-core.ko: the core part
>>> nvme.ko: the PCI driver
>>>
>>> Also changes config name:
>>> s/CONFIG_BLK_DEV_NVME/CONFIG_NVME_PCI
>>> s/CONFIG_BLK_DEV_NVME_SCSI/CONFIG_NVME_SCSI
>>
>> Is there any auto-migration for those who already have CONFIG_BLK_DEV_NVME set
>> in their kernel configs? If not, this could lead to a bunch of non-booting
>> systems.
> 
> Hi Michal(CCed),
> 
> As you saw above, I'm changing:
> CONFIG_BLK_DEV_NVME to CONFIG_NVME_PCI
> and 
> CONFIG_BLK_DEV_NVME_SCSI to CONFIG_NVME_SCSI
> 
> Is there Kconfig magic to keep existing config working?

Not if you remove the old option from the Kconfig file. You could keep
the old option and make it select the new one. But then the benefit of
the whole operation becomes questionable.

Michal

[PATCH 4/4] nvme: split pci module out of core module

[Christoph Hellwig]

Looks fine,

but as Johannes poined out we'll probably need some Kconfig magic
to keep existing configfs working.

[PATCH 4/4] nvme: split pci module out of core module

[Ming Lin]

On Tue, Feb 9, 2016@1:26 AM, Christoph Hellwig <hch@lst.de> wrote:
> Looks fine,
>
> but as Johannes poined out we'll probably need some Kconfig magic
> to keep existing configfs working.

I didn't find the magic.
So just keep the old CONFIG name in v2 patches.

[PATCH 4/4] nvme: split pci module out of core module

[Keith Busch]

On Mon, Feb 08, 2016@02:24:44PM -0800, Ming Lin wrote:
> This splits nvme.ko into 2 modules:
> nvme-core.ko: the core part
> nvme.ko: the PCI driver

Do we want to let PCI take the generic "nvme.ko" name? What will the
other nvme transport modules be called?

Otherwise, looks good to me.

[PATCH 4/4] nvme: split pci module out of core module

[Ming Lin]

On Wed, Feb 10, 2016@7:12 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Mon, Feb 08, 2016@02:24:44PM -0800, Ming Lin wrote:
>> This splits nvme.ko into 2 modules:
>> nvme-core.ko: the core part
>> nvme.ko: the PCI driver
>
> Do we want to let PCI take the generic "nvme.ko" name? What will the
> other nvme transport modules be called?

Better name would be: nvme_pci.ko, but that may break, for example,
userspace scripts.

nvme_rdma.ko: IB/RoCE/iWARP
nvme_fc.ko: Fibre Channel
nvme_vhost.ko
....

>
> Otherwise, looks good to me.

[PATCH 0/4] nvme: split pci module out of core module

[Christoph Hellwig]

On Mon, Feb 08, 2016@02:24:40PM -0800, Ming Lin wrote:
> 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

Might be worth explaining why we are doing this:  for the NVMe over
Fabrics drivers that are going to reuse the core, as well as things
like the vhost NVMe driver if we're going to merge it.

We'll also need something to take proper references to the transport
driver on open in this series.  E.g. this patch from Sagi in the Fabrics
tree:

---
>From 1fafa7768fe93bd6a7ec335738bbae9e00f4a634 Mon Sep 17 00:00:00 2001
From: Sagi Grimberg <sagig@mellanox.com>
Date: Tue, 2 Feb 2016 21:52:51 +0100
Subject: nvme/host: reference the fabric module for each bdev open callout

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>
---
 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(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a152b8c..9fe7471 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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)
@@ -517,7 +527,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)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 62209aa..cce40c9 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -119,6 +119,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);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ad24442d..526a30b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2016,6 +2016,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,
-- 
2.1.4