[RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

This patchset extends the ethtool netlink API to allow user space to
both flash transceiver modules' firmware and query the firmware
information (e.g., version, state).

The main use case is CMIS compliant modules such as QSFP-DD. The CMIS
standard specifies the interfaces used for both operations. See section
7.3.1 in revision 5.0 of the standard [1].

Despite the immediate use case being CMIS compliant modules, the user
interface is kept generic enough to accommodate future use cases, if
these arise.

The purpose of this RFC is to solicit feedback on both the proposed user
interface and the device driver API which are described in detail in
patches #1 and #3. The netdevsim patches are for RFC purposes only. The
plan is to implement the CMIS functionality in common code (under lib/)
so that it can be shared by MAC drivers that will pass function pointers
to it in order to read and write from their modules EEPROM.

ethtool(8) patches can be found here [2].

[1] http://www.qsfp-dd.com/wp-content/uploads/2021/05/CMIS5p0.pdf
[2] https://github.com/idosch/ethtool/tree/submit/module_fw_rfc

Ido Schimmel (4):
  ethtool: Add ability to query transceiver modules' firmware
    information
  netdevsim: Implement support for ethtool_ops::get_module_fw_info
  ethtool: Add ability to flash transceiver modules' firmware
  netdevsim: Implement support for ethtool_ops::start_fw_flash_module

 Documentation/networking/ethtool-netlink.rst | 128 +++++++-
 drivers/net/netdevsim/ethtool.c              | 164 ++++++++++
 drivers/net/netdevsim/netdevsim.h            |  10 +
 include/linux/ethtool.h                      | 109 +++++++
 include/linux/ethtool_netlink.h              |   9 +
 include/uapi/linux/ethtool.h                 |  18 +
 include/uapi/linux/ethtool_netlink.h         |  49 +++
 net/ethtool/module.c                         | 327 +++++++++++++++++++
 net/ethtool/netlink.c                        |  17 +
 net/ethtool/netlink.h                        |   4 +
 10 files changed, 833 insertions(+), 2 deletions(-)

-- 
2.31.1

[RFC PATCH net-next 1/4] ethtool: Add ability to query transceiver modules' firmware information

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

CMIS compliant modules such as QSFP-DD may have up to three firmware
images: Two host updateable images stored in up to two storage banks (A
and B) and an internal factory image. These images have various states:

* Running: Whether the image is currently running or not
* Committed: Whether the image is to be run upon reset or not
* Valid: Whether the image is runnable and persistently stored
completely undamaged

When user space wishes to update the firmware of such a module, it needs to
choose one of two images provided by the module vendor based on the current
state. For example, if image A is running and committed, image B should be
used. By only downloading and running (but not committing) image B, user space
can choose to fallback to image A if image B does not work as expected.

To that end, add a new ethtool message that allows user space to query
this information. Example output from the succeeding netdevsim
implementation:

 # ethtool --show-module-firmware-info eth0
 Module firmware info for eth0:
 image:
   name: a
   running: true
   committed: true
   valid: true
   version: 1.2.3-test
 image:
   name: b
   running: false
   committed: false
   valid: true
   version: 5.6.7
 image:
   name: factory
   running: false
   committed: false
   valid: true
   version: 11.12.13

The user interface is designed with CMIS compliant modules in mind, but
kept generic enough to accommodate future use cases, if these arise.

On the other hand, the device driver API is kept CMIS specific, but
extensible (see 'enum ethtool_module_fw_info_type'). This design choice
exploits the fact that the interface to query firmware information from
CMIS compliant modules is standardized by the CMIS specification. As
such, the information queried from device drivers is structured and
easier to police.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/ethtool-netlink.rst |  68 ++++++-
 include/linux/ethtool.h                      |  70 +++++++
 include/uapi/linux/ethtool_netlink.h         |  28 +++
 net/ethtool/module.c                         | 202 +++++++++++++++++++
 net/ethtool/netlink.c                        |  10 +
 net/ethtool/netlink.h                        |   2 +
 6 files changed, 378 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 9d98e0511249..01393de9d759 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -184,7 +184,7 @@ according to message purpose:
 
 Userspace to kernel:
 
-  ===================================== =================================
+  ===================================== ====================================
   ``ETHTOOL_MSG_STRSET_GET``            get string set
   ``ETHTOOL_MSG_LINKINFO_GET``          get link settings
   ``ETHTOOL_MSG_LINKINFO_SET``          set link settings
@@ -220,7 +220,8 @@ Userspace to kernel:
   ``ETHTOOL_MSG_PHC_VCLOCKS_GET``       get PHC virtual clocks info
   ``ETHTOOL_MSG_MODULE_SET``            set transceiver module parameters
   ``ETHTOOL_MSG_MODULE_GET``            get transceiver module parameters
-  ===================================== =================================
+  ``ETHTOOL_MSG_MODULE_FW_INFO_GET``    get transceiver module firmware info
+  ===================================== ====================================
 
 Kernel to userspace:
 
@@ -260,6 +261,7 @@ Kernel to userspace:
   ``ETHTOOL_MSG_STATS_GET_REPLY``          standard statistics
   ``ETHTOOL_MSG_PHC_VCLOCKS_GET_REPLY``    PHC virtual clocks info
   ``ETHTOOL_MSG_MODULE_GET_REPLY``         transceiver module parameters
+  ``ETHTOOL_MSG_MODULE_FW_INFO_GET_REPLY`` transceiver module firmware info
   ======================================== =================================
 
 ``GET`` requests are sent by userspace applications to retrieve device
@@ -1598,6 +1600,67 @@ For SFF-8636 modules, low power mode is forced by the host according to table
 For CMIS modules, low power mode is forced by the host according to table 6-12
 in revision 5.0 of the specification.
 
+MODULE_FW_INFO_GET
+==================
+
+Gets transceiver module firmware information.
+
+Request contents:
+
+  ======================================  ======  ==========================
+  ``ETHTOOL_A_MODULE_FW_INFO_HEADER``     nested  request header
+  ======================================  ======  ==========================
+
+Kernel response contents:
+
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_INFO_HEADER``               | nested | reply header   |
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_INFO_IMAGE``                | nested | firmware image |
+ +-+-------------------------------------------------+--------+----------------+
+ | | ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_NAME``         | string | image name     |
+ +-+-------------------------------------------------+--------+----------------+
+ | | ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_RUNNING``      | bool   | running        |
+ +-+-------------------------------------------------+--------+----------------+
+ | | ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_COMMITTED``    | bool   | committed      |
+ +-+-------------------------------------------------+--------+----------------+
+ | | ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_VALID``        | bool   | valid          |
+ +-+-------------------------------------------------+--------+----------------+
+ | | ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_VERSION``      | string | image version  |
+ +-+-------------------------------------------------+--------+----------------+
+
+The ``ETHTOOL_A_MODULE_FW_INFO_IMAGE`` nested attribute may appear multiple
+times in the response, according to the number of firmware images stored in the
+transceiver module. CMIS modules, for example, may have up to three images: Two
+host updateable images stored in up to two storage banks (A and B) and an
+internal factory image. The following paragraphs describe various image
+attributes.
+
+The ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_NAME`` attribute encodes the image's name.
+The name is significant as a vendor may provide two firmware images, each one
+intended for a different bank.
+
+The ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_RUNNING`` attribute indicates if the image
+is currently running or not.
+
+The ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_COMMITTED`` attribute indicates if the
+image is to be run upon reset or not.
+
+The last two attributes can be used by user space to determine which firmware
+image should be used for the firmware update. In CMIS modules, for example, if
+image A is running and committed, image B should be used.
+
+The ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_VALID`` attribute encodes the validity of
+the image. A valid image is runnable and persistently stored completely
+undamaged.
+
+The ``ETHTOOL_A_MODULE_FW_INFO_IMAGE_VERSION`` attribute encodes the version of
+the image.
+
+For CMIS modules, the above mentioned information can be queried from the
+module using CDB CMD 0100h (Get Firmware Info). See section 9.7.1 in revision
+5.0 of the specification.
+
 Request translation
 ===================
 
@@ -1699,4 +1762,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_PHC_VCLOCKS_GET``
   n/a                                 ``ETHTOOL_MSG_MODULE_GET``
   n/a                                 ``ETHTOOL_MSG_MODULE_SET``
+  n/a                                 ``ETHTOOL_MSG_MODULE_FW_INFO_GET``
   =================================== =====================================
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index a26f37a27167..43506b119429 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -443,6 +443,71 @@ struct ethtool_module_power_mode_params {
 	enum ethtool_module_power_mode mode;
 };
 
+#define ETH_MODULE_FW_VER_LEN 48
+
+/**
+ * struct ethtool_module_fw_info_image - Module firmware image information
+ * @running: Whether the image is currently running or not.
+ * @committed: Whether the image is run upon resets.
+ * @valid: Whether the image is runnable and persistently stored completely
+ *	undamaged.
+ * @ver_major: Firmware image major revision.
+ * @ver_minor: Firmware image minor revision.
+ * @ver_build: Firmware image build number.
+ * @ver_extra_str: Firmware image additional information.
+ */
+struct ethtool_module_fw_info_image {
+	u8 running:1,
+	   committed:1,
+	   valid:1;
+	u8 ver_major;
+	u8 ver_minor;
+	u8 ver_build;
+	char ver_extra_str[ETH_MODULE_FW_VER_LEN];
+};
+
+/**
+ * struct ethtool_module_fw_info_cmis - CMIS module firmware information
+ * @a_present: Whether image A is present or not.
+ * @b_present: Whether image B is present or not.
+ * @factory_present: Whether factory image is present or not.
+ * @a: Image A firmware information.
+ * @b: Image B firmware information.
+ * @factory: Factory image firmware information.
+ *
+ * CMIS modules can have up to two host updateable images stored in up to two
+ * firmware banks, called A and B. In addition, the module may also have an
+ * internal factory image.
+ */
+struct ethtool_module_fw_info_cmis {
+	u8 a_present:1,
+	   b_present:1,
+	   factory_present:1;
+	struct ethtool_module_fw_info_image a;
+	struct ethtool_module_fw_info_image b;
+	struct ethtool_module_fw_info_image factory;
+};
+
+/**
+ * enum ethtool_module_fw_info_type - Module firmware information type
+ * @ETHTOOL_MODULE_FW_INFO_TYPE_CMIS: CMIS module firmware information type.
+ */
+enum ethtool_module_fw_info_type {
+	ETHTOOL_MODULE_FW_INFO_TYPE_CMIS = 1,
+};
+
+/**
+ * struct ethtool_module_fw_info - module firmware information
+ * @type: Module firmware information type.
+ * @cmis: CMIS module firmware information.
+ */
+struct ethtool_module_fw_info {
+	enum ethtool_module_fw_info_type type;
+	union {
+		struct ethtool_module_fw_info_cmis cmis;
+	};
+};
+
 /**
  * struct ethtool_ops - optional netdev operations
  * @cap_link_lanes_supported: indicates if the driver supports lanes
@@ -614,6 +679,8 @@ struct ethtool_module_power_mode_params {
  *	plugged-in.
  * @set_module_power_mode: Set the power mode policy for the plug-in module
  *	used by the network device.
+ * @get_module_fw_info: Get the firmware information of the plug-in module
+ *	used by the network device.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -750,6 +817,9 @@ struct ethtool_ops {
 	int	(*set_module_power_mode)(struct net_device *dev,
 					 const struct ethtool_module_power_mode_params *params,
 					 struct netlink_ext_ack *extack);
+	int	(*get_module_fw_info)(struct net_device *dev,
+				      struct ethtool_module_fw_info *info,
+				      struct netlink_ext_ack *extack);
 };
 
 int ethtool_check_ops(const struct ethtool_ops *ops);
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index cca6e474a085..7f09bfb28a42 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -49,6 +49,7 @@ enum {
 	ETHTOOL_MSG_PHC_VCLOCKS_GET,
 	ETHTOOL_MSG_MODULE_GET,
 	ETHTOOL_MSG_MODULE_SET,
+	ETHTOOL_MSG_MODULE_FW_INFO_GET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -94,6 +95,7 @@ enum {
 	ETHTOOL_MSG_PHC_VCLOCKS_GET_REPLY,
 	ETHTOOL_MSG_MODULE_GET_REPLY,
 	ETHTOOL_MSG_MODULE_NTF,
+	ETHTOOL_MSG_MODULE_FW_INFO_GET_REPLY,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -853,6 +855,32 @@ enum {
 	ETHTOOL_A_MODULE_MAX = (__ETHTOOL_A_MODULE_CNT - 1)
 };
 
+/* MODULE_FW_INFO */
+
+enum {
+	ETHTOOL_A_MODULE_FW_INFO_UNSPEC,
+	ETHTOOL_A_MODULE_FW_INFO_HEADER,	/* nest - _A_HEADER_* */
+	ETHTOOL_A_MODULE_FW_INFO_IMAGE,		/* nest */
+
+	/* add new constants above here */
+	__ETHTOOL_A_MODULE_FW_INFO_CNT,
+	ETHTOOL_A_MODULE_FW_INFO_MAX = (__ETHTOOL_A_MODULE_FW_INFO_CNT - 1)
+};
+
+enum {
+	ETHTOOL_A_MODULE_FW_INFO_IMAGE_UNSPEC,
+	ETHTOOL_A_MODULE_FW_INFO_IMAGE_NAME,		/* string */
+	ETHTOOL_A_MODULE_FW_INFO_IMAGE_RUNNING,		/* u8 */
+	ETHTOOL_A_MODULE_FW_INFO_IMAGE_COMMITTED,	/* u8 */
+	ETHTOOL_A_MODULE_FW_INFO_IMAGE_VALID,		/* u8 */
+	ETHTOOL_A_MODULE_FW_INFO_IMAGE_VERSION,		/* string */
+
+	/* add new constants above here */
+	__ETHTOOL_A_MODULE_FW_INFO_IMAGE_CNT,
+	ETHTOOL_A_MODULE_FW_INFO_IMAGE_MAX
+		= (__ETHTOOL_A_MODULE_FW_INFO_IMAGE_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index bc2cef11bbda..7f86c01a0b40 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -18,6 +18,18 @@ struct module_reply_data {
 #define MODULE_REPDATA(__reply_base) \
 	container_of(__reply_base, struct module_reply_data, base)
 
+struct module_fw_info_req_info {
+	struct ethnl_req_info base;
+};
+
+struct module_fw_info_reply_data {
+	struct ethnl_reply_data	base;
+	struct ethtool_module_fw_info fw_info;
+};
+
+#define MODULE_FW_INFO_REPDATA(__reply_base) \
+	container_of(__reply_base, struct module_fw_info_reply_data, base)
+
 /* MODULE_GET */
 
 const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1] = {
@@ -178,3 +190,193 @@ int ethnl_set_module(struct sk_buff *skb, struct genl_info *info)
 	dev_put(dev);
 	return ret;
 }
+
+/* MODULE_FW_INFO_GET */
+
+const struct nla_policy ethnl_module_fw_info_get_policy[ETHTOOL_A_MODULE_FW_INFO_HEADER + 1] = {
+	[ETHTOOL_A_MODULE_FW_INFO_HEADER] =
+		NLA_POLICY_NESTED(ethnl_header_policy),
+};
+
+static int module_get_fw_info(struct net_device *dev,
+			      struct ethtool_module_fw_info *fw_info,
+			      struct netlink_ext_ack *extack)
+{
+	int ret;
+
+	ret = dev->ethtool_ops->get_module_fw_info(dev, fw_info, extack);
+	if (ret < 0)
+		return ret;
+
+	if (!fw_info->type) {
+		NL_SET_ERR_MSG(extack, "Module firmware info type was not set");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int module_fw_info_prepare_data(const struct ethnl_req_info *req_base,
+				       struct ethnl_reply_data *reply_base,
+				       struct genl_info *info)
+{
+	struct netlink_ext_ack *extack = info ? info->extack : NULL;
+	struct net_device *dev = reply_base->dev;
+	struct module_fw_info_reply_data *data;
+	int ret;
+
+	if (!dev->ethtool_ops->get_module_fw_info)
+		return -EOPNOTSUPP;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	data = MODULE_FW_INFO_REPDATA(reply_base);
+	ret = module_get_fw_info(dev, &data->fw_info, extack);
+	if (ret < 0)
+		goto out_complete;
+
+out_complete:
+	ethnl_ops_complete(dev);
+	return ret;
+}
+
+static int
+module_fw_info_reply_size_image(const struct ethtool_module_fw_info_image *image,
+				int name_len)
+{
+	       /* _MODULE_FW_INFO_IMAGE */
+	return nla_total_size(0) +
+	       /* _MODULE_FW_INFO_IMAGE_NAME */
+	       nla_total_size(name_len + 1) +
+	       /* _MODULE_FW_INFO_IMAGE_RUNNING */
+	       nla_total_size(sizeof(u8)) +
+	       /* _MODULE_FW_INFO_IMAGE_COMMITTED */
+	       nla_total_size(sizeof(u8)) +
+	       /* _MODULE_FW_INFO_IMAGE_VALID */
+	       nla_total_size(sizeof(u8)) +
+	       /* _MODULE_FW_INFO_IMAGE_VERSION */
+	       nla_total_size(ETH_MODULE_FW_VER_LEN + 1);
+}
+
+static int
+module_fw_info_reply_size_cmis(const struct ethtool_module_fw_info_cmis *cmis)
+{
+	int len = 0;
+
+	if (cmis->a_present)
+		len += module_fw_info_reply_size_image(&cmis->a, strlen("a"));
+	if (cmis->b_present)
+		len += module_fw_info_reply_size_image(&cmis->b, strlen("b"));
+	if (cmis->factory_present)
+		len += module_fw_info_reply_size_image(&cmis->factory,
+						       strlen("factory"));
+
+	return len;
+}
+
+static int module_fw_info_reply_size(const struct ethnl_req_info *req_base,
+				     const struct ethnl_reply_data *reply_base)
+{
+	struct module_fw_info_reply_data *data;
+
+	data = MODULE_FW_INFO_REPDATA(reply_base);
+
+	switch (data->fw_info.type) {
+	case ETHTOOL_MODULE_FW_INFO_TYPE_CMIS:
+		return module_fw_info_reply_size_cmis(&data->fw_info.cmis);
+	default:
+		/* Module firmware information type was already validated to be
+		 * set in prepare_data() callback.
+		 */
+		WARN_ON(1);
+		return -EINVAL;
+	}
+}
+
+static int
+module_fw_info_fill_reply_image(struct sk_buff *skb,
+				const struct ethtool_module_fw_info_image *image,
+				const char *image_name)
+{
+	char buf[ETH_MODULE_FW_VER_LEN];
+	struct nlattr *nest;
+
+	if (strlen(image->ver_extra_str))
+		snprintf(buf, ETH_MODULE_FW_VER_LEN, "%d.%d.%d-%s",
+			 image->ver_major, image->ver_minor, image->ver_build,
+			 image->ver_extra_str);
+	else
+		snprintf(buf, ETH_MODULE_FW_VER_LEN, "%d.%d.%d",
+			 image->ver_major, image->ver_minor, image->ver_build);
+
+	nest = nla_nest_start(skb, ETHTOOL_A_MODULE_FW_INFO_IMAGE);
+	if (!nest)
+		return -EMSGSIZE;
+
+	if (nla_put_string(skb, ETHTOOL_A_MODULE_FW_INFO_IMAGE_NAME,
+			   image_name) ||
+	    nla_put_u8(skb, ETHTOOL_A_MODULE_FW_INFO_IMAGE_RUNNING,
+		       image->running) ||
+	    nla_put_u8(skb, ETHTOOL_A_MODULE_FW_INFO_IMAGE_COMMITTED,
+		       image->committed) ||
+	    nla_put_u8(skb, ETHTOOL_A_MODULE_FW_INFO_IMAGE_VALID,
+		       image->valid) ||
+	    nla_put_string(skb, ETHTOOL_A_MODULE_FW_INFO_IMAGE_VERSION, buf))
+		goto err_cancel;
+
+	nla_nest_end(skb, nest);
+
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, nest);
+	return -EMSGSIZE;
+}
+
+static int
+module_fw_info_fill_reply_cmis(struct sk_buff *skb,
+			       const struct ethtool_module_fw_info_cmis *cmis)
+{
+	if (cmis->a_present &&
+	    module_fw_info_fill_reply_image(skb, &cmis->a, "a"))
+		return -EMSGSIZE;
+	if (cmis->b_present &&
+	    module_fw_info_fill_reply_image(skb, &cmis->b, "b"))
+		return -EMSGSIZE;
+	if (cmis->factory_present &&
+	    module_fw_info_fill_reply_image(skb, &cmis->factory, "factory"))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int module_fw_info_fill_reply(struct sk_buff *skb,
+				     const struct ethnl_req_info *req_base,
+				     const struct ethnl_reply_data *reply_base)
+{
+	const struct module_fw_info_reply_data *data;
+
+	data = MODULE_FW_INFO_REPDATA(reply_base);
+
+	switch (data->fw_info.type) {
+	case ETHTOOL_MODULE_FW_INFO_TYPE_CMIS:
+		return module_fw_info_fill_reply_cmis(skb, &data->fw_info.cmis);
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
+}
+
+const struct ethnl_request_ops ethnl_module_fw_info_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_MODULE_FW_INFO_GET,
+	.reply_cmd		= ETHTOOL_MSG_MODULE_FW_INFO_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_MODULE_FW_INFO_HEADER,
+	.req_info_size		= sizeof(struct module_fw_info_req_info),
+	.reply_data_size	= sizeof(struct module_fw_info_reply_data),
+
+	.prepare_data		= module_fw_info_prepare_data,
+	.reply_size		= module_fw_info_reply_size,
+	.fill_reply		= module_fw_info_fill_reply,
+};
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 38b44c0291b1..380a38b8535c 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -283,6 +283,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_STATS_GET]		= &ethnl_stats_request_ops,
 	[ETHTOOL_MSG_PHC_VCLOCKS_GET]	= &ethnl_phc_vclocks_request_ops,
 	[ETHTOOL_MSG_MODULE_GET]	= &ethnl_module_request_ops,
+	[ETHTOOL_MSG_MODULE_FW_INFO_GET] = &ethnl_module_fw_info_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1018,6 +1019,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_module_set_policy,
 		.maxattr = ARRAY_SIZE(ethnl_module_set_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_MODULE_FW_INFO_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_module_fw_info_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_module_fw_info_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 490598e5eedd..041ffe8db8cb 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -338,6 +338,7 @@ extern const struct ethnl_request_ops ethnl_module_eeprom_request_ops;
 extern const struct ethnl_request_ops ethnl_stats_request_ops;
 extern const struct ethnl_request_ops ethnl_phc_vclocks_request_ops;
 extern const struct ethnl_request_ops ethnl_module_request_ops;
+extern const struct ethnl_request_ops ethnl_module_fw_info_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];
@@ -376,6 +377,7 @@ extern const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_GROUPS + 1
 extern const struct nla_policy ethnl_phc_vclocks_get_policy[ETHTOOL_A_PHC_VCLOCKS_HEADER + 1];
 extern const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1];
 extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1];
+extern const struct nla_policy ethnl_module_fw_info_get_policy[ETHTOOL_A_MODULE_FW_INFO_HEADER + 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.31.1

Re: [RFC PATCH net-next 1/4] ethtool: Add ability to query transceiver modules' firmware information

[Jakub Kicinski]

On Sat, 27 Nov 2021 19:45:27 +0200 Ido Schimmel wrote:
> + * @committed: Whether the image is run upon resets.

Isn't activation tangential to state of the image?

> + * @valid: Whether the image is runnable and persistently stored completely
> + *	undamaged.

Is it worth distinguishing empty vs broken? How do we let the user know
that the device has downgraded the FW image because of bad flash vs the
device was never updated? Some statistic?

[RFC PATCH net-next 2/4] netdevsim: Implement support for ethtool_ops::get_module_fw_info

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

For RFC purposes only, implement support for
ethtool_ops::get_module_fw_info.

A real implementation is expected to call CMIS common code (WIP) that
can be shared across all MAC drivers.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/netdevsim/ethtool.c | 35 +++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 2b84169bf3a2..690cd12a4245 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -137,6 +137,40 @@ nsim_set_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
 	return 0;
 }
 
+static int nsim_get_module_fw_info(struct net_device *dev,
+				   struct ethtool_module_fw_info *info,
+				   struct netlink_ext_ack *extack)
+{
+	info->type = ETHTOOL_MODULE_FW_INFO_TYPE_CMIS;
+
+	info->cmis.a_present = true;
+	info->cmis.a.running = true;
+	info->cmis.a.committed = true;
+	info->cmis.a.valid = true;
+	info->cmis.a.ver_major = 1;
+	info->cmis.a.ver_minor = 2;
+	info->cmis.a.ver_build = 3;
+	strcpy(info->cmis.a.ver_extra_str, "test");
+
+	info->cmis.b_present = true;
+	info->cmis.b.running = false;
+	info->cmis.b.committed = false;
+	info->cmis.b.valid = true;
+	info->cmis.b.ver_major = 5;
+	info->cmis.b.ver_minor = 6;
+	info->cmis.b.ver_build = 7;
+
+	info->cmis.factory_present = true;
+	info->cmis.factory.running = false;
+	info->cmis.factory.committed = false;
+	info->cmis.factory.valid = true;
+	info->cmis.factory.ver_major = 11;
+	info->cmis.factory.ver_minor = 12;
+	info->cmis.factory.ver_build = 13;
+
+	return 0;
+}
+
 static const struct ethtool_ops nsim_ethtool_ops = {
 	.supported_coalesce_params	= ETHTOOL_COALESCE_ALL_PARAMS,
 	.get_pause_stats	        = nsim_get_pause_stats,
@@ -150,6 +184,7 @@ static const struct ethtool_ops nsim_ethtool_ops = {
 	.set_channels			= nsim_set_channels,
 	.get_fecparam			= nsim_get_fecparam,
 	.set_fecparam			= nsim_set_fecparam,
+	.get_module_fw_info		= nsim_get_module_fw_info,
 };
 
 static void nsim_ethtool_ring_init(struct netdevsim *ns)
-- 
2.31.1

[RFC PATCH net-next 3/4] ethtool: Add ability to flash transceiver modules' firmware

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

CMIS complaint modules such as QSFP-DD might be running a firmware that
can be updated in a vendor-neutral way by exchanging messages between
the host and the module as described in section 7.3.1 of revision 5.0 of
the CMIS standard.

Add a pair of new ethtool messages that allow:

* User space to trigger firmware update of transceiver modules

* The kernel to notify user space about the progress of the process

The user interface is designed to be asynchronous in order to avoid RTNL
being held for too long and to allow several modules to be updated
simultaneously. The interface is designed with CMIS complaint modules in
mind, but kept generic enough to accommodate future use cases, if these
arise.

Example from the succeeding netdevsim implementation:

Trigger the firmware update process:

 # ethtool --flash-module-firmware eth0 file test.img

 Transceiver module firmware flashing started for device eth0

 Transceiver module firmware flashing in progress for device eth0
 Status message: Downloading firmware image
 Progress: 0%

 Transceiver module firmware flashing in progress for device eth0
 Status message: Downloading firmware image
 Progress: 50%

 Transceiver module firmware flashing in progress for device eth0
 Status message: Downloading firmware image
 Progress: 100%

 Transceiver module firmware flashing in progress for device eth0
 Status message: Validating firmware image download

 Transceiver module firmware flashing in progress for device eth0
 Status message: Running firmware image

 Transceiver module firmware flashing completed for device eth0

After testing the new firmware image, commit it so that it is run upon
reset:

 # ethtool --flash-module-firmware eth0 commit on

 Transceiver module firmware flashing started for device eth0

 Transceiver module firmware flashing in progress for device eth0
 Status message: Committing firmware image

 Transceiver module firmware flashing completed for device eth0

The two stages can be combined together:

 # ethtool --flash-module-firmware eth0 file test.img commit on

 Transceiver module firmware flashing started for device eth0

 Transceiver module firmware flashing in progress for device eth0
 Status message: Downloading firmware image
 Progress: 0%

 Transceiver module firmware flashing in progress for device eth0
 Status message: Downloading firmware image
 Progress: 50%

 Transceiver module firmware flashing in progress for device eth0
 Status message: Downloading firmware image
 Progress: 100%

 Transceiver module firmware flashing in progress for device eth0
 Status message: Validating firmware image download

 Transceiver module firmware flashing in progress for device eth0
 Status message: Running firmware image

 Transceiver module firmware flashing in progress for device eth0
 Status message: Committing firmware image

 Transceiver module firmware flashing completed for device eth0

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/ethtool-netlink.rst |  60 +++++++++
 include/linux/ethtool.h                      |  39 ++++++
 include/linux/ethtool_netlink.h              |   9 ++
 include/uapi/linux/ethtool.h                 |  18 +++
 include/uapi/linux/ethtool_netlink.h         |  21 ++++
 net/ethtool/module.c                         | 125 +++++++++++++++++++
 net/ethtool/netlink.c                        |   7 ++
 net/ethtool/netlink.h                        |   2 +
 8 files changed, 281 insertions(+)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 01393de9d759..e845b6576962 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -221,6 +221,7 @@ Userspace to kernel:
   ``ETHTOOL_MSG_MODULE_SET``            set transceiver module parameters
   ``ETHTOOL_MSG_MODULE_GET``            get transceiver module parameters
   ``ETHTOOL_MSG_MODULE_FW_INFO_GET``    get transceiver module firmware info
+  ``ETHTOOL_MSG_MODULE_FW_FLASH_ACT``   flash transceiver module firmware
   ===================================== ====================================
 
 Kernel to userspace:
@@ -262,6 +263,7 @@ Kernel to userspace:
   ``ETHTOOL_MSG_PHC_VCLOCKS_GET_REPLY``    PHC virtual clocks info
   ``ETHTOOL_MSG_MODULE_GET_REPLY``         transceiver module parameters
   ``ETHTOOL_MSG_MODULE_FW_INFO_GET_REPLY`` transceiver module firmware info
+  ``ETHTOOL_MSG_MODULE_FW_FLASH_NTF``      transceiver module flash updates
   ======================================== =================================
 
 ``GET`` requests are sent by userspace applications to retrieve device
@@ -1661,6 +1663,63 @@ For CMIS modules, the above mentioned information can be queried from the
 module using CDB CMD 0100h (Get Firmware Info). See section 9.7.1 in revision
 5.0 of the specification.
 
+MODULE_FW_FLASH_ACT
+===================
+
+Flashes transceiver module firmware.
+
+Request contents:
+
+  =======================================  ======  ===========================
+  ``ETHTOOL_A_MODULE_FW_FLASH_HEADER``     nested  request header
+  ``ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME``  string  firmware image file name
+  ``ETHTOOL_A_MODULE_FW_FLASH_PASS``       u32     transceiver module password
+  ``ETHTOOL_A_MODULE_FW_FLASH_COMMIT``     u8      commit firmware image
+  =======================================  ======  ===========================
+
+The optional ``ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME`` attribute encodes the
+firmware image file name.
+
+The optional ``ETHTOOL_A_MODULE_FW_FLASH_PASS`` attribute encodes a password
+that might be required as part of the transceiver module firmware update
+process.
+
+The optional ``ETHTOOL_A_MODULE_FW_FLASH_COMMIT`` attribute is used to control
+whether the running firmware image is committed. That is, if the image is to be
+run upon reset. When not specified or not set, the specified firmware image
+(mandatory) is downloaded to the transceiver module and run, but not committed.
+This allows user space to make sure only valid images are committed.
+
+The firmware update process can take several minutes to complete. Therefore,
+during the update process notifications are emitted from the kernel to user
+space updating it about the status and progress.
+
+Notification contents:
+
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_FLASH_HEADER``              | nested | reply header   |
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_FLASH_STATUS``              | u8     | status         |
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG``          | string | status message |
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_FLASH_DONE``                | u64    | progress       |
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_FLASH_TOTAL``               | u64    | total          |
+ +---------------------------------------------------+--------+----------------+
+
+The ``ETHTOOL_A_MODULE_FW_FLASH_STATUS`` attribute encodes the current status
+of the firmware update process. Possible values are:
+
+.. kernel-doc:: include/uapi/linux/ethtool.h
+    :identifiers: ethtool_module_fw_flash_status
+
+The ``ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG`` attribute encodes a status message
+string.
+
+The ``ETHTOOL_A_MODULE_FW_FLASH_DONE`` and ``ETHTOOL_A_MODULE_FW_FLASH_TOTAL``
+attributes encode the completed and total amount of work, respectively.
+
 Request translation
 ===================
 
@@ -1763,4 +1822,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_MODULE_GET``
   n/a                                 ``ETHTOOL_MSG_MODULE_SET``
   n/a                                 ``ETHTOOL_MSG_MODULE_FW_INFO_GET``
+  n/a                                 ``ETHTOOL_MSG_MODULE_FW_FLASH_ACT``
   =================================== =====================================
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 43506b119429..25f4ef05abac 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -508,6 +508,37 @@ struct ethtool_module_fw_info {
 	};
 };
 
+/**
+ * struct ethtool_module_fw_flash_params - module firmware flashing parameters
+ * @file_name: Firmware image file name. Can be NULL when committing an
+ *	existing image. That is, not downloading and running a new one.
+ * @pass: Module password. Only valid when @pass_valid is set.
+ * @commit: Whether to commit the currently running firmware or not. If set and
+ *	@file_name is not NULL, the specified firmware image file needs to be
+ *	downloaded, run and committed.
+ * @pass_valid: Whether the module password is valid or not.
+ */
+struct ethtool_module_fw_flash_params {
+	const char *file_name;
+	u32 pass;
+	u8 commit:1,
+	   pass_valid:1;
+};
+
+/**
+ * struct ethtool_module_fw_flash_ntf_params - module firmware flashing notification parameters
+ * @status: Module firmware flashing status.
+ * @status_msg: Module firmware flashing status message.
+ * @done: Amount of work completed of total amount.
+ * @total: Amount of work expected to be done.
+ */
+struct ethtool_module_fw_flash_ntf_params {
+	enum ethtool_module_fw_flash_status status;
+	const char *status_msg;
+	u64 done;
+	u64 total;
+};
+
 /**
  * struct ethtool_ops - optional netdev operations
  * @cap_link_lanes_supported: indicates if the driver supports lanes
@@ -681,6 +712,11 @@ struct ethtool_module_fw_info {
  *	used by the network device.
  * @get_module_fw_info: Get the firmware information of the plug-in module
  *	used by the network device.
+ * @start_fw_flash_module: Start firmware flashing of the plug-in module used
+ *	by the network device. Device drivers are expected to defer the
+ *	operation to avoid holding RTNL for long periods of time and to allow
+ *	multiple modules to be flashed simultaneously. User space can be
+ *	notified about the progress by calling ethnl_module_fw_flash_ntf().
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -820,6 +856,9 @@ struct ethtool_ops {
 	int	(*get_module_fw_info)(struct net_device *dev,
 				      struct ethtool_module_fw_info *info,
 				      struct netlink_ext_ack *extack);
+	int	(*start_fw_flash_module)(struct net_device *dev,
+					 const struct ethtool_module_fw_flash_params *params,
+					 struct netlink_ext_ack *extack);
 };
 
 int ethtool_check_ops(const struct ethtool_ops *ops);
diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
index aba348d58ff6..1052989f881a 100644
--- a/include/linux/ethtool_netlink.h
+++ b/include/linux/ethtool_netlink.h
@@ -29,6 +29,9 @@ int ethnl_cable_test_amplitude(struct phy_device *phydev, u8 pair, s16 mV);
 int ethnl_cable_test_pulse(struct phy_device *phydev, u16 mV);
 int ethnl_cable_test_step(struct phy_device *phydev, u32 first, u32 last,
 			  u32 step);
+void
+ethnl_module_fw_flash_ntf(struct net_device *dev,
+			  const struct ethtool_module_fw_flash_ntf_params *params);
 #else
 static inline int ethnl_cable_test_alloc(struct phy_device *phydev, u8 cmd)
 {
@@ -70,5 +73,11 @@ static inline int ethnl_cable_test_step(struct phy_device *phydev, u32 first,
 {
 	return -EOPNOTSUPP;
 }
+
+static inline void
+ethnl_module_fw_flash_ntf(struct net_device *dev,
+			  const struct ethtool_module_fw_flash_ntf_params *params)
+{
+}
 #endif /* IS_ENABLED(CONFIG_ETHTOOL_NETLINK) */
 #endif /* _LINUX_ETHTOOL_NETLINK_H_ */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 7bc4b8def12c..e23779201f36 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -736,6 +736,24 @@ enum ethtool_module_power_mode {
 	ETHTOOL_MODULE_POWER_MODE_HIGH,
 };
 
+/**
+ * enum ethtool_module_fw_flash_status - plug-in module firmware flashing status
+ * @ETHTOOL_MODULE_FW_FLASH_STATUS_STARTED: The firmware flashing process has
+ *	started.
+ * @ETHTOOL_MODULE_FW_FLASH_STATUS_IN_PROGRESS: The firmware flashing process
+ *	is in progress.
+ * @ETHTOOL_MODULE_FW_FLASH_STATUS_COMPLETED: The firmware flashing process was
+ *	completed successfully.
+ * @ETHTOOL_MODULE_FW_FLASH_STATUS_ERROR: The firmware flashing process was
+ *	stopped due to an error.
+ */
+enum ethtool_module_fw_flash_status {
+	ETHTOOL_MODULE_FW_FLASH_STATUS_STARTED = 1,
+	ETHTOOL_MODULE_FW_FLASH_STATUS_IN_PROGRESS,
+	ETHTOOL_MODULE_FW_FLASH_STATUS_COMPLETED,
+	ETHTOOL_MODULE_FW_FLASH_STATUS_ERROR,
+};
+
 /**
  * struct ethtool_gstrings - string set for data tagging
  * @cmd: Command number = %ETHTOOL_GSTRINGS
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 7f09bfb28a42..e60a44f01c58 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -50,6 +50,7 @@ enum {
 	ETHTOOL_MSG_MODULE_GET,
 	ETHTOOL_MSG_MODULE_SET,
 	ETHTOOL_MSG_MODULE_FW_INFO_GET,
+	ETHTOOL_MSG_MODULE_FW_FLASH_ACT,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -96,6 +97,7 @@ enum {
 	ETHTOOL_MSG_MODULE_GET_REPLY,
 	ETHTOOL_MSG_MODULE_NTF,
 	ETHTOOL_MSG_MODULE_FW_INFO_GET_REPLY,
+	ETHTOOL_MSG_MODULE_FW_FLASH_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -881,6 +883,25 @@ enum {
 		= (__ETHTOOL_A_MODULE_FW_INFO_IMAGE_CNT - 1)
 };
 
+/* MODULE_FW_FLASH */
+
+enum {
+	ETHTOOL_A_MODULE_FW_FLASH_UNSPEC,
+	ETHTOOL_A_MODULE_FW_FLASH_HEADER,		/* nest - _A_HEADER_* */
+	ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME,		/* string */
+	ETHTOOL_A_MODULE_FW_FLASH_PASS,			/* u32 */
+	ETHTOOL_A_MODULE_FW_FLASH_COMMIT,		/* u8 */
+	ETHTOOL_A_MODULE_FW_FLASH_PAD,
+	ETHTOOL_A_MODULE_FW_FLASH_STATUS,		/* u8 */
+	ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG,		/* string */
+	ETHTOOL_A_MODULE_FW_FLASH_DONE,			/* u64 */
+	ETHTOOL_A_MODULE_FW_FLASH_TOTAL,		/* u64 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_MODULE_FW_FLASH_CNT,
+	ETHTOOL_A_MODULE_FW_FLASH_MAX = (__ETHTOOL_A_MODULE_FW_FLASH_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index 7f86c01a0b40..aa93fd7a8188 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -380,3 +380,128 @@ const struct ethnl_request_ops ethnl_module_fw_info_request_ops = {
 	.reply_size		= module_fw_info_reply_size,
 	.fill_reply		= module_fw_info_fill_reply,
 };
+
+/* MODULE_FW_FLASH_ACT */
+
+const struct nla_policy
+ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_COMMIT + 1] = {
+	[ETHTOOL_A_MODULE_FW_FLASH_HEADER] =
+		NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME] = { .type = NLA_NUL_STRING },
+	[ETHTOOL_A_MODULE_FW_FLASH_PASS] = { .type = NLA_U32 },
+	[ETHTOOL_A_MODULE_FW_FLASH_COMMIT] = NLA_POLICY_MAX(NLA_U8, 1),
+};
+
+static int module_flash_fw(struct net_device *dev, struct nlattr **tb,
+			   struct netlink_ext_ack *extack)
+{
+	struct ethtool_module_fw_flash_params params = {};
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (!ops->start_fw_flash_module) {
+		NL_SET_ERR_MSG(extack,
+			       "Flashing module firmware is not supported by this device");
+		return -EOPNOTSUPP;
+	}
+
+	/* Firmware image file name is only optional when committing the
+	 * currently running firmware.
+	 */
+	if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME] &&
+	    (!tb[ETHTOOL_A_MODULE_FW_FLASH_COMMIT] ||
+	     !nla_get_u8(tb[ETHTOOL_A_MODULE_FW_FLASH_COMMIT]))) {
+		NL_SET_ERR_MSG(extack, "Missing firmware file name");
+		return -EINVAL;
+	}
+
+	if (tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME])
+		params.file_name =
+			nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);
+
+	if (tb[ETHTOOL_A_MODULE_FW_FLASH_COMMIT])
+		params.commit =
+			nla_get_u8(tb[ETHTOOL_A_MODULE_FW_FLASH_COMMIT]);
+
+	if (tb[ETHTOOL_A_MODULE_FW_FLASH_PASS]) {
+		params.pass = nla_get_u32(tb[ETHTOOL_A_MODULE_FW_FLASH_PASS]);
+		params.pass_valid = true;
+	}
+
+	return ops->start_fw_flash_module(dev, &params, extack);
+}
+
+int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info)
+{
+	struct ethnl_req_info req_info = {};
+	struct nlattr **tb = info->attrs;
+	struct net_device *dev;
+	int ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info,
+					 tb[ETHTOOL_A_MODULE_FW_FLASH_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+	dev = req_info.dev;
+
+	rtnl_lock();
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	ret = module_flash_fw(dev, tb, info->extack);
+
+	ethnl_ops_complete(dev);
+
+out_rtnl:
+	rtnl_unlock();
+	dev_put(dev);
+	return ret;
+}
+
+void
+ethnl_module_fw_flash_ntf(struct net_device *dev,
+			  const struct ethtool_module_fw_flash_ntf_params *params)
+{
+	struct sk_buff *skb;
+	void *hdr;
+	int ret;
+
+	skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!skb)
+		return;
+
+	hdr = ethnl_bcastmsg_put(skb, ETHTOOL_MSG_MODULE_FW_FLASH_NTF);
+	if (!hdr)
+		goto err_skb;
+
+	ret = ethnl_fill_reply_header(skb, dev,
+				      ETHTOOL_A_MODULE_FW_FLASH_HEADER);
+	if (ret < 0)
+		goto err_skb;
+
+	if (nla_put_u8(skb, ETHTOOL_A_MODULE_FW_FLASH_STATUS, params->status))
+		goto err_skb;
+
+	if (params->status_msg &&
+	    nla_put_string(skb, ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG,
+			   params->status_msg))
+		goto err_skb;
+
+	if (nla_put_u64_64bit(skb, ETHTOOL_A_MODULE_FW_FLASH_DONE, params->done,
+			      ETHTOOL_A_MODULE_FW_FLASH_PAD))
+		goto err_skb;
+
+	if (nla_put_u64_64bit(skb, ETHTOOL_A_MODULE_FW_FLASH_TOTAL,
+			      params->total, ETHTOOL_A_MODULE_FW_FLASH_PAD))
+		goto err_skb;
+
+	genlmsg_end(skb, hdr);
+	ethnl_multicast(skb, dev);
+	return;
+
+err_skb:
+	nlmsg_free(skb);
+}
+EXPORT_SYMBOL_GPL(ethnl_module_fw_flash_ntf);
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 380a38b8535c..2337b576451f 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -1028,6 +1028,13 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_module_fw_info_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_module_fw_info_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_MODULE_FW_FLASH_ACT,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_act_module_fw_flash,
+		.policy	= ethnl_module_fw_flash_act_policy,
+		.maxattr = ARRAY_SIZE(ethnl_module_fw_flash_act_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 041ffe8db8cb..37fe8bd90ec0 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -378,6 +378,7 @@ extern const struct nla_policy ethnl_phc_vclocks_get_policy[ETHTOOL_A_PHC_VCLOCK
 extern const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1];
 extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1];
 extern const struct nla_policy ethnl_module_fw_info_get_policy[ETHTOOL_A_MODULE_FW_INFO_HEADER + 1];
