diff mbox

[RFC,1/2] net: ethtool: Add capability to retrieve plug-in module EEPROM

Message ID 4F71FE0A.3060203@solarflare.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Stuart Hodgson March 27, 2012, 5:51 p.m. UTC
Added extensions to the ethtool API to obtain plugin module eeprom data
This is useful for end users to be able to determine what the capabilities
of the in use module are.

The first provides a new struct ethtool_modinfo that will return the
type and size of plug-in module eeprom (such as SFP+) for parsing
by userland program.

The second provides the API to get the raw eeprom information
using the existing ethtool_eeprom structture to return the data

Signed-off-by: Stuart Hodgson <smhodgson@solarflare.com>
---
  include/linux/ethtool.h |   20 ++++++++++++
  net/core/ethtool.c      |   79 
+++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 99 insertions(+), 0 deletions(-)

Comments

Ben Hutchings April 2, 2012, 5:52 p.m. UTC | #1
We previously discussed the need for this in person, so I'm going to
review this as a submission rather than an RFC.

On Tue, 2012-03-27 at 18:51 +0100, Stuart Hodgson wrote:
> Added extensions to the ethtool API to obtain plugin module eeprom data
> This is useful for end users to be able to determine what the capabilities
> of the in use module are.
> 
> The first provides a new struct ethtool_modinfo that will return the
> type and size of plug-in module eeprom (such as SFP+) for parsing
> by userland program.
> 
> The second provides the API to get the raw eeprom information
> using the existing ethtool_eeprom structture to return the data
> 
> Signed-off-by: Stuart Hodgson <smhodgson@solarflare.com>
> ---
>   include/linux/ethtool.h |   20 ++++++++++++
>   net/core/ethtool.c      |   79 
> +++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 99 insertions(+), 0 deletions(-)

This is line-wrapped; you'll need to take care to avoid that when
submitting the patch for real.  Tabs have been converted to spaces,
which you also need to avoid.  See Documentation/email-clients.txt.

> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index da5b2de..50eaf35 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -117,6 +117,14 @@ struct ethtool_eeprom {
>       __u8    data[0];
>   };
>
> +/* for passing plug-in module information */
> +struct ethtool_modinfo {
> +    __u32   cmd;
> +    __u32   type;
> +    __u32   eeprom_len;
> +    __u32   reserved[8];
> +};
> +

This needs a proper kernel-doc comment with a description of each of the
fields (except reserved).

