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

[RFC PATCH V4 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:
v3 -> v4:
- Renamed many identifiers to use 'eeprom' instead of 'eeprom_data'.
- Renamed netlink enums and defines to use 'MODULE_EEPROM' instead of
   'EEPROM_DATA'.
- Renamed struct ethtool_eeprom_data to ethtool_module_eeprom.
- Added MODULE_EEPROM_MAX_OFFSET (257 * 256) macro and check global offset
    against it to avoid overflow.
- Removed ndo pointer check from _parse_request().
- Removed separate length element from netlink response.
- Limited reads to 128 bytes without crossing half page bound.

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

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



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

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

-- 
2.18.2

[RFC PATCH V4 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_by_page() ethtool callback and implement
netlink infrastructure.

get_module_eeprom_by_page() allows network drivers to dump a part of
module's EEPROM specified by page and bank numbers along with offset and
length. It is effectively a netlink replacement for get_module_info()
and get_module_eeprom() pair, which is needed due to emergence of
complex non-linear EEPROM layouts.

Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
---
 Documentation/networking/ethtool-netlink.rst |  32 +++-
 include/linux/ethtool.h                      |  33 +++-
 include/uapi/linux/ethtool_netlink.h         |  19 +++
 net/ethtool/Makefile                         |   2 +-
 net/ethtool/eeprom.c                         | 161 +++++++++++++++++++
 net/ethtool/netlink.c                        |  10 ++
 net/ethtool/netlink.h                        |   2 +
 7 files changed, 255 insertions(+), 4 deletions(-)
 create mode 100644 net/ethtool/eeprom.c

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 05073482db05..b370d1186ceb 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1280,6 +1280,34 @@ Kernel response contents:
 For UDP tunnel table empty ``ETHTOOL_A_TUNNEL_UDP_TABLE_TYPES`` indicates that
 the table contains static entries, hard-coded by the NIC.
 
+MODULE_EEPROM
+===========
+
+Fetch module EEPROM data dump.
+
+Request contents:
+
+  =======================================  ======  ==========================
+  ``ETHTOOL_A_MODULE_EEPROM_HEADER``       nested  request header
+  ``ETHTOOL_A_MODULE_EEPROM_OFFSET``       u32     offset within a page
+  ``ETHTOOL_A_MODULE_EEPROM_LENGTH``       u32     amount of bytes to read
+  ``ETHTOOL_A_MODULE_EEPROM_PAGE``         u8      page number
+  ``ETHTOOL_A_MODULE_EEPROM_BANK``         u8      bank number
+  ``ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS``  u8      page I2C address
+  =======================================  ======  ==========================
+
+Kernel response contents:
+
+ +---------------------------------------------+--------+---------------------+
+ | ``ETHTOOL_A_MODULE_EEPROM_HEADER``          | nested | reply header        |
+ +---------------------------------------------+--------+---------------------+
+ | ``ETHTOOL_A_MODULE_EEPROM_DATA``            | nested | array of bytes from |
+ |                                             |        | module EEPROM       |
+ +---------------------------------------------+--------+---------------------+
+
+``ETHTOOL_A_MODULE_EEPROM_DATA`` has an attribute length equal to the amount of
+bytes driver actually read.
+
 Request translation
 ===================
 
@@ -1357,8 +1385,8 @@ are netlink only.
   ``ETHTOOL_GET_DUMP_FLAG``           n/a
   ``ETHTOOL_GET_DUMP_DATA``           n/a
   ``ETHTOOL_GET_TS_INFO``             ``ETHTOOL_MSG_TSINFO_GET``
-  ``ETHTOOL_GMODULEINFO``             n/a
-  ``ETHTOOL_GMODULEEEPROM``           n/a
+  ``ETHTOOL_GMODULEINFO``             ``ETHTOOL_MSG_MODULE_EEPROM_GET``
+  ``ETHTOOL_GMODULEEEPROM``           ``ETHTOOL_MSG_MODULE_EEPROM_GET``
   ``ETHTOOL_GEEE``                    ``ETHTOOL_MSG_EEE_GET``
   ``ETHTOOL_SEEE``                    ``ETHTOOL_MSG_EEE_SET``
   ``ETHTOOL_GRSSH``                   n/a
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index ec4cd3921c67..2f6df3b75dd6 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);
@@ -265,6 +266,31 @@ struct ethtool_pause_stats {
 	u64 rx_pause_frames;
 };
 
+#define ETH_MODULE_EEPROM_PAGE_LEN	256
+#define ETH_MODULE_MAX_I2C_ADDRESS	0x7f
+
+/**
+ * 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_module_eeprom {
+	__u32	offset;
+	__u32	length;
+	__u8	page;
+	__u8	bank;
+	__u8	i2c_address;
+	__u8	*data;
+};
+
 /**
  * struct ethtool_ops - optional netdev operations
  * @cap_link_lanes_supported: indicates if the driver supports lanes
@@ -410,6 +436,9 @@ struct ethtool_pause_stats {
  * @get_ethtool_phy_stats: Return extended statistics about the PHY device.
  *	This is only useful if the device maintains PHY statistics and
  *	cannot use the standard PHY library helpers.
+ * @get_module_eeprom_by_page: Get a region of plug-in module EEPROM data from
+ *	specified page. Returns a negative error code or the amount of bytes
+ *	read.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -515,6 +544,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_by_page)(struct net_device *dev,
+					     const struct ethtool_module_eeprom *page,
+					     struct netlink_ext_ack *extack);
 };
 
 int ethtool_check_ops(const struct ethtool_ops *ops);
@@ -538,7 +570,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_netlink.h b/include/uapi/linux/ethtool_netlink.h
index a286635ac9b8..c99dea5afbb2 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_MODULE_EEPROM_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_MODULE_EEPROM_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 */
+
+enum {
+	ETHTOOL_A_MODULE_EEPROM_UNSPEC,
+	ETHTOOL_A_MODULE_EEPROM_HEADER,			/* nest - _A_HEADER_* */
+
+	ETHTOOL_A_MODULE_EEPROM_OFFSET,			/* u32 */
+	ETHTOOL_A_MODULE_EEPROM_LENGTH,			/* u32 */
+	ETHTOOL_A_MODULE_EEPROM_PAGE,			/* u8 */
+	ETHTOOL_A_MODULE_EEPROM_BANK,			/* u8 */
+	ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS,		/* u8 */
+	ETHTOOL_A_MODULE_EEPROM_DATA,			/* nested */
+
+	__ETHTOOL_A_MODULE_EEPROM_CNT,
+	ETHTOOL_A_MODULE_EEPROM_MAX = (__ETHTOOL_A_MODULE_EEPROM_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..79d75e0e2391
--- /dev/null
+++ b/net/ethtool/eeprom.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/ethtool.h>
+#include "netlink.h"
+#include "common.h"
+
+struct eeprom_req_info {
+	struct ethnl_req_info	base;
+	u32			offset;
+	u32			length;
+	u8			page;
+	u8			bank;
+	u8			i2c_address;
+};
+
+struct eeprom_reply_data {
+	struct ethnl_reply_data base;
+	u32			length;
+	u8			*data;
+};
+
+/* Module EEPROMs use one byte to store page number, while each page is 256
+ * bytes long, so in principle offset is limited by a multiple of that,
+ * plus the length of an additional SFP page located at high I2C address.
+ */
+#define MODULE_EEPROM_MAX_OFFSET (257 * ETH_MODULE_EEPROM_PAGE_LEN)
+
+#define MODULE_EEPROM_REQINFO(__req_base) \
+	container_of(__req_base, struct eeprom_req_info, base)
+
+#define MODULE_EEPROM_REPDATA(__reply_base) \
+	container_of(__reply_base, struct eeprom_reply_data, base)
+
+static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
+			       struct ethnl_reply_data *reply_base,
+			       struct genl_info *info)
+{
+	struct eeprom_reply_data *reply = MODULE_EEPROM_REPDATA(reply_base);
+	struct eeprom_req_info *request = MODULE_EEPROM_REQINFO(req_base);
+	struct ethtool_module_eeprom page_data = {0};
+	struct net_device *dev = reply_base->dev;
+	int ret;
+
+	if (!dev->ethtool_ops->get_module_eeprom_by_page)
+		return -EOPNOTSUPP;
+
+	/* Allow dumps either of low or high page without crossing half page boundary */
+	if ((request->offset < ETH_MODULE_EEPROM_PAGE_LEN / 2 &&
+	     request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN / 2) ||
+	    request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
+		return -EINVAL;
+
+	page_data.offset = request->offset;
+	page_data.length = request->length;
+	page_data.i2c_address = request->i2c_address;
+	page_data.page = request->page;
+	page_data.bank = request->bank;
+	page_data.data = kmalloc(page_data.length, GFP_KERNEL);
+	if (!page_data.data)
+		return -ENOMEM;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret)
+		goto err_free;
+
+	ret = dev->ethtool_ops->get_module_eeprom_by_page(dev, &page_data,
+							  info->extack);
+	if (ret < 0)
+		goto err_ops;
+
+	reply->length = ret;
+	reply->data = page_data.data;
+
+	ethnl_ops_complete(dev);
+	return 0;
+
+err_ops:
+	ethnl_ops_complete(dev);
+err_free:
+	kfree(page_data.data);
+	return ret;
+}
+
+static int eeprom_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
+				struct netlink_ext_ack *extack)
+{
+	struct eeprom_req_info *request = MODULE_EEPROM_REQINFO(req_info);
+
+	if (!tb[ETHTOOL_A_MODULE_EEPROM_OFFSET] ||
+	    !tb[ETHTOOL_A_MODULE_EEPROM_LENGTH] ||
+	    !tb[ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS])
+		return -EINVAL;
+
+	request->i2c_address = nla_get_u8(tb[ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS]);
+	request->offset = nla_get_u32(tb[ETHTOOL_A_MODULE_EEPROM_OFFSET]);
+	request->length = nla_get_u32(tb[ETHTOOL_A_MODULE_EEPROM_LENGTH]);
+
+	if (request->offset > MODULE_EEPROM_MAX_OFFSET)
+		return -EINVAL;
+
+	if (tb[ETHTOOL_A_MODULE_EEPROM_PAGE]) {
+		request->page = nla_get_u8(tb[ETHTOOL_A_MODULE_EEPROM_PAGE]);
+		if (request->page > 0 &&
+		    (request->offset < ETH_MODULE_EEPROM_PAGE_LEN / 2 ||
+		     request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN))
+			return -EINVAL;
+	}
+	if (tb[ETHTOOL_A_MODULE_EEPROM_BANK])
+		request->bank = nla_get_u8(tb[ETHTOOL_A_MODULE_EEPROM_BANK]);
+
+	return 0;
+}
+
+static int eeprom_reply_size(const struct ethnl_req_info *req_base,
+			     const struct ethnl_reply_data *reply_base)
+{
+	const struct eeprom_req_info *request = MODULE_EEPROM_REQINFO(req_base);
+
+	return nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA */
+}
+
+static int eeprom_fill_reply(struct sk_buff *skb,
+			     const struct ethnl_req_info *req_base,
+			     const struct ethnl_reply_data *reply_base)
+{
+	struct eeprom_reply_data *reply = MODULE_EEPROM_REPDATA(reply_base);
+
+	return nla_put(skb, ETHTOOL_A_MODULE_EEPROM_DATA, reply->length, reply->data);
+}
+
+static void eeprom_cleanup_data(struct ethnl_reply_data *reply_base)
+{
+	struct eeprom_reply_data *reply = MODULE_EEPROM_REPDATA(reply_base);
+
+	kfree(reply->data);
+}
+
+const struct ethnl_request_ops ethnl_module_eeprom_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_MODULE_EEPROM_GET,
+	.reply_cmd		= ETHTOOL_MSG_MODULE_EEPROM_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_MODULE_EEPROM_HEADER,
+	.req_info_size		= sizeof(struct eeprom_req_info),
+	.reply_data_size	= sizeof(struct eeprom_reply_data),
+
+	.parse_request		= eeprom_parse_request,
+	.prepare_data		= eeprom_prepare_data,
+	.reply_size		= eeprom_reply_size,
+	.fill_reply		= eeprom_fill_reply,
+	.cleanup_data		= eeprom_cleanup_data,
+};
+
+const struct nla_policy ethnl_module_eeprom_get_policy[] = {
+	[ETHTOOL_A_MODULE_EEPROM_HEADER]	= NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_MODULE_EEPROM_OFFSET]	= { .type = NLA_U32 },
+	[ETHTOOL_A_MODULE_EEPROM_LENGTH]	= { .type = NLA_U32 },
+	[ETHTOOL_A_MODULE_EEPROM_PAGE]		= { .type = NLA_U8 },
+	[ETHTOOL_A_MODULE_EEPROM_BANK]		= { .type = NLA_U8 },
+	[ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS]	=
+		NLA_POLICY_RANGE(NLA_U8, 0, ETH_MODULE_MAX_I2C_ADDRESS),
+};
+
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 50d3c8896f91..52d1ffb1bca2 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_MODULE_EEPROM_GET]	= &ethnl_module_eeprom_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_MODULE_EEPROM_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_module_eeprom_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_module_eeprom_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..71cbd9df8229 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_module_eeprom_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_module_eeprom_get_policy[ETHTOOL_A_MODULE_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 V4 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 V4 net-next 3/5] net/mlx5: Implement get_module_eeprom_by_page()

