[RFC PATCH V3 net-next 0/5] ethtool: Extend module EEPROM dump API

[RFC PATCH V3 net-next 0/5] ethtool: Extend module EEPROM dump API

[Moshe Shemesh]

Ethtool supports module EEPROM dumps via the `ethtool -m <dev>` command.
But in current state its functionality is limited - offset and length
parameters, which are used to specify a linear desired region of EEPROM
data to dump, is not enough, considering emergence of complex module
EEPROM layouts such as CMIS 4.0.
Moreover, CMIS 4.0 extends the amount of pages that may be accessible by
introducing another parameter for page addressing - banks.

Besides, currently module EEPROM is represented as a chunk of
concatenated pages, where lower 128 bytes of all pages, except page 00h,
are omitted. Offset and length are used to address parts of this fake
linear memory. But in practice drivers, which implement
get_module_info() and get_module_eeprom() ethtool ops still calculate
page number and set I2C address on their own.

This series tackles these issues by adding ethtool op, which allows to
pass page number, bank number and I2C address in addition to offset and
length parameters to the driver, adds corresponding netlink
infrastructure and implements the new interface in mlx5 driver.

This allows to extend userspace 'ethtool -m' CLI by adding new
parameters - page, bank and i2c. New command line format:
 ethtool -m <dev> [hex on|off] [raw on|off] [offset N] [length N] [page N] [bank N] [i2c N]

The consequence of this series is a possibility to dump arbitrary EEPROM
page at a time, in contrast to dumps of concatenated pages. Therefore,
offset and length change their semantics and may be used only to specify
a part of data within a page, which size is currently limited to 256
bytes.

As for backwards compatibility with get_module_info() and
get_module_eeprom() pair, the series addresses it as well by
implementing a fallback mechanism. As mentioned earlier, drivers derive
a page number from 'global' offset, so this can be done vice versa
without their involvement thanks to standardization. If kernel netlink
handler of 'ethtool -m' command detects that new ethtool op is not
supported by the driver, it calculates offset from given page number and
page offset and calls old ndos, if they are available.

Change log:
v2 -> v3:
- Removed page number limitations
- Added length limit when page is present in fallback
- Changed page, bank and i2c_address type to u8 all over the patchset
- Added 0x51 I2C address usage increasing offset by 256 for SFP

v1 -> v2:
- Limited i2c_address values by 127
- Added page bound check for offset and length
- Added defines for these two points
- Added extack to ndo parameters
- Moved ethnl_ops_begin(dev) and set error path accordingly


Vladyslav Tarasiuk (5):
  ethtool: Allow network drivers to dump arbitrary EEPROM data
  net/mlx5: Refactor module EEPROM query
  net/mlx5: Implement get_module_eeprom_data_by_page()
  net/mlx5: Add support for DSFP module EEPROM dumps
  ethtool: Add fallback to get_module_eeprom from netlink command

 Documentation/networking/ethtool-netlink.rst  |  34 ++-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  44 ++++
 .../net/ethernet/mellanox/mlx5/core/port.c    | 101 +++++---
 include/linux/ethtool.h                       |   8 +-
 include/linux/mlx5/port.h                     |  12 +
 include/uapi/linux/ethtool.h                  |  25 ++
 include/uapi/linux/ethtool_netlink.h          |  19 ++
 net/ethtool/Makefile                          |   2 +-
 net/ethtool/eeprom.c                          | 226 ++++++++++++++++++
 net/ethtool/netlink.c                         |  10 +
 net/ethtool/netlink.h                         |   2 +
 11 files changed, 451 insertions(+), 32 deletions(-)
 create mode 100644 net/ethtool/eeprom.c

-- 
2.26.2

[RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Moshe Shemesh]

From: Vladyslav Tarasiuk <vladyslavt@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 <vladyslavt@nvidia.com>
---
 Documentation/networking/ethtool-netlink.rst |  34 ++++-
 include/linux/ethtool.h                      |   8 +-
 include/uapi/linux/ethtool.h                 |  25 +++
 include/uapi/linux/ethtool_netlink.h         |  19 +++
 net/ethtool/Makefile                         |   2 +-
 net/ethtool/eeprom.c                         | 153 +++++++++++++++++++
 net/ethtool/netlink.c                        |  10 ++
 net/ethtool/netlink.h                        |   2 +
 8 files changed, 249 insertions(+), 4 deletions(-)
 create mode 100644 net/ethtool/eeprom.c

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 05073482db05..25846b97632a 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1280,6 +1280,36 @@ Kernel response contents:
 For UDP tunnel table empty ``ETHTOOL_A_TUNNEL_UDP_TABLE_TYPES`` indicates that
 the table contains static entries, hard-coded by the NIC.
 
+EEPROM_DATA
+===========
+
+Fetch module EEPROM data dump.
+
+Request contents:
+
+  =====================================  ======  ==========================
+  ``ETHTOOL_A_EEPROM_DATA_HEADER``       nested  request header
+  ``ETHTOOL_A_EEPROM_DATA_OFFSET``       u32     offset within a page
+  ``ETHTOOL_A_EEPROM_DATA_LENGTH``       u32     amount of bytes to read
+  ``ETHTOOL_A_EEPROM_DATA_PAGE``         u8      page number
+  ``ETHTOOL_A_EEPROM_DATA_BANK``         u8      bank number
+  ``ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS``  u8      page I2C address
+  =====================================  ======  ==========================
+
+Kernel response contents:
+
+ +---------------------------------------------+--------+---------------------+
+ | ``ETHTOOL_A_EEPROM_DATA_HEADER``            | nested | reply header        |
+ +---------------------------------------------+--------+---------------------+
+ | ``ETHTOOL_A_EEPROM_DATA_LENGTH``            | u32    | amount of bytes read|
+ +---------------------------------------------+--------+---------------------+
+ | ``ETHTOOL_A_EEPROM_DATA``                   | nested | array of bytes from |
+ |                                             |        | module EEPROM       |
+ +---------------------------------------------+--------+---------------------+
+
+``ETHTOOL_A_EEPROM_DATA`` has an attribute length equal to the amount of bytes
+driver actually read.
+
 Request translation
 ===================
 
@@ -1357,8 +1387,8 @@ are netlink only.
   ``ETHTOOL_GET_DUMP_FLAG``           n/a
   ``ETHTOOL_GET_DUMP_DATA``           n/a
   ``ETHTOOL_GET_TS_INFO``             ``ETHTOOL_MSG_TSINFO_GET``
-  ``ETHTOOL_GMODULEINFO``             n/a
-  ``ETHTOOL_GMODULEEEPROM``           n/a
+  ``ETHTOOL_GMODULEINFO``             ``ETHTOOL_MSG_MODULE_EEPROM_GET``
+  ``ETHTOOL_GMODULEEEPROM``           ``ETHTOOL_MSG_MODULE_EEPROM_GET``
   ``ETHTOOL_GEEE``                    ``ETHTOOL_MSG_EEE_GET``
   ``ETHTOOL_SEEE``                    ``ETHTOOL_MSG_EEE_SET``
   ``ETHTOOL_GRSSH``                   n/a
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index ec4cd3921c67..9551005b350a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -81,6 +81,7 @@ enum {
 #define ETH_RSS_HASH_NO_CHANGE	0
 
 struct net_device;
+struct netlink_ext_ack;
 
 /* Some generic methods drivers may use in their ethtool_ops */
 u32 ethtool_op_get_link(struct net_device *dev);
@@ -410,6 +411,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. Returns a negative error code or the amount of
+ *	bytes read.
  *
  * 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 +519,9 @@ 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,
+						  const struct ethtool_eeprom_data *page,
+						  struct netlink_ext_ack *extack);
 };
 
 int ethtool_check_ops(const struct ethtool_ops *ops);
@@ -538,7 +545,6 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
 				       const struct ethtool_link_ksettings *cmd,
 				       u32 *dev_speed, u8 *dev_duplex);
 
-struct netlink_ext_ack;
 struct phy_device;
 struct phy_tdr_config;
 
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index cde753bb2093..b3e92db3ad37 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -340,6 +340,28 @@ struct ethtool_eeprom {
 	__u8	data[0];
 };
 
+/**
+ * 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.
+ * @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. Value less than 0x7f expected. Most
+ *	EEPROMs use 0x50 or 0x51.
+ * @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;
+	__u8	page;
+	__u8	bank;
+	__u8	i2c_address;
+	__u8	*data;
+};
+
 /**
  * struct ethtool_eee - Energy Efficient Ethernet information
  * @cmd: ETHTOOL_{G,S}EEE
@@ -1865,6 +1887,9 @@ 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_PAGE_LEN	256
+#define ETH_MODULE_MAX_I2C_ADDRESS	0x7f
+
 /* 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..e1b1b962f3da 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,			/* nest - _A_HEADER_* */
+
+	ETHTOOL_A_EEPROM_DATA_OFFSET,			/* u32 */
+	ETHTOOL_A_EEPROM_DATA_LENGTH,			/* u32 */
+	ETHTOOL_A_EEPROM_DATA_PAGE,			/* u8 */
+	ETHTOOL_A_EEPROM_DATA_BANK,			/* u8 */
+	ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,		/* u8 */
+	ETHTOOL_A_EEPROM_DATA,				/* nested */
+
+	__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..e110336dc231
--- /dev/null
+++ b/net/ethtool/eeprom.c
@@ -0,0 +1,153 @@
+// 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;
+	u8			page;
+	u8			bank;
+	u8			i2c_address;
+};
+
+struct eeprom_data_reply_data {
+	struct ethnl_reply_data base;
+	u32			length;
+	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 ret;
+
+	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
+		return -EOPNOTSUPP;
+
+	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;
+	ret = ethnl_ops_begin(dev);
+	if (ret)
+		goto err_free;
+
+	ret = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, &page_data,
+							       info->extack);
+	if (ret < 0)
+		goto err_ops;
+
+	reply->length = ret;
+	reply->data = page_data.data;
+
+	ethnl_ops_complete(dev);
+	return 0;
+
+err_ops:
+	ethnl_ops_complete(dev);
+err_free:
+	kfree(page_data.data);
+	return ret;
+}
+
+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);
+	struct net_device *dev = req_info->dev;
+
+	if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
+	    !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
+	    !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
+		return -EINVAL;
+
+	request->i2c_address = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
+	if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
+		return -EINVAL;
+
+	request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
+	request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
+	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
+	    dev->ethtool_ops->get_module_eeprom_data_by_page &&
+	    request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
+		return -EINVAL;
+
+	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
+		request->page = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
+	if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
+		request->bank = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
+
+	return 0;
+}
+
+static int eeprom_data_reply_size(const struct ethnl_req_info *req_base,
+				  const struct ethnl_reply_data *reply_base)
+{
+	const struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_base);
+
+	return nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_LENGTH */
+	       nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA */
+}
+
+static int eeprom_data_fill_reply(struct sk_buff *skb,
+				  const struct ethnl_req_info *req_base,
+				  const struct ethnl_reply_data *reply_base)
+{
+	struct eeprom_data_reply_data *reply = EEPROM_DATA_REPDATA(reply_base);
+
+	if (nla_put_u32(skb, ETHTOOL_A_EEPROM_DATA_LENGTH, reply->length) ||
+	    nla_put(skb, ETHTOOL_A_EEPROM_DATA, reply->length, reply->data))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static void eeprom_data_cleanup_data(struct ethnl_reply_data *reply_base)
+{
+	struct eeprom_data_reply_data *reply = EEPROM_DATA_REPDATA(reply_base);
+
+	kfree(reply->data);
+}
+
+const struct ethnl_request_ops ethnl_eeprom_data_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_EEPROM_DATA_GET,
+	.reply_cmd		= ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_EEPROM_DATA_HEADER,
+	.req_info_size		= sizeof(struct eeprom_data_req_info),
+	.reply_data_size	= sizeof(struct eeprom_data_reply_data),
+
+	.parse_request		= eeprom_data_parse_request,
+	.prepare_data		= eeprom_data_prepare_data,
+	.reply_size		= eeprom_data_reply_size,
+	.fill_reply		= eeprom_data_fill_reply,
+	.cleanup_data		= eeprom_data_cleanup_data,
+};
+
+const struct nla_policy ethnl_eeprom_data_get_policy[] = {
+	[ETHTOOL_A_EEPROM_DATA_HEADER]		= NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_EEPROM_DATA_OFFSET]		= { .type = NLA_U32 },
+	[ETHTOOL_A_EEPROM_DATA_LENGTH]		= { .type = NLA_U32 },
+	[ETHTOOL_A_EEPROM_DATA_PAGE]		= { .type = NLA_U8 },
+	[ETHTOOL_A_EEPROM_DATA_BANK]		= { .type = NLA_U8 },
+	[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]	= { .type = NLA_U8 },
+	[ETHTOOL_A_EEPROM_DATA]			= { .type = NLA_BINARY },
+};
+
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 50d3c8896f91..ff2528bee192 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -245,6 +245,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_PAUSE_GET]		= &ethnl_pause_request_ops,
 	[ETHTOOL_MSG_EEE_GET]		= &ethnl_eee_request_ops,
 	[ETHTOOL_MSG_TSINFO_GET]	= &ethnl_tsinfo_request_ops,
