diff mbox

[3/4] drivers/net: enic: Make ASIC information available to USNIC

Message ID 1376071941-17001-4-git-send-email-neepatel@cisco.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Neel Patel Aug. 9, 2013, 6:12 p.m. UTC 
This patch provides asic information via ethtool.

Signed-off-by: Neel Patel <neepatel@cisco.com>
Signed-off-by: Nishank Trivedi <nistrive@cisco.com>
Signed-off-by: Christian Benvenuti <benve@cisco.com>
---
 drivers/net/ethernet/cisco/enic/driver_utils.h | 49 ++++++++++++++++++++++++++
 drivers/net/ethernet/cisco/enic/enic_ethtool.c |  4 +++
 2 files changed, 53 insertions(+)
 create mode 100644 drivers/net/ethernet/cisco/enic/driver_utils.h

Comments

Stephen Hemminger Aug. 9, 2013, 10:21 p.m. UTC | #1 
On Fri,  9 Aug 2013 11:12:20 -0700
Neel Patel <neepatel@cisco.com> wrote:

> This patch provides asic information via ethtool.
> 
> Signed-off-by: Neel Patel <neepatel@cisco.com>
> Signed-off-by: Nishank Trivedi <nistrive@cisco.com>
> Signed-off-by: Christian Benvenuti <benve@cisco.com>
> ---
>  drivers/net/ethernet/cisco/enic/driver_utils.h | 49 ++++++++++++++++++++++++++
>  drivers/net/ethernet/cisco/enic/enic_ethtool.c |  4 +++
>  2 files changed, 53 insertions(+)
>  create mode 100644 drivers/net/ethernet/cisco/enic/driver_utils.h
> 
> diff --git a/drivers/net/ethernet/cisco/enic/driver_utils.h b/drivers/net/ethernet/cisco/enic/driver_utils.h
> new file mode 100644
> index 0000000..e654b4d
> --- /dev/null
> +++ b/drivers/net/ethernet/cisco/enic/driver_utils.h
> @@ -0,0 +1,49 @@
> +/**
> + * Copyright 2013 Cisco Systems, Inc.  All rights reserved.
> + *
> + * This program is free software; you may redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + */
> +
> +#ifndef __DRIVER_UTILS_H__
> +#define __DRIVER_UTILS_H__
> +
> +#include <linux/string.h>
> +
> +static inline int driver_encode_asic_info(char *str, int strlen, u16 asic_type,
> +				u16 asic_rev)
> +{
> +	if (strlen < sizeof(asic_type) + sizeof(asic_rev))
> +		return -EINVAL;
> +
> +	memcpy(str, &asic_type, sizeof(asic_type));
> +	memcpy(str + sizeof(asic_type), &asic_rev, sizeof(asic_rev));
> +
> +	return 0;
> +}
> +
> +static inline int driver_decode_asic_info(char *str, int strlen, u16 *asic_type,
> +						u16 *asic_rev)
> +{
> +	if (strlen < sizeof(*asic_type) + sizeof(*asic_rev))
> +		return -EINVAL;
> +
> +	if (asic_type)
> +		memcpy(asic_type, str, sizeof(*asic_type));
> +	if (asic_rev)
> +		memcpy(asic_rev, str + sizeof(*asic_type), sizeof(*asic_rev));
> +	return 0;
> +}
> +
> +#endif /*!__DRIVER_UTILS_H__*/
> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> index 47e3562..c5d938a 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> @@ -19,6 +19,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/ethtool.h>
>  
> +#include "driver_utils.h"
>  #include "enic_res.h"
>  #include "enic.h"
>  #include "enic_dev.h"
> @@ -116,6 +117,9 @@ static void enic_get_drvinfo(struct net_device *netdev,
>  		sizeof(drvinfo->fw_version));
>  	strlcpy(drvinfo->bus_info, pci_name(enic->pdev),
>  		sizeof(drvinfo->bus_info));
> +	memset(drvinfo->reserved1, 0, sizeof(drvinfo->reserved1));
> +	driver_encode_asic_info(drvinfo->reserved1, sizeof(drvinfo->reserved1),
> +		fw_info->asic_type, fw_info->asic_rev);
>  }

If you want to use a reserved field, then make it a first class citizen.
Rename it to asic_info, make sure the result is okay for other drivers
and add send patch so Ben can make it part of normal ethtool support.