+extern const struct nla_policy ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_COMMIT + 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);
@@ -397,6 +398,7 @@ int ethnl_tunnel_info_start(struct netlink_callback *cb);
 int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_module(struct sk_buff *skb, struct genl_info *info);
+int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info);
 
 extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
-- 
2.31.1

Re: [RFC PATCH net-next 3/4] ethtool: Add ability to flash transceiver modules' firmware

[Andrew Lunn]

On Sat, Nov 27, 2021 at 07:45:29PM +0200, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> CMIS complaint modules such as QSFP-DD might be running a firmware that
> can be updated in a vendor-neutral way by exchanging messages between
> the host and the module as described in section 7.3.1 of revision 5.0 of
> the CMIS standard.
> 
> Add a pair of new ethtool messages that allow:
> 
> * User space to trigger firmware update of transceiver modules
> 
> * The kernel to notify user space about the progress of the process
> 
> The user interface is designed to be asynchronous in order to avoid RTNL
> being held for too long and to allow several modules to be updated
> simultaneously. The interface is designed with CMIS complaint modules in
> mind, but kept generic enough to accommodate future use cases, if these
> arise.
> 
> Example from the succeeding netdevsim implementation:
> 
> Trigger the firmware update process:
> 
>  # ethtool --flash-module-firmware eth0 file test.img