+	[ETHTOOL_MSG_EEPROM_DATA_GET]	= &ethnl_eeprom_data_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -912,6 +913,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_tunnel_info_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_tunnel_info_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_EEPROM_DATA_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_eeprom_data_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_eeprom_data_get_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 6eabd58d81bf..60954c7b4dfe 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -344,6 +344,7 @@ extern const struct ethnl_request_ops ethnl_coalesce_request_ops;
 extern const struct ethnl_request_ops ethnl_pause_request_ops;
 extern const struct ethnl_request_ops ethnl_eee_request_ops;
 extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
+extern const struct ethnl_request_ops ethnl_eeprom_data_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -375,6 +376,7 @@ extern const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_HEADER +
 extern const struct nla_policy ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_HEADER + 1];
 extern const struct nla_policy ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG + 1];
 extern const struct nla_policy ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_HEADER + 1];
+extern const struct nla_policy ethnl_eeprom_data_get_policy[ETHTOOL_A_EEPROM_DATA + 1];
 
 int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
-- 
2.26.2

[RFC PATCH V3 net-next 2/5] net/mlx5: Refactor module EEPROM query

[Moshe Shemesh]

From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>

Prepare for ethtool_ops::get_module_eeprom_data() implementation by
extracting common part of mlx5_query_module_eeprom() into a separate
function.

Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/port.c    | 79 +++++++++++--------
 include/linux/mlx5/port.h                     |  9 +++
 2 files changed, 54 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/port.c b/drivers/net/ethernet/mellanox/mlx5/core/port.c
index 4bb219565c58..9b9f870d67a4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/port.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/port.c
@@ -353,67 +353,78 @@ static void mlx5_sfp_eeprom_params_set(u16 *i2c_addr, int *page_num, u16 *offset
 	*offset -= MLX5_EEPROM_PAGE_LENGTH;
 }
 
-int mlx5_query_module_eeprom(struct mlx5_core_dev *dev,
-			     u16 offset, u16 size, u8 *data)
+static int mlx5_query_mcia(struct mlx5_core_dev *dev,
+			   struct mlx5_module_eeprom_query_params *params, u8 *data)
 {
-	int module_num, status, err, page_num = 0;
 	u32 in[MLX5_ST_SZ_DW(mcia_reg)] = {};
 	u32 out[MLX5_ST_SZ_DW(mcia_reg)];
-	u16 i2c_addr = 0;
-	u8 module_id;
+	int status, err;
 	void *ptr;
+	u16 size;
+
+	size = min_t(int, params->size, MLX5_EEPROM_MAX_BYTES);
+
+	MLX5_SET(mcia_reg, in, l, 0);
+	MLX5_SET(mcia_reg, in, size, size);
+	MLX5_SET(mcia_reg, in, module, params->module_number);
+	MLX5_SET(mcia_reg, in, device_address, params->offset);
+	MLX5_SET(mcia_reg, in, page_number, params->page);
+	MLX5_SET(mcia_reg, in, i2c_device_address, params->i2c_address);
 
-	err = mlx5_query_module_num(dev, &module_num);
+	err = mlx5_core_access_reg(dev, in, sizeof(in), out,
+				   sizeof(out), MLX5_REG_MCIA, 0, 0);
 	if (err)
 		return err;
 
-	err = mlx5_query_module_id(dev, module_num, &module_id);
+	status = MLX5_GET(mcia_reg, out, status);
+	if (status) {
+		mlx5_core_err(dev, "query_mcia_reg failed: status: 0x%x\n",
+			      status);
+		return -EIO;
+	}
+
+	ptr = MLX5_ADDR_OF(mcia_reg, out, dword_0);
+	memcpy(data, ptr, size);
+
+	return size;
+}
+
+int mlx5_query_module_eeprom(struct mlx5_core_dev *dev,
+			     u16 offset, u16 size, u8 *data)
+{
+	struct mlx5_module_eeprom_query_params query = {0};
+	u8 module_id;
+	int err;
+
+	err = mlx5_query_module_num(dev, &query.module_number);
+	if (err)
+		return err;
+
+	err = mlx5_query_module_id(dev, query.module_number, &module_id);
 	if (err)
 		return err;
 
 	switch (module_id) {
 	case MLX5_MODULE_ID_SFP:
-		mlx5_sfp_eeprom_params_set(&i2c_addr, &page_num, &offset);
+		mlx5_sfp_eeprom_params_set(&query.i2c_address, &query.page, &query.offset);
 		break;
 	case MLX5_MODULE_ID_QSFP:
 	case MLX5_MODULE_ID_QSFP_PLUS:
 	case MLX5_MODULE_ID_QSFP28:
-		mlx5_qsfp_eeprom_params_set(&i2c_addr, &page_num, &offset);
+		mlx5_qsfp_eeprom_params_set(&query.i2c_address, &query.page, &query.offset);
 		break;
 	default:
 		mlx5_core_err(dev, "Module ID not recognized: 0x%x\n", module_id);
 		return -EINVAL;
 	}
 
-	if (offset + size > MLX5_EEPROM_PAGE_LENGTH)
+	if (query.offset + size > MLX5_EEPROM_PAGE_LENGTH)
 		/* Cross pages read, read until offset 256 in low page */
 		size -= offset + size - MLX5_EEPROM_PAGE_LENGTH;
 
-	size = min_t(int, size, MLX5_EEPROM_MAX_BYTES);
+	query.size = size;
 
-	MLX5_SET(mcia_reg, in, l, 0);
-	MLX5_SET(mcia_reg, in, module, module_num);
-	MLX5_SET(mcia_reg, in, i2c_device_address, i2c_addr);
-	MLX5_SET(mcia_reg, in, page_number, page_num);
-	MLX5_SET(mcia_reg, in, device_address, offset);
-	MLX5_SET(mcia_reg, in, size, size);
-
-	err = mlx5_core_access_reg(dev, in, sizeof(in), out,
-				   sizeof(out), MLX5_REG_MCIA, 0, 0);
-	if (err)
-		return err;
-
-	status = MLX5_GET(mcia_reg, out, status);
-	if (status) {
-		mlx5_core_err(dev, "query_mcia_reg failed: status: 0x%x\n",
-			      status);
-		return -EIO;
-	}
-
-	ptr = MLX5_ADDR_OF(mcia_reg, out, dword_0);
-	memcpy(data, ptr, size);
-
-	return size;
+	return mlx5_query_mcia(dev, &query, data);
 }
 EXPORT_SYMBOL_GPL(mlx5_query_module_eeprom);
 
