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

[RFC PATCH V2 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:
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

 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  42 +++
 .../net/ethernet/mellanox/mlx5/core/port.c    | 101 ++++++--
 include/linux/ethtool.h                       |   7 +-
 include/linux/mlx5/port.h                     |  12 +
 include/uapi/linux/ethtool.h                  |  26 ++
 include/uapi/linux/ethtool_netlink.h          |  19 ++
 net/ethtool/Makefile                          |   2 +-
 net/ethtool/eeprom.c                          | 239 ++++++++++++++++++
 net/ethtool/netlink.c                         |  10 +
 net/ethtool/netlink.h                         |   2 +
 10 files changed, 430 insertions(+), 30 deletions(-)
 create mode 100644 net/ethtool/eeprom.c

-- 
2.18.2

[RFC PATCH V2 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>
---
 include/linux/ethtool.h              |   7 +-
 include/uapi/linux/ethtool.h         |  26 +++++
 include/uapi/linux/ethtool_netlink.h |  19 ++++
 net/ethtool/Makefile                 |   2 +-
 net/ethtool/eeprom.c                 | 157 +++++++++++++++++++++++++++
 net/ethtool/netlink.c                |  10 ++
 net/ethtool/netlink.h                |   2 +
 7 files changed, 221 insertions(+), 2 deletions(-)
 create mode 100644 net/ethtool/eeprom.c

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index ec4cd3921c67..2f65aae5f492 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,8 @@ 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 zero.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -515,6 +518,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 +544,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..2459571fc1d1 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;
+	__u32	page;
+	__u32	bank;
+	__u32	i2c_address;
+	__u8	*data;
+};
+
 /**
  * struct ethtool_eee - Energy Efficient Ethernet information
  * @cmd: ETHTOOL_{G,S}EEE
@@ -1865,6 +1887,10 @@ static inline int ethtool_validate_duplex(__u8 duplex)
 #define ETH_MODULE_SFF_8636_MAX_LEN     640
 #define ETH_MODULE_SFF_8436_MAX_LEN     640
 
+#define ETH_MODULE_EEPROM_MAX_LEN	640
+#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..60dd848d0b54 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -42,6 +42,7 @@ enum {
 	ETHTOOL_MSG_CABLE_TEST_ACT,
 	ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
 	ETHTOOL_MSG_TUNNEL_INFO_GET,
+	ETHTOOL_MSG_EEPROM_DATA_GET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -80,6 +81,7 @@ enum {
 	ETHTOOL_MSG_CABLE_TEST_NTF,
 	ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
 	ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
+	ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -629,6 +631,23 @@ enum {
 	ETHTOOL_A_TUNNEL_INFO_MAX = (__ETHTOOL_A_TUNNEL_INFO_CNT - 1)
 };
 
+/* MODULE EEPROM DATA */
+
+enum {
+	ETHTOOL_A_EEPROM_DATA_UNSPEC,
+	ETHTOOL_A_EEPROM_DATA_HEADER,
+
+	ETHTOOL_A_EEPROM_DATA_OFFSET,
+	ETHTOOL_A_EEPROM_DATA_LENGTH,
+	ETHTOOL_A_EEPROM_DATA_PAGE,
+	ETHTOOL_A_EEPROM_DATA_BANK,
+	ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
+	ETHTOOL_A_EEPROM_DATA,
+
+	__ETHTOOL_A_EEPROM_DATA_CNT,
+	ETHTOOL_A_EEPROM_DATA_MAX = (__ETHTOOL_A_EEPROM_DATA_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 7a849ff22dad..d604346bc074 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
 ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
-		   tunnels.o
+		   tunnels.o eeprom.o
diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
new file mode 100644
index 000000000000..2618a55b9a40
--- /dev/null
+++ b/net/ethtool/eeprom.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/ethtool.h>
+#include "netlink.h"
+#include "common.h"
+
+struct eeprom_data_req_info {
+	struct ethnl_req_info	base;
+	u32			offset;
+	u32			length;
+	u32			page;
+	u32			bank;
+	u32			i2c_address;
+};
+
+struct eeprom_data_reply_data {
+	struct ethnl_reply_data base;
+	u32			length;
+	u32			i2c_address;
+	u8			*data;
+};
+
+#define EEPROM_DATA_REQINFO(__req_base) \
+	container_of(__req_base, struct eeprom_data_req_info, base)
+
+#define EEPROM_DATA_REPDATA(__reply_base) \
+	container_of(__reply_base, struct eeprom_data_reply_data, base)
+
+static int eeprom_data_prepare_data(const struct ethnl_req_info *req_base,
+				    struct ethnl_reply_data *reply_base,
+				    struct genl_info *info)
+{
+	struct eeprom_data_reply_data *reply = EEPROM_DATA_REPDATA(reply_base);
+	struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_base);
+	struct ethtool_eeprom_data page_data = {0};
+	struct net_device *dev = reply_base->dev;
+	int err;
+
+	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
+		return -EOPNOTSUPP;
+
+	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;
+	err = ethnl_ops_begin(dev);
+	if (err)
+		goto err_free;
+
+	err = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, &page_data,
+							       info->extack);
+	if (err)
+		goto err_ops;
+
+	reply->length = page_data.length;
+	reply->i2c_address = page_data.i2c_address;
+	reply->data = page_data.data;
+
+	ethnl_ops_complete(dev);
+	return 0;
+
+err_ops:
+	ethnl_ops_complete(dev);
+err_free:
+	kfree(page_data.data);
+	return err;
+}
+
+static int eeprom_data_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
+				     struct netlink_ext_ack *extack)
+{
+	struct eeprom_data_req_info *request = EEPROM_DATA_REQINFO(req_info);
+
+	if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
+	    !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
+	    !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
+		return -EINVAL;
+
+	request->i2c_address = nla_get_u32(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 (request->length > ETH_MODULE_EEPROM_MAX_LEN)
+		return -EINVAL;
+	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
+	    request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
+		return -EINVAL;
+
+	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
+		request->page = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
+	if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
+		request->bank = nla_get_u32(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(u32)) + /* _EEPROM_DATA_I2C_ADDRESS */
+	       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_u32(skb, ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS, reply->i2c_address) ||
+	    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_U32 },
+	[ETHTOOL_A_EEPROM_DATA_BANK]		= { .type = NLA_U32 },
+	[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]	= { .type = NLA_U32 },
+	[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.18.2

[RFC PATCH V2 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.18.2

[RFC PATCH V2 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  | 42 +++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/port.c    | 33 +++++++++++++++
 include/linux/mlx5/port.h                     |  2 +
 3 files changed, 77 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..6766ffb07c81 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1769,6 +1769,47 @@ 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 */
+		if (!size_read)
+			return 0;
+
+		if (size_read < 0) {
+			netdev_err(priv->netdev, "%s: mlx5_query_module_eeprom_data failed:0x%x\n",
+				   __func__, size_read);
+			return 0;
+		}
+
+		i += size_read;
+		query.offset += size_read;
+	}
+
+	return 0;
+}
+
 int mlx5e_ethtool_flash_device(struct mlx5e_priv *priv,
 			       struct ethtool_flash *flash)
 {
@@ -2148,6 +2189,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.18.2

[RFC PATCH V2 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.18.2

[RFC PATCH V2 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 | 84 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
index 2618a55b9a40..72c7714a9d37 100644
--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c
@@ -26,6 +26,88 @@ 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)
+		offset = 128 + request->page * 128 + offset;
+
+	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;
+
+	switch (modinfo->type) {
+	case ETH_MODULE_SFF_8079:
+		if (request->page > 1)
+			return -EINVAL;
+		break;
+	case ETH_MODULE_SFF_8472:
+		if (request->page > 3)
+			return -EINVAL;
+		break;
+	case ETH_MODULE_SFF_8436:
+	case ETH_MODULE_SFF_8636:
+		if (request->page > 3)
+			return -EINVAL;
+		break;
+	}
+	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 || request->i2c_address) {
+		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)
@@ -37,7 +119,7 @@ static int eeprom_data_prepare_data(const struct ethnl_req_info *req_base,
 	int err;
 
 	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.18.2

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

[Don Bollinger]

On Thu, Mar 04, 2021 at 10:57AM-0800, Moshe Shemesh wrote:
> 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.

This is nice work, addressing the banks problem (though there are no devices
with bank switching yet?)

I suggest this change increase the maximum size of EEPROM to the maximum
the architecture allows.  That's 256 pages (128 bytes) plus the lower page
for
a total of 257*256 bytes.  SFP devices can access another 256 bytes since
they
use two i2c addresses but only one of them is paged.  The size increase is
necessary for bank support since banked pages are all above the current
640 byte limit.  Note that the SFF-* specs do not specify what is in pages
above page 3 (except CMIS), but they DO specify that those pages are
available for proprietary uses by module vendors.  I will call out these
changes
in the following patches.

Ethtool also supports module 'change-eeprom', a write function mirroring the
dump function.  That path needs to be implemented too.  There are some 
very interesting proprietary tricks that some modules can do by writing
the right magic to the right registers, some of which are on pages in the
0xF0 range.

> 
> 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.

Just to be clear, if you define a page to be 256 bytes, and only specify
offset
within a page, then offset 0-127 is the same for every page on the device,
and useful offsets for each page start at 128.  This can be confusing, but I
think it is the right approach.

> 
> 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:
> 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
> 
>  .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  42 +++
>  .../net/ethernet/mellanox/mlx5/core/port.c    | 101 ++++++--
>  include/linux/ethtool.h                       |   7 +-
>  include/linux/mlx5/port.h                     |  12 +
>  include/uapi/linux/ethtool.h                  |  26 ++
>  include/uapi/linux/ethtool_netlink.h          |  19 ++
>  net/ethtool/Makefile                          |   2 +-
>  net/ethtool/eeprom.c                          | 239 ++++++++++++++++++
>  net/ethtool/netlink.c                         |  10 +
>  net/ethtool/netlink.h                         |   2 +
>  10 files changed, 430 insertions(+), 30 deletions(-)  create mode 100644
> net/ethtool/eeprom.c
> 
> --
> 2.18.2

Don Bollinger

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

[Don Bollinger]

On Thu, Mar 04, 2021 at 10:57AM-0800, 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>
> ---
>  include/linux/ethtool.h              |   7 +-
>  include/uapi/linux/ethtool.h         |  26 +++++
>  include/uapi/linux/ethtool_netlink.h |  19 ++++
>  net/ethtool/Makefile                 |   2 +-
>  net/ethtool/eeprom.c                 | 157 +++++++++++++++++++++++++++
>  net/ethtool/netlink.c                |  10 ++
>  net/ethtool/netlink.h                |   2 +
>  7 files changed, 221 insertions(+), 2 deletions(-)  create mode 100644
> net/ethtool/eeprom.c
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index
> ec4cd3921c67..2f65aae5f492 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,8 @@
> 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 zero.
>   *
>   * All operations are optional (i.e. the function pointer may be set
>   * to %NULL) and callers must take this into account.  Callers must @@
-515,6
> +518,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 +544,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..2459571fc1d1 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.

Note here that bytes at offset 0-127 are the same for every page of the
module, only bytes at offset 128 and higher are actually paged.

> + * @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.

The standards are all very clear, the only legal values are 0x50 and 0x51.
It isn't 'expected', it is required.  I suggest that 0xA0 and 0xA2 also be
silently accepted, and translated to 0x50 and 0x51 respectively.  Some
of the specs use A0/A2 instead of 0x50/0x51.  They actually mean the
same thing.

> + * @data: Pointer to buffer with EEPROM data of @length size.
> + *
> + * This can be used to manage pages during EEPROM dump in ethtool and
> +pass
> + * required information to the driver.
> + */
> +struct ethtool_eeprom_data {
> +	__u32	offset;
> +	__u32	length;
> +	__u32	page;
> +	__u32	bank;
> +	__u32	i2c_address;
> +	__u8	*data;
> +};
> +
>  /**
>   * struct ethtool_eee - Energy Efficient Ethernet information
>   * @cmd: ETHTOOL_{G,S}EEE
> @@ -1865,6 +1887,10 @@ static inline int ethtool_validate_duplex(__u8
> duplex)
>  #define ETH_MODULE_SFF_8636_MAX_LEN     640
>  #define ETH_MODULE_SFF_8436_MAX_LEN     640
> 
> +#define ETH_MODULE_EEPROM_MAX_LEN	640

Please don't add this MAX_LEN constant.  Even better, remove
the two above it as well.

The proper value for all 3 of these MAX_LEN items is the
architectural limit imposed by the 8 bit page register plus the constant
lower page (hence 257*128 bytes).  The 8436 and 8636
specs do not actually limit these devices to 640 bytes (3 pages). 

There is no MAX_LEN listed for SFF_8472.  If there is one, it should
actually be 259 * 128 bytes (to account for 256 more bytes on the
unpaged 0x50 i2c address). 

 Nor is there one for CMIS.  The maximum
architected length for CMIS is (257*128) + (127 * 16 * 128).  That's
the QSFP max length plus 127 more banks of 16 pages.

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

Actually there are only two legal values for the i2c address (0x50, 0x51).
Rather than defining a MAX address, consider defining the legal values,
or...  is it used at all?  Leave it out?

> +
>  /* Reset flags */
>  /* The reset() operation must clear the flags for the components which
>   * were actually reset.  On successful return, the flags indicate the
diff --git
> a/include/uapi/linux/ethtool_netlink.h
> b/include/uapi/linux/ethtool_netlink.h
> index a286635ac9b8..60dd848d0b54 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -42,6 +42,7 @@ enum {
>  	ETHTOOL_MSG_CABLE_TEST_ACT,
>  	ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
>  	ETHTOOL_MSG_TUNNEL_INFO_GET,
> +	ETHTOOL_MSG_EEPROM_DATA_GET,
> 
>  	/* add new constants above here */
>  	__ETHTOOL_MSG_USER_CNT,
> @@ -80,6 +81,7 @@ enum {
>  	ETHTOOL_MSG_CABLE_TEST_NTF,
>  	ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
>  	ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
> +	ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
> 
>  	/* add new constants above here */
>  	__ETHTOOL_MSG_KERNEL_CNT,
> @@ -629,6 +631,23 @@ enum {
>  	ETHTOOL_A_TUNNEL_INFO_MAX =
> (__ETHTOOL_A_TUNNEL_INFO_CNT - 1)  };
> 
> +/* MODULE EEPROM DATA */
> +
> +enum {
> +	ETHTOOL_A_EEPROM_DATA_UNSPEC,
> +	ETHTOOL_A_EEPROM_DATA_HEADER,
> +
> +	ETHTOOL_A_EEPROM_DATA_OFFSET,
> +	ETHTOOL_A_EEPROM_DATA_LENGTH,
> +	ETHTOOL_A_EEPROM_DATA_PAGE,
> +	ETHTOOL_A_EEPROM_DATA_BANK,
> +	ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
> +	ETHTOOL_A_EEPROM_DATA,
> +
> +	__ETHTOOL_A_EEPROM_DATA_CNT,
> +	ETHTOOL_A_EEPROM_DATA_MAX =
> (__ETHTOOL_A_EEPROM_DATA_CNT - 1) };
> +
>  /* generic netlink info */
>  #define ETHTOOL_GENL_NAME "ethtool"
>  #define ETHTOOL_GENL_VERSION 1
> diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile index
> 7a849ff22dad..d604346bc074 100644
> --- a/net/ethtool/Makefile
> +++ b/net/ethtool/Makefile
> @@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
>  ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
>  		   linkstate.o debug.o wol.o features.o privflags.o rings.o
\
>  		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o
\
> -		   tunnels.o
> +		   tunnels.o eeprom.o
> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c new file mode
> 100644 index 000000000000..2618a55b9a40
> --- /dev/null
> +++ b/net/ethtool/eeprom.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/ethtool.h>
> +#include "netlink.h"
> +#include "common.h"
> +
> +struct eeprom_data_req_info {
> +	struct ethnl_req_info	base;
> +	u32			offset;
> +	u32			length;
> +	u32			page;
> +	u32			bank;
> +	u32			i2c_address;
> +};
> +
> +struct eeprom_data_reply_data {
> +	struct ethnl_reply_data base;
> +	u32			length;
> +	u32			i2c_address;
> +	u8			*data;
> +};
> +
> +#define EEPROM_DATA_REQINFO(__req_base) \
> +	container_of(__req_base, struct eeprom_data_req_info, base)
> +
> +#define EEPROM_DATA_REPDATA(__reply_base) \
> +	container_of(__reply_base, struct eeprom_data_reply_data, base)
> +
> +static int eeprom_data_prepare_data(const struct ethnl_req_info
> *req_base,
> +				    struct ethnl_reply_data *reply_base,
> +				    struct genl_info *info)
> +{
> +	struct eeprom_data_reply_data *reply =
> EEPROM_DATA_REPDATA(reply_base);
> +	struct eeprom_data_req_info *request =
> EEPROM_DATA_REQINFO(req_base);
> +	struct ethtool_eeprom_data page_data = {0};
> +	struct net_device *dev = reply_base->dev;
> +	int err;
> +
> +	if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
> +		return -EOPNOTSUPP;
> +
> +	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;
> +	err = ethnl_ops_begin(dev);
> +	if (err)
> +		goto err_free;
> +
> +	err = dev->ethtool_ops->get_module_eeprom_data_by_page(dev,
> &page_data,
> +
info->extack);
> +	if (err)
> +		goto err_ops;
> +
> +	reply->length = page_data.length;
> +	reply->i2c_address = page_data.i2c_address;
> +	reply->data = page_data.data;
> +
> +	ethnl_ops_complete(dev);

The two error paths below kfree(page_data.data).  Does someone else
free this memory when there is no error?

> +	return 0;
> +
> +err_ops:
> +	ethnl_ops_complete(dev);
> +err_free:
> +	kfree(page_data.data);
> +	return err;
> +}
> +
> +static int eeprom_data_parse_request(struct ethnl_req_info *req_info,
> struct nlattr **tb,
> +				     struct netlink_ext_ack *extack) {
> +	struct eeprom_data_req_info *request =
> EEPROM_DATA_REQINFO(req_info);
> +
> +	if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
> +	    !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
> +		return -EINVAL;
> +
> +	request->i2c_address =
> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
> +	if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
> +		return -EINVAL;

I would be much more restrictive, with one flexibility...

        if (request->i2c_address == 0xA0) request->i2c_address = 0x50;
        if (request->i2c_address == 0xA2) request->i2c_address = 0x51;
        if (request->i2c_address < 0x50) || (request->i2c_address > 0x51)
                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 (request->length > ETH_MODULE_EEPROM_MAX_LEN)
> +		return -EINVAL;	

This is really problematic as there are MANY different max values, within
the specs, for the various EEPROMs being generically supported here.
I would leave it to the drivers to handle out-of-range requests.  If you
really want to check, you need to know which spec the module supports,
whether it supports pages, and whether it supports banks.  I have not
found a register that actually reports the number of supported pages
that an eeprom supports.  The specs should have included that :-(.

> +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
> +	    request->offset + request->length >
> ETH_MODULE_EEPROM_PAGE_LEN)
> +		return -EINVAL;

Why does this stanza depend on DATA_PAGE?  In this new data 
structure, no requests can cross the 256 byte page boundary.

I suggest, rather then -EINVAL, you should reduce the length to reach
the end of the page:

        if (request->offset + request->length) > ETH_MODULE_EEPROM_PAGE_LEN)
                request->length = ETH_MODULE_EEPROM_PAGE_LEN - 
                                                      request->offset.

Note that this matches the choice you made to truncate rather than
error out in fallback_set_parms().

> +
> +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
> +		request->page =
> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
> +	if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
> +		request->bank =
> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_BANK]);

Other checks:

Page and bank have to be between 0 and 255 (inclusive), they
go into an 8 bit register in the eeprom.

Offset and length can't be negative.

> +
> +	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(u32)) + /* _EEPROM_DATA_I2C_ADDRESS
> */
> +	       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_u32(skb, ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
> reply->i2c_address) ||
> +	    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_U32 },
> +	[ETHTOOL_A_EEPROM_DATA_BANK]		= { .type = NLA_U32 },
> +	[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]	= { .type = NLA_U32 },
> +	[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.18.2

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

[Don Bollinger]

On Thu, Mar 04, 2021 at 10:57AM-0800, 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 | 84
> +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c index
> 2618a55b9a40..72c7714a9d37 100644
> --- a/net/ethtool/eeprom.c
> +++ b/net/ethtool/eeprom.c
> @@ -26,6 +26,88 @@ 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) {

This is translating the new data structure into the old.  Hence, I assume we
have i2c_addr, page, bank, offset, len to work with, and we should use
all of them.  We shouldn't be applying the legacy data structure's rules
to how we interpret the *request data.  Therefore...

> +	u32 offset = request->offset;
> +	u32 length = request->length;
> +
> +	if (request->page)
> +		offset = 128 + request->page * 128 + offset;

This is tricky to map to old behavior.  The new data structure should give
lower 
memory for offsets less than 128, and paged upper memory for offsets of 128
and higher.  There is no way to describe that request as {offset, length} in
the
old ethtool format with a fake linear memory.

        if (request->page) {
                if (offset < 128) && (offset + length > 128)
                       return -EINVAL;
                if (offset > 127) offset = request->page * 128 + offset;

> +
> +	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;
> +
> +	switch (modinfo->type) {
> +	case ETH_MODULE_SFF_8079:
> +		if (request->page > 1)
> +			return -EINVAL;
> +		break;
> +	case ETH_MODULE_SFF_8472:
> +		if (request->page > 3)

Not sure this is needed, there can be pages higher than 3.

> +			return -EINVAL;

I *think* the linear memory on SFP puts 0x50 in the first
256 bytes, 0x51 after that, including pages after that.  So,
the old fashioned linear memory offset needs to be adjusted
for accesses to 0x51.  Thus add:

        if (request->i2c_address == 0x51)
                offset += 256;

> +		break;
> +	case ETH_MODULE_SFF_8436:
> +	case ETH_MODULE_SFF_8636:

Not sure this is needed, there can be pages higher than 3.

> +		if (request->page > 3)
> +			return -EINVAL;
> +		break;
> +	}
> +	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 || request->i2c_address) {

We don't need to reject if there is an i2c_address.  Indeed, we need that
to determine the correct offset for the legacy linear memory offset.

Note my comment on an earlier patch in this series, I would have rejected
any request that didn't have either 0x50 or 0x51 here.

> +		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)
> @@ -37,7 +119,7 @@ static int eeprom_data_prepare_data(const struct
> ethnl_req_info *req_base,
>  	int err;
> 
>  	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.18.2

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

[Andrew Lunn]

> > + * @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.
> 
> The standards are all very clear

Our experience so far is that manufactures of SFP modules like to
ignore the standard. And none of the standards seem to cover copper
modules, which have additional registers at some other page.
Admittedly, they cannot be mapped as pages, you need some proprietary
protocol to map MDIO onto I2C. But i would not be surprised to find
some SFP that maps the FLASH of the microcontroller onto an address,
which we might be able to read out using this API.

So i suggested we keep it generic, allowing access to these
proprietary registers at other addresses. And if there is nothing
there, you probably get a 1/2 page of 0xff.

> I suggest that 0xA0 and 0xA2 also be silently accepted, and
> translated to 0x50 and 0x51 respectively.

No, i don't like having two different values mean the same thing.  It
just leads to confusion. And userspace is going to be confused when it
asks for 0xA0 but the reply says it is for 0x50.

The Linux I2C subsystem does not magically map 8bit addresses in 7bit
addresses. We should follow what the Linux I2C subsystem does.

> > +
> > +	request->offset =
> > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
> > +	request->length =
> > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
> > +	if (request->length > ETH_MODULE_EEPROM_MAX_LEN)
> > +		return -EINVAL;	
> 
> This is really problematic as there are MANY different max values, within
> the specs

I agree. We should only be returning one 1/2 page as a maximum. So it
should be limited to 128 bytes. And offset+length should not go beyond
the end of a 1/2 page.

> > +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
> > +		request->page =
> > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
> > +	if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
> > +		request->bank =
> > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
> 
> Other checks:
> 
> Page and bank have to be between 0 and 255 (inclusive), they
> go into an 8 bit register in the eeprom.

Yes, a u8 would be a better type here.

     Andrew

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

[Andrew Lunn]

> > +static int fallback_set_params(struct eeprom_data_req_info *request,
> > +			       struct ethtool_modinfo *modinfo,
> > +			       struct ethtool_eeprom *eeprom) {
> 
> This is translating the new data structure into the old.  Hence, I assume we
> have i2c_addr, page, bank, offset, len to work with, and we should use
> all of them.

Nope. We actually have none of them. The old API just asked the driver
to give me the data in the SFP. And the driver gets to decide what it
returns, following a well known layout. The driver can decide to give
just the first 1/2 page, or any number of multiple 1/2 pages in a well
known linear way, which ethtool knows how to decode.

So when mapping the new KAPI onto the old driver API, you need to call
the old API, and see if what is returned can be used to fulfil the
KAPI request. If the bytes are there, great, return them, otherwise
EOPNOTSUPP.

And we also need to consider the other way around. The old KAPI is
used, and the MAC driver only supports the new driver API. Since the
linear layout is well know, you need to make a number of calls into
the driver to read the 1/2 pages, and them glue them together and
return them.

I've not reviewed this code in detail yet, so i've no idea how it
actually works. But i would like to see as much compatibility as
possible. That has been the approach with moving from IOCTL to netlink
with ethool. Everything the old KAPI can do, netlink should also be
able to, plus there can be additional features.

> > +	switch (modinfo->type) {
> > +	case ETH_MODULE_SFF_8079:
> > +		if (request->page > 1)
> > +			return -EINVAL;
> > +		break;
> > +	case ETH_MODULE_SFF_8472:
> > +		if (request->page > 3)
> 
> Not sure this is needed, there can be pages higher than 3.

Not with the old KAPI call. As far as i remember, it stops at three
pages. But i need to check the ethtool(1) sources to be sure.

       Andrew

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

[Andrew Lunn]

> +/* MODULE EEPROM DATA */
> +
> +enum {
> +	ETHTOOL_A_EEPROM_DATA_UNSPEC,
> +	ETHTOOL_A_EEPROM_DATA_HEADER,
> +
> +	ETHTOOL_A_EEPROM_DATA_OFFSET,
> +	ETHTOOL_A_EEPROM_DATA_LENGTH,
> +	ETHTOOL_A_EEPROM_DATA_PAGE,
> +	ETHTOOL_A_EEPROM_DATA_BANK,
> +	ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
> +	ETHTOOL_A_EEPROM_DATA,

If you look at all the other such enums in ethtool_netlink, you will
see a comment indicating the type. Please add them here as well.

Please also update Documentation/networking/ethtool-netlink.rst.

       Andrew

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

[Don Bollinger]

> > > +static int fallback_set_params(struct eeprom_data_req_info *request,
> > > +			       struct ethtool_modinfo *modinfo,
> > > +			       struct ethtool_eeprom *eeprom) {
> >
> > This is translating the new data structure into the old.  Hence, I
> > assume we have i2c_addr, page, bank, offset, len to work with, and we
> > should use all of them.
> 
> Nope. We actually have none of them. The old API just asked the driver to
> give me the data in the SFP. And the driver gets to decide what it
returns,
> following a well known layout. The driver can decide to give just the
first 1/2
> page, or any number of multiple 1/2 pages in a well known linear way,
which
> ethtool knows how to decode.

This code is to take a new KAPI request (a struct eeprom_data_req_info), and
create an old driver API request (a struct ethtool_eeprom) that will get the

same data.  It isn't actually fetching the data, it is just forming the data
structure
to create the request.  So, we do indeed have all of the new KAPI
parameters, 
and need to use all of them to precisely create the matching old KAPI
request.

> So when mapping the new KAPI onto the old driver API, you need to call the
> old API, and see if what is returned can be used to fulfil the KAPI
request. If
> the bytes are there, great, return them, otherwise EOPNOTSUPP.

Actually, this code has to figure out in advance whether the old API can
return
the data to fulfill the request, then form a request to accomplish that.
 
> And we also need to consider the other way around. The old KAPI is used,
> and the MAC driver only supports the new driver API. Since the linear
layout
> is well know, you need to make a number of calls into the driver to read
the
> 1/2 pages, and them glue them together and return them.

That is a great idea, probably not difficult.  It is not in this patch set.
 
> I've not reviewed this code in detail yet, so i've no idea how it actually
works.
> But i would like to see as much compatibility as possible. That has been
the
> approach with moving from IOCTL to netlink with ethool. Everything the old
> KAPI can do, netlink should also be able to, plus there can be additional
> features.
> 
> > > +	switch (modinfo->type) {
> > > +	case ETH_MODULE_SFF_8079:
> > > +		if (request->page > 1)
> > > +			return -EINVAL;
> > > +		break;
> > > +	case ETH_MODULE_SFF_8472:
> > > +		if (request->page > 3)
> >
> > Not sure this is needed, there can be pages higher than 3.
> 
> Not with the old KAPI call. As far as i remember, it stops at three pages.
But i
> need to check the ethtool(1) sources to be sure.
> 
>        Andrew

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

[Don Bollinger]

> > --- a/net/ethtool/eeprom.c
> > +++ b/net/ethtool/eeprom.c
> > @@ -26,6 +26,88 @@ 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) {
> 
> This is translating the new data structure into the old.  Hence, I assume
we
> have i2c_addr, page, bank, offset, len to work with, and we should use all
of
> them.  We shouldn't be applying the legacy data structure's rules to how
we
> interpret the *request data.  Therefore...
> 
> > +	u32 offset = request->offset;
> > +	u32 length = request->length;
> > +
> > +	if (request->page)
> > +		offset = 128 + request->page * 128 + offset;
> 
> This is tricky to map to old behavior.  The new data structure should give
> lower memory for offsets less than 128, and paged upper memory for
> offsets of 128 and higher.  There is no way to describe that request as
> {offset, length} in the old ethtool format with a fake linear memory.
> 
>         if (request->page) {
>                 if (offset < 128) && (offset + length > 128)
>                        return -EINVAL;

Actually, reflecting on Andrew's response, it occurs to me this does not
have to be an error.  The routine eeprom_data_fallback() (below) could
detect this case (a request crossing the 128 byte offset boundary) and
create two requests, one for lower memory and one for the paged 
upper memory.  That can't be done as a single request with the linear
memory model, but the two pieces can be read separately and glued
together.

Don

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

[Moshe Shemesh]

On 3/5/2021 2:50 AM, Don Bollinger wrote:
>
> On Thu, Mar 04, 2021 at 10:57AM-0800, 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>
>> ---
>>   include/linux/ethtool.h              |   7 +-
>>   include/uapi/linux/ethtool.h         |  26 +++++
>>   include/uapi/linux/ethtool_netlink.h |  19 ++++
>>   net/ethtool/Makefile                 |   2 +-
>>   net/ethtool/eeprom.c                 | 157 +++++++++++++++++++++++++++
>>   net/ethtool/netlink.c                |  10 ++
>>   net/ethtool/netlink.h                |   2 +
>>   7 files changed, 221 insertions(+), 2 deletions(-)  create mode 100644
>> net/ethtool/eeprom.c
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index
>> ec4cd3921c67..2f65aae5f492 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,8 @@
>> 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 zero.
>>    *
>>    * All operations are optional (i.e. the function pointer may be set
>>    * to %NULL) and callers must take this into account.  Callers must @@
> -515,6
>> +518,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 +544,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..2459571fc1d1 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.
> Note here that bytes at offset 0-127 are the same for every page of the
> module, only bytes at offset 128 and higher are actually paged.
Right, but still having the offset relative to 128 will be confusing.
>> + * @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.
> The standards are all very clear, the only legal values are 0x50 and 0x51.
> It isn't 'expected', it is required.  I suggest that 0xA0 and 0xA2 also be
> silently accepted, and translated to 0x50 and 0x51 respectively.  Some
> of the specs use A0/A2 instead of 0x50/0x51.  They actually mean the
> same thing.
>
>> + * @data: Pointer to buffer with EEPROM data of @length size.
>> + *
>> + * This can be used to manage pages during EEPROM dump in ethtool and
>> +pass
>> + * required information to the driver.
>> + */
>> +struct ethtool_eeprom_data {
>> +     __u32   offset;
>> +     __u32   length;
>> +     __u32   page;
>> +     __u32   bank;
>> +     __u32   i2c_address;
>> +     __u8    *data;
>> +};
>> +
>>   /**
>>    * struct ethtool_eee - Energy Efficient Ethernet information
>>    * @cmd: ETHTOOL_{G,S}EEE
>> @@ -1865,6 +1887,10 @@ static inline int ethtool_validate_duplex(__u8
>> duplex)
>>   #define ETH_MODULE_SFF_8636_MAX_LEN     640
>>   #define ETH_MODULE_SFF_8436_MAX_LEN     640
>>
>> +#define ETH_MODULE_EEPROM_MAX_LEN    640
> Please don't add this MAX_LEN constant.  Even better, remove
> the two above it as well.


These constants are relevant to the ioctl function and used by the 
drivers. For netlink new KAPI I will remove the new MAX_LEN per your 
explanation, thanks.

> The proper value for all 3 of these MAX_LEN items is the
> architectural limit imposed by the 8 bit page register plus the constant
> lower page (hence 257*128 bytes).  The 8436 and 8636
> specs do not actually limit these devices to 640 bytes (3 pages).
>
> There is no MAX_LEN listed for SFF_8472.  If there is one, it should
> actually be 259 * 128 bytes (to account for 256 more bytes on the
> unpaged 0x50 i2c address).
>
>   Nor is there one for CMIS.  The maximum
> architected length for CMIS is (257*128) + (127 * 16 * 128).  That's
> the QSFP max length plus 127 more banks of 16 pages.
>
>> +#define ETH_MODULE_EEPROM_PAGE_LEN   256
>> +#define ETH_MODULE_MAX_I2C_ADDRESS   0x7f
> Actually there are only two legal values for the i2c address (0x50, 0x51).
> Rather than defining a MAX address, consider defining the legal values,
> or...  is it used at all?  Leave it out?
As Andrew commented there might be usage of other i2c adresses, I will 
keep it.
>> +
>>   /* Reset flags */
>>   /* The reset() operation must clear the flags for the components which
>>    * were actually reset.  On successful return, the flags indicate the
> diff --git
>> a/include/uapi/linux/ethtool_netlink.h
>> b/include/uapi/linux/ethtool_netlink.h
>> index a286635ac9b8..60dd848d0b54 100644
>> --- a/include/uapi/linux/ethtool_netlink.h
>> +++ b/include/uapi/linux/ethtool_netlink.h
>> @@ -42,6 +42,7 @@ enum {
>>        ETHTOOL_MSG_CABLE_TEST_ACT,
>>        ETHTOOL_MSG_CABLE_TEST_TDR_ACT,
>>        ETHTOOL_MSG_TUNNEL_INFO_GET,
>> +     ETHTOOL_MSG_EEPROM_DATA_GET,
>>
>>        /* add new constants above here */
>>        __ETHTOOL_MSG_USER_CNT,
>> @@ -80,6 +81,7 @@ enum {
>>        ETHTOOL_MSG_CABLE_TEST_NTF,
>>        ETHTOOL_MSG_CABLE_TEST_TDR_NTF,
>>        ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY,
>> +     ETHTOOL_MSG_EEPROM_DATA_GET_REPLY,
>>
>>        /* add new constants above here */
>>        __ETHTOOL_MSG_KERNEL_CNT,
>> @@ -629,6 +631,23 @@ enum {
>>        ETHTOOL_A_TUNNEL_INFO_MAX =
>> (__ETHTOOL_A_TUNNEL_INFO_CNT - 1)  };
>>
>> +/* MODULE EEPROM DATA */
>> +
>> +enum {
>> +     ETHTOOL_A_EEPROM_DATA_UNSPEC,
>> +     ETHTOOL_A_EEPROM_DATA_HEADER,
>> +
>> +     ETHTOOL_A_EEPROM_DATA_OFFSET,
>> +     ETHTOOL_A_EEPROM_DATA_LENGTH,
>> +     ETHTOOL_A_EEPROM_DATA_PAGE,
>> +     ETHTOOL_A_EEPROM_DATA_BANK,
>> +     ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
>> +     ETHTOOL_A_EEPROM_DATA,
>> +
>> +     __ETHTOOL_A_EEPROM_DATA_CNT,
>> +     ETHTOOL_A_EEPROM_DATA_MAX =
>> (__ETHTOOL_A_EEPROM_DATA_CNT - 1) };
>> +
>>   /* generic netlink info */
>>   #define ETHTOOL_GENL_NAME "ethtool"
>>   #define ETHTOOL_GENL_VERSION 1
>> diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile index
>> 7a849ff22dad..d604346bc074 100644
>> --- a/net/ethtool/Makefile
>> +++ b/net/ethtool/Makefile
>> @@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
>>   ethtool_nl-y := netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
>>                   linkstate.o debug.o wol.o features.o privflags.o rings.o
> \
>>                   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o
> \
>> -                tunnels.o
>> +                tunnels.o eeprom.o
>> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c new file mode
>> 100644 index 000000000000..2618a55b9a40
>> --- /dev/null
>> +++ b/net/ethtool/eeprom.c
>> @@ -0,0 +1,157 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <linux/ethtool.h>
>> +#include "netlink.h"
>> +#include "common.h"
>> +
>> +struct eeprom_data_req_info {
>> +     struct ethnl_req_info   base;
>> +     u32                     offset;
>> +     u32                     length;
>> +     u32                     page;
>> +     u32                     bank;
>> +     u32                     i2c_address;
>> +};
>> +
>> +struct eeprom_data_reply_data {
>> +     struct ethnl_reply_data base;
>> +     u32                     length;
>> +     u32                     i2c_address;
>> +     u8                      *data;
>> +};
>> +
>> +#define EEPROM_DATA_REQINFO(__req_base) \
>> +     container_of(__req_base, struct eeprom_data_req_info, base)
>> +
>> +#define EEPROM_DATA_REPDATA(__reply_base) \
>> +     container_of(__reply_base, struct eeprom_data_reply_data, base)
>> +
>> +static int eeprom_data_prepare_data(const struct ethnl_req_info
>> *req_base,
>> +                                 struct ethnl_reply_data *reply_base,
>> +                                 struct genl_info *info)
>> +{
>> +     struct eeprom_data_reply_data *reply =
>> EEPROM_DATA_REPDATA(reply_base);
>> +     struct eeprom_data_req_info *request =
>> EEPROM_DATA_REQINFO(req_base);
>> +     struct ethtool_eeprom_data page_data = {0};
>> +     struct net_device *dev = reply_base->dev;
>> +     int err;
>> +
>> +     if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
>> +             return -EOPNOTSUPP;
>> +
>> +     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;
>> +     err = ethnl_ops_begin(dev);
>> +     if (err)
>> +             goto err_free;
>> +
>> +     err = dev->ethtool_ops->get_module_eeprom_data_by_page(dev,
>> &page_data,
>> +
> info->extack);
>> +     if (err)
>> +             goto err_ops;
>> +
>> +     reply->length = page_data.length;
>> +     reply->i2c_address = page_data.i2c_address;
>> +     reply->data = page_data.data;
>> +
>> +     ethnl_ops_complete(dev);
> The two error paths below kfree(page_data.data).  Does someone else
> free this memory when there is no error?
eeprom_data_cleanup_data()
>> +     return 0;
>> +
>> +err_ops:
>> +     ethnl_ops_complete(dev);
>> +err_free:
>> +     kfree(page_data.data);
>> +     return err;
>> +}
>> +
>> +static int eeprom_data_parse_request(struct ethnl_req_info *req_info,
>> struct nlattr **tb,
>> +                                  struct netlink_ext_ack *extack) {
>> +     struct eeprom_data_req_info *request =
>> EEPROM_DATA_REQINFO(req_info);
>> +
>> +     if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] ||
>> +         !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] ||
>> +         !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS])
>> +             return -EINVAL;
>> +
>> +     request->i2c_address =
>> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
>> +     if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
>> +             return -EINVAL;
> I would be much more restrictive, with one flexibility...
>
>          if (request->i2c_address == 0xA0) request->i2c_address = 0x50;
>          if (request->i2c_address == 0xA2) request->i2c_address = 0x51;
>          if (request->i2c_address < 0x50) || (request->i2c_address > 0x51)
>                  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 (request->length > ETH_MODULE_EEPROM_MAX_LEN)
>> +             return -EINVAL;
> This is really problematic as there are MANY different max values, within
> the specs, for the various EEPROMs being generically supported here.
> I would leave it to the drivers to handle out-of-range requests.  If you
> really want to check, you need to know which spec the module supports,
> whether it supports pages, and whether it supports banks.  I have not
> found a register that actually reports the number of supported pages
> that an eeprom supports.  The specs should have included that :-(.


Will remove this check per your explanation, thanks.

>> +     if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
>> +         request->offset + request->length >
>> ETH_MODULE_EEPROM_PAGE_LEN)
>> +             return -EINVAL;
> Why does this stanza depend on DATA_PAGE?  In this new data
> structure, no requests can cross the 256 byte page boundary.
>
> I suggest, rather then -EINVAL, you should reduce the length to reach
> the end of the page:
>
>          if (request->offset + request->length) > ETH_MODULE_EEPROM_PAGE_LEN)
>                  request->length = ETH_MODULE_EEPROM_PAGE_LEN -
>                                                        request->offset.


Backward compatibility, users use the command without page and bank as 
it was till now expect to get data that can cross page as it worked till 
now, I can't break it.

> Note that this matches the choice you made to truncate rather than
> error out in fallback_set_parms().

We truncate in fallback_set_params() to eeprom_len.

length = modinfo->eeprom_len - offset;

>> +
>> +     if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
>> +             request->page =
>> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
>> +     if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
>> +             request->bank =
>> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
> Other checks:
>
> Page and bank have to be between 0 and 255 (inclusive), they
> go into an 8 bit register in the eeprom.
Will add, thanks.
> Offset and length can't be negative.
Already are u32.
>> +
>> +     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(u32)) + /* _EEPROM_DATA_I2C_ADDRESS
>> */
>> +            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_u32(skb, ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
>> reply->i2c_address) ||
>> +         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_U32 },
>> +     [ETHTOOL_A_EEPROM_DATA_BANK]            = { .type = NLA_U32 },
>> +     [ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]     = { .type = NLA_U32 },
>> +     [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.18.2

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

[Moshe Shemesh]

On 3/5/2021 3:58 AM, Andrew Lunn wrote:
>
>> +/* MODULE EEPROM DATA */
>> +
>> +enum {
>> +     ETHTOOL_A_EEPROM_DATA_UNSPEC,
>> +     ETHTOOL_A_EEPROM_DATA_HEADER,
>> +
>> +     ETHTOOL_A_EEPROM_DATA_OFFSET,
>> +     ETHTOOL_A_EEPROM_DATA_LENGTH,
>> +     ETHTOOL_A_EEPROM_DATA_PAGE,
>> +     ETHTOOL_A_EEPROM_DATA_BANK,
>> +     ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS,
>> +     ETHTOOL_A_EEPROM_DATA,
> If you look at all the other such enums in ethtool_netlink, you will
> see a comment indicating the type. Please add them here as well.
>
> Please also update Documentation/networking/ethtool-netlink.rst.
Sure, will add.
>         Andrew

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

[Moshe Shemesh]

On 3/5/2021 2:50 AM, Don Bollinger wrote:
>
> On Thu, Mar 04, 2021 at 10:57AM-0800, 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 | 84
>> +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 83 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c index
>> 2618a55b9a40..72c7714a9d37 100644
>> --- a/net/ethtool/eeprom.c
>> +++ b/net/ethtool/eeprom.c
>> @@ -26,6 +26,88 @@ 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) {
> This is translating the new data structure into the old.  Hence, I assume we
> have i2c_addr, page, bank, offset, len to work with, and we should use
> all of them.  We shouldn't be applying the legacy data structure's rules
> to how we interpret the *request data.  Therefore...
>
>> +     u32 offset = request->offset;
>> +     u32 length = request->length;
>> +
>> +     if (request->page)
>> +             offset = 128 + request->page * 128 + offset;
> This is tricky to map to old behavior.  The new data structure should give
> lower
> memory for offsets less than 128, and paged upper memory for offsets of 128
> and higher.  There is no way to describe that request as {offset, length} in
> the
> old ethtool format with a fake linear memory.
>
>          if (request->page) {
>                  if (offset < 128) && (offset + length > 128)
>                         return -EINVAL;
>                  if (offset > 127) offset = request->page * 128 + offset;
Yes, if we got page, that's the new API.
>> +
>> +     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;
>> +
>> +     switch (modinfo->type) {
>> +     case ETH_MODULE_SFF_8079:
>> +             if (request->page > 1)
>> +                     return -EINVAL;
>> +             break;
>> +     case ETH_MODULE_SFF_8472:
>> +             if (request->page > 3)
> Not sure this is needed, there can be pages higher than 3.
>
>> +                     return -EINVAL;
> I *think* the linear memory on SFP puts 0x50 in the first
> 256 bytes, 0x51 after that, including pages after that.  So,
> the old fashioned linear memory offset needs to be adjusted
> for accesses to 0x51.  Thus add:
>
>          if (request->i2c_address == 0x51)
>                  offset += 256;
Will check that. In the old KAPI the i2c address is not a parameter, so 
it depends on driver implementation.
>> +             break;
>> +     case ETH_MODULE_SFF_8436:
>> +     case ETH_MODULE_SFF_8636:
> Not sure this is needed, there can be pages higher than 3.
>
>> +             if (request->page > 3)
>> +                     return -EINVAL;
>> +             break;
>> +     }
>> +     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 || request->i2c_address) {
> We don't need to reject if there is an i2c_address.  Indeed, we need that
> to determine the correct offset for the legacy linear memory offset.
Will check that. As Andrew said, there might be usage of other i2c 
addresses with old KAPI.
> Note my comment on an earlier patch in this series, I would have rejected
> any request that didn't have either 0x50 or 0x51 here.
>
>> +             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)
>> @@ -37,7 +119,7 @@ static int eeprom_data_prepare_data(const struct
>> ethnl_req_info *req_base,
>>        int err;
>>
>>        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.18.2
>