[PATCH 0/5] LightNVM fixes

[PATCH 0/5] LightNVM fixes

[Matias Bjørling]

Hi Jens,

A couple of fixes for -rc3.

Patch 1: Fixes the device id detection for qemu.
Patch 2: Fix for conditional compilation of lightnvm with nvme by Keith.
Patch 3-5: Fixes for memory leaks and boundary check by Tao and Sudip.

Please pick up. Thanks.

Keith Busch (1):
  lightnvm: Simplify config when disabled

Matias Bjørling (1):
  lightnvm: change vendor and dev id for qemu

Sudip Mukherjee (1):
  lightnvm: fix ioctl memory leaks

Wenwei Tao (2):
  lightnvm: free memory when gennvm register fails
  lightnvm: do device max sectors boundary check first

 drivers/lightnvm/core.c      | 19 +++++++++++++------
 drivers/lightnvm/gennvm.c    | 15 ++++++++++-----
 drivers/nvme/host/Makefile   |  3 ++-
 drivers/nvme/host/lightnvm.c | 15 +--------------
 drivers/nvme/host/nvme.h     | 14 ++++++++++++++
 5 files changed, 40 insertions(+), 26 deletions(-)

-- 
2.1.4

[PATCH 1/5] lightnvm: change vendor and dev id for qemu

[Matias Bjørling]

The QEMU NVMe simulator uses the intel vendor, qemu device id, and the
first vendor specific byte to identify a lightnvm compatible nvme
instance.

Instead of using the Intel NVMe QEMU instance vendor and device id,
let's 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 <hch@infradead.org>
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 9202d1a..0789265 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -577,7 +577,7 @@ int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id)
 	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 == 0x1d1d && pdev->device == 0x1f1f &&
 							id->vs[0] == 0x1)
 		return 1;
 
-- 
2.1.4

[PATCH 2/5] lightnvm: Simplify config when disabled

[Matias Bjørling]

From: Keith Busch <keith.busch@intel.com>

We shouldn't compile an object file to get empty implementations; conforms
to linux coding style on conditional compilation.

Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Keith Busch <keith.busch@intel.com>
Signed-off-by: Matias Bjørling <m@bjorling.me>
---
 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 0789265..7fd20e5 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

[PATCH 3/5] lightnvm: free memory when gennvm register fails

[Matias Bjørling]

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

free allocated nvm block and gennvm lun structures when
gennvm register fails, otherwise it will cause memory leak.

Signed-off-by: Matias Bjørling <m@bjorling.me>
---
 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

[PATCH 4/5] lightnvm: fix ioctl memory leaks

[Matias Bjørling]

From: Sudip Mukherjee <sudipm.mukherjee@gmail.com>

If copy_to_user() fails we returned error but we missed releasing
devices.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
Signed-off-by: Matias Bjørling <m@bjorling.me>
---
 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

[PATCH 5/5] lightnvm: do device max sectors boundary check first

[Matias Bjørling]

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

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 <ww.tao0320@gmail.com>
Signed-off-by: Matias Bjørling <m@bjorling.me>
---
 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

RE: [PATCH 1/5] lightnvm: change vendor and dev id for qemu

[Elliott, Robert (Persistent Memory)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 856 bytes --]



> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Matias Bjørling
> Sent: Tuesday, November 24, 2015 7:27 AM
...
> Instead of using the Intel NVMe QEMU instance vendor and device id,
> let's use a preallocated from CNEX Labs instead. This lets us
...
>  	/* QEMU NVMe simulator - PCI ID + Vendor specific bit */
> -	if (pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device ==
> 0x5845 &&
> +	if (pdev->vendor == 0x1d1d && pdev->device == 0x1f1f &&
>  							id->vs[0] == 0x1)

Could this patch add PCI_VENDOR_ID_CNEX to the appropriate .h file
and use that instead?


---
Robert Elliott, HPE Persistent Memory


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/5] lightnvm: change vendor and dev id for qemu

[Matias Bjørling]

On 11/24/2015 04:52 PM, Elliott, Robert (Persistent Memory) wrote:
>
>
>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>> owner@vger.kernel.org] On Behalf Of Matias Bjørling
>> Sent: Tuesday, November 24, 2015 7:27 AM
> ...
>> Instead of using the Intel NVMe QEMU instance vendor and device id,
>> let's use a preallocated from CNEX Labs instead. This lets us
> ...
>>   	/* QEMU NVMe simulator - PCI ID + Vendor specific bit */
>> -	if (pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device ==
>> 0x5845 &&
>> +	if (pdev->vendor == 0x1d1d && pdev->device == 0x1f1f &&
>>   							id->vs[0] == 0x1)
>
> Could this patch add PCI_VENDOR_ID_CNEX to the appropriate .h file
> and use that instead?

We could. But it would only be for this single use-case? Might be a 
little overkill to put in pci_ids.h. Opt for lightnvm.h? or somewhere else?

Thanks

Re: [PATCH 1/5] lightnvm: change vendor and dev id for qemu

[Jens Axboe]

On 11/24/2015 11:24 AM, Matias Bjørling wrote:
> On 11/24/2015 04:52 PM, Elliott, Robert (Persistent Memory) wrote:
>>
>>
>>> -----Original Message-----
>>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>>> owner@vger.kernel.org] On Behalf Of Matias Bjørling
>>> Sent: Tuesday, November 24, 2015 7:27 AM
>> ...
>>> Instead of using the Intel NVMe QEMU instance vendor and device id,
>>> let's use a preallocated from CNEX Labs instead. This lets us
>> ...
>>>       /* QEMU NVMe simulator - PCI ID + Vendor specific bit */
>>> -    if (pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device ==
>>> 0x5845 &&
>>> +    if (pdev->vendor == 0x1d1d && pdev->device == 0x1f1f &&
>>>                               id->vs[0] == 0x1)
>>
>> Could this patch add PCI_VENDOR_ID_CNEX to the appropriate .h file
>> and use that instead?
>
> We could. But it would only be for this single use-case? Might be a
> little overkill to put in pci_ids.h. Opt for lightnvm.h? or somewhere else?

Or just add a comment, this:

if (pdev->vendor == 0x1d1d && pdev->device == 0x1f1f

means nothing to anyone.

-- 
Jens Axboe