diff --git a/include/linux/mlx5/port.h b/include/linux/mlx5/port.h
index 23edd2db4803..90b87aa82db3 100644
--- a/include/linux/mlx5/port.h
+++ b/include/linux/mlx5/port.h
@@ -62,6 +62,15 @@ enum mlx5_an_status {
 #define MLX5_EEPROM_PAGE_LENGTH		256
 #define MLX5_EEPROM_HIGH_PAGE_LENGTH	128
 
+struct mlx5_module_eeprom_query_params {
+	u16 size;
+	u16 offset;
+	u16 i2c_address;
+	u32 page;
+	u32 bank;
+	u32 module_number;
+};
+
 enum mlx5e_link_mode {
 	MLX5E_1000BASE_CX_SGMII	 = 0,
 	MLX5E_1000BASE_KX	 = 1,
-- 
2.26.2

[RFC PATCH V3 net-next 3/5] net/mlx5: Implement get_module_eeprom_data_by_page()

[Moshe Shemesh]

From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>

Implement ethtool_ops::get_module_eeprom_data_by_page() to enable
support of new SFP standards.

Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 44 +++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/port.c    | 33 ++++++++++++++
 include/linux/mlx5/port.h                     |  2 +
 3 files changed, 79 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index abdf721bb264..5da9edea4d07 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1769,6 +1769,49 @@ static int mlx5e_get_module_eeprom(struct net_device *netdev,
 	return 0;
 }
 
+static int mlx5e_get_module_eeprom_data_by_page(struct net_device *netdev,
+						const struct ethtool_eeprom_data *page_data,
+						struct netlink_ext_ack *extack)
+{
+	struct mlx5e_priv *priv = netdev_priv(netdev);
+	struct mlx5_module_eeprom_query_params query;
+	struct mlx5_core_dev *mdev = priv->mdev;
+	u8 *data = page_data->data;
+	int size_read;
+	int i = 0;
+
+	if (!page_data->length)
+		return -EINVAL;
+
+	memset(data, 0, page_data->length);
+
+	query.offset = page_data->offset;
+	query.i2c_address = page_data->i2c_address;
+	query.bank = page_data->bank;
+	query.page = page_data->page;
+	while (i < page_data->length) {
+		query.size = page_data->length - i;
+		size_read = mlx5_query_module_eeprom_data(mdev, &query, data + i);
+
+		/* Done reading, return how many bytes was read */
+		if (!size_read)
+			return i;
+
+		if (size_read == -EINVAL)
+			return -EINVAL;
+		if (size_read < 0) {
+			netdev_err(priv->netdev, "%s: mlx5_query_module_eeprom_data failed:0x%x\n",
+				   __func__, size_read);
+			return i;
+		}
+
+		i += size_read;
+		query.offset += size_read;
+	}
+
+	return i;
+}
+
 int mlx5e_ethtool_flash_device(struct mlx5e_priv *priv,
 			       struct ethtool_flash *flash)
 {
@@ -2148,6 +2191,7 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
 	.set_wol	   = mlx5e_set_wol,
 	.get_module_info   = mlx5e_get_module_info,
 	.get_module_eeprom = mlx5e_get_module_eeprom,
+	.get_module_eeprom_data_by_page = mlx5e_get_module_eeprom_data_by_page,
 	.flash_device      = mlx5e_flash_device,
 	.get_priv_flags    = mlx5e_get_priv_flags,
 	.set_priv_flags    = mlx5e_set_priv_flags,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/port.c b/drivers/net/ethernet/mellanox/mlx5/core/port.c
index 9b9f870d67a4..f7a16fdfb8d3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/port.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/port.c
@@ -428,6 +428,39 @@ int mlx5_query_module_eeprom(struct mlx5_core_dev *dev,
 }
 EXPORT_SYMBOL_GPL(mlx5_query_module_eeprom);
 
+int mlx5_query_module_eeprom_data(struct mlx5_core_dev *dev,
+				  struct mlx5_module_eeprom_query_params *params,
+				  u8 *data)
+{
+	u8 module_id;
+	int err;
+
+	err = mlx5_query_module_num(dev, &params->module_number);
+	if (err)
+		return err;
+
+	err = mlx5_query_module_id(dev, params->module_number, &module_id);
+	if (err)
+		return err;
+
+	if (module_id != MLX5_MODULE_ID_SFP &&
+	    module_id != MLX5_MODULE_ID_QSFP &&
+	    module_id != MLX5_MODULE_ID_QSFP28 &&
+	    module_id != MLX5_MODULE_ID_QSFP_PLUS) {
+		mlx5_core_err(dev, "Module ID not recognized: 0x%x\n", module_id);
+		return -EINVAL;
+	}
+
+	if (params->i2c_address != MLX5_I2C_ADDR_HIGH &&
+	    params->i2c_address != MLX5_I2C_ADDR_LOW) {
+		mlx5_core_err(dev, "I2C address not recognized: 0x%x\n", params->i2c_address);
+		return -EINVAL;
+	}
+
+	return mlx5_query_mcia(dev, params, data);
+}
+EXPORT_SYMBOL_GPL(mlx5_query_module_eeprom_data);
+
 static int mlx5_query_port_pvlc(struct mlx5_core_dev *dev, u32 *pvlc,
 				int pvlc_size,  u8 local_port)
 {
diff --git a/include/linux/mlx5/port.h b/include/linux/mlx5/port.h
index 90b87aa82db3..887cd43b41e8 100644
--- a/include/linux/mlx5/port.h
+++ b/include/linux/mlx5/port.h
@@ -209,6 +209,8 @@ void mlx5_query_port_fcs(struct mlx5_core_dev *mdev, bool *supported,
 			 bool *enabled);
 int mlx5_query_module_eeprom(struct mlx5_core_dev *dev,
 			     u16 offset, u16 size, u8 *data);
+int mlx5_query_module_eeprom_data(struct mlx5_core_dev *dev,
+				  struct mlx5_module_eeprom_query_params *params, u8 *data);
 
 int mlx5_query_port_dcbx_param(struct mlx5_core_dev *mdev, u32 *out);
 int mlx5_set_port_dcbx_param(struct mlx5_core_dev *mdev, u32 *in);
-- 
2.26.2

[RFC PATCH V3 net-next 4/5] net/mlx5: Add support for DSFP module EEPROM dumps

[Moshe Shemesh]

From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>

Allow the driver to recognise DSFP transceiver module ID and therefore
allow its EEPROM dumps using ethtool.

Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/port.c | 3 ++-
 include/linux/mlx5/port.h                      | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/port.c b/drivers/net/ethernet/mellanox/mlx5/core/port.c
index f7a16fdfb8d3..3a7aa6b05198 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/port.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/port.c
@@ -446,7 +446,8 @@ int mlx5_query_module_eeprom_data(struct mlx5_core_dev *dev,
 	if (module_id != MLX5_MODULE_ID_SFP &&
 	    module_id != MLX5_MODULE_ID_QSFP &&
 	    module_id != MLX5_MODULE_ID_QSFP28 &&
-	    module_id != MLX5_MODULE_ID_QSFP_PLUS) {
+	    module_id != MLX5_MODULE_ID_QSFP_PLUS &&
+	    module_id != MLX5_MODULE_ID_DSFP) {
 		mlx5_core_err(dev, "Module ID not recognized: 0x%x\n", module_id);
 		return -EINVAL;
 	}
diff --git a/include/linux/mlx5/port.h b/include/linux/mlx5/port.h
index 887cd43b41e8..71b4373cb96c 100644
--- a/include/linux/mlx5/port.h
+++ b/include/linux/mlx5/port.h
@@ -45,6 +45,7 @@ enum mlx5_module_id {
 	MLX5_MODULE_ID_QSFP             = 0xC,
 	MLX5_MODULE_ID_QSFP_PLUS        = 0xD,
 	MLX5_MODULE_ID_QSFP28           = 0x11,
+	MLX5_MODULE_ID_DSFP		= 0x1B,
 };
 
 enum mlx5_an_status {
-- 
2.26.2

[RFC PATCH V3 net-next 5/5] ethtool: Add fallback to get_module_eeprom from netlink command

[Moshe Shemesh]

From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>

In case netlink get_module_eeprom_data_by_page() callback is not
implemented by the driver, try to call old get_module_info() and
get_module_eeprom() pair. Recalculate parameters to get_module_eeprom()
offset and len using page number and their sizes. Return error if
this can't be done.

Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
---
 net/ethtool/eeprom.c | 75 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
index e110336dc231..33ba9ecc36cb 100644
--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c
@@ -25,6 +25,79 @@ struct eeprom_data_reply_data {
 #define EEPROM_DATA_REPDATA(__reply_base) \
 	container_of(__reply_base, struct eeprom_data_reply_data, base)
 
+static int fallback_set_params(struct eeprom_data_req_info *request,
+			       struct ethtool_modinfo *modinfo,
+			       struct ethtool_eeprom *eeprom)
+{
+	u32 offset = request->offset;
+	u32 length = request->length;
+
+	if (request->page) {
+		if (offset < 128 || offset + length > ETH_MODULE_EEPROM_PAGE_LEN)
+			return -EINVAL;
+		offset = request->page * 128 + offset;
+	}
+
+	if (modinfo->type == ETH_MODULE_SFF_8079 &&
+	    request->i2c_address == 0x51)
+		offset += ETH_MODULE_EEPROM_PAGE_LEN;
+
+	if (!length)
+		length = modinfo->eeprom_len;
+
+	if (offset >= modinfo->eeprom_len)
+		return -EINVAL;
+
+	if (modinfo->eeprom_len < offset + length)
+		length = modinfo->eeprom_len - offset;
+
+	eeprom->cmd = ETHTOOL_GMODULEEEPROM;
+	eeprom->len = length;
+	eeprom->offset = offset;
+
+	return 0;
+}
+
+static int eeprom_data_fallback(struct eeprom_data_req_info *request,
+				struct eeprom_data_reply_data *reply,
+				struct genl_info *info)
+{
+	struct net_device *dev = reply->base.dev;
+	struct ethtool_modinfo modinfo = {0};
+	struct ethtool_eeprom eeprom = {0};
+	u8 *data;
+	int err;
+
+	if ((!dev->ethtool_ops->get_module_info &&
+	     !dev->ethtool_ops->get_module_eeprom) || request->bank) {
+		return -EOPNOTSUPP;
+	}
+	modinfo.cmd = ETHTOOL_GMODULEINFO;
+	err = dev->ethtool_ops->get_module_info(dev, &modinfo);
+	if (err < 0)
+		return err;
+
+	err = fallback_set_params(request, &modinfo, &eeprom);
+	if (err < 0)
+		return err;
+
+	data = kmalloc(eeprom.len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	err = dev->ethtool_ops->get_module_eeprom(dev, &eeprom, data);
+	if (err < 0)
+		goto err_out;
+
+	reply->data = data;
+	reply->length = eeprom.len;
+
+	return 0;
+
+err_out:
+	kfree(data);
+	return err;
+}
+
 static int eeprom_data_prepare_data(const struct ethnl_req_info *req_base,
 				    struct ethnl_reply_data *reply_base,
 				    struct genl_info *info)
@@ -36,7 +109,7 @@ static int eeprom_data_prepare_data(const struct ethnl_req_info *req_base,
 	int ret;
 
 	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
-		return -EOPNOTSUPP;
+		return eeprom_data_fallback(request, reply, info);
 
 	page_data.offset = request->offset;
 	page_data.length = request->length;
-- 
2.26.2

Re: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Jakub Kicinski]

On Mon, 15 Mar 2021 19:12:39 +0200 Moshe Shemesh wrote:
> From: Vladyslav Tarasiuk <vladyslavt@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 <vladyslavt@nvidia.com>

s/eeprom_data/eeprom/ everywhere?

Otherwise some of your identifiers are over 30 characters long.

> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index cde753bb2093..b3e92db3ad37 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -340,6 +340,28 @@ struct ethtool_eeprom {
>  	__u8	data[0];
>  };
>  
> +/**
> + * 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.
> + * @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. Value less than 0x7f expected. Most
> + *	EEPROMs use 0x50 or 0x51.
> + * @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;
> +	__u8	page;
> +	__u8	bank;
> +	__u8	i2c_address;
> +	__u8	*data;
> +};
> +

This structure does not have to be part of uAPI, right?

> +#define ETH_MODULE_EEPROM_PAGE_LEN	256
> +#define ETH_MODULE_MAX_I2C_ADDRESS	0x7f

Not sure about these defines either.

> +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 ret;
> +
> +	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
> +		return -EOPNOTSUPP;
> +
> +	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;

nit: new line?

> +	ret = ethnl_ops_begin(dev);
> +	if (ret)
> +		goto err_free;
> +
> +	ret = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, &page_data,
> +							       info->extack);
> +	if (ret < 0)
> +		goto err_ops;
> +
> +	reply->length = ret;
> +	reply->data = page_data.data;
> +
> +	ethnl_ops_complete(dev);
> +	return 0;
> +
> +err_ops:
> +	ethnl_ops_complete(dev);
> +err_free:
> +	kfree(page_data.data);
> +	return ret;
> +}
> +
> +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);
> +	struct net_device *dev = req_info->dev;
> +
> +	if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
> +		return -EINVAL;
> +
> +	request->i2c_address = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
> +	if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
> +		return -EINVAL;

Max value should be set in the netlink policy.

> +	request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
> +	request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
> +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
> +	    dev->ethtool_ops->get_module_eeprom_data_by_page &&

Why check dev->ethtool_ops->get_module_eeprom_data_by_page here?

> +	    request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
> +		return -EINVAL;
> +
> +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
> +		request->page = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
> +	if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
> +		request->bank = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
> +
> +	return 0;
> +}
> +
> +static int eeprom_data_reply_size(const struct ethnl_req_info *req_base,
> +				  const struct ethnl_reply_data *reply_base)
> +{
> +	const struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_base);
> +
> +	return nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_LENGTH */
> +	       nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA */
> +}

Why report length? Is the netlink length of the DATA attribute not
sufficient?

Should we echo back offset in case driver wants to report re-aligned
data?

> +const struct nla_policy ethnl_eeprom_data_get_policy[] = {
> +	[ETHTOOL_A_EEPROM_DATA_HEADER]		= NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_EEPROM_DATA_OFFSET]		= { .type = NLA_U32 },
> +	[ETHTOOL_A_EEPROM_DATA_LENGTH]		= { .type = NLA_U32 },
> +	[ETHTOOL_A_EEPROM_DATA_PAGE]		= { .type = NLA_U8 },
> +	[ETHTOOL_A_EEPROM_DATA_BANK]		= { .type = NLA_U8 },
> +	[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]	= { .type = NLA_U8 },
> +	[ETHTOOL_A_EEPROM_DATA]			= { .type = NLA_BINARY },

This is a policy for inputs, DATA should not be allowed on input, right?

RE: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Don Bollinger]

On Mon, 15 Mar 2021 10:12:39 +0700 Moshe Shemesh wrote:
> From: Vladyslav Tarasiuk <vladyslavt@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 <vladyslavt@nvidia.com>
> ---
>  Documentation/networking/ethtool-netlink.rst |  34 ++++-
>  include/linux/ethtool.h                      |   8 +-
>  include/uapi/linux/ethtool.h                 |  25 +++
>  include/uapi/linux/ethtool_netlink.h         |  19 +++
>  net/ethtool/Makefile                         |   2 +-
>  net/ethtool/eeprom.c                         | 153 +++++++++++++++++++
>  net/ethtool/netlink.c                        |  10 ++
>  net/ethtool/netlink.h                        |   2 +
>  8 files changed, 249 insertions(+), 4 deletions(-)  create mode 100644
> net/ethtool/eeprom.c
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst
> b/Documentation/networking/ethtool-netlink.rst
> index 05073482db05..25846b97632a 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -1280,6 +1280,36 @@ Kernel response contents:
>  For UDP tunnel table empty ``ETHTOOL_A_TUNNEL_UDP_TABLE_TYPES``
> indicates that  the table contains static entries, hard-coded by the NIC.
> 
> +EEPROM_DATA
> +===========
> +
> +Fetch module EEPROM data dump.
> +
> +Request contents:
> +
> +  =====================================  ======
> ==========================
> +  ``ETHTOOL_A_EEPROM_DATA_HEADER``       nested  request header
> +  ``ETHTOOL_A_EEPROM_DATA_OFFSET``       u32     offset within a page
> +  ``ETHTOOL_A_EEPROM_DATA_LENGTH``       u32     amount of bytes to read
> +  ``ETHTOOL_A_EEPROM_DATA_PAGE``         u8      page number
> +  ``ETHTOOL_A_EEPROM_DATA_BANK``         u8      bank number
> +  ``ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS``  u8      page I2C address
> +  =====================================  ======
> + ==========================
> +
> +Kernel response contents:
> +
> +
+---------------------------------------------+--------+--------------------
-+
> + | ``ETHTOOL_A_EEPROM_DATA_HEADER``            | nested | reply header
> |
> +
+---------------------------------------------+--------+--------------------
-+
> + | ``ETHTOOL_A_EEPROM_DATA_LENGTH``            | u32    | amount of bytes
> read|
> +
+---------------------------------------------+--------+--------------------
-+
> + | ``ETHTOOL_A_EEPROM_DATA``                   | nested | array of bytes
from |
> + |                                             |        | module EEPROM
|
> +
+---------------------------------------------+--------+--------------------
-+
> +
> +``ETHTOOL_A_EEPROM_DATA`` has an attribute length equal to the amount
> +of bytes driver actually read.
> +
>  Request translation
>  ===================
> 
> @@ -1357,8 +1387,8 @@ are netlink only.
>    ``ETHTOOL_GET_DUMP_FLAG``           n/a
>    ``ETHTOOL_GET_DUMP_DATA``           n/a
>    ``ETHTOOL_GET_TS_INFO``             ``ETHTOOL_MSG_TSINFO_GET``
> -  ``ETHTOOL_GMODULEINFO``             n/a
> -  ``ETHTOOL_GMODULEEEPROM``           n/a
> +  ``ETHTOOL_GMODULEINFO``
> ``ETHTOOL_MSG_MODULE_EEPROM_GET``
> +  ``ETHTOOL_GMODULEEEPROM``
> ``ETHTOOL_MSG_MODULE_EEPROM_GET``
>    ``ETHTOOL_GEEE``                    ``ETHTOOL_MSG_EEE_GET``
>    ``ETHTOOL_SEEE``                    ``ETHTOOL_MSG_EEE_SET``
>    ``ETHTOOL_GRSSH``                   n/a
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index
> ec4cd3921c67..9551005b350a 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -81,6 +81,7 @@ enum {
>  #define ETH_RSS_HASH_NO_CHANGE	0
> 
>  struct net_device;
> +struct netlink_ext_ack;
> 
>  /* Some generic methods drivers may use in their ethtool_ops */
>  u32 ethtool_op_get_link(struct net_device *dev); @@ -410,6 +411,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. Returns a negative error code or the amount of
> + *	bytes read.
>   *
>   * 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
> +519,9 @@ 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,
> +						  const struct
> ethtool_eeprom_data *page,
> +						  struct netlink_ext_ack
> *extack);
>  };
> 
>  int ethtool_check_ops(const struct ethtool_ops *ops); @@ -538,7 +545,6
> @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>  				       const struct ethtool_link_ksettings
*cmd,
>  				       u32 *dev_speed, u8 *dev_duplex);
> 
> -struct netlink_ext_ack;
>  struct phy_device;
>  struct phy_tdr_config;
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index
> cde753bb2093..b3e92db3ad37 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -340,6 +340,28 @@ struct ethtool_eeprom {
>  	__u8	data[0];
>  };
> 
> +/**
> + * 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.
> + * @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. Value less than 0x7f expected.
> Most
> + *	EEPROMs use 0x50 or 0x51.
> + * @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;
> +	__u8	page;
> +	__u8	bank;
> +	__u8	i2c_address;
> +	__u8	*data;
> +};
> +
>  /**
>   * struct ethtool_eee - Energy Efficient Ethernet information
>   * @cmd: ETHTOOL_{G,S}EEE
> @@ -1865,6 +1887,9 @@ 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_PAGE_LEN	256
> +#define ETH_MODULE_MAX_I2C_ADDRESS	0x7f
> +
>  /* 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..e1b1b962f3da 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,			/* nest -
> _A_HEADER_* */
> +
> +	ETHTOOL_A_EEPROM_DATA_OFFSET,			/* u32 */
> +	ETHTOOL_A_EEPROM_DATA_LENGTH,			/* u32 */
> +	ETHTOOL_A_EEPROM_DATA_PAGE,			/* u8 */
> +	ETHTOOL_A_EEPROM_DATA_BANK,			/* u8 */
> +	ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,		/* u8 */
> +	ETHTOOL_A_EEPROM_DATA,				/* nested */
> +
> +	__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..e110336dc231
> --- /dev/null
> +++ b/net/ethtool/eeprom.c
> @@ -0,0 +1,153 @@
> +// 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;
> +	u8			page;
> +	u8			bank;
> +	u8			i2c_address;
> +};
> +
> +struct eeprom_data_reply_data {
> +	struct ethnl_reply_data base;
> +	u32			length;
> +	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 ret;
> +
> +	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
> +		return -EOPNOTSUPP;
> +
> +	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;
> +	ret = ethnl_ops_begin(dev);
> +	if (ret)
> +		goto err_free;
> +
> +	ret = dev->ethtool_ops->get_module_eeprom_data_by_page(dev,
> &page_data,
> +
info->extack);
> +	if (ret < 0)
> +		goto err_ops;
> +
> +	reply->length = ret;
> +	reply->data = page_data.data;
> +
> +	ethnl_ops_complete(dev);
> +	return 0;
> +
> +err_ops:
> +	ethnl_ops_complete(dev);
> +err_free:
> +	kfree(page_data.data);
> +	return ret;
> +}
> +
> +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);
> +	struct net_device *dev = req_info->dev;
> +
> +	if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
> +		return -EINVAL;

Suggestion:  Consider using i2c address 0x50 as a default if none is given.
0x50 is the first 256 bytes of SFP, and all of QSFP and CMIS EEPROM.  If
there is a page given on an SFP device, then you know i2c address is 0x51.
The only thing that REQUIRES 0x51 is legacy offset 256-511 on SFP.  Keep the
i2c address, but make it optional for the non-standard devices that Andrew
has mentioned, and for that one section of SFP data that requires it.  And
document it for the user.

Note also, legacy does not have an i2c address, so passing this along allows
the fallback to legacy to cover more cases.  (Legacy ethtool addresses this
part of SFP as offset 256-511.)

> +
> +	request->i2c_address =
> nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
> +	if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
> +		return -EINVAL;
> +
> +	request->offset =
> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
> +	request->length =
> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
> +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
> +	    dev->ethtool_ops->get_module_eeprom_data_by_page &&
> +	    request->offset + request->length >
> ETH_MODULE_EEPROM_PAGE_LEN)
> +		return -EINVAL;

If a request exceeds the length of EEPROM (in legacy), we truncate it to
EEPROM length.  I suggest if a request exceeds the length of the page (in
the new KAPI), then we truncate at the end of the page.  Thus rather than
'return -EINVAL;' I suggest

          request->length = ETH_MODULE_EEPROM_PAGE_LEN - request->offset;

> +
> +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
> +		request->page =
> nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
> +	if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
> +		request->bank =
> nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
> +
> +	return 0;
> +}
> +
> +static int eeprom_data_reply_size(const struct ethnl_req_info *req_base,
> +				  const struct ethnl_reply_data *reply_base)
{
> +	const struct eeprom_data_req_info *request =
> +EEPROM_DATA_REQINFO(req_base);
> +
> +	return nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_LENGTH */
> +	       nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA
> +*/ }
> +
> +static int eeprom_data_fill_reply(struct sk_buff *skb,
> +				  const struct ethnl_req_info *req_base,
> +				  const struct ethnl_reply_data *reply_base)
{
> +	struct eeprom_data_reply_data *reply =
> +EEPROM_DATA_REPDATA(reply_base);
> +
> +	if (nla_put_u32(skb, ETHTOOL_A_EEPROM_DATA_LENGTH, reply-
> >length) ||
> +	    nla_put(skb, ETHTOOL_A_EEPROM_DATA, reply->length, reply-
> >data))
> +		return -EMSGSIZE;
> +
> +	return 0;
> +}
> +
> +static void eeprom_data_cleanup_data(struct ethnl_reply_data
> +*reply_base) {
> +	struct eeprom_data_reply_data *reply =
> +EEPROM_DATA_REPDATA(reply_base);
> +
> +	kfree(reply->data);
> +}
> +
> +const struct ethnl_request_ops ethnl_eeprom_data_request_ops = {
> +	.request_cmd		= ETHTOOL_MSG_EEPROM_DATA_GET,
> +	.reply_cmd		=
> ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
> +	.hdr_attr		= ETHTOOL_A_EEPROM_DATA_HEADER,
> +	.req_info_size		= sizeof(struct eeprom_data_req_info),
> +	.reply_data_size	= sizeof(struct eeprom_data_reply_data),
> +
> +	.parse_request		= eeprom_data_parse_request,
> +	.prepare_data		= eeprom_data_prepare_data,
> +	.reply_size		= eeprom_data_reply_size,
> +	.fill_reply		= eeprom_data_fill_reply,
> +	.cleanup_data		= eeprom_data_cleanup_data,
> +};
> +
> +const struct nla_policy ethnl_eeprom_data_get_policy[] = {
> +	[ETHTOOL_A_EEPROM_DATA_HEADER]		=
> NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_EEPROM_DATA_OFFSET]		= { .type =
> NLA_U32 },
> +	[ETHTOOL_A_EEPROM_DATA_LENGTH]		= { .type =
> NLA_U32 },
> +	[ETHTOOL_A_EEPROM_DATA_PAGE]		= { .type = NLA_U8 },
> +	[ETHTOOL_A_EEPROM_DATA_BANK]		= { .type = NLA_U8 },
> +	[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]	= { .type = NLA_U8 },
> +	[ETHTOOL_A_EEPROM_DATA]			= { .type =
> NLA_BINARY },
> +};
> +
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index
> 50d3c8896f91..ff2528bee192 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -245,6 +245,7 @@
> ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
>  	[ETHTOOL_MSG_PAUSE_GET]		=
> &ethnl_pause_request_ops,
>  	[ETHTOOL_MSG_EEE_GET]		= &ethnl_eee_request_ops,
>  	[ETHTOOL_MSG_TSINFO_GET]	= &ethnl_tsinfo_request_ops,
> +	[ETHTOOL_MSG_EEPROM_DATA_GET]	=
> &ethnl_eeprom_data_request_ops,
>  };
> 
>  static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback
> *cb) @@ -912,6 +913,15 @@ static const struct genl_ops ethtool_genl_ops[]
> = {
>  		.policy = ethnl_tunnel_info_get_policy,
>  		.maxattr = ARRAY_SIZE(ethnl_tunnel_info_get_policy) - 1,
>  	},
> +	{
> +		.cmd	= ETHTOOL_MSG_EEPROM_DATA_GET,
> +		.doit	= ethnl_default_doit,
> +		.start	= ethnl_default_start,
> +		.dumpit	= ethnl_default_dumpit,
> +		.done	= ethnl_default_done,
> +		.policy = ethnl_eeprom_data_get_policy,
> +		.maxattr = ARRAY_SIZE(ethnl_eeprom_data_get_policy) - 1,
> +	},
>  };
> 
>  static const struct genl_multicast_group ethtool_nl_mcgrps[] = { diff
--git
> a/net/ethtool/netlink.h b/net/ethtool/netlink.h index
> 6eabd58d81bf..60954c7b4dfe 100644
> --- a/net/ethtool/netlink.h
> +++ b/net/ethtool/netlink.h
> @@ -344,6 +344,7 @@ extern const struct ethnl_request_ops
> ethnl_coalesce_request_ops;  extern const struct ethnl_request_ops
> ethnl_pause_request_ops;  extern const struct ethnl_request_ops
> ethnl_eee_request_ops;  extern const struct ethnl_request_ops
> ethnl_tsinfo_request_ops;
> +extern const struct ethnl_request_ops ethnl_eeprom_data_request_ops;
> 
>  extern const struct nla_policy
> ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];  extern const struct
> nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1]; @@
> -375,6 +376,7 @@ extern const struct nla_policy
> ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_HEADER +  extern const struct
> nla_policy ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_HEADER +
> 1];  extern const struct nla_policy
> ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG + 1];
> extern const struct nla_policy
> ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_HEADER + 1];
> +extern const struct nla_policy
> +ethnl_eeprom_data_get_policy[ETHTOOL_A_EEPROM_DATA + 1];
> 
>  int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);  int
> ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
> --
> 2.26.2

