Re: [RFC PATCH 1/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
From: Ben Hutchings <hidden>
Date: 2012-04-11 23:42:19
Also in:
lkml
Subsystem:
networking [general], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On Wed, 2012-04-11 at 19:16 +0100, Ben Hutchings wrote:
On Wed, 2012-04-11 at 17:50 +0100, Stuart Hodgson wrote:quoted
On 02/04/12 18:52, Ben Hutchings wrote:[...]quoted
quoted
quoted
--- a/net/core/ethtool.c +++ b/net/core/ethtool.c[...]quoted
quoted
quoted
+ 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.quoted
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 <redacted> --- 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);
@@ -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;
--
1.7.7.6
--
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.