>   /**
>    * struct ethtool_coalesce - coalescing parameters for IRQs and stats 
> updates
>    * @cmd: ETHTOOL_{G,S}COALESCE
> @@ -936,6 +944,10 @@ struct ethtool_ops {
>       int    (*get_dump_data)(struct net_device *,
>                    struct ethtool_dump *, void *);
>       int    (*set_dump)(struct net_device *, struct ethtool_dump *);
> +    int    (*get_module_info)(struct net_device *,
> +                   struct ethtool_modinfo *);
> +    int    (*get_module_eeprom)(struct net_device *,
> +                     struct ethtool_eeprom *, u8 *);

These need to be described in the kernel-doc comment for this structure.

>   };
>   #endif /* __KERNEL__ */
> @@ -1010,6 +1022,8 @@ struct ethtool_ops {
>   #define ETHTOOL_SET_DUMP    0x0000003e /* Set dump settings */
>   #define ETHTOOL_GET_DUMP_FLAG    0x0000003f /* Get dump settings */
>   #define ETHTOOL_GET_DUMP_DATA    0x00000040 /* Get dump data */
> +#define ETHTOOL_GMODULEINFO    0x00000041 /* Get plug-in module 
> information */
> +#define ETHTOOL_GMODULEEEPROM    0x00000042 /* Get plug-in module eeprom */
> 
>   /* compatibility with older code */
>   #define SPARC_ETH_GSET        ETHTOOL_GSET
> @@ -1159,6 +1173,12 @@ struct ethtool_ops {
>   #define RX_CLS_LOC_FIRST    0xfffffffe
>   #define RX_CLS_LOC_LAST        0xfffffffd
> 
> +/* EEPROM Standards for plug in modules */
> +#define SFF_8079        0x1
> +#define SFF_8079_LEN        256
> +#define SFF_8472        0x2
> +#define SFF_8472_LEN        512
> +

I think it's best to include a prefix of 'ethtool_' 'ETHTOOL_' or 'ETH_'
in new definitions in ethtool.h.  Since these are specific to modules I
would suggest a prefix of 'ETH_MODULE_'.

>   /* Reset flags */
>   /* The reset() operation must clear the flags for the components which
>    * were actually reset.  On successful return, the flags indicate the
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 3f79db1..1e86bd9 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
[...]
> +static int ethtool_get_module_eeprom(struct net_device *dev,
> +        void __user *useraddr)
> +{
> +    int ret;
> +    struct ethtool_eeprom eeprom;
> +    struct ethtool_modinfo modinfo;
> +    const struct ethtool_ops *ops = dev->ethtool_ops;
> +    void __user *userbuf = useraddr + sizeof(eeprom);
> +    u8 *data;
> +
> +    if (!ops->get_module_info || !ops->get_module_eeprom)
> +        return -EOPNOTSUPP;
> +
> +    if (copy_from_user(&eeprom, useraddr, sizeof(eeprom)))
> +        return -EFAULT;
> +
> +    /* Check for wrap and zero */
> +    if (eeprom.offset + eeprom.len <= eeprom.offset)
> +        return -EINVAL;
> +
> +    /* Get the modinfo to get the length */
> +    ret = ops->get_module_info(dev, &modinfo);
> +    if (ret)
> +        return ret;
> +
> +    if (eeprom.offset + eeprom.len > modinfo.eeprom_len)
> +        return -EINVAL;
> +
> +    data = kmalloc(PAGE_SIZE, GFP_USER);
> +    if (!data)
> +        return -ENOMEM;

What if some device has a larger EEPROM?  Surely this length should be
eeprom.len.

> +    ret = ops->get_module_eeprom(dev, &eeprom, data);
> +    if (ret)
> +        goto out;
> +
> +
> +    if (copy_to_user(userbuf, data, eeprom.len)) {
> +        ret = -EFAULT;
> +        goto out;
> +    }
> +
> +    if (copy_to_user(useraddr, &eeprom, sizeof(eeprom)))
> +        ret = -EFAULT;
[...]

I think you can drop this last copy as there's no information to return
in the eeprom structure itself.

Ben.
Ben Hutchings April 2, 2012, 6:14 p.m. UTC | #2
On Mon, 2012-04-02 at 18:52 +0100, Ben Hutchings wrote:
[...]
> > +    ret = ops->get_module_eeprom(dev, &eeprom, data);
> > +    if (ret)
> > +        goto out;
> > +
> > +
> > +    if (copy_to_user(userbuf, data, eeprom.len)) {
> > +        ret = -EFAULT;
> > +        goto out;
> > +    }
> > +
> > +    if (copy_to_user(useraddr, &eeprom, sizeof(eeprom)))
> > +        ret = -EFAULT;
> [...]
> 
> I think you can drop this last copy as there's no information to return
> in the eeprom structure itself.
[...]

This is not the case because we need to cover short reads.

Ben.
Stuart Hodgson April 11, 2012, 4:50 p.m. UTC | #3
On 02/04/12 18:52, Ben Hutchings wrote:
> We previously discussed the need for this in person, so I'm going to
> review this as a submission rather than an RFC.
>
> On Tue, 2012-03-27 at 18:51 +0100, Stuart Hodgson wrote:
>> Added extensions to the ethtool API to obtain plugin module eeprom data
>> This is useful for end users to be able to determine what the capabilities
>> of the in use module are.
>>
>> The first provides a new struct ethtool_modinfo that will return the
>> type and size of plug-in module eeprom (such as SFP+) for parsing
>> by userland program.
>>
>> The second provides the API to get the raw eeprom information
>> using the existing ethtool_eeprom structture to return the data
>>
>> Signed-off-by: Stuart Hodgson<smhodgson@solarflare.com>
>> ---
>>    include/linux/ethtool.h |   20 ++++++++++++
>>    net/core/ethtool.c      |   79
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>    2 files changed, 99 insertions(+), 0 deletions(-)
>
> This is line-wrapped; you'll need to take care to avoid that when
> submitting the patch for real.  Tabs have been converted to spaces,
> which you also need to avoid.  See Documentation/email-clients.txt.
>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index da5b2de..50eaf35 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -117,6 +117,14 @@ struct ethtool_eeprom {
>>        __u8    data[0];
>>    };
>>
>> +/* for passing plug-in module information */
>> +struct ethtool_modinfo {
>> +    __u32   cmd;
>> +    __u32   type;
>> +    __u32   eeprom_len;
>> +    __u32   reserved[8];
>> +};
>> +
>
> This needs a proper kernel-doc comment with a description of each of the
> fields (except reserved).
>
>>    /**
>>     * struct ethtool_coalesce - coalescing parameters for IRQs and stats
>> updates
>>     * @cmd: ETHTOOL_{G,S}COALESCE
>> @@ -936,6 +944,10 @@ struct ethtool_ops {
>>        int    (*get_dump_data)(struct net_device *,
>>                     struct ethtool_dump *, void *);
>>        int    (*set_dump)(struct net_device *, struct ethtool_dump *);
>> +    int    (*get_module_info)(struct net_device *,
>> +                   struct ethtool_modinfo *);
>> +    int    (*get_module_eeprom)(struct net_device *,
>> +                     struct ethtool_eeprom *, u8 *);
>
> These need to be described in the kernel-doc comment for this structure.
>
>>    };
>>    #endif /* __KERNEL__ */
>> @@ -1010,6 +1022,8 @@ struct ethtool_ops {
>>    #define ETHTOOL_SET_DUMP    0x0000003e /* Set dump settings */
>>    #define ETHTOOL_GET_DUMP_FLAG    0x0000003f /* Get dump settings */
>>    #define ETHTOOL_GET_DUMP_DATA    0x00000040 /* Get dump data */
>> +#define ETHTOOL_GMODULEINFO    0x00000041 /* Get plug-in module
>> information */
>> +#define ETHTOOL_GMODULEEEPROM    0x00000042 /* Get plug-in module eeprom */
>>
>>    /* compatibility with older code */
>>    #define SPARC_ETH_GSET        ETHTOOL_GSET
>> @@ -1159,6 +1173,12 @@ struct ethtool_ops {
>>    #define RX_CLS_LOC_FIRST    0xfffffffe
>>    #define RX_CLS_LOC_LAST        0xfffffffd
>>
>> +/* EEPROM Standards for plug in modules */
>> +#define SFF_8079        0x1
>> +#define SFF_8079_LEN        256
>> +#define SFF_8472        0x2
>> +#define SFF_8472_LEN        512
>> +
>
> I think it's best to include a prefix of 'ethtool_' 'ETHTOOL_' or 'ETH_'
> in new definitions in ethtool.h.  Since these are specific to modules I
> would suggest a prefix of 'ETH_MODULE_'.
>
>>    /* Reset flags */
>>    /* The reset() operation must clear the flags for the components which
>>     * were actually reset.  On successful return, the flags indicate the
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index 3f79db1..1e86bd9 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
> [...]
>> +static int ethtool_get_module_eeprom(struct net_device *dev,
>> +        void __user *useraddr)
>> +{
>> +    int ret;
>> +    struct ethtool_eeprom eeprom;
>> +    struct ethtool_modinfo modinfo;
>> +    const struct ethtool_ops *ops = dev->ethtool_ops;
>> +    void __user *userbuf = useraddr + sizeof(eeprom);
>> +    u8 *data;
>> +
>> +    if (!ops->get_module_info || !ops->get_module_eeprom)
>> +        return -EOPNOTSUPP;
>> +
>> +    if (copy_from_user(&eeprom, useraddr, sizeof(eeprom)))
>> +        return -EFAULT;
>> +
>> +    /* Check for wrap and zero */
>> +    if (eeprom.offset + eeprom.len<= eeprom.offset)
>> +        return -EINVAL;
>> +
>> +    /* Get the modinfo to get the length */
>> +    ret = ops->get_module_info(dev,&modinfo);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (eeprom.offset + eeprom.len>  modinfo.eeprom_len)
>> +        return -EINVAL;
>> +
>> +    data = kmalloc(PAGE_SIZE, GFP_USER);
>> +    if (!data)
>> +        return -ENOMEM;
>
> What if some device has a larger EEPROM?  Surely this length should be
> eeprom.len.
>

Do you mean what if the eeprom length in te device is larger than
PAGE_SIZE? If so then it should really use modinfo.eeprom_len since
this the size of the data. eeprom.len could be arbitary.

>> +    ret = ops->get_module_eeprom(dev,&eeprom, data);
>> +    if (ret)
>> +        goto out;
>> +
>> +
>> +    if (copy_to_user(userbuf, data, eeprom.len)) {
>> +        ret = -EFAULT;
>> +        goto out;
>> +    }
>> +
>> +    if (copy_to_user(useraddr,&eeprom, sizeof(eeprom)))
>> +        ret = -EFAULT;
> [...]
>
> I think you can drop this last copy as there's no information to return
> in the eeprom structure itself.
>
> Ben.
>

Other comments have been addressed and will be re-submitted.

Stu
--
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 April 11, 2012, 6:16 p.m. UTC | #4
On Wed, 2012-04-11 at 17:50 +0100, Stuart Hodgson wrote:
> On 02/04/12 18:52, Ben Hutchings wrote:
[...]
> >> --- a/net/core/ethtool.c
> >> +++ b/net/core/ethtool.c
[...]
> >> +    if (eeprom.offset + eeprom.len>  modinfo.eeprom_len)
> >> +        return -EINVAL;
> >> +
> >> +    data = kmalloc(PAGE_SIZE, GFP_USER);
> >> +    if (!data)
> >> +        return -ENOMEM;
> >
> > What if some device has a larger EEPROM?  Surely this length should be
> > eeprom.len.
> >
> 
> Do you mean what if the eeprom length in te device is larger than
> PAGE_SIZE?

Yes.

> If so then it should really use modinfo.eeprom_len since
> this the size of the data. eeprom.len could be arbitary.

No, eeprom.len is the size of the data and we've already validated it at
this point.

Ben.


--
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
diff mbox

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index da5b2de..50eaf35 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -117,6 +117,14 @@  struct ethtool_eeprom {
      __u8    data[0];
  };

+/* for passing plug-in module information */
+struct ethtool_modinfo {
+    __u32   cmd;
+    __u32   type;
+    __u32   eeprom_len;
+    __u32   reserved[8];
+};
+
  /**
   * struct ethtool_coalesce - coalescing parameters for IRQs and stats 
updates
   * @cmd: ETHTOOL_{G,S}COALESCE
@@ -936,6 +944,10 @@  struct ethtool_ops {
      int    (*get_dump_data)(struct net_device *,
                   struct ethtool_dump *, void *);
      int    (*set_dump)(struct net_device *, struct ethtool_dump *);
+    int    (*get_module_info)(struct net_device *,
+                   struct ethtool_modinfo *);
+    int    (*get_module_eeprom)(struct net_device *,
+                     struct ethtool_eeprom *, u8 *);

  };
  #endif /* __KERNEL__ */
@@ -1010,6 +1022,8 @@  struct ethtool_ops {
  #define ETHTOOL_SET_DUMP    0x0000003e /* Set dump settings */
  #define ETHTOOL_GET_DUMP_FLAG    0x0000003f /* Get dump settings */
  #define ETHTOOL_GET_DUMP_DATA    0x00000040 /* Get dump data */
+#define ETHTOOL_GMODULEINFO    0x00000041 /* Get plug-in module 
information */
+#define ETHTOOL_GMODULEEEPROM    0x00000042 /* Get plug-in module eeprom */

  /* compatibility with older code */
  #define SPARC_ETH_GSET        ETHTOOL_GSET
@@ -1159,6 +1173,12 @@  struct ethtool_ops {
  #define RX_CLS_LOC_FIRST    0xfffffffe
  #define RX_CLS_LOC_LAST        0xfffffffd

+/* EEPROM Standards for plug in modules */
+#define SFF_8079        0x1
+#define SFF_8079_LEN        256
+#define SFF_8472        0x2
+#define SFF_8472_LEN        512
+
  /* Reset flags */
  /* The reset() operation must clear the flags for the components which
   * were actually reset.  On successful return, the flags indicate the
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 3f79db1..1e86bd9 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1276,6 +1276,79 @@  out:
      return ret;
  }

+static int ethtool_get_module_info(struct net_device *dev,
+        void __user *useraddr)
+{
+    int ret;
+    struct ethtool_modinfo modinfo;
+    const struct ethtool_ops *ops = dev->ethtool_ops;
+
+    if (!ops->get_module_info)
+        return -EOPNOTSUPP;
+
+    if (copy_from_user(&modinfo, useraddr, sizeof(modinfo)))
+        return -EFAULT;
+
+    ret = ops->get_module_info(dev, &modinfo);
+    if (ret)
+        return ret;
+
+    if (copy_to_user(useraddr, &modinfo, sizeof(modinfo)))
+        return -EFAULT;
+
+    return 0;
+}
+
+static int ethtool_get_module_eeprom(struct net_device *dev,
+        void __user *useraddr)
+{
+    int ret;
+    struct ethtool_eeprom eeprom;
+    struct ethtool_modinfo modinfo;
+    const struct ethtool_ops *ops = dev->ethtool_ops;
+    void __user *userbuf = useraddr + sizeof(eeprom);
+    u8 *data;
+
+    if (!ops->get_module_info || !ops->get_module_eeprom)
+        return -EOPNOTSUPP;
+
+    if (copy_from_user(&eeprom, useraddr, sizeof(eeprom)))
+        return -EFAULT;
+
+    /* Check for wrap and zero */
+    if (eeprom.offset + eeprom.len <= eeprom.offset)
+        return -EINVAL;
+
+    /* Get the modinfo to get the length */
+    ret = ops->get_module_info(dev, &modinfo);
+    if (ret)
+        return ret;
+
+    if (eeprom.offset + eeprom.len > modinfo.eeprom_len)
+        return -EINVAL;
+
+    data = kmalloc(PAGE_SIZE, GFP_USER);
+    if (!data)
+        return -ENOMEM;
+
+    ret = ops->get_module_eeprom(dev, &eeprom, data);
+    if (ret)
+        goto out;
+
+
+    if (copy_to_user(userbuf, data, eeprom.len)) {
+        ret = -EFAULT;
+        goto out;
+    }
+
+    if (copy_to_user(useraddr, &eeprom, sizeof(eeprom)))
+        ret = -EFAULT;
+
+out:
+    kfree(data);
+    return ret;
+}
+
  /* The main entry point in this file.  Called from net/core/dev.c */

  int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -1494,6 +1567,12 @@  int dev_ethtool(struct net *net, struct ifreq *ifr)
      case ETHTOOL_GET_DUMP_DATA:
          rc = ethtool_get_dump_data(dev, useraddr);
          break;
+    case ETHTOOL_GMODULEINFO:
+        rc = ethtool_get_module_info(dev, useraddr);
+        break;
+    case ETHTOOL_GMODULEEEPROM:
+        rc = ethtool_get_module_eeprom(dev, useraddr);
+        break;
      default:
          rc = -EOPNOTSUPP;
      }