RE: [RFC PATCH V3 net-next 5/5] ethtool: Add fallback to get_module_eeprom from netlink command

[Don Bollinger]

On Mon, 15 Mar 2021 10:12:39 +0700 Moshe Shemesh wrote:
> From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
> 
> In case netlink get_module_eeprom_data_by_page() callback is not
> implemented by the driver, try to call old get_module_info() and
> get_module_eeprom() pair. Recalculate parameters to
> get_module_eeprom() offset and len using page number and their sizes.
> Return error if this can't be done.
> 
> Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
> ---
>  net/ethtool/eeprom.c | 75
> +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c index
> e110336dc231..33ba9ecc36cb 100644
> --- a/net/ethtool/eeprom.c
> +++ b/net/ethtool/eeprom.c
> @@ -25,6 +25,79 @@ struct eeprom_data_reply_data {  #define
> EEPROM_DATA_REPDATA(__reply_base) \
>  	container_of(__reply_base, struct eeprom_data_reply_data, base)
> 
> +static int fallback_set_params(struct eeprom_data_req_info *request,
> +			       struct ethtool_modinfo *modinfo,
> +			       struct ethtool_eeprom *eeprom) {
> +	u32 offset = request->offset;
> +	u32 length = request->length;
> +
> +	if (request->page) {
> +		if (offset < 128 || offset + length >
> ETH_MODULE_EEPROM_PAGE_LEN)
> +			return -EINVAL;
> +		offset = request->page * 128 + offset;
> +	}
> +
> +	if (modinfo->type == ETH_MODULE_SFF_8079 &&
> +	    request->i2c_address == 0x51)
> +		offset += ETH_MODULE_EEPROM_PAGE_LEN;
> +
> +	if (!length)
> +		length = modinfo->eeprom_len;

Need to move this check up 11 lines, before the 'if (request->page)...'
stanza.  You need to be doing that check against the final length.
Otherwise you could have a request that includes a page, and a length of 0,
which becomes a length of modinfo->eeprom_len.

> +
> +	if (offset >= modinfo->eeprom_len)
> +		return -EINVAL;
> +
> +	if (modinfo->eeprom_len < offset + length)
> +		length = modinfo->eeprom_len - offset;
> +
> +	eeprom->cmd = ETHTOOL_GMODULEEEPROM;
> +	eeprom->len = length;
> +	eeprom->offset = offset;
> +
> +	return 0;
> +}
> +
> +static int eeprom_data_fallback(struct eeprom_data_req_info *request,
> +				struct eeprom_data_reply_data *reply,
> +				struct genl_info *info)
> +{
> +	struct net_device *dev = reply->base.dev;
> +	struct ethtool_modinfo modinfo = {0};
> +	struct ethtool_eeprom eeprom = {0};
> +	u8 *data;
> +	int err;
> +
> +	if ((!dev->ethtool_ops->get_module_info &&
> +	     !dev->ethtool_ops->get_module_eeprom) || request->bank) {
> +		return -EOPNOTSUPP;
> +	}
> +	modinfo.cmd = ETHTOOL_GMODULEINFO;
> +	err = dev->ethtool_ops->get_module_info(dev, &modinfo);
> +	if (err < 0)
> +		return err;
> +
> +	err = fallback_set_params(request, &modinfo, &eeprom);
> +	if (err < 0)
> +		return err;
> +
> +	data = kmalloc(eeprom.len, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +	err = dev->ethtool_ops->get_module_eeprom(dev, &eeprom,
> data);
> +	if (err < 0)
> +		goto err_out;
> +
> +	reply->data = data;
> +	reply->length = eeprom.len;
> +
> +	return 0;
> +
> +err_out:
> +	kfree(data);
> +	return err;
> +}
> +
>  static int eeprom_data_prepare_data(const struct ethnl_req_info
> *req_base,
>  				    struct ethnl_reply_data *reply_base,
>  				    struct genl_info *info)
> @@ -36,7 +109,7 @@ static int eeprom_data_prepare_data(const struct
> ethnl_req_info *req_base,
>  	int ret;
> 
>  	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
> -		return -EOPNOTSUPP;
> +		return eeprom_data_fallback(request, reply, info);
> 
>  	page_data.offset = request->offset;
>  	page_data.length = request->length;
> --
> 2.26.2

Re: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Moshe Shemesh]

On 3/15/2021 11:01 PM, Jakub Kicinski wrote:
>
> On Mon, 15 Mar 2021 19:12:39 +0200 Moshe Shemesh wrote:
>> From: Vladyslav Tarasiuk <vladyslavt@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 <vladyslavt@nvidia.com>
> s/eeprom_data/eeprom/ everywhere?
>
> Otherwise some of your identifiers are over 30 characters long.


That can work part to one struct on the uapi file that can conflict with 
previous old KAPI struct, but we can figure it too.

Will fix.

>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index cde753bb2093..b3e92db3ad37 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -340,6 +340,28 @@ struct ethtool_eeprom {
>>        __u8    data[0];
>>   };
>>
>> +/**
>> + * 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.
>> + * @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. Value less than 0x7f expected. Most
>> + *   EEPROMs use 0x50 or 0x51.
>> + * @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;
>> +     __u8    page;
>> +     __u8    bank;
>> +     __u8    i2c_address;
>> +     __u8    *data;
>> +};
>> +
> This structure does not have to be part of uAPI, right?
Right, will fix.
>> +#define ETH_MODULE_EEPROM_PAGE_LEN   256
>> +#define ETH_MODULE_MAX_I2C_ADDRESS   0x7f
> Not sure about these defines either.
Probably.
>> +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 ret;
>> +
>> +     if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
>> +             return -EOPNOTSUPP;
>> +
>> +     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;
> nit: new line?
Ack.
>> +     ret = ethnl_ops_begin(dev);
>> +     if (ret)
>> +             goto err_free;
>> +
>> +     ret = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, &page_data,
>> +                                                            info->extack);
>> +     if (ret < 0)
>> +             goto err_ops;
>> +
>> +     reply->length = ret;
>> +     reply->data = page_data.data;
>> +
>> +     ethnl_ops_complete(dev);
>> +     return 0;
>> +
>> +err_ops:
>> +     ethnl_ops_complete(dev);
>> +err_free:
>> +     kfree(page_data.data);
>> +     return ret;
>> +}
>> +
>> +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);
>> +     struct net_device *dev = req_info->dev;
>> +
>> +     if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
>> +         !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
>> +         !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
>> +             return -EINVAL;
>> +
>> +     request->i2c_address = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
>> +     if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
>> +             return -EINVAL;
> Max value should be set in the netlink policy.
Ack
>> +     request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
>> +     request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
>> +     if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
>> +         dev->ethtool_ops->get_module_eeprom_data_by_page &&
> Why check dev->ethtool_ops->get_module_eeprom_data_by_page here?
Right, actually not needed here, thanks.
>> +         request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
>> +             return -EINVAL;
>> +
>> +     if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
>> +             request->page = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
>> +     if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
>> +             request->bank = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
>> +
>> +     return 0;
>> +}
>> +
>> +static int eeprom_data_reply_size(const struct ethnl_req_info *req_base,
>> +                               const struct ethnl_reply_data *reply_base)
>> +{
>> +     const struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_base);
>> +
>> +     return nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_LENGTH */
>> +            nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA */
>> +}
> Why report length? Is the netlink length of the DATA attribute not
> sufficient?
Right, thanks.
> Should we echo back offset in case driver wants to report re-aligned
> data?
The driver knows the requested offset and data has to be from that offset.
>> +const struct nla_policy ethnl_eeprom_data_get_policy[] = {
>> +     [ETHTOOL_A_EEPROM_DATA_HEADER]          = NLA_POLICY_NESTED(ethnl_header_policy),
>> +     [ETHTOOL_A_EEPROM_DATA_OFFSET]          = { .type = NLA_U32 },
>> +     [ETHTOOL_A_EEPROM_DATA_LENGTH]          = { .type = NLA_U32 },
>> +     [ETHTOOL_A_EEPROM_DATA_PAGE]            = { .type = NLA_U8 },
>> +     [ETHTOOL_A_EEPROM_DATA_BANK]            = { .type = NLA_U8 },
>> +     [ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]     = { .type = NLA_U8 },
>> +     [ETHTOOL_A_EEPROM_DATA]                 = { .type = NLA_BINARY },
> This is a policy for inputs, DATA should not be allowed on input, right?
Right, thanks.

Re: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Moshe Shemesh]

On 3/16/2021 12:31 AM, Don Bollinger wrote:
> On Mon, 15 Mar 2021 10:12:39 +0700 Moshe Shemesh wrote:
>> From: Vladyslav Tarasiuk <vladyslavt@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 <vladyslavt@nvidia.com>
>> ---
>>   Documentation/networking/ethtool-netlink.rst |  34 ++++-
>>   include/linux/ethtool.h                      |   8 +-
>>   include/uapi/linux/ethtool.h                 |  25 +++
>>   include/uapi/linux/ethtool_netlink.h         |  19 +++
>>   net/ethtool/Makefile                         |   2 +-
>>   net/ethtool/eeprom.c                         | 153 +++++++++++++++++++
>>   net/ethtool/netlink.c                        |  10 ++
>>   net/ethtool/netlink.h                        |   2 +
>>   8 files changed, 249 insertions(+), 4 deletions(-)  create mode 100644
>> net/ethtool/eeprom.c
>>
>> diff --git a/Documentation/networking/ethtool-netlink.rst
>> b/Documentation/networking/ethtool-netlink.rst
>> index 05073482db05..25846b97632a 100644
>> --- a/Documentation/networking/ethtool-netlink.rst
>> +++ b/Documentation/networking/ethtool-netlink.rst
>> @@ -1280,6 +1280,36 @@ Kernel response contents:
>>   For UDP tunnel table empty ``ETHTOOL_A_TUNNEL_UDP_TABLE_TYPES``
>> indicates that  the table contains static entries, hard-coded by the NIC.
>>
>> +EEPROM_DATA
>> +===========
>> +
>> +Fetch module EEPROM data dump.
>> +
>> +Request contents:
>> +
>> +  =====================================  ======
>> ==========================
>> +  ``ETHTOOL_A_EEPROM_DATA_HEADER``       nested  request header
>> +  ``ETHTOOL_A_EEPROM_DATA_OFFSET``       u32     offset within a page
>> +  ``ETHTOOL_A_EEPROM_DATA_LENGTH``       u32     amount of bytes to read
>> +  ``ETHTOOL_A_EEPROM_DATA_PAGE``         u8      page number
>> +  ``ETHTOOL_A_EEPROM_DATA_BANK``         u8      bank number
>> +  ``ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS``  u8      page I2C address
>> +  =====================================  ======
>> + ==========================
>> +
>> +Kernel response contents:
>> +
>> +
> +---------------------------------------------+--------+--------------------
> -+
>> + | ``ETHTOOL_A_EEPROM_DATA_HEADER``            | nested | reply header
>> |
>> +
> +---------------------------------------------+--------+--------------------
> -+
>> + | ``ETHTOOL_A_EEPROM_DATA_LENGTH``            | u32    | amount of bytes
>> read|
>> +
> +---------------------------------------------+--------+--------------------
> -+
>> + | ``ETHTOOL_A_EEPROM_DATA``                   | nested | array of bytes
> from |
>> + |                                             |        | module EEPROM
> |
>> +
> +---------------------------------------------+--------+--------------------
> -+
>> +
>> +``ETHTOOL_A_EEPROM_DATA`` has an attribute length equal to the amount
>> +of bytes driver actually read.
>> +
>>   Request translation
>>   ===================
>>
>> @@ -1357,8 +1387,8 @@ are netlink only.
>>     ``ETHTOOL_GET_DUMP_FLAG``           n/a
>>     ``ETHTOOL_GET_DUMP_DATA``           n/a
>>     ``ETHTOOL_GET_TS_INFO``             ``ETHTOOL_MSG_TSINFO_GET``
>> -  ``ETHTOOL_GMODULEINFO``             n/a
>> -  ``ETHTOOL_GMODULEEEPROM``           n/a
>> +  ``ETHTOOL_GMODULEINFO``
>> ``ETHTOOL_MSG_MODULE_EEPROM_GET``
>> +  ``ETHTOOL_GMODULEEEPROM``
>> ``ETHTOOL_MSG_MODULE_EEPROM_GET``
>>     ``ETHTOOL_GEEE``                    ``ETHTOOL_MSG_EEE_GET``
>>     ``ETHTOOL_SEEE``                    ``ETHTOOL_MSG_EEE_SET``
>>     ``ETHTOOL_GRSSH``                   n/a
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index
>> ec4cd3921c67..9551005b350a 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -81,6 +81,7 @@ enum {
>>   #define ETH_RSS_HASH_NO_CHANGE       0
>>
>>   struct net_device;
>> +struct netlink_ext_ack;
>>
>>   /* Some generic methods drivers may use in their ethtool_ops */
>>   u32 ethtool_op_get_link(struct net_device *dev); @@ -410,6 +411,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. Returns a negative error code or the amount of
>> + *   bytes read.
>>    *
>>    * 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
>> +519,9 @@ 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,
>> +                                               const struct
>> ethtool_eeprom_data *page,
>> +                                               struct netlink_ext_ack
>> *extack);
>>   };
>>
>>   int ethtool_check_ops(const struct ethtool_ops *ops); @@ -538,7 +545,6
>> @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>>                                       const struct ethtool_link_ksettings
> *cmd,
>>                                       u32 *dev_speed, u8 *dev_duplex);
>>
>> -struct netlink_ext_ack;
>>   struct phy_device;
>>   struct phy_tdr_config;
>>
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index
>> cde753bb2093..b3e92db3ad37 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -340,6 +340,28 @@ struct ethtool_eeprom {
>>        __u8    data[0];
>>   };
>>
>> +/**
>> + * 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.
>> + * @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. Value less than 0x7f expected.
>> Most
>> + *   EEPROMs use 0x50 or 0x51.
>> + * @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;
>> +     __u8    page;
>> +     __u8    bank;
>> +     __u8    i2c_address;
>> +     __u8    *data;
>> +};
>> +
>>   /**
>>    * struct ethtool_eee - Energy Efficient Ethernet information
>>    * @cmd: ETHTOOL_{G,S}EEE
>> @@ -1865,6 +1887,9 @@ 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_PAGE_LEN   256
>> +#define ETH_MODULE_MAX_I2C_ADDRESS   0x7f
>> +
>>   /* 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..e1b1b962f3da 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,                   /* nest -
>> _A_HEADER_* */
>> +
>> +     ETHTOOL_A_EEPROM_DATA_OFFSET,                   /* u32 */
>> +     ETHTOOL_A_EEPROM_DATA_LENGTH,                   /* u32 */
>> +     ETHTOOL_A_EEPROM_DATA_PAGE,                     /* u8 */
>> +     ETHTOOL_A_EEPROM_DATA_BANK,                     /* u8 */
>> +     ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,              /* u8 */
>> +     ETHTOOL_A_EEPROM_DATA,                          /* nested */
>> +
>> +     __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..e110336dc231
>> --- /dev/null
>> +++ b/net/ethtool/eeprom.c
>> @@ -0,0 +1,153 @@
>> +// 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;
>> +     u8                      page;
>> +     u8                      bank;
>> +     u8                      i2c_address;
>> +};
>> +
>> +struct eeprom_data_reply_data {
>> +     struct ethnl_reply_data base;
>> +     u32                     length;
>> +     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 ret;
>> +
>> +     if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
>> +             return -EOPNOTSUPP;
>> +
>> +     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;
>> +     ret = ethnl_ops_begin(dev);
>> +     if (ret)
>> +             goto err_free;
>> +
>> +     ret = dev->ethtool_ops->get_module_eeprom_data_by_page(dev,
>> &page_data,
>> +
> info->extack);
>> +     if (ret < 0)
>> +             goto err_ops;
>> +
>> +     reply->length = ret;
>> +     reply->data = page_data.data;
>> +
>> +     ethnl_ops_complete(dev);
>> +     return 0;
>> +
>> +err_ops:
>> +     ethnl_ops_complete(dev);
>> +err_free:
>> +     kfree(page_data.data);
>> +     return ret;
>> +}
>> +
>> +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);
>> +     struct net_device *dev = req_info->dev;
>> +
>> +     if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
>> +         !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
>> +         !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
>> +             return -EINVAL;
> Suggestion:  Consider using i2c address 0x50 as a default if none is given.
> 0x50 is the first 256 bytes of SFP, and all of QSFP and CMIS EEPROM.  If
> there is a page given on an SFP device, then you know i2c address is 0x51.
> The only thing that REQUIRES 0x51 is legacy offset 256-511 on SFP.  Keep the
> i2c address, but make it optional for the non-standard devices that Andrew
> has mentioned, and for that one section of SFP data that requires it.  And
> document it for the user.
Agree, but thought to have that i2c address default set on userspace, so 
here we expect it.
> Note also, legacy does not have an i2c address, so passing this along allows
> the fallback to legacy to cover more cases.  (Legacy ethtool addresses this
> part of SFP as offset 256-511.)
>
>> +
>> +     request->i2c_address =
>> nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
>> +     if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
>> +             return -EINVAL;
>> +
>> +     request->offset =
>> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
>> +     request->length =
>> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
>> +     if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
>> +         dev->ethtool_ops->get_module_eeprom_data_by_page &&
>> +         request->offset + request->length >
>> ETH_MODULE_EEPROM_PAGE_LEN)
>> +             return -EINVAL;
> If a request exceeds the length of EEPROM (in legacy), we truncate it to
> EEPROM length.  I suggest if a request exceeds the length of the page (in
> the new KAPI), then we truncate at the end of the page.  Thus rather than
> 'return -EINVAL;' I suggest
>
>            request->length = ETH_MODULE_EEPROM_PAGE_LEN - request->offset;