Otherwise, this code is likely to break when someone else actually unreserves
that field.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings Aug. 10, 2013, 4:13 p.m. UTC | #2 
On Fri, 2013-08-09 at 15:21 -0700, Stephen Hemminger wrote:
> On Fri,  9 Aug 2013 11:12:20 -0700
> Neel Patel <neepatel@cisco.com> wrote:
> 
> > This patch provides asic information via ethtool.
[...]
> > --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> > +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/ethtool.h>
> >  
> > +#include "driver_utils.h"
> >  #include "enic_res.h"
> >  #include "enic.h"
> >  #include "enic_dev.h"
> > @@ -116,6 +117,9 @@ static void enic_get_drvinfo(struct net_device *netdev,
> >  		sizeof(drvinfo->fw_version));
> >  	strlcpy(drvinfo->bus_info, pci_name(enic->pdev),
> >  		sizeof(drvinfo->bus_info));
> > +	memset(drvinfo->reserved1, 0, sizeof(drvinfo->reserved1));
> > +	driver_encode_asic_info(drvinfo->reserved1, sizeof(drvinfo->reserved1),
> > +		fw_info->asic_type, fw_info->asic_rev);
> >  }
> 
> If you want to use a reserved field, then make it a first class citizen.
> Rename it to asic_info, make sure the result is okay for other drivers
> and add send patch so Ben can make it part of normal ethtool support.
> 
> Otherwise, this code is likely to break when someone else actually unreserves
> that field.

Right.  I bet this is redundant with the IDs that lspci can show,
anyway.

Ben.
Nishank Trivedi Aug. 13, 2013, 5:10 a.m. UTC | #3 
On 8/10/13 9:13 AM, Ben Hutchings wrote:
> On Fri, 2013-08-09 at 15:21 -0700, Stephen Hemminger wrote:
>> On Fri,  9 Aug 2013 11:12:20 -0700
>> Neel Patel <neepatel@cisco.com> wrote:
>>
>>> This patch provides asic information via ethtool.
> [...]
>>> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>>> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>>> @@ -19,6 +19,7 @@
>>>   #include <linux/netdevice.h>
>>>   #include <linux/ethtool.h>
>>>
>>> +#include "driver_utils.h"
>>>   #include "enic_res.h"
>>>   #include "enic.h"
>>>   #include "enic_dev.h"
>>> @@ -116,6 +117,9 @@ static void enic_get_drvinfo(struct net_device *netdev,
>>>   		sizeof(drvinfo->fw_version));
>>>   	strlcpy(drvinfo->bus_info, pci_name(enic->pdev),
>>>   		sizeof(drvinfo->bus_info));
>>> +	memset(drvinfo->reserved1, 0, sizeof(drvinfo->reserved1));
>>> +	driver_encode_asic_info(drvinfo->reserved1, sizeof(drvinfo->reserved1),
>>> +		fw_info->asic_type, fw_info->asic_rev);
>>>   }
>>
>> If you want to use a reserved field, then make it a first class citizen.
>> Rename it to asic_info, make sure the result is okay for other drivers
>> and add send patch so Ben can make it part of normal ethtool support.
>>
>> Otherwise, this code is likely to break when someone else actually unreserves
>> that field.
>
> Right.  I bet this is redundant with the IDs that lspci can show,
> anyway.

Thanks Stephen, Ben for your input, they are valid points. Neel would 
send a new patch series minus 3/4 for now.

While you are right that lspci or sysfs can be used to get same info, we 
were trying to use asic info (encoded with type and version) within 
drvinfo so as to use one string to achieve same effect as reading PCI 
subsystem id and revision explicitly. Instead of going to different tool 
(lspci), ethtool would be enough to unqiuely identify the device. Asic 
version along with already existing firmware version, driver version, 
etc seems natural.

