[RFC PATCH net-next 0/9] Add ability to flash modules' firmware

[RFC PATCH net-next 0/9] Add ability to flash modules' firmware

[Danielle Ratson]

CMIS compliant 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.2.2 of revision
4.0 of the CMIS standard.

According to the CMIS standard, the firmware update process is done
using a CDB commands sequence.

CDB (Command Data Block Message Communication) reads and writes are
performed on memory map pages 9Fh-AFh according to the CMIS standard,
section 8.12 of revision 4.0.

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 compliant modules in
mind, but kept generic enough to accommodate future use cases, if these
arise.

The kernel interface that will implement the firmware update using CDB
command will include 2 layers that will be added under ethtool:

* The upper layer that will be triggered from the module layer, is
 cmis_ fw_update.
* The lower one is cmis_cdb.

In the future there might be more operations to implement using CDB
commands. Therefore, the idea is to keep the cmis_cdb interface clean and
the cmis_fw_update specific to the cdb commands handling it.

The communication between the kernel and the driver will be done using
two ethtool operations that enable reading and writing the transceiver
module EEPROM.
The operation ethtool_ops::get_module_eeprom_by_page, that is already
implemented, will be used for reading from the EEPROM the CDB reply,
e.g. reading module setting, state, etc.
The operation ethtool_ops::set_module_eeprom_by_page, that is added in
the current patchset, will be used for writing to the EEPROM the CDB
command such as start firmware image, run firmware image, etc.

Therefore in order for a driver to implement module flashing, that
driver needs to implement the two functions mentioned above.

Patchset overview:
Patch #1-#2: Implement the EEPROM writing in mlxsw.
Patch #3: Define the interface between the kernel and user space.
Patch #4: Add ability to notify the flashing firmware progress.
Patch #5: Add firmware flashing in progress flag.
Patch #6: Add extended compliance codes.
Patch #7: Add the cdb layer.
Patch #8: Add the fw_update layer.
Patch #9: Add ability to flash transceiver modules' firmware.

Danielle Ratson (7):
  ethtool: Add an interface for flashing transceiver modules' firmware
  ethtool: Add flashing transceiver modules' firmware notifications
    ability
  include: netdevice: Add module firmware flashing in progress flag to
    net_device
  net: sfp: Add more extended compliance codes
  ethtool: cmis_cdb: Add a layer for supporting CDB commands
  ethtool: cmis_fw_update: add a layer for supporting firmware update
    using CDB
  ethtool: Add ability to flash transceiver modules' firmware

Ido Schimmel (2):
  ethtool: Add ethtool operation to write to a transceiver module EEPROM
  mlxsw: Implement ethtool operation to write to a transceiver module
    EEPROM

 Documentation/netlink/specs/ethtool.yaml      |  57 ++
 Documentation/networking/ethtool-netlink.rst  |  62 ++
 .../net/ethernet/mellanox/mlxsw/core_env.c    |  57 ++
 .../net/ethernet/mellanox/mlxsw/core_env.h    |   6 +
 drivers/net/ethernet/mellanox/mlxsw/minimal.c |  15 +
 .../mellanox/mlxsw/spectrum_ethtool.c         |  15 +
 include/linux/ethtool.h                       |  18 +-
 include/linux/netdevice.h                     |   4 +-
 include/linux/sfp.h                           |   6 +
 include/uapi/linux/ethtool.h                  |  18 +
 include/uapi/linux/ethtool_netlink.h          |  20 +
 net/ethtool/Makefile                          |   2 +-
 net/ethtool/cmis.h                            | 119 ++++
 net/ethtool/cmis_cdb.c                        | 563 ++++++++++++++++++
 net/ethtool/cmis_fw_update.c                  | 371 ++++++++++++
 net/ethtool/module.c                          | 255 ++++++++
 net/ethtool/module_fw.h                       |  40 ++
 net/ethtool/netlink.c                         |   7 +
 net/ethtool/netlink.h                         |   2 +
 19 files changed, 1627 insertions(+), 10 deletions(-)
 create mode 100644 net/ethtool/cmis.h
 create mode 100644 net/ethtool/cmis_cdb.c
 create mode 100644 net/ethtool/cmis_fw_update.c
 create mode 100644 net/ethtool/module_fw.h

-- 
2.40.1

