| Message ID | 1465971728-24104-2-git-send-email-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Jun 15, 2016 at 02:21:56AM -0400, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > Allow for a common core function to get firmware version strings > from the individual devices. > > In later patches this format can then then be used to pass a > properly formated version string through the IPoIB layer. > > The problem with the current code in the IPoIB layer is that it is > specific to certain hardware types. > > Furthermore, this gives us a common function through which the core > can provide a common sysfs entry. Eventually we may want to > remove the sysfs export but this provides for user space backwards > compatibility. > > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes from V0: > Add parameter names to function signature > change default string assignment > > drivers/infiniband/core/device.c | 9 +++++++++ > include/rdma/ib_verbs.h | 3 +++ > 2 files changed, 12 insertions(+) > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > index 5c155fa91eec..760ef603a468 100644 > --- a/drivers/infiniband/core/device.c > +++ b/drivers/infiniband/core/device.c > @@ -311,6 +311,15 @@ static int read_port_immutable(struct ib_device *device) > return 0; > } > > +void ib_get_device_fw_str(struct ib_device *dev, char *str, size_t str_len) > +{ > + if (dev->get_dev_fw_str) > + dev->get_dev_fw_str(dev, str, str_len); > + else > + str[0] = '\0'; Interesting point, what is better for the caller to get as a version? You chose empty string, but it can be all Fs or all 0s. > +} > +EXPORT_SYMBOL(ib_get_device_fw_str);
On Wed, Jun 15, 2016 at 09:40:08AM +0300, Leon Romanovsky wrote: > On Wed, Jun 15, 2016 at 02:21:56AM -0400, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> [snip] > > > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > > index 5c155fa91eec..760ef603a468 100644 > > --- a/drivers/infiniband/core/device.c > > +++ b/drivers/infiniband/core/device.c > > @@ -311,6 +311,15 @@ static int read_port_immutable(struct ib_device *device) > > return 0; > > } > > > > +void ib_get_device_fw_str(struct ib_device *dev, char *str, size_t str_len) > > +{ > > + if (dev->get_dev_fw_str) > > + dev->get_dev_fw_str(dev, str, str_len); > > + else > > + str[0] = '\0'; > > Interesting point, what is better for the caller to get as a version? > You chose empty string, but it can be all Fs or all 0s. The default here is that a device does not provide a firmware string. In that case the assumption is that there is no firmware. Hence the empty string. I don't think that reporting 0's or F's are appropriate if a device does not have firmware. In the cases you speak of I would expect the device to supply the firmware function and report all 0s or all Fs for whatever version that may represent. (I'm assuming that would be some sort of debug or unreleased firmware...) I could just as well done something like. snprintf(str, str_len, "<none>"); Which may be more informative? I just don't want to get too wrapped up in the policy here. I think this is best left to the driver/hw vendor. Ira > > > +} > > +EXPORT_SYMBOL(ib_get_device_fw_str); -- 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
On Wed, Jun 15, 2016 at 11:20:49AM -0400, ira.weiny wrote: > The default here is that a device does not provide a firmware string. In that > case the assumption is that there is no firmware. Hence the empty string. I > don't think that reporting 0's or F's are appropriate if a device does not have > firmware. > > In the cases you speak of I would expect the device to supply the firmware > function and report all 0s or all Fs for whatever version that may represent. > (I'm assuming that would be some sort of debug or unreleased firmware...) > > I could just as well done something like. > > snprintf(str, str_len, "<none>"); Just don't provide a 'fw' sysfs file at all if the driver cannot support it. 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
On Wed, Jun 15, 2016 at 10:51:06AM -0600, Jason Gunthorpe wrote: > On Wed, Jun 15, 2016 at 11:20:49AM -0400, ira.weiny wrote: > > The default here is that a device does not provide a firmware string. In that > > case the assumption is that there is no firmware. Hence the empty string. I > > don't think that reporting 0's or F's are appropriate if a device does not have > > firmware. > > > > In the cases you speak of I would expect the device to supply the firmware > > function and report all 0s or all Fs for whatever version that may represent. > > (I'm assuming that would be some sort of debug or unreleased firmware...) > > > > I could just as well done something like. > > > > snprintf(str, str_len, "<none>"); > > Just don't provide a 'fw' sysfs file at all if the driver cannot > support it. This function is supporting both sysfs and ethtool. ethtool_drvinfo->fw_version is normally up to the device. I've just provided a default. For example I see "N/A", "n/a", and "none" all used in the network stack. ./ethernet/freescale/gianfar_ethtool.c: strlcpy(drvinfo->fw_version, "N/A", sizeof(drvinfo->fw_version)); ./rionet.c: strlcpy(info->fw_version, "n/a", sizeof(info->fw_version)); ./fjes/fjes_ethtool.c: strlcpy(drvinfo->fw_version, "none", sizeof(drvinfo->fw_version)); I really think we are in the weeds here and are fine to leave it blank. But if you wish, I can force all drivers to provide the string and then they can put something in like the above. Ira -- 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
On Wed, Jun 15, 2016 at 07:26:03PM -0400, ira.weiny wrote: > On Wed, Jun 15, 2016 at 10:51:06AM -0600, Jason Gunthorpe wrote: > > On Wed, Jun 15, 2016 at 11:20:49AM -0400, ira.weiny wrote: > > > The default here is that a device does not provide a firmware string. In that > > > case the assumption is that there is no firmware. Hence the empty string. I > > > don't think that reporting 0's or F's are appropriate if a device does not have > > > firmware. > > > > > > In the cases you speak of I would expect the device to supply the firmware > > > function and report all 0s or all Fs for whatever version that may represent. > > > (I'm assuming that would be some sort of debug or unreleased firmware...) > > > > > > I could just as well done something like. > > > > > > snprintf(str, str_len, "<none>"); > > > > Just don't provide a 'fw' sysfs file at all if the driver cannot > > support it. > > This function is supporting both sysfs and ethtool. > > ethtool_drvinfo->fw_version is normally up to the device. I've just provided a > default. > > For example I see "N/A", "n/a", and "none" all used in the network stack. > > ./ethernet/freescale/gianfar_ethtool.c: strlcpy(drvinfo->fw_version, "N/A", sizeof(drvinfo->fw_version)); > > ./rionet.c: strlcpy(info->fw_version, "n/a", sizeof(info->fw_version)); > > ./fjes/fjes_ethtool.c: strlcpy(drvinfo->fw_version, "none", sizeof(drvinfo->fw_version)); > > I really think we are in the weeds here and are fine to leave it blank. I'm fine with that. > > But if you wish, I can force all drivers to provide the string and then they can > put something in like the above. > > Ira >
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 5c155fa91eec..760ef603a468 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -311,6 +311,15 @@ static int read_port_immutable(struct ib_device *device) return 0; } +void ib_get_device_fw_str(struct ib_device *dev, char *str, size_t str_len) +{ + if (dev->get_dev_fw_str) + dev->get_dev_fw_str(dev, str, str_len); + else + str[0] = '\0'; +} +EXPORT_SYMBOL(ib_get_device_fw_str); + /** * ib_register_device - Register an IB device with IB core * @device:Device to register diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 7e440d41487a..1dc3d0d90202 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -1956,6 +1956,7 @@ struct ib_device { * in fast paths. */ int (*get_port_immutable)(struct ib_device *, u8, struct ib_port_immutable *); + void (*get_dev_fw_str)(struct ib_device *, char *str, size_t str_len); }; struct ib_client { @@ -1991,6 +1992,8 @@ struct ib_client { struct ib_device *ib_alloc_device(size_t size); void ib_dealloc_device(struct ib_device *device); +void ib_get_device_fw_str(struct ib_device *device, char *str, size_t str_len); + int ib_register_device(struct ib_device *device, int (*port_callback)(struct ib_device *, u8, struct kobject *));