NVMe:Expose model attribute in sysfs

PATCH New Sujith Pandel 3 participants 9 messages Aug 25, 2015 – Sep 9, 2015

Subsystems (2)

Block layer NVMe

lore

Share & tools

lore

Sujith Pandel

author

2015-08-25 04:54 UTC #0 lore raw

PATCH 1/1 NVMe:Expose model attribute in sysfs

Currently, nvme driver does not show the model attribute of disk in sysfs. nvme_dev already has this attribute populated during nvme_dev_add(). Define the end of string for model and create the sysfs model entry.

Signed-off-by Sujith Pandel <sujithpshankar at gmail.com>

Reviewed-by David Milburn <dmilburn at redhat.com>

---
 drivers/block/nvme-core.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/block/nvme-core.c +18 -0

index 7920c27..1098856 100644
@@ -2360,6 +2360,7 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	dev->vwc = ctrl->vwc;
 	memcpy(dev->serial, ctrl->sn, sizeof(ctrl->sn));
 	memcpy(dev->model, ctrl->mn, sizeof(ctrl->mn));
+	dev->model[sizeof(ctrl->mn) - 1] = '\0';
 	memcpy(dev->firmware_rev, ctrl->fr, sizeof(ctrl->fr));
 	if (ctrl->mdts)
 		dev->max_hw_sectors = 1 << (ctrl->mdts + shift - 9);

@@ -3031,6 +3032,16 @@ static ssize_t nvme_sysfs_reset(struct device *dev,
 }
 static DEVICE_ATTR(reset_controller, S_IWUSR, NULL, nvme_sysfs_reset);
 
