Re: [RFC PATCH 1/2] net: ethtool: Add capability to retrieve plug-inmodule 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@xxxxxxxxxxxxxx>
> > ---
> >    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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/