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

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

[Stuart Hodgson]

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

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;
      }
-- 
1.7.7.6

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

[Ben Hutchings]

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, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[Ben Hutchings]

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.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[Stuart Hodgson]

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

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

[Ben Hutchings]

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.

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

[Ben Hutchings]

On Wed, 2012-04-11 at 19:16 +0100, Ben Hutchings wrote:
> 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.

Maybe we should start by refactoring ethtool_get_eeprom() so we can
reuse most of its code in ethtool_get_module_eeprom(), rather than
having to worry about what the maximum size of a module EEPROM might be
and whether we need a loop:

Subject: ethtool: Split ethtool_get_eeprom() to allow for additional EEPROM accessors

We want to support reading module (SFP+, XFP, ...) EEPROMs as well as
NIC EEPROMs.  They will need a different command number and driver
operation, but the structure and arguments will be the same and so we
can share most of the code here.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 net/core/ethtool.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index beacdd9..ca7698f 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -751,18 +751,17 @@ static int ethtool_get_link(struct net_device *dev, char __user *useraddr)
 	return 0;
 }
 
-static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
+static int ethtool_get_any_eeprom(struct net_device *dev, void __user *useraddr,
+				  int (*getter)(struct net_device *,
+						struct ethtool_eeprom *, u8 *),
+				  u32 total_len)
 {
 	struct ethtool_eeprom eeprom;
-	const struct ethtool_ops *ops = dev->ethtool_ops;
 	void __user *userbuf = useraddr + sizeof(eeprom);
 	u32 bytes_remaining;
 	u8 *data;
 	int ret = 0;
 
-	if (!ops->get_eeprom || !ops->get_eeprom_len)
-		return -EOPNOTSUPP;
-
 	if (copy_from_user(&eeprom, useraddr, sizeof(eeprom)))
 		return -EFAULT;
 
@@ -771,7 +770,7 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 		return -EINVAL;
 
 	/* Check for exceeding total eeprom len */
-	if (eeprom.offset + eeprom.len > ops->get_eeprom_len(dev))
+	if (eeprom.offset + eeprom.len > total_len)
 		return -EINVAL;
 
 	data = kmalloc(PAGE_SIZE, GFP_USER);
@@ -782,7 +781,7 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 	while (bytes_remaining > 0) {
 		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-		ret = ops->get_eeprom(dev, &eeprom, data);
+		ret = getter(dev, &eeprom, data);
 		if (ret)
 			break;
 		if (copy_to_user(userbuf, data, eeprom.len)) {
@@ -803,6 +802,17 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 	return ret;
 }
 
+static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (!ops->get_eeprom || !ops->get_eeprom_len)
+		return -EOPNOTSUPP;
+
+	return ethtool_get_any_eeprom(dev, useraddr, ops->get_eeprom,
+				      ops->get_eeprom_len(dev));
+}
+
 static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
-- 
1.7.7.6

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[Stuart Hodgson]

On 12/04/12 00:42, Ben Hutchings wrote:
> On Wed, 2012-04-11 at 19:16 +0100, Ben Hutchings wrote:
>> 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.
>
> Maybe we should start by refactoring ethtool_get_eeprom() so we can
> reuse most of its code in ethtool_get_module_eeprom(), rather than
> having to worry about what the maximum size of a module EEPROM might be
> and whether we need a loop:
>
> Subject: ethtool: Split ethtool_get_eeprom() to allow for additional EEPROM accessors
>
> We want to support reading module (SFP+, XFP, ...) EEPROMs as well as
> NIC EEPROMs.  They will need a different command number and driver
> operation, but the structure and arguments will be the same and so we
> can share most of the code here.
>
> Signed-off-by: Ben Hutchings<bhutchings@solarflare.com>
> ---
>   net/core/ethtool.c |   24 +++++++++++++++++-------
>   1 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index beacdd9..ca7698f 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -751,18 +751,17 @@ static int ethtool_get_link(struct net_device *dev, char __user *useraddr)
>   	return 0;
>   }
>
> -static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
> +static int ethtool_get_any_eeprom(struct net_device *dev, void __user *useraddr,
> +				  int (*getter)(struct net_device *,
> +						struct ethtool_eeprom *, u8 *),
> +				  u32 total_len)
>   {
>   	struct ethtool_eeprom eeprom;
> -	const struct ethtool_ops *ops = dev->ethtool_ops;
>   	void __user *userbuf = useraddr + sizeof(eeprom);
>   	u32 bytes_remaining;
>   	u8 *data;
>   	int ret = 0;
>
> -	if (!ops->get_eeprom || !ops->get_eeprom_len)
> -		return -EOPNOTSUPP;
> -
>   	if (copy_from_user(&eeprom, useraddr, sizeof(eeprom)))
>   		return -EFAULT;
>
> @@ -771,7 +770,7 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
>   		return -EINVAL;
>
>   	/* Check for exceeding total eeprom len */
> -	if (eeprom.offset + eeprom.len>  ops->get_eeprom_len(dev))
> +	if (eeprom.offset + eeprom.len>  total_len)
>   		return -EINVAL;
>
>   	data = kmalloc(PAGE_SIZE, GFP_USER);

Should this not be eeprom.len?

> @@ -782,7 +781,7 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
>   	while (bytes_remaining>  0) {
>   		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
>
> -		ret = ops->get_eeprom(dev,&eeprom, data);
> +		ret = getter(dev,&eeprom, data);
>   		if (ret)
>   			break;
>   		if (copy_to_user(userbuf, data, eeprom.len)) {
> @@ -803,6 +802,17 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
>   	return ret;
>   }
>
> +static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
> +{
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +
> +	if (!ops->get_eeprom || !ops->get_eeprom_len)
> +		return -EOPNOTSUPP;
> +
> +	return ethtool_get_any_eeprom(dev, useraddr, ops->get_eeprom,
> +				      ops->get_eeprom_len(dev));
> +}
> +
>   static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
>   {
>   	struct ethtool_eeprom eeprom;

This would reduce the code size nicely between the two eeprom fetches.

Stu

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

[Ben Hutchings]

On Thu, 2012-04-12 at 10:18 +0100, Stuart Hodgson wrote:
> On 12/04/12 00:42, Ben Hutchings wrote:
[...]
> > Maybe we should start by refactoring ethtool_get_eeprom() so we can
> > reuse most of its code in ethtool_get_module_eeprom(), rather than
> > having to worry about what the maximum size of a module EEPROM might be
> > and whether we need a loop:
> >
> > Subject: ethtool: Split ethtool_get_eeprom() to allow for additional EEPROM accessors
[...]
> > @@ -771,7 +770,7 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
> >   		return -EINVAL;
> >
> >   	/* Check for exceeding total eeprom len */
> > -	if (eeprom.offset + eeprom.len>  ops->get_eeprom_len(dev))
> > +	if (eeprom.offset + eeprom.len>  total_len)
> >   		return -EINVAL;
> >
> >   	data = kmalloc(PAGE_SIZE, GFP_USER);
> 
> Should this not be eeprom.len?
[...]

No, because this function loops over PAGE_SIZE chunks.  That's
presumably necessary for large NIC EEPROMs, and if we reuse it we won't
have to wory about whether it's ever necessary for large module EEPROMs.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.