lkml.org 
[lkml]   [2012]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 1/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
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


\
 
 \ /
  Last update: 2012-04-12 11:21    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site