On 28-Feb-21 02:45, Andrew Lunn wrote:
On Wed, Feb 24, 2021 at 05:41:10PM +0200, Moshe Shemesh wrote:
From: Vladyslav Tarasiuk <vladysl...@nvidia.com>
Define get_module_eeprom_data_by_page() ethtool callback and implement
netlink infrastructure.
get_module_eeprom_data_by_page() allows network drivers to dump a part
of module's EEPROM specified by page and bank numbers along with offset
and length. It is effectively a netlink replacement for
get_module_info() and get_module_eeprom() pair, which is needed due to
emergence of complex non-linear EEPROM layouts.
Signed-off-by: Vladyslav Tarasiuk <vladysl...@nvidia.com>
---
include/linux/ethtool.h | 5 +
include/uapi/linux/ethtool.h | 25 +++++
include/uapi/linux/ethtool_netlink.h | 19 ++++
net/ethtool/Makefile | 2 +-
net/ethtool/eeprom.c | 149 +++++++++++++++++++++++++++
net/ethtool/netlink.c | 10 ++
net/ethtool/netlink.h | 2 +
7 files changed, 211 insertions(+), 1 deletion(-)
create mode 100644 net/ethtool/eeprom.c
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index ec4cd3921c67..6032313fa914 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -410,6 +410,9 @@ struct ethtool_pause_stats {
* @get_ethtool_phy_stats: Return extended statistics about the PHY device.
* This is only useful if the device maintains PHY statistics and
* cannot use the standard PHY library helpers.
+ * @get_module_eeprom_data_by_page: Get a region of plug-in module EEPROM data
+ * from specified page. Updates length to the amount actually read.
+ * Returns a negative error code or zero.
*
* All operations are optional (i.e. the function pointer may be set
* to %NULL) and callers must take this into account. Callers must
@@ -515,6 +518,8 @@ struct ethtool_ops {
const struct ethtool_tunable *, void *);
int (*set_phy_tunable)(struct net_device *,
const struct ethtool_tunable *, const void
*);
+ int (*get_module_eeprom_data_by_page)(struct net_device *dev,
+ struct ethtool_eeprom_data
*page);
Hi Moshe, Vladyslav
I think it would be good to pass an extack here, so we can report more
useful errors.
Hi Andrew!
Yes, I will add this parameter.
+/**
+ * struct ethtool_eeprom_data - EEPROM dump from specified page
+ * @offset: Offset within the specified EEPROM page to begin read, in bytes.
+ * @length: Number of bytes to read. On successful return, number of bytes
+ * actually read.
+ * @page: Page number to read from.
+ * @bank: Page bank number to read from, if applicable by EEPROM spec.
+ * @i2c_address: I2C address of a page. Zero indicates a driver should choose
+ * by itself.
I don't particularly like the idea of the driver deciding what to
read. User space should really be passing 0x50 or 0x51 for the normal
case. And we need to make it clear what these addresses mean, since
they are often referred to as 0xA0 and 0xA2, due to addresses being
shifted one bit left and a r/w bit added.
I was thinking what address should I send from userspace by default?
Should I send 0x50 or 0xA0 and at some point to do the shift?
mlx5 uses 0x50 and 0x51, for example.
I also don't think the in place length should be modified. It would be
better to follow the use semantics of returning a negative value on
error, or a positive value for the length actually
read. ethtool_eeprom_data can then be passed as a const.
But how would userspace know how much actually was read?
Should I fail the command if only part of a data requested was read?
+ * @data: Pointer to buffer with EEPROM data of @length size.
+ *
+ * This can be used to manage pages during EEPROM dump in ethtool and pass
+ * required information to the driver.
+ */
+struct ethtool_eeprom_data {
+ __u32 offset;
+ __u32 length;
+ __u32 page;
+ __u32 bank;
+ __u32 i2c_address;
+ __u8 *data;
+};
+
/**
* struct ethtool_eee - Energy Efficient Ethernet information
* @cmd: ETHTOOL_{G,S}EEE
@@ -1865,6 +1888,8 @@ static inline int ethtool_validate_duplex(__u8 duplex)
#define ETH_MODULE_SFF_8636_MAX_LEN 640
#define ETH_MODULE_SFF_8436_MAX_LEN 640
+#define ETH_MODULE_EEPROM_MAX_LEN 640
I'm surprised such a high value is allowed. 128 seems more
appropriate, given the size of 1/2 pages.
This is done for backwards compatibility mechanism (last patch in the
series)
to work properly. I wanted to limit the length to 128 (or 256, maybe), but
currently ethtool supports dumps of 640 bytes at most.
I guess I can add another value like ETH_MODULE_EEPROM_PAGE_MAX_LEN 256
and if
new ndo is available or page number is specified, use it for check.
If ndo is not implemented, use 640 instead.
What do you think?
+
/* 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/include/uapi/linux/ethtool_netlink.h
b/include/uapi/linux/ethtool_netlink.h
index a286635ac9b8..60dd848d0b54 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -42,6 +42,7 @@ enum {
ETHTOOL_MSG_CABLE_TEST_ACT,
ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
ETHTOOL_MSG_TUNNEL_INFO_GET,
+ ETHTOOL_MSG_EEPROM_DATA_GET,
/* add new constants above here */
__ETHTOOL_MSG_USER_CNT,
@@ -80,6 +81,7 @@ enum {
ETHTOOL_MSG_CABLE_TEST_NTF,
ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
+ ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
/* add new constants above here */
__ETHTOOL_MSG_KERNEL_CNT,
@@ -629,6 +631,23 @@ enum {
ETHTOOL_A_TUNNEL_INFO_MAX = (__ETHTOOL_A_TUNNEL_INFO_CNT - 1)
};
+/* MODULE EEPROM DATA */
+
+enum {
+ ETHTOOL_A_EEPROM_DATA_UNSPEC,
+ ETHTOOL_A_EEPROM_DATA_HEADER,
+
+ ETHTOOL_A_EEPROM_DATA_OFFSET,
+ ETHTOOL_A_EEPROM_DATA_LENGTH,
+ ETHTOOL_A_EEPROM_DATA_PAGE,
+ ETHTOOL_A_EEPROM_DATA_BANK,
+ ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
+ ETHTOOL_A_EEPROM_DATA,
+
+ __ETHTOOL_A_EEPROM_DATA_CNT,
+ ETHTOOL_A_EEPROM_DATA_MAX = (__ETHTOOL_A_EEPROM_DATA_CNT - 1)
+};
+
/* generic netlink info */
#define ETHTOOL_GENL_NAME "ethtool"
#define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 7a849ff22dad..d604346bc074 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
ethtool_nl-y := netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
linkstate.o debug.o wol.o features.o privflags.o rings.o \
channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
- tunnels.o
+ tunnels.o eeprom.o
diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
new file mode 100644
index 000000000000..51a2ed81a273
--- /dev/null
+++ b/net/ethtool/eeprom.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/ethtool.h>
+#include "netlink.h"
+#include "common.h"
+
+struct eeprom_data_req_info {
+ struct ethnl_req_info base;
+ u32 offset;
+ u32 length;
+ u32 page;
+ u32 bank;
+ u32 i2c_address;
+};
+
+struct eeprom_data_reply_data {
+ struct ethnl_reply_data base;
+ u32 length;
+ u32 i2c_address;
+ u8 *data;
+};
+
+#define EEPROM_DATA_REQINFO(__req_base) \
+ container_of(__req_base, struct eeprom_data_req_info, base)
+
+#define EEPROM_DATA_REPDATA(__reply_base) \
+ container_of(__reply_base, struct eeprom_data_reply_data, base)
+
+static int eeprom_data_prepare_data(const struct ethnl_req_info *req_base,
+ struct ethnl_reply_data *reply_base,
+ struct genl_info *info)
+{
+ struct eeprom_data_reply_data *reply = EEPROM_DATA_REPDATA(reply_base);
+ struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_base);
+ struct ethtool_eeprom_data page_data = {0};
+ struct net_device *dev = reply_base->dev;
+ int err;
+
+ if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
+ return -EOPNOTSUPP;
+ err = ethnl_ops_begin(dev);
+ if (err)
+ return err;
+
+ page_data.offset = request->offset;
+ page_data.length = request->length;
+ page_data.i2c_address = request->i2c_address;
+ page_data.page = request->page;
+ page_data.bank = request->bank;
+ page_data.data = kmalloc(page_data.length, GFP_KERNEL);
+ if (!page_data.data)
+ return -ENOMEM;
Isn't an ethnl_ops_complete(dev); needed here? Maybe postpone the
ethnl_ops_begin(dev) call until after the memory allocation?
Yes, I should just move it after kmalloc().
+
+ err = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, &page_data);
+ if (err)
+ goto err_out;
+
+ reply->length = page_data.length;
+ reply->i2c_address = page_data.i2c_address;
+ reply->data = page_data.data;
+
+ ethnl_ops_complete(dev);
+ return 0;
+
+err_out:
+ kfree(page_data.data);
+ ethnl_ops_complete(dev);
+ return err;
+}
+
+static int eeprom_data_parse_request(struct ethnl_req_info *req_info, struct
nlattr **tb,
+ struct netlink_ext_ack *extack)
+{
+ struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_info);
+
+ if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
+ !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
+ !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
+ return -EINVAL;
+
+ request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
+ if (request->length > ETH_MODULE_EEPROM_MAX_LEN)
+ return -EINVAL;
+
+ request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
I think you need to validate that offset + length is not passed the
end of a 1/2 page. There seems to be odd wrap around semantics, so we
probably want to avoid that.
I think I will add this check in case a page was specified and this is not
a case ofbackwards compatible flow.
+ request->i2c_address =
nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
Maybe validate the i2c address. Most busses are limited to 7 bit
addresses, but some do allow 10 bits. I think we can probably ignore
10 bit addresses for this use case, so check that address is < 128.
Yes, this check is possible, if I expect 0x50 or 0x51 in request. I will
add it.