+/* Expose model name in sysfs */
+static ssize_t nvme_sysfs_model(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct nvme_dev *ndev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", ndev->model);
+}
+static DEVICE_ATTR(model, S_IRUGO, nvme_sysfs_model, NULL);
+
 static void nvme_async_probe(struct work_struct *work);
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {

@@ -3081,6 +3092,12 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (result)
 		goto put_dev;
 
+	result = device_create_file(dev->device, &dev_attr_model);
+	if (result) {
+		device_remove_file(dev->device, &dev_attr_reset_controller);
+		goto put_dev;
+	}
+
 	INIT_LIST_HEAD(&dev->node);
 	INIT_WORK(&dev->scan_work, nvme_dev_scan);
 	INIT_WORK(&dev->probe_work, nvme_async_probe);

@@ -3140,6 +3157,7 @@ static void nvme_remove(struct pci_dev *pdev)
 	flush_work(&dev->reset_work);
 	flush_work(&dev->scan_work);
 	device_remove_file(dev->device, &dev_attr_reset_controller);
+	device_remove_file(dev->device, &dev_attr_model);
 	nvme_dev_remove(dev);
 	nvme_dev_shutdown(dev);
 	nvme_dev_remove_admin(dev);

Signature · 1 line

2.4.3

Matthew Wilcox

2015-08-25 15:38 UTC #1 lore raw

PATCH 1/1 NVMe:Expose model attribute in sysfs

drivers/block/nvme-core.c static int nvme_dev_add(struct nvme_dev *dev) · line 2366

Quoted 8 lines · Sujith Pandel

drivers/block/nvme-core.c +1 -0

@@ -2360,6 +2360,7 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	dev->vwc = ctrl->vwc;
 	memcpy(dev->serial, ctrl->sn, sizeof(ctrl->sn));
 	memcpy(dev->model, ctrl->mn, sizeof(ctrl->mn));
+	dev->model[sizeof(ctrl->mn) - 1] = '\0';
 	memcpy(dev->firmware_rev, ctrl->fr, sizeof(ctrl->fr));
 	if (ctrl->mdts)
 		dev->max_hw_sectors = 1 << (ctrl->mdts + shift - 9);

You've overwritten the last byte of the model name with a NUL byte. There could be useful information in that byte.

drivers/block/nvme-core.c static ssize_t nvme_sysfs_reset(struct device *dev, · line 3041

Quoted 7 lines

drivers/block/nvme-core.c +7 -0

+/* Expose model name in sysfs */
+static ssize_t nvme_sysfs_model(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct nvme_dev *ndev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", ndev->model);

Instead, how about:

	return sprintf(buf, "%.40s\n", ndev->model);

Sujith Pandel

author

2015-09-01 16:38 UTC #2 lore raw

PATCH 1/1 NVMe:Expose model attribute in sysfs

Quoted 7 lines · Matthew Wilcox

drivers/block/nvme-core.c static int nvme_dev_add(struct nvme_dev *dev) · line 2362
Quoted 4 lines
drivers/block/nvme-core.c +1 -0
@@ -2360,6 +2360,7 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	memcpy(dev->model, ctrl->mn, sizeof(ctrl->mn));
+	dev->model[sizeof(ctrl->mn) - 1] = '\0';
 	memcpy(dev->firmware_rev, ctrl->fr, sizeof(ctrl->fr));
You've overwritten the last byte of the model name with a NUL byte. There could be useful information in that byte.

Agree with you on this. I will remove that statement.

Quoted 10 lines

drivers/block/nvme-core.c static ssize_t nvme_sysfs_reset(struct device *dev, · line 3041
Quoted 6 lines
drivers/block/nvme-core.c +6 -0
+static ssize_t nvme_sysfs_model(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct nvme_dev *ndev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", ndev->model);
Instead, how about:
	return sprintf(buf, "%.40s\n", ndev->model);

Can we use:

	return sprintf(buf, "%.*s\n", (int)sizeof(ndev->model), ndev->model);

This way we need not hard-code the model attribute size in it. Please let me know your thoughts on this.

Regards,
Sujith

Keith Busch

2015-08-25 17:14 UTC #3 lore raw

PATCH 1/1 NVMe:Expose model attribute in sysfs

In reply to Sujith Pandel

drivers/block/nvme-core.c static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) · line 3099

Quoted 5 lines · Sujith Pandel

drivers/block/nvme-core.c +5 -0

+	result = device_create_file(dev->device, &dev_attr_model);
+	if (result) {
+		device_remove_file(dev->device, &dev_attr_reset_controller);
+		goto put_dev;
+	}

This isn't a very maintainable way to unwind on failure. A new label to 'goto' would be better.

But if we're going to have more than one sysfs entry, we can manage this easier using attribute groups instead. There are lots of examples in the kernel for this, like scsi_sysfs.c or blk-mq-sysfs.c

Sujith Pandel

author

2015-09-01 16:55 UTC #4 lore raw

PATCH 1/1 NVMe:Expose model attribute in sysfs

Quoted 10 lines · Keith Busch

drivers/block/nvme-core.c static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) · line 3099
Quoted 5 lines · Sujith Pandel
drivers/block/nvme-core.c +5 -0
+	result = device_create_file(dev->device, &dev_attr_model);
+	if (result) {
+		device_remove_file(dev->device, &dev_attr_reset_controller);
+		goto put_dev;
+	}
This isn't a very maintainable way to unwind on failure. A new label to 'goto' would be better.

Quoted 3 lines

But if we're going to have more than one sysfs entry, we can manage this easier using attribute groups instead. There are lots of examples in the kernel for this, like scsi_sysfs.c or blk-mq-sysfs.c

Since there are only two sysfs attributes here as of now,
I will be going with your former suggestion of using a new goto label. Will this be okay:

+ result = device_create_file(dev->device, &dev_attr_model);
+ if (result)
+ goto remove_file;
+

+ remove_file:
+ device_remove_file(dev->device, &dev_attr_reset_controller);
put_dev:

Please let me know.
Will send the version-2 of this patch once you confirm.

Regards,
Sujith

Keith Busch

2015-09-01 17:25 UTC #5 lore raw

PATCH 1/1 NVMe:Expose model attribute in sysfs

Quoted 7 lines · Sujith Pandel

Quoted 3 lines · Keith Busch
But if we're going to have more than one sysfs entry, we can manage this easier using attribute groups instead. There are lots of examples in the kernel for this, like scsi_sysfs.c or blk-mq-sysfs.c
Since there are only two sysfs attributes here as of now,
I will be going with your former suggestion of using a new goto label.

Aww, that just forces the person who finds a need to add a third entry to implement a better design, or maybe he/she will use the same reasoning to shift that problem to the next person.

The "zero, one, infinity rule" puts 2 in the same catagory as infinity.

Sujith Pandel

author

2015-09-04 04:54 UTC #6 lore raw

PATCH 1/1 NVMe:Expose model attribute in sysfs

Hi Keith,

Quoted 4 lines · Keith Busch

Quoted 4 lines
Quoted 3 lines
But if we're going to have more than one sysfs entry, we can manage this easier using attribute groups instead. There are lots of examples in the kernel for this, like scsi_sysfs.c or blk-mq-sysfs.c

Quoted 8 lines

Quoted 2 lines
Since there are only two sysfs attributes here as of now,
I will be going with your former suggestion of using a new goto label.
Aww, that just forces the person who finds a need to add a third entry to implement a better design, or maybe he/she will use the same reasoning to shift that problem to the next person.
The "zero, one, infinity rule" puts 2 in the same catagory as infinity.

:) Appreciate the way you and David are making me (a kernel newbie) interested to contribute more on this one. Hoping that this turns out to be my first kernel-patch!

Does this approach look okay?
If any new attributes are required, it can be added in the existing array or else, have to create a new group and call sysfs_create_group()/sysfs_remove_group() on it.

Please let me know your thoughts on this.

drivers/block/nvme-core.c +41 -2

@@ -3151,6 +3151,44 @@ static ssize_t nvme_sysfs_reset(struct device *dev,
 }
 static DEVICE_ATTR(reset_controller, S_IWUSR, NULL, nvme_sysfs_reset);
 
+#define nvme_string_attr(name)                                             \
+static ssize_t  name##_show(struct device *dev,                            \
+                struct device_attribute *attr, char *buf)                  \
+{                                                                          \
+        struct nvme_dev *ndev = dev_get_drvdata(dev);                      \
+        return sprintf(buf, "%.*s\n", (int)sizeof(ndev->name), ndev->name);\
+}                                                                          \
+static DEVICE_ATTR(name, S_IRUGO, name##_show, NULL);
+
+/*
+ * Device string attributes.
+ * Can add serial and firmware_rev attributes here.
+ */
+nvme_string_attr(model);
+
+static struct attribute *nvme_dev_attrs[] = {
+        &dev_attr_reset_controller.attr,
+        &dev_attr_model.attr,
+        NULL
+};
+
+static struct attribute_group nvme_dev_attrs_group = {
+        .attrs = nvme_dev_attrs,
+};
+
+void nvme_remove_sysfs_files(struct device *dev)
+{
+        sysfs_remove_group(&dev->kobj, &nvme_dev_attrs_group);
+}
+
+/*
+ * Create sysfs entries for device attributes.
+ */
+int nvme_create_sysfs_files(struct device *dev)
+{
+        return sysfs_create_group(&dev->kobj, &nvme_dev_attrs_group);
+}
+
 static void nvme_async_probe(struct work_struct *work);
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {

@@ -3197,7 +3235,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        get_device(dev->device);
        dev_set_drvdata(dev->device, dev);
 
-       result = device_create_file(dev->device, &dev_attr_reset_controller);
+       result = nvme_create_sysfs_files(dev->device);
        if (result)
                goto put_dev;

@@ -3208,6 +3246,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        return 0;
 
  put_dev:
+       nvme_remove_sysfs_files(dev->device);
        device_destroy(nvme_class, MKDEV(nvme_char_major, dev->instance));
        put_device(dev->device);
  release_pools:

@@ -3259,7 +3298,7 @@ static void nvme_remove(struct pci_dev *pdev)
        flush_work(&dev->probe_work);
        flush_work(&dev->reset_work);
        flush_work(&dev->scan_work);
-       device_remove_file(dev->device, &dev_attr_reset_controller);
+       nvme_remove_sysfs_files(dev->device);
        nvme_dev_remove(dev);
        nvme_dev_shutdown(dev);
        nvme_dev_remove_admin(dev);

Regards,
Sujith

Keith Busch

2015-09-08 21:29 UTC #7 lore raw

PATCH 1/1 NVMe:Expose model attribute in sysfs

Quoted 2 lines · Sujith Pandel

:) Appreciate the way you and David are making me (a kernel newbie) interested to contribute more on this one. Hoping that this turns out to be my first kernel-patch!

Ha, I was only half-serious and would have submitted an attribute group clean-up on the next sysfs I need to propose, but this looks pretty good! :)

I'm wondering why you chose to add the device model number to sysfs since this is available via other methods. Is it to maintain parity with scsi?

drivers/block/nvme-core.c static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) · line 3249