[RFC PATCH net-next 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM

[Danielle Ratson]

From: Ido Schimmel <idosch@nvidia.com>

Ethtool can already retrieve information from a transceiver module
EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page operation.
Add a corresponding operation that allows ethtool to write to a
transceiver module EEPROM.

The purpose of this operation is not to enable arbitrary read / write
access, but to allow the kernel to write to specific addresses as part
of transceiver module firmware flashing. In the future, more
functionality can be implemented on top of these read / write
operations.

Adjust the comments of the 'ethtool_module_eeprom' structure as it is
no longer used only for read access.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
 include/linux/ethtool.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 325e0778e937..bb253d90352b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -474,17 +474,14 @@ struct ethtool_rmon_stats {
 #define ETH_MODULE_MAX_I2C_ADDRESS	0x7f
 
 /**
- * struct ethtool_module_eeprom - 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.
+ * struct ethtool_module_eeprom - plug-in module EEPROM read / write parameters
+ * @offset: Offset within the specified page, in bytes.
+ * @length: Number of bytes to read / write.
+ * @page: Page number.
+ * @bank: Bank number, if supported 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;
@@ -789,6 +786,8 @@ struct ethtool_rxfh_param {
  * @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.
+ * @set_module_eeprom_by_page: Write to a region of plug-in module EEPROM.
+ *	Returns a negative error code or zero.
  * @get_eth_phy_stats: Query some of the IEEE 802.3 PHY statistics.
  * @get_eth_mac_stats: Query some of the IEEE 802.3 MAC statistics.
  * @get_eth_ctrl_stats: Query some of the IEEE 802.3 MAC Ctrl statistics.
@@ -921,6 +920,9 @@ struct ethtool_ops {
 	int	(*get_module_eeprom_by_page)(struct net_device *dev,
 					     const struct ethtool_module_eeprom *page,
 					     struct netlink_ext_ack *extack);
+	int	(*set_module_eeprom_by_page)(struct net_device *dev,
+					     const struct ethtool_module_eeprom *page,
+					     struct netlink_ext_ack *extack);
 	void	(*get_eth_phy_stats)(struct net_device *dev,
 				     struct ethtool_eth_phy_stats *phy_stats);
 	void	(*get_eth_mac_stats)(struct net_device *dev,
-- 
2.40.1

[RFC PATCH net-next 2/9] mlxsw: Implement ethtool operation to write to a transceiver module EEPROM

[Danielle Ratson]

From: Ido Schimmel <idosch@nvidia.com>

Implement the ethtool_ops::set_module_eeprom_by_page operation to allow
ethtool to write to a transceiver module EEPROM, in a similar fashion to
the ethtool_ops::get_module_eeprom_by_page operation.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
 .../net/ethernet/mellanox/mlxsw/core_env.c    | 57 +++++++++++++++++++
 .../net/ethernet/mellanox/mlxsw/core_env.h    |  6 ++
 drivers/net/ethernet/mellanox/mlxsw/minimal.c | 15 +++++
 .../mellanox/mlxsw/spectrum_ethtool.c         | 15 +++++
 4 files changed, 93 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_env.c b/drivers/net/ethernet/mellanox/mlxsw/core_env.c
index 53b150b7ae4e..79e4c745ac3b 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_env.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_env.c
@@ -513,6 +513,63 @@ mlxsw_env_get_module_eeprom_by_page(struct mlxsw_core *mlxsw_core,
 }
 EXPORT_SYMBOL(mlxsw_env_get_module_eeprom_by_page);
 
+int
+mlxsw_env_set_module_eeprom_by_page(struct mlxsw_core *mlxsw_core,
+				    u8 slot_index, u8 module,
+				    const struct ethtool_module_eeprom *page,
+				    struct netlink_ext_ack *extack)
+{
+	struct mlxsw_env *mlxsw_env = mlxsw_core_env(mlxsw_core);
+	u32 bytes_written = 0;
+	u16 device_addr;
+	int err;
+
+	if (!mlxsw_env_linecard_is_active(mlxsw_env, slot_index)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Cannot write to EEPROM of a module on an inactive line card");
+		return -EIO;
+	}
+
+	err = mlxsw_env_validate_module_type(mlxsw_core, slot_index, module);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "EEPROM is not equipped on port module type");
+		return err;
+	}
+
+	device_addr = page->offset;
+
+	while (bytes_written < page->length) {
+		char mcia_pl[MLXSW_REG_MCIA_LEN];
+		char eeprom_tmp[128] = {};
+		u8 size;
+
+		size = min_t(u8, page->length - bytes_written,
+			     mlxsw_env->max_eeprom_len);
+
+		mlxsw_reg_mcia_pack(mcia_pl, slot_index, module, page->page,
+				    device_addr + bytes_written, size,
+				    page->i2c_address);
+		mlxsw_reg_mcia_bank_number_set(mcia_pl, page->bank);
+		memcpy(eeprom_tmp, page->data + bytes_written, size);
+		mlxsw_reg_mcia_eeprom_memcpy_to(mcia_pl, eeprom_tmp);
+
+		err = mlxsw_reg_write(mlxsw_core, MLXSW_REG(mcia), mcia_pl);
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack, "Failed to access module's EEPROM");
+			return err;
+		}
+
+		err = mlxsw_env_mcia_status_process(mcia_pl, extack);
+		if (err)
+			return err;
+
+		bytes_written += size;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(mlxsw_env_set_module_eeprom_by_page);
+
 static int mlxsw_env_module_reset(struct mlxsw_core *mlxsw_core, u8 slot_index,
 				  u8 module)
 {
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_env.h b/drivers/net/ethernet/mellanox/mlxsw/core_env.h
index a197e3ae069c..e4ff17869400 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_env.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_env.h
@@ -28,6 +28,12 @@ mlxsw_env_get_module_eeprom_by_page(struct mlxsw_core *mlxsw_core,
 				    const struct ethtool_module_eeprom *page,
 				    struct netlink_ext_ack *extack);
 
+int
+mlxsw_env_set_module_eeprom_by_page(struct mlxsw_core *mlxsw_core,
+				    u8 slot_index, u8 module,
+				    const struct ethtool_module_eeprom *page,
+				    struct netlink_ext_ack *extack);
+
 int mlxsw_env_reset_module(struct net_device *netdev,
 			   struct mlxsw_core *mlxsw_core, u8 slot_index,
 			   u8 module, u32 *flags);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/minimal.c b/drivers/net/ethernet/mellanox/mlxsw/minimal.c
index 6b98c3287b49..e52be38b5495 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/minimal.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/minimal.c
@@ -140,6 +140,20 @@ mlxsw_m_get_module_eeprom_by_page(struct net_device *netdev,
 						   page, extack);
 }
 
+static int
+mlxsw_m_set_module_eeprom_by_page(struct net_device *netdev,
+				  const struct ethtool_module_eeprom *page,
+				  struct netlink_ext_ack *extack)
+{
+	struct mlxsw_m_port *mlxsw_m_port = netdev_priv(netdev);
+	struct mlxsw_core *core = mlxsw_m_port->mlxsw_m->core;
+
+	return mlxsw_env_set_module_eeprom_by_page(core,
+						   mlxsw_m_port->slot_index,
+						   mlxsw_m_port->module,
+						   page, extack);
+}
+
 static int mlxsw_m_reset(struct net_device *netdev, u32 *flags)
 {
 	struct mlxsw_m_port *mlxsw_m_port = netdev_priv(netdev);
@@ -181,6 +195,7 @@ static const struct ethtool_ops mlxsw_m_port_ethtool_ops = {
 	.get_module_info	= mlxsw_m_get_module_info,
 	.get_module_eeprom	= mlxsw_m_get_module_eeprom,
 	.get_module_eeprom_by_page = mlxsw_m_get_module_eeprom_by_page,
+	.set_module_eeprom_by_page = mlxsw_m_set_module_eeprom_by_page,
 	.reset			= mlxsw_m_reset,
 	.get_module_power_mode	= mlxsw_m_get_module_power_mode,
 	.set_module_power_mode	= mlxsw_m_set_module_power_mode,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c
index 0f29e9c19411..d4fd7f7d660f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c
@@ -1067,6 +1067,20 @@ mlxsw_sp_get_module_eeprom_by_page(struct net_device *dev,
 						   module, page, extack);
 }
 
+static int
+mlxsw_sp_set_module_eeprom_by_page(struct net_device *dev,
+				   const struct ethtool_module_eeprom *page,
+				   struct netlink_ext_ack *extack)
+{
+	struct mlxsw_sp_port *mlxsw_sp_port = netdev_priv(dev);
+	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
+	u8 slot_index = mlxsw_sp_port->mapping.slot_index;
+	u8 module = mlxsw_sp_port->mapping.module;
+
+	return mlxsw_env_set_module_eeprom_by_page(mlxsw_sp->core, slot_index,
+						   module, page, extack);
+}
+
 static int
 mlxsw_sp_get_ts_info(struct net_device *netdev, struct ethtool_ts_info *info)
 {
@@ -1256,6 +1270,7 @@ const struct ethtool_ops mlxsw_sp_port_ethtool_ops = {
 	.get_module_info		= mlxsw_sp_get_module_info,
 	.get_module_eeprom		= mlxsw_sp_get_module_eeprom,
 	.get_module_eeprom_by_page	= mlxsw_sp_get_module_eeprom_by_page,
+	.set_module_eeprom_by_page	= mlxsw_sp_set_module_eeprom_by_page,
 	.get_ts_info			= mlxsw_sp_get_ts_info,
 	.get_eth_phy_stats		= mlxsw_sp_get_eth_phy_stats,
 	.get_eth_mac_stats		= mlxsw_sp_get_eth_mac_stats,
-- 
2.40.1

[RFC PATCH net-next 3/9] ethtool: Add an interface for flashing transceiver modules' firmware

[Danielle Ratson]

CMIS compliant 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.2 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 compliant
modules in mind, but kept generic enough to accommodate future use
cases, if these arise.

Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---
 Documentation/netlink/specs/ethtool.yaml     | 57 ++++++++++++++++++
 Documentation/networking/ethtool-netlink.rst | 62 ++++++++++++++++++++
 include/uapi/linux/ethtool.h                 | 18 ++++++
 include/uapi/linux/ethtool_netlink.h         | 20 +++++++
 4 files changed, 157 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 197208f419dc..73adb23acccb 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -942,6 +942,38 @@ attribute-sets:
       -
         name: burst-tmr
         type: u32
+  -
+    name: module-fw-flash
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: file-name
+        type: string
+      -
+        name: password
+        type: u32
+  -
+    name: module-fw-flash-ntf
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: status
+        type: u8
+      -
+        name: status-msg
+        type: string
+      -
+        name: done
+        type: u64
+      -
+        name: total
+        type: u64
 
 operations:
   enum-model: directional
@@ -1693,3 +1725,28 @@ operations:
       name: mm-ntf
       doc: Notification for change in MAC Merge configuration.
       notify: mm-get
+    -
+      name: module-fw-flash-act
+      doc: Flash transceiver module firmware.
+
+      attribute-set: module-fw-flash
+
+      do:
+        request:
+          attributes:
+            - header
+            - file-name
+            - password
+    -
+      name: module-fw-flash-ntf
+      doc: Notification for firmware flashing progress and status.
+
+      attribute-set: module-fw-flash-ntf
+
+      event:
+        attributes:
+          - header
+          - status
+          - status-msg
+          - done
+          - total
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index d583d9abf2f8..cef0c51dd21f 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -228,6 +228,7 @@ Userspace to kernel:
   ``ETHTOOL_MSG_PLCA_GET_STATUS``       get PLCA RS status
   ``ETHTOOL_MSG_MM_GET``                get MAC merge layer state
   ``ETHTOOL_MSG_MM_SET``                set MAC merge layer parameters
+  ``ETHTOOL_MSG_MODULE_FW_FLASH_ACT``   flash transceiver module firmware
   ===================================== =================================
 
 Kernel to userspace:
@@ -274,6 +275,7 @@ Kernel to userspace:
   ``ETHTOOL_MSG_PLCA_GET_STATUS_REPLY``    PLCA RS status
   ``ETHTOOL_MSG_PLCA_NTF``                 PLCA RS parameters
   ``ETHTOOL_MSG_MM_GET_REPLY``             MAC merge layer status
+  ``ETHTOOL_MSG_MODULE_FW_FLASH_NTF``      transceiver module flash updates
   ======================================== =================================
 
 ``GET`` requests are sent by userspace applications to retrieve device
@@ -2004,6 +2006,65 @@ The attributes are propagated to the driver through the following structure:
 .. kernel-doc:: include/linux/ethtool.h
     :identifiers: ethtool_mm_cfg
 
+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_PASSWORD``   u32     transceiver module password
+  =======================================  ======  ===========================
+
+The firmware update process is composed from three logical steps:
+
+1. Downloading a firmware image to the transceiver module and validating it.
+2. Running the firmware image.
+3. Committing the firmware image so that it is run upon reset.
+
+When flash command is given, those three steps are taken in that order.
+
+The ``ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME`` attribute encodes the firmware
+image file name. The firmware image is downloaded to the transceiver module,
+validated, run and committed.
+
+The optional ``ETHTOOL_A_MODULE_FW_FLASH_PASSWORD`` attribute encodes a password
+that might be required as part of the transceiver module firmware update
+process.
+
+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
 ===================
 
@@ -2110,4 +2171,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_PLCA_GET_STATUS``
   n/a                                 ``ETHTOOL_MSG_MM_GET``
   n/a                                 ``ETHTOOL_MSG_MM_SET``
+  n/a                                 ``ETHTOOL_MSG_MODULE_FW_FLASH_ACT``
   =================================== =====================================
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 06ef6b78b7de..79041bdccd61 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -822,6 +822,24 @@ enum ethtool_mm_verify_status {
 	ETHTOOL_MM_VERIFY_STATUS_DISABLED,
 };
 
+/**
+ * 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 3f89074aa06c..44e02533d01d 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -57,6 +57,7 @@ enum {
 	ETHTOOL_MSG_PLCA_GET_STATUS,
 	ETHTOOL_MSG_MM_GET,
 	ETHTOOL_MSG_MM_SET,
+	ETHTOOL_MSG_MODULE_FW_FLASH_ACT,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -109,6 +110,7 @@ enum {
 	ETHTOOL_MSG_PLCA_NTF,
 	ETHTOOL_MSG_MM_GET_REPLY,
 	ETHTOOL_MSG_MM_NTF,
+	ETHTOOL_MSG_MODULE_FW_FLASH_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -976,6 +978,24 @@ enum {
 	ETHTOOL_A_MM_MAX = (__ETHTOOL_A_MM_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_PASSWORD,		/* u32 */
+	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
-- 
2.40.1

[RFC PATCH net-next 4/9] ethtool: Add flashing transceiver modules' firmware notifications ability

[Danielle Ratson]

Add progress notifications ability to user space while flashing modules'
firmware by implementing the interface between the user space and the
kernel.

Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---
 net/ethtool/module.c    | 76 +++++++++++++++++++++++++++++++++++++++++
 net/ethtool/module_fw.h | 10 ++++++
 2 files changed, 86 insertions(+)
 create mode 100644 net/ethtool/module_fw.h

diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index ceb575efc290..09cf11564840 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -5,6 +5,7 @@
 #include "netlink.h"
 #include "common.h"
 #include "bitset.h"
+#include "module_fw.h"
 
 struct module_req_info {
 	struct ethnl_req_info base;
@@ -158,3 +159,78 @@ const struct ethnl_request_ops ethnl_module_request_ops = {
 	.set			= ethnl_set_module,
 	.set_ntf_cmd		= ETHTOOL_MSG_MODULE_NTF,
 };
+
+/* MODULE_FW_FLASH_NTF */
+
+static void
+ethnl_module_fw_flash_ntf(struct net_device *dev,
+			  enum ethtool_module_fw_flash_status status,
+			  const char *status_msg, u64 done, u64 total)
+{
+	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, status))
+		goto err_skb;
+
+	if (status_msg &&
+	    nla_put_string(skb, ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG,
+			   status_msg))
+		goto err_skb;
+
+	if (nla_put_u64_64bit(skb, ETHTOOL_A_MODULE_FW_FLASH_DONE, done,
+			      ETHTOOL_A_MODULE_FW_FLASH_PAD))
+		goto err_skb;
+
+	if (nla_put_u64_64bit(skb, ETHTOOL_A_MODULE_FW_FLASH_TOTAL,
+			      total, ETHTOOL_A_MODULE_FW_FLASH_PAD))
+		goto err_skb;
+
+	genlmsg_end(skb, hdr);
+	ethnl_multicast(skb, dev);
+	return;
+
+err_skb:
+	nlmsg_free(skb);
+}
+
+void ethnl_module_fw_flash_ntf_err(struct net_device *dev,
+				   const char *status_msg)
+{
+	ethnl_module_fw_flash_ntf(dev, ETHTOOL_MODULE_FW_FLASH_STATUS_ERROR,
+				  status_msg, 0, 0);
+}
+
+void ethnl_module_fw_flash_ntf_start(struct net_device *dev)
+{
+	ethnl_module_fw_flash_ntf(dev, ETHTOOL_MODULE_FW_FLASH_STATUS_STARTED,
+				  NULL, 0, 0);
+}
+
+void ethnl_module_fw_flash_ntf_complete(struct net_device *dev)
+{
+	ethnl_module_fw_flash_ntf(dev, ETHTOOL_MODULE_FW_FLASH_STATUS_COMPLETED,
+				  NULL, 0, 0);
+}
+
+void ethnl_module_fw_flash_ntf_in_progress(struct net_device *dev, u64 done,
+					   u64 total)
+{
+	ethnl_module_fw_flash_ntf(dev,
+				  ETHTOOL_MODULE_FW_FLASH_STATUS_IN_PROGRESS,
+				  NULL, done, total);
+}
diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h
new file mode 100644
index 000000000000..36cb0bc85526
--- /dev/null
+++ b/net/ethtool/module_fw.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <uapi/linux/ethtool.h>
+
+void ethnl_module_fw_flash_ntf_err(struct net_device *dev,
+				   const char *status_msg);
+void ethnl_module_fw_flash_ntf_start(struct net_device *dev);
+void ethnl_module_fw_flash_ntf_complete(struct net_device *dev);
+void ethnl_module_fw_flash_ntf_in_progress(struct net_device *dev, u64 done,
+					   u64 total);
-- 
2.40.1

[RFC PATCH net-next 5/9] include: netdevice: Add module firmware flashing in progress flag to net_device

[Danielle Ratson]

Some operations cannot be performed during the firmware flashing
process. For example, flashing firmware on a device which is already in
a flashing process should be forbidden.

In order to veto those scenarios, add a flag in 'struct net_device' that
indicates when a firmware flash is taking place on the module.

Signed-off-by: Danielle Ratson <danieller@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
 include/linux/netdevice.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 118c40258d07..af42125ead69 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2061,6 +2061,8 @@ enum netdev_stat_type {
  *
  *	@threaded:	napi threaded mode is enabled
  *
+ *	@module_fw_flash_in_progress:	Module firmware flashing is in progress.
+ *
  *	@net_notifier_list:	List of per-net netdev notifier block
  *				that follow this device when it is moved
  *				to another network namespace.
@@ -2447,7 +2449,7 @@ struct net_device {
 	bool			proto_down;
 	unsigned		wol_enabled:1;
 	unsigned		threaded:1;
-
+	unsigned		module_fw_flash_in_progress:1;
 	struct list_head	net_notifier_list;
 
 #if IS_ENABLED(CONFIG_MACSEC)
-- 
2.40.1

[RFC PATCH net-next 6/9] net: sfp: Add more extended compliance codes

[Danielle Ratson]

SFF-8024 is used to define various constants re-used in several SFF
SFP-related specifications.

Add SFF-8024 extended compliance code definitions for CMIS compliant
modules and use them in the next patch to determine the firmware flashing
work.

Signed-off-by: Danielle Ratson <danieller@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
 include/linux/sfp.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index 9346cd44814d..df2eb988854f 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -284,6 +284,12 @@ enum {
 	SFF8024_ID_QSFP_8438		= 0x0c,
 	SFF8024_ID_QSFP_8436_8636	= 0x0d,
 	SFF8024_ID_QSFP28_8636		= 0x11,
+	SFF8024_ID_QSFP_DD		= 0x18,
+	SFF8024_ID_OSFP			= 0x19,
+	SFF8024_ID_DSFP			= 0x1B,
+	SFF8024_ID_QSFP_PLUS_CMIS	= 0x1E,
+	SFF8024_ID_SFP_DD_CMIS		= 0x1F,
+	SFF8024_ID_SFP_PLUS_CMIS	= 0x20,
 
 	SFF8024_ENCODING_UNSPEC		= 0x00,
 	SFF8024_ENCODING_8B10B		= 0x01,
-- 
2.40.1

[RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

[Danielle Ratson]

CDB (Command Data Block Message Communication) reads and writes are
performed on memory map pages 9Fh-AFh according to the CMIS standard,
section 8.20 of revision 5.2.
Page 9Fh is used to specify the CDB command to be executed and also
provides an area for a local payload (LPL).

According to the CMIS standard, the firmware update process is done using
a CDB commands sequence that will be implemented in the next patch.

The kernel interface that will implement the firmware update using CDB
command will include 2 layers that will be added under ethtool:

* The upper layer that will be triggered from the module layer, is
  cmis_fw_update.
* The lower one is cmis_cdb.

In the future there might be more operations to implement using CDB
commands. Therefore, the idea is to keep the CDB interface clean and the
cmis_fw_update specific to the CDB commands handling it.

These two layers will communicate using the API the consists of three
functions:

- struct ethtool_cmis_cdb *
  ethtool_cmis_cdb_init(struct net_device *dev,
			struct ethtool_module_fw_flash_params *params);
- void ethtool_cmis_cdb_fini(struct ethtool_cmis_cdb *cdb);
- int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
				   struct ethtool_cmis_cdb_cmd_args *args);

Add the CDB layer to support initializing, finishing and executing CDB
commands:

* The initialization process will include creating of an ethtool_cmis_cdb
  instance, querying the module CDB support, entering and validating the
  password from user space (CMD 0x0000) and querying the module features
  (CMD 0x0040).

* The finishing API will simply free the ethtool_cmis_cdb instance.

* The executing process will write the CDB command to EEPROM using
  set_module_eeprom_by_page() that was presented earlier, and will
  process the reply from EEPROM.

Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---
 net/ethtool/Makefile    |   2 +-
 net/ethtool/cmis.h      | 112 ++++++++
 net/ethtool/cmis_cdb.c  | 563 ++++++++++++++++++++++++++++++++++++++++
 net/ethtool/module_fw.h |  12 +
 4 files changed, 688 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/cmis.h
 create mode 100644 net/ethtool/cmis_cdb.c

diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 504f954a1b28..38806b3ecf83 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,4 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.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 fec.o eeprom.o stats.o phc_vclocks.o mm.o \
-		   module.o pse-pd.o plca.o mm.o
+		   module.o cmis_cdb.o pse-pd.o plca.o mm.o
diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
new file mode 100644
index 000000000000..a8d6f96ed26f
--- /dev/null
+++ b/net/ethtool/cmis.h
@@ -0,0 +1,112 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#define ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH		120
+#define ETHTOOL_CMIS_CDB_CMD_PAGE			0x9F
+#define ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR			0x50
+
+/**
+ * struct ethtool_cmis_cdb - CDB commands parameters
+ * @cmis_rev: CMIS revision major.
+ * @read_write_len_ext: Allowable additional number of byte octets to the LPL
+ *			in a READ or a WRITE CDB commands.
+ * @max_completion_time:  Maximum CDB command completion time in msec.
+ */
+struct ethtool_cmis_cdb {
+	u8	cmis_rev;
+	u8      read_write_len_ext;
+	u16     max_completion_time;
+};
+
+enum ethtool_cmis_cdb_cmd_id {
+	ETHTOOL_CMIS_CDB_CMD_QUERY_STATUS		= 0x0000,
+	ETHTOOL_CMIS_CDB_CMD_MODULE_FEATURES		= 0x0040,
+};
+
+/**
+ * struct ethtool_cmis_cdb_request - CDB commands request fields as decribed in
+ *				the CMIS standard
+ * @id: Command ID.
+ * @epl_len: EPL memory length.
+ * @lpl_len: LPL memory length.
+ * @chk_code: Check code for the previous field and the payload.
+ * @resv1: Added to match the CMIS standard request continuity.
+ * @resv2: Added to match the CMIS standard request continuity.
+ * @payload: Payload for the CDB commands.
+ */
+struct ethtool_cmis_cdb_request {
+	__be16 id;
+	struct_group(body,
+		u16 epl_len;
+		u8 lpl_len;
+		u8 chk_code;
+		u8 resv1;
+		u8 resv2;
+		u8 payload[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH];
+	);
+};
+
+#define CDB_F_COMPLETION_VALID		BIT(0)
+#define CDB_F_STATUS_VALID		BIT(1)
+
+/**
+ * struct ethtool_cmis_cdb_cmd_args - CDB commands execution arguments
+ * @req: CDB command fields as described in the CMIS standard.
+ * @max_duration: Maximum duration time for command completion in msec.
+ * @read_write_len_ext: Allowable additional number of byte octets to the LPL
+ *			in a READ or a WRITE commands.
+ * @rpl_exp_len: Expected reply length in bytes.
+ * @flags: Validation flags for CDB commands.
+ */
+struct ethtool_cmis_cdb_cmd_args {
+	struct ethtool_cmis_cdb_request req;
+	u16				max_duration;
+	u8				read_write_len_ext;
+	u8                              rpl_exp_len;
+	u8				flags;
+};
+
+/**
+ * struct ethtool_cmis_cdb_rpl_hdr - CDB commands reply header arguments
+ * @rpl_len: Reply length.
+ * @rpl_chk_code: Reply check code.
+ */
+struct ethtool_cmis_cdb_rpl_hdr {
+	u8 rpl_len;
+	u8 rpl_chk_code;
+};
+
+/**
+ * struct ethtool_cmis_cdb_rpl - CDB commands reply arguments
+ * @hdr: CDB commands reply header arguments.
+ * @payload: Payload for the CDB commands reply.
+ */
+struct ethtool_cmis_cdb_rpl {
+	struct ethtool_cmis_cdb_rpl_hdr hdr;
+	u8 payload[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH];
+};
+
+u32 ethtool_cmis_get_max_payload_size(u8 num_of_byte_octs);
+
+void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
+				   enum ethtool_cmis_cdb_cmd_id cmd, u8 *pl,
+				   u8 lpl_len, u16 max_duration,
+				   u8 read_write_len_ext, u8 rpl_exp_len,
+				   u8 flags);
+
+void ethtool_cmis_cdb_check_completion_flag(u8 cmis_rev, u8 *flags);
+
+int ethtool_cmis_page_init(struct ethtool_module_eeprom *page_data,
+			   u8 page, u32 offset, u32 length);
+void ethtool_cmis_page_fini(struct ethtool_module_eeprom *page_data);
+
+struct ethtool_cmis_cdb *
+ethtool_cmis_cdb_init(struct net_device *dev,
+		      const struct ethtool_module_fw_flash_params *params);
+void ethtool_cmis_cdb_fini(struct ethtool_cmis_cdb *cdb);
+
+int ethtool_cmis_wait_for_cond(struct net_device *dev, u8 flags, u8 flag,
+			       u16 max_duration, u32 offset,
+			       bool (*cond_success)(u8), bool (*cond_fail)(u8));
+
+int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
+				 struct ethtool_cmis_cdb_cmd_args *args);
diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
new file mode 100644
index 000000000000..b27e12871816
--- /dev/null
+++ b/net/ethtool/cmis_cdb.c
@@ -0,0 +1,563 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/ethtool.h>
+#include <linux/jiffies.h>
+
+#include "common.h"
+#include "module_fw.h"
+#include "cmis.h"
+
+/* For accessing the LPL field on page 9Fh, the allowable length extension is
+ * min(i, 15) byte octets where i specifies the allowable additional number of
+ * byte octets in a READ or a WRITE.
+ */
+u32 ethtool_cmis_get_max_payload_size(u8 num_of_byte_octs)
+{
+	return 8 * (1 + min_t(u8, num_of_byte_octs, 15));
+}
+
+void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
+				   enum ethtool_cmis_cdb_cmd_id cmd, u8 *pl,
+				   u8 lpl_len, u16 max_duration,
+				   u8 read_write_len_ext, u8 rpl_exp_len,
+				   u8 flags)
+{
+	args->req.id = cpu_to_be16(cmd);
+	args->req.lpl_len = lpl_len;
+	if (pl)
+		memcpy(args->req.payload, pl, args->req.lpl_len);
+
+	args->max_duration = max_duration;
+	args->read_write_len_ext =
+		ethtool_cmis_get_max_payload_size(read_write_len_ext);
+	args->rpl_exp_len = rpl_exp_len;
+	args->flags = flags;
+}
+
+int ethtool_cmis_page_init(struct ethtool_module_eeprom *page_data,
+			   u8 page, u32 offset, u32 length)
+{
+	page_data->page = page;
+	page_data->offset = offset;
+	page_data->length = length;
+	page_data->i2c_address = ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR;
+	page_data->data = kmalloc(page_data->length, GFP_KERNEL);
+	if (!page_data->data)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void ethtool_cmis_page_fini(struct ethtool_module_eeprom *page_data)
+{
+	kfree(page_data->data);
+}
+
+#define CMIS_REVISION_PAGE	0x00
+#define CMIS_REVISION_OFFSET	0x01
+
+struct cmis_rev_rpl {
+	u8 rev;
+};
+
+static inline u8
+cmis_rev_rpl_major(struct cmis_rev_rpl *rpl)
+{
+	return rpl->rev >> 4;
+}
+
+static int cmis_rev_major_get(struct net_device *dev, u8 *rev_major)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {};
+	struct netlink_ext_ack extack = {};
+	struct cmis_rev_rpl *rpl;
+	int err;
+
+	err = ethtool_cmis_page_init(&page_data, CMIS_REVISION_PAGE,
+				     CMIS_REVISION_OFFSET, sizeof(*rpl));
+	if (err < 0)
+		return err;
+
+	err = ops->get_module_eeprom_by_page(dev, &page_data, &extack);
+	if (err < 0) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+		goto out;
+	}
+
+	rpl = (struct cmis_rev_rpl *)page_data.data;
+	*rev_major = cmis_rev_rpl_major(rpl);
+
+out:
+	ethtool_cmis_page_fini(&page_data);
+	return err;
+}
+
+#define CMIS_CDB_ADVERTISEMENT_PAGE	0x01
+#define CMIS_CDB_ADVERTISEMENT_OFFSET	0xA3
+
+/* Based on section 8.4.11 "CDB Messaging Support Advertisement" in CMIS
+ * standard revision 5.2.
+ */
+struct cmis_cdb_advert_rpl {
+	u8	inst_supported;
+	u8	read_write_len_ext;
+	u8	resv1;
+	u8	resv2;
+};
+
+static inline u8
+cmis_cdb_advert_rpl_inst_supported(struct cmis_cdb_advert_rpl *rpl)
+{
+	return rpl->inst_supported >> 6;
+}
+
+static int cmis_cdb_advertisement_get(struct ethtool_cmis_cdb *cdb,
+				      struct net_device *dev)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {};
+	struct netlink_ext_ack extack = {};
+	struct cmis_cdb_advert_rpl *rpl;
+	int err;
+
+	err = ethtool_cmis_page_init(&page_data, CMIS_CDB_ADVERTISEMENT_PAGE,
+				     CMIS_CDB_ADVERTISEMENT_OFFSET,
+				     sizeof(*rpl));
+	if (err < 0)
+		return err;
+
+	err = ops->get_module_eeprom_by_page(dev, &page_data, &extack);
+	if (err < 0) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+		goto out;
+	}
+
+	rpl = (struct cmis_cdb_advert_rpl *)page_data.data;
+	if (!cmis_cdb_advert_rpl_inst_supported(rpl)) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	cdb->read_write_len_ext = rpl->read_write_len_ext;
+
+out:
+	ethtool_cmis_page_fini(&page_data);
+	return err;
+}
+
+#define CMIS_PASSWORD_ENTRY_PAGE	0x00
+#define CMIS_PASSWORD_ENTRY_OFFSET	0x7A
+
+struct cmis_password_entry_pl {
+	u32 password;
+};
+
+/* See section 9.3.1 "CMD 0000h: Query Status" in CMIS standard revision 5.2.
+ * struct cmis_cdb_query_status_pl and struct cmis_cdb_query_status_rpl are
+ * structured layouts of the flat arrays,
+ * struct ethtool_cmis_cdb_request::payload and
+ * struct ethtool_cmis_cdb_rpl::payload respectively.
+ */
+struct cmis_cdb_query_status_pl {
+	u16 response_delay;
+};
+
+struct cmis_cdb_query_status_rpl {
+	u8 length;
+	u8 status;
+};
+
+static int
+cmis_cdb_validate_password(struct ethtool_cmis_cdb *cdb,
+			   struct net_device *dev,
+			   const struct ethtool_module_fw_flash_params *params)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct cmis_cdb_query_status_pl qs_pl = {0};
+	struct ethtool_module_eeprom page_data = {};
+	struct cmis_cdb_query_status_rpl *rpl;
+	struct ethtool_cmis_cdb_cmd_args args;
+	struct cmis_password_entry_pl *pe_pl;
+	struct netlink_ext_ack extack = {};
+	int err;
+
+	err = ethtool_cmis_page_init(&page_data, CMIS_PASSWORD_ENTRY_PAGE,
+				     CMIS_PASSWORD_ENTRY_OFFSET,
+				     sizeof(*pe_pl));
+	if (err < 0)
+		return err;
+
+	pe_pl = (struct cmis_password_entry_pl *)page_data.data;
+	pe_pl->password = params->password;
+	err = ops->set_module_eeprom_by_page(dev, &page_data, &extack);
+	if (err < 0) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+		goto out;
+	}
+
+	ethtool_cmis_cdb_compose_args(&args, ETHTOOL_CMIS_CDB_CMD_QUERY_STATUS,
+				      (u8 *)&qs_pl, sizeof(qs_pl), 0,
+				      cdb->read_write_len_ext, sizeof(*rpl),
+				      CDB_F_COMPLETION_VALID | CDB_F_STATUS_VALID);
+
+	err = ethtool_cmis_cdb_execute_cmd(dev, &args);
+	if (err < 0) {
+		ethnl_module_fw_flash_ntf_err(dev,
+					      "Query Status command failed");
+		goto out;
+	}
+
+	rpl = (struct cmis_cdb_query_status_rpl *)args.req.payload;
+	if (!rpl->length || !rpl->status) {
+		ethnl_module_fw_flash_ntf_err(dev, "Password was not accepted");
+		err = -EINVAL;
+	}
+
+out:
+	ethtool_cmis_page_fini(&page_data);
+	return err;
+}
+
+/* Some CDB commands asserts the CDB completion flag only from CMIS
+ * revision 5. Therefore, check the relevant validity flag only when
+ * the revision supports it.
+ */
+inline void ethtool_cmis_cdb_check_completion_flag(u8 cmis_rev, u8 *flags)
+{
+	*flags |= cmis_rev >= 5 ? CDB_F_COMPLETION_VALID : 0;
+}
+
+#define CMIS_CDB_MODULE_FEATURES_RESV_DATA	34
+
+/* See section 9.4.1 "CMD 0040h: Module Features" in CMIS standard revision 5.2.
+ * struct cmis_cdb_module_features_rpl is structured layout of the flat
+ * array, ethtool_cmis_cdb_rpl::payload.
+ */
+struct cmis_cdb_module_features_rpl {
+	u8	resv1[CMIS_CDB_MODULE_FEATURES_RESV_DATA];
+	__be16	max_completion_time;
+};
+
+static inline u16
+cmis_cdb_module_features_completion_time(struct cmis_cdb_module_features_rpl *rpl)
+{
+	return be16_to_cpu(rpl->max_completion_time);
+}
+
+static int cmis_cdb_module_features_get(struct ethtool_cmis_cdb *cdb,
+					struct net_device *dev)
+{
+	struct cmis_cdb_module_features_rpl *rpl;
+	struct ethtool_cmis_cdb_cmd_args args;
+	u8 flags = CDB_F_STATUS_VALID;
+	int err;
+
+	ethtool_cmis_cdb_check_completion_flag(cdb->cmis_rev, &flags);
+	ethtool_cmis_cdb_compose_args(&args,
+				      ETHTOOL_CMIS_CDB_CMD_MODULE_FEATURES,
+				      NULL, 0, 0, cdb->read_write_len_ext,
+				      sizeof(*rpl), flags);
+
+	err = ethtool_cmis_cdb_execute_cmd(dev, &args);
+	if (err < 0) {
+		ethnl_module_fw_flash_ntf_err(dev,
+					      "Module Features command failed");
+		return err;
+	}
+
+	rpl = (struct cmis_cdb_module_features_rpl *)args.req.payload;
+	cdb->max_completion_time =
+		cmis_cdb_module_features_completion_time(rpl);
+
+	return 0;
+}
+
+struct ethtool_cmis_cdb *
+ethtool_cmis_cdb_init(struct net_device *dev,
+		      const struct ethtool_module_fw_flash_params *params)
+{
+	struct ethtool_cmis_cdb *cdb;
+	int err;
+
+	cdb = kzalloc(sizeof(*cdb), GFP_KERNEL);
+	if (!cdb)
+		return ERR_PTR(-ENOMEM);
+
+	err = cmis_rev_major_get(dev, &cdb->cmis_rev);
+	if (err < 0)
+		goto err;
+
+	if (cdb->cmis_rev < 4) {
+		ethnl_module_fw_flash_ntf_err(dev,
+					      "CMIS revision doesn't support module firmware flashing");
+		err = -EOPNOTSUPP;
+		goto err;
+	}
+
+	err = cmis_cdb_advertisement_get(cdb, dev);
+	if (err < 0)
+		goto err;
+
+	if (params->password_valid) {
+		err = cmis_cdb_validate_password(cdb, dev, params);
+		if (err < 0)
+			goto err;
+	}
+
+	err = cmis_cdb_module_features_get(cdb, dev);
+	if (err < 0)
+		goto err;
+
+	return cdb;
+
+err:
+	ethtool_cmis_cdb_fini(cdb);
+	return ERR_PTR(err);
+}
+
+void ethtool_cmis_cdb_fini(struct ethtool_cmis_cdb *cdb)
+{
+	kfree(cdb);
+}
+
+static bool is_completed(u8 data)
+{
+	return data;
+}
+
+#define CMIS_CDB_STATUS_SUCCESS	0x01
+
+static bool status_success(u8 data)
+{
+	return data == CMIS_CDB_STATUS_SUCCESS;
+}
+
+#define CMIS_CDB_STATUS_FAIL	0x40
+
+static bool status_fail(u8 data)
+{
+	return data & CMIS_CDB_STATUS_FAIL;
+}
+
+#define CMIS_LOWER_PAGE		0x00
+#define CMIS_BYTE_LENGTH	1
+
+struct cmis_wait_for_cond_rpl {
+	u8 state;
+};
+
+int ethtool_cmis_wait_for_cond(struct net_device *dev, u8 flags, u8 flag,
+			       u16 max_duration, u32 offset,
+			       bool (*cond_success)(u8), bool (*cond_fail)(u8))
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {};
+	struct cmis_wait_for_cond_rpl *rpl;
+	struct netlink_ext_ack extack = {};
+	unsigned long end;
+	int err;
+
+	if (!(flags & flag))
+		return 0;
+
+	err = ethtool_cmis_page_init(&page_data, CMIS_LOWER_PAGE, offset,
+				     CMIS_BYTE_LENGTH);
+	if (err < 0)
+		return err;
+
+	if (max_duration == 0)
+		max_duration = U16_MAX;
+
+	end = jiffies + msecs_to_jiffies(max_duration);
+	do {
+		err = ops->get_module_eeprom_by_page(dev, &page_data, &extack);
+		if (err < 0) {
+			if (extack._msg)
+				netdev_err(dev, "%s\n", extack._msg);
+			continue;
+		}
+
+		rpl = (struct cmis_wait_for_cond_rpl *)page_data.data;
+		if ((*cond_success)(rpl->state))
+			goto out;
+
+		if (*cond_fail && (*cond_fail)(rpl->state))
+			break;
+
+	} while (time_before(jiffies, end));
+
+	err = -EBUSY;
+
+out:
+	ethtool_cmis_page_fini(&page_data);
+	return err;
+}
+
+#define CMIS_CDB_COMPLETION_FLAG_OFFSET	0x08
+
+static int cmis_cdb_wait_for_completion(struct net_device *dev,
+					struct ethtool_cmis_cdb_cmd_args *args)
+{
+	return ethtool_cmis_wait_for_cond(dev, args->flags,
+					  CDB_F_COMPLETION_VALID,
+					  args->max_duration,
+					  CMIS_CDB_COMPLETION_FLAG_OFFSET,
+					  is_completed, NULL);
+}
+
+#define CMIS_CDB_STATUS_OFFSET	0x25
+
+static int cmis_cdb_wait_for_status(struct net_device *dev,
+				    struct ethtool_cmis_cdb_cmd_args *args)
+{
+	return ethtool_cmis_wait_for_cond(dev, args->flags, CDB_F_STATUS_VALID,
+					  args->max_duration,
+					  CMIS_CDB_STATUS_OFFSET,
+					  status_success, status_fail);
+}
+
+#define CMIS_CDB_REPLY_OFFSET	0x86
+
+static int cmis_cdb_get_reply(struct net_device *dev,
+			      struct ethtool_module_eeprom *page_data,
+			      u8 rpl_exp_len, struct netlink_ext_ack *extack)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	page_data->offset = CMIS_CDB_REPLY_OFFSET;
+	page_data->length = rpl_exp_len;
+	page_data->i2c_address = ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR;
+	page_data->page = ETHTOOL_CMIS_CDB_CMD_PAGE;
+
+	return ops->get_module_eeprom_by_page(dev, page_data, extack);
+}
+
+static int cmis_cdb_process_reply(struct net_device *dev,
+				  struct ethtool_module_eeprom *page_data,
+				  struct ethtool_cmis_cdb_cmd_args *args)
+{
+	u8 rpl_hdr_len = sizeof(struct ethtool_cmis_cdb_rpl_hdr);
+	struct netlink_ext_ack extack = {};
+	struct ethtool_cmis_cdb_rpl *rpl;
+	int err;
+
+	if (!args->rpl_exp_len)
+		return 0;
+
+	err = cmis_cdb_get_reply(dev, page_data,
+				 args->rpl_exp_len + rpl_hdr_len, &extack);
+	if (err < 0) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+		return err;
+	}
+
+	rpl = (struct ethtool_cmis_cdb_rpl *)page_data->data;
+	if ((args->rpl_exp_len > rpl->hdr.rpl_len + rpl_hdr_len) ||
+	    !rpl->hdr.rpl_chk_code)
+		return -EIO;
+
+	args->req.lpl_len = rpl->hdr.rpl_len;
+	memcpy(args->req.payload, rpl->payload, args->req.lpl_len);
+
+	return 0;
+}
+
+static int
+__ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
+			       struct ethtool_module_eeprom *page_data,
+			       u32 offset, u32 length, void *data)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct netlink_ext_ack extack = {};
+	int err;
+
+	page_data->offset = offset;
+	page_data->length = length;
+
+	memset(page_data->data, 0, ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH);
+	memcpy(page_data->data, data, page_data->length);
+
+	err = ops->set_module_eeprom_by_page(dev, page_data, &extack);
+	if (err < 0) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+	}
+
+	return err;
+}
+
+static u8 cmis_cdb_calc_checksum(const void *data, size_t size)
+{
+	const u8 *bytes = (const u8 *)data;
+	u8 checksum = 0;
+
+	for (size_t i = 0; i < size; i++)
+		checksum += bytes[i];
+
+	return ~checksum;
+}
+
+#define CMIS_CDB_CMD_ID_OFFSET	0x80
+
+int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
+				 struct ethtool_cmis_cdb_cmd_args *args)
+{
+	struct ethtool_module_eeprom page_data = {};
+	u32 offset;
+	int err;
+
+	args->req.chk_code =
+		cmis_cdb_calc_checksum(&args->req, sizeof(args->req));
+
+	if (args->req.lpl_len > args->read_write_len_ext) {
+		ethnl_module_fw_flash_ntf_err(dev,
+					      "LPL length is longer than CDB read write length extension allows");
+		return -EINVAL;
+	}
+
+	err = ethtool_cmis_page_init(&page_data, ETHTOOL_CMIS_CDB_CMD_PAGE, 0,
+				     ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH);
+	if (err < 0)
+		return err;
+
+	/* According to the CMIS standard, there are two options to trigger the
+	 * CDB commands. The default option is triggering the command by writing
+	 * the CMDID bytes. Therefore, the command will be split to 2 calls:
+	 * First, with everything except the CMDID field and then the CMDID
+	 * field.
+	 */
+	offset = CMIS_CDB_CMD_ID_OFFSET +
+		offsetof(struct ethtool_cmis_cdb_request, body);
+	err = __ethtool_cmis_cdb_execute_cmd(dev, &page_data, offset,
+					     sizeof(args->req.body),
+					     &args->req.body);
+	if (err < 0)
+		goto out;
+
+	offset = CMIS_CDB_CMD_ID_OFFSET +
+		offsetof(struct ethtool_cmis_cdb_request, id);
+	err = __ethtool_cmis_cdb_execute_cmd(dev, &page_data, offset,
+					     sizeof(args->req.id),
+					     &args->req.id);
+	if (err < 0)
+		goto out;
+
+	err = cmis_cdb_wait_for_completion(dev, args);
+	if (err < 0)
+		goto out;
+
+	err = cmis_cdb_wait_for_status(dev, args);
+	if (err < 0)
+		goto out;
+
+	err = cmis_cdb_process_reply(dev, &page_data, args);
+
+out:
+	ethtool_cmis_page_fini(&page_data);
+	return err;
+}
diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h
index 36cb0bc85526..a4d4220db35d 100644
--- a/net/ethtool/module_fw.h
+++ b/net/ethtool/module_fw.h
@@ -8,3 +8,15 @@ void ethnl_module_fw_flash_ntf_start(struct net_device *dev);
 void ethnl_module_fw_flash_ntf_complete(struct net_device *dev);
 void ethnl_module_fw_flash_ntf_in_progress(struct net_device *dev, u64 done,
 					   u64 total);
