[PATCH] NVMe:Expose model attribute in sysfs

[PATCH] NVMe:Expose model attribute in sysfs

[Sujith Pandel]

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(+)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 7920c27..1098856 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -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);
-- 
2.4.3

[PATCH] NVMe:Expose model attribute in sysfs

[Matthew Wilcox]

On Tue, Aug 25, 2015@10:24:39AM +0530, Sujith Pandel wrote:
> @@ -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.

> +/* 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);

[PATCH] NVMe:Expose model attribute in sysfs

[Keith Busch]

On Mon, 24 Aug 2015, Sujith Pandel wrote:
> +	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

[PATCH] NVMe:Expose model attribute in sysfs

[Sujith Pandel]

On Tue, Aug 25, 2015@10:38:54AM -0500, Matthew Wilcox wrote:
> > @@ -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.

> > +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

[PATCH] NVMe:Expose model attribute in sysfs

[Sujith Pandel]

On Tue, Aug 25, 2015@12:14:39PM -0500, Keith Busch wrote:
> On Mon, 24 Aug 2015, Sujith Pandel wrote:
> > +	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

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

[PATCH] NVMe:Expose model attribute in sysfs

[Keith Busch]

On Tue, 1 Sep 2015, Sujith Pandel wrote:
> On Tue, Aug 25, 2015@12:14:39PM -0500, Keith Busch wrote:
>> 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.

[PATCH] NVMe:Expose model attribute in sysfs

[Sujith Pandel]

Hi Keith,
On Tue, Sep 01, 2015@12:25:48PM -0500, Keith Busch wrote:
> >> 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.
 
:) 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.

--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -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

[PATCH] NVMe:Expose model attribute in sysfs

[Keith Busch]

On Thu, 3 Sep 2015, Sujith Pandel wrote:
> :) 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?

> +       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.

[PATCH] NVMe:Expose model attribute in sysfs

[Sujith Pandel]

On Tue, Sep 08, 2015@04:29:54PM -0500, Keith Busch wrote:
> 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.

> > +       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.

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

Regards,
Sujith