I was not sure about that, isn't it better that once user specified page 
he has to be in the page boundary and let him know the command failed ?

>> +
>> +     if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
>> +             request->page =
>> nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
>> +     if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
>> +             request->bank =
>> nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
>> +
>> +     return 0;
>> +}
>> +
>> +static int eeprom_data_reply_size(const struct ethnl_req_info *req_base,
>> +                               const struct ethnl_reply_data *reply_base)
> {
>> +     const struct eeprom_data_req_info *request =
>> +EEPROM_DATA_REQINFO(req_base);
>> +
>> +     return nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_LENGTH */
>> +            nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA
>> +*/ }
>> +
>> +static int eeprom_data_fill_reply(struct sk_buff *skb,
>> +                               const struct ethnl_req_info *req_base,
>> +                               const struct ethnl_reply_data *reply_base)
> {
>> +     struct eeprom_data_reply_data *reply =
>> +EEPROM_DATA_REPDATA(reply_base);
>> +
>> +     if (nla_put_u32(skb, ETHTOOL_A_EEPROM_DATA_LENGTH, reply-
>>> length) ||
>> +         nla_put(skb, ETHTOOL_A_EEPROM_DATA, reply->length, reply-
>>> data))
>> +             return -EMSGSIZE;
>> +
>> +     return 0;
>> +}
>> +
>> +static void eeprom_data_cleanup_data(struct ethnl_reply_data
>> +*reply_base) {
>> +     struct eeprom_data_reply_data *reply =
>> +EEPROM_DATA_REPDATA(reply_base);
>> +
>> +     kfree(reply->data);
>> +}
>> +
>> +const struct ethnl_request_ops ethnl_eeprom_data_request_ops = {
>> +     .request_cmd            = ETHTOOL_MSG_EEPROM_DATA_GET,
>> +     .reply_cmd              =
>> ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
>> +     .hdr_attr               = ETHTOOL_A_EEPROM_DATA_HEADER,
>> +     .req_info_size          = sizeof(struct eeprom_data_req_info),
>> +     .reply_data_size        = sizeof(struct eeprom_data_reply_data),
>> +
>> +     .parse_request          = eeprom_data_parse_request,
>> +     .prepare_data           = eeprom_data_prepare_data,
>> +     .reply_size             = eeprom_data_reply_size,
>> +     .fill_reply             = eeprom_data_fill_reply,
>> +     .cleanup_data           = eeprom_data_cleanup_data,
>> +};
>> +
>> +const struct nla_policy ethnl_eeprom_data_get_policy[] = {
>> +     [ETHTOOL_A_EEPROM_DATA_HEADER]          =
>> NLA_POLICY_NESTED(ethnl_header_policy),
>> +     [ETHTOOL_A_EEPROM_DATA_OFFSET]          = { .type =
>> NLA_U32 },
>> +     [ETHTOOL_A_EEPROM_DATA_LENGTH]          = { .type =
>> NLA_U32 },
>> +     [ETHTOOL_A_EEPROM_DATA_PAGE]            = { .type = NLA_U8 },
>> +     [ETHTOOL_A_EEPROM_DATA_BANK]            = { .type = NLA_U8 },
>> +     [ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]     = { .type = NLA_U8 },
>> +     [ETHTOOL_A_EEPROM_DATA]                 = { .type =
>> NLA_BINARY },
>> +};
>> +
>> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index
>> 50d3c8896f91..ff2528bee192 100644
>> --- a/net/ethtool/netlink.c
>> +++ b/net/ethtool/netlink.c
>> @@ -245,6 +245,7 @@
>> ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
>>        [ETHTOOL_MSG_PAUSE_GET]         =
>> &ethnl_pause_request_ops,
>>        [ETHTOOL_MSG_EEE_GET]           = &ethnl_eee_request_ops,
>>        [ETHTOOL_MSG_TSINFO_GET]        = &ethnl_tsinfo_request_ops,
>> +     [ETHTOOL_MSG_EEPROM_DATA_GET]   =
>> &ethnl_eeprom_data_request_ops,
>>   };
>>
>>   static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback
>> *cb) @@ -912,6 +913,15 @@ static const struct genl_ops ethtool_genl_ops[]
>> = {
>>                .policy = ethnl_tunnel_info_get_policy,
>>                .maxattr = ARRAY_SIZE(ethnl_tunnel_info_get_policy) - 1,
>>        },
>> +     {
>> +             .cmd    = ETHTOOL_MSG_EEPROM_DATA_GET,
>> +             .doit   = ethnl_default_doit,
>> +             .start  = ethnl_default_start,
>> +             .dumpit = ethnl_default_dumpit,
>> +             .done   = ethnl_default_done,
>> +             .policy = ethnl_eeprom_data_get_policy,
>> +             .maxattr = ARRAY_SIZE(ethnl_eeprom_data_get_policy) - 1,
>> +     },
>>   };
>>
>>   static const struct genl_multicast_group ethtool_nl_mcgrps[] = { diff
> --git
>> a/net/ethtool/netlink.h b/net/ethtool/netlink.h index
>> 6eabd58d81bf..60954c7b4dfe 100644
>> --- a/net/ethtool/netlink.h
>> +++ b/net/ethtool/netlink.h
>> @@ -344,6 +344,7 @@ extern const struct ethnl_request_ops
>> ethnl_coalesce_request_ops;  extern const struct ethnl_request_ops
>> ethnl_pause_request_ops;  extern const struct ethnl_request_ops
>> ethnl_eee_request_ops;  extern const struct ethnl_request_ops
>> ethnl_tsinfo_request_ops;
>> +extern const struct ethnl_request_ops ethnl_eeprom_data_request_ops;
>>
>>   extern const struct nla_policy
>> ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];  extern const struct
>> nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1]; @@
>> -375,6 +376,7 @@ extern const struct nla_policy
>> ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_HEADER +  extern const struct
>> nla_policy ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_HEADER +
>> 1];  extern const struct nla_policy
>> ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG + 1];
>> extern const struct nla_policy
>> ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_HEADER + 1];
>> +extern const struct nla_policy
>> +ethnl_eeprom_data_get_policy[ETHTOOL_A_EEPROM_DATA + 1];
>>
>>   int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);  int
>> ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
>> --
>> 2.26.2

