* [PATCH net-next v7 0/9] Add ability to flash modules' firmware
@ 2024-06-24 17:51 Danielle Ratson
2024-06-24 17:51 ` [PATCH net-next v7 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM Danielle Ratson
` (8 more replies)
0 siblings, 9 replies; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm, 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: Veto operations during flashing.
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.
v7:
Patch #4:
* Return -ENOMEM instead of PTR_ERR(attr) on
ethnl_module_fw_flash_ntf_put_err().
Patch #9:
* Fix Warning for not unlocking the spin_lock in the error flow
on module_flash_fw_work_list_add().
* Avoid the fall-through on ethnl_sock_priv_destroy().
v6:
* Squash some of the last patch to patch #5 and patch #9.
Patch #3:
* Add paragraph in .rst file.
Patch #4:
* Reserve '1' more place on SKB for NUL terminator in
the error message string.
* Add more prints on error flow, re-write the printing
function and add ethnl_module_fw_flash_ntf_put_err().
* Change the communication method so notification will be
sent in unicast instead of multicast.
* Add new 'struct ethnl_module_fw_flash_ntf_params' that holds
the relevant info for unicast communication and use it to
send notification to the specific socket.
* s/nla_put_u64_64bit/nla_put_uint/
Patch #7:
* In ethtool_cmis_cdb_init(), Use 'const' for the 'params'
parameter.
Patch #8:
* Add a list field to struct ethtool_module_fw_flash for
module_fw_flash_work_list that will be presented in the next
patch.
* Move ethtool_cmis_fw_update() cleaning to a new function that
will be represented in the next patch.
* Move some of the fields in struct ethtool_module_fw_flash to
a separate struct, so ethtool_cmis_fw_update() will get only
the relevant parameters for it.
* Edit the relevant functions to get the relevant params for
them.
* s/CMIS_MODULE_READY_MAX_DURATION_USEC/CMIS_MODULE_READY_MAX_DURATION_MSEC
Patch #9:
* Add a paragraph in the commit message.
* Rename labels in module_flash_fw_schedule().
* Add info to genl_sk_priv_*() and implement the relevant
callbacks, in order to handle properly a scenario of closing
the socket from user space before the work item was ended.
* Add a list the holds all the ethtool_module_fw_flash struct
that corresponds to the in progress work items.
* Add a new enum for the socket types.
* Use both above to identify a flashing socket, add it to the
list and when closing socket affect only the flashing type.
* Create a new function that will get the work item instead of
ethtool_cmis_fw_update().
* Edit the relevant functions to get the relevant params for
them.
* The new function will call the old ethtool_cmis_fw_update(),
and do the cleaning, so the existence of the list should be
completely isolated in module.c.
Danielle Ratson (7):
ethtool: Add an interface for flashing transceiver modules' firmware
ethtool: Add flashing transceiver modules' firmware notifications
ability
ethtool: Veto some operations during firmware flashing process
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 | 55 ++
Documentation/networking/ethtool-netlink.rst | 70 +++
.../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 | 20 +-
include/linux/netdevice.h | 4 +-
include/linux/sfp.h | 6 +
include/uapi/linux/ethtool.h | 18 +
include/uapi/linux/ethtool_netlink.h | 19 +
net/ethtool/Makefile | 2 +-
net/ethtool/cmis.h | 124 ++++
net/ethtool/cmis_cdb.c | 581 ++++++++++++++++++
net/ethtool/cmis_fw_update.c | 399 ++++++++++++
net/ethtool/eeprom.c | 6 +
net/ethtool/ioctl.c | 12 +
net/ethtool/module.c | 394 ++++++++++++
net/ethtool/module_fw.h | 75 +++
net/ethtool/netlink.c | 56 ++
net/ethtool/netlink.h | 16 +
tools/net/ynl/Makefile.deps | 3 +-
22 files changed, 1942 insertions(+), 11 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.45.0
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH net-next v7 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
@ 2024-06-24 17:51 ` Danielle Ratson
2024-06-24 19:01 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 2/9] mlxsw: Implement " Danielle Ratson
` (7 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm, 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 new write operation is purely an in-kernel API and is not exposed to
user space.
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>
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
include/linux/ethtool.h | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 6fd9107d3cc0..fa1a5d0e3213 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -504,17 +504,16 @@ struct ethtool_ts_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: When @offset is 0-127, it is used as an address to the Lower Memory
+ * (@page must be 0). Otherwise, it is used as an address to the
+ * Upper Memory.
+ * @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;
@@ -822,6 +821,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,
+ * from kernel space only. 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.
@@ -956,6 +957,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.45.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH net-next v7 2/9] mlxsw: Implement ethtool operation to write to a transceiver module EEPROM
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
2024-06-24 17:51 ` [PATCH net-next v7 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM Danielle Ratson
@ 2024-06-24 17:51 ` Danielle Ratson
2024-06-24 19:09 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 3/9] ethtool: Add an interface for flashing transceiver modules' firmware Danielle Ratson
` (6 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm
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 6c06b0592760..294e758f1067 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 f0ceb196a6ce..448263423e36 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 a755b0a901d3..c79da1411d33 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.45.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH net-next v7 3/9] ethtool: Add an interface for flashing transceiver modules' firmware
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
2024-06-24 17:51 ` [PATCH net-next v7 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM Danielle Ratson
2024-06-24 17:51 ` [PATCH net-next v7 2/9] mlxsw: Implement " Danielle Ratson
@ 2024-06-24 17:51 ` Danielle Ratson
2024-06-24 19:23 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 4/9] ethtool: Add flashing transceiver modules' firmware notifications ability Danielle Ratson
` (5 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm, 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>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
Notes:
v6:
* Add paragraph in .rst file.
v5:
* Modify tools/net/ynl/Makefile.deps so the ynl file will
include the ethtool.h changes.
* u64>uint for 'total' and 'done' attrs.
* Translate the enum from ethtool_netlink.h to YAML.
v4:
* s/is composed from/consists of/.
Documentation/netlink/specs/ethtool.yaml | 55 +++++++++++++++
Documentation/networking/ethtool-netlink.rst | 70 ++++++++++++++++++++
include/uapi/linux/ethtool.h | 18 +++++
include/uapi/linux/ethtool_netlink.h | 19 ++++++
tools/net/ynl/Makefile.deps | 3 +-
5 files changed, 164 insertions(+), 1 deletion(-)
diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 00dc61358be8..43c5f21e6096 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -20,6 +20,10 @@ definitions:
name: header-flags
type: flags
entries: [ compact-bitsets, omit-reply, stats ]
+ -
+ name: module-fw-flash-status
+ type: enum
+ entries: [ started, in_progress, completed, error ]
attribute-sets:
-
@@ -975,6 +979,32 @@ 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: status
+ type: u32
+ enum: module-fw-flash-status
+ -
+ name: status-msg
+ type: string
+ -
+ name: done
+ type: uint
+ -
+ name: total
+ type: uint
operations:
enum-model: directional
@@ -1730,3 +1760,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
+
+ event:
+ attributes:
+ - header
+ - status
+ - status-msg
+ - done
+ - total
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 160bfb0ae8ba..b73082d2bd52 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
@@ -2033,6 +2035,73 @@ 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 consists of 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.
+
+This message merely schedules the update process and returns immediately
+without blocking. The process then runs asynchronously.
+Since it can take several minutes to complete, during the update process
+notifications are emitted from the kernel to user space updating it about
+the status and progress.
+
+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`` | u32 | status |
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG`` | string | status message |
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_FLASH_DONE`` | uint | progress |
+ +---------------------------------------------------+--------+----------------+
+ | ``ETHTOOL_A_MODULE_FW_FLASH_TOTAL`` | uint | 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
===================
@@ -2139,4 +2208,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 8733a3117902..e011384c915c 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -877,6 +877,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 b49b804b9495..3748f018b985 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,
@@ -996,6 +998,23 @@ 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_STATUS, /* u32 */
+ ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG, /* string */
+ ETHTOOL_A_MODULE_FW_FLASH_DONE, /* uint */
+ ETHTOOL_A_MODULE_FW_FLASH_TOTAL, /* uint */
+
+ /* add new constants above here */
+ __ETHTOOL_A_MODULE_FW_FLASH_CNT,
+ ETHTOOL_A_MODULE_FW_FLASH_MAX = (__ETHTOOL_A_MODULE_FW_FLASH_CNT - 1)
+};
+
/* generic netlink info */
#define ETHTOOL_GENL_NAME "ethtool"
#define ETHTOOL_GENL_VERSION 1
diff --git a/tools/net/ynl/Makefile.deps b/tools/net/ynl/Makefile.deps
index f4e8eb79c1b8..dbdca32a1c61 100644
--- a/tools/net/ynl/Makefile.deps
+++ b/tools/net/ynl/Makefile.deps
@@ -16,7 +16,8 @@ get_hdr_inc=-D$(1) -include $(UAPI_PATH)/linux/$(2)
CFLAGS_devlink:=$(call get_hdr_inc,_LINUX_DEVLINK_H_,devlink.h)
CFLAGS_dpll:=$(call get_hdr_inc,_LINUX_DPLL_H,dpll.h)
-CFLAGS_ethtool:=$(call get_hdr_inc,_LINUX_ETHTOOL_NETLINK_H_,ethtool_netlink.h)
+CFLAGS_ethtool:=$(call get_hdr_inc,_LINUX_ETHTOOL_H,ethtool.h) \
+ $(call get_hdr_inc,_LINUX_ETHTOOL_NETLINK_H_,ethtool_netlink.h)
CFLAGS_handshake:=$(call get_hdr_inc,_LINUX_HANDSHAKE_H,handshake.h)
CFLAGS_mptcp_pm:=$(call get_hdr_inc,_LINUX_MPTCP_PM_H,mptcp_pm.h)
CFLAGS_netdev:=$(call get_hdr_inc,_LINUX_NETDEV_H,netdev.h)
--
2.45.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH net-next v7 4/9] ethtool: Add flashing transceiver modules' firmware notifications ability
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
` (2 preceding siblings ...)
2024-06-24 17:51 ` [PATCH net-next v7 3/9] ethtool: Add an interface for flashing transceiver modules' firmware Danielle Ratson
@ 2024-06-24 17:51 ` Danielle Ratson
2024-06-24 19:27 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 5/9] ethtool: Veto some operations during firmware flashing process Danielle Ratson
` (4 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm, 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>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
Notes:
v7:
* Return -ENOMEM instead of PTR_ERR(attr) on
ethnl_module_fw_flash_ntf_put_err()ץ
v6:
* Reserve '1' more place on SKB for NUL terminator in the error
message string.
* Add more prints on error flow, re-write the printing function
and add ethnl_module_fw_flash_ntf_put_err().
* Change the communication method so notification will be sent
in unicast instead of multicast.
* Add new 'struct ethnl_module_fw_flash_ntf_params' that holds
the relevant info for unicast communication and use it to send
notification to the specific socket.
* s/nla_put_u64_64bit/nla_put_uint/
v2:
* Increase err_msg length.
v6:
* Reserve '1' more place on SKB for NUL terminator in the error
message string.
* Add more prints on error flow, re-write the printing function
and add ethnl_module_fw_flash_ntf_put_err().
* Change the communication method so notification will be sent
in unicast instead of multicast.
* Add new 'struct ethnl_module_fw_flash_ntf_params' that holds
the relevant info for unicast communication and use it to send
notification to the specific socket.
* s/nla_put_u64_64bit/nla_put_uint/
v2:
* Increase err_msg length.
net/ethtool/module.c | 117 ++++++++++++++++++++++++++++++++++++++++
net/ethtool/module_fw.h | 31 +++++++++++
net/ethtool/netlink.c | 5 ++
net/ethtool/netlink.h | 1 +
4 files changed, 154 insertions(+)
create mode 100644 net/ethtool/module_fw.h
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index ceb575efc290..ba728b4a38a1 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,119 @@ 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 int
+ethnl_module_fw_flash_ntf_put_err(struct sk_buff *skb, char *err_msg,
+ char *sub_err_msg)
+{
+ int err_msg_len, sub_err_msg_len, total_len;
+ struct nlattr *attr;
+
+ if (!err_msg)
+ return 0;
+
+ err_msg_len = strlen(err_msg);
+ total_len = err_msg_len + 2; /* For period and NUL. */
+
+ if (sub_err_msg) {
+ sub_err_msg_len = strlen(sub_err_msg);
+ total_len += sub_err_msg_len + 2; /* For ", ". */
+ }
+
+ attr = nla_reserve(skb, ETHTOOL_A_MODULE_FW_FLASH_STATUS_MSG,
+ total_len);
+ if (!attr)
+ return -ENOMEM;
+
+ if (sub_err_msg)
+ sprintf(nla_data(attr), "%s, %s.", err_msg, sub_err_msg);
+ else
+ sprintf(nla_data(attr), "%s.", err_msg);
+
+ return 0;
+}
+
+static void
+ethnl_module_fw_flash_ntf(struct net_device *dev,
+ enum ethtool_module_fw_flash_status status,
+ struct ethnl_module_fw_flash_ntf_params *ntf_params,
+ char *err_msg, char *sub_err_msg,
+ u64 done, u64 total)
+{
+ struct sk_buff *skb;
+ void *hdr;
+ int ret;
+
+ if (ntf_params->closed_sock)
+ return;
+
+ skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+ if (!skb)
+ return;
+
+ hdr = ethnl_unicast_put(skb, ntf_params->portid, ntf_params->seq,
+ 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_u32(skb, ETHTOOL_A_MODULE_FW_FLASH_STATUS, status))
+ goto err_skb;
+
+ ret = ethnl_module_fw_flash_ntf_put_err(skb, err_msg, sub_err_msg);
+ if (ret < 0)
+ goto err_skb;
+
+ if (nla_put_uint(skb, ETHTOOL_A_MODULE_FW_FLASH_DONE, done))
+ goto err_skb;
+
+ if (nla_put_uint(skb, ETHTOOL_A_MODULE_FW_FLASH_TOTAL, total))
+ goto err_skb;
+
+ genlmsg_end(skb, hdr);
+ genlmsg_unicast(dev_net(dev), skb, ntf_params->portid);
+ return;
+
+err_skb:
+ nlmsg_free(skb);
+}
+
+void ethnl_module_fw_flash_ntf_err(struct net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *params,
+ char *err_msg, char *sub_err_msg)
+{
+ ethnl_module_fw_flash_ntf(dev, ETHTOOL_MODULE_FW_FLASH_STATUS_ERROR,
+ params, err_msg, sub_err_msg, 0, 0);
+}
+
+void
+ethnl_module_fw_flash_ntf_start(struct net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *params)
+{
+ ethnl_module_fw_flash_ntf(dev, ETHTOOL_MODULE_FW_FLASH_STATUS_STARTED,
+ params, NULL, NULL, 0, 0);
+}
+
+void
+ethnl_module_fw_flash_ntf_complete(struct net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *params)
+{
+ ethnl_module_fw_flash_ntf(dev, ETHTOOL_MODULE_FW_FLASH_STATUS_COMPLETED,
+ params, NULL, NULL, 0, 0);
+}
+
+void
+ethnl_module_fw_flash_ntf_in_progress(struct net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *params,
+ u64 done, u64 total)
+{
+ ethnl_module_fw_flash_ntf(dev,
+ ETHTOOL_MODULE_FW_FLASH_STATUS_IN_PROGRESS,
+ params, NULL, NULL, done, total);
+}
diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h
new file mode 100644
index 000000000000..ee4a291ac1d4
--- /dev/null
+++ b/net/ethtool/module_fw.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <uapi/linux/ethtool.h>
+
+/**
+ * struct ethnl_module_fw_flash_ntf_params - module firmware flashing
+ * notifications parameters
+ * @portid: Netlink portid of sender.
+ * @seq: Sequence number of sender.
+ * @closed_sock: Indicates whether the socket was closed from user space.
+ */
+struct ethnl_module_fw_flash_ntf_params {
+ u32 portid;
+ u32 seq;
+ bool closed_sock;
+};
+
+void
+ethnl_module_fw_flash_ntf_err(struct net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *params,
+ char *err_msg, char *sub_err_msg);
+void
+ethnl_module_fw_flash_ntf_start(struct net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *params);
+void
+ethnl_module_fw_flash_ntf_complete(struct net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *params);
+void
+ethnl_module_fw_flash_ntf_in_progress(struct net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *params,
+ u64 done, u64 total);
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index bd04f28d5cf4..393ce668fb04 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -239,6 +239,11 @@ void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd)
cmd);
}
+void *ethnl_unicast_put(struct sk_buff *skb, u32 portid, u32 seq, u8 cmd)
+{
+ return genlmsg_put(skb, portid, seq, ðtool_genl_family, 0, cmd);
+}
+
int ethnl_multicast(struct sk_buff *skb, struct net_device *dev)
{
return genlmsg_multicast_netns(ðtool_genl_family, dev_net(dev), skb,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 9a333a8d04c1..5e6c6a7b7adc 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -21,6 +21,7 @@ struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
void **ehdrp);
void *ethnl_dump_put(struct sk_buff *skb, struct netlink_callback *cb, u8 cmd);
void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd);
+void *ethnl_unicast_put(struct sk_buff *skb, u32 portid, u32 seq, u8 cmd);
int ethnl_multicast(struct sk_buff *skb, struct net_device *dev);
/**
--
2.45.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH net-next v7 5/9] ethtool: Veto some operations during firmware flashing process
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
` (3 preceding siblings ...)
2024-06-24 17:51 ` [PATCH net-next v7 4/9] ethtool: Add flashing transceiver modules' firmware notifications ability Danielle Ratson
@ 2024-06-24 17:51 ` Danielle Ratson
2024-06-24 19:31 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 6/9] net: sfp: Add more extended compliance codes Danielle Ratson
` (3 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm, Danielle Ratson
Some operations cannot be performed during the firmware flashing
process.
For example:
- Port must be down during the whole flashing process to avoid packet loss
while committing reset for example.
- Writing to EEPROM interrupts the flashing process, so operations like
ethtool dump, module reset, get and set power mode should be vetoed.
- Split port firmware flashing should be vetoed.
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 and use it
to prevent interruptions during the process.
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
Notes:
v6:
* Squash some of the vetoes from the last patch to this patch.
include/linux/netdevice.h | 4 +++-
net/ethtool/eeprom.c | 6 ++++++
net/ethtool/ioctl.c | 12 ++++++++++++
net/ethtool/netlink.c | 12 ++++++++++++
4 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f148a01dd1d1..43ec53fc0128 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1989,6 +1989,8 @@ enum netdev_reg_state {
*
* @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.
@@ -2373,7 +2375,7 @@ struct net_device {
bool proto_down;
bool threaded;
unsigned wol_enabled:1;
-
+ unsigned module_fw_flash_in_progress:1;
struct list_head net_notifier_list;
#if IS_ENABLED(CONFIG_MACSEC)
diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c
index 6209c3a9c8f7..f36811b3ecf1 100644
--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c
@@ -91,6 +91,12 @@ static int get_module_eeprom_by_page(struct net_device *dev,
{
const struct ethtool_ops *ops = dev->ethtool_ops;
+ if (dev->module_fw_flash_in_progress) {
+ NL_SET_ERR_MSG(extack,
+ "Module firmware flashing is in progress");
+ return -EBUSY;
+ }
+
if (dev->sfp_bus)
return sfp_get_module_eeprom_by_page(dev->sfp_bus, page_data, extack);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index e645d751a5e8..1cca372c0d80 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -658,6 +658,9 @@ static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
if (!dev->ethtool_ops->get_link_ksettings)
return -EOPNOTSUPP;
+ if (dev->module_fw_flash_in_progress)
+ return -EBUSY;
+
memset(&link_ksettings, 0, sizeof(link_ksettings));
err = dev->ethtool_ops->get_link_ksettings(dev, &link_ksettings);
if (err < 0)
@@ -1449,6 +1452,9 @@ static int ethtool_reset(struct net_device *dev, char __user *useraddr)
if (!dev->ethtool_ops->reset)
return -EOPNOTSUPP;
+ if (dev->module_fw_flash_in_progress)
+ return -EBUSY;
+
if (copy_from_user(&reset, useraddr, sizeof(reset)))
return -EFAULT;
@@ -2462,6 +2468,9 @@ int ethtool_get_module_info_call(struct net_device *dev,
const struct ethtool_ops *ops = dev->ethtool_ops;
struct phy_device *phydev = dev->phydev;
+ if (dev->module_fw_flash_in_progress)
+ return -EBUSY;
+
if (dev->sfp_bus)
return sfp_get_module_info(dev->sfp_bus, modinfo);
@@ -2499,6 +2508,9 @@ int ethtool_get_module_eeprom_call(struct net_device *dev,
const struct ethtool_ops *ops = dev->ethtool_ops;
struct phy_device *phydev = dev->phydev;
+ if (dev->module_fw_flash_in_progress)
+ return -EBUSY;
+
if (dev->sfp_bus)
return sfp_get_module_eeprom(dev->sfp_bus, ee, data);
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 393ce668fb04..a5907bbde427 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -765,10 +765,22 @@ static void ethnl_notify_features(struct netdev_notifier_info *info)
static int ethnl_netdev_event(struct notifier_block *this, unsigned long event,
void *ptr)
{
+ struct netdev_notifier_info *info = ptr;
+ struct netlink_ext_ack *extack;
+ struct net_device *dev;
+
+ dev = netdev_notifier_info_to_dev(info);
+ extack = netdev_notifier_info_to_extack(info);
+
switch (event) {
case NETDEV_FEAT_CHANGE:
ethnl_notify_features(ptr);
break;
+ case NETDEV_PRE_UP:
+ if (dev->module_fw_flash_in_progress) {
+ NL_SET_ERR_MSG(extack, "Can't set port up while flashing module firmware");
+ return NOTIFY_BAD;
+ }
}
return NOTIFY_DONE;
--
2.45.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH net-next v7 6/9] net: sfp: Add more extended compliance codes
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
` (4 preceding siblings ...)
2024-06-24 17:51 ` [PATCH net-next v7 5/9] ethtool: Veto some operations during firmware flashing process Danielle Ratson
@ 2024-06-24 17:51 ` Danielle Ratson
2024-06-24 19:31 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands Danielle Ratson
` (2 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm, 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>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
include/linux/sfp.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index a45da7eef9a2..b14be59550e3 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.45.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
` (5 preceding siblings ...)
2024-06-24 17:51 ` [PATCH net-next v7 6/9] net: sfp: Add more extended compliance codes Danielle Ratson
@ 2024-06-24 17:51 ` Danielle Ratson
2024-06-24 19:50 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB Danielle Ratson
2024-06-24 17:51 ` [PATCH net-next v7 9/9] ethtool: Add ability to flash transceiver modules' firmware Danielle Ratson
8 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm, 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>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
Notes:
v6:
* In ethtool_cmis_cdb_init(), Use 'const' for the params
parameter.
v5:
* Drop all the inline in cmis_cdb.c.
v4:
* Add kernel-doc for msleep_pre_rpl and err_msg.
v3:
* Use kmemdup() instead of kmalloc+memcpy.
v2:
* Define ethtool_cmis_cdb_request::epl_len to be __be16 instead
of u16.
net/ethtool/Makefile | 2 +-
net/ethtool/cmis.h | 117 ++++++++
net/ethtool/cmis_cdb.c | 581 ++++++++++++++++++++++++++++++++++++++++
net/ethtool/module_fw.h | 10 +
4 files changed, 709 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..295f5d0df915
--- /dev/null
+++ b/net/ethtool/cmis.h
@@ -0,0 +1,117 @@
+/* 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,
+ __be16 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.
+ * @msleep_pre_rpl: Waiting time before checking reply in msec.
+ * @rpl_exp_len: Expected reply length in bytes.
+ * @flags: Validation flags for CDB commands.
+ * @err_msg: Error message to be sent to user space.
+ */
+struct ethtool_cmis_cdb_cmd_args {
+ struct ethtool_cmis_cdb_request req;
+ u16 max_duration;
+ u8 read_write_len_ext;
+ u8 msleep_pre_rpl;
+ u8 rpl_exp_len;
+ u8 flags;
+ char *err_msg;
+};
+
+/**
+ * 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, u16 msleep_pre_rpl,
+ u8 rpl_exp_len, u8 flags);
+
+void ethtool_cmis_cdb_check_completion_flag(u8 cmis_rev, u8 *flags);
+
+void 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,
+ struct ethnl_module_fw_flash_ntf_params *ntf_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), u8 *state);
+
+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..ad7ac41bf679
--- /dev/null
+++ b/net/ethtool/cmis_cdb.c
@@ -0,0 +1,581 @@
+// 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, u16 msleep_pre_rpl,
+ 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->msleep_pre_rpl = msleep_pre_rpl;
+ args->rpl_exp_len = rpl_exp_len;
+ args->flags = flags;
+ args->err_msg = NULL;
+}
+
+void 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;
+}
+
+#define CMIS_REVISION_PAGE 0x00
+#define CMIS_REVISION_OFFSET 0x01
+
+struct cmis_rev_rpl {
+ u8 rev;
+};
+
+static 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 = {0};
+ struct netlink_ext_ack extack = {};
+ struct cmis_rev_rpl rpl = {};
+ int err;
+
+ ethtool_cmis_page_init(&page_data, CMIS_REVISION_PAGE,
+ CMIS_REVISION_OFFSET, sizeof(rpl));
+ page_data.data = (u8 *)&rpl;
+
+ err = ops->get_module_eeprom_by_page(dev, &page_data, &extack);
+ if (err < 0) {
+ if (extack._msg)
+ netdev_err(dev, "%s\n", extack._msg);
+ return err;
+ }
+
+ *rev_major = cmis_rev_rpl_major(&rpl);
+
+ return 0;
+}
+
+#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 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 cmis_cdb_advert_rpl rpl = {};
+ struct netlink_ext_ack extack = {};
+ int err;
+
+ ethtool_cmis_page_init(&page_data, CMIS_CDB_ADVERTISEMENT_PAGE,
+ CMIS_CDB_ADVERTISEMENT_OFFSET, sizeof(rpl));
+ page_data.data = (u8 *)&rpl;
+
+ err = ops->get_module_eeprom_by_page(dev, &page_data, &extack);
+ if (err < 0) {
+ if (extack._msg)
+ netdev_err(dev, "%s\n", extack._msg);
+ return err;
+ }
+
+ if (!cmis_cdb_advert_rpl_inst_supported(&rpl))
+ return -EOPNOTSUPP;
+
+ cdb->read_write_len_ext = rpl.read_write_len_ext;
+
+ return 0;
+}
+
+#define CMIS_PASSWORD_ENTRY_PAGE 0x00
+#define CMIS_PASSWORD_ENTRY_OFFSET 0x7A
+
+struct cmis_password_entry_pl {
+ __be32 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,
+ struct ethnl_module_fw_flash_ntf_params *ntf_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 ethtool_cmis_cdb_cmd_args args = {};
+ struct cmis_password_entry_pl pe_pl = {};
+ struct cmis_cdb_query_status_rpl *rpl;
+ struct netlink_ext_ack extack = {};
+ int err;
+
+ ethtool_cmis_page_init(&page_data, CMIS_PASSWORD_ENTRY_PAGE,
+ CMIS_PASSWORD_ENTRY_OFFSET, sizeof(pe_pl));
+ page_data.data = (u8 *)&pe_pl;
+
+ 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);
+ return err;
+ }
+
+ ethtool_cmis_cdb_compose_args(&args, ETHTOOL_CMIS_CDB_CMD_QUERY_STATUS,
+ (u8 *)&qs_pl, sizeof(qs_pl), 0,
+ cdb->read_write_len_ext, 1000,
+ 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, ntf_params,
+ "Query Status command failed",
+ args.err_msg);
+ return err;
+ }
+
+ rpl = (struct cmis_cdb_query_status_rpl *)args.req.payload;
+ if (!rpl->length || !rpl->status) {
+ ethnl_module_fw_flash_ntf_err(dev, ntf_params,
+ "Password was not accepted",
+ NULL);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/* 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.
+ */
+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 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 ethnl_module_fw_flash_ntf_params *ntf_params)
+{
+ struct ethtool_cmis_cdb_cmd_args args = {};
+ struct cmis_cdb_module_features_rpl *rpl;
+ 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,
+ 1000, sizeof(*rpl), flags);
+
+ err = ethtool_cmis_cdb_execute_cmd(dev, &args);
+ if (err < 0) {
+ ethnl_module_fw_flash_ntf_err(dev, ntf_params,
+ "Module Features command failed",
+ args.err_msg);
+ 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 ethnl_module_fw_flash_ntf_params *ntf_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, ntf_params,
+ "CMIS revision doesn't support module firmware flashing",
+ NULL);
+ 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, ntf_params);
+ if (err < 0)
+ goto err;
+ }
+
+ err = cmis_cdb_module_features_get(cdb, dev, ntf_params);
+ 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 & 0x40);
+}
+
+#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;
+}
+
+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),
+ u8 *state)
+{
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+ struct ethtool_module_eeprom page_data = {0};
+ struct cmis_wait_for_cond_rpl rpl = {};
+ struct netlink_ext_ack extack = {};
+ unsigned long end;
+ int err;
+
+ if (!(flags & flag))
+ return 0;
+
+ if (max_duration == 0)
+ max_duration = U16_MAX;
+
+ end = jiffies + msecs_to_jiffies(max_duration);
+ do {
+ ethtool_cmis_page_init(&page_data, 0, offset, sizeof(rpl));
+ page_data.data = (u8 *)&rpl;
+
+ 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;
+ }
+
+ if ((*cond_success)(rpl.state))
+ return 0;
+
+ if (*cond_fail && (*cond_fail)(rpl.state))
+ break;
+
+ msleep(20);
+ } while (time_before(jiffies, end));
+
+ *state = rpl.state;
+ return -EBUSY;
+}
+
+#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)
+{
+ u8 flag;
+ int err;
+
+ /* Some vendors demand waiting time before checking completion flag
+ * in some CDB commands.
+ */
+ msleep(args->msleep_pre_rpl);
+
+ err = ethtool_cmis_wait_for_cond(dev, args->flags,
+ CDB_F_COMPLETION_VALID,
+ args->max_duration,
+ CMIS_CDB_COMPLETION_FLAG_OFFSET,
+ is_completed, NULL, &flag);
+ if (err < 0)
+ args->err_msg = "Completion Flag did not set on time";
+
+ return err;
+}
+
+#define CMIS_CDB_STATUS_OFFSET 0x25
+
+static void cmis_cdb_status_fail_msg_get(u8 status, char **err_msg)
+{
+ switch (status) {
+ case 0b10000001:
+ *err_msg = "CDB Status is in progress: Busy capturing command";
+ break;
+ case 0b10000010:
+ *err_msg =
+ "CDB Status is in progress: Busy checking/validating command";
+ break;
+ case 0b10000011:
+ *err_msg = "CDB Status is in progress: Busy executing";
+ break;
+ case 0b01000000:
+ *err_msg = "CDB status failed: no specific failure";
+ break;
+ case 0b01000010:
+ *err_msg =
+ "CDB status failed: Parameter range error or parameter not supported";
+ break;
+ case 0b01000101:
+ *err_msg = "CDB status failed: CdbChkCode error";
+ break;
+ default:
+ *err_msg = "Unknown failure reason";
+ }
+};
+
+static int cmis_cdb_wait_for_status(struct net_device *dev,
+ struct ethtool_cmis_cdb_cmd_args *args)
+{
+ u8 status;
+ int err;
+
+ /* Some vendors demand waiting time before checking status in some
+ * CDB commands.
+ */
+ msleep(args->msleep_pre_rpl);
+
+ err = ethtool_cmis_wait_for_cond(dev, args->flags, CDB_F_STATUS_VALID,
+ args->max_duration,
+ CMIS_CDB_STATUS_OFFSET,
+ status_success, status_fail, &status);
+ if (err < 0 && !args->err_msg)
+ cmis_cdb_status_fail_msg_get(status, &args->err_msg);
+
+ return err;
+}
+
+#define CMIS_CDB_REPLY_OFFSET 0x86
+
+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);
+ u8 rpl_exp_len = args->rpl_exp_len + rpl_hdr_len;
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+ struct netlink_ext_ack extack = {};
+ struct ethtool_cmis_cdb_rpl *rpl;
+ int err;
+
+ if (!args->rpl_exp_len)
+ return 0;
+
+ ethtool_cmis_page_init(page_data, ETHTOOL_CMIS_CDB_CMD_PAGE,
+ CMIS_CDB_REPLY_OFFSET, rpl_exp_len);
+ 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) {
+ if (extack._msg)
+ netdev_err(dev, "%s\n", extack._msg);
+ goto out;
+ }
+
+ 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) {
+ err = -EIO;
+ goto out;
+ }
+
+ args->req.lpl_len = rpl->hdr.rpl_len;
+ memcpy(args->req.payload, rpl->payload, args->req.lpl_len);
+
+out:
+ kfree(page_data->data);
+ return err;
+}
+
+static int
+__ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
+ struct ethtool_module_eeprom *page_data,
+ u8 page, u32 offset, u32 length, void *data)
+{
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+ struct netlink_ext_ack extack = {};
+ int err;
+
+ ethtool_cmis_page_init(page_data, page, offset, length);
+ page_data->data = kmemdup(data, page_data->length, GFP_KERNEL);
+ if (!page_data->data)
+ return -ENOMEM;
+
+ err = ops->set_module_eeprom_by_page(dev, page_data, &extack);
+ if (err < 0) {
+ if (extack._msg)
+ netdev_err(dev, "%s\n", extack._msg);
+ }
+
+ kfree(page_data->data);
+ 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) {
+ args->err_msg = "LPL length is longer than CDB read write length extension allows";
+ return -EINVAL;
+ }
+
+ /* 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,
+ ETHTOOL_CMIS_CDB_CMD_PAGE, offset,
+ sizeof(args->req.body),
+ &args->req.body);
+ if (err < 0)
+ return err;
+
+ offset = CMIS_CDB_CMD_ID_OFFSET +
+ offsetof(struct ethtool_cmis_cdb_request, id);
+ err = __ethtool_cmis_cdb_execute_cmd(dev, &page_data,
+ ETHTOOL_CMIS_CDB_CMD_PAGE, offset,
+ sizeof(args->req.id),
+ &args->req.id);
+ if (err < 0)
+ return err;
+
+ err = cmis_cdb_wait_for_completion(dev, args);
+ if (err < 0)
+ return err;
+
+ err = cmis_cdb_wait_for_status(dev, args);
+ if (err < 0)
+ return err;
+
+ return cmis_cdb_process_reply(dev, &page_data, args);
+}
diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h
index ee4a291ac1d4..6c86d05ab6cf 100644
--- a/net/ethtool/module_fw.h
+++ b/net/ethtool/module_fw.h
@@ -15,6 +15,16 @@ struct ethnl_module_fw_flash_ntf_params {
bool closed_sock;
};
+/**
+ * struct ethtool_module_fw_flash_params - module firmware flashing parameters
+ * @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 {
+ __be32 password;
+ u8 password_valid:1;
+};
+
void
ethnl_module_fw_flash_ntf_err(struct net_device *dev,
struct ethnl_module_fw_flash_ntf_params *params,
--
2.45.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH net-next v7 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
` (6 preceding siblings ...)
2024-06-24 17:51 ` [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands Danielle Ratson
@ 2024-06-24 17:51 ` Danielle Ratson
2024-06-24 19:57 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 9/9] ethtool: Add ability to flash transceiver modules' firmware Danielle Ratson
8 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm, 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>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
Notes:
v6:
* Add a list field to struct ethtool_module_fw_flash for
module_fw_flash_work_list that will be presented in the next
patch.
* Move ethtool_cmis_fw_update() cleaning to a new function that
will be represented in the next patch.
* Move some of the fields in struct ethtool_module_fw_flash to a
separate struct, so ethtool_cmis_fw_update() will get only the
relevant parameters for it.
* Edit the relevant functions to get the relevant params for them.
* s/CMIS_MODULE_READY_MAX_DURATION_USEC/CMIS_MODULE_READY_MAX_DURATION_MSEC.
v2:
* Decrease msleep before querying completion flag in Write FW
Image command.
* Change the condition for failing when LPL is not supported.
* Re-write cmis_fw_update_write_image().
net/ethtool/Makefile | 2 +-
net/ethtool/cmis.h | 7 +
net/ethtool/cmis_fw_update.c | 399 +++++++++++++++++++++++++++++++++++
net/ethtool/module_fw.h | 31 +++
4 files changed, 438 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 295f5d0df915..e71cc3e1b7eb 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..ae4b4b28a601
--- /dev/null
+++ b/net/ethtool/cmis_fw_update.c
@@ -0,0 +1,399 @@
+// 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 ethnl_module_fw_flash_ntf_params *ntf_params)
+{
+ struct ethtool_cmis_cdb_cmd_args args = {};
+ struct cmis_cdb_fw_mng_features_rpl *rpl;
+ 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, 1000,
+ sizeof(*rpl), flags);
+
+ err = ethtool_cmis_cdb_execute_cmd(dev, &args);
+ if (err < 0) {
+ ethnl_module_fw_flash_ntf_err(dev, ntf_params,
+ "FW Management Features command failed",
+ args.err_msg);
+ 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, ntf_params,
+ "Write LPL is not supported",
+ NULL);
+ 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_cmis_fw_update_params *fw_update,
+ 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(fw_update->fw->size);
+ memcpy(pl.vendor_data, fw_update->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, 1000, 0,
+ CDB_F_COMPLETION_VALID | CDB_F_STATUS_VALID);
+
+ err = ethtool_cmis_cdb_execute_cmd(fw_update->dev, &args);
+ if (err < 0)
+ ethnl_module_fw_flash_ntf_err(fw_update->dev,
+ &fw_update->ntf_params,
+ "Start FW download command failed",
+ args.err_msg);
+
+ 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_cmis_fw_update_params *fw_update,
+ struct cmis_fw_update_fw_mng_features *fw_mng)
+{
+ u8 start = fw_mng->start_cmd_payload_size;
+ u32 offset, max_block_size, max_lpl_len;
+ u32 image_size = fw_update->fw->size;
+ int err;
+
+ max_lpl_len = min_t(u32,
+ ethtool_cmis_get_max_payload_size(cdb->read_write_len_ext),
+ ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH);
+ max_block_size =
+ max_lpl_len - sizeof_field(struct cmis_cdb_write_fw_block_lpl_pl,
+ block_address);
+
+ for (offset = start; offset < image_size; offset += max_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 = {};
+ u32 block_size, lpl_len;
+
+ ethnl_module_fw_flash_ntf_in_progress(fw_update->dev,
+ &fw_update->ntf_params,
+ offset - start,
+ image_size);
+ block_size = min_t(u32, max_block_size, image_size - offset);
+ memcpy(pl.fw_block, &fw_update->fw->data[offset], block_size);
+ lpl_len = block_size +
+ sizeof_field(struct cmis_cdb_write_fw_block_lpl_pl,
+ block_address);
+
+ ethtool_cmis_cdb_compose_args(&args,
+ ETHTOOL_CMIS_CDB_CMD_WRITE_FW_BLOCK_LPL,
+ (u8 *)&pl, lpl_len,
+ fw_mng->max_duration_write,
+ cdb->read_write_len_ext, 1, 0,
+ CDB_F_COMPLETION_VALID | CDB_F_STATUS_VALID);
+
+ err = ethtool_cmis_cdb_execute_cmd(fw_update->dev, &args);
+ if (err < 0) {
+ ethnl_module_fw_flash_ntf_err(fw_update->dev,
+ &fw_update->ntf_params,
+ "Write FW block LPL command failed",
+ args.err_msg);
+ 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 ethnl_module_fw_flash_ntf_params *ntf_params)
+{
+ 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, 1000, 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, ntf_params,
+ "Complete FW download command failed",
+ args.err_msg);
+
+ return err;
+}
+
+static int
+cmis_fw_update_download_image(struct ethtool_cmis_cdb *cdb,
+ struct ethtool_cmis_fw_update_params *fw_update,
+ struct cmis_fw_update_fw_mng_features *fw_mng)
+{
+ int err;
+
+ err = cmis_fw_update_start_download(cdb, fw_update, fw_mng);
+ if (err < 0)
+ return err;
+
+ err = cmis_fw_update_write_image(cdb, fw_update, fw_mng);
+ if (err < 0)
+ return err;
+
+ err = cmis_fw_update_complete_download(cdb, fw_update->dev, fw_mng,
+ &fw_update->ntf_params);
+ 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_MSEC 1000
+#define CMIS_MODULE_STATE_OFFSET 3
+
+static int
+cmis_fw_update_wait_for_module_state(struct net_device *dev, u8 flags)
+{
+ u8 state;
+
+ return ethtool_cmis_wait_for_cond(dev, flags, CDB_F_MODULE_STATE_VALID,
+ CMIS_MODULE_READY_MAX_DURATION_MSEC,
+ CMIS_MODULE_STATE_OFFSET,
+ module_is_ready, NULL, &state);
+}
+
+/* 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 net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *ntf_params)
+{
+ struct ethtool_cmis_cdb_cmd_args args = {};
+ struct cmis_cdb_run_fw_image_pl pl = {0};
+ 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, 1000, 0,
+ CDB_F_MODULE_STATE_VALID);
+
+ err = ethtool_cmis_cdb_execute_cmd(dev, &args);
+ if (err < 0) {
+ ethnl_module_fw_flash_ntf_err(dev, ntf_params,
+ "Run image command failed",
+ args.err_msg);
+ return err;
+ }
+
+ err = cmis_fw_update_wait_for_module_state(dev, args.flags);
+ if (err < 0)
+ ethnl_module_fw_flash_ntf_err(dev, ntf_params,
+ "Module is not ready on time after reset",
+ NULL);
+
+ return err;
+}
+
+static int
+cmis_fw_update_commit_image(struct ethtool_cmis_cdb *cdb,
+ struct net_device *dev,
+ struct ethnl_module_fw_flash_ntf_params *ntf_params)
+{
+ 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, 1000, 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, ntf_params,
+ "Commit image command failed",
+ args.err_msg);
+
+ return err;
+}
+
+static int cmis_fw_update_reset(struct net_device *dev)
+{
+ __u32 reset_data = ETH_RESET_PHY;
+
+ return dev->ethtool_ops->reset(dev, &reset_data);
+}
+
+void
+ethtool_cmis_fw_update(struct ethtool_cmis_fw_update_params *fw_update)
+{
+ struct ethnl_module_fw_flash_ntf_params *ntf_params =
+ &fw_update->ntf_params;
+ struct cmis_fw_update_fw_mng_features fw_mng = {0};
+ struct net_device *dev = fw_update->dev;
+ struct ethtool_cmis_cdb *cdb;
+ int err;
+
+ cdb = ethtool_cmis_cdb_init(dev, &fw_update->params, ntf_params);
+ if (IS_ERR(cdb))
+ goto err_send_ntf;
+
+ ethnl_module_fw_flash_ntf_start(dev, ntf_params);
+
+ err = cmis_fw_update_fw_mng_features_get(cdb, dev, &fw_mng, ntf_params);
+ if (err < 0)
+ goto err_cdb_fini;
+
+ err = cmis_fw_update_download_image(cdb, fw_update, &fw_mng);
+ if (err < 0)
+ goto err_cdb_fini;
+
+ err = cmis_fw_update_run_image(cdb, dev, ntf_params);
+ if (err < 0)
+ goto err_cdb_fini;
+
+ /* 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(dev, &fw_update->params, ntf_params);
+ if (IS_ERR(cdb))
+ goto err_send_ntf;
+
+ err = cmis_fw_update_commit_image(cdb, dev, ntf_params);
+ if (err < 0)
+ goto err_cdb_fini;
+
+ err = cmis_fw_update_reset(dev);
+ if (err < 0)
+ goto err_cdb_fini;
+
+ ethnl_module_fw_flash_ntf_complete(dev, ntf_params);
+ ethtool_cmis_cdb_fini(cdb);
+ return;
+
+err_cdb_fini:
+ ethtool_cmis_cdb_fini(cdb);
+err_send_ntf:
+ ethnl_module_fw_flash_ntf_err(dev, ntf_params, NULL, NULL);
+}
diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h
index 6c86d05ab6cf..d0fc2529b60e 100644
--- a/net/ethtool/module_fw.h
+++ b/net/ethtool/module_fw.h
@@ -25,6 +25,35 @@ struct ethtool_module_fw_flash_params {
u8 password_valid:1;
};
+/**
+ * struct ethtool_cmis_fw_update_params - CMIS firmware update specific
+ * parameters
+ * @dev: Pointer to the net_device to be flashed.
+ * @params: Module firmware flashing parameters.
+ * @ntf_params: Module firmware flashing notification parameters.
+ * @fw: Firmware to flash.
+ */
+struct ethtool_cmis_fw_update_params {
+ struct net_device *dev;
+ struct ethtool_module_fw_flash_params params;
+ struct ethnl_module_fw_flash_ntf_params ntf_params;
+ const struct firmware *fw;
+};
+
+/**
+ * struct ethtool_module_fw_flash - module firmware flashing
+ * @list: List node for &module_fw_flash_work_list.
+ * @dev_tracker: Refcount tracker for @dev.
+ * @work: The flashing firmware work.
+ * @fw_update: CMIS firmware update specific parameters.
+ */
+struct ethtool_module_fw_flash {
+ struct list_head list;
+ netdevice_tracker dev_tracker;
+ struct work_struct work;
+ struct ethtool_cmis_fw_update_params fw_update;
+};
+
void
ethnl_module_fw_flash_ntf_err(struct net_device *dev,
struct ethnl_module_fw_flash_ntf_params *params,
@@ -39,3 +68,5 @@ void
ethnl_module_fw_flash_ntf_in_progress(struct net_device *dev,
struct ethnl_module_fw_flash_ntf_params *params,
u64 done, u64 total);
+
+void ethtool_cmis_fw_update(struct ethtool_cmis_fw_update_params *params);
--
2.45.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH net-next v7 9/9] ethtool: Add ability to flash transceiver modules' firmware
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
` (7 preceding siblings ...)
2024-06-24 17:51 ` [PATCH net-next v7 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB Danielle Ratson
@ 2024-06-24 17:51 ` Danielle Ratson
8 siblings, 0 replies; 25+ messages in thread
From: Danielle Ratson @ 2024-06-24 17:51 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, corbet, linux, sdf, kory.maincent,
maxime.chevallier, vladimir.oltean, przemyslaw.kitszel,
ahmed.zaki, richardcochran, shayagr, paul.greenwalt, jiri,
linux-doc, linux-kernel, mlxsw, idosch, petrm, 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 swp40
Transceiver module firmware flashing in progress for device swp40
Progress: 99%
Transceiver module firmware flashing completed for device swp40
In addition, add infrastructure that allows modules to set socket-specific
private data. This ensures that when a socket is closed from user space
during the flashing process, the right socket halts sending notifications
to user space until the work item is completed.
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
Notes:
v7:
* Fix Warning for not unlocking the spin_lock in the error flow
on module_flash_fw_work_list_add().
* Avoid the fall-through on ethnl_sock_priv_destroy().
v6:
* Add a paragraph in the commit message.
* Rename labels in module_flash_fw_schedule().
* Add info to genl_sk_priv_*() and implement the relevant
callbacks, in order to handle properly a scenario of closing the
socket from user space before the work item was ended.
* Add a list the holds all the ethtool_module_fw_flash struct
that corresponds to the in progress work items.
* Add a new enum for the socket types.
* Use both above to identify a flashing socket, add it to the
list and when closing socket affect only the flashing type.
* Create a new function that will get the work item instead of
ethtool_cmis_fw_update().
* Edit the relevant functions to get the relevant params for them.
* The new function will call the old ethtool_cmis_fw_update(), and do
the cleaning, so the existence of the list should be completely
isolated in module.c.
net/ethtool/module.c | 277 ++++++++++++++++++++++++++++++++++++++++
net/ethtool/module_fw.h | 3 +
net/ethtool/netlink.c | 39 ++++++
net/ethtool/netlink.h | 15 +++
4 files changed, 334 insertions(+)
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index ba728b4a38a1..6b7448df08d5 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -1,6 +1,9 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/ethtool.h>
+#include <linux/firmware.h>
+#include <linux/sfp.h>
+#include <net/devlink.h>
#include "netlink.h"
#include "common.h"
@@ -34,6 +37,12 @@ static int module_get_power_mode(struct net_device *dev,
if (!ops->get_module_power_mode)
return 0;
+ if (dev->module_fw_flash_in_progress) {
+ NL_SET_ERR_MSG(extack,
+ "Module firmware flashing is in progress");
+ return -EBUSY;
+ }
+
return ops->get_module_power_mode(dev, &data->power, extack);
}
@@ -110,6 +119,12 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info,
if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY])
return 0;
+ if (req_info->dev->module_fw_flash_in_progress) {
+ NL_SET_ERR_MSG(info->extack,
+ "Module firmware flashing is in progress");
+ return -EBUSY;
+ }
+
if (!ops->get_module_power_mode || !ops->set_module_power_mode) {
NL_SET_ERR_MSG_ATTR(info->extack,
tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY],
@@ -160,6 +175,268 @@ 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 },
+};
+
+static LIST_HEAD(module_fw_flash_work_list);
+static DEFINE_SPINLOCK(module_fw_flash_work_list_lock);
+
+static int
+module_flash_fw_work_list_add(struct ethtool_module_fw_flash *module_fw,
+ struct genl_info *info)
+{
+ struct ethtool_module_fw_flash *work;
+
+ /* First, check if already registered. */
+ spin_lock(&module_fw_flash_work_list_lock);
+ list_for_each_entry(work, &module_fw_flash_work_list, list) {
+ if (work->fw_update.ntf_params.portid == info->snd_portid &&
+ work->fw_update.dev == module_fw->fw_update.dev) {
+ spin_unlock(&module_fw_flash_work_list_lock);
+ return -EALREADY;
+ }
+ }
+
+ list_add_tail(&module_fw->list, &module_fw_flash_work_list);
+ spin_unlock(&module_fw_flash_work_list_lock);
+
+ return 0;
+}
+
+static void module_flash_fw_work_list_del(struct list_head *list)
+{
+ spin_lock(&module_fw_flash_work_list_lock);
+ list_del(list);
+ spin_unlock(&module_fw_flash_work_list_lock);
+}
+
+static void module_flash_fw_work(struct work_struct *work)
+{
+ struct ethtool_module_fw_flash *module_fw;
+
+ module_fw = container_of(work, struct ethtool_module_fw_flash, work);
+
+ ethtool_cmis_fw_update(&module_fw->fw_update);
+
+ module_flash_fw_work_list_del(&module_fw->list);
+ module_fw->fw_update.dev->module_fw_flash_in_progress = false;
+ netdev_put(module_fw->fw_update.dev, &module_fw->dev_tracker);
+ release_firmware(module_fw->fw_update.fw);
+ kfree(module_fw);
+}
+
+#define MODULE_EEPROM_PHYS_ID_PAGE 0
+#define MODULE_EEPROM_PHYS_ID_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 = {};
+ u8 phys_id;
+ 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_PHYS_ID_PAGE;
+ page_data.offset = SFP_PHYS_ID;
+ page_data.length = sizeof(phys_id);
+ page_data.i2c_address = MODULE_EEPROM_PHYS_ID_I2C_ADDR;
+ page_data.data = &phys_id;
+
+ err = ops->get_module_eeprom_by_page(dev, &page_data, extack);
+ if (err < 0)
+ return err;
+
+ switch (phys_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, module_flash_fw_work);
+ break;
+ default:
+ NL_SET_ERR_MSG(extack,
+ "Module type does not support firmware flashing");
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+void ethnl_module_fw_flash_sock_destroy(struct ethnl_sock_priv *sk_priv)
+{
+ struct ethtool_module_fw_flash *work;
+
+ spin_lock(&module_fw_flash_work_list_lock);
+ list_for_each_entry(work, &module_fw_flash_work_list, list) {
+ if (work->fw_update.dev == sk_priv->dev &&
+ work->fw_update.ntf_params.portid == sk_priv->portid) {
+ work->fw_update.ntf_params.closed_sock = true;
+ break;
+ }
+ }
+ spin_unlock(&module_fw_flash_work_list_lock);
+}
+
+static int
+module_flash_fw_schedule(struct net_device *dev, const char *file_name,
+ struct ethtool_module_fw_flash_params *params,
+ struct sk_buff *skb, struct genl_info *info)
+{
+ struct ethtool_cmis_fw_update_params *fw_update;
+ struct ethtool_module_fw_flash *module_fw;
+ int err;
+
+ module_fw = kzalloc(sizeof(*module_fw), GFP_KERNEL);
+ if (!module_fw)
+ return -ENOMEM;
+
+ fw_update = &module_fw->fw_update;
+ fw_update->params = *params;
+ err = request_firmware_direct(&fw_update->fw,
+ file_name, &dev->dev);
+ if (err) {
+ NL_SET_ERR_MSG(info->extack,
+ "Failed to request module firmware image");
+ goto err_free;
+ }
+
+ err = module_flash_fw_work_init(module_fw, dev, info->extack);
+ if (err < 0)
+ goto err_release_firmware;
+
+ dev->module_fw_flash_in_progress = true;
+ netdev_hold(dev, &module_fw->dev_tracker, GFP_KERNEL);
+ fw_update->dev = dev;
+ fw_update->ntf_params.portid = info->snd_portid;
+ fw_update->ntf_params.seq = info->snd_seq;
+ fw_update->ntf_params.closed_sock = false;
+
+ err = ethnl_sock_priv_set(skb, dev, fw_update->ntf_params.portid,
+ ETHTOOL_SOCK_TYPE_MODULE_FW_FLASH);
+ if (err < 0)
+ goto err_release_firmware;
+
+ err = module_flash_fw_work_list_add(module_fw, info);
+ if (err < 0)
+ goto err_release_firmware;
+
+ schedule_work(&module_fw->work);
+
+ return 0;
+
+err_release_firmware:
+ release_firmware(fw_update->fw);
+err_free:
+ kfree(module_fw);
+ return err;
+}
+
+static int module_flash_fw(struct net_device *dev, struct nlattr **tb,
+ struct sk_buff *skb, struct genl_info *info)
+{
+ struct ethtool_module_fw_flash_params params = {};
+ const char *file_name;
+ struct nlattr *attr;
+
+ if (GENL_REQ_ATTR_CHECK(info, ETHTOOL_A_MODULE_FW_FLASH_FILE_NAME))
+ return -EINVAL;
+
+ 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, file_name, ¶ms, skb, info);
+}
+
+static int ethnl_module_fw_flash_validate(struct net_device *dev,
+ struct netlink_ext_ack *extack)
+{
+ struct devlink_port *devlink_port = dev->devlink_port;
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+
+ 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 (!ops->reset) {
+ NL_SET_ERR_MSG(extack,
+ "Reset module is not supported by this device, so flashing is not permitted");
+ return -EOPNOTSUPP;
+ }
+
+ if (dev->module_fw_flash_in_progress) {
+ NL_SET_ERR_MSG(extack, "Module firmware flashing already in progress");
+ return -EBUSY;
+ }
+
+ if (dev->flags & IFF_UP) {
+ NL_SET_ERR_MSG(extack, "Netdevice is up, so flashing is not permitted");
+ return -EBUSY;
+ }
+
+ if (devlink_port && devlink_port->attrs.split) {
+ NL_SET_ERR_MSG(extack, "Can't perform firmware flashing on a split port");
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+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 = ethnl_module_fw_flash_validate(dev, info->extack);
+ if (ret < 0)
+ goto out_rtnl;
+
+ ret = module_flash_fw(dev, tb, skb, info);
+
+ ethnl_ops_complete(dev);
+
+out_rtnl:
+ rtnl_unlock();
+ ethnl_parse_header_dev_put(&req_info);
+ return ret;
+}
+
/* MODULE_FW_FLASH_NTF */
static int
diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h
index d0fc2529b60e..634543a12d0c 100644
--- a/net/ethtool/module_fw.h
+++ b/net/ethtool/module_fw.h
@@ -1,6 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0-only */
#include <uapi/linux/ethtool.h>
+#include "netlink.h"
/**
* struct ethnl_module_fw_flash_ntf_params - module firmware flashing
@@ -54,6 +55,8 @@ struct ethtool_module_fw_flash {
struct ethtool_cmis_fw_update_params fw_update;
};
+void ethnl_module_fw_flash_sock_destroy(struct ethnl_sock_priv *sk_priv);
+
void
ethnl_module_fw_flash_ntf_err(struct net_device *dev,
struct ethnl_module_fw_flash_ntf_params *params,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index a5907bbde427..81fe2e5b95f6 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -4,6 +4,7 @@
#include <linux/ethtool_netlink.h>
#include <linux/pm_runtime.h>
#include "netlink.h"
+#include "module_fw.h"
static struct genl_family ethtool_genl_family;
@@ -30,6 +31,35 @@ const struct nla_policy ethnl_header_policy_stats[] = {
ETHTOOL_FLAGS_STATS),
};
+int ethnl_sock_priv_set(struct sk_buff *skb, struct net_device *dev, u32 portid,
+ enum ethnl_sock_type type)
+{
+ struct ethnl_sock_priv *sk_priv;
+
+ sk_priv = genl_sk_priv_get(ðtool_genl_family, NETLINK_CB(skb).sk);
+ if (IS_ERR(sk_priv))
+ return PTR_ERR(sk_priv);
+
+ sk_priv->dev = dev;
+ sk_priv->portid = portid;
+ sk_priv->type = type;
+
+ return 0;
+}
+
+static void ethnl_sock_priv_destroy(void *priv)
+{
+ struct ethnl_sock_priv *sk_priv = priv;
+
+ switch (sk_priv->type) {
+ case ETHTOOL_SOCK_TYPE_MODULE_FW_FLASH:
+ ethnl_module_fw_flash_sock_destroy(sk_priv);
+ break;
+ default:
+ break;
+ }
+}
+
int ethnl_ops_begin(struct net_device *dev)
{
int ret;
@@ -1142,6 +1172,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[] = {
@@ -1158,6 +1195,8 @@ static struct genl_family ethtool_genl_family __ro_after_init = {
.resv_start_op = ETHTOOL_MSG_MODULE_GET + 1,
.mcgrps = ethtool_nl_mcgrps,
.n_mcgrps = ARRAY_SIZE(ethtool_nl_mcgrps),
+ .sock_priv_size = sizeof(struct ethnl_sock_priv),
+ .sock_priv_destroy = ethnl_sock_priv_destroy,
};
/* module setup */
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 5e6c6a7b7adc..46ec273a87c5 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -284,6 +284,19 @@ struct ethnl_reply_data {
int ethnl_ops_begin(struct net_device *dev);
void ethnl_ops_complete(struct net_device *dev);
+enum ethnl_sock_type {
+ ETHTOOL_SOCK_TYPE_MODULE_FW_FLASH,
+};
+
+struct ethnl_sock_priv {
+ struct net_device *dev;
+ u32 portid;
+ enum ethnl_sock_type type;
+};
+
+int ethnl_sock_priv_set(struct sk_buff *skb, struct net_device *dev, u32 portid,
+ enum ethnl_sock_type type);
+
/**
* struct ethnl_request_ops - unified handling of GET and SET requests
* @request_cmd: command id for request (GET)
@@ -442,6 +455,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);
@@ -449,6 +463,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.45.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM
2024-06-24 17:51 ` [PATCH net-next v7 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM Danielle Ratson
@ 2024-06-24 19:01 ` Andrew Lunn
0 siblings, 0 replies; 25+ messages in thread
From: Andrew Lunn @ 2024-06-24 19:01 UTC (permalink / raw)
To: Danielle Ratson
Cc: netdev, davem, edumazet, kuba, pabeni, corbet, linux, sdf,
kory.maincent, maxime.chevallier, vladimir.oltean,
przemyslaw.kitszel, ahmed.zaki, richardcochran, shayagr,
paul.greenwalt, jiri, linux-doc, linux-kernel, mlxsw, idosch,
petrm
On Mon, Jun 24, 2024 at 08:51:51PM +0300, 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 new write operation is purely an in-kernel API and is not exposed to
> user space.
>
> 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>
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 2/9] mlxsw: Implement ethtool operation to write to a transceiver module EEPROM
2024-06-24 17:51 ` [PATCH net-next v7 2/9] mlxsw: Implement " Danielle Ratson
@ 2024-06-24 19:09 ` Andrew Lunn
0 siblings, 0 replies; 25+ messages in thread
From: Andrew Lunn @ 2024-06-24 19:09 UTC (permalink / raw)
To: Danielle Ratson
Cc: netdev, davem, edumazet, kuba, pabeni, corbet, linux, sdf,
kory.maincent, maxime.chevallier, vladimir.oltean,
przemyslaw.kitszel, ahmed.zaki, richardcochran, shayagr,
paul.greenwalt, jiri, linux-doc, linux-kernel, mlxsw, idosch,
petrm
On Mon, Jun 24, 2024 at 08:51:52PM +0300, Danielle Ratson wrote:
> 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 3/9] ethtool: Add an interface for flashing transceiver modules' firmware
2024-06-24 17:51 ` [PATCH net-next v7 3/9] ethtool: Add an interface for flashing transceiver modules' firmware Danielle Ratson
@ 2024-06-24 19:23 ` Andrew Lunn
0 siblings, 0 replies; 25+ messages in thread
From: Andrew Lunn @ 2024-06-24 19:23 UTC (permalink / raw)
To: Danielle Ratson
Cc: netdev, davem, edumazet, kuba, pabeni, corbet, linux, sdf,
kory.maincent, maxime.chevallier, vladimir.oltean,
przemyslaw.kitszel, ahmed.zaki, richardcochran, shayagr,
paul.greenwalt, jiri, linux-doc, linux-kernel, mlxsw, idosch,
petrm
On Mon, Jun 24, 2024 at 08:51:53PM +0300, Danielle Ratson wrote:
> 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>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 4/9] ethtool: Add flashing transceiver modules' firmware notifications ability
2024-06-24 17:51 ` [PATCH net-next v7 4/9] ethtool: Add flashing transceiver modules' firmware notifications ability Danielle Ratson
@ 2024-06-24 19:27 ` Andrew Lunn
0 siblings, 0 replies; 25+ messages in thread
From: Andrew Lunn @ 2024-06-24 19:27 UTC (permalink / raw)
To: Danielle Ratson
Cc: netdev, davem, edumazet, kuba, pabeni, corbet, linux, sdf,
kory.maincent, maxime.chevallier, vladimir.oltean,
przemyslaw.kitszel, ahmed.zaki, richardcochran, shayagr,
paul.greenwalt, jiri, linux-doc, linux-kernel, mlxsw, idosch,
petrm
On Mon, Jun 24, 2024 at 08:51:54PM +0300, Danielle Ratson wrote:
> 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>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 5/9] ethtool: Veto some operations during firmware flashing process
2024-06-24 17:51 ` [PATCH net-next v7 5/9] ethtool: Veto some operations during firmware flashing process Danielle Ratson
@ 2024-06-24 19:31 ` Andrew Lunn
0 siblings, 0 replies; 25+ messages in thread
From: Andrew Lunn @ 2024-06-24 19:31 UTC (permalink / raw)
To: Danielle Ratson
Cc: netdev, davem, edumazet, kuba, pabeni, corbet, linux, sdf,
kory.maincent, maxime.chevallier, vladimir.oltean,
przemyslaw.kitszel, ahmed.zaki, richardcochran, shayagr,
paul.greenwalt, jiri, linux-doc, linux-kernel, mlxsw, idosch,
petrm
On Mon, Jun 24, 2024 at 08:51:55PM +0300, Danielle Ratson wrote:
> Some operations cannot be performed during the firmware flashing
> process.
>
> For example:
>
> - Port must be down during the whole flashing process to avoid packet loss
> while committing reset for example.
>
> - Writing to EEPROM interrupts the flashing process, so operations like
> ethtool dump, module reset, get and set power mode should be vetoed.
>
> - Split port firmware flashing should be vetoed.
>
> 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 and use it
> to prevent interruptions during the process.
>
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 6/9] net: sfp: Add more extended compliance codes
2024-06-24 17:51 ` [PATCH net-next v7 6/9] net: sfp: Add more extended compliance codes Danielle Ratson
@ 2024-06-24 19:31 ` Andrew Lunn
0 siblings, 0 replies; 25+ messages in thread
From: Andrew Lunn @ 2024-06-24 19:31 UTC (permalink / raw)
To: Danielle Ratson
Cc: netdev, davem, edumazet, kuba, pabeni, corbet, linux, sdf,
kory.maincent, maxime.chevallier, vladimir.oltean,
przemyslaw.kitszel, ahmed.zaki, richardcochran, shayagr,
paul.greenwalt, jiri, linux-doc, linux-kernel, mlxsw, idosch,
petrm
On Mon, Jun 24, 2024 at 08:51:56PM +0300, 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands
2024-06-24 17:51 ` [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands Danielle Ratson
@ 2024-06-24 19:50 ` Andrew Lunn
2024-06-26 6:14 ` Danielle Ratson
0 siblings, 1 reply; 25+ messages in thread
From: Andrew Lunn @ 2024-06-24 19:50 UTC (permalink / raw)
To: Danielle Ratson
Cc: netdev, davem, edumazet, kuba, pabeni, corbet, linux, sdf,
kory.maincent, maxime.chevallier, vladimir.oltean,
przemyslaw.kitszel, ahmed.zaki, richardcochran, shayagr,
paul.greenwalt, jiri, linux-doc, linux-kernel, mlxsw, idosch,
petrm
> +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),
> + u8 *state)
> +{
> + const struct ethtool_ops *ops = dev->ethtool_ops;
> + struct ethtool_module_eeprom page_data = {0};
> + struct cmis_wait_for_cond_rpl rpl = {};
> + struct netlink_ext_ack extack = {};
> + unsigned long end;
> + int err;
> +
> + if (!(flags & flag))
> + return 0;
> +
> + if (max_duration == 0)
> + max_duration = U16_MAX;
> +
> + end = jiffies + msecs_to_jiffies(max_duration);
> + do {
> + ethtool_cmis_page_init(&page_data, 0, offset, sizeof(rpl));
> + page_data.data = (u8 *)&rpl;
> +
> + 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;
continue here is interested. Say you get -EIO because the module has
been ejected. I would say that is fatal. Won't this spam the logs, as
fast as the I2C bus can fail, without the 20ms sleep, for 65535
jiffies?
> + }
> +
> + if ((*cond_success)(rpl.state))
> + return 0;
> +
> + if (*cond_fail && (*cond_fail)(rpl.state))
> + break;
> +
> + msleep(20);
> + } while (time_before(jiffies, end));
Please could you implement this using iopoll.h. This appears to have
the usual problem. Say msleep(20) actually sleeps a lot longer,
because the system is busy doing other things. time_before(jiffies,
end)) is false, because of the long delay, but in fact the operation
has completed without error. Yet you return EBUSY. iopoll.h gets this
correct, it does one more evaluation of the condition after exiting
the loop to handle this issue.
> +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;
> +}
I expect there is already a helper do that somewhere.
Andrew
---
pw-bot: cr
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB
2024-06-24 17:51 ` [PATCH net-next v7 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB Danielle Ratson
@ 2024-06-24 19:57 ` Andrew Lunn
2024-06-25 6:09 ` Danielle Ratson
0 siblings, 1 reply; 25+ messages in thread
From: Andrew Lunn @ 2024-06-24 19:57 UTC (permalink / raw)
To: Danielle Ratson
Cc: netdev, davem, edumazet, kuba, pabeni, corbet, linux, sdf,
kory.maincent, maxime.chevallier, vladimir.oltean,
przemyslaw.kitszel, ahmed.zaki, richardcochran, shayagr,
paul.greenwalt, jiri, linux-doc, linux-kernel, mlxsw, idosch,
petrm
> +static int cmis_fw_update_reset(struct net_device *dev)
> +{
> + __u32 reset_data = ETH_RESET_PHY;
> +
> + return dev->ethtool_ops->reset(dev, &reset_data);
Is there a test somewhere that this op is actually implemented?
Maybe the next patch.
Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH net-next v7 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB
2024-06-24 19:57 ` Andrew Lunn
@ 2024-06-25 6:09 ` Danielle Ratson
0 siblings, 0 replies; 25+ messages in thread
From: Danielle Ratson @ 2024-06-25 6:09 UTC (permalink / raw)
To: Andrew Lunn
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,
Ido Schimmel, Petr Machata
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, 24 June 2024 22:57
> 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>; Ido Schimmel
> <idosch@nvidia.com>; Petr Machata <petrm@nvidia.com>
> Subject: Re: [PATCH net-next v7 8/9] ethtool: cmis_fw_update: add a layer for
> supporting firmware update using CDB
>
> > +static int cmis_fw_update_reset(struct net_device *dev) {
> > + __u32 reset_data = ETH_RESET_PHY;
> > +
> > + return dev->ethtool_ops->reset(dev, &reset_data);
>
> Is there a test somewhere that this op is actually implemented?
>
> Maybe the next patch.
Yes, indeed.
>
> Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands
2024-06-24 19:50 ` Andrew Lunn
@ 2024-06-26 6:14 ` Danielle Ratson
2024-06-26 11:52 ` Danielle Ratson
0 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-26 6:14 UTC (permalink / raw)
To: Andrew Lunn
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,
Ido Schimmel, Petr Machata
Hi Andrew,
Thanks for reviewing the patches.
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, 24 June 2024 22:51
> 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>; Ido Schimmel
> <idosch@nvidia.com>; Petr Machata <petrm@nvidia.com>
> Subject: Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
>
> > +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),
> > + u8 *state)
> > +{
> > + const struct ethtool_ops *ops = dev->ethtool_ops;
> > + struct ethtool_module_eeprom page_data = {0};
> > + struct cmis_wait_for_cond_rpl rpl = {};
> > + struct netlink_ext_ack extack = {};
> > + unsigned long end;
> > + int err;
> > +
> > + if (!(flags & flag))
> > + return 0;
> > +
> > + if (max_duration == 0)
> > + max_duration = U16_MAX;
> > +
> > + end = jiffies + msecs_to_jiffies(max_duration);
> > + do {
> > + ethtool_cmis_page_init(&page_data, 0, offset, sizeof(rpl));
> > + page_data.data = (u8 *)&rpl;
> > +
> > + 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;
>
> continue here is interested. Say you get -EIO because the module has been
> ejected. I would say that is fatal. Won't this spam the logs, as fast as the I2C
> bus can fail, without the 20ms sleep, for 65535 jiffies?
If the module is ejected from some reason, it might span the logs I guess.
But it is less likely than the scenario I wanted to cover.
According to SPEC 5.2:
"
7.2.5.1 Foreground Mode CDB Messaging
[...]
In foreground mode the module rejects any register ACCESS until a currently executing CDB command execution has completed.
Note: READs of the CdbStatus registers 00h:37 or 00h:38 (see Table 8-13) will also be rejected by the module.
"
So in that case the module won't be able to respond and we need to wait for it to be responsive and the status to be valid.
>
> > + }
> > +
> > + if ((*cond_success)(rpl.state))
> > + return 0;
> > +
> > + if (*cond_fail && (*cond_fail)(rpl.state))
> > + break;
> > +
> > + msleep(20);
> > + } while (time_before(jiffies, end));
>
> Please could you implement this using iopoll.h. This appears to have the usual
> problem. Say msleep(20) actually sleeps a lot longer, because the system is
> busy doing other things. time_before(jiffies,
> end)) is false, because of the long delay, but in fact the operation has
> completed without error. Yet you return EBUSY. iopoll.h gets this correct, it
> does one more evaluation of the condition after exiting the loop to handle this
> issue.
OK.
>
> > +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;
> > +}
>
> I expect there is already a helper do that somewhere.
>
> Andrew
Yes it does, but actually it is an helper that occurs in specific places (for example pci_vpd_check_csum()), that i can use from here.
>
> ---
> pw-bot: cr
^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands
2024-06-26 6:14 ` Danielle Ratson
@ 2024-06-26 11:52 ` Danielle Ratson
2024-06-26 13:40 ` Andrew Lunn
0 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-26 11:52 UTC (permalink / raw)
To: Andrew Lunn
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,
Ido Schimmel, Petr Machata
> From: Danielle Ratson <danieller@nvidia.com>
> Sent: Wednesday, 26 June 2024 9:14
> 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>; Ido Schimmel
> <idosch@nvidia.com>; Petr Machata <petrm@nvidia.com>
> Subject: RE: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
>
> Hi Andrew,
>
> Thanks for reviewing the patches.
>
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Monday, 24 June 2024 22:51
> > 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>; Ido Schimmel
> > <idosch@nvidia.com>; Petr Machata <petrm@nvidia.com>
> > Subject: Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer
> > for supporting CDB commands
> >
> > > +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),
> > > + u8 *state)
> > > +{
> > > + const struct ethtool_ops *ops = dev->ethtool_ops;
> > > + struct ethtool_module_eeprom page_data = {0};
> > > + struct cmis_wait_for_cond_rpl rpl = {};
> > > + struct netlink_ext_ack extack = {};
> > > + unsigned long end;
> > > + int err;
> > > +
> > > + if (!(flags & flag))
> > > + return 0;
> > > +
> > > + if (max_duration == 0)
> > > + max_duration = U16_MAX;
> > > +
> > > + end = jiffies + msecs_to_jiffies(max_duration);
> > > + do {
> > > + ethtool_cmis_page_init(&page_data, 0, offset, sizeof(rpl));
> > > + page_data.data = (u8 *)&rpl;
> > > +
> > > + 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;
> >
> > continue here is interested. Say you get -EIO because the module has
> > been ejected. I would say that is fatal. Won't this spam the logs, as
> > fast as the I2C bus can fail, without the 20ms sleep, for 65535 jiffies?
>
> If the module is ejected from some reason, it might span the logs I guess.
> But it is less likely than the scenario I wanted to cover.
> According to SPEC 5.2:
>
> "
> 7.2.5.1 Foreground Mode CDB Messaging
> [...]
> In foreground mode the module rejects any register ACCESS until a currently
> executing CDB command execution has completed.
> Note: READs of the CdbStatus registers 00h:37 or 00h:38 (see Table 8-13) will
> also be rejected by the module.
> "
>
> So in that case the module won't be able to respond and we need to wait for it
> to be responsive and the status to be valid.
>
> >
> > > + }
> > > +
> > > + if ((*cond_success)(rpl.state))
> > > + return 0;
> > > +
> > > + if (*cond_fail && (*cond_fail)(rpl.state))
> > > + break;
> > > +
> > > + msleep(20);
> > > + } while (time_before(jiffies, end));
> >
> > Please could you implement this using iopoll.h. This appears to have
> > the usual problem. Say msleep(20) actually sleeps a lot longer,
> > because the system is busy doing other things. time_before(jiffies,
> > end)) is false, because of the long delay, but in fact the operation
> > has completed without error. Yet you return EBUSY. iopoll.h gets this
> > correct, it does one more evaluation of the condition after exiting
> > the loop to handle this issue.
>
> OK.
Hi Andrew,
I implemented the above as you asked, but it seems to have a problem.
The iopoll functions have a sleeping parameter "sleep_us" that supposed to be equivalent to the msleep(20) if I put 20000 there.
However, this parameter is defined as 'Maximum time to sleep between reads in us', so it will not always sleep 20msec as it should.
This is problematic since there are modules that needs this 20msec sleep in order to be able to poll again from the module.
Otherwise, these modules fail during the write FW command iterations, while polling the flag or status.
Therefore, unfortunately in this case I'd rather stay with the origin code.
Thank you for all your comments,
Danielle
>
> >
> > > +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;
> > > +}
> >
> > I expect there is already a helper do that somewhere.
> >
> > Andrew
>
> Yes it does, but actually it is an helper that occurs in specific places (for example
> pci_vpd_check_csum()), that i can use from here.
>
> >
> > ---
> > pw-bot: cr
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands
2024-06-26 11:52 ` Danielle Ratson
@ 2024-06-26 13:40 ` Andrew Lunn
2024-06-26 17:26 ` Danielle Ratson
0 siblings, 1 reply; 25+ messages in thread
From: Andrew Lunn @ 2024-06-26 13:40 UTC (permalink / raw)
To: Danielle Ratson
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,
Ido Schimmel, Petr Machata
> > > > +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),
> > > > + u8 *state)
> > > > +{
> > > > + const struct ethtool_ops *ops = dev->ethtool_ops;
> > > > + struct ethtool_module_eeprom page_data = {0};
> > > > + struct cmis_wait_for_cond_rpl rpl = {};
> > > > + struct netlink_ext_ack extack = {};
> > > > + unsigned long end;
> > > > + int err;
> > > > +
> > > > + if (!(flags & flag))
> > > > + return 0;
> > > > +
> > > > + if (max_duration == 0)
> > > > + max_duration = U16_MAX;
> > > > +
> > > > + end = jiffies + msecs_to_jiffies(max_duration);
> > > > + do {
> > > > + ethtool_cmis_page_init(&page_data, 0, offset, sizeof(rpl));
> > > > + page_data.data = (u8 *)&rpl;
> > > > +
> > > > + 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;
> > >
> > > continue here is interested. Say you get -EIO because the module has
> > > been ejected. I would say that is fatal. Won't this spam the logs, as
> > > fast as the I2C bus can fail, without the 20ms sleep, for 65535 jiffies?
> >
> > If the module is ejected from some reason, it might span the logs I guess.
Please could you test it.
65535 jiffies is i think 655 seconds? That is probably too long to
loop when the module has been ejected. Maybe replace it with HZ?
Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
delay before the continue.
> > > > + }
> > > > +
> > > > + if ((*cond_success)(rpl.state))
> > > > + return 0;
> > > > +
> > > > + if (*cond_fail && (*cond_fail)(rpl.state))
> > > > + break;
> > > > +
> > > > + msleep(20);
> > > > + } while (time_before(jiffies, end));
> > >
> > > Please could you implement this using iopoll.h. This appears to have
> > > the usual problem. Say msleep(20) actually sleeps a lot longer,
> > > because the system is busy doing other things. time_before(jiffies,
> > > end)) is false, because of the long delay, but in fact the operation
> > > has completed without error. Yet you return EBUSY. iopoll.h gets this
> > > correct, it does one more evaluation of the condition after exiting
> > > the loop to handle this issue.
> >
> > OK.
>
> Hi Andrew,
>
> Therefore, unfortunately in this case I'd rather stay with the origin code.
O.K. Please evaluate the condition again after the while() just so
ETIMEDOUT is not returned in error.
Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands
2024-06-26 13:40 ` Andrew Lunn
@ 2024-06-26 17:26 ` Danielle Ratson
2024-06-26 17:42 ` Andrew Lunn
0 siblings, 1 reply; 25+ messages in thread
From: Danielle Ratson @ 2024-06-26 17:26 UTC (permalink / raw)
To: Andrew Lunn
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,
Ido Schimmel, Petr Machata
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, 26 June 2024 16:40
> 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>; Ido Schimmel
> <idosch@nvidia.com>; Petr Machata <petrm@nvidia.com>
> Subject: Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
>
> > > > > +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),
> > > > > + u8 *state)
> > > > > +{
> > > > > + const struct ethtool_ops *ops = dev->ethtool_ops;
> > > > > + struct ethtool_module_eeprom page_data = {0};
> > > > > + struct cmis_wait_for_cond_rpl rpl = {};
> > > > > + struct netlink_ext_ack extack = {};
> > > > > + unsigned long end;
> > > > > + int err;
> > > > > +
> > > > > + if (!(flags & flag))
> > > > > + return 0;
> > > > > +
> > > > > + if (max_duration == 0)
> > > > > + max_duration = U16_MAX;
> > > > > +
> > > > > + end = jiffies + msecs_to_jiffies(max_duration);
> > > > > + do {
> > > > > + ethtool_cmis_page_init(&page_data, 0, offset,
> sizeof(rpl));
> > > > > + page_data.data = (u8 *)&rpl;
> > > > > +
> > > > > + 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;
> > > >
> > > > continue here is interested. Say you get -EIO because the module
> > > > has been ejected. I would say that is fatal. Won't this spam the
> > > > logs, as fast as the I2C bus can fail, without the 20ms sleep, for 65535
> jiffies?
> > >
> > > If the module is ejected from some reason, it might span the logs I guess.
>
> Please could you test it.
>
> 65535 jiffies is i think 655 seconds? That is probably too long to loop when
> the module has been ejected. Maybe replace it with HZ?
>
Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.
The test you are asking for is a bit complicated since I don’t have a machine physically nearby, do you find it very much important?
I mean, it is not very reasonable thing to do, burning fw on a module and in the exact same time eject it.
> Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
> delay before the continue.
>
> > > > > + }
> > > > > +
> > > > > + if ((*cond_success)(rpl.state))
> > > > > + return 0;
> > > > > +
> > > > > + if (*cond_fail && (*cond_fail)(rpl.state))
> > > > > + break;
> > > > > +
> > > > > + msleep(20);
> > > > > + } while (time_before(jiffies, end));
> > > >
> > > > Please could you implement this using iopoll.h. This appears to
> > > > have the usual problem. Say msleep(20) actually sleeps a lot
> > > > longer, because the system is busy doing other things.
> > > > time_before(jiffies,
> > > > end)) is false, because of the long delay, but in fact the
> > > > operation has completed without error. Yet you return EBUSY.
> > > > iopoll.h gets this correct, it does one more evaluation of the
> > > > condition after exiting the loop to handle this issue.
> > >
> > > OK.
> >
> > Hi Andrew,
> >
> > Therefore, unfortunately in this case I'd rather stay with the origin code.
>
> O.K. Please evaluate the condition again after the while() just so ETIMEDOUT is
> not returned in error.
Not sure I understood.
Do you want to have one more polling in the end of the loop? What could return ETIMEDOUT?
Thanks,
Danielle
>
> Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands
2024-06-26 17:26 ` Danielle Ratson
@ 2024-06-26 17:42 ` Andrew Lunn
2024-06-27 13:12 ` Danielle Ratson
0 siblings, 1 reply; 25+ messages in thread
From: Andrew Lunn @ 2024-06-26 17:42 UTC (permalink / raw)
To: Danielle Ratson
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,
Ido Schimmel, Petr Machata
> > Please could you test it.
> >
> > 65535 jiffies is i think 655 seconds? That is probably too long to loop when
> > the module has been ejected. Maybe replace it with HZ?
> >
>
> Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.
I _think_ it depends on CONFIG_HZ, which can be 100, 250, 300 and
1000.
> The test you are asking for is a bit complicated since I don’t have
> a machine physically nearby, do you find it very much important?
> I mean, it is not very reasonable thing to do, burning fw on a
> module and in the exact same time eject it.
Shooting yourself in the foot is not a very reasonable thing to do,
but the Unix philosophy is to all root to do it. Do we really want 60
to 600 seconds of the kernel spamming the log when somebody does do
this?
> > Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
> > delay before the continue.
> >
> > > > > > + }
> > > > > > +
> > > > > > + if ((*cond_success)(rpl.state))
> > > > > > + return 0;
> > > > > > +
> > > > > > + if (*cond_fail && (*cond_fail)(rpl.state))
> > > > > > + break;
> > > > > > +
> > > > > > + msleep(20);
> > > > > > + } while (time_before(jiffies, end));
> > > > >
> > O.K. Please evaluate the condition again after the while() just so ETIMEDOUT is
> > not returned in error.
>
> Not sure I understood.
> Do you want to have one more polling in the end of the loop? What could return ETIMEDOUT?
Consider what happens when msleep(20) actually sleeps a lot longer.
Look at the core code which gets this correct:
#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
sleep_before_read, args...) \
({ \
u64 __timeout_us = (timeout_us); \
unsigned long __sleep_us = (sleep_us); \
ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
might_sleep_if((__sleep_us) != 0); \
if (sleep_before_read && __sleep_us) \
usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
for (;;) { \
(val) = op(args); \
if (cond) \
break; \
if (__timeout_us && \
ktime_compare(ktime_get(), __timeout) > 0) { \
(val) = op(args); \
break; \
} \
if (__sleep_us) \
usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
cpu_relax(); \
} \
(cond) ? 0 : -ETIMEDOUT; \
})
So after breaking out of the for loop with a timeout, it evaluates the
condition one more time, and uses that to decide on 0 or ETIMEDOUT. So
it does not matter if usleep_range() range slept for 60 seconds, not
60ms, the exit code will be correct.
Andrew
^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands
2024-06-26 17:42 ` Andrew Lunn
@ 2024-06-27 13:12 ` Danielle Ratson
0 siblings, 0 replies; 25+ messages in thread
From: Danielle Ratson @ 2024-06-27 13:12 UTC (permalink / raw)
To: Andrew Lunn
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,
Ido Schimmel, Petr Machata
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, 26 June 2024 20:43
> 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>; Ido Schimmel
> <idosch@nvidia.com>; Petr Machata <petrm@nvidia.com>
> Subject: Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
>
> > > Please could you test it.
> > >
> > > 65535 jiffies is i think 655 seconds? That is probably too long to
> > > loop when the module has been ejected. Maybe replace it with HZ?
> > >
> >
> > Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.
>
> I _think_ it depends on CONFIG_HZ, which can be 100, 250, 300 and 1000.
>
> > The test you are asking for is a bit complicated since I don’t have a
> > machine physically nearby, do you find it very much important?
>
> > I mean, it is not very reasonable thing to do, burning fw on a module
> > and in the exact same time eject it.
>
> Shooting yourself in the foot is not a very reasonable thing to do, but the Unix
> philosophy is to all root to do it. Do we really want 60 to 600 seconds of the
> kernel spamming the log when somebody does do this?
Ok i checked it and using netdev_err_once() fulfill that issue. Thanks!
>
> > > Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
> > > delay before the continue.
> > >
> > > > > > > + }
> > > > > > > +
> > > > > > > + if ((*cond_success)(rpl.state))
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + if (*cond_fail && (*cond_fail)(rpl.state))
> > > > > > > + break;
> > > > > > > +
> > > > > > > + msleep(20);
> > > > > > > + } while (time_before(jiffies, end));
> > > > > >
>
> > > O.K. Please evaluate the condition again after the while() just so
> > > ETIMEDOUT is not returned in error.
> >
> > Not sure I understood.
> > Do you want to have one more polling in the end of the loop? What could
> return ETIMEDOUT?
>
> Consider what happens when msleep(20) actually sleeps a lot longer.
>
> Look at the core code which gets this correct:
>
> #define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
> sleep_before_read, args...) \ ({ \
> u64 __timeout_us = (timeout_us); \
> unsigned long __sleep_us = (sleep_us); \
> ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> might_sleep_if((__sleep_us) != 0); \
> if (sleep_before_read && __sleep_us) \
> usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
> for (;;) { \
> (val) = op(args); \
> if (cond) \
> break; \
> if (__timeout_us && \
> ktime_compare(ktime_get(), __timeout) > 0) { \
> (val) = op(args); \
> break; \
> } \
> if (__sleep_us) \
> usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
> cpu_relax(); \
> } \
> (cond) ? 0 : -ETIMEDOUT; \
> })
>
> So after breaking out of the for loop with a timeout, it evaluates the condition
> one more time, and uses that to decide on 0 or ETIMEDOUT. So it does not
> matter if usleep_range() range slept for 60 seconds, not 60ms, the exit code
> will be correct.
>
> Andrew
Ok ill fix it, thanks.
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2024-06-27 13:12 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-24 17:51 [PATCH net-next v7 0/9] Add ability to flash modules' firmware Danielle Ratson
2024-06-24 17:51 ` [PATCH net-next v7 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM Danielle Ratson
2024-06-24 19:01 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 2/9] mlxsw: Implement " Danielle Ratson
2024-06-24 19:09 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 3/9] ethtool: Add an interface for flashing transceiver modules' firmware Danielle Ratson
2024-06-24 19:23 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 4/9] ethtool: Add flashing transceiver modules' firmware notifications ability Danielle Ratson
2024-06-24 19:27 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 5/9] ethtool: Veto some operations during firmware flashing process Danielle Ratson
2024-06-24 19:31 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 6/9] net: sfp: Add more extended compliance codes Danielle Ratson
2024-06-24 19:31 ` Andrew Lunn
2024-06-24 17:51 ` [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands Danielle Ratson
2024-06-24 19:50 ` Andrew Lunn
2024-06-26 6:14 ` Danielle Ratson
2024-06-26 11:52 ` Danielle Ratson
2024-06-26 13:40 ` Andrew Lunn
2024-06-26 17:26 ` Danielle Ratson
2024-06-26 17:42 ` Andrew Lunn
2024-06-27 13:12 ` Danielle Ratson
2024-06-24 17:51 ` [PATCH net-next v7 8/9] ethtool: cmis_fw_update: add a layer for supporting firmware update using CDB Danielle Ratson
2024-06-24 19:57 ` Andrew Lunn
2024-06-25 6:09 ` Danielle Ratson
2024-06-24 17:51 ` [PATCH net-next v7 9/9] ethtool: Add ability to flash transceiver modules' firmware Danielle Ratson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).