[PATCH RFC 1/3] lightnvm: check mm before use

[PATCH RFC 1/3] lightnvm: check mm before use

[Matias Bjørling]

The core can initialize I/Os before a media manager is registered with
the lightnvm subsystem. Make sure that we don't call the media manager
prematurely.

Signed-off-by: Matias Bjørling <m@bjorling.me>
---
 drivers/nvme/host/lightnvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 06c3364..762c9a7 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -455,7 +455,7 @@ static void nvme_nvm_end_io(struct request *rq, int error)
 	struct nvm_rq *rqd = rq->end_io_data;
 	struct nvm_dev *dev = rqd->dev;
 
-	if (dev->mt->end_io(rqd, error))
+	if (dev->mt && dev->mt->end_io(rqd, error))
 		pr_err("nvme: err status: %x result: %lx\n",
 				rq->errors, (unsigned long)rq->special);
 
-- 
2.1.4

[PATCH RFC 2/3] lightnvm: comments on constants

[Matias Bjørling]

It is not obvious what NVM_IO_* and NVM_BLK_T_* is used for. Make sure
to comment them appropriately as the other constants.

Signed-off-by: Matias Bjørling <m@bjorling.me>
---
 include/linux/lightnvm.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index c6916ae..935ef38 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -50,9 +50,16 @@ enum {
 	NVM_IO_DUAL_ACCESS	= 0x1,
 	NVM_IO_QUAD_ACCESS	= 0x2,
 
+	/* NAND Access Modes */
 	NVM_IO_SUSPEND		= 0x80,
 	NVM_IO_SLC_MODE		= 0x100,
 	NVM_IO_SCRAMBLE_DISABLE	= 0x200,
+
+	/* Block Types */
+	NVM_BLK_T_FREE		= 0x0,
+	NVM_BLK_T_BAD		= 0x1,
+	NVM_BLK_T_DEV		= 0x2,
+	NVM_BLK_T_HOST		= 0x4,
 };
 
 struct nvm_id_group {
-- 
2.1.4

[PATCH RFC 3/3] lightnvm: fix media mgr registration

[Matias Bjørling]

The ppa pool can be used at media manager registration.
Therefore the ppa pool should be allocated before registering.

If a media manager can't be found, this should not lead to the
device being unallocated. A media manager can be registered later, that
can manage a device. Only warn if a media manager fails initialization.

Additionally, protect against the media managed being registered or
deregistered while looping through available media managers. This was
reported by Wenwei Tao.

Signed-off-by: Matias Bjørling <m@bjorling.me>
---
 drivers/lightnvm/core.c | 79 ++++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 40 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 4601cf1..f4d5291 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -97,15 +97,47 @@ static struct nvmm_type *nvm_find_mgr_type(const char *name)
 	return NULL;
 }
 
