| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1333389160.2623.30.camel@bwh-desktop.uk.solarflarecom.com>
Date: Mon, 2 Apr 2012 18:52:40 +0100
From: Ben Hutchings <bhutchings@...arflare.com>
To: Stuart Hodgson <smhodgson@...arflare.com>
CC: <netdev@...r.kernel.org>, <bruce.w.allan@...el.com>,
<mirq-linux@...e.qmqm.pl>, <decot@...gle.com>,
<amit.salecha@...gic.com>, <alexander.h.duyck@...el.com>,
<davem@...emloft.net>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/2] net: ethtool: Add capability to retrieve
plug-in module EEPROM
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@...arflare.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.
> + 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.
--
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists