[PATCH 0/2] nvme: Improvements in sysfs entry for NVMe CMBs

Hi

This series adds some more verbosity to the NVMe CMB sysfs entry.

Jens I based this off v4.9 because for some reason your for-4.10/block
is missing my original CMB commit (202021c1a63c6)?

Stephen

Stephen Bates (2):
  nvme : Use correct scnprintf in cmb show
  nvme: improve cmb sysfs reporting

 drivers/nvme/host/pci.c | 33 ++++++++++++++++++++++++++++++---
 include/linux/nvme.h    |  8 ++++++++
 2 files changed, 38 insertions(+), 3 deletions(-)

--
2.1.4

Add more information to the NVMe CMB sysfs entry. This includes
information about the CMB size, location and capabilities.

Signed-off-by: Stephen Bates <redacted>
---
 drivers/nvme/host/pci.c | 31 +++++++++++++++++++++++++++++--
 include/linux/nvme.h    |  8 ++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index be10860..1f70630 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c

@@ -98,6 +98,7 @@ struct nvme_dev {
 	void __iomem *cmb;
 	dma_addr_t cmb_dma_addr;
 	u64 cmb_size;
+	u64 cmb_offset;
 	u32 cmbsz;
 	u32 cmbloc;
 	struct nvme_ctrl ctrl;

@@ -1326,14 +1327,39 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
 	return ret >= 0 ? 0 : ret;
 }
 
+static const char * const cmb_caps[] = {
+	[NVME_CMB_CAP_SQS] = "SQS",
+	[NVME_CMB_CAP_CQS] = "CQS",
+	[NVME_CMB_CAP_LISTS] = "LISTS",
+	[NVME_CMB_CAP_RDS] = "RDS",
+	[NVME_CMB_CAP_WDS] = "WDS",
+};
+
 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));
+	unsigned int i, len = 0;
+
+	len += scnprintf(buf+len, PAGE_SIZE-len,
+			"cmbloc  : 0x%08x\ncmbsz   : 0x%08x\n\n",
+			ndev->cmbloc, ndev->cmbsz);
+
+	len += scnprintf(buf+len, PAGE_SIZE-len,
+			"OFFSET   : 0x%016llx\nSIZE     : %llu Bytes\n" \
+			"DMA ADDR : %pad\n\n",
+			ndev->cmb_offset, ndev->cmb_size,
+			&ndev->cmb_dma_addr);
+
+	for (i = NVME_CMB_CAP_SQS; i <= NVME_CMB_CAP_WDS; i++)
+		len += scnprintf(buf+len, PAGE_SIZE-len, "%-7s: %s\n",
+				 cmb_caps[i],
+				 ((ndev->cmbsz) & (1<<i)) ? "SUPPORTED" :
+				 "NOT SUPPORTED");
+
+	return len;
 
-	return scnprintf(buf, PAGE_SIZE, "cmbloc : x%08x\ncmbsz  : x%08x\n",
-		       ndev->cmbloc, ndev->cmbsz);
 }
 static DEVICE_ATTR(cmb, S_IRUGO, nvme_cmb_show, NULL);
 

@@ -1376,6 +1402,7 @@ static void __iomem *nvme_map_cmb(struct nvme_dev *dev)
 
 	dev->cmb_dma_addr = dma_addr;
 	dev->cmb_size = size;
+	dev->cmb_offset = offset;
 	return cmb;
 }
 

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index fc3c242..cd0d324 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h

@@ -121,6 +121,14 @@ enum {
 #define NVME_CMB_CQS(cmbsz)	((cmbsz) & 0x2)
 #define NVME_CMB_SQS(cmbsz)	((cmbsz) & 0x1)
 
+enum {
+	NVME_CMB_CAP_SQS = 0,
+	NVME_CMB_CAP_CQS,
+	NVME_CMB_CAP_LISTS,
+	NVME_CMB_CAP_RDS,
+	NVME_CMB_CAP_WDS,
+};
+
 /*
  * Submission and Completion Queue Entry Sizes for the NVM command set.
  * (In bytes and specified as a power of two (2^n)).

-- 
2.1.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

On 12/16/2016 11:54 AM, Stephen Bates wrote:

Hi

This series adds some more verbosity to the NVMe CMB sysfs entry.

Jens I based this off v4.9 because for some reason your for-4.10/block
is missing my original CMB commit (202021c1a63c6)?

for-4.10/block was forked off v4.9-rc1, and that patch didn't make it
in until v4.9-rc2. Since for-4.10/block has been merged, any patches
for this series or next should be based on my for-linus, or just master.

Assuming these two apply directly to master as well?

-- 
Jens Axboe

quoted

Jens I based this off v4.9 because for some reason your for-4.10/block
is missing my original CMB commit (202021c1a63c6)?

for-4.10/block was forked off v4.9-rc1, and that patch didn't make it in
until v4.9-rc2. Since for-4.10/block has been merged, any patches for this
series or next should be based on my for-linus, or just master.

Assuming these two apply directly to master as well?

Ah thanks for clearing up my confusion ;-). Yes, this series applies
cleanly to both your master and for-linus branches.


Stephen

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Make sure we are using the correct scnprintf in the sysfs show
function for the CMB.

Signed-off-by: Stephen Bates <redacted>
---
 drivers/nvme/host/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5e52034..be10860 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c

@@ -50,7 +50,7 @@
 #define NVME_AQ_DEPTH		256
 #define SQ_SIZE(depth)		(depth * sizeof(struct nvme_command))
 #define CQ_SIZE(depth)		(depth * sizeof(struct nvme_completion))
-		
+
 /*
  * We handle AEN commands ourselves and don't even let the
  * block layer know about them.

@@ -1332,7 +1332,7 @@ static ssize_t nvme_cmb_show(struct device *dev,
 {
 	struct nvme_dev *ndev = to_nvme_dev(dev_get_drvdata(dev));
 
-	return snprintf(buf, PAGE_SIZE, "cmbloc : x%08x\ncmbsz  : x%08x\n",
+	return scnprintf(buf, PAGE_SIZE, "cmbloc : x%08x\ncmbsz  : x%08x\n",
 		       ndev->cmbloc, ndev->cmbsz);
 }
 static DEVICE_ATTR(cmb, S_IRUGO, nvme_cmb_show, NULL);

-- 
2.1.4

Looks good. Thanks Stephen.

Reviewed-by Jon Derrick: [off-list ref]

On Fri, Dec 16, 2016 at 11:54:50AM -0700, Stephen Bates wrote:

On 12/16/2016 11:54 AM, Stephen Bates wrote:

Hi

This series adds some more verbosity to the NVMe CMB sysfs entry.

Jens I based this off v4.9 because for some reason your for-4.10/block
is missing my original CMB commit (202021c1a63c6)?

Stephen

Stephen Bates (2):
  nvme : Use correct scnprintf in cmb show
  nvme: improve cmb sysfs reporting

 drivers/nvme/host/pci.c | 33 ++++++++++++++++++++++++++++++---
 include/linux/nvme.h    |  8 ++++++++
 2 files changed, 38 insertions(+), 3 deletions(-)

I have added 1/2, since that one is a no-brainer. For 2/2, not so sure.
Generally we try to avoid having sysfs file that aren't single value
output. That isn't a super hard rule, but it is preferable.

-- 
Jens Axboe

Minor nit below

On Fri, Dec 16, 2016 at 11:54:51AM -0700, Stephen Bates wrote:

I'd prefer seeing (i = 0; i < ARRAY_SIZE(..); i++) because it provides
automatic bounds checking against future code.

I have added 1/2, since that one is a no-brainer. For 2/2, not so sure.
Generally we try to avoid having sysfs file that aren't single value
output. That isn't a super hard rule, but it is preferable.

--
Jens Axboe

Thanks Jens and sorry for the delay (extended vacation). Thanks for
pulling 1/2. I will go take a look at 2/2.

Stephen

Minor nit below

quoted

+
+	for (i = NVME_CMB_CAP_SQS; i <= NVME_CMB_CAP_WDS; i++)

I'd prefer seeing (i = 0; i < ARRAY_SIZE(..); i++) because it provides
automatic bounds checking against future code.

Thanks Jon, I will take a look at doing this in a V1.

Stephen