Hi Ido

Does the design allow control over which partition gets upgraded?

It seems like it should be possible to boot into the read only factory
firmware image, making both A and B eligible for upgrade.

	 Andrew

Re: [RFC PATCH net-next 3/4] ethtool: Add ability to flash transceiver modules' firmware

[Andrew Lunn]

On Sat, Nov 27, 2021 at 07:45:29PM +0200, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> CMIS complaint modules such as QSFP-DD might be running a firmware that
> can be updated in a vendor-neutral way by exchanging messages between
> the host and the module as described in section 7.3.1 of revision 5.0 of
> the CMIS standard.
> 
> Add a pair of new ethtool messages that allow:
> 
> * User space to trigger firmware update of transceiver modules
> 
> * The kernel to notify user space about the progress of the process
> 
> The user interface is designed to be asynchronous in order to avoid RTNL
> being held for too long and to allow several modules to be updated
> simultaneously. The interface is designed with CMIS complaint modules in
> mind, but kept generic enough to accommodate future use cases, if these
> arise.

Hi Ido

What i'm missing is some sort of state machine to keep track of the
SFP. Since RTNL is not held other operations could be performed in
parallel. Does CMIS allow this? Can you intermix firmware writes with
reading the temperature sensor for hwmon? Poll the LOS indicator to
see if the link has been lost?