+
+/**
+ * struct ethtool_module_fw_flash_params - module firmware flashing parameters
+ * @file_name: Firmware image file name.
+ * @password: Module password. Only valid when @pass_valid is set.
+ * @password_valid: Whether the module password is valid or not.
+ */
+struct ethtool_module_fw_flash_params {
+	const char *file_name;
+	u32 password;
+	u8 password_valid:1;
+};
-- 
2.40.1

[RFC PATCH net-next 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB

[Danielle Ratson]

According to the CMIS standard, the firmware update process is done using
a CDB commands sequence.

Implement a work that will be triggered from the module layer in the
next patch the will initiate and execute all the CDB commands in order, to
eventually complete the firmware update process.

This flashing process includes, writing the firmware image, running the new
firmware image and committing it after testing, so that it will run upon
reset.

This work will also notify user space about the progress of the firmware
update process.

Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---
 net/ethtool/Makefile         |   2 +-
 net/ethtool/cmis.h           |   7 +
 net/ethtool/cmis_fw_update.c | 371 +++++++++++++++++++++++++++++++++++
 net/ethtool/module_fw.h      |  18 ++
 4 files changed, 397 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/cmis_fw_update.c

diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 38806b3ecf83..9a190635fe95 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,4 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.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 fec.o eeprom.o stats.o phc_vclocks.o mm.o \
-		   module.o cmis_cdb.o pse-pd.o plca.o mm.o
+		   module.o cmis_fw_update.o cmis_cdb.o pse-pd.o plca.o mm.o
diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
index a8d6f96ed26f..dd1ac13c5e25 100644
--- a/net/ethtool/cmis.h
+++ b/net/ethtool/cmis.h
@@ -20,6 +20,12 @@ struct ethtool_cmis_cdb {
 enum ethtool_cmis_cdb_cmd_id {
 	ETHTOOL_CMIS_CDB_CMD_QUERY_STATUS		= 0x0000,
 	ETHTOOL_CMIS_CDB_CMD_MODULE_FEATURES		= 0x0040,
+	ETHTOOL_CMIS_CDB_CMD_FW_MANAGMENT_FEATURES	= 0x0041,
+	ETHTOOL_CMIS_CDB_CMD_START_FW_DOWNLOAD		= 0x0101,
+	ETHTOOL_CMIS_CDB_CMD_WRITE_FW_BLOCK_LPL		= 0x0103,
+	ETHTOOL_CMIS_CDB_CMD_COMPLETE_FW_DOWNLOAD	= 0x0107,
+	ETHTOOL_CMIS_CDB_CMD_RUN_FW_IMAGE		= 0x0109,
+	ETHTOOL_CMIS_CDB_CMD_COMMIT_FW_IMAGE		= 0x010A,
 };
 
 /**
@@ -47,6 +53,7 @@ struct ethtool_cmis_cdb_request {
 
 #define CDB_F_COMPLETION_VALID		BIT(0)
 #define CDB_F_STATUS_VALID		BIT(1)
+#define CDB_F_MODULE_STATE_VALID	BIT(2)
 
 /**
  * struct ethtool_cmis_cdb_cmd_args - CDB commands execution arguments
diff --git a/net/ethtool/cmis_fw_update.c b/net/ethtool/cmis_fw_update.c
new file mode 100644
index 000000000000..6ecaaf6cccda
--- /dev/null
+++ b/net/ethtool/cmis_fw_update.c
@@ -0,0 +1,371 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/ethtool.h>
+#include <linux/firmware.h>
+
+#include "common.h"
+#include "module_fw.h"
+#include "cmis.h"
+
+struct cmis_fw_update_fw_mng_features {
+	u8	start_cmd_payload_size;
+	u16	max_duration_start;
+	u16	max_duration_write;
+	u16	max_duration_complete;
+};
+
+/* See section 9.4.2 "CMD 0041h: Firmware Management Features" in CMIS standard
+ * revision 5.2.
+ * struct cmis_cdb_fw_mng_features_rpl is a structured layout of the flat
+ * array, ethtool_cmis_cdb_rpl::payload.
+ */
+struct cmis_cdb_fw_mng_features_rpl {
+	u8	resv1;
+	u8	resv2;
+	u8	start_cmd_payload_size;
+	u8	resv3;
+	u8	read_write_len_ext;
+	u8	write_mechanism;
+	u8	resv4;
+	u8	resv5;
+	__be16	max_duration_start;
+	__be16	resv6;
+	__be16	max_duration_write;
+	__be16	max_duration_complete;
+	__be16	resv7;
+};
+
+#define CMIS_CDB_FW_WRITE_MECHANISM_LPL	0x01
+
+static int
+cmis_fw_update_fw_mng_features_get(struct ethtool_cmis_cdb *cdb,
+				   struct net_device *dev,
+				   struct cmis_fw_update_fw_mng_features *fw_mng)
+{
+	struct cmis_cdb_fw_mng_features_rpl *rpl;
+	struct ethtool_cmis_cdb_cmd_args args;
+	u8 flags = CDB_F_STATUS_VALID;
+	int err;
+
+	ethtool_cmis_cdb_check_completion_flag(cdb->cmis_rev, &flags);
+	ethtool_cmis_cdb_compose_args(&args,
+				      ETHTOOL_CMIS_CDB_CMD_FW_MANAGMENT_FEATURES,
+				      NULL, 0, cdb->max_completion_time,
+				      cdb->read_write_len_ext, sizeof(*rpl),
+				      flags);
+
+	err = ethtool_cmis_cdb_execute_cmd(dev, &args);
+	if (err < 0) {
+		ethnl_module_fw_flash_ntf_err(dev,
+					      "FW Management Features command failed");
+		return err;
+	}
+
+	rpl = (struct cmis_cdb_fw_mng_features_rpl *)args.req.payload;
+	if (!(rpl->write_mechanism & CMIS_CDB_FW_WRITE_MECHANISM_LPL)) {
+		ethnl_module_fw_flash_ntf_err(dev,
+					      "Write LPL is not supported");
+		return  -EOPNOTSUPP;
+	}
+
+	/* Above, we used read_write_len_ext that we got from CDB
+	 * advertisement. Update it with the value that we got from module
+	 * features query, which is specific for Firmware Management Commands
+	 * (IDs 0100h-01FFh).
+	 */
+	cdb->read_write_len_ext = rpl->read_write_len_ext;
+	fw_mng->start_cmd_payload_size = rpl->start_cmd_payload_size;
+	fw_mng->max_duration_start = be16_to_cpu(rpl->max_duration_start);
+	fw_mng->max_duration_write = be16_to_cpu(rpl->max_duration_write);
+	fw_mng->max_duration_complete = be16_to_cpu(rpl->max_duration_complete);
+
+	return 0;
+}
+
+/* See section 9.7.2 "CMD 0101h: Start Firmware Download" in CMIS standard
+ * revision 5.2.
+ * struct cmis_cdb_start_fw_download_pl is a structured layout of the
+ * flat array, ethtool_cmis_cdb_request::payload.
+ */
+struct cmis_cdb_start_fw_download_pl {
+	__struct_group(cmis_cdb_start_fw_download_pl_h, head, /* no attrs */,
+			__be32	image_size;
+			__be32	resv1;
+	);
+	u8 vendor_data[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH -
+		sizeof(struct cmis_cdb_start_fw_download_pl_h)];
+};
+
+static int
+cmis_fw_update_start_download(struct ethtool_cmis_cdb *cdb,
+			      struct ethtool_module_fw_flash *module_fw,
+			      struct cmis_fw_update_fw_mng_features *fw_mng)
+{
+	u8 vendor_data_size = fw_mng->start_cmd_payload_size;
+	struct cmis_cdb_start_fw_download_pl pl = {};
+	struct ethtool_cmis_cdb_cmd_args args;
+	u8 lpl_len;
+	int err;
+
+	pl.image_size = cpu_to_be32(module_fw->fw->size);
+	memcpy(pl.vendor_data, module_fw->fw->data, vendor_data_size);
+
+	lpl_len = offsetof(struct cmis_cdb_start_fw_download_pl,
+			   vendor_data[vendor_data_size]);
+
+	ethtool_cmis_cdb_compose_args(&args,
+				      ETHTOOL_CMIS_CDB_CMD_START_FW_DOWNLOAD,
+				      (u8 *)&pl, lpl_len,
+				      fw_mng->max_duration_start,
+				      cdb->read_write_len_ext, 0,
+				      CDB_F_COMPLETION_VALID | CDB_F_STATUS_VALID);
+
+	err = ethtool_cmis_cdb_execute_cmd(module_fw->dev, &args);
+	if (err < 0)
+		ethnl_module_fw_flash_ntf_err(module_fw->dev,
+					      "Start FW download command failed");
+
+	return err;
+}
+
+/* See section 9.7.4 "CMD 0103h: Write Firmware Block LPL" in CMIS standard
+ * revision 5.2.
+ * struct cmis_cdb_write_fw_block_lpl_pl is a structured layout of the
+ * flat array, ethtool_cmis_cdb_request::payload.
+ */
+struct cmis_cdb_write_fw_block_lpl_pl {
+	__be32	block_address;
+	u8 fw_block[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH - sizeof(__be32)];
+};
+
+static int
+cmis_fw_update_write_image(struct ethtool_cmis_cdb *cdb,
+			   struct ethtool_module_fw_flash *module_fw,
+			   struct cmis_fw_update_fw_mng_features *fw_mng)
+{
+	u8 start = fw_mng->start_cmd_payload_size;
+	u32 image_size = module_fw->fw->size;
+	u32 offset, block_size, lpl_len;
+	int err;
+
+	lpl_len = ethtool_cmis_get_max_payload_size(cdb->read_write_len_ext);
+	block_size =
+		lpl_len - sizeof_field(struct cmis_cdb_write_fw_block_lpl_pl,
+				       block_address);
+
+	for (offset = start; offset < image_size; offset += block_size) {
+		struct cmis_cdb_write_fw_block_lpl_pl pl = {
+			.block_address = cpu_to_be32(offset - start),
+		};
+		struct ethtool_cmis_cdb_cmd_args args;
+
+		ethnl_module_fw_flash_ntf_in_progress(module_fw->dev,
+						      offset - start,
+						      image_size);
+
+		memcpy(pl.fw_block, &module_fw->fw->data[offset],
+		       min(block_size, image_size - offset));
+
+		ethtool_cmis_cdb_compose_args(&args,
+					      ETHTOOL_CMIS_CDB_CMD_WRITE_FW_BLOCK_LPL,
+					      (u8 *)&pl,
+					      min_t(u32, lpl_len, sizeof(pl)),
+					      fw_mng->max_duration_write,
+					      cdb->read_write_len_ext, 0,
+					      CDB_F_COMPLETION_VALID | CDB_F_STATUS_VALID);
+
+		err = ethtool_cmis_cdb_execute_cmd(module_fw->dev, &args);
+		if (err < 0) {
+			ethnl_module_fw_flash_ntf_err(module_fw->dev,
+						      "Write FW block LPL command failed");
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int
+cmis_fw_update_complete_download(struct ethtool_cmis_cdb *cdb,
+				 struct net_device *dev,
+				 struct cmis_fw_update_fw_mng_features *fw_mng)
+{
+	struct ethtool_cmis_cdb_cmd_args args;
+	int err;
+
+	ethtool_cmis_cdb_compose_args(&args,
+				      ETHTOOL_CMIS_CDB_CMD_COMPLETE_FW_DOWNLOAD,
+				      NULL, 0, fw_mng->max_duration_complete,
+				      cdb->read_write_len_ext, 0,
+				      CDB_F_COMPLETION_VALID | CDB_F_STATUS_VALID);
+
+	err = ethtool_cmis_cdb_execute_cmd(dev, &args);
+	if (err < 0)
+		ethnl_module_fw_flash_ntf_err(dev,
+					      "Complete FW download command failed");
+
+	return err;
+}
+
+static int
+cmis_fw_update_download_image(struct ethtool_cmis_cdb *cdb,
+			      struct ethtool_module_fw_flash *module_fw,
+			      struct cmis_fw_update_fw_mng_features *fw_mng)
+{
+	int err;
+
+	err = cmis_fw_update_start_download(cdb, module_fw, fw_mng);
+	if (err < 0)
+		return err;
+
+	err = cmis_fw_update_write_image(cdb, module_fw, fw_mng);
+	if (err < 0)
+		return err;
+
+	err = cmis_fw_update_complete_download(cdb, module_fw->dev, fw_mng);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+enum {
+	CMIS_MODULE_LOW_PWR	= 1,
+	CMIS_MODULE_READY	= 3,
+};
+
+static bool module_is_ready(u8 data)
+{
+	u8 state = (data >> 1) & 7;
+
+	return state == CMIS_MODULE_READY || state == CMIS_MODULE_LOW_PWR;
+}
+
+#define CMIS_MODULE_READY_MAX_DURATION_USEC	1000
+#define CMIS_MODULE_STATE_OFFSET		3
+
+static int
+cmis_fw_update_wait_for_module_state(struct ethtool_module_fw_flash *module_fw,
+				     u8 flags)
+{
+	return ethtool_cmis_wait_for_cond(module_fw->dev, flags,
+					  CDB_F_MODULE_STATE_VALID,
+					  CMIS_MODULE_READY_MAX_DURATION_USEC,
+					  CMIS_MODULE_STATE_OFFSET,
+					  module_is_ready, NULL);
+}
+
+/* See section 9.7.10 "CMD 0109h: Run Firmware Image" in CMIS standard
+ * revision 5.2.
+ * struct cmis_cdb_run_fw_image_pl is a structured layout of the flat
+ * array, ethtool_cmis_cdb_request::payload.
+ */
+struct cmis_cdb_run_fw_image_pl {
+	u8 resv1;
+	u8 image_to_run;
+	u16 delay_to_reset;
+};
+
+static int cmis_fw_update_run_image(struct ethtool_cmis_cdb *cdb,
+				    struct ethtool_module_fw_flash *module_fw)
+{
+	struct cmis_cdb_run_fw_image_pl pl = {0};
+	struct ethtool_cmis_cdb_cmd_args args;
+	int err;
+
+	ethtool_cmis_cdb_compose_args(&args, ETHTOOL_CMIS_CDB_CMD_RUN_FW_IMAGE,
+				      (u8 *)&pl, sizeof(pl),
+				      cdb->max_completion_time,
+				      cdb->read_write_len_ext, 0,
+				      CDB_F_MODULE_STATE_VALID);
+
+	err = ethtool_cmis_cdb_execute_cmd(module_fw->dev, &args);
+	if (err < 0) {
+		ethnl_module_fw_flash_ntf_err(module_fw->dev,
+					      "Run image command failed");
+		return err;
+	}
+
+	return cmis_fw_update_wait_for_module_state(module_fw, args.flags);
+}
+
+static int
+cmis_fw_update_commit_image(struct ethtool_cmis_cdb *cdb,
+			    struct ethtool_module_fw_flash *module_fw)
+{
+	struct ethtool_cmis_cdb_cmd_args args;
+	int err;
+
+	ethtool_cmis_cdb_compose_args(&args,
+				      ETHTOOL_CMIS_CDB_CMD_COMMIT_FW_IMAGE,
+				      NULL, 0, cdb->max_completion_time,
+				      cdb->read_write_len_ext, 0,
+				      CDB_F_COMPLETION_VALID | CDB_F_STATUS_VALID);
+
+	err = ethtool_cmis_cdb_execute_cmd(module_fw->dev, &args);
+	if (err < 0)
+		ethnl_module_fw_flash_ntf_err(module_fw->dev,
+					      "Commit image command failed");
+
+	return err;
+}
+
+void ethtool_cmis_fw_update(struct work_struct *work)
+{
+	struct cmis_fw_update_fw_mng_features fw_mng = {0};
+	struct ethtool_module_fw_flash *module_fw;
+	struct ethtool_cmis_cdb *cdb;
+	int err;
+
+	module_fw = container_of(work, struct ethtool_module_fw_flash, work);
+
+	cdb = ethtool_cmis_cdb_init(module_fw->dev, &module_fw->params);
+	if (IS_ERR(cdb))
+		goto err_cdb_init;
+
+	ethnl_module_fw_flash_ntf_start(module_fw->dev);
+
+	err = cmis_fw_update_fw_mng_features_get(cdb, module_fw->dev, &fw_mng);
+	if (err < 0)
+		goto err_fw_mng_features_get;
+
+	err = cmis_fw_update_download_image(cdb, module_fw, &fw_mng);
+	if (err < 0)
+		goto err_download_image;
+
+	err = cmis_fw_update_run_image(cdb, module_fw);
+	if (err < 0)
+		goto err_run_image;
+
+	/* The CDB command "Run Firmware Image" resets the firmware, so the new
+	 * one might have different settings.
+	 * Free the old CDB instance, and init a new one.
+	 */
+	ethtool_cmis_cdb_fini(cdb);
+
+	cdb = ethtool_cmis_cdb_init(module_fw->dev, &module_fw->params);
+	if (IS_ERR(cdb))
+		goto err_cdb_init;
+
+	err = cmis_fw_update_commit_image(cdb, module_fw);
+	if (err < 0)
+		goto err_commit_image;
+
+	ethnl_module_fw_flash_ntf_complete(module_fw->dev);
+	ethtool_cmis_cdb_fini(cdb);
+	goto out;
+
+err_commit_image:
+err_run_image:
+err_download_image:
+err_fw_mng_features_get:
+	ethtool_cmis_cdb_fini(cdb);
+err_cdb_init:
+	ethnl_module_fw_flash_ntf_err(module_fw->dev, NULL);
+out:
+	netdev_put(module_fw->dev, &module_fw->dev_tracker);
+	module_fw->dev->module_fw_flash_in_progress = false;
+	release_firmware(module_fw->fw);
+	kfree(module_fw);
+}
+EXPORT_SYMBOL_GPL(ethtool_cmis_fw_update);
diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h
index a4d4220db35d..969de116f995 100644
--- a/net/ethtool/module_fw.h
+++ b/net/ethtool/module_fw.h
@@ -9,6 +9,8 @@ void ethnl_module_fw_flash_ntf_complete(struct net_device *dev);
 void ethnl_module_fw_flash_ntf_in_progress(struct net_device *dev, u64 done,
 					   u64 total);
 
+void ethtool_cmis_fw_update(struct work_struct *work);
+
 /**
  * struct ethtool_module_fw_flash_params - module firmware flashing parameters
  * @file_name: Firmware image file name.
@@ -20,3 +22,19 @@ struct ethtool_module_fw_flash_params {
 	u32 password;
 	u8 password_valid:1;
 };
+
+/**
+ * struct ethtool_module_fw_flash - module firmware flashing
+ * @dev: Pointer to the net_device to be flashed.
+ * @dev_tracker: Refcount tracker for @dev.
+ * @params: Module firmware flashing parameters.
+ * @work: The flashing firmware work.
+ * @fw: Firmware to flash.
+ */
+struct ethtool_module_fw_flash {
+	struct net_device *dev;
+	netdevice_tracker dev_tracker;
+	struct ethtool_module_fw_flash_params params;
+	struct work_struct work;
+	const struct firmware *fw;
+};
-- 
2.40.1

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

[Danielle Ratson]

Add the ability to flash the modules' firmware by implementing the
interface between the user space and the kernel.

Example from a succeeding implementation:

 # ethtool --flash-module-firmware swp40 file test.bin

 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 completed for device eth0

Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---
 net/ethtool/module.c  | 179 ++++++++++++++++++++++++++++++++++++++++++
 net/ethtool/netlink.c |   7 ++
 net/ethtool/netlink.h |   2 +
 3 files changed, 188 insertions(+)

diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index 09cf11564840..69cedb3ede6d 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #include <linux/ethtool.h>
+#include <linux/sfp.h>
+#include <linux/firmware.h>
 
 #include "netlink.h"
 #include "common.h"
@@ -160,6 +162,183 @@ const struct ethnl_request_ops ethnl_module_request_ops = {
 	.set_ntf_cmd		= ETHTOOL_MSG_MODULE_NTF,
 };
 
+/* MODULE_FW_FLASH_ACT */
+
+const struct nla_policy
+ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD + 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_PASSWORD] = { .type = NLA_U32 },
+};
+
+struct module_sff8024_id_rpl {
+	u8 id;
+};
+
+#define MODULE_EEPROM_PAGE	0
+#define MODULE_EEPROM_OFFSET	0
+#define MODULE_EEPROM_LENGTH	1
+#define MODULE_EEPROM_I2C_ADDR	0x50
+
+static int module_flash_fw_work_init(struct ethtool_module_fw_flash *module_fw,
+				     struct net_device *dev,
+				     struct netlink_ext_ack *extack)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {};
+	struct module_sff8024_id_rpl *rpl;
+	int err;
+
+	/* Fetch the SFF-8024 Identifier Value. For all supported standards, it
+	 * is located at I2C address 0x50, byte 0. See section 4.1 in SFF-8024,
+	 * revision 4.9.
+	 */
+	page_data.page = MODULE_EEPROM_PAGE;
+	page_data.offset = MODULE_EEPROM_OFFSET;
+	page_data.length = MODULE_EEPROM_LENGTH;
+	page_data.i2c_address = MODULE_EEPROM_I2C_ADDR;
+	page_data.data = kmalloc(page_data.length, GFP_KERNEL);
+	if (!page_data.data)
+		return -ENOMEM;
+
+	err = ops->get_module_eeprom_by_page(dev, &page_data, extack);
+	if (err < 0)
+		goto out;
+
+	rpl = (struct module_sff8024_id_rpl *)page_data.data;
+	switch (rpl->id) {
+	case SFF8024_ID_QSFP_DD:
+	case SFF8024_ID_OSFP:
+	case SFF8024_ID_DSFP:
+	case SFF8024_ID_QSFP_PLUS_CMIS:
+	case SFF8024_ID_SFP_DD_CMIS:
+	case SFF8024_ID_SFP_PLUS_CMIS:
+		INIT_WORK(&module_fw->work, ethtool_cmis_fw_update);
+		goto out;
+	default:
+		NL_SET_ERR_MSG(extack,
+			       "Module type does not support firmware flashing");
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+out:
+	kfree(page_data.data);
+	return err;
+}
+
+static int
+module_flash_fw_schedule(struct net_device *dev,
+			 struct ethtool_module_fw_flash_params *params,
+			 struct netlink_ext_ack *extack)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_fw_flash *module_fw;
+	int err;
+
+	if (!ops->set_module_eeprom_by_page ||
+	    !ops->get_module_eeprom_by_page) {
+		NL_SET_ERR_MSG(extack,
+			       "Flashing module firmware is not supported by this device");
+		return -EOPNOTSUPP;
+	}
+
+	if (dev->module_fw_flash_in_progress) {
+		NL_SET_ERR_MSG(extack, "Module firmware flashing already in progress");
+		return -EBUSY;
+	}
+
+	module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL);
+	if (!module_fw)
+		return -ENOMEM;
+
+	module_fw->params = *params;
+	err = request_firmware(&module_fw->fw, module_fw->params.file_name,
+			       &dev->dev);
+	if (err) {
+		NL_SET_ERR_MSG(extack,
+			       "Failed to request module firmware image");
+		goto err_request_firmware;
+	}
+
+	err = module_flash_fw_work_init(module_fw, dev, extack);
+	if (err < 0) {
+		NL_SET_ERR_MSG(extack,
+			       "Flashing module firmware is not supported by this device");
+		goto err_work_init;
+	}
+
+	dev->module_fw_flash_in_progress = true;
+	netdev_hold(dev, &module_fw->dev_tracker, GFP_KERNEL);
+	module_fw->dev = dev;
+
+	schedule_work(&module_fw->work);
+
+	return 0;
+
+err_work_init:
+	release_firmware(module_fw->fw);
+err_request_firmware:
+	kfree(module_fw);
+	return err;
+}
+
+static int module_flash_fw(struct net_device *dev, struct nlattr **tb,
+			   struct netlink_ext_ack *extack)
+{
+	struct ethtool_module_fw_flash_params params = {};
+	struct nlattr *attr;
+
+	if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) {
+		NL_SET_ERR_MSG_ATTR(extack,
+				    tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME],
+				    "File name attribute is missing");
+		return -EINVAL;
+	}
+
+	params.file_name =
+		nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);
+
+	attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD];
+	if (attr) {
+		params.password = cpu_to_be32(nla_get_u32(attr));
+		params.password_valid = true;
+	}
+
+	return module_flash_fw_schedule(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();
+	ethnl_parse_header_dev_put(&req_info);
+	return ret;
+}
+
 /* MODULE_FW_FLASH_NTF */
 
 static void
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index fe3553f60bf3..85e27bdb1f73 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -1129,6 +1129,13 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_mm_set_policy,
 		.maxattr = ARRAY_SIZE(ethnl_mm_set_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 9a333a8d04c1..46712c9531ae 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -441,6 +441,7 @@ extern const struct nla_policy ethnl_plca_set_cfg_policy[ETHTOOL_A_PLCA_MAX + 1]
 extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADER + 1];
 extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
