nvme : Add sysfs entry for NVMe CMBs when appropriate

PATCH New Stephen Bates 3 participants 7 messages Oct 6, 2016 – Oct 12, 2016 NVMe

Share & tools

Stephen Bates

author

2016-10-06 02:01 UTC #0 lore raw

cover nvme : Add sysfs entry for NVMe CMBs when appropriate

We have had a few attempts at adding CMB properties and control to sysfs [1][2]. This patch is based on Jon Derrick's [1] to some exent but takes a much more conservative approach by only displaying CMB properties and not attmpting to control them. Assuming this gets accepted I do have a follow-on that adds more information to the cmb attribute file.

Rather nicely we contain these changes solely to pci.c.

Applies cleanly to commit 997198ba1ed691c ("fs/block_dev.c: return the right error in thaw_bdev()") in Jens' for-4.9/block branch.

Tested on both QEMU and on some real hardware I have access too that can advertise and expose a CMB.

[1] http://www.spinics.net/lists/linux-block/msg01560.html
[2] http://www.spinics.net/lists/linux-rdma/msg38752.html

Changes since v0:
Use direct refernece to nvme_dev cmbsz.
Use snprintf.
Added Reviewed-By tags.

Stephen Bates (1):
nvme : Add sysfs entry for NVMe CMBs when appropriate

 drivers/nvme/host/pci.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

--
2.1.4

Stephen Bates

author

2016-10-06 02:01 UTC #1 lore raw

PATCH 1/1 nvme : Add sysfs entry for NVMe CMBs when appropriate

Add a sysfs attribute that contains salient information about the NVMe Controller Memory Buffer when one is present. For now, just display the information about the CMB available from the control registers. We attach the CMB attribute file to the existing nvme_ctrl sysfs group so it can handle the sysfs teardown.

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Reviewed-by Jay Freyensee <james_p_freyensee at linux.intel.com>

Signed-off-by Stephen Bates <sbates at raithlin.com>

---
 drivers/nvme/host/pci.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

drivers/nvme/host/pci.c +35 -9

index 8dcf5a9..d4c7bac 100644
@@ -99,6 +99,7 @@ struct nvme_dev {
 	dma_addr_t cmb_dma_addr;
 	u64 cmb_size;
 	u32 cmbsz;
+	u32 cmbloc;
 	struct nvme_ctrl ctrl;
 	struct completion ioq_wait;
 };

@@ -1321,28 +1322,37 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
 	return ret >= 0 ? 0 : ret;
 }
 
+static ssize_t nvme_cmb_show(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	struct nvme_dev *ndev = to_nvme_dev(dev_get_drvdata(dev));
+
+	return snprintf(buf, PAGE_SIZE, "cmbloc : x%08x\ncmbsz  : x%08x\n",
+		       ndev->cmbloc, ndev->cmbsz);
+}
+static DEVICE_ATTR(cmb, S_IRUGO, nvme_cmb_show, NULL);
+
 static void __iomem *nvme_map_cmb(struct nvme_dev *dev)
 {
 	u64 szu, size, offset;
-	u32 cmbloc;
 	resource_size_t bar_size;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	void __iomem *cmb;
 	dma_addr_t dma_addr;
 
-	if (!use_cmb_sqes)
-		return NULL;
-
 	dev->cmbsz = readl(dev->bar + NVME_REG_CMBSZ);
 	if (!(NVME_CMB_SZ(dev->cmbsz)))
 		return NULL;
+	dev->cmbloc = readl(dev->bar + NVME_REG_CMBLOC);
 
-	cmbloc = readl(dev->bar + NVME_REG_CMBLOC);
+	if (!use_cmb_sqes)
+		return NULL;
 
 	szu = (u64)1 << (12 + 4 * NVME_CMB_SZU(dev->cmbsz));
 	size = szu * NVME_CMB_SZ(dev->cmbsz);
-	offset = szu * NVME_CMB_OFST(cmbloc);
-	bar_size = pci_resource_len(pdev, NVME_CMB_BIR(cmbloc));
+	offset = szu * NVME_CMB_OFST(dev->cmbloc);
+	bar_size = pci_resource_len(pdev, NVME_CMB_BIR(dev->cmbloc));
 
 	if (offset > bar_size)
 		return NULL;

@@ -1355,7 +1365,7 @@ static void __iomem *nvme_map_cmb(struct nvme_dev *dev)
 	if (size > bar_size - offset)
 		size = bar_size - offset;
 
-	dma_addr = pci_resource_start(pdev, NVME_CMB_BIR(cmbloc)) + offset;
+	dma_addr = pci_resource_start(pdev, NVME_CMB_BIR(dev->cmbloc)) + offset;
 	cmb = ioremap_wc(dma_addr, size);
 	if (!cmb)
 		return NULL;

@@ -1642,9 +1652,25 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 			dev->q_depth);
 	}
 