[Moshe Shemesh]

From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>

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

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

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index abdf721bb264..7f413f7bd1d3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1769,6 +1769,49 @@ static int mlx5e_get_module_eeprom(struct net_device *netdev,
 	return 0;
 }
 
+static int mlx5e_get_module_eeprom_by_page(struct net_device *netdev,
+					   const struct ethtool_module_eeprom *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_by_page(mdev, &query, data + i);
+
+		/* Done reading, return how many bytes was read */
+		if (!size_read)
+			return i;
+
+		if (size_read == -EINVAL)
+			return -EINVAL;
+		if (size_read < 0) {
+			netdev_err(priv->netdev, "%s: mlx5_query_module_eeprom_by_page failed:0x%x\n",
+				   __func__, size_read);
+			return i;
+		}
+
+		i += size_read;
+		query.offset += size_read;
+	}
+
+	return i;
+}
+
 int mlx5e_ethtool_flash_device(struct mlx5e_priv *priv,
 			       struct ethtool_flash *flash)
 {
@@ -2148,6 +2191,7 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
 	.set_wol	   = mlx5e_set_wol,
 	.get_module_info   = mlx5e_get_module_info,
 	.get_module_eeprom = mlx5e_get_module_eeprom,
+	.get_module_eeprom_by_page = mlx5e_get_module_eeprom_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..114214728e32 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_by_page(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_by_page);
+
 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..58d56adb9842 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_by_page(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 V4 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 114214728e32..5008d6d30899 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_by_page(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 58d56adb9842..77ea4f9c5265 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 V4 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_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 | 72 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
index 79d75e0e2391..060fe0610044 100644
--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c
@@ -31,6 +31,76 @@ struct eeprom_reply_data {
 #define MODULE_EEPROM_REPDATA(__reply_base) \
 	container_of(__reply_base, struct eeprom_reply_data, base)
 
+static int fallback_set_params(struct eeprom_req_info *request,
+			       struct ethtool_modinfo *modinfo,
+			       struct ethtool_eeprom *eeprom)
+{
+	u32 offset = request->offset;
+	u32 length = request->length;
+
+	if (!length)
+		length = modinfo->eeprom_len;
+
+	if (request->page)
+		offset = request->page * 128 + offset;
+
+	if (modinfo->type == ETH_MODULE_SFF_8079 &&
+	    request->i2c_address == 0x51)
+		offset += ETH_MODULE_EEPROM_PAGE_LEN;
+
+	if (offset >= modinfo->eeprom_len)
+		return -EINVAL;
+
+	if (modinfo->eeprom_len < offset + length)
+		length = modinfo->eeprom_len - offset;
+
+	eeprom->cmd = ETHTOOL_GMODULEEEPROM;
+	eeprom->len = length;
+	eeprom->offset = offset;
+
+	return 0;
+}
+
+static int eeprom_fallback(struct eeprom_req_info *request,
+			   struct eeprom_reply_data *reply,
+			   struct genl_info *info)
+{
+	struct net_device *dev = reply->base.dev;
+	struct ethtool_modinfo modinfo = {0};
+	struct ethtool_eeprom eeprom = {0};
+	u8 *data;
+	int err;
+
+	if (!dev->ethtool_ops->get_module_info ||
+	    !dev->ethtool_ops->get_module_eeprom || request->bank) {
+		return -EOPNOTSUPP;
+	}
+	modinfo.cmd = ETHTOOL_GMODULEINFO;
+	err = dev->ethtool_ops->get_module_info(dev, &modinfo);
+	if (err < 0)
+		return err;
+
+	err = fallback_set_params(request, &modinfo, &eeprom);
+	if (err < 0)
+		return err;
+
+	data = kmalloc(eeprom.len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	err = dev->ethtool_ops->get_module_eeprom(dev, &eeprom, data);
+	if (err < 0)
+		goto err_out;
+
+	reply->data = data;
+	reply->length = eeprom.len;
+
+	return 0;
+
+err_out:
+	kfree(data);
+	return err;
+}
+
 static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
 			       struct ethnl_reply_data *reply_base,
 			       struct genl_info *info)
@@ -42,7 +112,7 @@ static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
 	int ret;
 
 	if (!dev->ethtool_ops->get_module_eeprom_by_page)
-		return -EOPNOTSUPP;
+		return eeprom_fallback(request, reply, info);
 
 	/* Allow dumps either of low or high page without crossing half page boundary */
 	if ((request->offset < ETH_MODULE_EEPROM_PAGE_LEN / 2 &&
-- 
2.18.2

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

[Don Bollinger]

> From: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
> 
> Define get_module_eeprom_by_page() ethtool callback and implement
> netlink infrastructure.
> 
> get_module_eeprom_by_page() allows network drivers to dump a part of
> module's EEPROM specified by page and bank numbers along with offset and
> length. It is effectively a netlink replacement for get_module_info() and
> get_module_eeprom() pair, which is needed due to emergence of complex
> non-linear EEPROM layouts.
> 
> Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com>
> ---
>  Documentation/networking/ethtool-netlink.rst |  32 +++-
>  include/linux/ethtool.h                      |  33 +++-
>  include/uapi/linux/ethtool_netlink.h         |  19 +++
>  net/ethtool/Makefile                         |   2 +-
>  net/ethtool/eeprom.c                         | 161 +++++++++++++++++++
>  net/ethtool/netlink.c                        |  10 ++
>  net/ethtool/netlink.h                        |   2 +
>  7 files changed, 255 insertions(+), 4 deletions(-)  create mode 100644
> net/ethtool/eeprom.c
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst
> b/Documentation/networking/ethtool-netlink.rst
> index 05073482db05..b370d1186ceb 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -1280,6 +1280,34 @@ Kernel response contents:
>  For UDP tunnel table empty ``ETHTOOL_A_TUNNEL_UDP_TABLE_TYPES``
> indicates that  the table contains static entries, hard-coded by the NIC.
> 
> +MODULE_EEPROM
> +===========
> +
> +Fetch module EEPROM data dump.
> +
> +Request contents:
> +
> +  =======================================  ======
> ==========================
> +  ``ETHTOOL_A_MODULE_EEPROM_HEADER``       nested  request header
> +  ``ETHTOOL_A_MODULE_EEPROM_OFFSET``       u32     offset within a page
> +  ``ETHTOOL_A_MODULE_EEPROM_LENGTH``       u32     amount of bytes to
> read
> +  ``ETHTOOL_A_MODULE_EEPROM_PAGE``         u8      page number
> +  ``ETHTOOL_A_MODULE_EEPROM_BANK``         u8      bank number
> +  ``ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS``  u8      page I2C address
> +  =======================================  ======
> + ==========================
> +
> +Kernel response contents:
> +
> +
+---------------------------------------------+--------+--------------------
-+
> + | ``ETHTOOL_A_MODULE_EEPROM_HEADER``          | nested | reply header
> |
> +
+---------------------------------------------+--------+--------------------
-+
> + | ``ETHTOOL_A_MODULE_EEPROM_DATA``            | nested | array of bytes
> from |
> + |                                             |        | module EEPROM
|
> +
+---------------------------------------------+--------+--------------------
-+
> +
> +``ETHTOOL_A_MODULE_EEPROM_DATA`` has an attribute length equal to
> the
> +amount of bytes driver actually read.
> +
>  Request translation
>  ===================
> 
> @@ -1357,8 +1385,8 @@ are netlink only.
>    ``ETHTOOL_GET_DUMP_FLAG``           n/a
>    ``ETHTOOL_GET_DUMP_DATA``           n/a
>    ``ETHTOOL_GET_TS_INFO``             ``ETHTOOL_MSG_TSINFO_GET``
> -  ``ETHTOOL_GMODULEINFO``             n/a
> -  ``ETHTOOL_GMODULEEEPROM``           n/a
> +  ``ETHTOOL_GMODULEINFO``
> ``ETHTOOL_MSG_MODULE_EEPROM_GET``
> +  ``ETHTOOL_GMODULEEEPROM``
> ``ETHTOOL_MSG_MODULE_EEPROM_GET``
>    ``ETHTOOL_GEEE``                    ``ETHTOOL_MSG_EEE_GET``
>    ``ETHTOOL_SEEE``                    ``ETHTOOL_MSG_EEE_SET``
>    ``ETHTOOL_GRSSH``                   n/a
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index
> ec4cd3921c67..2f6df3b75dd6 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); @@ -265,6 +266,31 @@
> struct ethtool_pause_stats {
>  	u64 rx_pause_frames;
>  };
> 
> +#define ETH_MODULE_EEPROM_PAGE_LEN	256

Sorry to keep raising issues, but I think you want to make this constant
128.  The page register swaps in different pages, each 128 bytes long, into
the full 256 byte address range of the device.  If you check, you will find
you use ETH_MODULE_EEPROM_PAGE_LEN/2 most of the time.  More important, note
below, your #define MODULE_EEPROM_MAX_OFFSET  is twice as large as it should
be because you count each page as 256 bytes.

If you prefer to call the whole 256 bytes a page, I'm OK with that.  You'll
need to change the MAX_OFFSET constant as noted below.

> +#define ETH_MODULE_MAX_I2C_ADDRESS	0x7f
> +
> +/**
> + * 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_module_eeprom {
> +	__u32	offset;
> +	__u32	length;
> +	__u8	page;
> +	__u8	bank;
> +	__u8	i2c_address;
> +	__u8	*data;
> +};
> +
>  /**
>   * struct ethtool_ops - optional netdev operations
>   * @cap_link_lanes_supported: indicates if the driver supports lanes @@ -
> 410,6 +436,9 @@ struct ethtool_pause_stats {
>   * @get_ethtool_phy_stats: Return extended statistics about the PHY
> device.
>   *	This is only useful if the device maintains PHY statistics and
>   *	cannot use the standard PHY library helpers.
> + * @get_module_eeprom_by_page: Get a region of plug-in module
> EEPROM data from
> + *	specified page. Returns a negative error code or the amount of bytes
> + *	read.
>   *
>   * All operations are optional (i.e. the function pointer may be set
>   * to %NULL) and callers must take this into account.  Callers must @@
-515,6
> +544,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_by_page)(struct net_device *dev,
> +					     const struct
> ethtool_module_eeprom *page,
> +					     struct netlink_ext_ack
*extack);
>  };
> 
>  int ethtool_check_ops(const struct ethtool_ops *ops); @@ -538,7 +570,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_netlink.h
> b/include/uapi/linux/ethtool_netlink.h
> index a286635ac9b8..c99dea5afbb2 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_MODULE_EEPROM_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_MODULE_EEPROM_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 */
> +
> +enum {
> +	ETHTOOL_A_MODULE_EEPROM_UNSPEC,
> +	ETHTOOL_A_MODULE_EEPROM_HEADER,			/*
> nest - _A_HEADER_* */
> +
> +	ETHTOOL_A_MODULE_EEPROM_OFFSET,			/*
> u32 */
> +	ETHTOOL_A_MODULE_EEPROM_LENGTH,			/*
> u32 */
> +	ETHTOOL_A_MODULE_EEPROM_PAGE,			/* u8
> */
> +	ETHTOOL_A_MODULE_EEPROM_BANK,			/* u8
> */
> +	ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS,		/* u8
> */
> +	ETHTOOL_A_MODULE_EEPROM_DATA,			/*
> nested */
> +
> +	__ETHTOOL_A_MODULE_EEPROM_CNT,
> +	ETHTOOL_A_MODULE_EEPROM_MAX =
> (__ETHTOOL_A_MODULE_EEPROM_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..79d75e0e2391
> --- /dev/null
> +++ b/net/ethtool/eeprom.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/ethtool.h>
> +#include "netlink.h"
> +#include "common.h"
> +
> +struct eeprom_req_info {
> +	struct ethnl_req_info	base;
> +	u32			offset;
> +	u32			length;
> +	u8			page;
> +	u8			bank;
> +	u8			i2c_address;
> +};
> +
> +struct eeprom_reply_data {
> +	struct ethnl_reply_data base;
> +	u32			length;
> +	u8			*data;
> +};
> +
> +/* Module EEPROMs use one byte to store page number, while each page
> is
> +256
> + * bytes long, so in principle offset is limited by a multiple of that,
> + * plus the length of an additional SFP page located at high I2C address.
> + */
> +#define MODULE_EEPROM_MAX_OFFSET (257 *
> ETH_MODULE_EEPROM_PAGE_LEN)

The device actually has 257 addressable chunks of 128 bytes each.  With
ETH_MODULE_EEPROM_PAGE_LEN set to 256, your constant is 2X too big.

Note also, SFP devices (but not QSFP or CMIS) actually have another 256
bytes available at 0x50, in addition to the full 257*128 at 0x51.  So SFP is
actually 259*128 or (256 + 257 * 128).

Devices that don't support pages have much lower limits (256 bytes for
QSFP/CMIS and 512 for SFP).  Some SFP only support 256 bytes.  Most devices
will return nonsense for pages above 3.  So, this check is really only an
absolute limit.  The SFP driver that takes this request will probably check
against a more refined MAX length (eg modinfo->eeprom_len).

I suggest setting this constant to 259 * (ETH_MODULE_EEPROM_PAGE_LEN / 2).
Let the driver refine it from there.

> +
> +#define MODULE_EEPROM_REQINFO(__req_base) \
> +	container_of(__req_base, struct eeprom_req_info, base)
> +
> +#define MODULE_EEPROM_REPDATA(__reply_base) \
> +	container_of(__reply_base, struct eeprom_reply_data, base)
> +
> +static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
> +			       struct ethnl_reply_data *reply_base,
> +			       struct genl_info *info)
> +{
> +	struct eeprom_reply_data *reply =
> MODULE_EEPROM_REPDATA(reply_base);
> +	struct eeprom_req_info *request =
> MODULE_EEPROM_REQINFO(req_base);
> +	struct ethtool_module_eeprom page_data = {0};
> +	struct net_device *dev = reply_base->dev;
> +	int ret;
> +
> +	if (!dev->ethtool_ops->get_module_eeprom_by_page)
> +		return -EOPNOTSUPP;
> +
> +	/* Allow dumps either of low or high page without crossing half page
> boundary */
> +	if ((request->offset < ETH_MODULE_EEPROM_PAGE_LEN / 2 &&
> +	     request->offset + request->length >
> ETH_MODULE_EEPROM_PAGE_LEN / 2) ||
> +	    request->offset + request->length >
> ETH_MODULE_EEPROM_PAGE_LEN)
> +		return -EINVAL;
> +
> +	page_data.offset = request->offset;
> +	page_data.length = request->length;
> +	page_data.i2c_address = request->i2c_address;
> +	page_data.page = request->page;
> +	page_data.bank = request->bank;
> +	page_data.data = kmalloc(page_data.length, GFP_KERNEL);
> +	if (!page_data.data)
> +		return -ENOMEM;
> +
> +	ret = ethnl_ops_begin(dev);
> +	if (ret)
> +		goto err_free;
> +
> +	ret = dev->ethtool_ops->get_module_eeprom_by_page(dev,
> &page_data,
> +							  info->extack);
> +	if (ret < 0)
> +		goto err_ops;
> +
> +	reply->length = ret;
> +	reply->data = page_data.data;
> +
> +	ethnl_ops_complete(dev);
> +	return 0;
> +
> +err_ops:
> +	ethnl_ops_complete(dev);
> +err_free:
> +	kfree(page_data.data);
> +	return ret;
> +}
> +
> +static int eeprom_parse_request(struct ethnl_req_info *req_info, struct
> nlattr **tb,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct eeprom_req_info *request =
> MODULE_EEPROM_REQINFO(req_info);
> +
> +	if (!tb[ETHTOOL_A_MODULE_EEPROM_OFFSET] ||
> +	    !tb[ETHTOOL_A_MODULE_EEPROM_LENGTH] ||
> +	    !tb[ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS])
> +		return -EINVAL;
> +
> +	request->i2c_address =
> nla_get_u8(tb[ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS]);
> +	request->offset =
> nla_get_u32(tb[ETHTOOL_A_MODULE_EEPROM_OFFSET]);
> +	request->length =
> nla_get_u32(tb[ETHTOOL_A_MODULE_EEPROM_LENGTH]);
> +
> +	if (request->offset > MODULE_EEPROM_MAX_OFFSET)
> +		return -EINVAL;
> +
> +	if (tb[ETHTOOL_A_MODULE_EEPROM_PAGE]) {
> +		request->page =
> nla_get_u8(tb[ETHTOOL_A_MODULE_EEPROM_PAGE]);
> +		if (request->page > 0 &&
> +		    (request->offset < ETH_MODULE_EEPROM_PAGE_LEN / 2
> ||
> +		     request->offset + request->length >
> ETH_MODULE_EEPROM_PAGE_LEN))
> +			return -EINVAL;
> +	}
> +	if (tb[ETHTOOL_A_MODULE_EEPROM_BANK])
> +		request->bank =
> nla_get_u8(tb[ETHTOOL_A_MODULE_EEPROM_BANK]);
> +
> +	return 0;
> +}
> +
> +static int eeprom_reply_size(const struct ethnl_req_info *req_base,
> +			     const struct ethnl_reply_data *reply_base) {
> +	const struct eeprom_req_info *request =
> +MODULE_EEPROM_REQINFO(req_base);
> +
> +	return nla_total_size(sizeof(u8) * request->length); /*
> _EEPROM_DATA
> +*/ }
> +
> +static int eeprom_fill_reply(struct sk_buff *skb,
> +			     const struct ethnl_req_info *req_base,
> +			     const struct ethnl_reply_data *reply_base) {
> +	struct eeprom_reply_data *reply =
> MODULE_EEPROM_REPDATA(reply_base);
> +
> +	return nla_put(skb, ETHTOOL_A_MODULE_EEPROM_DATA, reply-
> >length,
> +reply->data); }
> +
> +static void eeprom_cleanup_data(struct ethnl_reply_data *reply_base) {
> +	struct eeprom_reply_data *reply =
> MODULE_EEPROM_REPDATA(reply_base);
> +
> +	kfree(reply->data);
> +}
> +
> +const struct ethnl_request_ops ethnl_module_eeprom_request_ops = {
> +	.request_cmd		= ETHTOOL_MSG_MODULE_EEPROM_GET,
> +	.reply_cmd		=
> ETHTOOL_MSG_MODULE_EEPROM_GET_REPLY,
> +	.hdr_attr		= ETHTOOL_A_MODULE_EEPROM_HEADER,
> +	.req_info_size		= sizeof(struct eeprom_req_info),
> +	.reply_data_size	= sizeof(struct eeprom_reply_data),
> +
> +	.parse_request		= eeprom_parse_request,
> +	.prepare_data		= eeprom_prepare_data,
> +	.reply_size		= eeprom_reply_size,
> +	.fill_reply		= eeprom_fill_reply,
> +	.cleanup_data		= eeprom_cleanup_data,
> +};
> +
> +const struct nla_policy ethnl_module_eeprom_get_policy[] = {
> +	[ETHTOOL_A_MODULE_EEPROM_HEADER]	=
> NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_MODULE_EEPROM_OFFSET]	= { .type = NLA_U32 },
> +	[ETHTOOL_A_MODULE_EEPROM_LENGTH]	= { .type = NLA_U32 },
> +	[ETHTOOL_A_MODULE_EEPROM_PAGE]		= { .type =
> NLA_U8 },
> +	[ETHTOOL_A_MODULE_EEPROM_BANK]		= { .type =
> NLA_U8 },
> +	[ETHTOOL_A_MODULE_EEPROM_I2C_ADDRESS]	=
> +		NLA_POLICY_RANGE(NLA_U8, 0,
> ETH_MODULE_MAX_I2C_ADDRESS), };
> +
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index
> 50d3c8896f91..52d1ffb1bca2 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_MODULE_EEPROM_GET]	=
> &ethnl_module_eeprom_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_MODULE_EEPROM_GET,
> +		.doit	= ethnl_default_doit,
> +		.start	= ethnl_default_start,
> +		.dumpit	= ethnl_default_dumpit,
> +		.done	= ethnl_default_done,
> +		.policy = ethnl_module_eeprom_get_policy,
> +		.maxattr = ARRAY_SIZE(ethnl_module_eeprom_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..71cbd9df8229 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_module_eeprom_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_module_eeprom_get_policy[ETHTOOL_A_MODULE_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 V4 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Andrew Lunn]

> > +#define ETH_MODULE_EEPROM_PAGE_LEN	256
> 
> Sorry to keep raising issues, but I think you want to make this constant
> 128.

Yes, i also think the KAPI should be limited to returning a maximum of
a 1/2 page per call.

> > +#define MODULE_EEPROM_MAX_OFFSET (257 *
> > ETH_MODULE_EEPROM_PAGE_LEN)
> 
> The device actually has 257 addressable chunks of 128 bytes each.  With
> ETH_MODULE_EEPROM_PAGE_LEN set to 256, your constant is 2X too big.
> 
> Note also, SFP devices (but not QSFP or CMIS) actually have another 256
> bytes available at 0x50, in addition to the full 257*128 at 0x51.  So SFP is
> actually 259*128 or (256 + 257 * 128).
> 
> Devices that don't support pages have much lower limits (256 bytes for
> QSFP/CMIS and 512 for SFP).  Some SFP only support 256 bytes.  Most devices
> will return nonsense for pages above 3.  So, this check is really only an
> absolute limit.  The SFP driver that takes this request will probably check
> against a more refined MAX length (eg modinfo->eeprom_len).
> 
> I suggest setting this constant to 259 * (ETH_MODULE_EEPROM_PAGE_LEN / 2).
> Let the driver refine it from there.

I don't even see a need for this. The offset should be within one 1/2
page, of one bank. So offset >= 0 and <= 127. Length is also > 0 and
<- 127. And offset+length is <= 127.

For the moment, please forget about backwards compatibility with the
IOCTL interface. Lets get a new clean KAPI and a new clean internal
API between the ethtool core and the drivers. Once we have that agreed
on, we can work on the various compatibility shims we need to work
between old and new APIs in various places.

      Andrew

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

[Andrew Lunn]

> +static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
> +			       struct ethnl_reply_data *reply_base,
> +			       struct genl_info *info)
> +{
> +	struct eeprom_reply_data *reply = MODULE_EEPROM_REPDATA(reply_base);
> +	struct eeprom_req_info *request = MODULE_EEPROM_REQINFO(req_base);
> +	struct ethtool_module_eeprom page_data = {0};
> +	struct net_device *dev = reply_base->dev;
> +	int ret;
> +
> +	if (!dev->ethtool_ops->get_module_eeprom_by_page)
> +		return -EOPNOTSUPP;
> +
> +	/* Allow dumps either of low or high page without crossing half page boundary */
> +	if ((request->offset < ETH_MODULE_EEPROM_PAGE_LEN / 2 &&
> +	     request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN / 2) ||
> +	    request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
> +		return -EINVAL;

Please keep all the parameter validation together, in
eeprom_parse_request(). At some point, we may extend
eeprom_parse_request() to make use of extack, to indicate which
parameter is invalid. Just getting an -EINVAL can be hard to debug,
where as NL_SET_ERR_MSG_ATTR() can help the user.

      Andrew

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

[Don Bollinger]

> Subject: Re: [RFC PATCH V4 net-next 1/5] ethtool: Allow network drivers to
> dump arbitrary EEPROM data
> 
> > > +#define ETH_MODULE_EEPROM_PAGE_LEN	256
> >
> > Sorry to keep raising issues, but I think you want to make this
> > constant 128.
> 
> Yes, i also think the KAPI should be limited to returning a maximum of a
1/2
> page per call.
> 
> > > +#define MODULE_EEPROM_MAX_OFFSET (257 *
> > > ETH_MODULE_EEPROM_PAGE_LEN)
> >
> > The device actually has 257 addressable chunks of 128 bytes each.
> > With ETH_MODULE_EEPROM_PAGE_LEN set to 256, your constant is 2X too
> big.
> >
> > Note also, SFP devices (but not QSFP or CMIS) actually have another
> > 256 bytes available at 0x50, in addition to the full 257*128 at 0x51.
> > So SFP is actually 259*128 or (256 + 257 * 128).
> >
> > Devices that don't support pages have much lower limits (256 bytes for
> > QSFP/CMIS and 512 for SFP).  Some SFP only support 256 bytes.  Most
> > devices will return nonsense for pages above 3.  So, this check is
> > really only an absolute limit.  The SFP driver that takes this request
> > will probably check against a more refined MAX length (eg modinfo-
> >eeprom_len).
> >
> > I suggest setting this constant to 259 * (ETH_MODULE_EEPROM_PAGE_LEN
> / 2).
> > Let the driver refine it from there.
> 
> I don't even see a need for this. The offset should be within one 1/2
page, of
> one bank. So offset >= 0 and <= 127. Length is also > 0 and
> <- 127. And offset+length is <= 127.