+extern const struct nla_policy ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
@@ -448,6 +449,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info);
 int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info);
 int ethnl_tunnel_info_start(struct netlink_callback *cb);
 int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
+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.40.1

Re: [RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

[Simon Horman]

On Mon, Jan 22, 2024 at 10:45:28AM +0200, Danielle Ratson wrote:

...

> +/**
> + * struct ethtool_cmis_cdb_request - CDB commands request fields as decribed in
> + *				the CMIS standard
> + * @id: Command ID.
> + * @epl_len: EPL memory length.
> + * @lpl_len: LPL memory length.
> + * @chk_code: Check code for the previous field and the payload.
> + * @resv1: Added to match the CMIS standard request continuity.
> + * @resv2: Added to match the CMIS standard request continuity.
> + * @payload: Payload for the CDB commands.
> + */
> +struct ethtool_cmis_cdb_request {
> +	__be16 id;
> +	struct_group(body,
> +		u16 epl_len;
> +		u8 lpl_len;
> +		u8 chk_code;
> +		u8 resv1;
> +		u8 resv2;
> +		u8 payload[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH];
> +	);
> +};
> +
> +#define CDB_F_COMPLETION_VALID		BIT(0)
> +#define CDB_F_STATUS_VALID		BIT(1)
> +
> +/**
> + * struct ethtool_cmis_cdb_cmd_args - CDB commands execution arguments
> + * @req: CDB command fields as described in the CMIS standard.
> + * @max_duration: Maximum duration time for command completion in msec.
> + * @read_write_len_ext: Allowable additional number of byte octets to the LPL
> + *			in a READ or a WRITE commands.
> + * @rpl_exp_len: Expected reply length in bytes.
> + * @flags: Validation flags for CDB commands.
> + */
> +struct ethtool_cmis_cdb_cmd_args {
> +	struct ethtool_cmis_cdb_request req;
> +	u16				max_duration;
> +	u8				read_write_len_ext;
> +	u8                              rpl_exp_len;
> +	u8				flags;
> +};

...

> +int ethtool_cmis_page_init(struct ethtool_module_eeprom *page_data,
> +			   u8 page, u32 offset, u32 length)
> +{
> +	page_data->page = page;
> +	page_data->offset = offset;
> +	page_data->length = length;
> +	page_data->i2c_address = ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR;
> +	page_data->data = kmalloc(page_data->length, GFP_KERNEL);
> +	if (!page_data->data)
> +		return -ENOMEM;
> +
> +	return 0;
> +}

...

> +static int
> +__ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
> +			       struct ethtool_module_eeprom *page_data,
> +			       u32 offset, u32 length, void *data)
> +{
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	struct netlink_ext_ack extack = {};
> +	int err;
> +
> +	page_data->offset = offset;
> +	page_data->length = length;
> +
> +	memset(page_data->data, 0, ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH);
> +	memcpy(page_data->data, data, page_data->length);
> +
> +	err = ops->set_module_eeprom_by_page(dev, page_data, &extack);
> +	if (err < 0) {
> +		if (extack._msg)
> +			netdev_err(dev, "%s\n", extack._msg);
> +	}
> +
> +	return err;
> +}