With cable testing, phylib already has a state machine, and i added a
new state for cable test running. If any other operation happened
which would cause a change out of this state, like ifdown, or a
request to restart autoneg, the cable test is aborted first.

    Andrew

Re: [RFC PATCH net-next 3/4] ethtool: Add ability to flash transceiver modules' firmware

[Jakub Kicinski]

On Tue, 30 Nov 2021 01:05:08 +0100 Andrew Lunn wrote:
> What i'm missing is some sort of state machine to keep track of the
> SFP. Since RTNL is not held other operations could be performed in
> parallel. Does CMIS allow this? Can you intermix firmware writes with
> reading the temperature sensor for hwmon? Poll the LOS indicator to
> see if the link has been lost?

Ah, rtnl_lock is not held throughout? I just looked at this code:

+	rtnl_lock();
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	ret = module_flash_fw(dev, tb, info->extack);
+
+	ethnl_ops_complete(dev);
+
+out_rtnl:
+	rtnl_unlock();

and assumed module_flash_fw() flashes the module's FW, not starts 
an async process...

And it appears the user is racy:

+	dev_put(ns->netdev);
+	rtnl_lock();
+	ns->ethtool.module_fw.in_progress = false;
+	rtnl_unlock();
+	release_firmware(ns->ethtool.module_fw.fw);