I like the clean approach, but...   How do you request low memory?  If all
of the pages start at offset 0, then page 0 is at page 0, offset 0.  That
has always referred to low memory.  (I have actually used 'page -1' for this
in some code I don't share with others.)  I believe all of this code
currently is consistent with the paged area starting at offset 128.  If that
changes to start paged memory at offset 0, there are several places that
will need to be adjusted.

Whatever choice is made, there should be documentation scattered around in
the code, man pages and Documentation directories to tell the
user/programmer whether the paged area starts at offset 0 or offset 128.  It
is constantly confusing, and there is no obvious answer.

> 
> For the moment, please forget about backwards compatibility with the IOCTL
> interface. Lets get a new clean KAPI and a new clean internal API between
> the ethtool core and the drivers. Once we have that agreed on, we can work
> on the various compatibility shims we need to work between old and new
> APIs in various places.
> 
>       Andrew

Don

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

[Andrew Lunn]

> > I don't even see a need for this. The offset should be within one 1/2
> page, of
> > one bank. So offset >= 0 and <= 127. Length is also > 0 and
> > <- 127. And offset+length is <= 127.
> 
> I like the clean approach, but...   How do you request low memory?

Duh!

I got my conditions wrong. Too focused on 1/2 pages to think that two
of them makes one page!

Lets try again:

offset < 256
0 < len < 128

if (offset < 128)
   offset + len < 128
else
   offset + len < 256

Does that look better?

Reading bytes from the lower 1/2 of page 0 should give the same data
as reading data from the lower 1/2 of page 42. So we can allow that,
but don't be too surprised when an SFP gets it wrong and gives you
rubbish. I would suggest ethtool(1) never actually does read from the
lower 1/2 of any page other than 0.

And i agree about documentation. I would suggest a comment in
ethtool_netlink.h, and the RST documentation.

		   Andrew

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

[Don Bollinger]

> > > I don't even see a need for this. The offset should be within one
> > > 1/2
> > page, of
> > > one bank. So offset >= 0 and <= 127. Length is also > 0 and
> > > <- 127. And offset+length is <= 127.
> >
> > I like the clean approach, but...   How do you request low memory?
> 
> Duh!
> 
> I got my conditions wrong. Too focused on 1/2 pages to think that two of
> them makes one page!
> 
> Lets try again:
> 
> offset < 256
> 0 < len < 128

Actually 0 < len <= 128.  Length of 128 is not only legal, but very common.
"Read the whole 1/2 page block".

> 
> if (offset < 128)
>    offset + len < 128

Again, offset + len <= 128

> else
>    offset + len < 256

offset + len <= 256

> 
> Does that look better?
> 
> Reading bytes from the lower 1/2 of page 0 should give the same data as
> reading data from the lower 1/2 of page 42. So we can allow that, but
don't
> be too surprised when an SFP gets it wrong and gives you rubbish. I would

The spec is clear that the lower half is the same for all pages.  If the SFP
gives you rubbish you should throw the device in the rubbish.

> suggest ethtool(1) never actually does read from the lower 1/2 of any page
> other than 0.

I agree, despite my previous comment.  While the spec is clear that should
work, I believe virtually all such instances are bugs not yet discovered.

And, note that the legacy API provides no way to access lower memory from
any page but 0.  There's just no syntax for it.  Not that we care about
legacy :-).