...

> +int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
> +				 struct ethtool_cmis_cdb_cmd_args *args)
> +{
> +	struct ethtool_module_eeprom page_data = {};
> +	u32 offset;
> +	int err;
> +
> +	args->req.chk_code =
> +		cmis_cdb_calc_checksum(&args->req, sizeof(args->req));
> +
> +	if (args->req.lpl_len > args->read_write_len_ext) {
> +		ethnl_module_fw_flash_ntf_err(dev,
> +					      "LPL length is longer than CDB read write length extension allows");
> +		return -EINVAL;
> +	}
> +
> +	err = ethtool_cmis_page_init(&page_data, ETHTOOL_CMIS_CDB_CMD_PAGE, 0,
> +				     ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH);

ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH is passed as the length argument
of ethtool_cmis_page_init, which will allocate that many
bytes for page_data->data.

> +	if (err < 0)
> +		return err;
> +
> +	/* According to the CMIS standard, there are two options to trigger the
> +	 * CDB commands. The default option is triggering the command by writing
> +	 * the CMDID bytes. Therefore, the command will be split to 2 calls:
> +	 * First, with everything except the CMDID field and then the CMDID
> +	 * field.
> +	 */
> +	offset = CMIS_CDB_CMD_ID_OFFSET +
> +		offsetof(struct ethtool_cmis_cdb_request, body);
> +	err = __ethtool_cmis_cdb_execute_cmd(dev, &page_data, offset,
> +					     sizeof(args->req.body),
> +					     &args->req.body);

Hi Danielle,

However, here sizeof(args->req.body) is passed as the length
argument of __ethtool_cmis_cdb_execute_cmd() which will:

1. Zero ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH bytes of page_data->data
2. Copy sizeof(args->req.body) bytes into page_data->data

args->req.body includes several fields, one of which is
ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH bytes long. So,
args->req.body > ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH
and it seems that step 2 above causes a buffer overrun.

Flagged by clang-17 W=1 build

 In file included from net/ethtool/cmis_cdb.c:3:
 In file included from ./include/linux/ethtool.h:16:
 In file included from ./include/linux/bitmap.h:12:
 In file included from ./include/linux/string.h:295:
 ./include/linux/fortify-string.h:579:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
   579 |                         __write_overflow_field(p_size_field, size);
       |                         ^
 ./include/linux/fortify-string.h:579:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
 ./include/linux/fortify-string.h:579:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]


> +	if (err < 0)
> +		goto out;
> +
> +	offset = CMIS_CDB_CMD_ID_OFFSET +
> +		offsetof(struct ethtool_cmis_cdb_request, id);
> +	err = __ethtool_cmis_cdb_execute_cmd(dev, &page_data, offset,
> +					     sizeof(args->req.id),
> +					     &args->req.id);
> +	if (err < 0)
> +		goto out;
> +
> +	err = cmis_cdb_wait_for_completion(dev, args);
> +	if (err < 0)
> +		goto out;
> +
> +	err = cmis_cdb_wait_for_status(dev, args);
> +	if (err < 0)
> +		goto out;
> +
> +	err = cmis_cdb_process_reply(dev, &page_data, args);
> +
> +out:
> +	ethtool_cmis_page_fini(&page_data);
> +	return err;
> +}

...

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

[Simon Horman]

On Mon, Jan 22, 2024 at 10:45:30AM +0200, Danielle Ratson wrote:

...

> +static int module_flash_fw(struct net_device *dev, struct nlattr **tb,
> +			   struct netlink_ext_ack *extack)
> +{
> +	struct ethtool_module_fw_flash_params params = {};
> +	struct nlattr *attr;
> +
> +	if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) {
> +		NL_SET_ERR_MSG_ATTR(extack,
> +				    tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME],
> +				    "File name attribute is missing");
> +		return -EINVAL;
> +	}
> +
> +	params.file_name =
> +		nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);
> +
> +	attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD];
> +	if (attr) {
> +		params.password = cpu_to_be32(nla_get_u32(attr));

Hi Danielle,

The type of password is u32, so perhaps cpu_to_be32() isn't needed here?

Flagged by Sparse.

> +		params.password_valid = true;
> +	}
> +
> +	return module_flash_fw_schedule(dev, &params, extack);
> +}

...