The dev_put() should be last, otherwise references to ns could be UAF?

> With cable testing, phylib already has a state machine, and i added a
> new state for cable test running. If any other operation happened
> which would cause a change out of this state, like ifdown, or a
> request to restart autoneg, the cable test is aborted first.

[RFC PATCH net-next 4/4] netdevsim: Implement support for ethtool_ops::start_fw_flash_module

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

For RFC purposes only, implement support for
ethtool_ops::start_fw_flash_module.

A real implementation is expected to call CMIS common code (WIP) that
can be shared across all MAC drivers.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/netdevsim/ethtool.c   | 129 ++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdevsim.h |  10 +++
 2 files changed, 139 insertions(+)

diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 690cd12a4245..f126b03bf5b0 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -3,6 +3,8 @@
 
 #include <linux/debugfs.h>
 #include <linux/ethtool.h>
+#include <linux/ethtool_netlink.h>
+#include <linux/firmware.h>
 #include <linux/random.h>
 
 #include "netdevsim.h"
@@ -171,6 +173,127 @@ static int nsim_get_module_fw_info(struct net_device *dev,
 	return 0;
 }
 
+static void nsim_module_fw_flash_download(struct netdevsim *ns)
+{
+	struct ethtool_module_fw_flash_ntf_params params = {};
+
+	params.status = ETHTOOL_MODULE_FW_FLASH_STATUS_IN_PROGRESS;
+	params.status_msg = "Downloading firmware image";
+	params.done = 0;
+	params.total = 1500;
+	ethnl_module_fw_flash_ntf(ns->netdev, &params);
+
+	msleep(5000);
+
+	params.done = 750;
+	ethnl_module_fw_flash_ntf(ns->netdev, &params);
+
+	msleep(5000);
+
+	params.done = 1500;
+	ethnl_module_fw_flash_ntf(ns->netdev, &params);
+
+	msleep(5000);
+}
+
+static void nsim_module_fw_flash_validate(struct netdevsim *ns)
+{
+	struct ethtool_module_fw_flash_ntf_params params = {};
+
+	params.status = ETHTOOL_MODULE_FW_FLASH_STATUS_IN_PROGRESS;
+	params.status_msg = "Validating firmware image download";
+	ethnl_module_fw_flash_ntf(ns->netdev, &params);
+
+	msleep(5000);
+}
+
+static void nsim_module_fw_flash_run(struct netdevsim *ns)
+{
+	struct ethtool_module_fw_flash_ntf_params params = {};
+
+	params.status = ETHTOOL_MODULE_FW_FLASH_STATUS_IN_PROGRESS;
+	params.status_msg = "Running firmware image";
+	ethnl_module_fw_flash_ntf(ns->netdev, &params);
+
+	msleep(5000);
+}
+
+static void nsim_module_fw_flash_commit(struct netdevsim *ns)
+{
+	struct ethtool_module_fw_flash_ntf_params params = {};
+
+	if (!ns->ethtool.module_fw.params.commit)
+		return;
+
+	params.status = ETHTOOL_MODULE_FW_FLASH_STATUS_IN_PROGRESS;
+	params.status_msg = "Committing firmware image";
+	ethnl_module_fw_flash_ntf(ns->netdev, &params);
+
+	msleep(5000);
+}
+
+static void nsim_module_fw_flash(struct work_struct *work)
+{
+	struct ethtool_module_fw_flash_ntf_params params = {};
+	struct netdevsim *ns;
+
+	ns = container_of(work, struct netdevsim, ethtool.module_fw.work);
+
+	params.status = ETHTOOL_MODULE_FW_FLASH_STATUS_STARTED;
+	ethnl_module_fw_flash_ntf(ns->netdev, &params);
+
+	if (!ns->ethtool.module_fw.fw)
+		goto commit;
+
+	nsim_module_fw_flash_download(ns);
+	nsim_module_fw_flash_validate(ns);
+	nsim_module_fw_flash_run(ns);
+commit:
+	nsim_module_fw_flash_commit(ns);
+
+	params.status = ETHTOOL_MODULE_FW_FLASH_STATUS_COMPLETED;
+	ethnl_module_fw_flash_ntf(ns->netdev, &params);
+
+	dev_put(ns->netdev);
+	rtnl_lock();
+	ns->ethtool.module_fw.in_progress = false;
+	rtnl_unlock();
+	release_firmware(ns->ethtool.module_fw.fw);
+}
+
+static int
+nsim_start_fw_flash_module(struct net_device *dev,
+			   const struct ethtool_module_fw_flash_params *params,
+			   struct netlink_ext_ack *extack)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (ns->ethtool.module_fw.in_progress) {
+		NL_SET_ERR_MSG(extack, "Module firmware flashing already in progress");
+		return -EBUSY;
+	}
+
+	ns->ethtool.module_fw.fw = NULL;
+	if (params->file_name) {
+		int err;
+
+		err = request_firmware(&ns->ethtool.module_fw.fw,
+				       params->file_name, &dev->dev);
+		if (err) {
+			NL_SET_ERR_MSG(extack,
+				       "Failed to request module firmware image");
+			return err;
+		}
+	}
+
+	ns->ethtool.module_fw.in_progress = true;
+	dev_hold(dev);
+	ns->ethtool.module_fw.params = *params;
+	schedule_work(&ns->ethtool.module_fw.work);
+
+	return 0;
+}
+
 static const struct ethtool_ops nsim_ethtool_ops = {
 	.supported_coalesce_params	= ETHTOOL_COALESCE_ALL_PARAMS,
 	.get_pause_stats	        = nsim_get_pause_stats,
@@ -185,6 +308,7 @@ static const struct ethtool_ops nsim_ethtool_ops = {
 	.get_fecparam			= nsim_get_fecparam,
 	.set_fecparam			= nsim_set_fecparam,
 	.get_module_fw_info		= nsim_get_module_fw_info,
+	.start_fw_flash_module		= nsim_start_fw_flash_module,
 };
 
 static void nsim_ethtool_ring_init(struct netdevsim *ns)
@@ -228,4 +352,9 @@ void nsim_ethtool_init(struct netdevsim *ns)
 			   &ns->ethtool.ring.rx_mini_max_pending);
 	debugfs_create_u32("tx_max_pending", 0600, dir,
 			   &ns->ethtool.ring.tx_max_pending);