> 
> And i agree about documentation. I would suggest a comment in
> ethtool_netlink.h, and the RST documentation.
> 
> 		   Andrew

Don

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

[Andrew Lunn]

> The spec is clear that the lower half is the same for all pages.  If the SFP
> gives you rubbish you should throw the device in the rubbish.

Sometimes you don't get the choice. You have to use the GPON SFP the
ISP sent you, if you want FTTH. And they tend to be cheap and broken
with respect to the spec.

    Andrew

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

[Moshe Shemesh]

On 3/23/2021 2:27 AM, Andrew Lunn wrote:
>
>>> +#define ETH_MODULE_EEPROM_PAGE_LEN 256
>> Sorry to keep raising issues, but I think you want to make this constant
>> 128.
> Yes, i also think the KAPI should be limited to returning a maximum of
> a 1/2 page per call.
OK.
>>> +#define MODULE_EEPROM_MAX_OFFSET (257 *
>>> ETH_MODULE_EEPROM_PAGE_LEN)
>> The device actually has 257 addressable chunks of 128 bytes each.  With
>> ETH_MODULE_EEPROM_PAGE_LEN set to 256, your constant is 2X too big.
>>
>> Note also, SFP devices (but not QSFP or CMIS) actually have another 256
>> bytes available at 0x50, in addition to the full 257*128 at 0x51.  So SFP is
>> actually 259*128 or (256 + 257 * 128).
>>
>> Devices that don't support pages have much lower limits (256 bytes for
>> QSFP/CMIS and 512 for SFP).  Some SFP only support 256 bytes.  Most devices
>> will return nonsense for pages above 3.  So, this check is really only an
>> absolute limit.  The SFP driver that takes this request will probably check
>> against a more refined MAX length (eg modinfo->eeprom_len).
>>
>> I suggest setting this constant to 259 * (ETH_MODULE_EEPROM_PAGE_LEN / 2).
>> Let the driver refine it from there.
> I don't even see a need for this. The offset should be within one 1/2
> page, of one bank. So offset >= 0 and <= 127. Length is also > 0 and
> <- 127. And offset+length is <= 127.
>
> For the moment, please forget about backwards compatibility with the
> IOCTL interface. Lets get a new clean KAPI and a new clean internal
> API between the ethtool core and the drivers. Once we have that agreed
> on, we can work on the various compatibility shims we need to work
> between old and new APIs in various places.


