diff mbox

[12/13] IB/core: Export a common fw_ver sysfs entry

Message ID 1465509248-11324-13-git-send-email-ira.weiny@intel.com (mailing list archive)
State Superseded
Headers show

Commit Message

Ira Weiny June 9, 2016, 9:54 p.m. UTC 
From: Ira Weiny <ira.weiny@intel.com>

Now that all the devices have stopped exporting their own sysfs
entry points we can have the core export this on their behalf.

Eventually this may be removed but this provides for backwards
compatibility.

Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/infiniband/core/sysfs.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Jason Gunthorpe June 9, 2016, 10:40 p.m. UTC | #1 
On Thu, Jun 09, 2016 at 05:54:07PM -0400, ira.weiny@intel.com wrote:
>  
> +static ssize_t show_fw_ver(struct device *device, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	char tmp[ETHTOOL_FWVERS_LEN];
> +	struct ib_device *dev = container_of(device, struct ib_device, dev);
> +
> +	ib_get_device_fw_str(dev, tmp, sizeof(tmp));
> +
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", tmp);

No need to use the stack:

ib_get_device_fw_str(dev, buf, PAGE_SIZE);
strlcat(buf, "", PAGE_SIZE);
return strlen(buf);

>  	&dev_attr_node_guid,
> -	&dev_attr_node_desc
> +	&dev_attr_node_desc,
> +	&dev_attr_fw_ver

Should be 
 +	&dev_attr_fw_ver,

Note trailing comma. Avoids git churn like the above on append.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Leon Romanovsky June 10, 2016, 2 p.m. UTC | #2 
On Thu, Jun 09, 2016 at 04:40:38PM -0600, Jason Gunthorpe wrote:
> On Thu, Jun 09, 2016 at 05:54:07PM -0400, ira.weiny@intel.com wrote:
> >  
> > +static ssize_t show_fw_ver(struct device *device, struct device_attribute *attr,
> > +			   char *buf)
> > +{
> > +	char tmp[ETHTOOL_FWVERS_LEN];

Very minor request,
Can we avoid variable names like this?
fw_str can be a good candidate for the name.


> > +	struct ib_device *dev = container_of(device, struct ib_device, dev);
> > +
> > +	ib_get_device_fw_str(dev, tmp, sizeof(tmp));
> > +
> > +	return scnprintf(buf, PAGE_SIZE, "%s\n", tmp);
> 
> No need to use the stack:
> 
> ib_get_device_fw_str(dev, buf, PAGE_SIZE);
> strlcat(buf, "", PAGE_SIZE);
> return strlen(buf);
> 
> >  	&dev_attr_node_guid,
> > -	&dev_attr_node_desc
> > +	&dev_attr_node_desc,
> > +	&dev_attr_fw_ver
> 
> Should be 
>  +	&dev_attr_fw_ver,
> 
> Note trailing comma. Avoids git churn like the above on append.
> 
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny June 14, 2016, 2:54 a.m. UTC | #3 
On Thu, Jun 09, 2016 at 04:40:38PM -0600, Jason Gunthorpe wrote:
> On Thu, Jun 09, 2016 at 05:54:07PM -0400, ira.weiny@intel.com wrote:
> >  
> > +static ssize_t show_fw_ver(struct device *device, struct device_attribute *attr,
> > +			   char *buf)
> > +{
> > +	char tmp[ETHTOOL_FWVERS_LEN];
> > +	struct ib_device *dev = container_of(device, struct ib_device, dev);
> > +
> > +	ib_get_device_fw_str(dev, tmp, sizeof(tmp));
> > +
> > +	return scnprintf(buf, PAGE_SIZE, "%s\n", tmp);
> 
> No need to use the stack:
> 
> ib_get_device_fw_str(dev, buf, PAGE_SIZE);
> strlcat(buf, "", PAGE_SIZE);
> return strlen(buf);

That works.  Fixes Leon's comment as well.

Done.

> 
> >  	&dev_attr_node_guid,
> > -	&dev_attr_node_desc
> > +	&dev_attr_node_desc,
> > +	&dev_attr_fw_ver
> 
> Should be 
>  +	&dev_attr_fw_ver,
> 
> Note trailing comma. Avoids git churn like the above on append.

Done.

Ira


> 
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny June 14, 2016, 2:56 a.m. UTC | #4 
On Fri, Jun 10, 2016 at 05:00:59PM +0300, Leon Romanovsky wrote:
> On Thu, Jun 09, 2016 at 04:40:38PM -0600, Jason Gunthorpe wrote:
> > On Thu, Jun 09, 2016 at 05:54:07PM -0400, ira.weiny@intel.com wrote:
> > >  
> > > +static ssize_t show_fw_ver(struct device *device, struct device_attribute *attr,
> > > +			   char *buf)
> > > +{
> > > +	char tmp[ETHTOOL_FWVERS_LEN];
> 
> Very minor request,
> Can we avoid variable names like this?
> fw_str can be a good candidate for the name.
>

Sure, but Jason's comment removes the need of this variable.  I'll remember for
next time.

Ira

> 
> 
> > > +	struct ib_device *dev = container_of(device, struct ib_device, dev);
> > > +
> > > +	ib_get_device_fw_str(dev, tmp, sizeof(tmp));
> > > +
> > > +	return scnprintf(buf, PAGE_SIZE, "%s\n", tmp);
> > 
> > No need to use the stack:
> > 
> > ib_get_device_fw_str(dev, buf, PAGE_SIZE);
> > strlcat(buf, "", PAGE_SIZE);
> > return strlen(buf);
> > 
> > >  	&dev_attr_node_guid,
> > > -	&dev_attr_node_desc
> > > +	&dev_attr_node_desc,
> > > +	&dev_attr_fw_ver
> > 
> > Should be 
> >  +	&dev_attr_fw_ver,
> > 
> > Note trailing comma. Avoids git churn like the above on append.
> > 
> > Jason
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index 5e573bb18660..462dbb64d71d 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -38,6 +38,7 @@ 
 #include <linux/stat.h>
 #include <linux/string.h>
 #include <linux/netdevice.h>
+#include <linux/ethtool.h>
 
 #include <rdma/ib_mad.h>
 #include <rdma/ib_pma.h>
@@ -1188,16 +1189,29 @@  static ssize_t set_node_desc(struct device *device,
 	return count;
 }
 
+static ssize_t show_fw_ver(struct device *device, struct device_attribute *attr,
+			   char *buf)
+{
+	char tmp[ETHTOOL_FWVERS_LEN];
+	struct ib_device *dev = container_of(device, struct ib_device, dev);
+
+	ib_get_device_fw_str(dev, tmp, sizeof(tmp));
+
+	return scnprintf(buf, PAGE_SIZE, "%s\n", tmp);
+}
+
 static DEVICE_ATTR(node_type, S_IRUGO, show_node_type, NULL);
 static DEVICE_ATTR(sys_image_guid, S_IRUGO, show_sys_image_guid, NULL);
 static DEVICE_ATTR(node_guid, S_IRUGO, show_node_guid, NULL);
 static DEVICE_ATTR(node_desc, S_IRUGO | S_IWUSR, show_node_desc, set_node_desc);
+static DEVICE_ATTR(fw_ver, S_IRUGO, show_fw_ver, NULL);
 
 static struct device_attribute *ib_class_attributes[] = {
 	&dev_attr_node_type,
 	&dev_attr_sys_image_guid,
 	&dev_attr_node_guid,
-	&dev_attr_node_desc
+	&dev_attr_node_desc,
+	&dev_attr_fw_ver
 };
 
 static void free_port_list_attributes(struct ib_device *device)