nvme: fix lightnvm check
Subsystems (2)
Christoph Hellwig
maintainerPATCH 1/1 nvme: fix lightnvm check
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.
--- 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(-)
drivers/nvme/host/core.c +5 -4
@@ -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);
drivers/nvme/host/lightnvm.c +0 -26
@@ -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 ; -}
drivers/nvme/host/nvme.h +5 -5
@@ -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) {
drivers/nvme/host/pci.c +4 -0
@@ -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) },
Signature · 1 line
2.11.0
Looks good,
Looks good,
Quoted 5 lines · Christoph Hellwig
nvme_nvm_ns_supportedassumes every device is apci_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.
Christoph Hellwig
maintainerPATCH 1/1 nvme: fix lightnvm check
Quoted 5 lines · Keith Busch
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..
PATCH 1/1 nvme: fix lightnvm check
Quoted 10 lines · Christoph Hellwig
Quoted 5 lines · Keith Busch
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.
Christoph Hellwig
maintainerPATCH 1/1 nvme: fix lightnvm check
Quoted 5 lines · Matias Bj?rling
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.
Quoted 3 lines
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.
PATCH 1/1 nvme: fix lightnvm check
Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Matias Bj?rling
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.
Quoted 8 lines
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.
Quoted 11 lines
Quoted 4 lines
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