Hi Jens,
A couple of patches during the last week.
Patch 1: Fix from Keith to compile out lightnvm when it is not
available.
Patch 2-4: Various memory and check fixes from Tao, Sudip and me.
Patch 5: Refactor device detection ids.
Patch 6: Make get_bb_tbl return converted ppa back to media manager.
Patch 7: Missing locks fixes from Tao on module iterations.
Please pick up when convenient.
Keith Busch (1):
lightnvm: Simplify config when disabled
Matias Bjørling (2):
lightnvm: refactor and change vendor id for qemu
lightnvm: unconverted ppa returned in get_bb_tbl
Sudip Mukherjee (1):
lightnvm: fix ioctl memory leaks
Wenwei Tao (3):
lightnvm: free memory when gennvm register fails
lightnvm: do device max sectors boundary check first
lightnvm: missing nvm_lock acquire
drivers/lightnvm/core.c | 94 ++++++++++++++++++++++++++------------------
drivers/lightnvm/gennvm.c | 18 +++++----
drivers/nvme/host/Makefile | 3 +-
drivers/nvme/host/lightnvm.c | 28 ++++++-------
drivers/nvme/host/nvme.h | 14 +++++++
include/linux/lightnvm.h | 2 +-
6 files changed, 95 insertions(+), 64 deletions(-)
--
2.1.4
From: Keith Busch <[email protected]>
We shouldn't compile an object file to get empty implementations;
conforms to linux coding style on conditional compilation.
Signed-off-by: Keith Busch <[email protected]>
Signed-off-by: Matias Bjørling <[email protected]>
---
drivers/nvme/host/Makefile | 3 ++-
drivers/nvme/host/lightnvm.c | 13 -------------
drivers/nvme/host/nvme.h | 14 ++++++++++++++
3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/nvme/host/Makefile b/drivers/nvme/host/Makefile
index 219dc206..a5fe239 100644
--- a/drivers/nvme/host/Makefile
+++ b/drivers/nvme/host/Makefile
@@ -1,4 +1,5 @@
obj-$(CONFIG_BLK_DEV_NVME) += nvme.o
-nvme-y += pci.o scsi.o lightnvm.o
+lightnvm-$(CONFIG_NVM) := lightnvm.o
+nvme-y += pci.o scsi.o $(lightnvm-y)
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 9202d1a..07451d6 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -22,8 +22,6 @@
#include "nvme.h"
-#ifdef CONFIG_NVM
-
#include <linux/nvme.h>
#include <linux/bitops.h>
#include <linux/lightnvm.h>
@@ -588,14 +586,3 @@ int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id)
return 0;
}
-#else
-int nvme_nvm_register(struct request_queue *q, char *disk_name)
-{
- return 0;
-}
-void nvme_nvm_unregister(struct request_queue *q, char *disk_name) {};
-int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id)
-{
- return 0;
-}
-#endif /* CONFIG_NVM */
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index fdb4e5b..044253d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -136,8 +136,22 @@ int nvme_sg_io(struct nvme_ns *ns, struct sg_io_hdr __user *u_hdr);
int nvme_sg_io32(struct nvme_ns *ns, unsigned long arg);
int nvme_sg_get_version_num(int __user *ip);
+#ifdef CONFIG_NVM
int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id);
int nvme_nvm_register(struct request_queue *q, char *disk_name);
void nvme_nvm_unregister(struct request_queue *q, char *disk_name);
+#else
+static inline int nvme_nvm_register(struct request_queue *q, char *disk_name)
+{
+ return 0;
+}
+
+static inline void nvme_nvm_unregister(struct request_queue *q, char *disk_name) {};
+
+static inline int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id)
+{
+ return 0;
+}
+#endif /* CONFIG_NVM */
#endif /* _NVME_H */
--
2.1.4
From: Wenwei Tao <[email protected]>
free allocated nvm block and gennvm lun structures when
gennvm register fails, otherwise it will cause memory leak.
Signed-off-by: Matias Bjørling <[email protected]>
---
drivers/lightnvm/gennvm.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
index e20e74e..3969a98 100644
--- a/drivers/lightnvm/gennvm.c
+++ b/drivers/lightnvm/gennvm.c
@@ -207,6 +207,14 @@ static int gennvm_blocks_init(struct nvm_dev *dev, struct gen_nvm *gn)
return 0;
}
+static void gennvm_free(struct nvm_dev *dev)
+{
+ gennvm_blocks_free(dev);
+ gennvm_luns_free(dev);
+ kfree(dev->mp);
+ dev->mp = NULL;
+}
+
static int gennvm_register(struct nvm_dev *dev)
{
struct gen_nvm *gn;
@@ -234,16 +242,13 @@ static int gennvm_register(struct nvm_dev *dev)
return 1;
err:
- kfree(gn);
+ gennvm_free(dev);
return ret;
}
static void gennvm_unregister(struct nvm_dev *dev)
{
- gennvm_blocks_free(dev);
- gennvm_luns_free(dev);
- kfree(dev->mp);
- dev->mp = NULL;
+ gennvm_free(dev);
}
static struct nvm_block *gennvm_get_blk(struct nvm_dev *dev,
--
2.1.4
From: Sudip Mukherjee <[email protected]>
If copy_to_user() fails we returned error but we missed releasing
devices.
Signed-off-by: Sudip Mukherjee <[email protected]>
Signed-off-by: Matias Bjørling <[email protected]>
---
drivers/lightnvm/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 5178645..ea50fa5 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -680,8 +680,10 @@ static long nvm_ioctl_info(struct file *file, void __user *arg)
info->tgtsize = tgt_iter;
up_write(&nvm_lock);
- if (copy_to_user(arg, info, sizeof(struct nvm_ioctl_info)))
+ if (copy_to_user(arg, info, sizeof(struct nvm_ioctl_info))) {
+ kfree(info);
return -EFAULT;
+ }
kfree(info);
return 0;
@@ -724,8 +726,11 @@ static long nvm_ioctl_get_devices(struct file *file, void __user *arg)
devices->nr_devices = i;
- if (copy_to_user(arg, devices, sizeof(struct nvm_ioctl_get_devices)))
+ if (copy_to_user(arg, devices,
+ sizeof(struct nvm_ioctl_get_devices))) {
+ kfree(devices);
return -EFAULT;
+ }
kfree(devices);
return 0;
--
2.1.4
From: Wenwei Tao <[email protected]>
do device max_phys_sect boundary check first, otherwise
we will allocate dma_pools for devices whose max sectors
are beyond lightnvm support and register them.
Signed-off-by: Wenwei Tao <[email protected]>
Signed-off-by: Matias Bjørling <[email protected]>
---
drivers/lightnvm/core.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index ea50fa5..ea6dba5 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -308,6 +308,12 @@ int nvm_register(struct request_queue *q, char *disk_name,
if (ret)
goto err_init;
+ if (dev->ops->max_phys_sect > 256) {
+ pr_info("nvm: max sectors supported is 256.\n");
+ ret = -EINVAL;
+ goto err_init;
+ }
+
if (dev->ops->max_phys_sect > 1) {
dev->ppalist_pool = dev->ops->create_dma_pool(dev->q,
"ppalist");
@@ -316,10 +322,6 @@ int nvm_register(struct request_queue *q, char *disk_name,
ret = -ENOMEM;
goto err_init;
}
- } else if (dev->ops->max_phys_sect > 256) {
- pr_info("nvm: max sectors supported is 256.\n");
- ret = -EINVAL;
- goto err_init;
}
down_write(&nvm_lock);
--
2.1.4
The QEMU NVMe implementation uses Intel vendor, Intel device id, and the
first vendor specific byte to identify a LightNVM compatible nvme
instance.
Instead of using the Intel specific, use a preallocated from CNEX Labs
instead. This lets us uniquely identify a QEMU lightnvm device without
breaking other vendor specific work in the qemu device driver.
Reported-by: Christoph Hellwig <[email protected]>
Signed-off-by: Matias Bjørling <[email protected]>
---
drivers/nvme/host/lightnvm.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 07451d6..b9e5cc7 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -569,18 +569,25 @@ void nvme_nvm_unregister(struct request_queue *q, char *disk_name)
nvm_unregister(disk_name);
}
+/* move to shared place when used in multiple places. */
+#define PCI_VENDOR_ID_CNEX 0x1d1d
+#define PCI_DEVICE_ID_CNEX_WL 0x2807
+#define PCI_DEVICE_ID_CNEX_QEMU 0x1f1f
+
int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id)
{
struct nvme_dev *dev = ns->dev;
struct pci_dev *pdev = to_pci_dev(dev->dev);
/* QEMU NVMe simulator - PCI ID + Vendor specific bit */
- if (pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device == 0x5845 &&
+ if (pdev->vendor == PCI_VENDOR_ID_CNEX &&
+ pdev->device == PCI_DEVICE_ID_CNEX_QEMU &&
id->vs[0] == 0x1)
return 1;
/* CNEX Labs - PCI ID + Vendor specific bit */
- if (pdev->vendor == 0x1d1d && pdev->device == 0x2807 &&
+ if (pdev->vendor == PCI_VENDOR_ID_CNEX &&
+ pdev->device == PCI_DEVICE_ID_CNEX_WL &&
id->vs[0] == 0x1)
return 1;
--
2.1.4
The get_bb_tbl function takes ppa as a generic address, which is
converted to the ppa device address within the device driver. When
the update_bbtbl callback is called from get_bb_tbl, the device
specific ppa is used, instead of the generic ppa.
Make sure to pass the generic ppa.
Signed-off-by: Matias Bjørling <[email protected]>
---
drivers/lightnvm/gennvm.c | 3 +--
drivers/nvme/host/lightnvm.c | 4 +++-
include/linux/lightnvm.h | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
index 3969a98..35dde84 100644
--- a/drivers/lightnvm/gennvm.c
+++ b/drivers/lightnvm/gennvm.c
@@ -75,7 +75,6 @@ static int gennvm_block_bb(struct ppa_addr ppa, int nr_blocks, u8 *blks,
struct nvm_block *blk;
int i;
- ppa = dev_to_generic_addr(gn->dev, ppa);
lun = &gn->luns[(dev->nr_luns * ppa.g.ch) + ppa.g.lun];
for (i = 0; i < nr_blocks; i++) {
@@ -187,7 +186,7 @@ static int gennvm_blocks_init(struct nvm_dev *dev, struct gen_nvm *gn)
ppa.g.lun = lun->vlun.id;
ppa = generic_to_dev_addr(dev, ppa);
- ret = dev->ops->get_bb_tbl(dev->q, ppa,
+ ret = dev->ops->get_bb_tbl(dev, ppa,
dev->blks_per_lun,
gennvm_block_bb, gn);
if (ret)
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index b9e5cc7..06c3364 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -355,10 +355,11 @@ out:
return ret;
}
-static int nvme_nvm_get_bb_tbl(struct request_queue *q, struct ppa_addr ppa,
+static int nvme_nvm_get_bb_tbl(struct nvm_dev *nvmdev, struct ppa_addr ppa,
int nr_blocks, nvm_bb_update_fn *update_bbtbl,
void *priv)
{
+ struct request_queue *q = nvmdev->q;
struct nvme_ns *ns = q->queuedata;
struct nvme_dev *dev = ns->dev;
struct nvme_nvm_command c = {};
@@ -402,6 +403,7 @@ static int nvme_nvm_get_bb_tbl(struct request_queue *q, struct ppa_addr ppa,
goto out;
}
+ ppa = dev_to_generic_addr(nvmdev, ppa);
ret = update_bbtbl(ppa, nr_blocks, bb_tbl->blk, priv);
if (ret) {
ret = -EINTR;
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index 3db5552..c6916ae 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -179,7 +179,7 @@ typedef int (nvm_bb_update_fn)(struct ppa_addr, int, u8 *, void *);
typedef int (nvm_id_fn)(struct request_queue *, struct nvm_id *);
typedef int (nvm_get_l2p_tbl_fn)(struct request_queue *, u64, u32,
nvm_l2p_update_fn *, void *);
-typedef int (nvm_op_bb_tbl_fn)(struct request_queue *, struct ppa_addr, int,
+typedef int (nvm_op_bb_tbl_fn)(struct nvm_dev *, struct ppa_addr, int,
nvm_bb_update_fn *, void *);
typedef int (nvm_op_set_bb_fn)(struct request_queue *, struct nvm_rq *, int);
typedef int (nvm_submit_io_fn)(struct request_queue *, struct nvm_rq *);
--
2.1.4
From: Wenwei Tao <[email protected]>
To avoid race conditions, traverse dev, media manager,
and target lists and also register, unregister entries
to/from them, should be always under the nvm_lock control.
Signed-off-by: Wenwei Tao <[email protected]>
Signed-off-by: Matias Bjørling <[email protected]>
---
drivers/lightnvm/core.c | 75 +++++++++++++++++++++++++++----------------------
1 file changed, 42 insertions(+), 33 deletions(-)
diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index ea6dba5..86ce887 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -123,6 +123,26 @@ void nvm_unregister_mgr(struct nvmm_type *mt)
}
EXPORT_SYMBOL(nvm_unregister_mgr);
+/* register with device with a supported manager */
+static int register_mgr(struct nvm_dev *dev)
+{
+ struct nvmm_type *mt;
+ int ret = 0;
+
+ list_for_each_entry(mt, &nvm_mgrs, list) {
+ ret = mt->register_mgr(dev);
+ if (ret > 0) {
+ dev->mt = mt;
+ break; /* successfully initialized */
+ }
+ }
+
+ if (!ret)
+ pr_info("nvm: no compatible nvm manager found.\n");
+
+ return ret;
+}
+
static struct nvm_dev *nvm_find_nvm_dev(const char *name)
{
struct nvm_dev *dev;
@@ -221,7 +241,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,21 +271,13 @@ 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");
+ down_write(&nvm_lock);
+ ret = register_mgr(dev);
+ up_write(&nvm_lock);
+ if (ret < 0)
+ goto err;
+ if (!ret)
return 0;
- }
pr_info("nvm: registered %s [%u/%u/%u/%u/%u/%u]\n",
dev->name, dev->sec_per_pg, dev->nr_planes,
@@ -337,15 +348,17 @@ EXPORT_SYMBOL(nvm_register);
void nvm_unregister(char *disk_name)
{
- struct nvm_dev *dev = nvm_find_nvm_dev(disk_name);
+ struct nvm_dev *dev;
+ down_write(&nvm_lock);
+ dev = nvm_find_nvm_dev(disk_name);
if (!dev) {
pr_err("nvm: could not find device %s to unregister\n",
disk_name);
+ up_write(&nvm_lock);
return;
}
- down_write(&nvm_lock);
list_del(&dev->devices);
up_write(&nvm_lock);
@@ -363,38 +376,30 @@ 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)
- return ret; /* initialization failed */
- if (ret > 0) {
- dev->mt = mt;
- break; /* successfully initialized */
- }
- }
-
- if (!ret) {
- pr_info("nvm: no compatible nvm manager found.\n");
- return -ENODEV;
+ ret = register_mgr(dev);
+ if (!ret)
+ ret = -ENODEV;
+ if (ret < 0) {
+ up_write(&nvm_lock);
+ return ret;
}
}
tt = nvm_find_target_type(create->tgttype);
if (!tt) {
pr_err("nvm: target type %s not found\n", create->tgttype);
+ up_write(&nvm_lock);
return -EINVAL;
}
- down_write(&nvm_lock);
list_for_each_entry(t, &dev->online_targets, list) {
if (!strcmp(create->tgtname, t->disk->disk_name)) {
pr_err("nvm: target name already exists.\n");
@@ -478,7 +483,9 @@ static int __nvm_configure_create(struct nvm_ioctl_create *create)
struct nvm_dev *dev;
struct nvm_ioctl_create_simple *s;
+ down_write(&nvm_lock);
dev = nvm_find_nvm_dev(create->dev);
+ up_write(&nvm_lock);
if (!dev) {
pr_err("nvm: device not found\n");
return -EINVAL;
@@ -537,7 +544,9 @@ static int nvm_configure_show(const char *val)
return -EINVAL;
}
+ down_write(&nvm_lock);
dev = nvm_find_nvm_dev(devname);
+ up_write(&nvm_lock);
if (!dev) {
pr_err("nvm: device not found\n");
return -EINVAL;
--
2.1.4
On 11/28/2015 08:49 AM, Matias Bjørling wrote:
> Hi Jens,
>
> A couple of patches during the last week.
>
> Patch 1: Fix from Keith to compile out lightnvm when it is not
> available.
> Patch 2-4: Various memory and check fixes from Tao, Sudip and me.
> Patch 5: Refactor device detection ids.
> Patch 6: Make get_bb_tbl return converted ppa back to media manager.
> Patch 7: Missing locks fixes from Tao on module iterations.
>
> Please pick up when convenient.
Added, thanks.
--
Jens Axboe