-	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2))
+	/*
+	 * CMBs can currently only exist on >=1.2 PCIe devices. We only
+	 * populate sysfs if a CMB is implemented. Note that we add the
+	 * CMB attribute to the nvme_ctrl kobj which removes the need to remove
+	 * it on exit. Since nvme_dev_attrs_group has no name we can pass
+	 * NULL as final argument to sysfs_add_file_to_group.
+	 */
+
+	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2)) {
 		dev->cmb = nvme_map_cmb(dev);
 
+		if (dev->cmbsz) {
+			if (sysfs_add_file_to_group(&dev->ctrl.device->kobj,
+						    &dev_attr_cmb.attr, NULL))
+				dev_warn(dev->dev,
+					 "failed to add sysfs attribute for CMB\n");
+		}
+	}
+
 	pci_enable_pcie_error_reporting(pdev);
 	pci_save_state(pdev);
 	return 0;

--
2.1.4

Jon Derrick

2016-10-07 15:26 UTC #2 lore raw

PATCH 1/1 nvme : Add sysfs entry for NVMe CMBs when appropriate

Looks good. Thanks for working on this. I'm looking forward to seeing more CMB code.

Acked-by Jon Derrick: <jonathan.derrick at intel.com>

Quoted 108 lines · Stephen Bates

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Reviewed-by Jay Freyensee <james_p_freyensee at linux.intel.com>

Signed-off-by Stephen Bates <sbates at raithlin.com>

---
 drivers/nvme/host/pci.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

drivers/nvme/host/pci.c +35 ~~-10~~

index 8dcf5a9..d4c7bac 100644
@@ -99,6 +99,7 @@ struct nvme_dev {
 	dma_addr_t cmb_dma_addr;
 	u64 cmb_size;
 	u32 cmbsz;
+	u32 cmbloc;
 	struct nvme_ctrl ctrl;
 	struct completion ioq_wait;
 };

@@ -1321,28 +1322,37 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
 	return ret >= 0 ? 0 : ret;
 }
 
+static ssize_t nvme_cmb_show(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	struct nvme_dev *ndev = to_nvme_dev(dev_get_drvdata(dev));
+
+	return snprintf(buf, PAGE_SIZE, "cmbloc : x%08x\ncmbsz  : x%08x\n",
+		       ndev->cmbloc, ndev->cmbsz);
+}
+static DEVICE_ATTR(cmb, S_IRUGO, nvme_cmb_show, NULL);
+
 static void __iomem *nvme_map_cmb(struct nvme_dev *dev)
 {
 	u64 szu, size, offset;
-	u32 cmbloc;
 	resource_size_t bar_size;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 	void __iomem *cmb;
 	dma_addr_t dma_addr;
 
-	if (!use_cmb_sqes)
-		return NULL;
-
 	dev->cmbsz = readl(dev->bar + NVME_REG_CMBSZ);
 	if (!(NVME_CMB_SZ(dev->cmbsz)))
 		return NULL;
+	dev->cmbloc = readl(dev->bar + NVME_REG_CMBLOC);
 
-	cmbloc = readl(dev->bar + NVME_REG_CMBLOC);
+	if (!use_cmb_sqes)
+		return NULL;
 
 	szu = (u64)1 << (12 + 4 * NVME_CMB_SZU(dev->cmbsz));
 	size = szu * NVME_CMB_SZ(dev->cmbsz);
-	offset = szu * NVME_CMB_OFST(cmbloc);
-	bar_size = pci_resource_len(pdev, NVME_CMB_BIR(cmbloc));
+	offset = szu * NVME_CMB_OFST(dev->cmbloc);
+	bar_size = pci_resource_len(pdev, NVME_CMB_BIR(dev->cmbloc));
 
 	if (offset > bar_size)
 		return NULL;

@@ -1355,7 +1365,7 @@ static void __iomem *nvme_map_cmb(struct nvme_dev *dev)
 	if (size > bar_size - offset)
 		size = bar_size - offset;
 
-	dma_addr = pci_resource_start(pdev, NVME_CMB_BIR(cmbloc)) + offset;
+	dma_addr = pci_resource_start(pdev, NVME_CMB_BIR(dev->cmbloc)) + offset;
 	cmb = ioremap_wc(dma_addr, size);
 	if (!cmb)
 		return NULL;

@@ -1642,9 +1652,25 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 			dev->q_depth);
 	}
 