Re: [RFC PATCH V3 net-next 5/5] ethtool: Add fallback to get_module_eeprom from netlink command

[Moshe Shemesh]

On 3/16/2021 12:31 AM, Don Bollinger wrote:
>
> On Mon, 15 Mar 2021 10:12:39 +0700 Moshe Shemesh wrote:
>> From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
>>
>> In case netlink get_module_eeprom_data_by_page() callback is not
>> implemented by the driver, try to call old get_module_info() and
>> get_module_eeprom() pair. Recalculate parameters to
>> get_module_eeprom() offset and len using page number and their sizes.
>> Return error if this can't be done.
>>
>> Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
>> ---
>>   net/ethtool/eeprom.c | 75
>> +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 74 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c index
>> e110336dc231..33ba9ecc36cb 100644
>> --- a/net/ethtool/eeprom.c
>> +++ b/net/ethtool/eeprom.c
>> @@ -25,6 +25,79 @@ struct eeprom_data_reply_data {  #define
>> EEPROM_DATA_REPDATA(__reply_base) \
>>        container_of(__reply_base, struct eeprom_data_reply_data, base)
>>
>> +static int fallback_set_params(struct eeprom_data_req_info *request,
>> +                            struct ethtool_modinfo *modinfo,
>> +                            struct ethtool_eeprom *eeprom) {
>> +     u32 offset = request->offset;
>> +     u32 length = request->length;
>> +
>> +     if (request->page) {
>> +             if (offset < 128 || offset + length >
>> ETH_MODULE_EEPROM_PAGE_LEN)
>> +                     return -EINVAL;
>> +             offset = request->page * 128 + offset;
>> +     }
>> +
>> +     if (modinfo->type == ETH_MODULE_SFF_8079 &&
>> +         request->i2c_address == 0x51)
>> +             offset += ETH_MODULE_EEPROM_PAGE_LEN;
>> +
>> +     if (!length)
>> +             length = modinfo->eeprom_len;
> Need to move this check up 11 lines, before the 'if (request->page)...'
> stanza.  You need to be doing that check against the final length.
> Otherwise you could have a request that includes a page, and a length of 0,
> which becomes a length of modinfo->eeprom_len.


Right, thanks.

>> +
>> +     if (offset >= modinfo->eeprom_len)
>> +             return -EINVAL;
>> +
>> +     if (modinfo->eeprom_len < offset + length)
>> +             length = modinfo->eeprom_len - offset;
>> +
>> +     eeprom->cmd = ETHTOOL_GMODULEEEPROM;
>> +     eeprom->len = length;
>> +     eeprom->offset = offset;
>> +
>> +     return 0;
>> +}
>> +
>> +static int eeprom_data_fallback(struct eeprom_data_req_info *request,
>> +                             struct eeprom_data_reply_data *reply,
>> +                             struct genl_info *info)
>> +{
>> +     struct net_device *dev = reply->base.dev;
>> +     struct ethtool_modinfo modinfo = {0};
>> +     struct ethtool_eeprom eeprom = {0};
>> +     u8 *data;
>> +     int err;
>> +
>> +     if ((!dev->ethtool_ops->get_module_info &&
>> +          !dev->ethtool_ops->get_module_eeprom) || request->bank) {
>> +             return -EOPNOTSUPP;
>> +     }
>> +     modinfo.cmd = ETHTOOL_GMODULEINFO;
>> +     err = dev->ethtool_ops->get_module_info(dev, &modinfo);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     err = fallback_set_params(request, &modinfo, &eeprom);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     data = kmalloc(eeprom.len, GFP_KERNEL);
>> +     if (!data)
>> +             return -ENOMEM;
>> +     err = dev->ethtool_ops->get_module_eeprom(dev, &eeprom,
>> data);
>> +     if (err < 0)
>> +             goto err_out;
>> +
>> +     reply->data = data;
>> +     reply->length = eeprom.len;
>> +
>> +     return 0;
>> +
>> +err_out:
>> +     kfree(data);
>> +     return err;
>> +}
>> +
>>   static int eeprom_data_prepare_data(const struct ethnl_req_info
>> *req_base,
>>                                    struct ethnl_reply_data *reply_base,
>>                                    struct genl_info *info)
>> @@ -36,7 +109,7 @@ static int eeprom_data_prepare_data(const struct
>> ethnl_req_info *req_base,
>>        int ret;
>>
>>        if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
>> -             return -EOPNOTSUPP;
>> +             return eeprom_data_fallback(request, reply, info);
>>
>>        page_data.offset = request->offset;
>>        page_data.length = request->length;
>> --
>> 2.26.2

RE: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Don Bollinger]

On 3/16/2021 11:24 AM, Moshe Shemesh wrote:
> On 3/16/2021 12:31 AM, Don Bollinger wrote:
> > On Mon, 15 Mar 2021 10:12:39 +0700 Moshe Shemesh wrote:
> >> From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
> >>
> >> Define get_module_eeprom_data_by_page() ethtool callback and
> >> implement netlink infrastructure.
> >>

<snip>

> >> +     request->offset =
> >> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
> >> +     request->length =
> >> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
> >> +     if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
> >> +         dev->ethtool_ops->get_module_eeprom_data_by_page &&
> >> +         request->offset + request->length >
> >> ETH_MODULE_EEPROM_PAGE_LEN)
> >> +             return -EINVAL;
> > If a request exceeds the length of EEPROM (in legacy), we truncate it
> > to EEPROM length.  I suggest if a request exceeds the length of the
> > page (in the new KAPI), then we truncate at the end of the page.  Thus
> > rather than 'return -EINVAL;' I suggest
> >
> >            request->length = ETH_MODULE_EEPROM_PAGE_LEN -
> > request->offset;
> 
> I was not sure about that, isn't it better that once user specified page he has
> to be in the page boundary and let him know the command failed ?