+struct nvmm_type *nvm_init_mgr(struct nvm_dev *dev)
+{
+	struct nvmm_type *mt;
+	int ret;
+
+	lockdep_assert_held(&nvm_lock);
+
+	list_for_each_entry(mt, &nvm_mgrs, list) {
+		ret = mt->register_mgr(dev);
+		if (ret < 0) {
+			pr_err("nvm: media mgr failed to init (%d) on dev %s\n",
+								ret, dev->name);
+			return NULL; /* initialization failed */
+		} else if (ret > 0)
+			return mt;
+	}
+
+	return NULL;
+}
+
 int nvm_register_mgr(struct nvmm_type *mt)
 {
+	struct nvm_dev *dev;
 	int ret = 0;
 
 	down_write(&nvm_lock);
-	if (nvm_find_mgr_type(mt->name))
+	if (nvm_find_mgr_type(mt->name)) {
 		ret = -EEXIST;
-	else
+		goto finish;
+	} else {
 		list_add(&mt->list, &nvm_mgrs);
+	}
+
+	/* try to register media mgr if any device have none configured */
+	list_for_each_entry(dev, &nvm_devices, devices) {
+		if (dev->mt)
+			continue;
+
+		dev->mt = nvm_init_mgr(dev);
+	}
+finish:
 	up_write(&nvm_lock);
 
 	return ret;
@@ -221,7 +253,6 @@ static void nvm_free(struct nvm_dev *dev)
 
 static int nvm_init(struct nvm_dev *dev)
 {
-	struct nvmm_type *mt;
 	int ret = -EINVAL;
 
 	if (!dev->q || !dev->ops)
@@ -252,22 +283,6 @@ static int nvm_init(struct nvm_dev *dev)
 		goto err;
 	}
 
-	/* register with device with a supported manager */
-	list_for_each_entry(mt, &nvm_mgrs, list) {
-		ret = mt->register_mgr(dev);
-		if (ret < 0)
-			goto err; /* initialization failed */
-		if (ret > 0) {
-			dev->mt = mt;
-			break; /* successfully initialized */
-		}
-	}
-
-	if (!ret) {
-		pr_info("nvm: no compatible manager found.\n");
-		return 0;
-	}
-
 	pr_info("nvm: registered %s [%u/%u/%u/%u/%u/%u]\n",
 			dev->name, dev->sec_per_pg, dev->nr_planes,
 			dev->pgs_per_blk, dev->blks_per_lun, dev->nr_luns,
@@ -324,7 +339,9 @@ int nvm_register(struct request_queue *q, char *disk_name,
 		}
 	}
 
+	/* register device with a supported media manager */
 	down_write(&nvm_lock);
+	dev->mt = nvm_init_mgr(dev);
 	list_add(&dev->devices, &nvm_devices);
 	up_write(&nvm_lock);
 
@@ -365,35 +382,17 @@ static int nvm_create_target(struct nvm_dev *dev,
 {
 	struct nvm_ioctl_create_simple *s = &create->conf.s;
 	struct request_queue *tqueue;
-	struct nvmm_type *mt;
 	struct gendisk *tdisk;
 	struct nvm_tgt_type *tt;
 	struct nvm_target *t;
 	void *targetdata;
-	int ret = 0;
 
-	down_write(&nvm_lock);
 	if (!dev->mt) {
-		/* register with device with a supported NVM manager */
-		list_for_each_entry(mt, &nvm_mgrs, list) {
-			ret = mt->register_mgr(dev);
-			if (ret < 0) {
-				up_write(&nvm_lock);
-				return ret; /* initialization failed */
-			}
-			if (ret > 0) {
-				dev->mt = mt;
-				break; /* successfully initialized */
-			}
-		}
-
-		if (!ret) {
-			up_write(&nvm_lock);
-			pr_info("nvm: no compatible nvm manager found.\n");
-			return -ENODEV;
-		}
+		pr_info("nvm: device has no media manager registered.\n");
+		return -ENODEV;
 	}
 
+	down_write(&nvm_lock);
 	tt = nvm_find_target_type(create->tgttype);
 	if (!tt) {
 		pr_err("nvm: target type %s not found\n", create->tgttype);
-- 
2.1.4

Re: [PATCH RFC 1/3] lightnvm: check mm before use

[Wenwei Tao]

2015-12-02 20:16 GMT+08:00 Matias Bjørling <m@bjorling.me>:
> The core can initialize I/Os before a media manager is registered with
> the lightnvm subsystem. Make sure that we don't call the media manager
> prematurely.
>
> Signed-off-by: Matias Bjørling <m@bjorling.me>
> ---
>  drivers/nvme/host/lightnvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
> index 06c3364..762c9a7 100644
> --- a/drivers/nvme/host/lightnvm.c
> +++ b/drivers/nvme/host/lightnvm.c
> @@ -455,7 +455,7 @@ static void nvme_nvm_end_io(struct request *rq, int error)
>         struct nvm_rq *rqd = rq->end_io_data;
>         struct nvm_dev *dev = rqd->dev;
>
> -       if (dev->mt->end_io(rqd, error))
> +       if (dev->mt && dev->mt->end_io(rqd, error))

Is there any chance core can initialize IOs on device without register
with a media manger ?
In my understanding core cannot create target on device without a
media manger, if core doesn't have a target how can core initialize
IOs on these devices?  If I'm wrong about this, please correct me.

Re: [PATCH RFC 3/3] lightnvm: fix media mgr registration

[Wenwei Tao]

2015-12-02 20:16 GMT+08:00 Matias Bjørling <m@bjorling.me>:
> The ppa pool can be used at media manager registration.
> Therefore the ppa pool should be allocated before registering.
>
> If a media manager can't be found, this should not lead to the
> device being unallocated. A media manager can be registered later, that
> can manage a device. Only warn if a media manager fails initialization.
>
> Additionally, protect against the media managed being registered or
> deregistered while looping through available media managers. This was
> reported by Wenwei Tao.
>
> Signed-off-by: Matias Bjørling <m@bjorling.me>
> ---
>  drivers/lightnvm/core.c | 79 ++++++++++++++++++++++++-------------------------
>  1 file changed, 39 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 4601cf1..f4d5291 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -97,15 +97,47 @@ static struct nvmm_type *nvm_find_mgr_type(const char *name)
>         return NULL;
>  }
>
> +struct nvmm_type *nvm_init_mgr(struct nvm_dev *dev)
> +{
> +       struct nvmm_type *mt;
> +       int ret;
> +
> +       lockdep_assert_held(&nvm_lock);
> +
> +       list_for_each_entry(mt, &nvm_mgrs, list) {
> +               ret = mt->register_mgr(dev);
> +               if (ret < 0) {
> +                       pr_err("nvm: media mgr failed to init (%d) on dev %s\n",
> +                                                               ret, dev->name);
> +                       return NULL; /* initialization failed */

Do we just return or continue to try next media manager ?  In my commit

commit d0a712ceb83ebaea32d520825ee7b997f59b168f

Author: Wenwei Tao <ww.tao0320@gmail.com>

Date:   Sat Nov 28 16:49:28 2015 +0100

    lightnvm: missing nvm_lock acquire

I use the second solution.


> +               } else if (ret > 0)
> +                       return mt;
> +       }
> +
> +       return NULL;
> +}
> +

Re: [PATCH RFC 1/3] lightnvm: check mm before use

[Matias Bjørling]

> Is there any chance core can initialize IOs on device without register
> with a media manger ?
> In my understanding core cannot create target on device without a
> media manger, if core doesn't have a target how can core initialize
> IOs on these devices?  If I'm wrong about this, please correct me.
>

You're right. This is preparation patches for system blocks. The core 
gets features added to work with the device, before gennvm is 
initialized. For that case, no media manager will have been initialized, 
and we therefore can't call it.

Re: [PATCH RFC 3/3] lightnvm: fix media mgr registration

[Matias Bjørling]

> > +struct nvmm_type *nvm_init_mgr(struct nvm_dev *dev)
>> +{
>> +       struct nvmm_type *mt;
>> +       int ret;
>> +
>> +       lockdep_assert_held(&nvm_lock);
>> +
>> +       list_for_each_entry(mt, &nvm_mgrs, list) {
>> +               ret = mt->register_mgr(dev);
>> +               if (ret < 0) {
>> +                       pr_err("nvm: media mgr failed to init (%d) on dev %s\n",
>> +                                                               ret, dev->name);
>> +                       return NULL; /* initialization failed */
>
> Do we just return or continue to try next media manager ?  In my commit
>
> commit d0a712ceb83ebaea32d520825ee7b997f59b168f
>
> Author: Wenwei Tao <ww.tao0320@gmail.com>
>
> Date:   Sat Nov 28 16:49:28 2015 +0100
>
>      lightnvm: missing nvm_lock acquire
>
> I use the second solution.
>

We just return. With the system blocks, there might now have been 
registered a media manager, and that is why it should just give up if 
nothing is found. On media manager module init, it will then attach 
appropriately.