+
+	/* The work item holds a reference on the netdev, so its unregistration
+	 * cannot be completed while the work is queued or executing.
+	 */
+	INIT_WORK(&ns->ethtool.module_fw.work, nsim_module_fw_flash);
 }
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index c49771f27f17..afa8f9c7f22c 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -16,6 +16,7 @@
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/ethtool.h>
+#include <linux/firmware.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
@@ -59,14 +60,23 @@ struct nsim_ethtool_pauseparam {
 	bool report_stats_tx;
 };
 
+struct nsim_ethtool_module_fw {
+	struct ethtool_module_fw_flash_params params;
+	struct work_struct work;
+	const struct firmware *fw;
+	bool in_progress;
+};
+
 struct nsim_ethtool {
 	u32 get_err;
 	u32 set_err;
 	u32 channels;
 	struct nsim_ethtool_pauseparam pauseparam;
+	struct nsim_ethtool_module_fw module_fw;
 	struct ethtool_coalesce coalesce;
 	struct ethtool_ringparam ring;
 	struct ethtool_fecparam fec;
+
 };
 
 struct netdevsim {
-- 
2.31.1

Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Jakub Kicinski]

On Sat, 27 Nov 2021 19:45:26 +0200 Ido Schimmel wrote:
> This patchset extends the ethtool netlink API to allow user space to
> both flash transceiver modules' firmware and query the firmware
> information (e.g., version, state).
> 
> The main use case is CMIS compliant modules such as QSFP-DD. The CMIS
> standard specifies the interfaces used for both operations. See section
> 7.3.1 in revision 5.0 of the standard [1].
> 
> Despite the immediate use case being CMIS compliant modules, the user
> interface is kept generic enough to accommodate future use cases, if
> these arise.
> 
> The purpose of this RFC is to solicit feedback on both the proposed user
> interface and the device driver API which are described in detail in
> patches #1 and #3. The netdevsim patches are for RFC purposes only. The
> plan is to implement the CMIS functionality in common code (under lib/)
> so that it can be shared by MAC drivers that will pass function pointers
> to it in order to read and write from their modules EEPROM.
> 
> ethtool(8) patches can be found here [2].