OK, so following the comments here, I will ignore backward compatibility 
of having global offset and length.

That means I assume in this KAPI I am asked to get data from specific 
page. Should I also require user space to send page number to this KAPI 
(I mean make page number mandatory) ?

>        Andrew

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

[Moshe Shemesh]

On 3/23/2021 2:54 AM, Andrew Lunn wrote:
>
>> +static int eeprom_prepare_data(const struct ethnl_req_info *req_base,
>> +                            struct ethnl_reply_data *reply_base,
>> +                            struct genl_info *info)
>> +{
>> +     struct eeprom_reply_data *reply = MODULE_EEPROM_REPDATA(reply_base);
>> +     struct eeprom_req_info *request = MODULE_EEPROM_REQINFO(req_base);
>> +     struct ethtool_module_eeprom page_data = {0};
>> +     struct net_device *dev = reply_base->dev;
>> +     int ret;
>> +
>> +     if (!dev->ethtool_ops->get_module_eeprom_by_page)
>> +             return -EOPNOTSUPP;
>> +
>> +     /* Allow dumps either of low or high page without crossing half page boundary */
>> +     if ((request->offset < ETH_MODULE_EEPROM_PAGE_LEN / 2 &&
>> +          request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN / 2) ||
>> +         request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
>> +             return -EINVAL;
> Please keep all the parameter validation together, in
> eeprom_parse_request(). At some point, we may extend
> eeprom_parse_request() to make use of extack, to indicate which
> parameter is invalid. Just getting an -EINVAL can be hard to debug,
> where as NL_SET_ERR_MSG_ATTR() can help the user.
Sure, we can add that.
>        Andrew

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

[Moshe Shemesh]

On 3/23/2021 7:47 PM, Don Bollinger wrote:
>>>> I don't even see a need for this. The offset should be within one
>>>> 1/2
>>> page, of
>>>> one bank. So offset >= 0 and <= 127. Length is also > 0 and
>>>> <- 127. And offset+length is <= 127.
>>> I like the clean approach, but...   How do you request low memory?
>> Duh!
>>
>> I got my conditions wrong. Too focused on 1/2 pages to think that two of
>> them makes one page!
>>
>> Lets try again:
>>
>> offset < 256
>> 0 < len < 128
> Actually 0 < len <= 128.  Length of 128 is not only legal, but very common.
> "Read the whole 1/2 page block".
Ack.
>> if (offset < 128)
>>     offset + len < 128
> Again, offset + len <= 128
>
>> else
>>     offset + len < 256
> offset + len <= 256
Ack.
>> Does that look better?
>>
>> Reading bytes from the lower 1/2 of page 0 should give the same data as
>> reading data from the lower 1/2 of page 42. So we can allow that, but
> don't
>> be too surprised when an SFP gets it wrong and gives you rubbish. I would
> The spec is clear that the lower half is the same for all pages.  If the SFP
> gives you rubbish you should throw the device in the rubbish.
>
>> suggest ethtool(1) never actually does read from the lower 1/2 of any page
>> other than 0.
> I agree, despite my previous comment.  While the spec is clear that should
> work, I believe virtually all such instances are bugs not yet discovered.


Agreed, so we will accept offset < 128 only on page zero.

> And, note that the legacy API provides no way to access lower memory from
> any page but 0.  There's just no syntax for it.  Not that we care about
> legacy :-).
>
>> And i agree about documentation. I would suggest a comment in
>> ethtool_netlink.h, and the RST documentation.


Ack, will comment there on limiting new KAPI only to half pages reading.

We may address reading cross pages by user space (implementing multiple 
calls to KAPI to get such data).

>>                   Andrew
> Don
>
>

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

[Andrew Lunn]

> OK, so following the comments here, I will ignore backward compatibility of
> having global offset and length.

Yes, we can do that in userspace.

> That means I assume in this KAPI I am asked to get data from specific page.
> Should I also require user space to send page number to this KAPI (I mean
> make page number mandatory) ?

It makes the API more explicit. We always need the page, so if it is
not passed you need to default to something. As with addresses, you
have no way to pass back what page was actually read. So yes, make it
mandatory.

And i suppose the next question is, do we make bank mandatory? Once
you have a page > 0x10, you need the bank. Either we need to encode
the logic of when a bank is needed, and make it mandatory then, or it
is always mandatory.

Given the general pattern, we make it mandatory.

But sometime in the future when we get yet another SFP format, with
additional parameters, they will be optional, in order to keep
backwards compatibility.

	  Andrew