-	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2))
+	/*
+	 * CMBs can currently only exist on >=1.2 PCIe devices. We only
+	 * populate sysfs if a CMB is implemented. Note that we add the
+	 * CMB attribute to the nvme_ctrl kobj which removes the need to remove
+	 * it on exit. Since nvme_dev_attrs_group has no name we can pass
+	 * NULL as final argument to sysfs_add_file_to_group.
+	 */
+
+	if (readl(dev->bar + NVME_REG_VS) >= NVME_VS(1, 2)) {
 		dev->cmb = nvme_map_cmb(dev);
 
+		if (dev->cmbsz) {
+			if (sysfs_add_file_to_group(&dev->ctrl.device->kobj,
+						    &dev_attr_cmb.attr, NULL))
+				dev_warn(dev->dev,
+					 "failed to add sysfs attribute for CMB\n");
+		}
+	}
+
 	pci_enable_pcie_error_reporting(pdev);
 	pci_save_state(pdev);
 	return 0;
--

2.1.4

Stephen Bates

author

2016-10-12 17:03 UTC #3 lore raw

PATCH 1/1 nvme : Add sysfs entry for NVMe CMBs when appropriate

Quoted 5 lines · Jon Derrick

Looks good. Thanks for working on this. I'm looking forward to seeing more CMB code.
Acked-by Jon Derrick: <jonathan.derrick at intel.com>

Thanks Jon

Quoted 8 lines

From [1]
Due to being away for a bit, I'm currently a bit behind on patches. I'll be going over those today. But if you, or anyone else, have things pending that I haven't picked up yet, do send me a ping.
--
Jens Axboe

ping ;-)

Jens, can you pick this one up?

Cheers

Stephen

[1] http://lists.infradead.org/pipermail/linux-nvme/2016-October/006604.html

Jens Axboe

2016-10-12 17:10 UTC #4 lore raw

PATCH 1/1 nvme : Add sysfs entry for NVMe CMBs when appropriate

Quoted 15 lines · Stephen Bates

Quoted 5 lines · Jon Derrick
Looks good. Thanks for working on this. I'm looking forward to seeing more CMB code.
Acked-by Jon Derrick: <jonathan.derrick at intel.com>
Thanks Jon
From [1]
Quoted 4 lines
Due to being away for a bit, I'm currently a bit behind on patches. I'll be going over those today. But if you, or anyone else, have things pending that I haven't picked up yet, do send me a ping.

Quoted 6 lines

Quoted 2 lines
--
Jens Axboe
ping ;-)
Jens, can you pick this one up?

Thanks for the ping, added!

Signature · 1 line

Jens Axboe

Stephen Bates

author

2016-10-12 17:10 UTC #5 lore raw

PATCH 1/1 nvme : Add sysfs entry for NVMe CMBs when appropriate

In reply to Jon Derrick

Quoted 3 lines · Jon Derrick

Looks good. Thanks for working on this. I'm looking forward to seeing more CMB code.

Jens

My last email might have been confusing. The link to the patch is actually here [1]. It also has a subsequent

Acked-by Jon Derrick: <jonathan.derrick at intel.com>

tag from Jon. Sorry for any confusion.

Cheers

Stephen

[1] http://lists.infradead.org/pipermail/linux-nvme/2016-October/006540.html

Jens Axboe

2016-10-12 17:13 UTC #6 lore raw

PATCH 1/1 nvme : Add sysfs entry for NVMe CMBs when appropriate

Quoted 5 lines · Stephen Bates

Quoted 3 lines · Jon Derrick
Looks good. Thanks for working on this. I'm looking forward to seeing more CMB code.

Quoted 8 lines

Jens
My last email might have been confusing. The link to the patch is actually here [1]. It also has a subsequent
Acked-by Jon Derrick: <jonathan.derrick at intel.com>
tag from Jon. Sorry for any confusion.

I did catch that, and the tag was added.

Signature · 1 line

Jens Axboe