On further review, I agree with your original code.  Reading across a page boundary will require explicit tracking of offset and page to read from.  If you get a short read, you *might* have to update the page on the next read.  If you have to keep track of where the page boundary is and adjust the offset and page together, then you should be explicitly managing not to try to read across a page boundary.  Put another way, if you try to read across a page boundary you probably have a bug in your code and will appreciate an immediate error.

I withdraw my previous suggestion, don't change it.

Don

Re: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Andrew Lunn]

On Mon, Mar 15, 2021 at 07:12:39PM +0200, Moshe Shemesh wrote:
> From: Vladyslav Tarasiuk <vladyslavt@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 <vladyslavt@nvidia.com>
> ---
>  Documentation/networking/ethtool-netlink.rst |  34 ++++-
>  include/linux/ethtool.h                      |   8 +-
>  include/uapi/linux/ethtool.h                 |  25 +++
>  include/uapi/linux/ethtool_netlink.h         |  19 +++
>  net/ethtool/Makefile                         |   2 +-
>  net/ethtool/eeprom.c                         | 153 +++++++++++++++++++
>  net/ethtool/netlink.c                        |  10 ++
>  net/ethtool/netlink.h                        |   2 +
>  8 files changed, 249 insertions(+), 4 deletions(-)
>  create mode 100644 net/ethtool/eeprom.c
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 05073482db05..25846b97632a 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -1280,6 +1280,36 @@ Kernel response contents:
>  For UDP tunnel table empty ``ETHTOOL_A_TUNNEL_UDP_TABLE_TYPES`` indicates that
>  the table contains static entries, hard-coded by the NIC.
>  
> +EEPROM_DATA
> +===========
> +
> +Fetch module EEPROM data dump.
> +
> +Request contents:
> +
> +  =====================================  ======  ==========================
> +  ``ETHTOOL_A_EEPROM_DATA_HEADER``       nested  request header
> +  ``ETHTOOL_A_EEPROM_DATA_OFFSET``       u32     offset within a page
> +  ``ETHTOOL_A_EEPROM_DATA_LENGTH``       u32     amount of bytes to read

