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    [W:0.031 / U:0.108 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site