Date:	Mon, 2 Apr 2012 18:52:40 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Stuart Hodgson <smhodgson@...arflare.com>
CC:	<netdev@...r.kernel.org>, <bruce.w.allan@...el.com>,
	<mirq-linux@...e.qmqm.pl>, <decot@...gle.com>,
	<amit.salecha@...gic.com>, <alexander.h.duyck@...el.com>,
	<davem@...emloft.net>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/2] net: ethtool: Add capability to retrieve
 plug-in module EEPROM

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@...arflare.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.

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

Powered by blists - more mailing lists