Quoted 9 lines

drivers/block/nvme-core.c +2 -0

+       result = nvme_create_sysfs_files(dev->device);
       if (result)
               goto put_dev;

@@ -3208,6 +3246,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
       return 0;
 
 put_dev:
+       nvme_remove_sysfs_files(dev->device);

You don't need to remove the sysfs files if creating them was not successful, so you shouldn't have to add this to the error out at this label. We'd need to add a new label with this in the future if the driver performs more tasks that could potentially fail, but this is the last thing this section of code does, so no need to unwind this.

Sujith Pandel

author

2015-09-09 04:46 UTC #8 lore raw

PATCH 1/1 NVMe:Expose model attribute in sysfs

Quoted 2 lines · Keith Busch

I'm wondering why you chose to add the device model number to sysfs since this is available via other methods. Is it to maintain parity with scsi?

Yes. scsi devices show model and vendor details as device attributes in sysfs. Needed these two from nvme also.

Quoted 7 lines

drivers/block/nvme-core.c +1 -0

+       result = nvme_create_sysfs_files(dev->device);
       if (result)
               goto put_dev;

@@ -3208,6 +3246,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
       return 0;

Quoted 8 lines

drivers/block/nvme-core.c static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id) · line 3249
Quoted 2 lines
drivers/block/nvme-core.c +1 -0
 put_dev:
+       nvme_remove_sysfs_files(dev->device);
You don't need to remove the sysfs files if creating them was not successful, so you shouldn't have to add this to the error out at this label. We'd need to add a new label with this in the future if the driver performs more tasks that could potentially fail, but this is the last thing this section of code does, so no need to unwind this.

Ok, I will make the change and send version-2 of the patch. Thanks!

Regards,
Sujith