* [PATCH] nvme: fix lightnvm check
@ 2017-09-06 9:48 Christoph Hellwig
2017-09-06 10:22 ` Sagi Grimberg
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Christoph Hellwig @ 2017-09-06 9:48 UTC (permalink / raw)
To: keith.busch, sagi, mb; +Cc: linux-nvme, stable
nvme_nvm_ns_supported assumes every device is a pci_dev, which leads to
reading an incorrect field, or possible even a dereference of unallocated
memory for fabrics controllers.
Fix this by introducing a quirk for lighnvm capable devices instead.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org
---
drivers/nvme/host/core.c | 9 +++++----
drivers/nvme/host/lightnvm.c | 26 --------------------------
drivers/nvme/host/nvme.h | 10 +++++-----
drivers/nvme/host/pci.c | 4 ++++
4 files changed, 14 insertions(+), 35 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 277a7a02cba5..8040fc14fd15 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2377,10 +2377,11 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
nvme_report_ns_ids(ctrl, ns->ns_id, id, ns->eui, ns->nguid, &ns->uuid);
- if (nvme_nvm_ns_supported(ns, id) &&
- nvme_nvm_register(ns, disk_name, node)) {
- dev_warn(ctrl->device, "%s: LightNVM init failure\n", __func__);
- goto out_free_id;
+ if ((ctrl->quirks & NVME_QUIRK_LIGHTNVM) && id->vs[0] == 0x1) {
+ if (nvme_nvm_register(ns, disk_name, node)) {
+ dev_warn(ctrl->device, "LightNVM init failure\n");
+ goto out_free_id;
+ }
}
disk = alloc_disk_node(0, node);
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index c1a28569e843..1f79e3f141e6 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -955,29 +955,3 @@ void nvme_nvm_unregister_sysfs(struct nvme_ns *ns)
sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
&nvm_dev_attr_group);
}
-
-/* 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_ctrl *ctrl = ns->ctrl;
- /* XXX: this is poking into PCI structures from generic code! */
- struct pci_dev *pdev = to_pci_dev(ctrl->dev);
-
- /* QEMU NVMe simulator - PCI ID + Vendor specific bit */
- 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 == PCI_VENDOR_ID_CNEX &&
- pdev->device == PCI_DEVICE_ID_CNEX_WL &&
- id->vs[0] == 0x1)
- return 1;
-
- return 0;
-}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a19a587d60ed..b8ba7c85e61b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -75,6 +75,11 @@ enum nvme_quirks {
* The deepest sleep state should not be used.
*/
NVME_QUIRK_NO_DEEPEST_PS = (1 << 5),
+
+ /*
+ * Supports the LighNVM command set if indicated in vs[1].
+ */
+ NVME_QUIRK_LIGHTNVM = (1 << 6),
};
/*
@@ -320,7 +325,6 @@ void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);
int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
#ifdef CONFIG_NVM
-int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id);
int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node);
void nvme_nvm_unregister(struct nvme_ns *ns);
int nvme_nvm_register_sysfs(struct nvme_ns *ns);
@@ -339,10 +343,6 @@ static inline int nvme_nvm_register_sysfs(struct nvme_ns *ns)
return 0;
}
static inline void nvme_nvm_unregister_sysfs(struct nvme_ns *ns) {};
-static inline int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id)
-{
- return 0;
-}
static inline int nvme_nvm_ioctl(struct nvme_ns *ns, unsigned int cmd,
unsigned long arg)
{
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 11874afb2422..dc0146e6c952 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2497,6 +2497,10 @@ static const struct pci_device_id nvme_id_table[] = {
.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
{ PCI_DEVICE(0x144d, 0xa822), /* Samsung PM1725a */
.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
+ { PCI_DEVICE(0x1d1d, 0x1f1f), /* LighNVM qemu device */
+ .driver_data = NVME_QUIRK_LIGHTNVM, },
+ { PCI_DEVICE(0x1d1d, 0x2807), /* CNEX WL */
+ .driver_data = NVME_QUIRK_LIGHTNVM, },
{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] nvme: fix lightnvm check
2017-09-06 9:48 [PATCH] nvme: fix lightnvm check Christoph Hellwig
@ 2017-09-06 10:22 ` Sagi Grimberg
2017-09-06 10:25 ` Sagi Grimberg
2017-09-06 15:06 ` Keith Busch
2 siblings, 0 replies; 9+ messages in thread
From: Sagi Grimberg @ 2017-09-06 10:22 UTC (permalink / raw)
To: Christoph Hellwig, keith.busch, mb; +Cc: linux-nvme, stable
Looks good,
Reviewed-by: Sagi Grimberg <sagi@grimberg.m>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: fix lightnvm check
2017-09-06 9:48 [PATCH] nvme: fix lightnvm check Christoph Hellwig
2017-09-06 10:22 ` Sagi Grimberg
@ 2017-09-06 10:25 ` Sagi Grimberg
2017-09-06 15:06 ` Keith Busch
2 siblings, 0 replies; 9+ messages in thread
From: Sagi Grimberg @ 2017-09-06 10:25 UTC (permalink / raw)
To: Christoph Hellwig, keith.busch, mb; +Cc: linux-nvme, stable
Looks good,
Reviewed-by: Sagi Grimberg <sagi@rimberg.me>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: fix lightnvm check
2017-09-06 9:48 [PATCH] nvme: fix lightnvm check Christoph Hellwig
2017-09-06 10:22 ` Sagi Grimberg
2017-09-06 10:25 ` Sagi Grimberg
@ 2017-09-06 15:06 ` Keith Busch
2017-09-07 12:01 ` Christoph Hellwig
2 siblings, 1 reply; 9+ messages in thread
From: Keith Busch @ 2017-09-06 15:06 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: sagi, mb, linux-nvme, stable
On Wed, Sep 06, 2017 at 11:48:25AM +0200, Christoph Hellwig wrote:
> nvme_nvm_ns_supported assumes every device is a pci_dev, which leads to
> reading an incorrect field, or possible even a dereference of unallocated
> memory for fabrics controllers.
>
> Fix this by introducing a quirk for lighnvm capable devices instead.
Ugh, even with this quirk, new lightnvm device will be broken for
all existing kernels. This sort of thing really could benefit from a
standard's defined capability or command set supported bit to distinguish
lightnvm devices. Lacking that, this looks like the only thing we can
do at the moment.
Reviewed-by: Keith Busch <keith.busch@intel.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: fix lightnvm check
2017-09-06 15:06 ` Keith Busch
@ 2017-09-07 12:01 ` Christoph Hellwig
2017-09-07 12:15 ` Matias Bjørling
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2017-09-07 12:01 UTC (permalink / raw)
To: Keith Busch; +Cc: Christoph Hellwig, sagi, mb, linux-nvme, stable
On Wed, Sep 06, 2017 at 11:06:04AM -0400, Keith Busch wrote:
> Ugh, even with this quirk, new lightnvm device will be broken for
> all existing kernels. This sort of thing really could benefit from a
> standard's defined capability or command set supported bit to distinguish
> lightnvm devices. Lacking that, this looks like the only thing we can
> do at the moment.
I told them to register a different class code for these devices a long
time ago, but nothing happened. Then again I might underestimate the
complexity of getting a class code..
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: fix lightnvm check
2017-09-07 12:01 ` Christoph Hellwig
@ 2017-09-07 12:15 ` Matias Bjørling
2017-09-07 12:20 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Matias Bjørling @ 2017-09-07 12:15 UTC (permalink / raw)
To: Christoph Hellwig, Keith Busch; +Cc: stable, mb, linux-nvme, sagi
On 09/07/2017 02:01 PM, Christoph Hellwig wrote:
> On Wed, Sep 06, 2017 at 11:06:04AM -0400, Keith Busch wrote:
>> Ugh, even with this quirk, new lightnvm device will be broken for
>> all existing kernels. This sort of thing really could benefit from a
>> standard's defined capability or command set supported bit to distinguish
>> lightnvm devices. Lacking that, this looks like the only thing we can
>> do at the moment.
>
> I told them to register a different class code for these devices a long
> time ago, but nothing happened. Then again I might underestimate the
> complexity of getting a class code..
I must have misunderstood when we discussed it. I understood it as to
add detection to the NVMe specification.
Going with Class code might not be the right way due to them describing
the base protocol.
It would be great to have a command set id dedicated for it. We have
been waiting on completing the specification before going in and
allocating a bit. Another approach, as OCSSD has grown, might be to use
the existing command set, and teach the identify command about
Zones/Chunks, where one writes sequentially within.
Reviewed-by: Matias Bjørling <mb@lightnvm.io>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: fix lightnvm check
2017-09-07 12:15 ` Matias Bjørling
@ 2017-09-07 12:20 ` Christoph Hellwig
2017-09-07 12:34 ` Matias Bjørling
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2017-09-07 12:20 UTC (permalink / raw)
To: Matias Bjørling
Cc: Christoph Hellwig, Keith Busch, stable, linux-nvme, sagi
On Thu, Sep 07, 2017 at 02:15:52PM +0200, Matias Bj�rling wrote:
> I must have misunderstood when we discussed it. I understood it as to add
> detection to the NVMe specification.
>
> Going with Class code might not be the right way due to them describing the
> base protocol.
Well - existing NVMe (except Linux with lighnvm) simply expect a NVMe
PCIe device to support the NVM I/O command set. So if you have a drive
that wants to speak a different one it needs to be bindable to a new
driver and thus have a different PCIe class code.
But even with that in place we should also have the new command set
in CAP - so that a common code base can work all these devices, and
the command set can be used over fabrics as well for example.
> bit. Another approach, as OCSSD has grown, might be to use the existing
> command set, and teach the identify command about Zones/Chunks, where one
> writes sequentially within.
Even then we need a way to identify such devices. SCSI is a great
example - if the device has zones that MUST be written sequentially
it's a new ZBC device type. If the device just wants zones to be
written sequentially but can handle random writes with a performance
penalty it's the good old SBC type with a few extensions.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: fix lightnvm check
2017-09-07 12:20 ` Christoph Hellwig
@ 2017-09-07 12:34 ` Matias Bjørling
0 siblings, 0 replies; 9+ messages in thread
From: Matias Bjørling @ 2017-09-07 12:34 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, stable, linux-nvme, sagi
On 09/07/2017 02:20 PM, Christoph Hellwig wrote:
> On Thu, Sep 07, 2017 at 02:15:52PM +0200, Matias Bjørling wrote:
>> I must have misunderstood when we discussed it. I understood it as to add
>> detection to the NVMe specification.
>>
>> Going with Class code might not be the right way due to them describing the
>> base protocol.
>
> Well - existing NVMe (except Linux with lighnvm) simply expect a NVMe
> PCIe device to support the NVM I/O command set. So if you have a drive
> that wants to speak a different one it needs to be bindable to a new
> driver and thus have a different PCIe class code.
>
> But even with that in place we should also have the new command set
> in CAP - so that a common code base can work all these devices, and
> the command set can be used over fabrics as well for example.
Agree.
>
>> bit. Another approach, as OCSSD has grown, might be to use the existing
>> command set, and teach the identify command about Zones/Chunks, where one
>> writes sequentially within.
>
> Even then we need a way to identify such devices. SCSI is a great
> example - if the device has zones that MUST be written sequentially
> it's a new ZBC device type. If the device just wants zones to be
> written sequentially but can handle random writes with a performance
> penalty it's the good old SBC type with a few extensions.
>
Agree, it would be a device type that requires the zones to be written
sequentially.
I did a PoC a while back on exposing OCSSD as a ZBC device. Fairly
straight-forward to do.
https://github.com/OpenChannelSSD/linux/commit/d1e0dbd88fab049631c868c6a6f6b6806ca752ec
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] nvme: fix lightnvm check
@ 2017-09-06 9:50 Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2017-09-06 9:50 UTC (permalink / raw)
To: keith.busch, sagi, mb; +Cc: linux-nvme, stable
nvme_nvm_ns_supported assumes every device is a pci_dev, which leads to
reading an incorrect field, or possible even a dereference of unallocated
memory for fabrics controllers.
Fix this by introducing a quirk for lighnvm capable devices instead.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org
---
Now sent to the correct linux-nvme list address..
drivers/nvme/host/core.c | 9 +++++----
drivers/nvme/host/lightnvm.c | 26 --------------------------
drivers/nvme/host/nvme.h | 10 +++++-----
drivers/nvme/host/pci.c | 4 ++++
4 files changed, 14 insertions(+), 35 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 277a7a02cba5..8040fc14fd15 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2377,10 +2377,11 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
nvme_report_ns_ids(ctrl, ns->ns_id, id, ns->eui, ns->nguid, &ns->uuid);
- if (nvme_nvm_ns_supported(ns, id) &&
- nvme_nvm_register(ns, disk_name, node)) {
- dev_warn(ctrl->device, "%s: LightNVM init failure\n", __func__);
- goto out_free_id;
+ if ((ctrl->quirks & NVME_QUIRK_LIGHTNVM) && id->vs[0] == 0x1) {
+ if (nvme_nvm_register(ns, disk_name, node)) {
+ dev_warn(ctrl->device, "LightNVM init failure\n");
+ goto out_free_id;
+ }
}
disk = alloc_disk_node(0, node);
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index c1a28569e843..1f79e3f141e6 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -955,29 +955,3 @@ void nvme_nvm_unregister_sysfs(struct nvme_ns *ns)
sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
&nvm_dev_attr_group);
}
-
-/* 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_ctrl *ctrl = ns->ctrl;
- /* XXX: this is poking into PCI structures from generic code! */
- struct pci_dev *pdev = to_pci_dev(ctrl->dev);
-
- /* QEMU NVMe simulator - PCI ID + Vendor specific bit */
- 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 == PCI_VENDOR_ID_CNEX &&
- pdev->device == PCI_DEVICE_ID_CNEX_WL &&
- id->vs[0] == 0x1)
- return 1;
-
- return 0;
-}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a19a587d60ed..b8ba7c85e61b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -75,6 +75,11 @@ enum nvme_quirks {
* The deepest sleep state should not be used.
*/
NVME_QUIRK_NO_DEEPEST_PS = (1 << 5),
+
+ /*
+ * Supports the LighNVM command set if indicated in vs[1].
+ */
+ NVME_QUIRK_LIGHTNVM = (1 << 6),
};
/*
@@ -320,7 +325,6 @@ void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);
int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
#ifdef CONFIG_NVM
-int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id);
int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node);
void nvme_nvm_unregister(struct nvme_ns *ns);
int nvme_nvm_register_sysfs(struct nvme_ns *ns);
@@ -339,10 +343,6 @@ static inline int nvme_nvm_register_sysfs(struct nvme_ns *ns)
return 0;
}
static inline void nvme_nvm_unregister_sysfs(struct nvme_ns *ns) {};
-static inline int nvme_nvm_ns_supported(struct nvme_ns *ns, struct nvme_id_ns *id)
-{
- return 0;
-}
static inline int nvme_nvm_ioctl(struct nvme_ns *ns, unsigned int cmd,
unsigned long arg)
{
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 11874afb2422..dc0146e6c952 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2497,6 +2497,10 @@ static const struct pci_device_id nvme_id_table[] = {
.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
{ PCI_DEVICE(0x144d, 0xa822), /* Samsung PM1725a */
.driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
+ { PCI_DEVICE(0x1d1d, 0x1f1f), /* LighNVM qemu device */
+ .driver_data = NVME_QUIRK_LIGHTNVM, },
+ { PCI_DEVICE(0x1d1d, 0x2807), /* CNEX WL */
+ .driver_data = NVME_QUIRK_LIGHTNVM, },
{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-07 12:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-06 9:48 [PATCH] nvme: fix lightnvm check Christoph Hellwig
2017-09-06 10:22 ` Sagi Grimberg
2017-09-06 10:25 ` Sagi Grimberg
2017-09-06 15:06 ` Keith Busch
2017-09-07 12:01 ` Christoph Hellwig
2017-09-07 12:15 ` Matias Bjørling
2017-09-07 12:20 ` Christoph Hellwig
2017-09-07 12:34 ` Matias Bjørling
-- strict thread matches above, loose matches on Subject: below --
2017-09-06 9:50 Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).