RE: [RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

[Danielle Ratson]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Monday, 22 January 2024 12:31
> To: Danielle Ratson <danieller@nvidia.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; corbet@lwn.net;
> linux@armlinux.org.uk; sdf@google.com; kory.maincent@bootlin.com;
> maxime.chevallier@bootlin.com; vladimir.oltean@nxp.com;
> przemyslaw.kitszel@intel.com; ahmed.zaki@intel.com;
> richardcochran@gmail.com; shayagr@amazon.com;
> paul.greenwalt@intel.com; jiri@resnulli.us; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; mlxsw <mlxsw@nvidia.com>; Petr Machata
> <petrm@nvidia.com>; Ido Schimmel <idosch@nvidia.com>
> Subject: Re: [RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
> 
> On Mon, Jan 22, 2024 at 10:45:28AM +0200, Danielle Ratson wrote:
> 
> ...
> 
> > +/**
> > + * struct ethtool_cmis_cdb_request - CDB commands request fields as
> decribed in
> > + *				the CMIS standard
> > + * @id: Command ID.
> > + * @epl_len: EPL memory length.
> > + * @lpl_len: LPL memory length.
> > + * @chk_code: Check code for the previous field and the payload.
> > + * @resv1: Added to match the CMIS standard request continuity.
> > + * @resv2: Added to match the CMIS standard request continuity.
> > + * @payload: Payload for the CDB commands.
> > + */
> > +struct ethtool_cmis_cdb_request {
> > +	__be16 id;
> > +	struct_group(body,
> > +		u16 epl_len;
> > +		u8 lpl_len;
> > +		u8 chk_code;
> > +		u8 resv1;
> > +		u8 resv2;
> > +		u8 payload[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH];
> > +	);
> > +};
> > +
> > +#define CDB_F_COMPLETION_VALID		BIT(0)
> > +#define CDB_F_STATUS_VALID		BIT(1)
> > +
> > +/**
> > + * struct ethtool_cmis_cdb_cmd_args - CDB commands execution
> > +arguments
> > + * @req: CDB command fields as described in the CMIS standard.
> > + * @max_duration: Maximum duration time for command completion in
> msec.
> > + * @read_write_len_ext: Allowable additional number of byte octets to the
> LPL
> > + *			in a READ or a WRITE commands.
> > + * @rpl_exp_len: Expected reply length in bytes.
> > + * @flags: Validation flags for CDB commands.
> > + */
> > +struct ethtool_cmis_cdb_cmd_args {
> > +	struct ethtool_cmis_cdb_request req;
> > +	u16				max_duration;
> > +	u8				read_write_len_ext;
> > +	u8                              rpl_exp_len;
> > +	u8				flags;
> > +};
> 
> ...
> 
> > +int ethtool_cmis_page_init(struct ethtool_module_eeprom *page_data,
> > +			   u8 page, u32 offset, u32 length) {
> > +	page_data->page = page;
> > +	page_data->offset = offset;
> > +	page_data->length = length;
> > +	page_data->i2c_address = ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR;
> > +	page_data->data = kmalloc(page_data->length, GFP_KERNEL);
> > +	if (!page_data->data)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int
> > +__ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
> > +			       struct ethtool_module_eeprom *page_data,
> > +			       u32 offset, u32 length, void *data) {
> > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > +	struct netlink_ext_ack extack = {};
> > +	int err;
> > +
> > +	page_data->offset = offset;
> > +	page_data->length = length;
> > +
> > +	memset(page_data->data, 0,
> ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH);
> > +	memcpy(page_data->data, data, page_data->length);
> > +
> > +	err = ops->set_module_eeprom_by_page(dev, page_data, &extack);
> > +	if (err < 0) {
> > +		if (extack._msg)
> > +			netdev_err(dev, "%s\n", extack._msg);
> > +	}
> > +
> > +	return err;
> > +}
> 
> ...
> 
> > +int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
> > +				 struct ethtool_cmis_cdb_cmd_args *args) {
> > +	struct ethtool_module_eeprom page_data = {};
> > +	u32 offset;
> > +	int err;
> > +
> > +	args->req.chk_code =
> > +		cmis_cdb_calc_checksum(&args->req, sizeof(args->req));
> > +
> > +	if (args->req.lpl_len > args->read_write_len_ext) {
> > +		ethnl_module_fw_flash_ntf_err(dev,
> > +					      "LPL length is longer than CDB read
> write length extension allows");
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = ethtool_cmis_page_init(&page_data,
> ETHTOOL_CMIS_CDB_CMD_PAGE, 0,
> > +
> ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH);
> 
> ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH is passed as the length argument
> of ethtool_cmis_page_init, which will allocate that many bytes for page_data-
> >data.
> 
> > +	if (err < 0)
> > +		return err;
> > +
> > +	/* According to the CMIS standard, there are two options to trigger
> the
> > +	 * CDB commands. The default option is triggering the command by
> writing
> > +	 * the CMDID bytes. Therefore, the command will be split to 2 calls:
> > +	 * First, with everything except the CMDID field and then the CMDID
> > +	 * field.
> > +	 */
> > +	offset = CMIS_CDB_CMD_ID_OFFSET +
> > +		offsetof(struct ethtool_cmis_cdb_request, body);
> > +	err = __ethtool_cmis_cdb_execute_cmd(dev, &page_data, offset,
> > +					     sizeof(args->req.body),
> > +					     &args->req.body);
> 
> Hi Danielle,
> 
> However, here sizeof(args->req.body) is passed as the length argument of
> __ethtool_cmis_cdb_execute_cmd() which will:
> 
> 1. Zero ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH bytes of page_data->data
> 2. Copy sizeof(args->req.body) bytes into page_data->data
> 
> args->req.body includes several fields, one of which is
> ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH bytes long. So,
> args->req.body > ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH
> and it seems that step 2 above causes a buffer overrun.
> 
> Flagged by clang-17 W=1 build
> 
>  In file included from net/ethtool/cmis_cdb.c:3:
>  In file included from ./include/linux/ethtool.h:16:
>  In file included from ./include/linux/bitmap.h:12:
>  In file included from ./include/linux/string.h:295:
>  ./include/linux/fortify-string.h:579:4: error: call to '__write_overflow_field'
> declared with 'warning' attribute: detected write beyond size of field (1st
> parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>    579 |                         __write_overflow_field(p_size_field, size);
>        |                         ^
>  ./include/linux/fortify-string.h:579:4: error: call to '__write_overflow_field'
> declared with 'warning' attribute: detected write beyond size of field (1st
> parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>  ./include/linux/fortify-string.h:579:4: error: call to '__write_overflow_field'
> declared with 'warning' attribute: detected write beyond size of field (1st
> parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
> 

Hi Simon,

Thanks for the feedback.

Ill fix that, something like:

diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
index b27e12871816..888b02e6eade 100644
--- a/net/ethtool/cmis_cdb.c
+++ b/net/ethtool/cmis_cdb.c
@@ -521,7 +521,7 @@ int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
        }

        err = ethtool_cmis_page_init(&page_data, ETHTOOL_CMIS_CDB_CMD_PAGE, 0,
-                                    ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH);
+                                    sizeof(args->req.body));
        if (err < 0)
                return err;

Danielle

> 
> > +	if (err < 0)
> > +		goto out;
> > +
> > +	offset = CMIS_CDB_CMD_ID_OFFSET +
> > +		offsetof(struct ethtool_cmis_cdb_request, id);
> > +	err = __ethtool_cmis_cdb_execute_cmd(dev, &page_data, offset,
> > +					     sizeof(args->req.id),
> > +					     &args->req.id);
> > +	if (err < 0)
> > +		goto out;
> > +
> > +	err = cmis_cdb_wait_for_completion(dev, args);
> > +	if (err < 0)
> > +		goto out;
> > +
> > +	err = cmis_cdb_wait_for_status(dev, args);
> > +	if (err < 0)
> > +		goto out;
> > +
> > +	err = cmis_cdb_process_reply(dev, &page_data, args);
> > +
> > +out:
> > +	ethtool_cmis_page_fini(&page_data);
> > +	return err;
> > +}
> 
> ...

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

[Danielle Ratson]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Monday, 22 January 2024 12:38
> To: Danielle Ratson <danieller@nvidia.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; corbet@lwn.net;
> linux@armlinux.org.uk; sdf@google.com; kory.maincent@bootlin.com;
> maxime.chevallier@bootlin.com; vladimir.oltean@nxp.com;
> przemyslaw.kitszel@intel.com; ahmed.zaki@intel.com;
> richardcochran@gmail.com; shayagr@amazon.com;
> paul.greenwalt@intel.com; jiri@resnulli.us; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; mlxsw <mlxsw@nvidia.com>; Petr Machata
> <petrm@nvidia.com>; Ido Schimmel <idosch@nvidia.com>
> Subject: Re: [RFC PATCH net-next 9/9] ethtool: Add ability to flash transceiver
> modules' firmware
> 
> On Mon, Jan 22, 2024 at 10:45:30AM +0200, Danielle Ratson wrote:
> 
> ...
> 
> > +static int module_flash_fw(struct net_device *dev, struct nlattr **tb,
> > +			   struct netlink_ext_ack *extack) {
> > +	struct ethtool_module_fw_flash_params params = {};
> > +	struct nlattr *attr;
> > +
> > +	if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) {
> > +		NL_SET_ERR_MSG_ATTR(extack,
> > +
> tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME],
> > +				    "File name attribute is missing");
> > +		return -EINVAL;
> > +	}
> > +
> > +	params.file_name =
> > +		nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);
> > +
> > +	attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD];
> > +	if (attr) {
> > +		params.password = cpu_to_be32(nla_get_u32(attr));
> 
> Hi Danielle,
> 
> The type of password is u32, so perhaps cpu_to_be32() isn't needed here?
> 
> Flagged by Sparse.

Hi Simon,

The cpu_to_be32() is actually needed here, without it the password parameter from user space is passed with wrong endianness.

Thanks,
Danielle

> 
> > +		params.password_valid = true;
> > +	}
> > +
> > +	return module_flash_fw_schedule(dev, &params, extack); }
> 
> ...

Re: [RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

[Simon Horman]

On Mon, Jan 22, 2024 at 02:35:59PM +0000, Danielle Ratson wrote:

...

> Hi Simon,
> 
> Thanks for the feedback.
> 
> Ill fix that, something like:
> 
> diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
> index b27e12871816..888b02e6eade 100644
> --- a/net/ethtool/cmis_cdb.c
> +++ b/net/ethtool/cmis_cdb.c
> @@ -521,7 +521,7 @@ int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
>         }
> 
>         err = ethtool_cmis_page_init(&page_data, ETHTOOL_CMIS_CDB_CMD_PAGE, 0,
> -                                    ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH);
> +                                    sizeof(args->req.body));
>         if (err < 0)
>                 return err;

Thanks Danielle,

I also think something like that should fix the problem.

Re: [RFC PATCH net-next 3/9] ethtool: Add an interface for flashing transceiver modules' firmware

[Jakub Kicinski]

On Mon, 22 Jan 2024 10:45:24 +0200 Danielle Ratson wrote:
> +        name: status
> +        type: u8

u32, also - it'd be nice to define the enum values in the spec and link
it here by
           enum: ...

> +      -
> +        name: status-msg
> +        type: string
> +      -
> +        name: done
> +        type: u64
> +      -
> +        name: total
> +        type: u64

perhaps u64 -> uint?

Re: [RFC PATCH net-next 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB

[Jakub Kicinski]

On Mon, 22 Jan 2024 10:45:29 +0200 Danielle Ratson wrote:
> +err_commit_image:
> +err_run_image:
> +err_download_image:
> +err_fw_mng_features_get:

name labels after the target

> +	ethtool_cmis_cdb_fini(cdb);
> +err_cdb_init:
> +	ethnl_module_fw_flash_ntf_err(module_fw->dev, NULL);
> +out:
> +	netdev_put(module_fw->dev, &module_fw->dev_tracker);
> +	module_fw->dev->module_fw_flash_in_progress = false;
> +	release_firmware(module_fw->fw);
> +	kfree(module_fw);
> +}
> +EXPORT_SYMBOL_GPL(ethtool_cmis_fw_update);

does this really need to be exported?

Re: [RFC PATCH net-next 3/9] ethtool: Add an interface for flashing transceiver modules' firmware

[Jakub Kicinski]

On Mon, 22 Jan 2024 10:45:24 +0200 Danielle Ratson wrote:
> +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.

We should state more explicitly that the op just starts the process,
and does not block. Looks like cable test already uses _ACT as a
suffix, is it based on some standard? Doesn't seem all that intuitive
to me (or at least less intuitive than calling it _START...)

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

[Jakub Kicinski]

On Mon, 22 Jan 2024 10:45:30 +0200 Danielle Ratson wrote:
>  #include <linux/ethtool.h>
> +#include <linux/sfp.h>
> +#include <linux/firmware.h>

alphabetical order, please

> +static int
> +module_flash_fw_schedule(struct net_device *dev,
> +			 struct ethtool_module_fw_flash_params *params,
> +			 struct netlink_ext_ack *extack)
> +{
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	struct ethtool_module_fw_flash *module_fw;
> +	int err;
> +
> +	if (!ops->set_module_eeprom_by_page ||
> +	    !ops->get_module_eeprom_by_page) {
> +		NL_SET_ERR_MSG(extack,
> +			       "Flashing module firmware is not supported by this device");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (dev->module_fw_flash_in_progress) {
> +		NL_SET_ERR_MSG(extack, "Module firmware flashing already in progress");
> +		return -EBUSY;
> +	}
> +
> +	module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL);
> +	if (!module_fw)
> +		return -ENOMEM;
> +
> +	module_fw->params = *params;
> +	err = request_firmware(&module_fw->fw, module_fw->params.file_name,

request_firmware_direct() ? I think udev timeout is 30 sec and we're
holding rtnl_lock.. I don't remember why we didn't use that in devlink

> +			       &dev->dev);
> +	if (err) {
> +		NL_SET_ERR_MSG(extack,
> +			       "Failed to request module firmware image");
> +		goto err_request_firmware;
> +	}
> +
> +	err = module_flash_fw_work_init(module_fw, dev, extack);
> +	if (err < 0) {
> +		NL_SET_ERR_MSG(extack,
> +			       "Flashing module firmware is not supported by this device");
> +		goto err_work_init;
> +	}
> +
> +	dev->module_fw_flash_in_progress = true;

What does this protect us from? 

> +static int module_flash_fw(struct net_device *dev, struct nlattr **tb,
> +			   struct netlink_ext_ack *extack)
> +{
> +	struct ethtool_module_fw_flash_params params = {};
> +	struct nlattr *attr;
> +
> +	if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) {
> +		NL_SET_ERR_MSG_ATTR(extack,

GENL_REQ_ATTR_CHECK, and you can check it in the caller,
before taking rtnl_lock.

> +				    tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME],
> +				    "File name attribute is missing");
> +		return -EINVAL;
> +	}
> +
> +	params.file_name =
> +		nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);

Hm. I think you copy the param struct by value to the work container.
nla_data() is in the skb which is going to get freed after _ACT returns.
So if anyone tries to access the name from the work it's going to UAF?

> +
> +	attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD];
> +	if (attr) {
> +		params.password = cpu_to_be32(nla_get_u32(attr));
> +		params.password_valid = true;
> +	}
> +
> +	return module_flash_fw_schedule(dev, &params, extack);
> +}

Re: [RFC PATCH net-next 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB

[Jakub Kicinski]

On Mon, 22 Jan 2024 10:45:29 +0200 Danielle Ratson wrote:
> +	netdev_put(module_fw->dev, &module_fw->dev_tracker);
> +	module_fw->dev->module_fw_flash_in_progress = false;

The other way around :) Once you release the reference you can't touch
dev at all.

RE: [RFC PATCH net-next 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB

[Danielle Ratson]

> > +err_commit_image:
> > +err_run_image:
> > +err_download_image:
> > +err_fw_mng_features_get:
> 
> name labels after the target
> 
> > +	ethtool_cmis_cdb_fini(cdb);
> > +err_cdb_init:
> > +	ethnl_module_fw_flash_ntf_err(module_fw->dev, NULL);
> > +out:
> > +	netdev_put(module_fw->dev, &module_fw->dev_tracker);
> > +	module_fw->dev->module_fw_flash_in_progress = false;
> > +	release_firmware(module_fw->fw);
> > +	kfree(module_fw);
> > +}
> > +EXPORT_SYMBOL_GPL(ethtool_cmis_fw_update);
> 
> does this really need to be exported?

It is actually can be dropped now, thanks.

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

[Danielle Ratson]

> >  #include <linux/ethtool.h>
> > +#include <linux/sfp.h>
> > +#include <linux/firmware.h>
> 
> alphabetical order, please

Ok.