Immediate question I have is why not devlink. We purposefully moved 
FW flashing to devlink because I may take long, so doing it under
rtnl_lock is really bad. Other advantages exist (like flashing
non-Ethernet ports). Ethtool netlink already existed at the time.

I think device flashing may also benefit from the infra you're adding.

Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Michal Kubecek]

On Mon, Nov 29, 2021 at 09:37:24AM -0800, Jakub Kicinski wrote:
> On Sat, 27 Nov 2021 19:45:26 +0200 Ido Schimmel wrote:
> > This patchset extends the ethtool netlink API to allow user space to
> > both flash transceiver modules' firmware and query the firmware
> > information (e.g., version, state).
> > 
> > The main use case is CMIS compliant modules such as QSFP-DD. The CMIS
> > standard specifies the interfaces used for both operations. See section
> > 7.3.1 in revision 5.0 of the standard [1].
> > 
> > Despite the immediate use case being CMIS compliant modules, the user
> > interface is kept generic enough to accommodate future use cases, if
> > these arise.
> > 
> > The purpose of this RFC is to solicit feedback on both the proposed user
> > interface and the device driver API which are described in detail in
> > patches #1 and #3. The netdevsim patches are for RFC purposes only. The
> > plan is to implement the CMIS functionality in common code (under lib/)
> > so that it can be shared by MAC drivers that will pass function pointers
> > to it in order to read and write from their modules EEPROM.
> > 
> > ethtool(8) patches can be found here [2].
> 
> Immediate question I have is why not devlink. We purposefully moved 
> FW flashing to devlink because I may take long, so doing it under
> rtnl_lock is really bad. Other advantages exist (like flashing
> non-Ethernet ports). Ethtool netlink already existed at the time.

Note that ethtool (as userspace utility) can still provide the
functionality even if it's implemented in devlink API; this is likely
going to be the case for device EEPROM flash (ethtool -f). At the
moment, there is a problem that not nearly every device capable of
flashing implements devlink but that could be addressed by the "generic
devlink" idea (a wrapper implementing selected parts of devlink API for
devices without an actual devlink implementation).

Michal

Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Andrew Lunn]

On Mon, Nov 29, 2021 at 09:37:24AM -0800, Jakub Kicinski wrote:
> On Sat, 27 Nov 2021 19:45:26 +0200 Ido Schimmel wrote:
> > This patchset extends the ethtool netlink API to allow user space to
> > both flash transceiver modules' firmware and query the firmware
> > information (e.g., version, state).
> > 
> > The main use case is CMIS compliant modules such as QSFP-DD. The CMIS
> > standard specifies the interfaces used for both operations. See section
> > 7.3.1 in revision 5.0 of the standard [1].
> > 
> > Despite the immediate use case being CMIS compliant modules, the user
> > interface is kept generic enough to accommodate future use cases, if
> > these arise.
> > 
> > The purpose of this RFC is to solicit feedback on both the proposed user
> > interface and the device driver API which are described in detail in
> > patches #1 and #3. The netdevsim patches are for RFC purposes only. The
> > plan is to implement the CMIS functionality in common code (under lib/)
> > so that it can be shared by MAC drivers that will pass function pointers
> > to it in order to read and write from their modules EEPROM.
> > 
> > ethtool(8) patches can be found here [2].
> 
> Immediate question I have is why not devlink. We purposefully moved 
> FW flashing to devlink because I may take long, so doing it under
> rtnl_lock is really bad. Other advantages exist (like flashing
> non-Ethernet ports). Ethtool netlink already existed at the time.
> 
> I think device flashing may also benefit from the infra you're adding.

The idea of asynchronous operations without holding RTNL is not that
new. The cable test code does it, but clearly cable testing is likely
network specific, unlike FW flashing.

	Andrew

Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Jakub Kicinski]

On Tue, 30 Nov 2021 00:50:41 +0100 Andrew Lunn wrote:
> On Mon, Nov 29, 2021 at 09:37:24AM -0800, Jakub Kicinski wrote:
> > On Sat, 27 Nov 2021 19:45:26 +0200 Ido Schimmel wrote:  
> > > This patchset extends the ethtool netlink API to allow user space to
> > > both flash transceiver modules' firmware and query the firmware
> > > information (e.g., version, state).
> > > 
> > > The main use case is CMIS compliant modules such as QSFP-DD. The CMIS
> > > standard specifies the interfaces used for both operations. See section
> > > 7.3.1 in revision 5.0 of the standard [1].
> > > 
> > > Despite the immediate use case being CMIS compliant modules, the user
> > > interface is kept generic enough to accommodate future use cases, if
> > > these arise.
> > > 
> > > The purpose of this RFC is to solicit feedback on both the proposed user
> > > interface and the device driver API which are described in detail in
> > > patches #1 and #3. The netdevsim patches are for RFC purposes only. The
> > > plan is to implement the CMIS functionality in common code (under lib/)
> > > so that it can be shared by MAC drivers that will pass function pointers
> > > to it in order to read and write from their modules EEPROM.
> > > 
> > > ethtool(8) patches can be found here [2].  
> > 
> > Immediate question I have is why not devlink. We purposefully moved 
> > FW flashing to devlink because I may take long, so doing it under
> > rtnl_lock is really bad. Other advantages exist (like flashing
> > non-Ethernet ports). Ethtool netlink already existed at the time.
> > 
> > I think device flashing may also benefit from the infra you're adding.  
> 
> The idea of asynchronous operations without holding RTNL is not that
> new. The cable test code does it, but clearly cable testing is likely
> network specific, unlike FW flashing.

Right, I missed this is async. Presumably since there is a plan for 
a common module the chance of bugs sneaking in will be somewhat lower,
but still flashing FW is flashing FW, would be great if we could align
with device FW flashing as done via devlink.

