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

[Stuart Hodgson <smhodgson@solarflare.com>]

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