I wonder if offset and length should be u8. At most, we should only be
returning a 1/2 page, so 128 bytes. We don't need a u32.

>  Request translation
>  ===================
>  
> @@ -1357,8 +1387,8 @@ are netlink only.
>    ``ETHTOOL_GET_DUMP_FLAG``           n/a
>    ``ETHTOOL_GET_DUMP_DATA``           n/a
>    ``ETHTOOL_GET_TS_INFO``             ``ETHTOOL_MSG_TSINFO_GET``
> -  ``ETHTOOL_GMODULEINFO``             n/a
> -  ``ETHTOOL_GMODULEEEPROM``           n/a
> +  ``ETHTOOL_GMODULEINFO``             ``ETHTOOL_MSG_MODULE_EEPROM_GET``
> +  ``ETHTOOL_GMODULEEEPROM``           ``ETHTOOL_MSG_MODULE_EEPROM_GET``
>    ``ETHTOOL_GEEE``                    ``ETHTOOL_MSG_EEE_GET``
>    ``ETHTOOL_SEEE``                    ``ETHTOOL_MSG_EEE_SET``
>    ``ETHTOOL_GRSSH``                   n/a

We should check with Michal about this. It is not a direct replacement
of the old IOCTL API, it is new API. He may want it documented
differently.

> +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);
> +	struct net_device *dev = req_info->dev;
> +
> +	if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
> +		return -EINVAL;
> +
> +	request->i2c_address = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
> +	if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
> +		return -EINVAL;

> +
> +	request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
> +	request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
> +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
> +	    dev->ethtool_ops->get_module_eeprom_data_by_page &&
> +	    request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
> +		return -EINVAL;

You need to watch out for overflows here. 0xfffffff0 + 0x20 is less
than ETH_MODULE_EEPROM_PAGE_LEN when it wraps around, but will cause
bad things to happen.

      Andrew

Re: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Moshe Shemesh]

On 3/18/2021 3:03 PM, Andrew Lunn wrote:
>
> On Mon, Mar 15, 2021 at 07:12:39PM +0200, Moshe Shemesh wrote:
>> From: Vladyslav Tarasiuk <vladyslavt@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 <vladyslavt@nvidia.com>
>> ---
>>   Documentation/networking/ethtool-netlink.rst |  34 ++++-
>>   include/linux/ethtool.h                      |   8 +-
>>   include/uapi/linux/ethtool.h                 |  25 +++
>>   include/uapi/linux/ethtool_netlink.h         |  19 +++
>>   net/ethtool/Makefile                         |   2 +-
>>   net/ethtool/eeprom.c                         | 153 +++++++++++++++++++
>>   net/ethtool/netlink.c                        |  10 ++
>>   net/ethtool/netlink.h                        |   2 +
>>   8 files changed, 249 insertions(+), 4 deletions(-)
>>   create mode 100644 net/ethtool/eeprom.c
>>
>> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
>> index 05073482db05..25846b97632a 100644
>> --- a/Documentation/networking/ethtool-netlink.rst
>> +++ b/Documentation/networking/ethtool-netlink.rst
>> @@ -1280,6 +1280,36 @@ Kernel response contents:
>>   For UDP tunnel table empty ``ETHTOOL_A_TUNNEL_UDP_TABLE_TYPES`` indicates that
>>   the table contains static entries, hard-coded by the NIC.
>>
>> +EEPROM_DATA
>> +===========
>> +
>> +Fetch module EEPROM data dump.
>> +
>> +Request contents:
>> +
>> +  =====================================  ======  ==========================
>> +  ``ETHTOOL_A_EEPROM_DATA_HEADER``       nested  request header
>> +  ``ETHTOOL_A_EEPROM_DATA_OFFSET``       u32     offset within a page
>> +  ``ETHTOOL_A_EEPROM_DATA_LENGTH``       u32     amount of bytes to read
> I wonder if offset and length should be u8. At most, we should only be
> returning a 1/2 page, so 128 bytes. We don't need a u32.


That's right when page is given, but user may have commands that used to 
work on the ioctl KAPI with offset higher than one page.

>>   Request translation
>>   ===================
>>
>> @@ -1357,8 +1387,8 @@ are netlink only.
>>     ``ETHTOOL_GET_DUMP_FLAG``           n/a
>>     ``ETHTOOL_GET_DUMP_DATA``           n/a
>>     ``ETHTOOL_GET_TS_INFO``             ``ETHTOOL_MSG_TSINFO_GET``
>> -  ``ETHTOOL_GMODULEINFO``             n/a
>> -  ``ETHTOOL_GMODULEEEPROM``           n/a
>> +  ``ETHTOOL_GMODULEINFO``             ``ETHTOOL_MSG_MODULE_EEPROM_GET``
>> +  ``ETHTOOL_GMODULEEEPROM``           ``ETHTOOL_MSG_MODULE_EEPROM_GET``
>>     ``ETHTOOL_GEEE``                    ``ETHTOOL_MSG_EEE_GET``
>>     ``ETHTOOL_SEEE``                    ``ETHTOOL_MSG_EEE_SET``
>>     ``ETHTOOL_GRSSH``                   n/a
> We should check with Michal about this. It is not a direct replacement
> of the old IOCTL API, it is new API. He may want it documented
> differently.
Michal, please comment on where it should be.
>> +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);
>> +     struct net_device *dev = req_info->dev;
>> +
>> +     if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
>> +         !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
>> +         !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
>> +             return -EINVAL;
>> +
>> +     request->i2c_address = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
>> +     if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
>> +             return -EINVAL;
>> +
>> +     request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
>> +     request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
>> +     if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
>> +         dev->ethtool_ops->get_module_eeprom_data_by_page &&
>> +         request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
>> +             return -EINVAL;
> You need to watch out for overflows here. 0xfffffff0 + 0x20 is less
> than ETH_MODULE_EEPROM_PAGE_LEN when it wraps around, but will cause
> bad things to happen.
Ack.
>        Andrew

Re: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Michal Kubecek]

On Thu, Mar 18, 2021 at 02:03:02PM +0100, Andrew Lunn wrote:
> On Mon, Mar 15, 2021 at 07:12:39PM +0200, Moshe Shemesh wrote:
> >  
> > +EEPROM_DATA
> > +===========
> > +
> > +Fetch module EEPROM data dump.
> > +
> > +Request contents:
> > +
> > +  =====================================  ======  ==========================
> > +  ``ETHTOOL_A_EEPROM_DATA_HEADER``       nested  request header
> > +  ``ETHTOOL_A_EEPROM_DATA_OFFSET``       u32     offset within a page
> > +  ``ETHTOOL_A_EEPROM_DATA_LENGTH``       u32     amount of bytes to read
> 
> I wonder if offset and length should be u8. At most, we should only be
> returning a 1/2 page, so 128 bytes. We don't need a u32.

There is no actual gain using NLA_U8 due to padding. Out of the
interfaces used here, kernel-userspace API is the least flexible so
I would generally prefer NLA_U32, except for bools or enumerated values
where it's absolutely obvious the number of possible values cannot grow
too much. In this case, I can't really say it's impossible we could have
devices with bigger pages in something like 20 years.

> >  Request translation
> >  ===================
> >  
> > @@ -1357,8 +1387,8 @@ are netlink only.
> >    ``ETHTOOL_GET_DUMP_FLAG``           n/a
> >    ``ETHTOOL_GET_DUMP_DATA``           n/a
> >    ``ETHTOOL_GET_TS_INFO``             ``ETHTOOL_MSG_TSINFO_GET``
> > -  ``ETHTOOL_GMODULEINFO``             n/a
> > -  ``ETHTOOL_GMODULEEEPROM``           n/a
> > +  ``ETHTOOL_GMODULEINFO``             ``ETHTOOL_MSG_MODULE_EEPROM_GET``
> > +  ``ETHTOOL_GMODULEEEPROM``           ``ETHTOOL_MSG_MODULE_EEPROM_GET``
> >    ``ETHTOOL_GEEE``                    ``ETHTOOL_MSG_EEE_GET``
> >    ``ETHTOOL_SEEE``                    ``ETHTOOL_MSG_EEE_SET``
> >    ``ETHTOOL_GRSSH``                   n/a
> 
> We should check with Michal about this. It is not a direct replacement
> of the old IOCTL API, it is new API. He may want it documented
> differently.

This table is meant to give a hint in the sense "for what you used
ioctl command in left column, use now netlink request in the right".
So IMHO it's appropriate. Perhaps it would deserve a comment explaining
this.

> > +	request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
> > +	request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
> > +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
> > +	    dev->ethtool_ops->get_module_eeprom_data_by_page &&
> > +	    request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
> > +		return -EINVAL;
> 
> You need to watch out for overflows here. 0xfffffff0 + 0x20 is less
> than ETH_MODULE_EEPROM_PAGE_LEN when it wraps around, but will cause
> bad things to happen.

BtW, the ioctl code also suffers from this problem and we recently had
a report from customer (IIRC the effect was ethtool trying to allocate
~4GB of memory), upstream fix should follow soon.

Michal

Re: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Andrew Lunn]

> > > +Request contents:
> > > +
> > > +  =====================================  ======  ==========================
> > > +  ``ETHTOOL_A_EEPROM_DATA_HEADER``       nested  request header
> > > +  ``ETHTOOL_A_EEPROM_DATA_OFFSET``       u32     offset within a page
> > > +  ``ETHTOOL_A_EEPROM_DATA_LENGTH``       u32     amount of bytes to read
> > I wonder if offset and length should be u8. At most, we should only be
> > returning a 1/2 page, so 128 bytes. We don't need a u32.
> 
> 
> That's right when page is given, but user may have commands that
> used to work on the ioctl KAPI with offset higher than one page.

CMIS section 5.4.1 says:

  The slave maintains an internal current byte address counter
  containing the byte address accessed during the latest read or write
  operation incremented by one with roll-over as follows: The current
  byte address counter rolls-over after a read or write operation at
  the last byte address of the current 128-byte memory page (127 or
  255) to the first byte address (0 or 128) of the same 128-byte
  memory page.

This wrapping is somewhat unexpected. If the user access is for a read
starting at 120 and a length of 20, they get bytes 120-127 followed by
0-11. The user is more likely to be expecting 120-139.

We have two ways to address this:

1) We limit reads to a maximum of a 1/2 page, and the start and end
point needs to be within that 1/2 page.

2) We detect that the read is going to go across a 1/2 page boarder,
and perform two reads to the MAC driver, and glue the data back
together again in the ethtool core.

What i don't want is to leave the individual drivers to solve this,
because i expect some of them will get it wrong.

	Andrew

Re: [RFC PATCH V3 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Andrew Lunn]

> > > +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);
> > > +     struct net_device *dev = req_info->dev;
> > > +
> > > +     if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
> > > +         !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
> > > +         !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
> > > +             return -EINVAL;
> > Suggestion:  Consider using i2c address 0x50 as a default if none is given.
> > 0x50 is the first 256 bytes of SFP, and all of QSFP and CMIS EEPROM.  If
> > there is a page given on an SFP device, then you know i2c address is 0x51.
> > The only thing that REQUIRES 0x51 is legacy offset 256-511 on SFP.  Keep the
> > i2c address, but make it optional for the non-standard devices that Andrew
> > has mentioned, and for that one section of SFP data that requires it.  And
> > document it for the user.
> Agree, but thought to have that i2c address default set on userspace, so
> here we expect it.

I would make it mandatory. If you default it to 0x50, the current
message structure has no way of including this information in the
reply. So user space gets some data, but it has no idea from where?

       Andrew