> 
> > +static int
> > +module_flash_fw_schedule(struct net_device *dev,
> > +			 struct ethtool_module_fw_flash_params *params,
> > +			 struct netlink_ext_ack *extack)
> > +{
> > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > +	struct ethtool_module_fw_flash *module_fw;
> > +	int err;
> > +
> > +	if (!ops->set_module_eeprom_by_page ||
> > +	    !ops->get_module_eeprom_by_page) {
> > +		NL_SET_ERR_MSG(extack,
> > +			       "Flashing module firmware is not supported by
> this device");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (dev->module_fw_flash_in_progress) {
> > +		NL_SET_ERR_MSG(extack, "Module firmware flashing already
> in progress");
> > +		return -EBUSY;
> > +	}
> > +
> > +	module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL);
> > +	if (!module_fw)
> > +		return -ENOMEM;
> > +
> > +	module_fw->params = *params;
> > +	err = request_firmware(&module_fw->fw, module_fw-
> >params.file_name,
> 
> request_firmware_direct() ? I think udev timeout is 30 sec and we're holding
> rtnl_lock.. I don't remember why we didn't use that in devlink

Ok will change.

> 
> > +			       &dev->dev);
> > +	if (err) {
> > +		NL_SET_ERR_MSG(extack,
> > +			       "Failed to request module firmware image");
> > +		goto err_request_firmware;
> > +	}
> > +
> > +	err = module_flash_fw_work_init(module_fw, dev, extack);
> > +	if (err < 0) {
> > +		NL_SET_ERR_MSG(extack,
> > +			       "Flashing module firmware is not supported by
> this device");
> > +		goto err_work_init;
> > +	}
> > +
> > +	dev->module_fw_flash_in_progress = true;
> 
> What does this protect us from?

Currently, it protects us from flashing an in-progress-flashing-module.

> 
> > +static int module_flash_fw(struct net_device *dev, struct nlattr **tb,
> > +			   struct netlink_ext_ack *extack) {
> > +	struct ethtool_module_fw_flash_params params = {};
> > +	struct nlattr *attr;
> > +
> > +	if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) {
> > +		NL_SET_ERR_MSG_ATTR(extack,
> 
> GENL_REQ_ATTR_CHECK, and you can check it in the caller, before taking
> rtnl_lock.
> 

OK, np. The idea was to have module_flash_fw() that checks the attrs and extract them into params and ethnl_act_module_fw_flash() should be free from those checks.
But if so, maybe this separation is redundant and should combine the two?

> > +
> tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME],
> > +				    "File name attribute is missing");
> > +		return -EINVAL;
> > +	}
> > +
> > +	params.file_name =
> > +		nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);
> 
> Hm. I think you copy the param struct by value to the work container.
> nla_data() is in the skb which is going to get freed after _ACT returns.
> So if anyone tries to access the name from the work it's going to UAF?

The file_name parameter is not really needed inside the work. Once we called request_firmware_direct(), we have all that we need in module_fw->fw.
Do we still need to avoid that situation? If so, can you please suggest how?

> 
> > +
> > +	attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD];
> > +	if (attr) {
> > +		params.password = cpu_to_be32(nla_get_u32(attr));
> > +		params.password_valid = true;
> > +	}
> > +
> > +	return module_flash_fw_schedule(dev, &params, extack); }
> 

RE: [RFC PATCH net-next 3/9] ethtool: Add an interface for flashing transceiver modules' firmware

[Danielle Ratson]

> > +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.
> 
> We should state more explicitly that the op just starts the process, and does
> not block. Looks like cable test already uses _ACT as a suffix, is it based on
> some standard? Doesn't seem all that intuitive to me (or at least less intuitive
> than calling it _START...)

From Documentation/networking/ethtool-netlink.rst:
"
List of message types
=====================

All constants identifying message types use ``ETHTOOL_CMD_`` prefix and suffix
according to message purpose:

==============    ======================================
``_GET``          userspace request to retrieve data
``_SET``          userspace request to set data
``_ACT``          userspace request to perform an action
``_GET_REPLY``    kernel reply to a ``GET`` request
``_SET_REPLY``    kernel reply to a ``SET`` request
``_ACT_REPLY``    kernel reply to an ``ACT`` request
``_NTF``          kernel notification
==============    ======================================
"

So, it looks suitable to me.

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

[Jakub Kicinski]

On Tue, 23 Jan 2024 13:05:16 +0000 Danielle Ratson wrote:
> > GENL_REQ_ATTR_CHECK, and you can check it in the caller, before taking
> > rtnl_lock.
> >   
> 
> OK, np. The idea was to have module_flash_fw() that checks the attrs
> and extract them into params and ethnl_act_module_fw_flash() should
> be free from those checks. But if so, maybe this separation is
> redundant and should combine the two?

No strong preference, whatever looks better :)
To use GENL_REQ_ATTR_CHECK() I think you'll need to pass genl_info here.
You can either to that or move the validation.

> > > +  
> > tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME],  
> > > +				    "File name attribute is missing");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	params.file_name =
> > > +		nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);  
> > 
> > Hm. I think you copy the param struct by value to the work container.
> > nla_data() is in the skb which is going to get freed after _ACT returns.
> > So if anyone tries to access the name from the work it's going to UAF?  
> 
> The file_name parameter is not really needed inside the work. Once we
> called request_firmware_direct(), we have all that we need in
> module_fw->fw. Do we still need to avoid that situation? If so, can
> you please suggest how?

I'd pass it to module_flash_fw_schedule() as a separate argument, if it
doesn't have to be saved.

Re: [RFC PATCH net-next 3/9] ethtool: Add an interface for flashing transceiver modules' firmware

[Jakub Kicinski]

On Tue, 23 Jan 2024 13:34:18 +0000 Danielle Ratson wrote:
> > > +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.  
> > 
> > We should state more explicitly that the op just starts the process, and does
> > not block. Looks like cable test already uses _ACT as a suffix, is it based on
> > some standard? Doesn't seem all that intuitive to me (or at least less intuitive
> > than calling it _START...)  
> 
> From Documentation/networking/ethtool-netlink.rst:
> "
> List of message types
> =====================
> 
> All constants identifying message types use ``ETHTOOL_CMD_`` prefix and suffix
> according to message purpose:
> 
> ==============    ======================================
> ``_GET``          userspace request to retrieve data
> ``_SET``          userspace request to set data
> ``_ACT``          userspace request to perform an action
> ``_GET_REPLY``    kernel reply to a ``GET`` request
> ``_SET_REPLY``    kernel reply to a ``SET`` request
> ``_ACT_REPLY``    kernel reply to an ``ACT`` request
> ``_NTF``          kernel notification
> ==============    ======================================
> "
> 
> So, it looks suitable to me.

True, didn't see that. It's fine as a distinction of "doing something"
vs "setting configuration" but it doesn't express the fact that the
action is async. AFAIU cable test is also async, so that's fine.
We'll worry about it when some tries to add _ACT which isn't async.. 🤷️

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

[Russell King (Oracle)]

On Mon, Jan 22, 2024 at 10:45:30AM +0200, Danielle Ratson wrote:
> +#define MODULE_EEPROM_PAGE	0
> +#define MODULE_EEPROM_OFFSET	0
> +#define MODULE_EEPROM_LENGTH	1
> +#define MODULE_EEPROM_I2C_ADDR	0x50
> +
> +static int module_flash_fw_work_init(struct ethtool_module_fw_flash *module_fw,
> +				     struct net_device *dev,
> +				     struct netlink_ext_ack *extack)
> +{
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	struct ethtool_module_eeprom page_data = {};
> +	struct module_sff8024_id_rpl *rpl;
> +	int err;
> +
> +	/* Fetch the SFF-8024 Identifier Value. For all supported standards, it
> +	 * is located at I2C address 0x50, byte 0. See section 4.1 in SFF-8024,
> +	 * revision 4.9.
> +	 */
> +	page_data.page = MODULE_EEPROM_PAGE;
> +	page_data.offset = MODULE_EEPROM_OFFSET;
> +	page_data.length = MODULE_EEPROM_LENGTH;
> +	page_data.i2c_address = MODULE_EEPROM_I2C_ADDR;

Please use better names - these aren't any better than using integers.

Maybe use SFP_PHYS_ID for the offset?

> +	page_data.data = kmalloc(page_data.length, GFP_KERNEL);
> +	if (!page_data.data)
> +		return -ENOMEM;
> +
> +	err = ops->get_module_eeprom_by_page(dev, &page_data, extack);
> +	if (err < 0)
> +		goto out;
> +
> +	rpl = (struct module_sff8024_id_rpl *)page_data.data;

What purpose does this structure of a single byte serve? To me, it just
obfuscates the code.

	u8 phys_id;

	...
	page_data.offset = SFP_PHYS_ID;
	page_data.length = sizeof(phys_id);
	page_data.data = &phys_id;
	...
	switch (phys_id) {

will work just as well, and be more explicit about what's actually going
on here. It doesn't mean that I have to understand what this new
module_sff8024_id_rpl structure is. I can see that we're just getting
one byte which is the module physical ID.

You also then don't need to care about kfree()ing one byte of data
structure.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM

[Russell King (Oracle)]

On Mon, Jan 22, 2024 at 10:45:22AM +0200, Danielle Ratson wrote:
>  /**
> - * struct ethtool_module_eeprom - 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.
> + * struct ethtool_module_eeprom - plug-in module EEPROM read / write parameters
> + * @offset: Offset within the specified page, in bytes.
> + * @length: Number of bytes to read / write.
> + * @page: Page number.
> + * @bank: Bank number, if supported by EEPROM spec.

I suppose I should have reviewed the addition of this (I can't recall
whether I got the original or not.)

If one looks at SFF-8472, then the first 128 bytes of the EEPROM at
0x50 (0xA0 on the wire) are not paged. Whereas bytes 128..255 are the
paged bytes. Therefore, "offset within the specified page" can sensibly
be interpreted as referring to the EEPROM at 0x50, at an offset of
128 + offset.

Meanwhile, the actual implementation doesn't do that - the offset is
the offset from the beginning of the EEPROM, and offsets >= 128 access
the paged area.

What this means is that the parameter description here is basically
wrong, both before and after your change.

This really ought to be fixed so that we describe things correctly
rather than misleading people who read documentation. Otherwise, it's
a recipe for broken implementations... and it's also completely
pointless documenting it if the documentation is wrong.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 6/9] net: sfp: Add more extended compliance codes

[Russell King (Oracle)]

On Mon, Jan 22, 2024 at 10:45:27AM +0200, Danielle Ratson wrote:
> SFF-8024 is used to define various constants re-used in several SFF
> SFP-related specifications.
> 
> Add SFF-8024 extended compliance code definitions for CMIS compliant
> modules and use them in the next patch to determine the firmware flashing
> work.
> 
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

[Russell King (Oracle)]

On Mon, Jan 22, 2024 at 10:45:28AM +0200, Danielle Ratson wrote:
> +int ethtool_cmis_page_init(struct ethtool_module_eeprom *page_data,
> +			   u8 page, u32 offset, u32 length)
> +{
> +	page_data->page = page;
> +	page_data->offset = offset;
> +	page_data->length = length;
> +	page_data->i2c_address = ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR;
> +	page_data->data = kmalloc(page_data->length, GFP_KERNEL);
> +	if (!page_data->data)
> +		return -ENOMEM;

Hmm, so every use is forced to use kmalloc() even when it's just one
byte? That seems rather wasteful.

> +/* See section 9.4.1 "CMD 0040h: Module Features" in CMIS standard revision 5.2.
> + * struct cmis_cdb_module_features_rpl is structured layout of the flat
> + * array, ethtool_cmis_cdb_rpl::payload.
> + */
> +struct cmis_cdb_module_features_rpl {
> +	u8	resv1[CMIS_CDB_MODULE_FEATURES_RESV_DATA];
> +	__be16	max_completion_time;
> +};

Does this structure need to be packed? I would suggest it does to
ensure that the __be16 is correctly placed after the 34 bytes of u8.

Overall, I think the idea of always kmalloc()ing the data is a bad idea
at the moment. We have no implementations that DMA to/from this buffer,
and it means extra cycles spent, and an extra failure point each time
we want to do a CMIS command.

It also introduces extra complexity, where we could just be passing
a pointer to a function local variable or function local structure.

Unless we decide that the data pointer should be DMA-able from (in
which case, that needs documenting as such) then I would suggest
getting rid of the extra kmalloc()...kfree() bits.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [RFC PATCH net-next 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM

[Danielle Ratson]

> >  /**
> > - * struct ethtool_module_eeprom - 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.
> > + * struct ethtool_module_eeprom - plug-in module EEPROM read / write
> > + parameters
> > + * @offset: Offset within the specified page, in bytes.
> > + * @length: Number of bytes to read / write.
> > + * @page: Page number.
> > + * @bank: Bank number, if supported by EEPROM spec.
> 
> I suppose I should have reviewed the addition of this (I can't recall whether I
> got the original or not.)
> 
> If one looks at SFF-8472, then the first 128 bytes of the EEPROM at
> 0x50 (0xA0 on the wire) are not paged. Whereas bytes 128..255 are the paged
> bytes. Therefore, "offset within the specified page" can sensibly be
> interpreted as referring to the EEPROM at 0x50, at an offset of
> 128 + offset.
> 
> Meanwhile, the actual implementation doesn't do that - the offset is the
> offset from the beginning of the EEPROM, and offsets >= 128 access the
> paged area.
> 
> What this means is that the parameter description here is basically wrong,
> both before and after your change.
> 
> This really ought to be fixed so that we describe things correctly rather than
> misleading people who read documentation. Otherwise, it's a recipe for
> broken implementations... and it's also completely pointless documenting it if
> the documentation is wrong.
> 

Will rephrase.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Danielle Ratson]

> > +#define MODULE_EEPROM_PAGE	0
> > +#define MODULE_EEPROM_OFFSET	0
> > +#define MODULE_EEPROM_LENGTH	1
> > +#define MODULE_EEPROM_I2C_ADDR	0x50
> > +
> > +static int module_flash_fw_work_init(struct ethtool_module_fw_flash
> *module_fw,
> > +				     struct net_device *dev,
> > +				     struct netlink_ext_ack *extack) {
> > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > +	struct ethtool_module_eeprom page_data = {};
> > +	struct module_sff8024_id_rpl *rpl;
> > +	int err;
> > +
> > +	/* Fetch the SFF-8024 Identifier Value. For all supported standards, it
> > +	 * is located at I2C address 0x50, byte 0. See section 4.1 in SFF-8024,
> > +	 * revision 4.9.
> > +	 */
> > +	page_data.page = MODULE_EEPROM_PAGE;
> > +	page_data.offset = MODULE_EEPROM_OFFSET;
> > +	page_data.length = MODULE_EEPROM_LENGTH;
> > +	page_data.i2c_address = MODULE_EEPROM_I2C_ADDR;
> 
> Please use better names - these aren't any better than using integers.
> 
> Maybe use SFP_PHYS_ID for the offset?
> 
> > +	page_data.data = kmalloc(page_data.length, GFP_KERNEL);
> > +	if (!page_data.data)
> > +		return -ENOMEM;
> > +
> > +	err = ops->get_module_eeprom_by_page(dev, &page_data, extack);
> > +	if (err < 0)
> > +		goto out;
> > +
> > +	rpl = (struct module_sff8024_id_rpl *)page_data.data;
> 
> What purpose does this structure of a single byte serve? To me, it just
> obfuscates the code.
> 
> 	u8 phys_id;
> 
> 	...
> 	page_data.offset = SFP_PHYS_ID;
> 	page_data.length = sizeof(phys_id);
> 	page_data.data = &phys_id;
> 	...
> 	switch (phys_id) {
> 
> will work just as well, and be more explicit about what's actually going on
> here. It doesn't mean that I have to understand what this new
> module_sff8024_id_rpl structure is. I can see that we're just getting one byte
> which is the module physical ID.
> 
> You also then don't need to care about kfree()ing one byte of data structure.

OK, will change, thanks!

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Danielle Ratson]

> > > GENL_REQ_ATTR_CHECK, and you can check it in the caller, before
> > > taking rtnl_lock.
> > >
> >
> > OK, np. The idea was to have module_flash_fw() that checks the attrs
> > and extract them into params and ethnl_act_module_fw_flash() should be
> > free from those checks. But if so, maybe this separation is redundant
> > and should combine the two?
> 
> No strong preference, whatever looks better :) To use
> GENL_REQ_ATTR_CHECK() I think you'll need to pass genl_info here.
> You can either to that or move the validation.
> 
> > > > +
> > > tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME],
> > > > +				    "File name attribute is missing");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	params.file_name =
> > > > +		nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);
> > >
> > > Hm. I think you copy the param struct by value to the work container.
> > > nla_data() is in the skb which is going to get freed after _ACT returns.
> > > So if anyone tries to access the name from the work it's going to UAF?
> >
> > The file_name parameter is not really needed inside the work. Once we
> > called request_firmware_direct(), we have all that we need in
> > module_fw->fw. Do we still need to avoid that situation? If so, can
> > you please suggest how?
> 
> I'd pass it to module_flash_fw_schedule() as a separate argument, if it doesn't
> have to be saved.

It doesn’t make the module_fw->file_name attribute redundant?

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

[Jakub Kicinski]

On Wed, 24 Jan 2024 15:46:55 +0000 Danielle Ratson wrote:
> > > The file_name parameter is not really needed inside the work. Once we
> > > called request_firmware_direct(), we have all that we need in
> > > module_fw->fw. Do we still need to avoid that situation? If so, can
> > > you please suggest how?  
> > 
> > I'd pass it to module_flash_fw_schedule() as a separate argument, if it doesn't
> > have to be saved.  
> 
> It doesn’t make the module_fw->file_name attribute redundant?

This is what I mean:

diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index 69cedb3ede6d..ea048a7089d9 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -229,7 +229,7 @@ static int module_flash_fw_work_init(struct ethtool_module_fw_flash *module_fw,
 }
 
 static int