Thanks,
nishank


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings Aug. 13, 2013, 8:31 a.m. UTC | #4 
On Mon, 2013-08-12 at 22:10 -0700, Nishank Trivedi wrote:
> On 8/10/13 9:13 AM, Ben Hutchings wrote:
> > On Fri, 2013-08-09 at 15:21 -0700, Stephen Hemminger wrote:
> >> On Fri,  9 Aug 2013 11:12:20 -0700
> >> Neel Patel <neepatel@cisco.com> wrote:
> >>
> >>> This patch provides asic information via ethtool.
> > [...]
> >>> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> >>> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> >>> @@ -19,6 +19,7 @@
> >>>   #include <linux/netdevice.h>
> >>>   #include <linux/ethtool.h>
> >>>
> >>> +#include "driver_utils.h"
> >>>   #include "enic_res.h"
> >>>   #include "enic.h"
> >>>   #include "enic_dev.h"
> >>> @@ -116,6 +117,9 @@ static void enic_get_drvinfo(struct net_device *netdev,
> >>>   		sizeof(drvinfo->fw_version));
> >>>   	strlcpy(drvinfo->bus_info, pci_name(enic->pdev),
> >>>   		sizeof(drvinfo->bus_info));
> >>> +	memset(drvinfo->reserved1, 0, sizeof(drvinfo->reserved1));
> >>> +	driver_encode_asic_info(drvinfo->reserved1, sizeof(drvinfo->reserved1),
> >>> +		fw_info->asic_type, fw_info->asic_rev);
> >>>   }
> >>
> >> If you want to use a reserved field, then make it a first class citizen.
> >> Rename it to asic_info, make sure the result is okay for other drivers
> >> and add send patch so Ben can make it part of normal ethtool support.
> >>
> >> Otherwise, this code is likely to break when someone else actually unreserves
> >> that field.
> >
> > Right.  I bet this is redundant with the IDs that lspci can show,
> > anyway.
> 
> Thanks Stephen, Ben for your input, they are valid points. Neel would 
> send a new patch series minus 3/4 for now.
> 
> While you are right that lspci or sysfs can be used to get same info, we 
> were trying to use asic info (encoded with type and version) within 
> drvinfo so as to use one string to achieve same effect as reading PCI 
> subsystem id and revision explicitly. Instead of going to different tool 
> (lspci), ethtool would be enough to unqiuely identify the device. Asic 
> version along with already existing firmware version, driver version, 
> etc seems natural.

Well we've managed without this for the last 15 years, so it seems like
it's not that much of a problem to run lspci too.

Sure, it's an extra command for users to run, but if you're trying to
get diagnostic information from them there's a *lot* more you'll want to
gather.

We could do something in the ethtool *command* to report what the device
is (if bus_info matches PCI address format), which would then work for
every PCI network driver.

Ben.
diff mbox

Patch

diff --git a/drivers/net/ethernet/cisco/enic/driver_utils.h b/drivers/net/ethernet/cisco/enic/driver_utils.h
new file mode 100644
index 0000000..e654b4d
--- /dev/null
+++ b/drivers/net/ethernet/cisco/enic/driver_utils.h
@@ -0,0 +1,49 @@ 
+/**
+ * Copyright 2013 Cisco Systems, Inc.  All rights reserved.
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ */
+
+#ifndef __DRIVER_UTILS_H__
+#define __DRIVER_UTILS_H__
+
+#include <linux/string.h>
+
+static inline int driver_encode_asic_info(char *str, int strlen, u16 asic_type,
+				u16 asic_rev)
+{
+	if (strlen < sizeof(asic_type) + sizeof(asic_rev))
+		return -EINVAL;
+
+	memcpy(str, &asic_type, sizeof(asic_type));
+	memcpy(str + sizeof(asic_type), &asic_rev, sizeof(asic_rev));
+
+	return 0;
+}
+
+static inline int driver_decode_asic_info(char *str, int strlen, u16 *asic_type,
+						u16 *asic_rev)
+{
+	if (strlen < sizeof(*asic_type) + sizeof(*asic_rev))
+		return -EINVAL;
+
+	if (asic_type)
+		memcpy(asic_type, str, sizeof(*asic_type));
+	if (asic_rev)
+		memcpy(asic_rev, str + sizeof(*asic_type), sizeof(*asic_rev));
+	return 0;
+}
+
+#endif /*!__DRIVER_UTILS_H__*/
diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index 47e3562..c5d938a 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -19,6 +19,7 @@ 
 #include <linux/netdevice.h>
 #include <linux/ethtool.h>
 
+#include "driver_utils.h"
 #include "enic_res.h"
 #include "enic.h"
 #include "enic_dev.h"
@@ -116,6 +117,9 @@  static void enic_get_drvinfo(struct net_device *netdev,
 		sizeof(drvinfo->fw_version));
 	strlcpy(drvinfo->bus_info, pci_name(enic->pdev),
 		sizeof(drvinfo->bus_info));
+	memset(drvinfo->reserved1, 0, sizeof(drvinfo->reserved1));
+	driver_encode_asic_info(drvinfo->reserved1, sizeof(drvinfo->reserved1),
+		fw_info->asic_type, fw_info->asic_rev);
 }
 
 static void enic_get_strings(struct net_device *netdev, u32 stringset,