RE: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, November 29, 2021 9:37 AM
> To: Ido Schimmel <idosch@idosch.org>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; andrew@lunn.ch;
> mkubecek@suse.cz; pali@kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>;
> vadimp@nvidia.com; mlxsw@nvidia.com; Ido Schimmel <idosch@nvidia.com>
> Subject: Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query
> transceiver modules' firmware
> 
> On Sat, 27 Nov 2021 19:45:26 +0200 Ido Schimmel wrote:
> > This patchset extends the ethtool netlink API to allow user space to
> > both flash transceiver modules' firmware and query the firmware
> > information (e.g., version, state).
> >
> > The main use case is CMIS compliant modules such as QSFP-DD. The CMIS
> > standard specifies the interfaces used for both operations. See section
> > 7.3.1 in revision 5.0 of the standard [1].
> >
> > Despite the immediate use case being CMIS compliant modules, the user
> > interface is kept generic enough to accommodate future use cases, if
> > these arise.
> >
> > The purpose of this RFC is to solicit feedback on both the proposed user
> > interface and the device driver API which are described in detail in
> > patches #1 and #3. The netdevsim patches are for RFC purposes only. The
> > plan is to implement the CMIS functionality in common code (under lib/)
> > so that it can be shared by MAC drivers that will pass function pointers
> > to it in order to read and write from their modules EEPROM.
> >
> > ethtool(8) patches can be found here [2].
> 
> Immediate question I have is why not devlink. We purposefully moved
> FW flashing to devlink because I may take long, so doing it under
> rtnl_lock is really bad. Other advantages exist (like flashing
> non-Ethernet ports). Ethtool netlink already existed at the time.
> 
> I think device flashing may also benefit from the infra you're adding.

I also immediately thought of devlink here.

Thanks,
Jake

Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Ido Schimmel]

On Mon, Nov 29, 2021 at 09:37:24AM -0800, Jakub Kicinski wrote:
> On Sat, 27 Nov 2021 19:45:26 +0200 Ido Schimmel wrote:
> > This patchset extends the ethtool netlink API to allow user space to
> > both flash transceiver modules' firmware and query the firmware
> > information (e.g., version, state).
> > 
> > The main use case is CMIS compliant modules such as QSFP-DD. The CMIS
> > standard specifies the interfaces used for both operations. See section
> > 7.3.1 in revision 5.0 of the standard [1].
> > 
> > Despite the immediate use case being CMIS compliant modules, the user
> > interface is kept generic enough to accommodate future use cases, if
> > these arise.
> > 
> > The purpose of this RFC is to solicit feedback on both the proposed user
> > interface and the device driver API which are described in detail in
> > patches #1 and #3. The netdevsim patches are for RFC purposes only. The
> > plan is to implement the CMIS functionality in common code (under lib/)
> > so that it can be shared by MAC drivers that will pass function pointers
> > to it in order to read and write from their modules EEPROM.
> > 
> > ethtool(8) patches can be found here [2].
> 
> Immediate question I have is why not devlink. We purposefully moved 
> FW flashing to devlink because I may take long, so doing it under
> rtnl_lock is really bad. Other advantages exist (like flashing
> non-Ethernet ports). Ethtool netlink already existed at the time.

Device firmware flashing doesn't belong in devlink because of locking
semantics. It belongs in devlink because you are updating the firmware
of the device, which can instantiate multiple netdevs. For multi-port
devices, it always seemed weird to tell users "choose some random port
and use it for 'ethtool -f'". I remember being asked if the command
needs to be run for all swp* netdevs (no).

On the other hand, each netdev corresponds to a single transceiver
module and each transceiver module corresponds to a single netdev
(modulo split which is a user configuration).

In addition, users are already dumping the EEPROM contents of
transceiver modules via ethtool and also toggling their settings.

Given the above, it's beyond me why we should tell users to use anything
other than ethtool to update transceiver modules' firmware.

Also, note that an important difference from the devlink implementation
is that this mechanism is asynchronous, which allows the firmware on
multiple modules to be updated simultaneously.

Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Michal Kubecek]

On Tue, Nov 30, 2021 at 10:36:02AM +0200, Ido Schimmel wrote:
> On Mon, Nov 29, 2021 at 09:37:24AM -0800, Jakub Kicinski wrote:
> > 
> > Immediate question I have is why not devlink. We purposefully moved 
> > FW flashing to devlink because I may take long, so doing it under
> > rtnl_lock is really bad. Other advantages exist (like flashing
> > non-Ethernet ports). Ethtool netlink already existed at the time.
> 
> Device firmware flashing doesn't belong in devlink because of locking
> semantics. It belongs in devlink because you are updating the firmware
> of the device, which can instantiate multiple netdevs. For multi-port
> devices, it always seemed weird to tell users "choose some random port
> and use it for 'ethtool -f'". I remember being asked if the command
> needs to be run for all swp* netdevs (no).
> 
> On the other hand, each netdev corresponds to a single transceiver
> module and each transceiver module corresponds to a single netdev
> (modulo split which is a user configuration).

Devlink also has abstraction for ports so it can be used so it is not
necessarily a problem.

> In addition, users are already dumping the EEPROM contents of
> transceiver modules via ethtool and also toggling their settings.
> 
> Given the above, it's beyond me why we should tell users to use anything
> other than ethtool to update transceiver modules' firmware.

As I already mentioned, we should distinguish between ethtool API and
ethtool utility. It is possible to implement the flashing in devlink API
and let both devlink and ethtool utilities use that API.

I'm not saying ethtool API is a wrong choice, IMHO either option has its
pros and cons. I'm just trying to point out that implementation in
devlink API does not necessarily mean one cannot use the ethtool to use
the feature.

Michal

Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Ido Schimmel]

On Tue, Nov 30, 2021 at 09:54:26AM +0100, Michal Kubecek wrote:
> On Tue, Nov 30, 2021 at 10:36:02AM +0200, Ido Schimmel wrote:
> > On Mon, Nov 29, 2021 at 09:37:24AM -0800, Jakub Kicinski wrote:
> > > 
> > > Immediate question I have is why not devlink. We purposefully moved 
> > > FW flashing to devlink because I may take long, so doing it under
> > > rtnl_lock is really bad. Other advantages exist (like flashing
> > > non-Ethernet ports). Ethtool netlink already existed at the time.
> > 
> > Device firmware flashing doesn't belong in devlink because of locking
> > semantics. It belongs in devlink because you are updating the firmware
> > of the device, which can instantiate multiple netdevs. For multi-port
> > devices, it always seemed weird to tell users "choose some random port
> > and use it for 'ethtool -f'". I remember being asked if the command
> > needs to be run for all swp* netdevs (no).
> > 
> > On the other hand, each netdev corresponds to a single transceiver
> > module and each transceiver module corresponds to a single netdev
> > (modulo split which is a user configuration).
> 
> Devlink also has abstraction for ports so it can be used so it is not
> necessarily a problem.
> 
> > In addition, users are already dumping the EEPROM contents of
> > transceiver modules via ethtool and also toggling their settings.
> > 
> > Given the above, it's beyond me why we should tell users to use anything
> > other than ethtool to update transceiver modules' firmware.
> 
> As I already mentioned, we should distinguish between ethtool API and
> ethtool utility. It is possible to implement the flashing in devlink API
> and let both devlink and ethtool utilities use that API.
> 
> I'm not saying ethtool API is a wrong choice, IMHO either option has its
> pros and cons.

What are the cons of implementing it in ethtool? It seems that the only
thing devlink has going for it is the fact that it supports devlink
device firmware update API, but it cannot be used as-is and needs to be
heavily extended (e.g., asynchronicity is a must, per-port as opposed to
per-device). It doesn't support any transceiver module API, as opposed
to ethtool.

> I'm just trying to point out that implementation in devlink API does
> not necessarily mean one cannot use the ethtool to use the feature.

I agree it can be done, but the fact that something can be done doesn't
mean it should be done. If I'm extending devlink with new uAPI, then I
will add support for it in devlink(8) and not ethtool(8) and vice versa.

Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Jakub Kicinski]

On Tue, 30 Nov 2021 11:46:48 +0200 Ido Schimmel wrote:
> > As I already mentioned, we should distinguish between ethtool API and
> > ethtool utility. It is possible to implement the flashing in devlink API
> > and let both devlink and ethtool utilities use that API.
> > 
> > I'm not saying ethtool API is a wrong choice, IMHO either option has its
> > pros and cons.  
> 
> What are the cons of implementing it in ethtool? It seems that the only
> thing devlink has going for it is the fact that it supports devlink
> device firmware update API, but it cannot be used as-is and needs to be
> heavily extended (e.g., asynchronicity is a must, per-port as opposed to
> per-device). It doesn't support any transceiver module API, as opposed
> to ethtool.

The primary advantage is that we could hopefully share some of the
infrastructure around versioning, A/B image selection, activation and
error reporting. All those are universal firmware update problems.

> > I'm just trying to point out that implementation in devlink API does
> > not necessarily mean one cannot use the ethtool to use the feature.  
> 
> I agree it can be done, but the fact that something can be done doesn't
> mean it should be done. If I'm extending devlink with new uAPI, then I
> will add support for it in devlink(8) and not ethtool(8) and vice versa.

I'm not dead set on SFP flashing being in devlink, I just think it's
the right choice, but at the end of the day - your call.

From my experience working with and on FW management in production
(using devlink) I don't think that the "rest of the SFP API is in
ethtool" motivation matters in practice. At least not in my
environment. Upgrading firmware is a process that's more concerned with
different device components than the functionality those devices
actually provide. For a person writing FW update automation its better
if they have one type of API to talk to. IOW nobody cares if e.g. the FW
upgrade on a soundcard is via the sound API.

When automation gets more complex (again versioning, checking if there
is degradation and FW has to be re-applied, checking if upgrades can be
live, or device has to be reset, power cycled, etc) plugging into a
consistent API is what matters most.