-module_flash_fw_schedule(struct net_device *dev,
+module_flash_fw_schedule(struct net_device *dev, const char *file_name,
 			 struct ethtool_module_fw_flash_params *params,
 			 struct netlink_ext_ack *extack)
 {
@@ -254,8 +254,7 @@ module_flash_fw_schedule(struct net_device *dev,
 		return -ENOMEM;
 
 	module_fw->params = *params;
-	err = request_firmware(&module_fw->fw, module_fw->params.file_name,
-			       &dev->dev);
+	err = request_firmware(&module_fw->fw, file_name, &dev->dev);
 	if (err) {
 		NL_SET_ERR_MSG(extack,
 			       "Failed to request module firmware image");
@@ -288,6 +287,7 @@ 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 char *file_name;
 	struct nlattr *attr;
 
 	if (!tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]) {
@@ -297,8 +297,7 @@ static int module_flash_fw(struct net_device *dev, struct nlattr **tb,
 		return -EINVAL;
 	}
 
-	params.file_name =
-		nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);
+	file_name = nla_data(tb[ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME]);
 
 	attr = tb[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD];
 	if (attr) {
@@ -306,7 +305,7 @@ static int module_flash_fw(struct net_device *dev, struct nlattr **tb,
 		params.password_valid = true;
 	}
 
-	return module_flash_fw_schedule(dev, &params, extack);
+	return module_flash_fw_schedule(dev, file_name, &params, extack);
 }
 
 int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info)
diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h
index 969de116f995..888f082f8daf 100644
--- a/net/ethtool/module_fw.h
+++ b/net/ethtool/module_fw.h
@@ -13,12 +13,10 @@ void ethtool_cmis_fw_update(struct work_struct *work);
 
 /**
  * struct ethtool_module_fw_flash_params - module firmware flashing parameters
- * @file_name: Firmware image file name.
  * @password: Module password. Only valid when @pass_valid is set.
  * @password_valid: Whether the module password is valid or not.
  */
 struct ethtool_module_fw_flash_params {
-	const char *file_name;
 	u32 password;
 	u8 password_valid:1;
 };

Re: [RFC PATCH net-next 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM

[Andrew Lunn]

On Mon, Jan 22, 2024 at 10:45:22AM +0200, Danielle Ratson wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> Ethtool can already retrieve information from a transceiver module
> EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page operation.
> Add a corresponding operation that allows ethtool to write to a
> transceiver module EEPROM.
> 
> The purpose of this operation is not to enable arbitrary read / write
> access, but to allow the kernel to write to specific addresses as part
> of transceiver module firmware flashing. In the future, more
> functionality can be implemented on top of these read / write
> operations.

My memory is dim, but i thought we decided that since the algorithm to
program these modules is defined in the standard, all we need to do is
pass the firmware blob, and have an in kernel implementation of the
algorithm. There is no need to have an arbitrary write blob to module,
which might, or might not be abused in the future.

Also, is the module functional while its firmware is being upgraded?
Do we need to enforce the link is down?

   Andrew

Re: [RFC PATCH net-next 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM

[Andrew Lunn]

On Thu, Jan 25, 2024 at 09:26:16PM +0100, Andrew Lunn wrote:
> On Mon, Jan 22, 2024 at 10:45:22AM +0200, Danielle Ratson wrote:
> > From: Ido Schimmel <idosch@nvidia.com>
> > 
> > Ethtool can already retrieve information from a transceiver module
> > EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page operation.
> > Add a corresponding operation that allows ethtool to write to a
> > transceiver module EEPROM.
> > 
> > The purpose of this operation is not to enable arbitrary read / write
> > access, but to allow the kernel to write to specific addresses as part
> > of transceiver module firmware flashing. In the future, more
> > functionality can be implemented on top of these read / write
> > operations.
> 
> My memory is dim, but i thought we decided that since the algorithm to
> program these modules is defined in the standard, all we need to do is
> pass the firmware blob, and have an in kernel implementation of the
> algorithm. There is no need to have an arbitrary write blob to module,
> which might, or might not be abused in the future.

O.K, back after reading more of the patches.

If i'm understanding the code correctly, this is never exposed to
userspace? Its purely an in kernel API? It would be good to make that
clear in the commit message, and document that in the ethtool ops
structure.

Thanks
      Andrew

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

[Andrew Lunn]

> +static int
> +module_flash_fw_schedule(struct net_device *dev,
> +			 struct ethtool_module_fw_flash_params *params,
> +			 struct netlink_ext_ack *extack)
> +{
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	struct ethtool_module_fw_flash *module_fw;
> +	int err;
> +
> +	if (!ops->set_module_eeprom_by_page ||
> +	    !ops->get_module_eeprom_by_page) {
> +		NL_SET_ERR_MSG(extack,
> +			       "Flashing module firmware is not supported by this device");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (dev->module_fw_flash_in_progress) {
> +		NL_SET_ERR_MSG(extack, "Module firmware flashing already in progress");
> +		return -EBUSY;
> +	}
> +
> +	module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL);
> +	if (!module_fw)
> +		return -ENOMEM;
> +
> +	module_fw->params = *params;
> +	err = request_firmware(&module_fw->fw, module_fw->params.file_name,
> +			       &dev->dev);

How big are these firmware blobs?

Ideally we want to be able to use the same API to upgrade things like
GPON modules, which often run an openwrt image, and they are plugged
into a cable modem which does not have too much RAM.

Given that the interface to the EEPROM is using 128 byte 1/2 pages,
would it be possible to use request_partial_firmware_into_buf() to
read it on demand, rather than all at once?

     Andrew

RE: [RFC PATCH net-next 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM

[Danielle Ratson]

> > On Mon, Jan 22, 2024 at 10:45:22AM +0200, Danielle Ratson wrote:
> > > From: Ido Schimmel <idosch@nvidia.com>
> > >
> > > Ethtool can already retrieve information from a transceiver module
> > > EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page
> operation.
> > > Add a corresponding operation that allows ethtool to write to a
> > > transceiver module EEPROM.
> > >
> > > The purpose of this operation is not to enable arbitrary read /
> > > write access, but to allow the kernel to write to specific addresses
> > > as part of transceiver module firmware flashing. In the future, more
> > > functionality can be implemented on top of these read / write
> > > operations.
> >
> > My memory is dim, but i thought we decided that since the algorithm to
> > program these modules is defined in the standard, all we need to do is
> > pass the firmware blob, and have an in kernel implementation of the
> > algorithm. There is no need to have an arbitrary write blob to module,
> > which might, or might not be abused in the future.
> 
> O.K, back after reading more of the patches.
> 
> If i'm understanding the code correctly, this is never exposed to userspace? Its
> purely an in kernel API? It would be good to make that clear in the commit
> message, and document that in the ethtool ops structure.
> 
> Thanks
>       Andrew

Hi Andrew,

Yes, that is correct.
Will add a clarification.

Thanks.

RE: [RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

[Danielle Ratson]

> > +int ethtool_cmis_page_init(struct ethtool_module_eeprom *page_data,
> > +			   u8 page, u32 offset, u32 length) {
> > +	page_data->page = page;
> > +	page_data->offset = offset;
> > +	page_data->length = length;
> > +	page_data->i2c_address = ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR;
> > +	page_data->data = kmalloc(page_data->length, GFP_KERNEL);
> > +	if (!page_data->data)
> > +		return -ENOMEM;
> 
> Hmm, so every use is forced to use kmalloc() even when it's just one byte?
> That seems rather wasteful.
> 
> > +/* See section 9.4.1 "CMD 0040h: Module Features" in CMIS standard
> revision 5.2.
> > + * struct cmis_cdb_module_features_rpl is structured layout of the
> > +flat
> > + * array, ethtool_cmis_cdb_rpl::payload.
> > + */
> > +struct cmis_cdb_module_features_rpl {
> > +	u8	resv1[CMIS_CDB_MODULE_FEATURES_RESV_DATA];
> > +	__be16	max_completion_time;
> > +};
> 
> Does this structure need to be packed? I would suggest it does to ensure that
> the __be16 is correctly placed after the 34 bytes of u8.
> 
> Overall, I think the idea of always kmalloc()ing the data is a bad idea at the
> moment. We have no implementations that DMA to/from this buffer, and it
> means extra cycles spent, and an extra failure point each time we want to do a
> CMIS command.
> 
> It also introduces extra complexity, where we could just be passing a pointer
> to a function local variable or function local structure.
> 
> Unless we decide that the data pointer should be DMA-able from (in which
> case, that needs documenting as such) then I would suggest getting rid of the
> extra kmalloc()...kfree() bits.
> 
> Thanks.
> 

Will fix, thanks!

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [RFC PATCH net-next 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM

[Danielle Ratson]

> > From: Ido Schimmel <idosch@nvidia.com>
> >
> > Ethtool can already retrieve information from a transceiver module
> > EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page
> operation.
> > Add a corresponding operation that allows ethtool to write to a
> > transceiver module EEPROM.
> >
> > The purpose of this operation is not to enable arbitrary read / write
> > access, but to allow the kernel to write to specific addresses as part
> > of transceiver module firmware flashing. In the future, more
> > functionality can be implemented on top of these read / write
> > operations.
> 
> My memory is dim, but i thought we decided that since the algorithm to
> program these modules is defined in the standard, all we need to do is pass
> the firmware blob, and have an in kernel implementation of the algorithm.
> There is no need to have an arbitrary write blob to module, which might, or
> might not be abused in the future.
> 
> Also, is the module functional while its firmware is being upgraded?
> Do we need to enforce the link is down?
> 
>    Andrew

This is part of the reasons why we kept a flag for module_fw_flash_in_progress. 
I think it should be down since the module is doing some sort of reset during the flashing process (after the Run Firmware Image).
So in order to avoid packet loss, this should be considered.

Ill consider the relevant scenarios for vetoing in the actual version.

Thanks.

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

[Danielle Ratson]

> Subject: Re: [RFC PATCH net-next 9/9] ethtool: Add ability to flash transceiver
> modules' firmware
> 
> > +static int
> > +module_flash_fw_schedule(struct net_device *dev,
> > +			 struct ethtool_module_fw_flash_params *params,
> > +			 struct netlink_ext_ack *extack)
> > +{
> > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > +	struct ethtool_module_fw_flash *module_fw;
> > +	int err;
> > +
> > +	if (!ops->set_module_eeprom_by_page ||
> > +	    !ops->get_module_eeprom_by_page) {
> > +		NL_SET_ERR_MSG(extack,
> > +			       "Flashing module firmware is not supported by
> this device");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (dev->module_fw_flash_in_progress) {
> > +		NL_SET_ERR_MSG(extack, "Module firmware flashing already
> in progress");
> > +		return -EBUSY;
> > +	}
> > +
> > +	module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL);
> > +	if (!module_fw)
> > +		return -ENOMEM;
> > +
> > +	module_fw->params = *params;
> > +	err = request_firmware(&module_fw->fw, module_fw-
> >params.file_name,
> > +			       &dev->dev);
> 
> How big are these firmware blobs?
> 
> Ideally we want to be able to use the same API to upgrade things like GPON
> modules, which often run an openwrt image, and they are plugged into a cable
> modem which does not have too much RAM.
> 
> Given that the interface to the EEPROM is using 128 byte 1/2 pages, would it
> be possible to use request_partial_firmware_into_buf() to read it on demand,
> rather than all at once?
> 
>      Andrew

OK, Ill handle that in the actual version.
Thanks.

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

[Danielle Ratson]

> > Subject: Re: [RFC PATCH net-next 9/9] ethtool: Add ability to flash
> > transceiver modules' firmware
> >
> > > +static int
> > > +module_flash_fw_schedule(struct net_device *dev,
> > > +			 struct ethtool_module_fw_flash_params *params,
> > > +			 struct netlink_ext_ack *extack) {
> > > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > > +	struct ethtool_module_fw_flash *module_fw;
> > > +	int err;
> > > +
> > > +	if (!ops->set_module_eeprom_by_page ||
> > > +	    !ops->get_module_eeprom_by_page) {
> > > +		NL_SET_ERR_MSG(extack,
> > > +			       "Flashing module firmware is not supported by
> > this device");
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +
> > > +	if (dev->module_fw_flash_in_progress) {
> > > +		NL_SET_ERR_MSG(extack, "Module firmware flashing already
> > in progress");
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL);
> > > +	if (!module_fw)
> > > +		return -ENOMEM;
> > > +
> > > +	module_fw->params = *params;
> > > +	err = request_firmware(&module_fw->fw, module_fw-
> > >params.file_name,
> > > +			       &dev->dev);
> >
> > How big are these firmware blobs?
> >

The largest file I came across is 400K.

> > Ideally we want to be able to use the same API to upgrade things like
> > GPON modules, which often run an openwrt image, and they are plugged
> > into a cable modem which does not have too much RAM.
> >
> > Given that the interface to the EEPROM is using 128 byte 1/2 pages,
> > would it be possible to use request_partial_firmware_into_buf() to
> > read it on demand, rather than all at once?
> >
> >      Andrew
> 
> OK, Ill handle that in the actual version.
> Thanks.

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

[Andrew Lunn]

> > > How big are these firmware blobs?
> > >
> 
> The largest file I came across is 400K.

https://hack-gpon.org/ont-fs-com-gpon-onu-stick-with-mac/

Suggests that some GPON devices have 16MB of flash. Ideally we don't
want to map a 16MB firmware image into the kernel address space. A
high end switch could do it, but a typical OpenWRT 'cable modem' is
likely to have trouble.

     Andrew

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

[Danielle Ratson]

> -----Original Message-----
> From: Danielle Ratson
> Sent: Wednesday, 31 January 2024 17:53
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; corbet@lwn.net;
> linux@armlinux.org.uk; sdf@google.com; kory.maincent@bootlin.com;
> maxime.chevallier@bootlin.com; vladimir.oltean@nxp.com;
> przemyslaw.kitszel@intel.com; ahmed.zaki@intel.com;
> richardcochran@gmail.com; shayagr@amazon.com;
> paul.greenwalt@intel.com; jiri@resnulli.us; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; mlxsw <mlxsw@nvidia.com>; Petr Machata
> <petrm@nvidia.com>; Ido Schimmel <idosch@nvidia.com>
> Subject: RE: [RFC PATCH net-next 9/9] ethtool: Add ability to flash transceiver
> modules' firmware
> 
> > > Subject: Re: [RFC PATCH net-next 9/9] ethtool: Add ability to flash
> > > transceiver modules' firmware
> > >
> > > > +static int
> > > > +module_flash_fw_schedule(struct net_device *dev,
> > > > +			 struct ethtool_module_fw_flash_params *params,
> > > > +			 struct netlink_ext_ack *extack) {
> > > > +	const struct ethtool_ops *ops = dev->ethtool_ops;
> > > > +	struct ethtool_module_fw_flash *module_fw;
> > > > +	int err;
> > > > +
> > > > +	if (!ops->set_module_eeprom_by_page ||
> > > > +	    !ops->get_module_eeprom_by_page) {
> > > > +		NL_SET_ERR_MSG(extack,
> > > > +			       "Flashing module firmware is not supported by
> > > this device");
> > > > +		return -EOPNOTSUPP;
> > > > +	}
> > > > +
> > > > +	if (dev->module_fw_flash_in_progress) {
> > > > +		NL_SET_ERR_MSG(extack, "Module firmware flashing already
> > > in progress");
> > > > +		return -EBUSY;
> > > > +	}
> > > > +
> > > > +	module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL);
> > > > +	if (!module_fw)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	module_fw->params = *params;
> > > > +	err = request_firmware(&module_fw->fw, module_fw-
> > > >params.file_name,
> > > > +			       &dev->dev);
> > >
> > > How big are these firmware blobs?
> > >
> 
> The largest file I came across is 400K.
> 
> > > Ideally we want to be able to use the same API to upgrade things
> > > like GPON modules, which often run an openwrt image, and they are
> > > plugged into a cable modem which does not have too much RAM.
> > >
> > > Given that the interface to the EEPROM is using 128 byte 1/2 pages,
> > > would it be possible to use request_partial_firmware_into_buf() to
> > > read it on demand, rather than all at once?
> > >
> > >      Andrew
> >
> > OK, Ill handle that in the actual version.
> > Thanks.

Hi Andrew,

Thanks again for your feedback.
I thought again about you comment, and this patchset adds support for flashing CMIS compliant modules only.
Later on, if it will be expanded to more modules, it will be more reasonable to add support like you have suggested to fit the new supported standard.
So, currently I think it is better to not add it to this specific patchset.

Thanks,
Danielle

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

[Andrew Lunn]

> Thanks again for your feedback.
> I thought again about you comment, and this patchset adds support for flashing CMIS compliant modules only.
> Later on, if it will be expanded to more modules, it will be more reasonable to add support like you have suggested to fit the new supported standard.
> So, currently I think it is better to not add it to this specific patchset.

O.K. It should not be a big change, and i doubt it has any major
performance impacts. I2C is not very fast, so we can probably get it
off the disk faster than it can be written to the device.

    Andrew