* [RFC PATCH net-next v2 1/6] ice: Add support for unified NVM update flow capability
2020-07-17 18:35 [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jacob Keller
@ 2020-07-17 18:35 ` Jacob Keller
2020-07-17 18:35 ` [RFC PATCH net-next v2 2/6] ice: Add AdminQ commands for FW update Jacob Keller
` (5 subsequent siblings)
6 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-17 18:35 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Tom Herbert, Jacek Naczyk
From: Jacek Naczyk <jacek.naczyk@intel.com>
Extends function parsing response from Discover Device
Capability AQC to check if the device supports unified NVM update flow.
Signed-off-by: Jacek Naczyk <jacek.naczyk@intel.com>
---
drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 1 +
drivers/net/ethernet/intel/ice/ice_common.c | 7 +++++++
drivers/net/ethernet/intel/ice/ice_type.h | 3 +++
3 files changed, 11 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index d1640ff2aea9..59d3bbcf692e 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -109,6 +109,7 @@ struct ice_aqc_list_caps_elem {
#define ICE_AQC_CAPS_MSIX 0x0043
#define ICE_AQC_CAPS_FD 0x0045
#define ICE_AQC_CAPS_MAX_MTU 0x0047
+#define ICE_AQC_CAPS_NVM_MGMT 0x0080
u8 major_ver;
u8 minor_ver;
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 63cff81d234f..ffa7490b5789 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1855,6 +1855,13 @@ ice_parse_common_caps(struct ice_hw *hw, struct ice_hw_common_caps *caps,
"%s: msix_vector_first_id = %d\n", prefix,
caps->msix_vector_first_id);
break;
+ case ICE_AQC_CAPS_NVM_MGMT:
+ caps->nvm_unified_update =
+ (number & ICE_NVM_MGMT_UNIFIED_UPD_SUPPORT) ?
+ true : false;
+ ice_debug(hw, ICE_DBG_INIT, "%s: nvm_unified_update = %d\n", prefix,
+ caps->nvm_unified_update);
+ break;
case ICE_AQC_CAPS_MAX_MTU:
caps->max_mtu = number;
ice_debug(hw, ICE_DBG_INIT, "%s: max_mtu = %d\n",
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 1fe8b0cbf064..f91b6d944b63 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -262,6 +262,9 @@ struct ice_hw_common_caps {
u8 rss_table_entry_width; /* RSS Entry width in bits */
u8 dcb;
+
+ bool nvm_unified_update;
+#define ICE_NVM_MGMT_UNIFIED_UPD_SUPPORT BIT(3)
};
/* Function specific capabilities */
--
2.27.0.353.gb9a2d1a0207f
^ permalink raw reply related [flat|nested] 36+ messages in thread* [RFC PATCH net-next v2 2/6] ice: Add AdminQ commands for FW update
2020-07-17 18:35 [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jacob Keller
2020-07-17 18:35 ` [RFC PATCH net-next v2 1/6] ice: Add support for unified NVM update flow capability Jacob Keller
@ 2020-07-17 18:35 ` Jacob Keller
2020-07-17 18:35 ` [RFC PATCH net-next v2 3/6] ice: add flags indicating pending update of firmware module Jacob Keller
` (4 subsequent siblings)
6 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-17 18:35 UTC (permalink / raw)
To: netdev
Cc: Jakub Kicinski, Jiri Pirko, Tom Herbert, Cudzilo, Szymon T,
Cudzilo, Jacob Keller
From: "Cudzilo, Szymon T" <szymon.t.cudzilo@intel.com>
Add structures, identifiers, and helper functions for several AdminQ
commands related to performing a firmware update for the ice hardware.
These will be used in future code for implementing the devlink
.flash_update handler.
Signed-off-by: Cudzilo, Szymon T <szymon.t.cudzilo@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
.../net/ethernet/intel/ice/ice_adminq_cmd.h | 76 +++++++
drivers/net/ethernet/intel/ice/ice_common.h | 2 -
drivers/net/ethernet/intel/ice/ice_nvm.c | 186 ++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_nvm.h | 16 ++
drivers/net/ethernet/intel/ice/ice_type.h | 3 +
5 files changed, 281 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index 59d3bbcf692e..d81ee985a2c3 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -1299,7 +1299,14 @@ struct ice_aqc_nvm {
#define ICE_AQC_NVM_PRESERVATION_M (3 << ICE_AQC_NVM_PRESERVATION_S)
#define ICE_AQC_NVM_NO_PRESERVATION (0 << ICE_AQC_NVM_PRESERVATION_S)
#define ICE_AQC_NVM_PRESERVE_ALL BIT(1)
+#define ICE_AQC_NVM_FACTORY_DEFAULT (2 << ICE_AQC_NVM_PRESERVATION_S)
#define ICE_AQC_NVM_PRESERVE_SELECTED (3 << ICE_AQC_NVM_PRESERVATION_S)
+#define ICE_AQC_NVM_ACTIV_SEL_NVM BIT(3) /* Write Activate/SR Dump only */
+#define ICE_AQC_NVM_ACTIV_SEL_OROM BIT(4)
+#define ICE_AQC_NVM_ACTIV_SEL_NETLIST BIT(5)
+#define ICE_AQC_NVM_SPECIAL_UPDATE BIT(6)
+#define ICE_AQC_NVM_REVERT_LAST_ACTIV BIT(6) /* Write Activate only */
+#define ICE_AQC_NVM_ACTIV_SEL_MASK ICE_M(0x7, 3)
#define ICE_AQC_NVM_FLASH_ONLY BIT(7)
__le16 module_typeid;
__le16 length;
@@ -1348,6 +1355,67 @@ struct ice_aqc_nvm_checksum {
#define ICE_AQC_NVM_NETLIST_ID_BLK_SHA_HASH 0xA
#define ICE_AQC_NVM_NETLIST_ID_BLK_CUST_VER 0x2F
+/* Used for NVM Set Package Data command - 0x070A */
+struct ice_aqc_nvm_pkg_data {
+ u8 reserved[3];
+ u8 cmd_flags;
+#define ICE_AQC_NVM_PKG_DELETE BIT(0) /* used for command call */
+#define ICE_AQC_NVM_PKG_SKIPPED BIT(0) /* used for command response */
+
+ u32 reserved1;
+ __le32 addr_high;
+ __le32 addr_low;
+};
+
+/* Used for Pass Component Table command - 0x070B */
+struct ice_aqc_nvm_pass_comp_tbl {
+ u8 component_response; /* Response only */
+#define ICE_AQ_NVM_PASS_COMP_CAN_BE_UPDATED 0x0
+#define ICE_AQ_NVM_PASS_COMP_CAN_MAY_BE_UPDATEABLE 0x1
+#define ICE_AQ_NVM_PASS_COMP_CAN_NOT_BE_UPDATED 0x2
+ u8 component_response_code; /* Response only */
+#define ICE_AQ_NVM_PASS_COMP_CAN_BE_UPDATED_CODE 0x0
+#define ICE_AQ_NVM_PASS_COMP_STAMP_IDENTICAL_CODE 0x1
+#define ICE_AQ_NVM_PASS_COMP_STAMP_LOWER 0x2
+#define ICE_AQ_NVM_PASS_COMP_INVALID_STAMP_CODE 0x3
+#define ICE_AQ_NVM_PASS_COMP_CONFLICT_CODE 0x4
+#define ICE_AQ_NVM_PASS_COMP_PRE_REQ_NOT_MET_CODE 0x5
+#define ICE_AQ_NVM_PASS_COMP_NOT_SUPPORTED_CODE 0x6
+#define ICE_AQ_NVM_PASS_COMP_CANNOT_DOWNGRADE_CODE 0x7
+#define ICE_AQ_NVM_PASS_COMP_INCOMPLETE_IMAGE_CODE 0x8
+#define ICE_AQ_NVM_PASS_COMP_VER_STR_IDENTICAL_CODE 0xA
+#define ICE_AQ_NVM_PASS_COMP_VER_STR_LOWER_CODE 0xB
+ u8 reserved;
+ u8 transfer_flag;
+#define ICE_AQ_NVM_PASS_COMP_TBL_START 0x1
+#define ICE_AQ_NVM_PASS_COMP_TBL_MIDDLE 0x2
+#define ICE_AQ_NVM_PASS_COMP_TBL_END 0x4
+#define ICE_AQ_NVM_PASS_COMP_TBL_START_AND_END 0x5
+ __le32 reserved1;
+ __le32 addr_high;
+ __le32 addr_low;
+};
+
+struct ice_aqc_nvm_comp_tbl {
+ __le16 comp_class;
+#define NVM_COMP_CLASS_ALL_FW 0x000A
+
+ __le16 comp_id;
+#define NVM_COMP_ID_OROM 0x5
+#define NVM_COMP_ID_NVM 0x6
+#define NVM_COMP_ID_NETLIST 0x8
+
+ u8 comp_class_idx;
+#define FWU_COMP_CLASS_IDX_NOT_USE 0x0
+
+ __le32 comp_cmp_stamp;
+ u8 cvs_type;
+#define NVM_CVS_TYPE_ASCII 0x1
+
+ u8 cvs_len;
+ u8 cvs[]; /* Component Version String */
+} __packed;
+
/**
* Send to PF command (indirect 0x0801) ID is only used by PF
*
@@ -1795,6 +1863,8 @@ struct ice_aq_desc {
struct ice_aqc_rl_profile rl_profile;
struct ice_aqc_nvm nvm;
struct ice_aqc_nvm_checksum nvm_checksum;
+ struct ice_aqc_nvm_pkg_data pkg_data;
+ struct ice_aqc_nvm_pass_comp_tbl pass_comp_tbl;
struct ice_aqc_pf_vf_msg virt;
struct ice_aqc_lldp_get_mib lldp_get_mib;
struct ice_aqc_lldp_set_mib_change lldp_set_event;
@@ -1929,7 +1999,13 @@ enum ice_adminq_opc {
/* NVM commands */
ice_aqc_opc_nvm_read = 0x0701,
+ ice_aqc_opc_nvm_erase = 0x0702,
+ ice_aqc_opc_nvm_write = 0x0703,
ice_aqc_opc_nvm_checksum = 0x0706,
+ ice_aqc_opc_nvm_write_activate = 0x0707,
+ ice_aqc_opc_nvm_update_empr = 0x0709,
+ ice_aqc_opc_nvm_pkg_data = 0x070A,
+ ice_aqc_opc_nvm_pass_component_tbl = 0x070B,
/* PF/VF mailbox commands */
ice_mbx_opc_send_msg_to_pf = 0x0801,
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index b4bf6f84b464..485e2ac792b5 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -11,8 +11,6 @@
#include "ice_switch.h"
#include <linux/avf/virtchnl.h>
-enum ice_status ice_nvm_validate_checksum(struct ice_hw *hw);
-
enum ice_status ice_init_hw(struct ice_hw *hw);
void ice_deinit_hw(struct ice_hw *hw);
enum ice_status ice_check_reset(struct ice_hw *hw);
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
index 7c2a06892bbb..5903a36763de 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.c
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
@@ -107,6 +107,76 @@ ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
return status;
}
+/**
+ * ice_aq_update_nvm
+ * @hw: pointer to the HW struct
+ * @module_typeid: module pointer location in words from the NVM beginning
+ * @offset: byte offset from the module beginning
+ * @length: length of the section to be written (in bytes from the offset)
+ * @data: command buffer (size [bytes] = length)
+ * @last_command: tells if this is the last command in a series
+ * @command_flags: command parameters
+ * @cd: pointer to command details structure or NULL
+ *
+ * Update the NVM using the admin queue commands (0x0703)
+ */
+enum ice_status
+ice_aq_update_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
+ u16 length, void *data, bool last_command, u8 command_flags,
+ struct ice_sq_cd *cd)
+{
+ struct ice_aq_desc desc;
+ struct ice_aqc_nvm *cmd;
+
+ cmd = &desc.params.nvm;
+
+ /* In offset the highest byte must be zeroed. */
+ if (offset & 0xFF000000)
+ return ICE_ERR_PARAM;
+
+ ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_nvm_write);
+
+ cmd->cmd_flags |= command_flags;
+
+ /* If this is the last command in a series, set the proper flag. */
+ if (last_command)
+ cmd->cmd_flags |= ICE_AQC_NVM_LAST_CMD;
+ cmd->module_typeid = cpu_to_le16(module_typeid);
+ cmd->offset_low = cpu_to_le16(offset & 0xFFFF);
+ cmd->offset_high = (offset >> 16) & 0xFF;
+ cmd->length = cpu_to_le16(length);
+
+ desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+
+ return ice_aq_send_cmd(hw, &desc, data, length, cd);
+}
+
+/**
+ * ice_aq_erase_nvm
+ * @hw: pointer to the HW struct
+ * @module_typeid: module pointer location in words from the NVM beginning
+ * @cd: pointer to command details structure or NULL
+ *
+ * Erase the NVM sector using the admin queue commands (0x0702)
+ */
+enum ice_status
+ice_aq_erase_nvm(struct ice_hw *hw, u16 module_typeid, struct ice_sq_cd *cd)
+{
+ struct ice_aq_desc desc;
+ struct ice_aqc_nvm *cmd;
+
+ cmd = &desc.params.nvm;
+
+ ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_nvm_erase);
+
+ cmd->module_typeid = cpu_to_le16(module_typeid);
+ cmd->length = cpu_to_le16(ICE_AQC_NVM_ERASE_LEN);
+ cmd->offset_low = 0;
+ cmd->offset_high = 0;
+
+ return ice_aq_send_cmd(hw, &desc, NULL, 0, cd);
+}
+
/**
* ice_read_sr_word_aq - Reads Shadow RAM via AQ
* @hw: pointer to the HW structure
@@ -634,3 +704,119 @@ enum ice_status ice_nvm_validate_checksum(struct ice_hw *hw)
return status;
}
+
+/**
+ * ice_nvm_write_activate
+ * @hw: pointer to the HW struct
+ * @cmd_flags: NVM activate admin command bits (banks to be validated)
+ *
+ * Update the control word with the required banks' validity bits
+ * and dumps the Shadow RAM to flash (0x0707)
+ */
+enum ice_status ice_nvm_write_activate(struct ice_hw *hw, u8 cmd_flags)
+{
+ struct ice_aqc_nvm *cmd;
+ struct ice_aq_desc desc;
+
+ cmd = &desc.params.nvm;
+ ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_nvm_write_activate);
+
+ cmd->cmd_flags = cmd_flags;
+
+ return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
+}
+
+/**
+ * ice_aq_nvm_update_empr
+ * @hw: pointer to the HW struct
+ *
+ * Update empr (0x0709). This command allows SW to
+ * request an EMPR to activate new FW.
+ */
+enum ice_status ice_aq_nvm_update_empr(struct ice_hw *hw)
+{
+ struct ice_aq_desc desc;
+
+ ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_nvm_update_empr);
+
+ return ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
+}
+
+/* ice_nvm_set_pkg_data
+ * @hw: pointer to the HW struct
+ * @del_pkg_data_flag: If is set then the current pkg_data store by FW
+ * is deleted.
+ * If bit is set to 1, then buffer should be size 0.
+ * @data: pointer to buffer
+ * @length: length of the buffer
+ * @cd: pointer to command details structure or NULL
+ *
+ * Set package data (0x070A). This command is equivalent to the reception
+ * of a PLDM FW Update GetPackageData cmd. This command should be sent
+ * as part of the NVM update as the first cmd in the flow.
+ */
+
+enum ice_status
+ice_nvm_set_pkg_data(struct ice_hw *hw, bool del_pkg_data_flag, u8 *data,
+ u16 length, struct ice_sq_cd *cd)
+{
+ struct ice_aqc_nvm_pkg_data *cmd;
+ struct ice_aq_desc desc;
+
+ if (length != 0 && !data)
+ return ICE_ERR_PARAM;
+
+ cmd = &desc.params.pkg_data;
+
+ ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_nvm_pkg_data);
+ desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+
+ if (del_pkg_data_flag)
+ cmd->cmd_flags |= ICE_AQC_NVM_PKG_DELETE;
+
+ return ice_aq_send_cmd(hw, &desc, data, length, cd);
+}
+
+/* ice_nvm_pass_component_tbl
+ * @hw: pointer to the HW struct
+ * @data: pointer to buffer
+ * @length: length of the buffer
+ * @transfer_flag: parameter for determining stage of the update
+ * @comp_response: a pointer to the response from the 0x070B AQC.
+ * @comp_response_code: a pointer to the response code from the 0x070B AQC.
+ * @cd: pointer to command details structure or NULL
+ *
+ * Pass component table (0x070B). This command is equivalent to the reception
+ * of a PLDM FW Update PassComponentTable cmd. This command should be sent once
+ * per component. It can be only sent after Set Package Data cmd and before
+ * actual update. FW will assume these commands are going to be sent until
+ * the TransferFlag is set to End or StartAndEnd.
+ */
+
+enum ice_status
+ice_nvm_pass_component_tbl(struct ice_hw *hw, u8 *data, u16 length,
+ u8 transfer_flag, u8 *comp_response,
+ u8 *comp_response_code, struct ice_sq_cd *cd)
+{
+ struct ice_aqc_nvm_pass_comp_tbl *cmd;
+ struct ice_aq_desc desc;
+ enum ice_status status;
+
+ if (!data || !comp_response || !comp_response_code)
+ return ICE_ERR_PARAM;
+
+ cmd = &desc.params.pass_comp_tbl;
+
+ ice_fill_dflt_direct_cmd_desc(&desc,
+ ice_aqc_opc_nvm_pass_component_tbl);
+ desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
+
+ cmd->transfer_flag = transfer_flag;
+ status = ice_aq_send_cmd(hw, &desc, data, length, cd);
+
+ if (!status) {
+ *comp_response = cmd->component_response;
+ *comp_response_code = cmd->component_response_code;
+ }
+ return status;
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
index 999f273ba6ad..8d430909f846 100644
--- a/drivers/net/ethernet/intel/ice/ice_nvm.h
+++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
@@ -17,4 +17,20 @@ enum ice_status
ice_read_pba_string(struct ice_hw *hw, u8 *pba_num, u32 pba_num_size);
enum ice_status ice_init_nvm(struct ice_hw *hw);
enum ice_status ice_read_sr_word(struct ice_hw *hw, u16 offset, u16 *data);
+enum ice_status
+ice_aq_update_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset,
+ u16 length, void *data, bool last_command, u8 command_flags,
+ struct ice_sq_cd *cd);
+enum ice_status
+ice_aq_erase_nvm(struct ice_hw *hw, u16 module_typeid, struct ice_sq_cd *cd);
+enum ice_status ice_nvm_validate_checksum(struct ice_hw *hw);
+enum ice_status ice_nvm_write_activate(struct ice_hw *hw, u8 cmd_flags);
+enum ice_status ice_aq_nvm_update_empr(struct ice_hw *hw);
+enum ice_status
+ice_nvm_set_pkg_data(struct ice_hw *hw, bool del_pkg_data_flag, u8 *data,
+ u16 length, struct ice_sq_cd *cd);
+enum ice_status
+ice_nvm_pass_component_tbl(struct ice_hw *hw, u8 *data, u16 length,
+ u8 transfer_flag, u8 *comp_response,
+ u8 *comp_response_code, struct ice_sq_cd *cd);
#endif /* _ICE_NVM_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index f91b6d944b63..4314da808679 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -801,6 +801,9 @@ enum ice_sw_fwd_act_type {
#define ICE_OROM_VER_SHIFT 24
#define ICE_OROM_VER_MASK (0xff << ICE_OROM_VER_SHIFT)
#define ICE_SR_PFA_PTR 0x40
+#define ICE_SR_1ST_NVM_BANK_PTR 0x42
+#define ICE_SR_1ST_OROM_BANK_PTR 0x44
+#define ICE_SR_NETLIST_BANK_PTR 0x46
#define ICE_SR_SECTOR_SIZE_IN_WORDS 0x800
/* Link override related */
--
2.27.0.353.gb9a2d1a0207f
^ permalink raw reply related [flat|nested] 36+ messages in thread* [RFC PATCH net-next v2 3/6] ice: add flags indicating pending update of firmware module
2020-07-17 18:35 [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jacob Keller
2020-07-17 18:35 ` [RFC PATCH net-next v2 1/6] ice: Add support for unified NVM update flow capability Jacob Keller
2020-07-17 18:35 ` [RFC PATCH net-next v2 2/6] ice: Add AdminQ commands for FW update Jacob Keller
@ 2020-07-17 18:35 ` Jacob Keller
2020-07-17 18:35 ` [RFC PATCH net-next v2 4/6] Add pldmfw library for PLDM firmware update Jacob Keller
` (3 subsequent siblings)
6 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-17 18:35 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Tom Herbert, Jacob Keller
After a flash update, the pending status of the update can be determined
from the device capabilities.
Read the appropriate device capability and store whether there is
a pending update awaiting a reboot.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 6 ++++++
drivers/net/ethernet/intel/ice/ice_common.c | 12 ++++++++++++
drivers/net/ethernet/intel/ice/ice_type.h | 6 ++++++
3 files changed, 24 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index d81ee985a2c3..a35739d726a7 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -109,6 +109,12 @@ struct ice_aqc_list_caps_elem {
#define ICE_AQC_CAPS_MSIX 0x0043
#define ICE_AQC_CAPS_FD 0x0045
#define ICE_AQC_CAPS_MAX_MTU 0x0047
+#define ICE_AQC_CAPS_NVM_VER 0x0048
+#define ICE_AQC_CAPS_PENDING_NVM_VER 0x0049
+#define ICE_AQC_CAPS_OROM_VER 0x004A
+#define ICE_AQC_CAPS_PENDING_OROM_VER 0x004B
+#define ICE_AQC_CAPS_NET_VER 0x004C
+#define ICE_AQC_CAPS_PENDING_NET_VER 0x004D
#define ICE_AQC_CAPS_NVM_MGMT 0x0080
u8 major_ver;
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index ffa7490b5789..2e9a43bae67f 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1855,6 +1855,18 @@ ice_parse_common_caps(struct ice_hw *hw, struct ice_hw_common_caps *caps,
"%s: msix_vector_first_id = %d\n", prefix,
caps->msix_vector_first_id);
break;
+ case ICE_AQC_CAPS_PENDING_NVM_VER:
+ caps->nvm_update_pending_nvm = true;
+ ice_debug(hw, ICE_DBG_INIT, "%s: update_pending_nvm\n", prefix);
+ break;
+ case ICE_AQC_CAPS_PENDING_OROM_VER:
+ caps->nvm_update_pending_orom = true;
+ ice_debug(hw, ICE_DBG_INIT, "%s: update_pending_orom\n", prefix);
+ break;
+ case ICE_AQC_CAPS_PENDING_NET_VER:
+ caps->nvm_update_pending_netlist = true;
+ ice_debug(hw, ICE_DBG_INIT, "%s: update_pending_netlist\n", prefix);
+ break;
case ICE_AQC_CAPS_NVM_MGMT:
caps->nvm_unified_update =
(number & ICE_NVM_MGMT_UNIFIED_UPD_SUPPORT) ?
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 4314da808679..714bb5749fd4 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -263,6 +263,12 @@ struct ice_hw_common_caps {
u8 dcb;
+ bool nvm_update_pending_nvm;
+ bool nvm_update_pending_orom;
+ bool nvm_update_pending_netlist;
+#define ICE_NVM_PENDING_NVM_IMAGE BIT(0)
+#define ICE_NVM_PENDING_OROM BIT(1)
+#define ICE_NVM_PENDING_NETLIST BIT(2)
bool nvm_unified_update;
#define ICE_NVM_MGMT_UNIFIED_UPD_SUPPORT BIT(3)
};
--
2.27.0.353.gb9a2d1a0207f
^ permalink raw reply related [flat|nested] 36+ messages in thread* [RFC PATCH net-next v2 4/6] Add pldmfw library for PLDM firmware update
2020-07-17 18:35 [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jacob Keller
` (2 preceding siblings ...)
2020-07-17 18:35 ` [RFC PATCH net-next v2 3/6] ice: add flags indicating pending update of firmware module Jacob Keller
@ 2020-07-17 18:35 ` Jacob Keller
2020-07-17 18:35 ` [RFC PATCH net-next v2 5/6] ice: implement device flash update via devlink Jacob Keller
` (2 subsequent siblings)
6 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-17 18:35 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Tom Herbert, Jacob Keller
The pldmfw library is used to implement common logic needed to flash
devices based on firmware files using the format described by the PLDM
for Firmware Update standard.
This library consists of logic to parse the PLDM file format from
a firmware file object, as well as common logic for sending the relevant
PLDM header data to the device firmware.
A simple ops table is provided so that device drivers can implement
device specific hardware interactions while keeping the common logic to
the pldmfw library.
This library will be used by the Intel ice networking driver as part of
implementing device flash update via devlink. The library aims to be
vendor and device agnostic. For this reason, it has been placed in
lib/pldmfw, in the hopes that other devices which use the PLDM firmware
file format may benefit from it in the future. However, do note that not
all features defined in the PLDM standard have been implemented.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Documentation/driver-api/index.rst | 1 +
.../driver-api/pldmfw/driver-ops.rst | 56 ++
.../driver-api/pldmfw/file-format.rst | 203 ++++
Documentation/driver-api/pldmfw/index.rst | 72 ++
MAINTAINERS | 7 +
include/linux/pldmfw.h | 165 ++++
lib/Kconfig | 4 +
lib/Makefile | 3 +
lib/pldmfw/Makefile | 2 +
lib/pldmfw/pldmfw.c | 879 ++++++++++++++++++
lib/pldmfw/pldmfw_private.h | 238 +++++
11 files changed, 1630 insertions(+)
create mode 100644 Documentation/driver-api/pldmfw/driver-ops.rst
create mode 100644 Documentation/driver-api/pldmfw/file-format.rst
create mode 100644 Documentation/driver-api/pldmfw/index.rst
create mode 100644 include/linux/pldmfw.h
create mode 100644 lib/pldmfw/Makefile
create mode 100644 lib/pldmfw/pldmfw.c
create mode 100644 lib/pldmfw/pldmfw_private.h
diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
index 6567187e7687..7fc1e0cccae7 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -95,6 +95,7 @@ available subsections can be seen below.
phy/index
pti_intel_mid
pwm
+ pldmfw/index
rfkill
serial/index
sm501
diff --git a/Documentation/driver-api/pldmfw/driver-ops.rst b/Documentation/driver-api/pldmfw/driver-ops.rst
new file mode 100644
index 000000000000..f0654783d3b3
--- /dev/null
+++ b/Documentation/driver-api/pldmfw/driver-ops.rst
@@ -0,0 +1,56 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+=========================
+Driver-specific callbacks
+=========================
+
+The ``pldmfw`` module relies on the device driver for implementing device
+specific behavior using the following operations.
+
+``.match_record``
+-----------------
+
+The ``.match_record`` operation is used to determine whether a given PLDM
+record matches the device being updated. This requires comparing the record
+descriptors in the record with information from the device. Many record
+descriptors are defined by the PLDM standard, but it is also allowed for
+devices to implement their own descriptors.
+
+The ``.match_record`` operation should return true if a given record matches
+the device.
+
+``.send_package_data``
+----------------------
+
+The ``.send_package_data`` operation is used to send the device-specific
+package data in a record to the device firmware. If the matching record
+provides package data, ``pldmfw`` will call the ``.send_package_data``
+function with a pointer to the package data and with the package data
+length. The device driver should send this data to firmware.
+
+``.send_component_table``
+-------------------------
+
+The ``.send_component_table`` operation is used to forward component
+information to the device. It is called once for each applicable component,
+that is, for each component indicated by the matching record. The
+device driver should send the component information to the device firmware,
+and wait for a response. The provided transfer flag indicates whether this
+is the first, last, or a middle component, and is expected to be forwarded
+to firmware as part of the component table information. The driver should an
+error in the case when the firmware indicates that the component cannot be
+updated, or return zero if the component can be updated.
+
+``.flash_component``
+--------------------
+
+The ``.flash_component`` operation is used to inform the device driver to
+flash a given component. The driver must perform any steps necessary to send
+the component data to the device.
+
+``.finalize_update``
+--------------------
+
+The ``.finalize_update`` operation is used by the ``pldmfw`` library in
+order to allow the device driver to perform any remaining device specific
+logic needed to finish the update.
diff --git a/Documentation/driver-api/pldmfw/file-format.rst b/Documentation/driver-api/pldmfw/file-format.rst
new file mode 100644
index 000000000000..b7a9cebe09c6
--- /dev/null
+++ b/Documentation/driver-api/pldmfw/file-format.rst
@@ -0,0 +1,203 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+==================================
+PLDM Firmware file format overview
+==================================
+
+A PLDM firmware package is a binary file which contains a header that
+describes the contents of the firmware package. This includes an initial
+package header, one or more firmware records, and one or more components
+describing the actual flash contents to program.
+
+This diagram provides an overview of the file format::
+
+ overall file layout
+ +----------------------+
+ | |
+ | Package Header |
+ | |
+ +----------------------+
+ | |
+ | Device Records |
+ | |
+ +----------------------+
+ | |
+ | Component Info |
+ | |
+ +----------------------+
+ | |
+ | Package Header CRC |
+ | |
+ +----------------------+
+ | |
+ | Component Image 1 |
+ | |
+ +----------------------+
+ | |
+ | Component Image 2 |
+ | |
+ +----------------------+
+ | |
+ | ... |
+ | |
+ +----------------------+
+ | |
+ | Component Image N |
+ | |
+ +----------------------+
+
+Package Header
+==============
+
+The package header begins with the UUID of the PLDM file format, and
+contains information about the version of the format that the file uses. It
+also includes the total header size, a release date, the size of the
+component bitmap, and an overall package version.
+
+The following diagram provides an overview of the package header::
+
+ header layout
+ +-------------------------+
+ | PLDM UUID |
+ +-------------------------+
+ | Format Revision |
+ +-------------------------+
+ | Header Size |
+ +-------------------------+
+ | Release Date |
+ +-------------------------+
+ | Component Bitmap Length |
+ +-------------------------+
+ | Package Version Info |
+ +-------------------------+
+
+Device Records
+==============
+
+The device firmware records area starts with a count indicating the total
+number of records in the file, followed by each record. A single device
+record describes what device matches this record. All valid PLDM firmware
+files must contain at least one record, but optionally may contain more than
+one record if they support multiple devices.
+
+Each record will identify the device it supports via TLVs that describe the
+device, such as the PCI device and vendor information. It will also indicate
+which set of components that are used by this device. It is possible that
+only subset of provided components will be used by a given record. A record
+may also optionally contain device-specific package data that will be used
+by the device firmware during the update process.
+
+The following diagram provides an overview of the device record area::
+
+ area layout
+ +---------------+
+ | |
+ | Record Count |
+ | |
+ +---------------+
+ | |
+ | Record 1 |
+ | |
+ +---------------+
+ | |
+ | Record 2 |
+ | |
+ +---------------+
+ | |
+ | ... |
+ | |
+ +---------------+
+ | |
+ | Record N |
+ | |
+ +---------------+
+
+ record layout
+ +-----------------------+
+ | Record Length |
+ +-----------------------+
+ | Descriptor Count |
+ +-----------------------+
+ | Option Flags |
+ +-----------------------+
+ | Version Settings |
+ +-----------------------+
+ | Package Data Length |
+ +-----------------------+
+ | Applicable Components |
+ +-----------------------+
+ | Version String |
+ +-----------------------+
+ | Descriptor TLVs |
+ +-----------------------+
+ | Package Data |
+ +-----------------------+
+
+Component Info
+==============
+
+The component information area begins with a count of the number of
+components. Following this count is a description for each component. The
+component information points to the location in the file where the component
+data is stored, and includes version data used to identify the version of
+the component.
+
+The following diagram provides an overview of the component area::
+
+ area layout
+ +-----------------+
+ | |
+ | Component Count |
+ | |
+ +-----------------+
+ | |
+ | Component 1 |
+ | |
+ +-----------------+
+ | |
+ | Component 2 |
+ | |
+ +-----------------+
+ | |
+ | ... |
+ | |
+ +-----------------+
+ | |
+ | Component N |
+ | |
+ +-----------------+
+
+ component layout
+ +------------------------+
+ | Classification |
+ +------------------------+
+ | Component Identifier |
+ +------------------------+
+ | Comparison Stamp |
+ +------------------------+
+ | Component Options |
+ +------------------------+
+ | Activation Method |
+ +------------------------+
+ | Location Offset |
+ +------------------------+
+ | Component Size |
+ +------------------------+
+ | Component Version Info |
+ +------------------------+
+ | Package Data |
+ +------------------------+
+
+
+Package Header CRC
+==================
+
+Following the component information is a short 4-byte CRC calculated over
+the contents of all of the header information.
+
+Component Images
+================
+
+The component images follow the package header information in the PLDM
+firmware file. Each of these is simply a binary chunk with its start and
+size defined by the matching component structure in the component info area.
diff --git a/Documentation/driver-api/pldmfw/index.rst b/Documentation/driver-api/pldmfw/index.rst
new file mode 100644
index 000000000000..ad2c33ece30f
--- /dev/null
+++ b/Documentation/driver-api/pldmfw/index.rst
@@ -0,0 +1,72 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+==================================
+PLDM Firmware Flash Update Library
+==================================
+
+``pldmfw`` implements functionality for updating the flash on a device using
+the PLDM for Firmware Update standard
+<https://www.dmtf.org/documents/pmci/pldm-firmware-update-specification-100>.
+
+.. toctree::
+ :maxdepth: 1
+
+ file-format
+ driver-ops
+
+==================================
+Overview of the ``pldmfw`` library
+==================================
+
+The ``pldmfw`` library is intended to be used by device drivers for
+implementing device flash update based on firmware files following the PLDM
+firwmare file format.
+
+It is implemented using an ops table that allows device drivers to provide
+the underlying device specific functionality.
+
+``pldmfw`` implements logic to parse the packed binary format of the PLDM
+firmware file into data structures, and then uses the provided function
+operations to determine if the firmware file is a match for the device. If
+so, it sends the record and component data to the firmware using the device
+specific implementations provided by device drivers. Once the device
+firmware indicates that the update may be performed, the firmware data is
+sent to the device for programming.
+
+Parsing the PLDM file
+=====================
+
+The PLDM file format uses packed binary data, with most multi-byte fields
+stored in the Little Endian format. Several pieces of data are variable
+length, including version strings and the number of records and components.
+Due to this, it is not straight forward to index the record, record
+descriptors, or components.
+
+To avoid proliferating access to the packed binary data, the ``pldmfw``
+library parses and extracts this data into simpler structures for ease of
+access.
+
+In order to safely process the firmware file, care is taken to avoid
+unaligned access of multi-byte fields, and to properly convert from Little
+Endian to CPU host format. Additionally the records, descriptors, and
+components are stored in linked lists.
+
+Performing a flash update
+=========================
+
+To perform a flash update, the ``pldmfw`` module performs the following
+steps
+
+1. Parse the firmware file for record and component information
+2. Scan through the records and determine if the device matches any record
+ in the file. The first matched record will be used.
+3. If the matching record provides package data, send this package data to
+ the device.
+4. For each component that the record indicates, send the component data to
+ the device. For each component, the firmware may respond with an
+ indication of whether the update is suitable or not. If any component is
+ not suitable, the update is canceled.
+5. For each component, send the binary data to the device firmware for
+ updating.
+6. After all components are programmed, perform any final device-specific
+ actions to finalize the update.
diff --git a/MAINTAINERS b/MAINTAINERS
index 46274e134eda..8e2e180e33b6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13587,6 +13587,13 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/chemical/plantower,pms7003.yaml
F: drivers/iio/chemical/pms7003.c
+PLDMFW LIBRARY
+M: Jacob Keller <jacob.e.keller@intel.com>
+S: Maintained
+F: Documentation/driver-api/pldmfw/
+F: include/linux/pldmfw.h
+F: lib/pldmfw/
+
PLX DMA DRIVER
M: Logan Gunthorpe <logang@deltatee.com>
S: Maintained
diff --git a/include/linux/pldmfw.h b/include/linux/pldmfw.h
new file mode 100644
index 000000000000..0fc831338226
--- /dev/null
+++ b/include/linux/pldmfw.h
@@ -0,0 +1,165 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2018-2019, Intel Corporation. */
+
+#ifndef _PLDMFW_H_
+#define _PLDMFW_H_
+
+#include <linux/list.h>
+#include <linux/firmware.h>
+
+#define PLDM_DEVICE_UPDATE_CONTINUE_AFTER_FAIL BIT(0)
+
+#define PLDM_STRING_TYPE_UNKNOWN 0
+#define PLDM_STRING_TYPE_ASCII 1
+#define PLDM_STRING_TYPE_UTF8 2
+#define PLDM_STRING_TYPE_UTF16 3
+#define PLDM_STRING_TYPE_UTF16LE 4
+#define PLDM_STRING_TYPE_UTF16BE 5
+
+struct pldmfw_record {
+ struct list_head entry;
+
+ /* List of descriptor TLVs */
+ struct list_head descs;
+
+ /* Component Set version string*/
+ const u8 *version_string;
+ u8 version_type;
+ u8 version_len;
+
+ /* Package Data length */
+ u16 package_data_len;
+
+ /* Bitfield of Device Update Flags */
+ u32 device_update_flags;
+
+ /* Package Data block */
+ const u8 *package_data;
+
+ /* Bitmap of components applicable to this record */
+ unsigned long *component_bitmap;
+ u16 component_bitmap_len;
+};
+
+/* Standard descriptor TLV identifiers */
+#define PLDM_DESC_ID_PCI_VENDOR_ID 0x0000
+#define PLDM_DESC_ID_IANA_ENTERPRISE_ID 0x0001
+#define PLDM_DESC_ID_UUID 0x0002
+#define PLDM_DESC_ID_PNP_VENDOR_ID 0x0003
+#define PLDM_DESC_ID_ACPI_VENDOR_ID 0x0004
+#define PLDM_DESC_ID_PCI_DEVICE_ID 0x0100
+#define PLDM_DESC_ID_PCI_SUBVENDOR_ID 0x0101
+#define PLDM_DESC_ID_PCI_SUBDEV_ID 0x0102
+#define PLDM_DESC_ID_PCI_REVISION_ID 0x0103
+#define PLDM_DESC_ID_PNP_PRODUCT_ID 0x0104
+#define PLDM_DESC_ID_ACPI_PRODUCT_ID 0x0105
+#define PLDM_DESC_ID_VENDOR_DEFINED 0xFFFF
+
+struct pldmfw_desc_tlv {
+ struct list_head entry;
+
+ const u8 *data;
+ u16 type;
+ u16 size;
+};
+
+#define PLDM_CLASSIFICATION_UNKNOWN 0x0000
+#define PLDM_CLASSIFICATION_OTHER 0x0001
+#define PLDM_CLASSIFICATION_DRIVER 0x0002
+#define PLDM_CLASSIFICATION_CONFIG_SW 0x0003
+#define PLDM_CLASSIFICATION_APP_SW 0x0004
+#define PLDM_CLASSIFICATION_INSTRUMENTATION 0x0005
+#define PLDM_CLASSIFICATION_BIOS 0x0006
+#define PLDM_CLASSIFICATION_DIAGNOSTIC_SW 0x0007
+#define PLDM_CLASSIFICATION_OS 0x0008
+#define PLDM_CLASSIFICATION_MIDDLEWARE 0x0009
+#define PLDM_CLASSIFICATION_FIRMWARE 0x000A
+#define PLDM_CLASSIFICATION_CODE 0x000B
+#define PLDM_CLASSIFICATION_SERVICE_PACK 0x000C
+#define PLDM_CLASSIFICATION_SOFTWARE_BUNDLE 0x000D
+
+#define PLDM_ACTIVATION_METHOD_AUTO BIT(0)
+#define PLDM_ACTIVATION_METHOD_SELF_CONTAINED BIT(1)
+#define PLDM_ACTIVATION_METHOD_MEDIUM_SPECIFIC BIT(2)
+#define PLDM_ACTIVATION_METHOD_REBOOT BIT(3)
+#define PLDM_ACTIVATION_METHOD_DC_CYCLE BIT(4)
+#define PLDM_ACTIVATION_METHOD_AC_CYCLE BIT(5)
+
+#define PLDMFW_COMPONENT_OPTION_FORCE_UPDATE BIT(0)
+#define PLDMFW_COMPONENT_OPTION_USE_COMPARISON_STAMP BIT(1)
+
+struct pldmfw_component {
+ struct list_head entry;
+
+ /* component identifier */
+ u16 classification;
+ u16 identifier;
+
+ u16 options;
+ u16 activation_method;
+
+ u32 comparison_stamp;
+
+ u32 component_size;
+ const u8 *component_data;
+
+ /* Component version string */
+ const u8 *version_string;
+ u8 version_type;
+ u8 version_len;
+
+ /* component index */
+ u8 index;
+
+};
+
+/* Transfer flag used for sending components to the firmware */
+#define PLDM_TRANSFER_FLAG_START BIT(0)
+#define PLDM_TRANSFER_FLAG_MIDDLE BIT(1)
+#define PLDM_TRANSFER_FLAG_END BIT(2)
+
+struct pldmfw_ops;
+
+/* Main entry point to the PLDM firmware update engine. Device drivers
+ * should embed this in a private structure and use container_of to obtain
+ * a pointer to their own data, used to implement the device specific
+ * operations.
+ */
+struct pldmfw {
+ const struct pldmfw_ops *ops;
+ struct device *dev;
+};
+
+bool pldmfw_op_pci_match_record(struct pldmfw *context, struct pldmfw_record *record);
+
+/* Operations invoked by the generic PLDM firmware update engine. Used to
+ * implement device specific logic.
+ *
+ * @match_record: check if the device matches the given record. For
+ * convenience, a standard implementation is provided for PCI devices.
+ *
+ * @send_package_data: send the package data associated with the matching
+ * record to firmware.
+ *
+ * @send_component_table: send the component data associated with a given
+ * component to firmware. Called once for each applicable component.
+ *
+ * @flash_component: Flash the data for a given component to the device.
+ * Called once for each applicable component, after all component tables have
+ * been sent.
+ *
+ * @finalize_update: (optional) Finish the update. Called after all components
+ * have been flashed.
+ */
+struct pldmfw_ops {
+ bool (*match_record)(struct pldmfw *context, struct pldmfw_record *record);
+ int (*send_package_data)(struct pldmfw *context, const u8 *data, u16 length);
+ int (*send_component_table)(struct pldmfw *context, struct pldmfw_component *component,
+ u8 transfer_flag);
+ int (*flash_component)(struct pldmfw *context, struct pldmfw_component *component);
+ int (*finalize_update)(struct pldmfw *context);
+};
+
+int pldmfw_flash_image(struct pldmfw *context, const struct firmware *fw);
+
+#endif
diff --git a/lib/Kconfig b/lib/Kconfig
index df3f3da95990..3ffbca6998e5 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -676,3 +676,7 @@ config GENERIC_LIB_CMPDI2
config GENERIC_LIB_UCMPDI2
bool
+
+config PLDMFW
+ bool
+ default n
diff --git a/lib/Makefile b/lib/Makefile
index b1c42c10073b..281888ff713b 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -315,6 +315,9 @@ obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
obj-$(CONFIG_OBJAGG) += objagg.o
+# pldmfw library
+obj-$(CONFIG_PLDMFW) += pldmfw/
+
# KUnit tests
obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
diff --git a/lib/pldmfw/Makefile b/lib/pldmfw/Makefile
new file mode 100644
index 000000000000..99ad10711abe
--- /dev/null
+++ b/lib/pldmfw/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_PLDMFW) += pldmfw.o
diff --git a/lib/pldmfw/pldmfw.c b/lib/pldmfw/pldmfw.c
new file mode 100644
index 000000000000..e5d4b3b2af81
--- /dev/null
+++ b/lib/pldmfw/pldmfw.c
@@ -0,0 +1,879 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2018-2019, Intel Corporation. */
+
+#include <asm/unaligned.h>
+#include <linux/crc32.h>
+#include <linux/device.h>
+#include <linux/firmware.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pldmfw.h>
+#include <linux/slab.h>
+#include <linux/uuid.h>
+
+#include "pldmfw_private.h"
+
+/* Internal structure used to store details about the PLDM image file as it is
+ * being validated and processed.
+ */
+struct pldmfw_priv {
+ struct pldmfw *context;
+ const struct firmware *fw;
+
+ /* current offset of firmware image */
+ size_t offset;
+
+ struct list_head records;
+ struct list_head components;
+
+ /* PLDM Firmware Package Header */
+ const struct __pldm_header *header;
+ u16 total_header_size;
+
+ /* length of the component bitmap */
+ u16 component_bitmap_len;
+ u16 bitmap_size;
+
+ /* Start of the component image information */
+ u16 component_count;
+ const u8 *component_start;
+
+ /* Start pf the firmware device id records */
+ const u8 *record_start;
+ u8 record_count;
+
+ /* The CRC at the end of the package header */
+ u32 header_crc;
+
+ struct pldmfw_record *matching_record;
+};
+
+/**
+ * pldm_check_fw_space - Verify that the firmware image has space left
+ * @data: pointer to private data
+ * @offset: offset to start from
+ * @length: length to check for
+ *
+ * Verify that the firmware data can hold a chunk of bytes with the specified
+ * offset and length.
+ *
+ * Returns: zero on success, or -EFAULT if the image does not have enough
+ * space left to fit the expected length.
+ */
+static int
+pldm_check_fw_space(struct pldmfw_priv *data, size_t offset, size_t length)
+{
+ size_t expected_size = offset + length;
+ struct device *dev = data->context->dev;
+
+ if (data->fw->size < expected_size) {
+ dev_dbg(dev, "Firmware file size smaller than expected. Got %zu bytes, needed %zu bytes\n",
+ data->fw->size, expected_size);
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+/**
+ * pldm_move_fw_offset - Move the current firmware offset forward
+ * @data: pointer to private data
+ * @bytes_to_move: number of bytes to move the offset forward by
+ *
+ * Check that there is enough space past the current offset, and then move the
+ * offset forward by this ammount.
+ *
+ * Returns: zero on success, or -EFAULT if the image is too small to fit the
+ * expected length.
+ */
+static int
+pldm_move_fw_offset(struct pldmfw_priv *data, size_t bytes_to_move)
+{
+ int err;
+
+ err = pldm_check_fw_space(data, data->offset, bytes_to_move);
+ if (err)
+ return err;
+
+ data->offset += bytes_to_move;
+
+ return 0;
+}
+
+/**
+ * pldm_parse_header - Validate and extract details about the PLDM header
+ * @data: pointer to private data
+ *
+ * Performs initial basic verification of the PLDM image, up to the first
+ * firmware record.
+ *
+ * This includes the following checks and extractions
+ *
+ * * Verify that the UUID at the start of the header matches the expected
+ * value as defined in the DSP0267 PLDM specification
+ * * Check that the revision is 0x01
+ * * Extract the total header_size and verify that the image is large enough
+ * to contain at least the length of this header
+ * * Extract the size of the component bitmap length
+ * * Extract a pointer to the start of the record area
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int pldm_parse_header(struct pldmfw_priv *data)
+{
+ const struct __pldmfw_record_area *record_area;
+ struct device *dev = data->context->dev;
+ const struct __pldm_header *header;
+ size_t header_size;
+ int err;
+
+ err = pldm_move_fw_offset(data, sizeof(*header));
+ if (err)
+ return err;
+
+ header = (const struct __pldm_header *)data->fw->data;
+ data->header = header;
+
+ if (!uuid_equal(&header->id, &pldm_firmware_header_id)) {
+ dev_dbg(dev, "Invalid package header identifier. Expected UUID %pUB, but got %pUB\n",
+ &pldm_firmware_header_id, &header->id);
+ return -EINVAL;
+ }
+
+ if (header->revision != PACKAGE_HEADER_FORMAT_REVISION) {
+ dev_dbg(dev, "Invalid package header revision. Expected revision %u but got %u\n",
+ PACKAGE_HEADER_FORMAT_REVISION, header->revision);
+ return -EOPNOTSUPP;
+ }
+
+ data->total_header_size = get_unaligned_le16(&header->size);
+ header_size = data->total_header_size - sizeof(*header);
+
+ err = pldm_check_fw_space(data, data->offset, header_size);
+ if (err)
+ return err;
+
+ data->component_bitmap_len =
+ get_unaligned_le16(&header->component_bitmap_len);
+
+ if (data->component_bitmap_len % 8 != 0) {
+ dev_dbg(dev, "Invalid component bitmap length. The length is %u, which is not a multiple of 8\n",
+ data->component_bitmap_len);
+ return -EINVAL;
+ }
+
+ data->bitmap_size = data->component_bitmap_len / 8;
+
+ err = pldm_move_fw_offset(data, header->version_len);
+ if (err)
+ return err;
+
+ /* extract a pointer to the record area, which just follows the main
+ * PLDM header data.
+ */
+ record_area = (const struct __pldmfw_record_area *)(data->fw->data +
+ data->offset);
+
+ err = pldm_move_fw_offset(data, sizeof(*record_area));
+ if (err)
+ return err;
+
+ data->record_count = record_area->record_count;
+ data->record_start = record_area->records;
+
+ return 0;
+}
+
+/**
+ * pldm_check_desc_tlv_len - Check that the length matches expectation
+ * @data: pointer to image details
+ * @type: the descriptor type
+ * @size: the length from the descriptor header
+ *
+ * If the descriptor type is one of the documented descriptor types according
+ * to the standard, verify that the provided length matches.
+ *
+ * If the type is not recognized or is VENDOR_DEFINED, return zero.
+ *
+ * Returns: zero on success, or -EINVAL if the specified size of a standard
+ * TLV does not match the expected value defined for that TLV.
+ */
+static int
+pldm_check_desc_tlv_len(struct pldmfw_priv *data, u16 type, u16 size)
+{
+ struct device *dev = data->context->dev;
+ u16 expected_size;
+
+ switch (type) {
+ case PLDM_DESC_ID_PCI_VENDOR_ID:
+ case PLDM_DESC_ID_PCI_DEVICE_ID:
+ case PLDM_DESC_ID_PCI_SUBVENDOR_ID:
+ case PLDM_DESC_ID_PCI_SUBDEV_ID:
+ expected_size = 2;
+ break;
+ case PLDM_DESC_ID_PCI_REVISION_ID:
+ expected_size = 1;
+ break;
+ case PLDM_DESC_ID_PNP_VENDOR_ID:
+ expected_size = 3;
+ break;
+ case PLDM_DESC_ID_IANA_ENTERPRISE_ID:
+ case PLDM_DESC_ID_ACPI_VENDOR_ID:
+ case PLDM_DESC_ID_PNP_PRODUCT_ID:
+ case PLDM_DESC_ID_ACPI_PRODUCT_ID:
+ expected_size = 4;
+ break;
+ case PLDM_DESC_ID_UUID:
+ expected_size = 16;
+ break;
+ case PLDM_DESC_ID_VENDOR_DEFINED:
+ return 0;
+ default:
+ /* Do not report an error on an unexpected TLV */
+ dev_dbg(dev, "Found unrecognized TLV type 0x%04x\n", type);
+ return 0;
+ }
+
+ if (size != expected_size) {
+ dev_dbg(dev, "Found TLV type 0x%04x with unexpected length. Got %u bytes, but expected %u bytes\n",
+ type, size, expected_size);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/**
+ * pldm_parse_desc_tlvs - Check and skip past a number of TLVs
+ * @data: pointer to private data
+ * @record: pointer to the record this TLV belongs too
+ * @desc_count: descriptor count
+ *
+ * From the current offset, read and extract the descriptor TLVs, updating the
+ * current offset each time.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+pldm_parse_desc_tlvs(struct pldmfw_priv *data, struct pldmfw_record *record, u8 desc_count)
+{
+ const struct __pldmfw_desc_tlv *__desc;
+ const u8 *desc_start;
+ u8 i;
+
+ desc_start = data->fw->data + data->offset;
+
+ pldm_for_each_desc_tlv(i, __desc, desc_start, desc_count) {
+ struct pldmfw_desc_tlv *desc;
+ int err;
+ u16 type, size;
+
+ err = pldm_move_fw_offset(data, sizeof(*__desc));
+ if (err)
+ return err;
+
+ type = get_unaligned_le16(&__desc->type);
+
+ /* According to DSP0267, this only includes the data field */
+ size = get_unaligned_le16(&__desc->size);
+
+ err = pldm_check_desc_tlv_len(data, type, size);
+ if (err)
+ return err;
+
+ /* check that we have space and move the offset forward */
+ err = pldm_move_fw_offset(data, size);
+ if (err)
+ return err;
+
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
+
+ desc->type = type;
+ desc->size = size;
+ desc->data = __desc->data;
+
+ list_add_tail(&desc->entry, &record->descs);
+ }
+
+ return 0;
+}
+
+/**
+ * pldm_parse_one_record - Verify size of one PLDM record
+ * @data: pointer to image details
+ * @__record: pointer to the record to check
+ *
+ * This function checks that the record size does not exceed either the size
+ * of the firmware file or the total length specified in the header section.
+ *
+ * It also verifies that the recorded length of the start of the record
+ * matches the size calculated by adding the static structure length, the
+ * component bitmap length, the version string length, the length of all
+ * descriptor TLVs, and the length of the package data.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+pldm_parse_one_record(struct pldmfw_priv *data,
+ const struct __pldmfw_record_info *__record)
+{
+ struct pldmfw_record *record;
+ size_t measured_length;
+ int err;
+ const u8 *bitmap_ptr;
+ u16 record_len;
+ int i;
+
+ /* Make a copy and insert it into the record list */
+ record = kzalloc(sizeof(*record), GFP_KERNEL);
+ if (!record)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&record->descs);
+ list_add_tail(&record->entry, &data->records);
+
+ /* Then check that we have space and move the offset */
+ err = pldm_move_fw_offset(data, sizeof(*__record));
+ if (err)
+ return err;
+
+ record_len = get_unaligned_le16(&__record->record_len);
+ record->package_data_len = get_unaligned_le16(&__record->package_data_len);
+ record->version_len = __record->version_len;
+ record->version_type = __record->version_type;
+
+ bitmap_ptr = data->fw->data + data->offset;
+
+ /* check that we have space for the component bitmap length */
+ err = pldm_move_fw_offset(data, data->bitmap_size);
+ if (err)
+ return err;
+
+ record->component_bitmap_len = data->component_bitmap_len;
+ record->component_bitmap = bitmap_zalloc(record->component_bitmap_len,
+ GFP_KERNEL);
+ if (!record->component_bitmap)
+ return -ENOMEM;
+
+ for (i = 0; i < data->bitmap_size; i++)
+ bitmap_set_value8(record->component_bitmap, bitmap_ptr[i], i * 8);
+
+ record->version_string = data->fw->data + data->offset;
+
+ err = pldm_move_fw_offset(data, record->version_len);
+ if (err)
+ return err;
+
+ /* Scan through the descriptor TLVs and find the end */
+ err = pldm_parse_desc_tlvs(data, record, __record->descriptor_count);
+ if (err)
+ return err;
+
+ record->package_data = data->fw->data + data->offset;
+
+ err = pldm_move_fw_offset(data, record->package_data_len);
+ if (err)
+ return err;
+
+ measured_length = data->offset - ((const u8 *)__record - data->fw->data);
+ if (measured_length != record_len) {
+ dev_dbg(data->context->dev, "Unexpected record length. Measured record length is %zu bytes, expected length is %u bytes\n",
+ measured_length, record_len);
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+/**
+ * pldm_parse_records - Locate the start of the component area
+ * @data: pointer to private data
+ *
+ * Extract the record count, and loop through each record, searching for the
+ * component area.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int pldm_parse_records(struct pldmfw_priv *data)
+{
+ const struct __pldmfw_component_area *component_area;
+ const struct __pldmfw_record_info *record;
+ int err;
+ u8 i;
+
+ pldm_for_each_record(i, record, data->record_start, data->record_count) {
+ err = pldm_parse_one_record(data, record);
+ if (err)
+ return err;
+ }
+
+ /* Extract a pointer to the component area, which just follows the
+ * PLDM device record data.
+ */
+ component_area = (const struct __pldmfw_component_area *)(data->fw->data + data->offset);
+
+ err = pldm_move_fw_offset(data, sizeof(*component_area));
+ if (err)
+ return err;
+
+ data->component_count =
+ get_unaligned_le16(&component_area->component_image_count);
+ data->component_start = component_area->components;
+
+ return 0;
+}
+
+/**
+ * pldm_parse_components - Locate the CRC header checksum
+ * @data: pointer to private data
+ *
+ * Extract the component count, and find the pointer to the component area.
+ * Scan through each component searching for the end, which should point to
+ * the package header checksum.
+ *
+ * Extract the package header CRC and save it for verification.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int pldm_parse_components(struct pldmfw_priv *data)
+{
+ const struct __pldmfw_component_info *__component;
+ struct device *dev = data->context->dev;
+ const u8 *header_crc_ptr;
+ int err;
+ u8 i;
+
+ pldm_for_each_component(i, __component, data->component_start, data->component_count) {
+ struct pldmfw_component *component;
+ u32 offset, size;
+
+ err = pldm_move_fw_offset(data, sizeof(*__component));
+ if (err)
+ return err;
+
+ err = pldm_move_fw_offset(data, __component->version_len);
+ if (err)
+ return err;
+
+ offset = get_unaligned_le32(&__component->location_offset);
+ size = get_unaligned_le32(&__component->size);
+
+ err = pldm_check_fw_space(data, offset, size);
+ if (err)
+ return err;
+
+ component = kzalloc(sizeof(*component), GFP_KERNEL);
+ if (!component)
+ return -ENOMEM;
+
+ component->index = i;
+ component->classification = get_unaligned_le16(&__component->classification);
+ component->identifier = get_unaligned_le16(&__component->identifier);
+ component->comparison_stamp = get_unaligned_le32(&__component->comparison_stamp);
+ component->options = get_unaligned_le16(&__component->options);
+ component->activation_method = get_unaligned_le16(&__component->activation_method);
+ component->version_type = __component->version_type;
+ component->version_len = __component->version_len;
+ component->version_string = __component->version_string;
+ component->component_data = data->fw->data + offset;
+ component->component_size = size;
+
+ list_add_tail(&component->entry, &data->components);
+ }
+
+ header_crc_ptr = data->fw->data + data->offset;
+
+ err = pldm_move_fw_offset(data, sizeof(data->header_crc));
+ if (err)
+ return err;
+
+ /* Make sure that we reached the expected offset */
+ if (data->offset != data->total_header_size) {
+ dev_dbg(dev, "Invalid firmware header size. Expected %u but got %zu\n",
+ data->total_header_size, data->offset);
+ return -EFAULT;
+ }
+
+ data->header_crc = get_unaligned_le32(header_crc_ptr);
+
+ return 0;
+}
+
+/**
+ * pldm_verify_header_crc - Verify that the CRC in the header matches
+ * @data: pointer to private data
+ *
+ * Calculates the 32-bit CRC using the standard IEEE 802.3 CRC polynomial and
+ * compares it to the value stored in the header.
+ *
+ * Returns: zero on success if the CRC matches, or -EBADMSG on an invalid CRC.
+ */
+static int pldm_verify_header_crc(struct pldmfw_priv *data)
+{
+ struct device *dev = data->context->dev;
+ u32 calculated_crc;
+ size_t length;
+
+ /* Calculate the 32-bit CRC of the header header contents up to but
+ * not including the checksum. Note that the Linux crc32_le function
+ * does not perform an expected final XOR.
+ */
+ length = data->offset - sizeof(data->header_crc);
+ calculated_crc = crc32_le(~0, data->fw->data, length) ^ ~0;
+
+ if (calculated_crc != data->header_crc) {
+ dev_dbg(dev, "Invalid CRC in firmware header. Got 0x%08x but expected 0x%08x\n",
+ calculated_crc, data->header_crc);
+ return -EBADMSG;
+ }
+
+ return 0;
+}
+
+/**
+ * pldmfw_free_priv - Free memory allocated while parsing the PLDM image
+ * @data: pointer to the PLDM data structure
+ *
+ * Loops through and clears all allocated memory associated with each
+ * allocated descriptor, record, and component.
+ */
+static void pldmfw_free_priv(struct pldmfw_priv *data)
+{
+ struct pldmfw_component *component, *c_safe;
+ struct pldmfw_record *record, *r_safe;
+ struct pldmfw_desc_tlv *desc, *d_safe;
+
+ list_for_each_entry_safe(component, c_safe, &data->components, entry) {
+ list_del(&component->entry);
+ kfree(component);
+ }
+
+ list_for_each_entry_safe(record, r_safe, &data->records, entry) {
+ list_for_each_entry_safe(desc, d_safe, &record->descs, entry) {
+ list_del(&desc->entry);
+ kfree(desc);
+ }
+
+ if (record->component_bitmap) {
+ bitmap_free(record->component_bitmap);
+ record->component_bitmap = NULL;
+ }
+
+ list_del(&record->entry);
+ kfree(record);
+ }
+}
+
+/**
+ * pldm_parse_image - parse and extract details from PLDM image
+ * @data: pointer to private data
+ *
+ * Verify that the firmware file contains valid data for a PLDM firmware
+ * file. Extract useful pointers and data from the firmware file and store
+ * them in the data structure.
+ *
+ * The PLDM firmware file format is defined in DMTF DSP0267 1.0.0. Care
+ * should be taken to use get_unaligned_le* when accessing data from the
+ * pointers in data.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int pldm_parse_image(struct pldmfw_priv *data)
+{
+ int err;
+
+ if (WARN_ON(!(data->context->dev && data->fw->data && data->fw->size)))
+ return -EINVAL;
+
+ err = pldm_parse_header(data);
+ if (err)
+ return err;
+
+ err = pldm_parse_records(data);
+ if (err)
+ return err;
+
+ err = pldm_parse_components(data);
+ if (err)
+ return err;
+
+ return pldm_verify_header_crc(data);
+}
+
+/* these are u32 so that we can store PCI_ANY_ID */
+struct pldm_pci_record_id {
+ int vendor;
+ int device;
+ int subsystem_vendor;
+ int subsystem_device;
+};
+
+/**
+ * pldmfw_op_pci_match_record - Check if a PCI device matches the record
+ * @context: PLDM fw update structure
+ * @record: list of records extracted from the PLDM image
+ *
+ * Determine of the PCI device associated with this device matches the record
+ * data provided.
+ *
+ * Searches the descriptor TLVs and extracts the relevant descriptor data into
+ * a pldm_pci_record_id. This is then compared against the PCI device ID
+ * information.
+ *
+ * Returns: true if the device matches the record, false otherwise.
+ */
+bool pldmfw_op_pci_match_record(struct pldmfw *context, struct pldmfw_record *record)
+{
+ struct pci_dev *pdev = to_pci_dev(context->dev);
+ struct pldm_pci_record_id id = {
+ .vendor = PCI_ANY_ID,
+ .device = PCI_ANY_ID,
+ .subsystem_vendor = PCI_ANY_ID,
+ .subsystem_device = PCI_ANY_ID,
+ };
+ struct pldmfw_desc_tlv *desc;
+
+ list_for_each_entry(desc, &record->descs, entry) {
+ u16 value;
+ int *ptr;
+
+ switch (desc->type) {
+ case PLDM_DESC_ID_PCI_VENDOR_ID:
+ ptr = &id.vendor;
+ break;
+ case PLDM_DESC_ID_PCI_DEVICE_ID:
+ ptr = &id.device;
+ break;
+ case PLDM_DESC_ID_PCI_SUBVENDOR_ID:
+ ptr = &id.subsystem_vendor;
+ break;
+ case PLDM_DESC_ID_PCI_SUBDEV_ID:
+ ptr = &id.subsystem_device;
+ break;
+ default:
+ /* Skip unrelated TLVs */
+ continue;
+ }
+
+ value = get_unaligned_le16(desc->data);
+ /* A value of zero for one of the descriptors is sometimes
+ * used when the record should ignore this field when matching
+ * device. For example if the record applies to any subsystem
+ * device or vendor.
+ */
+ if (value)
+ *ptr = (int)value;
+ else
+ *ptr = PCI_ANY_ID;
+ }
+
+ if ((id.vendor == PCI_ANY_ID || id.vendor == pdev->vendor) &&
+ (id.device == PCI_ANY_ID || id.device == pdev->device) &&
+ (id.subsystem_vendor == PCI_ANY_ID || id.subsystem_vendor == pdev->subsystem_vendor) &&
+ (id.subsystem_device == PCI_ANY_ID || id.subsystem_device == pdev->subsystem_device))
+ return true;
+ else
+ return false;
+}
+EXPORT_SYMBOL(pldmfw_op_pci_match_record);
+
+/**
+ * pldm_find_matching_record - Find the first matching PLDM record
+ * @data: pointer to private data
+ *
+ * Search through PLDM records and find the first matching entry. It is
+ * expected that only one entry matches.
+ *
+ * Store a pointer to the matching record, if found.
+ *
+ * Returns: zero on success, or -ENOENT if no matching record is found.
+ */
+static int pldm_find_matching_record(struct pldmfw_priv *data)
+{
+ struct pldmfw_record *record;
+
+ list_for_each_entry(record, &data->records, entry) {
+ if (data->context->ops->match_record(data->context, record)) {
+ data->matching_record = record;
+ return 0;
+ }
+ }
+
+ return -ENOENT;
+}
+
+/**
+ * pldm_send_package_data - Send firmware the package data for the record
+ * @data: pointer to private data
+ *
+ * Send the package data associated with the matching record to the firmware,
+ * using the send_pkg_data operation.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+pldm_send_package_data(struct pldmfw_priv *data)
+{
+ struct pldmfw_record *record = data->matching_record;
+ const struct pldmfw_ops *ops = data->context->ops;
+
+ return ops->send_package_data(data->context, record->package_data,
+ record->package_data_len);
+}
+
+/**
+ * pldm_send_component_tables - Send component table information to firmware
+ * @data: pointer to private data
+ *
+ * Loop over each component, sending the applicable components to the firmware
+ * via the send_component_table operation.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+pldm_send_component_tables(struct pldmfw_priv *data)
+{
+ unsigned long *bitmap = data->matching_record->component_bitmap;
+ struct pldmfw_component *component;
+ int err;
+
+ list_for_each_entry(component, &data->components, entry) {
+ u8 index = component->index, transfer_flag = 0;
+
+ /* Skip components which are not intended for this device */
+ if (!test_bit(index, bitmap))
+ continue;
+
+ /* determine whether this is the start, middle, end, or both
+ * the start and end of the component tables
+ */
+ if (index == find_first_bit(bitmap, data->component_bitmap_len))
+ transfer_flag |= PLDM_TRANSFER_FLAG_START;
+ if (index == find_last_bit(bitmap, data->component_bitmap_len))
+ transfer_flag |= PLDM_TRANSFER_FLAG_END;
+ if (!transfer_flag)
+ transfer_flag = PLDM_TRANSFER_FLAG_MIDDLE;
+
+ err = data->context->ops->send_component_table(data->context,
+ component,
+ transfer_flag);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+/**
+ * pldm_flash_components - Program each component to device flash
+ * @data: pointer to private data
+ *
+ * Loop through each component that is active for the matching device record,
+ * and send it to the device driver for flashing.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int pldm_flash_components(struct pldmfw_priv *data)
+{
+ unsigned long *bitmap = data->matching_record->component_bitmap;
+ struct pldmfw_component *component;
+ int err;
+
+ list_for_each_entry(component, &data->components, entry) {
+ u8 index = component->index;
+
+ /* Skip components which are not intended for this device */
+ if (!test_bit(index, bitmap))
+ continue;
+
+ err = data->context->ops->flash_component(data->context, component);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+/**
+ * pldm_finalize_update - Finalize the device flash update
+ * @data: pointer to private data
+ *
+ * Tell the device driver to perform any remaining logic to complete the
+ * device update.
+ *
+ * Returns: zero on success, or a PLFM_FWU error indicating the reason for
+ * failure.
+ */
+static int pldm_finalize_update(struct pldmfw_priv *data)
+{
+ if (data->context->ops->finalize_update)
+ return data->context->ops->finalize_update(data->context);
+
+ return 0;
+}
+
+/**
+ * pldmfw_flash_image - Write a PLDM-formatted firmware image to the device
+ * @context: ops and data for firmware update
+ * @fw: firmware object pointing to the relevant firmware file to program
+ *
+ * Parse the data for a given firmware file, verifying that it is a valid PLDM
+ * formatted image that matches this device.
+ *
+ * Extract the device record Package Data and Component Tables and send them
+ * to the device firmware. Extract and write the flash data for each of the
+ * components indicated in the firmware file.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+int pldmfw_flash_image(struct pldmfw *context, const struct firmware *fw)
+{
+ struct pldmfw_priv *data;
+ int err;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&data->records);
+ INIT_LIST_HEAD(&data->components);
+
+ data->fw = fw;
+ data->context = context;
+
+ err = pldm_parse_image(data);
+ if (err)
+ goto out_release_data;
+
+ err = pldm_find_matching_record(data);
+ if (err)
+ goto out_release_data;
+
+ err = pldm_send_package_data(data);
+ if (err)
+ goto out_release_data;
+
+ err = pldm_send_component_tables(data);
+ if (err)
+ goto out_release_data;
+
+ err = pldm_flash_components(data);
+ if (err)
+ goto out_release_data;
+
+ err = pldm_finalize_update(data);
+
+out_release_data:
+ pldmfw_free_priv(data);
+ kfree(data);
+
+ return err;
+}
+EXPORT_SYMBOL(pldmfw_flash_image);
+
+MODULE_AUTHOR("Jacob Keller <jacob.e.keller@intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("PLDM firmware flash update library");
diff --git a/lib/pldmfw/pldmfw_private.h b/lib/pldmfw/pldmfw_private.h
new file mode 100644
index 000000000000..0abc0238df3b
--- /dev/null
+++ b/lib/pldmfw/pldmfw_private.h
@@ -0,0 +1,238 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2018-2019, Intel Corporation. */
+
+#ifndef _PLDMFW_PRIVATE_H_
+#define _PLDMFW_PRIVATE_H_
+
+/* The following data structures define the layout of a firmware binary
+ * following the "PLDM For Firmware Update Specification", DMTF standard
+ * #DSP0267.
+ *
+ * pldmfw.c uses these structures to implement a simple engine that will parse
+ * a fw binary file in this format and perform a firmware update for a given
+ * device.
+ *
+ * Due to the variable sized data layout, alignment of fields within these
+ * structures is not guaranteed when reading. For this reason, all multi-byte
+ * field accesses should be done using the unaligned access macros.
+ * Additionally, the standard specifies that multi-byte fields are in
+ * LittleEndian format.
+ *
+ * The structure definitions are not made public, in order to keep direct
+ * accesses within code that is prepared to deal with the limitation of
+ * unaligned access.
+ */
+
+/* UUID for PLDM firmware packages: f018878c-cb7d-4943-9800-a02f059aca02 */
+static const uuid_t pldm_firmware_header_id =
+ UUID_INIT(0xf018878c, 0xcb7d, 0x4943,
+ 0x98, 0x00, 0xa0, 0x2f, 0x05, 0x9a, 0xca, 0x02);
+
+/* Revision number of the PLDM header format this code supports */
+#define PACKAGE_HEADER_FORMAT_REVISION 0x01
+
+/* timestamp104 structure defined in PLDM Base specification */
+#define PLDM_TIMESTAMP_SIZE 13
+struct __pldm_timestamp {
+ u8 b[PLDM_TIMESTAMP_SIZE];
+} __aligned(1);
+
+/* Package Header Information */
+struct __pldm_header {
+ uuid_t id; /* PackageHeaderIdentifier */
+ u8 revision; /* PackageHeaderFormatRevision */
+ __le16 size; /* PackageHeaderSize */
+ struct __pldm_timestamp release_date; /* PackageReleaseDateTime */
+ __le16 component_bitmap_len; /* ComponentBitmapBitLength */
+ u8 version_type; /* PackageVersionStringType */
+ u8 version_len; /* PackageVersionStringLength */
+
+ /*
+ * DSP0267 also includes the following variable length fields at the
+ * end of this structure:
+ *
+ * PackageVersionString, length is version_len.
+ *
+ * The total size of this section is
+ * sizeof(pldm_header) + version_len;
+ */
+ u8 version_string[]; /* PackageVersionString */
+} __packed __aligned(1);
+
+/* Firmware Device ID Record */
+struct __pldmfw_record_info {
+ __le16 record_len; /* RecordLength */
+ u8 descriptor_count; /* DescriptorCount */
+ __le32 device_update_flags; /* DeviceUpdateOptionFlags */
+ u8 version_type; /* ComponentImageSetVersionType */
+ u8 version_len; /* ComponentImageSetVersionLength */
+ __le16 package_data_len; /* FirmwareDevicePackageDataLength */
+
+ /*
+ * DSP0267 also includes the following variable length fields at the
+ * end of this structure:
+ *
+ * ApplicableComponents, length is component_bitmap_len from header
+ * ComponentImageSetVersionString, length is version_len
+ * RecordDescriptors, a series of TLVs with 16bit type and length
+ * FirmwareDevicePackageData, length is package_data_len
+ *
+ * The total size of each record is
+ * sizeof(pldmfw_record_info) +
+ * component_bitmap_len (converted to bytes!) +
+ * version_len +
+ * <length of RecordDescriptors> +
+ * package_data_len
+ */
+ u8 variable_record_data[];
+} __packed __aligned(1);
+
+/* Firmware Descriptor Definition */
+struct __pldmfw_desc_tlv {
+ __le16 type; /* DescriptorType */
+ __le16 size; /* DescriptorSize */
+ u8 data[]; /* DescriptorData */
+} __aligned(1);
+
+/* Firmware Device Identification Area */
+struct __pldmfw_record_area {
+ u8 record_count; /* DeviceIDRecordCount */
+ /* This is not a struct type because the size of each record varies */
+ u8 records[];
+} __aligned(1);
+
+/* Individual Component Image Information */
+struct __pldmfw_component_info {
+ __le16 classification; /* ComponentClassfication */
+ __le16 identifier; /* ComponentIdentifier */
+ __le32 comparison_stamp; /* ComponentComparisonStamp */
+ __le16 options; /* componentOptions */
+ __le16 activation_method; /* RequestedComponentActivationMethod */
+ __le32 location_offset; /* ComponentLocationOffset */
+ __le32 size; /* ComponentSize */
+ u8 version_type; /* ComponentVersionStringType */
+ u8 version_len; /* ComponentVersionStringLength */
+
+ /*
+ * DSP0267 also includes the following variable length fields at the
+ * end of this structure:
+ *
+ * ComponentVersionString, length is version_len
+ *
+ * The total size of this section is
+ * sizeof(pldmfw_component_info) + version_len;
+ */
+ u8 version_string[]; /* ComponentVersionString */
+} __packed __aligned(1);
+
+/* Component Image Information Area */
+struct __pldmfw_component_area {
+ __le16 component_image_count;
+ /* This is not a struct type because the component size varies */
+ u8 components[];
+} __aligned(1);
+
+/**
+ * pldm_first_desc_tlv
+ * @start: byte offset of the start of the descriptor TLVs
+ *
+ * Converts the starting offset of the descriptor TLVs into a pointer to the
+ * first descriptor.
+ */
+#define pldm_first_desc_tlv(start) \
+ ((const struct __pldmfw_desc_tlv *)(start))
+
+/**
+ * pldm_next_desc_tlv
+ * @desc: pointer to a descriptor TLV
+ *
+ * Finds the pointer to the next descriptor following a given descriptor
+ */
+#define pldm_next_desc_tlv(desc) \
+ ((const struct __pldmfw_desc_tlv *)((desc)->data + \
+ get_unaligned_le16(&(desc)->size)))
+
+/**
+ * pldm_for_each_desc_tlv
+ * @i: variable to store descriptor index
+ * @desc: variable to store descriptor pointer
+ * @start: byte offset of the start of the descriptors
+ * @count: the number of descriptors
+ *
+ * for loop macro to iterate over all of the descriptors of a given PLDM
+ * record.
+ */
+#define pldm_for_each_desc_tlv(i, desc, start, count) \
+ for ((i) = 0, (desc) = pldm_first_desc_tlv(start); \
+ (i) < (count); \
+ (i)++, (desc) = pldm_next_desc_tlv(desc))
+
+/**
+ * pldm_first_record
+ * @start: byte offset of the start of the PLDM records
+ *
+ * Converts a starting offset of the PLDM records into a pointer to the first
+ * record.
+ */
+#define pldm_first_record(start) \
+ ((const struct __pldmfw_record_info *)(start))
+
+/**
+ * pldm_next_record
+ * @record: pointer to a PLDM record
+ *
+ * Finds a pointer to the next record following a given record
+ */
+#define pldm_next_record(record) \
+ ((const struct __pldmfw_record_info *) \
+ ((const u8 *)(record) + get_unaligned_le16(&(record)->record_len)))
+
+/**
+ * pldm_for_each_record
+ * @i: variable to store record index
+ * @record: variable to store record pointer
+ * @start: byte offset of the start of the records
+ * @count: the number of records
+ *
+ * for loop macro to iterate over all of the records of a PLDM file.
+ */
+#define pldm_for_each_record(i, record, start, count) \
+ for ((i) = 0, (record) = pldm_first_record(start); \
+ (i) < (count); \
+ (i)++, (record) = pldm_next_record(record))
+
+/**
+ * pldm_first_component
+ * @start: byte offset of the start of the PLDM components
+ *
+ * Convert a starting offset of the PLDM components into a pointer to the
+ * first component
+ */
+#define pldm_first_component(start) \
+ ((const struct __pldmfw_component_info *)(start))
+
+/**
+ * pldm_next_component
+ * @component: pointer to a PLDM component
+ *
+ * Finds a pointer to the next component following a given component
+ */
+#define pldm_next_component(component) \
+ ((const struct __pldmfw_component_info *)((component)->version_string + \
+ (component)->version_len))
+
+/**
+ * pldm_for_each_component
+ * @i: variable to store component index
+ * @component: variable to store component pointer
+ * @start: byte offset to the start of the first component
+ * @count: the number of components
+ *
+ * for loop macro to iterate over all of the components of a PLDM file.
+ */
+#define pldm_for_each_component(i, component, start, count) \
+ for ((i) = 0, (component) = pldm_first_component(start); \
+ (i) < (count); \
+ (i)++, (component) = pldm_next_component(component))
+
+#endif
--
2.27.0.353.gb9a2d1a0207f
^ permalink raw reply related [flat|nested] 36+ messages in thread* [RFC PATCH net-next v2 5/6] ice: implement device flash update via devlink
2020-07-17 18:35 [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jacob Keller
` (3 preceding siblings ...)
2020-07-17 18:35 ` [RFC PATCH net-next v2 4/6] Add pldmfw library for PLDM firmware update Jacob Keller
@ 2020-07-17 18:35 ` Jacob Keller
2020-07-20 5:26 ` kernel test robot
2020-07-23 23:33 ` Jacob Keller
2020-07-17 18:35 ` [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update Jacob Keller
2020-07-17 19:58 ` [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jakub Kicinski
6 siblings, 2 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-17 18:35 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Jiri Pirko, Tom Herbert, Jacob Keller
Use the newly added pldmfw library to implement device flash update for
the Intel ice networking device driver. This support uses the devlink
flash update interface.
The main parts of the flash include the Option ROM, the netlist module,
and the main NVM data. The PLDM firmware file contains modules for each
of these components.
Using the pldmfw library, the provided firmware file will be scanned for
the three major components, "fw.undi" for the Option ROM, "fw.mgmt" for
the main NVM module containing the primary device firmware, and
"fw.netlist" containing the netlist module.
The flash is separated into two banks, the active bank containing the
running firmware, and the inactive bank which we use for update. Each
module is updated in a staged process. First, the inactive bank is
erased, preparing the device for update. Second, the contents of the
component are copied to the inactive portion of the flash. After all
components are updated, the driver signals the device to switch the
active bank during the next EMP reset (which would usually occur during
the next reboot).
Although the firmware AdminQ interface does report an immediate status
for each command, the NVM erase and NVM write commands receive status
asynchronously. The driver must not continue writing until previous
erase and write commands have finished. The real status of the NVM
commands is returned over the receive AdminQ. Implement a simple
interface that uses a wait queue so that the main update thread can
sleep until the completion status is reported by firmware. For erasing
the inactive banks, this can take quite a while in practice.
To help visualize the process to the devlink application and other
applications based on the devlink netlink interface, status is reported
via the devlink_flash_update_status_notify. While we do report status
after each 4k block when writing, there is no real status we can report
during erasing. We simply must wait for the complete module erasure to
finish.
With this implementation, basic flash update for the ice hardware is
supported.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/Kconfig | 1 +
drivers/net/ethernet/intel/ice/Makefile | 1 +
drivers/net/ethernet/intel/ice/ice.h | 9 +
drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
drivers/net/ethernet/intel/ice/ice_common.h | 2 +
drivers/net/ethernet/intel/ice/ice_devlink.c | 54 ++
.../net/ethernet/intel/ice/ice_fw_update.c | 780 ++++++++++++++++++
.../net/ethernet/intel/ice/ice_fw_update.h | 12 +
drivers/net/ethernet/intel/ice/ice_main.c | 154 ++++
9 files changed, 1014 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/ethernet/intel/ice/ice_fw_update.c
create mode 100644 drivers/net/ethernet/intel/ice/ice_fw_update.h
diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index c99fe92d4b3c..6aee67276cf5 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -295,6 +295,7 @@ config ICE
default n
depends on PCI_MSI
select NET_DEVLINK
+ select PLDMFW
help
This driver supports Intel(R) Ethernet Connection E800 Series of
devices. For more information on how to identify your adapter, go
diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
index 229740c3c1e1..3a34e717adf3 100644
--- a/drivers/net/ethernet/intel/ice/Makefile
+++ b/drivers/net/ethernet/intel/ice/Makefile
@@ -23,6 +23,7 @@ ice-y := ice_main.o \
ice_flex_pipe.o \
ice_flow.o \
ice_devlink.o \
+ ice_fw_update.o \
ice_ethtool.o
ice-$(CONFIG_PCI_IOV) += ice_virtchnl_pf.o ice_sriov.o ice_dcf.o ice_virtchnl_fdir.o
ice-$(CONFIG_DCB) += ice_dcb.o ice_dcb_nl.o ice_dcb_lib.o
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 9b4c70310c3a..152b1f71045c 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -19,6 +19,7 @@
#include <linux/dma-mapping.h>
#include <linux/pci.h>
#include <linux/workqueue.h>
+#include <linux/wait.h>
#include <linux/aer.h>
#include <linux/interrupt.h>
#include <linux/ethtool.h>
@@ -415,6 +416,12 @@ struct ice_pf {
struct mutex sw_mutex; /* lock for protecting VSI alloc flow */
struct mutex tc_mutex; /* lock to protect TC changes */
u32 msg_enable;
+
+ /* spinlock to protect the AdminQ wait list */
+ spinlock_t aq_wait_lock;
+ struct hlist_head aq_wait_list;
+ wait_queue_head_t aq_wait_queue;
+
u32 hw_csum_rx_error;
u16 oicr_idx; /* Other interrupt cause MSIX vector index */
u16 num_avail_sw_msix; /* remaining MSIX SW vectors left unclaimed */
@@ -598,6 +605,8 @@ void ice_fdir_release_flows(struct ice_hw *hw);
void ice_fdir_replay_flows(struct ice_hw *hw);
void ice_fdir_replay_fltrs(struct ice_pf *pf);
int ice_fdir_create_dflt_rules(struct ice_pf *pf);
+int ice_aq_wait_for_event(struct ice_pf *pf, u16 opcode, unsigned long timeout,
+ struct ice_rq_event_info *event);
int ice_open(struct net_device *netdev);
int ice_stop(struct net_device *netdev);
void ice_service_task_schedule(struct ice_pf *pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 2e9a43bae67f..25b654d2d11e 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -2221,7 +2221,7 @@ ice_aq_list_caps(struct ice_hw *hw, void *buf, u16 buf_size, u32 *cap_count,
* Read the device capabilities and extract them into the dev_caps structure
* for later use.
*/
-static enum ice_status
+enum ice_status
ice_discover_dev_caps(struct ice_hw *hw, struct ice_hw_dev_caps *dev_caps)
{
enum ice_status status;
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 485e2ac792b5..3ebb973878c7 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -88,6 +88,8 @@ ice_aq_get_phy_caps(struct ice_port_info *pi, bool qual_mods, u8 report_mode,
enum ice_status
ice_aq_list_caps(struct ice_hw *hw, void *buf, u16 buf_size, u32 *cap_count,
enum ice_adminq_opc opc, struct ice_sq_cd *cd);
+enum ice_status
+ice_discover_dev_caps(struct ice_hw *hw, struct ice_hw_dev_caps *dev_caps);
void
ice_update_phy_type(u64 *phy_type_low, u64 *phy_type_high,
u16 link_speeds_bitmap);
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 43da2dcb0cbc..dbbd8b6f9d1a 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -4,6 +4,7 @@
#include "ice.h"
#include "ice_lib.h"
#include "ice_devlink.h"
+#include "ice_fw_update.h"
static int ice_info_get_dsn(struct ice_pf *pf, char *buf, size_t len)
{
@@ -229,8 +230,61 @@ static int ice_devlink_info_get(struct devlink *devlink,
return 0;
}
+/**
+ * ice_devlink_flash_update - Update firmware stored in flash on the device
+ * @devlink: pointer to devlink associated with device to update
+ * @path: the path of the firmware file to use via request_firmware
+ * @component: name of the component to update, or NULL
+ * @extack: netlink extended ACK structure
+ *
+ * Perform a device flash update. The bulk of the update logic is contained
+ * within the ice_flash_pldm_image function.
+ *
+ * Returns: zero on success, or an error code on failure.
+ */
+static int
+ice_devlink_flash_update(struct devlink *devlink, const char *path,
+ const char *component, struct netlink_ext_ack *extack)
+{
+ struct ice_pf *pf = devlink_priv(devlink);
+ struct device *dev = &pf->pdev->dev;
+ struct ice_hw *hw = &pf->hw;
+ const struct firmware *fw;
+ int err;
+
+ /* individual component update is not yet supported */
+ if (component)
+ return -EOPNOTSUPP;
+
+ if (!hw->dev_caps.common_cap.nvm_unified_update) {
+ NL_SET_ERR_MSG_MOD(extack, "Current firmware does not support unified update");
+ return -EOPNOTSUPP;
+ }
+
+ err = ice_check_for_pending_update(pf, component, extack);
+ if (err)
+ return err;
+
+ err = request_firmware(&fw, path, dev);
+ if (err) {
+ NL_SET_ERR_MSG_MOD(extack, "Unable to read file from disk");
+ return err;
+ }
+
+ devlink_flash_update_begin_notify(devlink);
+ devlink_flash_update_status_notify(devlink, "Preparing to flash",
+ component, 0, 0);
+ err = ice_flash_pldm_image(pf, fw, extack);
+ devlink_flash_update_end_notify(devlink);
+
+ release_firmware(fw);
+
+ return err;
+}
+
static const struct devlink_ops ice_devlink_ops = {
.info_get = ice_devlink_info_get,
+ .flash_update = ice_devlink_flash_update,
};
static void ice_devlink_free(void *devlink_ptr)
diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
new file mode 100644
index 000000000000..51b575ba197d
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
@@ -0,0 +1,780 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2018-2019, Intel Corporation. */
+
+#include <asm/unaligned.h>
+#include <linux/uuid.h>
+#include <linux/crc32.h>
+#include <linux/pldmfw.h>
+#include "ice.h"
+#include "ice_fw_update.h"
+
+struct ice_fwu_priv {
+ struct pldmfw context;
+
+ struct ice_pf *pf;
+ struct netlink_ext_ack *extack;
+
+ /* Track which NVM banks to activate at the end of the update */
+ u8 activate_flags;
+};
+
+/**
+ * ice_send_package_data - Send record package data to firmware
+ * @context: PLDM fw update structure
+ * @data: pointer to the package data
+ * @length: length of the package data
+ *
+ * Send a copy of the package data associated with the PLDM record matching
+ * this device to the firmware.
+ *
+ * Note that this function sends an AdminQ command that will fail unless the
+ * NVM resource has been acquired.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+ice_send_package_data(struct pldmfw *context, const u8 *data, u16 length)
+{
+ struct ice_fwu_priv *priv = container_of(context, struct ice_fwu_priv, context);
+ struct netlink_ext_ack *extack = priv->extack;
+ struct device *dev = context->dev;
+ struct ice_pf *pf = priv->pf;
+ struct ice_hw *hw = &pf->hw;
+ enum ice_status status;
+ u8 *package_data;
+
+ package_data = kmemdup(data, length, GFP_KERNEL);
+ if (!package_data)
+ return -ENOMEM;
+
+ status = ice_nvm_set_pkg_data(hw, false, package_data, length, NULL);
+
+ kfree(package_data);
+
+ if (status) {
+ dev_err(dev, "Failed to send record package data to firmware, err %s aq_err %s\n",
+ ice_stat_str(status),
+ ice_aq_str(hw->adminq.sq_last_status));
+ NL_SET_ERR_MSG_MOD(extack, "Failed to record package data to firmware");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+/**
+ * ice_check_component_response - Report firmware response to a component
+ * @pf: device private data structure
+ * @id: component id being checked
+ * @response: indicates whether this component can be updated
+ * @code: code indicating reason for response
+ * @extack: netlink extended ACK structure
+ *
+ * Check whether firmware indicates if this component can be updated. Report
+ * a suitable error message over the netlink extended ACK if the component
+ * cannot be updated.
+ *
+ * Returns: zero if the component can be updated, or -ECANCELED of the
+ * firmware indicates the component cannot be updated.
+ */
+static int
+ice_check_component_response(struct ice_pf *pf, u16 id, u8 response, u8 code,
+ struct netlink_ext_ack *extack)
+{
+ struct device *dev = ice_pf_to_dev(pf);
+ const char *component;
+
+ switch (id) {
+ case NVM_COMP_ID_OROM:
+ component = "fw.undi";
+ break;
+ case NVM_COMP_ID_NVM:
+ component = "fw.mgmt";
+ break;
+ case NVM_COMP_ID_NETLIST:
+ component = "fw.netlist";
+ break;
+ default:
+ WARN(1, "Unexpected unknown component identifier 0x%02x", id);
+ return -EINVAL;
+ }
+
+ dev_dbg(dev, "%s: firmware response 0x%x, code 0x%x\n",
+ component, response, code);
+
+ switch (response) {
+ case ICE_AQ_NVM_PASS_COMP_CAN_BE_UPDATED:
+ /* firmware indicated this update is good to proceed */
+ return 0;
+ case ICE_AQ_NVM_PASS_COMP_CAN_MAY_BE_UPDATEABLE:
+ dev_warn(dev, "firmware recommends not updating %s, as it may result in a downgrade. continuing anyways\n", component);
+ return 0;
+ case ICE_AQ_NVM_PASS_COMP_CAN_NOT_BE_UPDATED:
+ dev_info(dev, "firmware has rejected updating %s\n", component);
+ break;
+ }
+
+ switch (code) {
+ case ICE_AQ_NVM_PASS_COMP_STAMP_IDENTICAL_CODE:
+ dev_err(dev, "Component comparison stamp for %s is identical to the running image\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Component comparison stamp is identical to running image");
+ break;
+ case ICE_AQ_NVM_PASS_COMP_STAMP_LOWER:
+ dev_err(dev, "Component comparison stamp for %s is lower than the running image\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Component comparison stamp is lower than running image");
+ break;
+ case ICE_AQ_NVM_PASS_COMP_INVALID_STAMP_CODE:
+ dev_err(dev, "Component comparison stamp for %s is invalid\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Component comparison stamp is invalid");
+ break;
+ case ICE_AQ_NVM_PASS_COMP_CONFLICT_CODE:
+ dev_err(dev, "%s conflicts with a previous component table\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Component table conflict occurred");
+ break;
+ case ICE_AQ_NVM_PASS_COMP_PRE_REQ_NOT_MET_CODE:
+ dev_err(dev, "Pre-requisites for component %s have not been met\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Component pre-requisites not met");
+ break;
+ case ICE_AQ_NVM_PASS_COMP_NOT_SUPPORTED_CODE:
+ dev_err(dev, "%s is not a supported component\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Component not supported");
+ break;
+ case ICE_AQ_NVM_PASS_COMP_CANNOT_DOWNGRADE_CODE:
+ dev_err(dev, "Security restrictions prevent %s from being downgraded\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Component cannot be downgraded");
+ break;
+ case ICE_AQ_NVM_PASS_COMP_INCOMPLETE_IMAGE_CODE:
+ dev_err(dev, "Received an incomplete component image for %s\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Incomplete component image");
+ break;
+ case ICE_AQ_NVM_PASS_COMP_VER_STR_IDENTICAL_CODE:
+ dev_err(dev, "Component version for %s is identical to the running image\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Component version is identical to running image");
+ break;
+ case ICE_AQ_NVM_PASS_COMP_VER_STR_LOWER_CODE:
+ dev_err(dev, "Component version for %s is lower than the running image\n",
+ component);
+ NL_SET_ERR_MSG_MOD(extack, "Component version is lower than the running image");
+ break;
+ default:
+ dev_err(dev, "Unexpected response code 0x02%x for %s\n",
+ code, component);
+ NL_SET_ERR_MSG_MOD(extack, "Received unexpected response code from firmware");
+ break;
+ }
+
+ return -ECANCELED;
+}
+
+/**
+ * ice_send_component_table - Send PLDM component table to firmware
+ * @context: PLDM fw update structure
+ * @component: the component to process
+ * @transfer_flag: relative transfer order of this component
+ *
+ * Read relevant data from the component and forward it to the device
+ * firmware. Check the response to determine if the firmware indicates that
+ * the update can proceed.
+ *
+ * This function sends AdminQ commands related to the NVM, and assumes that
+ * the NVM resource has been acquired.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+ice_send_component_table(struct pldmfw *context, struct pldmfw_component *component,
+ u8 transfer_flag)
+{
+ struct ice_fwu_priv *priv = container_of(context, struct ice_fwu_priv, context);
+ struct netlink_ext_ack *extack = priv->extack;
+ struct ice_aqc_nvm_comp_tbl *comp_tbl;
+ u8 comp_response, comp_response_code;
+ struct device *dev = context->dev;
+ struct ice_pf *pf = priv->pf;
+ struct ice_hw *hw = &pf->hw;
+ enum ice_status status;
+ size_t length;
+
+ switch (component->identifier) {
+ case NVM_COMP_ID_OROM:
+ case NVM_COMP_ID_NVM:
+ case NVM_COMP_ID_NETLIST:
+ break;
+ default:
+ dev_err(dev, "Unable to update due to a firmware component with unknown ID %u\n",
+ component->identifier);
+ NL_SET_ERR_MSG_MOD(extack, "Unable to update due to unknown firmware component");
+ return -EOPNOTSUPP;
+ }
+
+ length = struct_size(comp_tbl, cvs, component->version_len);
+ comp_tbl = kzalloc(length, GFP_KERNEL);
+ if (!comp_tbl)
+ return -ENOMEM;
+
+ comp_tbl->comp_class = cpu_to_le16(component->classification);
+ comp_tbl->comp_id = cpu_to_le16(component->identifier);
+ comp_tbl->comp_class_idx = FWU_COMP_CLASS_IDX_NOT_USE;
+ comp_tbl->comp_cmp_stamp = cpu_to_le32(component->comparison_stamp);
+ comp_tbl->cvs_type = component->version_type;
+ comp_tbl->cvs_len = component->version_len;
+ memcpy(comp_tbl->cvs, component->version_string, component->version_len);
+
+ status = ice_nvm_pass_component_tbl(hw, (u8 *)comp_tbl, length,
+ transfer_flag, &comp_response,
+ &comp_response_code, NULL);
+
+ kfree(comp_tbl);
+
+ if (status) {
+ dev_err(dev, "Failed to transfer component table to firmware, err %s aq_err %s\n",
+ ice_stat_str(status),
+ ice_aq_str(hw->adminq.sq_last_status));
+ NL_SET_ERR_MSG_MOD(extack, "Failed to transfer component table to firmware");
+ return -EIO;
+ }
+
+ return ice_check_component_response(pf, component->identifier, comp_response,
+ comp_response_code, extack);
+}
+
+/**
+ * ice_write_one_nvm_block - Write an NVM block and await completion response
+ * @pf: the PF data structure
+ * @module: the module to write to
+ * @offset: offset in bytes
+ * @block_size: size of the block to write, up to 4k
+ * @block: pointer to block of data to write
+ * @last_cmd: whether this is the last command
+ * @extack: netlink extended ACK structure
+ *
+ * Write a block of data to a flash module, and await for the completion
+ * response message from firmware.
+ *
+ * Note this function assumes the caller has acquired the NVM resource.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+ice_write_one_nvm_block(struct ice_pf *pf, u16 module, u32 offset,
+ u16 block_size, u8 *block, bool last_cmd,
+ struct netlink_ext_ack *extack)
+{
+ u16 completion_module, completion_retval;
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_rq_event_info event;
+ struct ice_hw *hw = &pf->hw;
+ enum ice_status status;
+ u32 completion_offset;
+ int err;
+
+ memset(&event, 0, sizeof(event));
+
+ status = ice_aq_update_nvm(hw, module, offset, block_size, block,
+ last_cmd, 0, NULL);
+ if (status) {
+ dev_err(dev, "Failed to program flash module 0x%02x at offset %u, err %s aq_err %s\n",
+ module, offset, ice_stat_str(status),
+ ice_aq_str(hw->adminq.sq_last_status));
+ NL_SET_ERR_MSG_MOD(extack, "Failed to program flash module");
+ return -EIO;
+ }
+
+ err = ice_aq_wait_for_event(pf, ice_aqc_opc_nvm_write, HZ, &event);
+ if (err) {
+ dev_err(dev, "Timed out waiting for firmware write completion for module 0x%02x, err %d\n",
+ module, err);
+ NL_SET_ERR_MSG_MOD(extack, "Timed out waiting for firmware");
+ return -EIO;
+ }
+
+ completion_module = le16_to_cpu(event.desc.params.nvm.module_typeid);
+ completion_retval = le16_to_cpu(event.desc.retval);
+
+ completion_offset = le16_to_cpu(event.desc.params.nvm.offset_low);
+ completion_offset |= event.desc.params.nvm.offset_high << 16;
+
+ if (completion_module != module) {
+ dev_err(dev, "Unexpected module_typeid in write completion: got 0x%x, expected 0x%x\n",
+ completion_module, module);
+ NL_SET_ERR_MSG_MOD(extack, "Unexpected firmware response");
+ return -EIO;
+ }
+
+ if (completion_offset != offset) {
+ dev_err(dev, "Unexpected offset in write completion: got %u, expected %u\n",
+ completion_offset, offset);
+ NL_SET_ERR_MSG_MOD(extack, "Unexpected firmware response");
+ return -EIO;
+ }
+
+ if (completion_retval) {
+ dev_err(dev, "Firmware failed to program flash module 0x%02x at offset %u, completion err %s\n",
+ module, offset,
+ ice_aq_str((enum ice_aq_err)completion_retval));
+ NL_SET_ERR_MSG_MOD(extack, "Firmware failed to program flash module");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+/**
+ * ice_write_nvm_module - Write data to an NVM module
+ * @pf: the PF driver structure
+ * @module: the module id to program
+ * @component: the name of the component being updated
+ * @image: buffer of image data to write to the NVM
+ * @length: length of the buffer
+ * @extack: netlink extended ACK structure
+ *
+ * Loop over the data for a given NVM module and program it in 4 Kb
+ * blocks. Notify devlink core of progress after each block is programmed.
+ * Loops over a block of data and programs the NVM in 4k block chunks.
+ *
+ * Note this function assumes the caller has acquired the NVM resource.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+ice_write_nvm_module(struct ice_pf *pf, u16 module, const char *component,
+ const u8 *image, u32 length,
+ struct netlink_ext_ack *extack)
+{
+ struct devlink *devlink;
+ u32 offset = 0;
+ bool last_cmd;
+ u8 *block;
+ int err;
+
+ devlink = priv_to_devlink(pf);
+
+ devlink_flash_update_status_notify(devlink, "Flashing",
+ component, 0, length);
+
+ block = kzalloc(ICE_AQ_MAX_BUF_LEN, GFP_KERNEL);
+ if (!block)
+ return -ENOMEM;
+
+ do {
+ u32 block_size;
+
+ block_size = min_t(u32, ICE_AQ_MAX_BUF_LEN, length - offset);
+ last_cmd = !(offset + block_size < length);
+
+ /* ice_aq_update_nvm may copy the firmware response into the
+ * buffer, so we must make a copy since the source data is
+ * constant.
+ */
+ memcpy(block, image + offset, block_size);
+
+ err = ice_write_one_nvm_block(pf, module, offset, block_size,
+ block, last_cmd, extack);
+ if (err)
+ break;
+
+ offset += block_size;
+
+ devlink_flash_update_status_notify(devlink, "Flashing",
+ component, offset, length);
+ } while (!last_cmd);
+
+ if (err)
+ devlink_flash_update_status_notify(devlink, "Flashing failed",
+ component, length, length);
+ else
+ devlink_flash_update_status_notify(devlink, "Flashing done",
+ component, length, length);
+
+ kfree(block);
+ return err;
+}
+
+/**
+ * ice_erase_nvm_module - Erase an NVM module and await firmware completion
+ * @pf: the PF data structure
+ * @module: the module to erase
+ * @component: name of the component being updated
+ * @extack: netlink extended ACK structure
+ *
+ * Erase the inactive NVM bank associated with this module, and await for
+ * a completion response message from firmware.
+ *
+ * Note this function assumes the caller has acquired the NVM resource.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+ice_erase_nvm_module(struct ice_pf *pf, u16 module, const char *component,
+ struct netlink_ext_ack *extack)
+{
+ u16 completion_module, completion_retval;
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_rq_event_info event;
+ struct ice_hw *hw = &pf->hw;
+ struct devlink *devlink;
+ enum ice_status status;
+ int err;
+
+ memset(&event, 0, sizeof(event));
+
+ devlink = priv_to_devlink(pf);
+
+ devlink_flash_update_status_notify(devlink, "Erasing", component, 0, 0);
+
+ status = ice_aq_erase_nvm(hw, module, NULL);
+ if (status) {
+ dev_err(dev, "Failed to erase %s (module 0x%02x), err %s aq_err %s\n",
+ component, module, ice_stat_str(status),
+ ice_aq_str(hw->adminq.sq_last_status));
+ NL_SET_ERR_MSG_MOD(extack, "Failed to erase flash module");
+ err = -EIO;
+ goto out_notify_devlink;
+ }
+
+ /* Yes, this really can take minutes to complete */
+ err = ice_aq_wait_for_event(pf, ice_aqc_opc_nvm_erase, 300 * HZ, &event);
+ if (err) {
+ dev_err(dev, "Timed out waiting for firmware to respond with erase completion for %s (module 0x%02x), err %d\n",
+ component, module, err);
+ NL_SET_ERR_MSG_MOD(extack, "Timed out waiting for firmware");
+ goto out_notify_devlink;
+ }
+
+ completion_module = le16_to_cpu(event.desc.params.nvm.module_typeid);
+ completion_retval = le16_to_cpu(event.desc.retval);
+
+ if (completion_module != module) {
+ dev_err(dev, "Unexpected module_typeid in erase completion for %s: got 0x%x, expected 0x%x\n",
+ component, completion_module, module);
+ NL_SET_ERR_MSG_MOD(extack, "Unexpected firmware response");
+ err = -EIO;
+ goto out_notify_devlink;
+ }
+
+ if (completion_retval) {
+ dev_err(dev, "Firmware failed to erase %s (module 0x02%x), aq_err %s\n",
+ component, module,
+ ice_aq_str((enum ice_aq_err)completion_retval));
+ NL_SET_ERR_MSG_MOD(extack, "Firmware failed to erase flash");
+ err = -EIO;
+ goto out_notify_devlink;
+ }
+
+out_notify_devlink:
+ if (err)
+ devlink_flash_update_status_notify(devlink, "Erasing failed",
+ component, 0, 0);
+ else
+ devlink_flash_update_status_notify(devlink, "Erasing done",
+ component, 0, 0);
+
+ return err;
+}
+
+/**
+ * ice_switch_flash_banks - Tell firmware to switch NVM banks
+ * @pf: Pointer to the PF data structure
+ * @activate_flags: flags used for the activation command
+ * @extack: netlink extended ACK structure
+ *
+ * Notify firmware to activate the newly written flash banks, and wait for the
+ * firmware response.
+ *
+ * Returns: zero on success or an error code on failure.
+ */
+static int ice_switch_flash_banks(struct ice_pf *pf, u8 activate_flags,
+ struct netlink_ext_ack *extack)
+{
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_rq_event_info event;
+ struct ice_hw *hw = &pf->hw;
+ enum ice_status status;
+ u16 completion_retval;
+ int err;
+
+ memset(&event, 0, sizeof(event));
+
+ status = ice_nvm_write_activate(hw, activate_flags);
+ if (status) {
+ dev_err(dev, "Failed to switch active flash banks, err %s aq_err %s\n",
+ ice_stat_str(status),
+ ice_aq_str(hw->adminq.sq_last_status));
+ NL_SET_ERR_MSG_MOD(extack, "Failed to switch active flash banks");
+ return -EIO;
+ }
+
+ err = ice_aq_wait_for_event(pf, ice_aqc_opc_nvm_write_activate, HZ,
+ &event);
+ if (err) {
+ dev_err(dev, "Timed out waiting for firmware to switch active flash banks, err %d\n",
+ err);
+ NL_SET_ERR_MSG_MOD(extack, "Timed out waiting for firmware");
+ return err;
+ }
+
+ completion_retval = le16_to_cpu(event.desc.retval);
+ if (completion_retval) {
+ dev_err(dev, "Firmware failed to switch active flash banks aq_err %s\n",
+ ice_aq_str((enum ice_aq_err)completion_retval));
+ NL_SET_ERR_MSG_MOD(extack, "Firmware failed to switch active flash banks");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+/**
+ * ice_flash_component - Flash a component of the NVM
+ * @context: PLDM fw update structure
+ * @component: the component table to program
+ *
+ * Program the flash contents for a given component. First, determine the
+ * module id. Then, erase the secondary bank for this module. Finally, write
+ * the contents of the component to the NVM.
+ *
+ * Note this function assumes the caller has acquired the NVM resource.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+static int
+ice_flash_component(struct pldmfw *context, struct pldmfw_component *component)
+{
+ struct ice_fwu_priv *priv = container_of(context, struct ice_fwu_priv, context);
+ struct netlink_ext_ack *extack = priv->extack;
+ struct ice_pf *pf = priv->pf;
+ const char *name;
+ u16 module;
+ u8 flag;
+ int err;
+
+ switch (component->identifier) {
+ case NVM_COMP_ID_OROM:
+ module = ICE_SR_1ST_OROM_BANK_PTR;
+ flag = ICE_AQC_NVM_ACTIV_SEL_OROM;
+ name = "fw.undi";
+ break;
+ case NVM_COMP_ID_NVM:
+ module = ICE_SR_1ST_NVM_BANK_PTR;
+ flag = ICE_AQC_NVM_ACTIV_SEL_NVM;
+ name = "fw.mgmt";
+ break;
+ case NVM_COMP_ID_NETLIST:
+ module = ICE_SR_NETLIST_BANK_PTR;
+ flag = ICE_AQC_NVM_ACTIV_SEL_NETLIST;
+ name = "fw.netlist";
+ break;
+ default:
+ /* This should not trigger, since we check the id before
+ * sending the component table to firmware.
+ */
+ WARN(1, "Unexpected unknown component identifier 0x%02x",
+ component->identifier);
+ return -EINVAL;
+ }
+
+ /* Mark this component for activating at the end */
+ priv->activate_flags |= flag;
+
+ err = ice_erase_nvm_module(pf, module, name, extack);
+ if (err)
+ return err;
+
+ return ice_write_nvm_module(pf, module, name, component->component_data,
+ component->component_size, extack);
+}
+
+/**
+ * ice_finalize_update - Perform last steps to complete device update
+ * @context: PLDM fw update structure
+ *
+ * Called as the last step of the update process. Complete the update by
+ * telling the firmware to switch active banks, and perform a reset of
+ * configured.
+ *
+ * Returns: 0 on success, or an error code on failure.
+ */
+static int ice_finalize_update(struct pldmfw *context)
+{
+ struct ice_fwu_priv *priv = container_of(context, struct ice_fwu_priv, context);
+ struct netlink_ext_ack *extack = priv->extack;
+ struct ice_pf *pf = priv->pf;
+ int err;
+
+ /* Finally, notify firmware to activate the written NVM banks */
+ err = ice_switch_flash_banks(pf, priv->activate_flags, extack);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static const struct pldmfw_ops ice_fwu_ops = {
+ .match_record = &pldmfw_op_pci_match_record,
+ .send_package_data = &ice_send_package_data,
+ .send_component_table = &ice_send_component_table,
+ .flash_component = &ice_flash_component,
+ .finalize_update = &ice_finalize_update,
+};
+
+/**
+ * ice_flash_pldm_image - Write a PLDM-formatted firmware image to the device
+ * @pf: private device driver structure
+ * @fw: firmware object pointing to the relevant firmware file
+ * @extack: netlink extended ACK structure
+ *
+ * Parse the data for a given firmware file, verifying that it is a valid PLDM
+ * formatted image that matches this device.
+ *
+ * Extract the device record Package Data and Component Tables and send them
+ * to the firmware. Extract and write the flash data for each of the three
+ * main flash components, "fw.mgmt", "fw.undi", and "fw.netlist". Notify
+ * firmware once the data is written to the inactive banks.
+ *
+ * Returns: zero on success or a negative error code on failure.
+ */
+int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw,
+ struct netlink_ext_ack *extack)
+{
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_hw *hw = &pf->hw;
+ struct ice_fwu_priv priv;
+ enum ice_status status;
+ int err;
+
+ memset(&priv, 0, sizeof(priv));
+
+ priv.context.ops = &ice_fwu_ops;
+ priv.context.dev = dev;
+ priv.extack = extack;
+ priv.pf = pf;
+ priv.activate_flags = ICE_AQC_NVM_PRESERVE_ALL;
+
+ status = ice_acquire_nvm(hw, ICE_RES_WRITE);
+ if (status) {
+ dev_err(dev, "Failed to acquire device flash lock, err %s aq_err %s\n",
+ ice_stat_str(status),
+ ice_aq_str(hw->adminq.sq_last_status));
+ NL_SET_ERR_MSG_MOD(extack, "Failed to acquire device flash lock");
+ return -EIO;
+ }
+
+ err = pldmfw_flash_image(&priv.context, fw);
+
+ ice_release_nvm(hw);
+
+ return err;
+}
+
+/**
+ * ice_check_for_pending_update - Check for a pending flash update
+ * @pf: the PF driver structure
+ * @component: if not NULL, the name of the component being updated
+ * @extack: Netlink extended ACK structure
+ *
+ * Check whether the device already has a pending flash update.
+ *
+ * If `ignore_pending_flash_update` is false, return an error in case of
+ * a pending update.
+ *
+ * If `ignore_pending_flash_update` is true, tell the device to cancel the
+ * pending update and return 0.
+ *
+ * Returns: -EALREADY if `ignore_pending_flash_update` is false and there is
+ * a pending update. Zero if there is no pending update or if the update is
+ * successfully canceled.
+ */
+int ice_check_for_pending_update(struct ice_pf *pf, const char *component,
+ struct netlink_ext_ack *extack)
+{
+ struct devlink *devlink = priv_to_devlink(pf);
+ struct device *dev = ice_pf_to_dev(pf);
+ struct ice_hw_dev_caps *dev_caps;
+ struct ice_hw *hw = &pf->hw;
+ enum ice_status status;
+ u8 pending = 0;
+ int err;
+
+ dev_caps = kzalloc(sizeof(*dev_caps), GFP_KERNEL);
+ if (!dev_caps)
+ return -ENOMEM;
+
+ /* Read the most recent device capabilities from firmware. Do not use
+ * the cached values in hw->dev_caps, because the pending update flag
+ * may have changed, e.g. if an update was previously completed and
+ * the system has not yet rebooted.
+ */
+ status = ice_discover_dev_caps(hw, dev_caps);
+ if (status) {
+ NL_SET_ERR_MSG_MOD(extack, "Unable to read device capabilities");
+ kfree(dev_caps);
+ return -EIO;
+ }
+
+ if (dev_caps->common_cap.nvm_update_pending_nvm) {
+ dev_info(dev, "The fw.mgmt flash component has a pending update\n");
+ pending |= ICE_AQC_NVM_ACTIV_SEL_NVM;
+ }
+
+ if (dev_caps->common_cap.nvm_update_pending_orom) {
+ dev_info(dev, "The fw.undi flash component has a pending update\n");
+ pending |= ICE_AQC_NVM_ACTIV_SEL_OROM;
+ }
+
+ if (dev_caps->common_cap.nvm_update_pending_netlist) {
+ dev_info(dev, "The fw.netlist flash component has a pending update\n");
+ pending |= ICE_AQC_NVM_ACTIV_SEL_NETLIST;
+ }
+
+ kfree(dev_caps);
+
+ /* If the flash_update request is for a specific component, ignore all
+ * of the other components.
+ */
+ if (component) {
+ if (strcmp(component, "fw.mgmt") == 0)
+ pending &= ICE_AQC_NVM_ACTIV_SEL_NVM;
+ else if (strcmp(component, "fw.undi") == 0)
+ pending &= ICE_AQC_NVM_ACTIV_SEL_OROM;
+ else if (strcmp(component, "fw.netlist") == 0)
+ pending &= ICE_AQC_NVM_ACTIV_SEL_NETLIST;
+ else
+ WARN(1, "Unexpected flash component %s", component);
+ }
+
+ /* There is no previous pending update, so this request may continue */
+ if (!pending)
+ return 0;
+
+ /* In order to allow overwriting a previous pending update, notify
+ * firmware to cancel that update by issuing the appropriate command.
+ */
+ devlink_flash_update_status_notify(devlink,
+ "Canceling previous pending update",
+ component, 0, 0);
+
+ status = ice_acquire_nvm(hw, ICE_RES_WRITE);
+ if (status) {
+ dev_err(dev, "Failed to acquire device flash lock, err %s aq_err %s\n",
+ ice_stat_str(status),
+ ice_aq_str(hw->adminq.sq_last_status));
+ NL_SET_ERR_MSG_MOD(extack, "Failed to acquire device flash lock");
+ return -EIO;
+ }
+
+ pending |= ICE_AQC_NVM_REVERT_LAST_ACTIV;
+ err = ice_switch_flash_banks(pf, pending, extack);
+
+ ice_release_nvm(hw);
+
+ return err;
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.h b/drivers/net/ethernet/intel/ice/ice_fw_update.h
new file mode 100644
index 000000000000..79472cc618b4
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2018-2019, Intel Corporation. */
+
+#ifndef _ICE_FW_UPDATE_H_
+#define _ICE_FW_UPDATE_H_
+
+int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw,
+ struct netlink_ext_ack *extack);
+int ice_check_for_pending_update(struct ice_pf *pf, const char *component,
+ struct netlink_ext_ack *extack);
+
+#endif
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 840e35e5f6f3..5dbb18c4359a 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1020,6 +1020,151 @@ ice_handle_link_event(struct ice_pf *pf, struct ice_rq_event_info *event)
return status;
}
+enum ice_aq_task_state {
+ ICE_AQ_TASK_WAITING = 0,
+ ICE_AQ_TASK_COMPLETE,
+ ICE_AQ_TASK_CANCELED,
+};
+
+struct ice_aq_task {
+ struct hlist_node entry;
+
+ u16 opcode;
+ struct ice_rq_event_info *event;
+ enum ice_aq_task_state state;
+};
+
+/**
+ * ice_wait_for_aq_event - Wait for an AdminQ event from firmware
+ * @pf: pointer to the PF private structure
+ * @opcode: the opcode to wait for
+ * @timeout: how long to wait, in jiffies
+ * @event: storage for the event info
+ *
+ * Waits for a specific AdminQ completion event on the ARQ for a given PF. The
+ * current thread will be put to sleep until the specified event occurs or
+ * until the given timeout is reached.
+ *
+ * To obtain only the descriptor contents, pass an event without an allocated
+ * msg_buf. If the complete data buffer is desired, allocate the
+ * event->msg_buf with enough space ahead of time.
+ *
+ * Returns: zero on success, or a negative error code on failure.
+ */
+int ice_aq_wait_for_event(struct ice_pf *pf, u16 opcode, unsigned long timeout,
+ struct ice_rq_event_info *event)
+{
+ struct ice_aq_task *task;
+ long ret;
+ int err;
+
+ task = kzalloc(sizeof(*task), GFP_KERNEL);
+ if (!task)
+ return -ENOMEM;
+
+ INIT_HLIST_NODE(&task->entry);
+ task->opcode = opcode;
+ task->event = event;
+ task->state = ICE_AQ_TASK_WAITING;
+
+ spin_lock_bh(&pf->aq_wait_lock);
+ hlist_add_head(&task->entry, &pf->aq_wait_list);
+ spin_unlock_bh(&pf->aq_wait_lock);
+
+ ret = wait_event_interruptible_timeout(pf->aq_wait_queue, task->state,
+ timeout);
+ switch (task->state) {
+ case ICE_AQ_TASK_WAITING:
+ err = ret < 0 ? ret : -ETIMEDOUT;
+ break;
+ case ICE_AQ_TASK_CANCELED:
+ err = ret < 0 ? ret : -ECANCELED;
+ break;
+ case ICE_AQ_TASK_COMPLETE:
+ err = ret < 0 ? ret : 0;
+ break;
+ default:
+ WARN(1, "Unexpected AdminQ wait task state %u", task->state);
+ err = -EINVAL;
+ break;
+ }
+
+ spin_lock_bh(&pf->aq_wait_lock);
+ hlist_del(&task->entry);
+ spin_unlock_bh(&pf->aq_wait_lock);
+ kfree(task);
+
+ return err;
+}
+
+/**
+ * ice_aq_check_events - Check if any thread is waiting for an AdminQ event
+ * @pf: pointer to the PF private structure
+ * @opcode: the opcode of the event
+ * @event: the event to check
+ *
+ * Loops over the current list of pending threads waiting for an AdminQ event.
+ * For each matching task, copy the contents of the event into the task
+ * structure and wake up the thread.
+ *
+ * If multiple threads wait for the same opcode, they will all be woken up.
+ *
+ * Note that event->msg_buf will only be duplicated if the event has a buffer
+ * with enough space already allocated. Otherwise, only the descriptor and
+ * message length will be copied.
+ *
+ * Returns: true if an event was found, false otherwise
+ */
+static void ice_aq_check_events(struct ice_pf *pf, u16 opcode,
+ struct ice_rq_event_info *event)
+{
+ struct ice_aq_task *task;
+ bool found = false;
+
+ spin_lock_bh(&pf->aq_wait_lock);
+ hlist_for_each_entry(task, &pf->aq_wait_list, entry) {
+ if (task->state || task->opcode != opcode)
+ continue;
+
+ memcpy(&task->event->desc, &event->desc, sizeof(event->desc));
+ task->event->msg_len = event->msg_len;
+
+ /* Only copy the data buffer if a destination was set */
+ if (task->event->msg_buf &&
+ task->event->buf_len > event->buf_len) {
+ memcpy(task->event->msg_buf, event->msg_buf,
+ event->buf_len);
+ task->event->buf_len = event->buf_len;
+ }
+
+ task->state = ICE_AQ_TASK_COMPLETE;
+ found = true;
+ }
+ spin_unlock_bh(&pf->aq_wait_lock);
+
+ if (found)
+ wake_up(&pf->aq_wait_queue);
+}
+
+/**
+ * ice_aq_cancel_waiting_tasks - Immediately cancel all waiting tasks
+ * @pf: the PF private structure
+ *
+ * Set all waiting tasks to ICE_AQ_TASK_CANCELED, and wake up their threads.
+ * This will then cause ice_aq_wait_for_event to exit with -ECANCELED.
+ */
+static void ice_aq_cancel_waiting_tasks(struct ice_pf *pf)
+{
+ struct ice_aq_task *task;
+
+ spin_lock_bh(&pf->aq_wait_lock);
+ hlist_for_each_entry(task, &pf->aq_wait_list, entry)
+ task->state = ICE_AQ_TASK_CANCELED;
+ spin_unlock_bh(&pf->aq_wait_lock);
+
+ wake_up(&pf->aq_wait_queue);
+}
+
/**
* __ice_clean_ctrlq - helper function to clean controlq rings
* @pf: ptr to struct ice_pf
@@ -1116,6 +1261,9 @@ static int __ice_clean_ctrlq(struct ice_pf *pf, enum ice_ctl_q q_type)
opcode = le16_to_cpu(event.desc.opcode);
+ /* Notify any thread that might be waiting for this event */
+ ice_aq_check_events(pf, opcode, &event);
+
switch (opcode) {
case ice_aqc_opc_get_link_status:
if (ice_handle_link_event(pf, &event))
@@ -3202,6 +3350,10 @@ static int ice_init_pf(struct ice_pf *pf)
mutex_init(&pf->sw_mutex);
mutex_init(&pf->tc_mutex);
+ INIT_HLIST_HEAD(&pf->aq_wait_list);
+ spin_lock_init(&pf->aq_wait_lock);
+ init_waitqueue_head(&pf->aq_wait_queue);
+
/* setup service timer and periodic service task */
timer_setup(&pf->serv_tmr, ice_service_timer, 0);
pf->serv_tmr_period = HZ;
@@ -4189,6 +4341,8 @@ static void ice_remove(struct pci_dev *pdev)
set_bit(__ICE_DOWN, pf->state);
ice_service_task_stop(pf);
+ ice_aq_cancel_waiting_tasks(pf);
+
mutex_destroy(&(&pf->hw)->fdir_fltr_lock);
if (!ice_is_safe_mode(pf))
ice_remove_arfs(pf);
--
2.27.0.353.gb9a2d1a0207f
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC PATCH net-next v2 5/6] ice: implement device flash update via devlink
2020-07-17 18:35 ` [RFC PATCH net-next v2 5/6] ice: implement device flash update via devlink Jacob Keller
@ 2020-07-20 5:26 ` kernel test robot
2020-07-23 23:33 ` Jacob Keller
1 sibling, 0 replies; 36+ messages in thread
From: kernel test robot @ 2020-07-20 5:26 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3645 bytes --]
Hi Jacob,
[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on jkirsher-next-queue/dev-queue]
[cannot apply to net-next/master ipvs/master linus/master v5.8-rc6 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jacob-Keller/introduce-PLDM-firmware-update-library/20200718-031225
base: https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git dev-queue
config: arm-randconfig-c022-20200717 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from lib/pldmfw/pldmfw.c:15:
>> lib/pldmfw/pldmfw_private.h:60:1: warning: alignment 1 of 'struct __pldm_header' is less than 4 [-Wpacked-not-aligned]
60 | } __packed __aligned(1);
| ^
>> lib/pldmfw/pldmfw_private.h:45:26: warning: 'release_date' offset 19 in 'struct __pldm_header' isn't aligned to 4 [-Wpacked-not-aligned]
45 | struct __pldm_timestamp release_date; /* PackageReleaseDateTime */
| ^~~~~~~~~~~~
vim +60 lib/pldmfw/pldmfw_private.h
6b3d6687bf587b Jacob Keller 2020-07-17 39
6b3d6687bf587b Jacob Keller 2020-07-17 40 /* Package Header Information */
6b3d6687bf587b Jacob Keller 2020-07-17 41 struct __pldm_header {
6b3d6687bf587b Jacob Keller 2020-07-17 42 uuid_t id; /* PackageHeaderIdentifier */
6b3d6687bf587b Jacob Keller 2020-07-17 43 u8 revision; /* PackageHeaderFormatRevision */
6b3d6687bf587b Jacob Keller 2020-07-17 44 __le16 size; /* PackageHeaderSize */
6b3d6687bf587b Jacob Keller 2020-07-17 @45 struct __pldm_timestamp release_date; /* PackageReleaseDateTime */
6b3d6687bf587b Jacob Keller 2020-07-17 46 __le16 component_bitmap_len; /* ComponentBitmapBitLength */
6b3d6687bf587b Jacob Keller 2020-07-17 47 u8 version_type; /* PackageVersionStringType */
6b3d6687bf587b Jacob Keller 2020-07-17 48 u8 version_len; /* PackageVersionStringLength */
6b3d6687bf587b Jacob Keller 2020-07-17 49
6b3d6687bf587b Jacob Keller 2020-07-17 50 /*
6b3d6687bf587b Jacob Keller 2020-07-17 51 * DSP0267 also includes the following variable length fields at the
6b3d6687bf587b Jacob Keller 2020-07-17 52 * end of this structure:
6b3d6687bf587b Jacob Keller 2020-07-17 53 *
6b3d6687bf587b Jacob Keller 2020-07-17 54 * PackageVersionString, length is version_len.
6b3d6687bf587b Jacob Keller 2020-07-17 55 *
6b3d6687bf587b Jacob Keller 2020-07-17 56 * The total size of this section is
6b3d6687bf587b Jacob Keller 2020-07-17 57 * sizeof(pldm_header) + version_len;
6b3d6687bf587b Jacob Keller 2020-07-17 58 */
6b3d6687bf587b Jacob Keller 2020-07-17 59 u8 version_string[]; /* PackageVersionString */
6b3d6687bf587b Jacob Keller 2020-07-17 @60 } __packed __aligned(1);
6b3d6687bf587b Jacob Keller 2020-07-17 61
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28549 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH net-next v2 5/6] ice: implement device flash update via devlink
2020-07-17 18:35 ` [RFC PATCH net-next v2 5/6] ice: implement device flash update via devlink Jacob Keller
2020-07-20 5:26 ` kernel test robot
@ 2020-07-23 23:33 ` Jacob Keller
1 sibling, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-23 23:33 UTC (permalink / raw)
To: netdev, Jiri Pirko, Jakub Kicinski
Cc: Jakub Kicinski, Jiri Pirko, Tom Herbert
On 7/17/2020 11:35 AM, Jacob Keller wrote:
> + devlink_flash_update_begin_notify(devlink);
> + devlink_flash_update_status_notify(devlink, "Preparing to flash",
> + component, 0, 0);
> + err = ice_flash_pldm_image(pf, fw, extack);
> + devlink_flash_update_end_notify(devlink);
> +
> + release_firmware(fw);
> +
> + return err;
> +}
> +
Hi Jakub, Jiri,
I noticed something interesting recently in regards to the
devlink_flash_update_end_notify.
The way that iproute2/devlink works is that if
DEVLINK_CMD_FLASH_UPDATE_END is sent, then it will stop waiting for the
error response from DEVLINK_CMD_FLASH_UPDATE.
This means that if a driver sends DEVLINK_CMD_FLASH_UPDATE_END on a
failed update, the devlink program doesn't report the error or the
netlink status message.
Is it expected to send DEVLINK_CMD_FLASH_UPDATE_END on failures? All
current drivers appear to do so.
Perhaps its a bug in the devlink application on not waiting for the
flash command to properly complete?
Would it make sense to extend DEVLINK_FLASH_UPDATE_END to include an
attribute which specified the error code? But then would that include
the netlink extended status message?
It's not a huge deal but it seemed weird that if we detect any errors
during the main flash update process we will not properly report them.
I wasn't quite sure where the bug actually lies, so help here is
appreciated on figuring out what the best fix is.
Regards,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-17 18:35 [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jacob Keller
` (4 preceding siblings ...)
2020-07-17 18:35 ` [RFC PATCH net-next v2 5/6] ice: implement device flash update via devlink Jacob Keller
@ 2020-07-17 18:35 ` Jacob Keller
2020-07-20 10:09 ` Jiri Pirko
2020-07-17 19:58 ` [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jakub Kicinski
6 siblings, 1 reply; 36+ messages in thread
From: Jacob Keller @ 2020-07-17 18:35 UTC (permalink / raw)
To: netdev
Cc: Jakub Kicinski, Jiri Pirko, Tom Herbert, Jacob Keller,
Jiri Pirko, Jakub Kicinski, Jonathan Corbet, Michael Chan,
Bin Luo, Saeed Mahameed, Leon Romanovsky, Ido Schimmel,
Danielle Ratson
A flash image may contain settings or device identifying information.
When performing a flash update, these settings and information may
conflict with contents already in the flash. Devices may handle this
conflict in multiple ways.
Add a new attribute to the devlink command,
DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE, which specifies how the device
should handle these settings and fields.
DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, the default, requests that all
settings and device identifiers within the current flash are kept. That
is, no settings or fields will be overwritten. This is the expected
behavior for most updates, and appears to be how all of the drivers are
implemented today.
DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS, requests that the device
overwrite any device settings in the flash section with the settings
from the flash image, but to preserve identifiers such as the MAC
address and serial identifier. This may be useful as a way to restore
a device to known-good settings from a new flash image.
DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING, requests that all content in
the flash image be preserved over content of flash on the device. This
mode requests the device to completely overwrite the flash section,
possibly changing settings and device identifiers. The primary
motivation is to support writing initial device identifiers during
manufacturing. It is not expected to be necessary in normal end-user
flash updates.
For the ice driver, implement support for the overwrite mode by
selecting the associated preservation level to request from firmware.
For all other drivers that support flash update, require that the mode
be DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, which is the expected
default.
Update the documentation to explain the overwrite mode attribute.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: Bin Luo <luobin9@huawei.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Ido Schimmel <idosch@mellanox.com>
Cc: Danielle Ratson <danieller@mellanox.com>
---
.../networking/devlink/devlink-flash.rst | 31 +++++++++++++++++++
Documentation/networking/devlink/ice.rst | 27 ++++++++++++++++
.../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 7 ++++-
.../net/ethernet/huawei/hinic/hinic_devlink.c | 3 ++
drivers/net/ethernet/intel/ice/ice_devlink.c | 6 ++--
.../net/ethernet/intel/ice/ice_fw_update.c | 24 +++++++++++++-
.../net/ethernet/intel/ice/ice_fw_update.h | 1 +
.../net/ethernet/mellanox/mlx5/core/devlink.c | 4 +++
drivers/net/ethernet/mellanox/mlxsw/core.c | 3 +-
drivers/net/ethernet/mellanox/mlxsw/core.h | 1 +
.../net/ethernet/mellanox/mlxsw/spectrum.c | 4 +++
.../net/ethernet/netronome/nfp/nfp_devlink.c | 6 +++-
drivers/net/netdevsim/dev.c | 1 +
include/net/devlink.h | 1 +
include/uapi/linux/devlink.h | 16 ++++++++++
net/core/devlink.c | 16 ++++++++--
16 files changed, 142 insertions(+), 9 deletions(-)
diff --git a/Documentation/networking/devlink/devlink-flash.rst b/Documentation/networking/devlink/devlink-flash.rst
index 40a87c0222cb..75f7e284a97a 100644
--- a/Documentation/networking/devlink/devlink-flash.rst
+++ b/Documentation/networking/devlink/devlink-flash.rst
@@ -16,6 +16,37 @@ Note that the file name is a path relative to the firmware loading path
(usually ``/lib/firmware/``). Drivers may send status updates to inform
user space about the progress of the update operation.
+Overwrite Mode
+==============
+
+The ``devlink-flash`` command allows optionally specifying an overwrite mode
+indicating how the device should handle static settings and fields in the
+device flash when updating.
+
+.. list-table:: List of overwrite modes
+ :widths: 5 95
+
+ * - Name
+ - Description
+ * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING``
+ - Do not overwrite any settings or fields in the device flash; only
+ update the associated firmware binaries. This preserves settings
+ stored in the flash, as well as device identifiers. This is the
+ default if no overwrite mode is specified.
+ * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS``
+ - Overwrite device settings stored in the flash with settings from the
+ provided image, but do not overwrite device identifiers such as MAC
+ addresses or serial identifiers. This may be useful to restore a
+ device to default settings from a new image.
+ * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING``
+ - Overwrite everything in the device flash including settings and
+ device identifiers with the contents from the provided flash image.
+ Unless the provided image has been customized for this device, it
+ will result in clearing the identifying information in the device
+ flash. This mode is primarily intended for device manufacturers
+ performing initial device programming, and is not expected to be
+ necessary when performing regular flash updates.
+
Firmware Loading
================
diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
index 237848d56f9b..0f4428d7e693 100644
--- a/Documentation/networking/devlink/ice.rst
+++ b/Documentation/networking/devlink/ice.rst
@@ -81,6 +81,33 @@ The ``ice`` driver reports the following versions
- 0xee16ced7
- The first 4 bytes of the hash of the netlist module contents.
+Flash Update
+============
+
+The ``ice`` driver implements support for flash update using the
+``devlink-flash`` interface. It supports updating the device flash using a
+combined flash image that contains the ``fw.mgmt``, ``fw.undi``, and
+``fw.netlist`` components.
+
+.. list-table:: List of supported overwrite modes
+ :widths: 5 95
+
+ * - Name
+ - Behavior
+ * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING``
+ - Do not overwrite any settings or device identifying information.
+ * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS``
+ - Overwrite device settings, but not identifying information. This
+ includes overwriting the port configuration that determines the
+ number of physical functions the device will initialize with.
+ * - ``DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING``
+ - Overwrite everything in the flash with the contents from the provided
+ flash image. This includes overwriting all settings as well as device
+ identifying information such as the MAC address and device serial
+ number. It is expected that this be used with an image customized for
+ the specific device, and is not necessary or expected in most
+ circumstances.
+
Regions
=======
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 3a854195d5b0..eec99acd9d39 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -18,7 +18,9 @@
static int
bnxt_dl_flash_update(struct devlink *dl, const char *filename,
- const char *region, struct netlink_ext_ack *extack)
+ const char *region,
+ enum devlink_flash_update_overwrite_mode mode,
+ struct netlink_ext_ack *extack)
{
struct bnxt *bp = bnxt_get_bp_from_dl(dl);
int rc;
@@ -26,6 +28,9 @@ bnxt_dl_flash_update(struct devlink *dl, const char *filename,
if (region)
return -EOPNOTSUPP;
+ if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING)
+ return -EOPNOTSUPP;
+
if (!BNXT_PF(bp)) {
NL_SET_ERR_MSG_MOD(extack,
"flash update not supported from a VF");
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
index a40a10ac1ee9..7994b5615b0e 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
@@ -283,6 +283,7 @@ static int hinic_firmware_update(struct hinic_devlink_priv *priv,
static int hinic_devlink_flash_update(struct devlink *devlink,
const char *file_name,
const char *component,
+ enum devlink_flash_update_overwrite_mode mode,
struct netlink_ext_ack *extack)
{
struct hinic_devlink_priv *priv = devlink_priv(devlink);
@@ -291,6 +292,8 @@ static int hinic_devlink_flash_update(struct devlink *devlink,
if (component)
return -EOPNOTSUPP;
+ if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING)
+ return -EOPNOTSUPP;
err = request_firmware_direct(&fw, file_name,
&priv->hwdev->hwif->pdev->dev);
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index dbbd8b6f9d1a..d53e5d86857a 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -244,7 +244,9 @@ static int ice_devlink_info_get(struct devlink *devlink,
*/
static int
ice_devlink_flash_update(struct devlink *devlink, const char *path,
- const char *component, struct netlink_ext_ack *extack)
+ const char *component,
+ enum devlink_flash_update_overwrite_mode mode,
+ struct netlink_ext_ack *extack)
{
struct ice_pf *pf = devlink_priv(devlink);
struct device *dev = &pf->pdev->dev;
@@ -274,7 +276,7 @@ ice_devlink_flash_update(struct devlink *devlink, const char *path,
devlink_flash_update_begin_notify(devlink);
devlink_flash_update_status_notify(devlink, "Preparing to flash",
component, 0, 0);
- err = ice_flash_pldm_image(pf, fw, extack);
+ err = ice_flash_pldm_image(pf, fw, mode, extack);
devlink_flash_update_end_notify(devlink);
release_firmware(fw);
diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
index 51b575ba197d..7d053e4879db 100644
--- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
@@ -630,6 +630,7 @@ static const struct pldmfw_ops ice_fwu_ops = {
* ice_flash_pldm_image - Write a PLDM-formatted firmware image to the device
* @pf: private device driver structure
* @fw: firmware object pointing to the relevant firmware file
+ * @mode: flash overwrite mode
* @extack: netlink extended ACK structure
*
* Parse the data for a given firmware file, verifying that it is a valid PLDM
@@ -643,6 +644,7 @@ static const struct pldmfw_ops ice_fwu_ops = {
* Returns: zero on success or a negative error code on failure.
*/
int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw,
+ enum devlink_flash_update_overwrite_mode mode,
struct netlink_ext_ack *extack)
{
struct device *dev = ice_pf_to_dev(pf);
@@ -657,7 +659,27 @@ int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw,
priv.context.dev = dev;
priv.extack = extack;
priv.pf = pf;
- priv.activate_flags = ICE_AQC_NVM_PRESERVE_ALL;
+
+ /* Based on the requested overwrite mode, determine what preservation
+ * level to specify when activating the NVM banks at the end of the
+ * update.
+ */
+ switch (mode) {
+ case DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING:
+ /* Preserve all settings and fields in the existing flash */
+ priv.activate_flags = ICE_AQC_NVM_PRESERVE_ALL;
+ break;
+ case DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS:
+ /* Overwrite settings, but preserve limited fields */
+ priv.activate_flags = ICE_AQC_NVM_PRESERVE_SELECTED;
+ break;
+ case DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING:
+ /* Overwrite everything in the flash */
+ priv.activate_flags = ICE_AQC_NVM_NO_PRESERVATION;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
status = ice_acquire_nvm(hw, ICE_RES_WRITE);
if (status) {
diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.h b/drivers/net/ethernet/intel/ice/ice_fw_update.h
index 79472cc618b4..66d539ae87d6 100644
--- a/drivers/net/ethernet/intel/ice/ice_fw_update.h
+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.h
@@ -5,6 +5,7 @@
#define _ICE_FW_UPDATE_H_
int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw,
+ enum devlink_flash_update_overwrite_mode mode,
struct netlink_ext_ack *extack);
int ice_check_for_pending_update(struct ice_pf *pf, const char *component,
struct netlink_ext_ack *extack);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
index c709e9a385f6..8c7472ff0376 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
@@ -10,6 +10,7 @@
static int mlx5_devlink_flash_update(struct devlink *devlink,
const char *file_name,
const char *component,
+ enum devlink_flash_update_overwrite_mode mode,
struct netlink_ext_ack *extack)
{
struct mlx5_core_dev *dev = devlink_priv(devlink);
@@ -19,6 +20,9 @@ static int mlx5_devlink_flash_update(struct devlink *devlink,
if (component)
return -EOPNOTSUPP;
+ if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING)
+ return -EOPNOTSUPP;
+
err = request_firmware_direct(&fw, file_name, &dev->pdev->dev);
if (err)
return err;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 1363168b3c82..310a863f0b69 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -1139,6 +1139,7 @@ mlxsw_devlink_core_bus_device_reload_up(struct devlink *devlink,
static int mlxsw_devlink_flash_update(struct devlink *devlink,
const char *file_name,
const char *component,
+ enum devlink_flash_update_overwrite_mode mode,
struct netlink_ext_ack *extack)
{
struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
@@ -1147,7 +1148,7 @@ static int mlxsw_devlink_flash_update(struct devlink *devlink,
if (!mlxsw_driver->flash_update)
return -EOPNOTSUPP;
return mlxsw_driver->flash_update(mlxsw_core, file_name,
- component, extack);
+ component, mode, extack);
}
static int mlxsw_devlink_trap_init(struct devlink *devlink,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index c1c1e039323a..d4ec9cb6e5f3 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -319,6 +319,7 @@ struct mlxsw_driver {
u32 *p_cur, u32 *p_max);
int (*flash_update)(struct mlxsw_core *mlxsw_core,
const char *file_name, const char *component,
+ enum devlink_flash_update_overwrite_mode mode,
struct netlink_ext_ack *extack);
int (*trap_init)(struct mlxsw_core *mlxsw_core,
const struct devlink_trap *trap, void *trap_ctx);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 519eb44e4097..52f099659827 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -419,6 +419,7 @@ static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp)
static int mlxsw_sp_flash_update(struct mlxsw_core *mlxsw_core,
const char *file_name, const char *component,
+ enum devlink_flash_update_overwrite_mode mode,
struct netlink_ext_ack *extack)
{
struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
@@ -428,6 +429,9 @@ static int mlxsw_sp_flash_update(struct mlxsw_core *mlxsw_core,
if (component)
return -EOPNOTSUPP;
+ if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING)
+ return -EOPNOTSUPP;
+
err = request_firmware_direct(&firmware, file_name,
mlxsw_sp->bus_info->dev);
if (err)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index be52510d446b..37401cde9fe9 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -330,10 +330,14 @@ nfp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
static int
nfp_devlink_flash_update(struct devlink *devlink, const char *path,
- const char *component, struct netlink_ext_ack *extack)
+ const char *component,
+ enum devlink_flash_update_overwrite_mode mode,
+ struct netlink_ext_ack *extack)
{
if (component)
return -EOPNOTSUPP;
+ if (mode != DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING)
+ return -EOPNOTSUPP;
return nfp_flash_update_common(devlink_priv(devlink), path, extack);
}
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index ce719c830a77..a75b1421d179 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -742,6 +742,7 @@ static int nsim_dev_info_get(struct devlink *devlink,
static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
const char *component,
+ enum devlink_flash_update_overwrite_mode mode,
struct netlink_ext_ack *extack)
{
struct nsim_dev *nsim_dev = devlink_priv(devlink);
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 913e8679ae35..a851209c7145 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1045,6 +1045,7 @@ struct devlink_ops {
struct netlink_ext_ack *extack);
int (*flash_update)(struct devlink *devlink, const char *file_name,
const char *component,
+ enum devlink_flash_update_overwrite_mode mode,
struct netlink_ext_ack *extack);
/**
* @trap_init: Trap initialization function.
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index cfef4245ea5a..b5341fe5fa61 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -228,6 +228,20 @@ enum {
DEVLINK_ATTR_STATS_MAX = __DEVLINK_ATTR_STATS_MAX - 1
};
+/* All flash update overwrite modes must be documented in
+ * Documentation/networking/devlink/devlink-flash.rst
+ */
+enum devlink_flash_update_overwrite_mode {
+ DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING,
+ DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS,
+ DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING,
+
+ /* Add new flash overwrite modes above */
+ __DEVLINK_FLASH_UPDATE_OVERWRITE_MODE_MAX,
+ DEVLINK_FLASH_UPDATE_OVERWRITE_MODE_MAX =
+ __DEVLINK_FLASH_UPDATE_OVERWRITE_MODE_MAX - 1,
+};
+
/**
* enum devlink_trap_action - Packet trap action.
* @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
@@ -458,6 +472,8 @@ enum devlink_attr {
DEVLINK_ATTR_PORT_LANES, /* u32 */
DEVLINK_ATTR_PORT_SPLITTABLE, /* u8 */
+ DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE, /* u8 */
+
/* add new attributes above here, update the policy in devlink.c */
__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6335e1851088..48b3a5739363 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3097,9 +3097,10 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
struct genl_info *info)
{
+ enum devlink_flash_update_overwrite_mode mode;
struct devlink *devlink = info->user_ptr[0];
+ struct nlattr *nla_component, *nla_mode;
const char *file_name, *component;
- struct nlattr *nla_component;
if (!devlink->ops->flash_update)
return -EOPNOTSUPP;
@@ -3111,7 +3112,13 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
nla_component = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
component = nla_component ? nla_data(nla_component) : NULL;
- return devlink->ops->flash_update(devlink, file_name, component,
+ nla_mode = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE];
+ mode = nla_mode ? nla_get_u8(nla_mode) : DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING;
+
+ if (mode > DEVLINK_FLASH_UPDATE_OVERWRITE_MODE_MAX)
+ return -EINVAL;
+
+ return devlink->ops->flash_update(devlink, file_name, component, mode,
info->extack);
}
@@ -6999,6 +7006,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER] = { .type = NLA_U8 },
[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME] = { .type = NLA_NUL_STRING },
[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT] = { .type = NLA_NUL_STRING },
+ [DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE] = { .type = NLA_U8 },
[DEVLINK_ATTR_TRAP_NAME] = { .type = NLA_NUL_STRING },
[DEVLINK_ATTR_TRAP_ACTION] = { .type = NLA_U8 },
[DEVLINK_ATTR_TRAP_GROUP_NAME] = { .type = NLA_NUL_STRING },
@@ -9564,7 +9572,9 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
}
mutex_lock(&devlink->lock);
- ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL);
+ ret = devlink->ops->flash_update(devlink, file_name, NULL,
+ DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING,
+ NULL);
mutex_unlock(&devlink->lock);
out:
--
2.27.0.353.gb9a2d1a0207f
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-17 18:35 ` [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update Jacob Keller
@ 2020-07-20 10:09 ` Jiri Pirko
2020-07-20 15:51 ` Jakub Kicinski
0 siblings, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2020-07-20 10:09 UTC (permalink / raw)
To: Jacob Keller
Cc: netdev, Jakub Kicinski, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
Fri, Jul 17, 2020 at 08:35:41PM CEST, jacob.e.keller@intel.com wrote:
>A flash image may contain settings or device identifying information.
>When performing a flash update, these settings and information may
>conflict with contents already in the flash. Devices may handle this
>conflict in multiple ways.
>
>Add a new attribute to the devlink command,
>DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MODE, which specifies how the device
>should handle these settings and fields.
>
>DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, the default, requests that all
>settings and device identifiers within the current flash are kept. That
>is, no settings or fields will be overwritten. This is the expected
>behavior for most updates, and appears to be how all of the drivers are
>implemented today.
>
>DEVLINK_FLASH_UPDATE_OVERWRITE_SETTINGS, requests that the device
>overwrite any device settings in the flash section with the settings
>from the flash image, but to preserve identifiers such as the MAC
>address and serial identifier. This may be useful as a way to restore
>a device to known-good settings from a new flash image.
>
>DEVLINK_FLASH_UPDATE_OVERWRITE_EVERYTHING, requests that all content in
>the flash image be preserved over content of flash on the device. This
>mode requests the device to completely overwrite the flash section,
>possibly changing settings and device identifiers. The primary
>motivation is to support writing initial device identifiers during
>manufacturing. It is not expected to be necessary in normal end-user
>flash updates.
>
>For the ice driver, implement support for the overwrite mode by
>selecting the associated preservation level to request from firmware.
>
>For all other drivers that support flash update, require that the mode
>be DEVLINK_FLASH_UPDATE_OVERWRITE_NOTHING, which is the expected
>default.
>
>Update the documentation to explain the overwrite mode attribute.
This looks odd. You have a single image yet you somehow divide it
into "program" and "config" areas. We already have infra in place to
take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
You should have 2 components:
1) "program"
2) "config"
Then it is up to the user what he decides to flash.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-20 10:09 ` Jiri Pirko
@ 2020-07-20 15:51 ` Jakub Kicinski
2020-07-20 18:52 ` Jacob Keller
2020-07-21 13:53 ` Jiri Pirko
0 siblings, 2 replies; 36+ messages in thread
From: Jakub Kicinski @ 2020-07-20 15:51 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jacob Keller, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
> This looks odd. You have a single image yet you somehow divide it
> into "program" and "config" areas. We already have infra in place to
> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
> You should have 2 components:
> 1) "program"
> 2) "config"
>
> Then it is up to the user what he decides to flash.
99.9% of the time users want to flash "all". To achieve "don't flash
config" with current infra users would have to flash each component
one by one and then omit the one(s) which is config (guessing which
one that is based on the name).
Wouldn't this be quite inconvenient?
In case of MLX is PSID considered config?
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-20 15:51 ` Jakub Kicinski
@ 2020-07-20 18:52 ` Jacob Keller
2020-07-21 13:56 ` Jiri Pirko
2020-07-21 13:53 ` Jiri Pirko
1 sibling, 1 reply; 36+ messages in thread
From: Jacob Keller @ 2020-07-20 18:52 UTC (permalink / raw)
To: Jakub Kicinski, Jiri Pirko
Cc: netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
On 7/20/2020 8:51 AM, Jakub Kicinski wrote:
> On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
>> This looks odd. You have a single image yet you somehow divide it
>> into "program" and "config" areas. We already have infra in place to
>> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
>> You should have 2 components:
>> 1) "program"
>> 2) "config"
>>
First off, unfortunately at least for ice, the "main" section of NVM
contains both the management firmware as well as config settings. I
don't really have a way to split it up.
This series includes support for updating the main NVM section
containing the management firmware (and some config) "fw.mgmt", as well
as "fw.undi" which contains the OptionROM, and "fw.netlist" which
contains additional configuration TLVs.
The firmware interface allows me to separate the three components, but
does not let me separate the "fw binary" from the "config settings" that
are stored within the main NVM bank. (These fields include other data
like the device MAC address and VPD area of the device too, so using
"config" is a bit of a misnomer).
>> Then it is up to the user what he decides to flash.
>
> 99.9% of the time users want to flash "all". To achieve "don't flash
> config" with current infra users would have to flash each component
> one by one and then omit the one(s) which is config (guessing which
> one that is based on the name).
>
> Wouldn't this be quite inconvenient?
>
I also agree here, I'd like to be able to make the "update with the
complete file" just work in the most straight forward way (i.e. without
erasing stuff by accident) be the default.
The option I'm proposing here is to enable allowing tools to optionally
specify handling this type of overwrite. The goal is to support these
use cases:
a) (default) just update the image, but keep the config and vital data
the same as before the update.
b) overwrite config fields, but keep vital fields the same. Intended to
allow returning configuration to "image defaults". This mostly is
intended in case regular update caused some issues like if somehow the
config preservation didn't work properly.
c) overwrite all fields. The intention here is to allow programming a
customized image during initial setup that would contain new IDs etc. It
is not expected to be used in general, as this does overwrite vital data
like the MAC addresses and such.
So the problem is that the vital data, config data, and firmware
binaries are stored in the same section, without a good way to separate
between them. We program all of these together as one chunk to the
"secondary NVM bank" and then ask firmware to update. It reads through
and based on our "preservation" setting will update the binaries and
merge the configuration sections.
> In case of MLX is PSID considered config?
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-20 18:52 ` Jacob Keller
@ 2020-07-21 13:56 ` Jiri Pirko
2020-07-21 17:28 ` Jacob Keller
0 siblings, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2020-07-21 13:56 UTC (permalink / raw)
To: Jacob Keller
Cc: Jakub Kicinski, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
Mon, Jul 20, 2020 at 08:52:58PM CEST, jacob.e.keller@intel.com wrote:
>
>
>On 7/20/2020 8:51 AM, Jakub Kicinski wrote:
>> On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
>>> This looks odd. You have a single image yet you somehow divide it
>>> into "program" and "config" areas. We already have infra in place to
>>> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
>>> You should have 2 components:
>>> 1) "program"
>>> 2) "config"
>>>
>
>First off, unfortunately at least for ice, the "main" section of NVM
>contains both the management firmware as well as config settings. I
>don't really have a way to split it up.
You don't have to split it up. Just for component "x" you push binary
"A" and flash part of it and for comonent "y" you push the same binary
"A" and flash different part of it.
Consider the component as a "mask" in your case.
>
>This series includes support for updating the main NVM section
>containing the management firmware (and some config) "fw.mgmt", as well
>as "fw.undi" which contains the OptionROM, and "fw.netlist" which
>contains additional configuration TLVs.
>
>The firmware interface allows me to separate the three components, but
>does not let me separate the "fw binary" from the "config settings" that
>are stored within the main NVM bank. (These fields include other data
>like the device MAC address and VPD area of the device too, so using
>"config" is a bit of a misnomer).
>
>>> Then it is up to the user what he decides to flash.
>>
>> 99.9% of the time users want to flash "all". To achieve "don't flash
>> config" with current infra users would have to flash each component
>> one by one and then omit the one(s) which is config (guessing which
>> one that is based on the name).
>>
>> Wouldn't this be quite inconvenient?
>>
>
>I also agree here, I'd like to be able to make the "update with the
>complete file" just work in the most straight forward way (i.e. without
>erasing stuff by accident) be the default.
>
>The option I'm proposing here is to enable allowing tools to optionally
>specify handling this type of overwrite. The goal is to support these
>use cases:
>
>a) (default) just update the image, but keep the config and vital data
>the same as before the update.
>
>b) overwrite config fields, but keep vital fields the same. Intended to
>allow returning configuration to "image defaults". This mostly is
>intended in case regular update caused some issues like if somehow the
>config preservation didn't work properly.
>
>c) overwrite all fields. The intention here is to allow programming a
>customized image during initial setup that would contain new IDs etc. It
>is not expected to be used in general, as this does overwrite vital data
>like the MAC addresses and such.
>
>So the problem is that the vital data, config data, and firmware
>binaries are stored in the same section, without a good way to separate
>between them. We program all of these together as one chunk to the
>"secondary NVM bank" and then ask firmware to update. It reads through
>and based on our "preservation" setting will update the binaries and
>merge the configuration sections.
>
>> In case of MLX is PSID considered config?
>>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-21 13:56 ` Jiri Pirko
@ 2020-07-21 17:28 ` Jacob Keller
0 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-21 17:28 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
On 7/21/2020 6:56 AM, Jiri Pirko wrote:
> Mon, Jul 20, 2020 at 08:52:58PM CEST, jacob.e.keller@intel.com wrote:
>>
>>
>> On 7/20/2020 8:51 AM, Jakub Kicinski wrote:
>>> On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
>>>> This looks odd. You have a single image yet you somehow divide it
>>>> into "program" and "config" areas. We already have infra in place to
>>>> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
>>>> You should have 2 components:
>>>> 1) "program"
>>>> 2) "config"
>>>>
>>
>> First off, unfortunately at least for ice, the "main" section of NVM
>> contains both the management firmware as well as config settings. I
>> don't really have a way to split it up.
>
> You don't have to split it up. Just for component "x" you push binary
> "A" and flash part of it and for comonent "y" you push the same binary
> "A" and flash different part of it.
>
> Consider the component as a "mask" in your case.
>
>
The driver itself doesn't really know what parts of the image are which.
I have to ask the firmware. And it doesn't have a "settings only" flag.
I have roughly equivalent to "binary only", "binary + settings" and
"binary + settings + vital fields"
Plus, this means that every update must be single-component and there's
no way to distinguish this when an update is supposed to be for all of
the components in the PLDM file.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-20 15:51 ` Jakub Kicinski
2020-07-20 18:52 ` Jacob Keller
@ 2020-07-21 13:53 ` Jiri Pirko
2020-07-21 17:04 ` Jakub Kicinski
1 sibling, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2020-07-21 13:53 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jacob Keller, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote:
>On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
>> This looks odd. You have a single image yet you somehow divide it
>> into "program" and "config" areas. We already have infra in place to
>> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
>> You should have 2 components:
>> 1) "program"
>> 2) "config"
>>
>> Then it is up to the user what he decides to flash.
>
>99.9% of the time users want to flash "all". To achieve "don't flash
>config" with current infra users would have to flash each component
Well you can have multiple component what would overlap:
1) "program" + "config" (default)
2) "program"
3) "config"
>one by one and then omit the one(s) which is config (guessing which
>one that is based on the name).
>
>Wouldn't this be quite inconvenient?
I see it as an extra knob that is actually somehow provides degradation
of components.
>
>In case of MLX is PSID considered config?
Nope.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-21 13:53 ` Jiri Pirko
@ 2020-07-21 17:04 ` Jakub Kicinski
2020-07-21 17:31 ` Jacob Keller
2020-07-22 10:51 ` Jiri Pirko
0 siblings, 2 replies; 36+ messages in thread
From: Jakub Kicinski @ 2020-07-21 17:04 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jacob Keller, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote:
> Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote:
> >On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
> >> This looks odd. You have a single image yet you somehow divide it
> >> into "program" and "config" areas. We already have infra in place to
> >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
> >> You should have 2 components:
> >> 1) "program"
> >> 2) "config"
> >>
> >> Then it is up to the user what he decides to flash.
> >
> >99.9% of the time users want to flash "all". To achieve "don't flash
> >config" with current infra users would have to flash each component
>
> Well you can have multiple component what would overlap:
> 1) "program" + "config" (default)
> 2) "program"
> 3) "config"
Say I have FW component and UNDI driver. Now I'll have 4 components?
fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just
"implied"? If they are visible what version does the config have?
Also (3) - flashing config from one firmware version and program from
another - makes a very limited amount of sense to me.
> >one by one and then omit the one(s) which is config (guessing which
> >one that is based on the name).
> >
> >Wouldn't this be quite inconvenient?
>
> I see it as an extra knob that is actually somehow provides degradation
> of components.
Hm. We have the exact opposite view on the matter. To me components
currently correspond to separate fw/hw entities, that's a very clear
meaning. PHY firmware, management FW, UNDI. Now we would add a
completely orthogonal meaning to the same API.
Why?
In the name of "field reuse"?
> >In case of MLX is PSID considered config?
>
> Nope.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-21 17:04 ` Jakub Kicinski
@ 2020-07-21 17:31 ` Jacob Keller
2020-07-22 10:51 ` Jiri Pirko
1 sibling, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-21 17:31 UTC (permalink / raw)
To: Jakub Kicinski, Jiri Pirko
Cc: netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
On 7/21/2020 10:04 AM, Jakub Kicinski wrote:
> On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote:
>> Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote:
>>> On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
>>>> This looks odd. You have a single image yet you somehow divide it
>>>> into "program" and "config" areas. We already have infra in place to
>>>> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
>>>> You should have 2 components:
>>>> 1) "program"
>>>> 2) "config"
>>>>
>>>> Then it is up to the user what he decides to flash.
>>>
>>> 99.9% of the time users want to flash "all". To achieve "don't flash
>>> config" with current infra users would have to flash each component
>>
>> Well you can have multiple component what would overlap:
>> 1) "program" + "config" (default)
>> 2) "program"
>> 3) "config"
>
> Say I have FW component and UNDI driver. Now I'll have 4 components?
> fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just
> "implied"? If they are visible what version does the config have?
>
> Also (3) - flashing config from one firmware version and program from
> another - makes a very limited amount of sense to me.
>
Right, this is actually one of the potential problems I've been told
about: if the config doesn't match the firmware it's supposed to work,
but the "overwrite config" option is partially there to help have a way
out in case the config and firmware aren't in sync and something goes wrong.
>>> one by one and then omit the one(s) which is config (guessing which
>>> one that is based on the name).
>>>
>>> Wouldn't this be quite inconvenient?
>>
>> I see it as an extra knob that is actually somehow provides degradation
>> of components.
>
> Hm. We have the exact opposite view on the matter. To me components
> currently correspond to separate fw/hw entities, that's a very clear
> meaning. PHY firmware, management FW, UNDI. Now we would add a
> completely orthogonal meaning to the same API.
>
> Why?
>
> In the name of "field reuse"?
>
Right. I understand that other hardware works differently and has all
config separated to separate distinct components, but I think it would
be needlessly confusing to have separate component names. Plus, as I
said in another thread: I can't really separate the two components when
I update. I have to send the combined block to firmware with the flag
indicating how it should do preservation/merging. So I can't really do
"just settings" anyways, meaning that it really would be two components
which overlap. Plus, I wouldn't really have a separate "info" display.
Ultimately it ends up feeling like a significant hack of the component
name if I go that route.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-21 17:04 ` Jakub Kicinski
2020-07-21 17:31 ` Jacob Keller
@ 2020-07-22 10:51 ` Jiri Pirko
2020-07-22 15:30 ` Keller, Jacob E
1 sibling, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2020-07-22 10:51 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jacob Keller, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
Tue, Jul 21, 2020 at 07:04:06PM CEST, kubakici@wp.pl wrote:
>On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote:
>> Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote:
>> >On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
>> >> This looks odd. You have a single image yet you somehow divide it
>> >> into "program" and "config" areas. We already have infra in place to
>> >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
>> >> You should have 2 components:
>> >> 1) "program"
>> >> 2) "config"
>> >>
>> >> Then it is up to the user what he decides to flash.
>> >
>> >99.9% of the time users want to flash "all". To achieve "don't flash
>> >config" with current infra users would have to flash each component
>>
>> Well you can have multiple component what would overlap:
>> 1) "program" + "config" (default)
>> 2) "program"
>> 3) "config"
>
>Say I have FW component and UNDI driver. Now I'll have 4 components?
>fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just
Visible in which sense? We don't show components anywhere if I'm not
mistaken. They are currently very rarely used. Basically we just ported
it from ethtool without much thinking.
>"implied"? If they are visible what version does the config have?
Good question. we don't have per-component version so far. I think it
would be good to have it alonside with the listing.
>
>Also (3) - flashing config from one firmware version and program from
>another - makes a very limited amount of sense to me.
>
>> >one by one and then omit the one(s) which is config (guessing which
>> >one that is based on the name).
>> >
>> >Wouldn't this be quite inconvenient?
>>
>> I see it as an extra knob that is actually somehow provides degradation
>> of components.
>
>Hm. We have the exact opposite view on the matter. To me components
>currently correspond to separate fw/hw entities, that's a very clear
>meaning. PHY firmware, management FW, UNDI. Now we would add a
>completely orthogonal meaning to the same API.
I understand. My concern is, we would have a component with some
"subparts". Now it is some fuzzy vagely defined "config part",
in the future it might be something else. That is what I'm concerned
about. Components have clear api.
So perhaps we can introduce something like "component mask", which would
allow to flash only part of the component. That is basically what Jacob
has, I would just like to have it well defined.
>
>Why?
>
>In the name of "field reuse"?
>
>> >In case of MLX is PSID considered config?
>>
>> Nope.
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-22 10:51 ` Jiri Pirko
@ 2020-07-22 15:30 ` Keller, Jacob E
2020-07-22 16:52 ` Jakub Kicinski
2020-07-26 7:16 ` Jiri Pirko
0 siblings, 2 replies; 36+ messages in thread
From: Keller, Jacob E @ 2020-07-22 15:30 UTC (permalink / raw)
To: Jiri Pirko, Jakub Kicinski
Cc: netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Jiri Pirko
> Sent: Wednesday, July 22, 2020 3:52 AM
> To: Jakub Kicinski <kubakici@wp.pl>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; Tom
> Herbert <tom@herbertland.com>; Jiri Pirko <jiri@mellanox.com>; Jakub Kicinski
> <kuba@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Michael Chan
> <michael.chan@broadcom.com>; Bin Luo <luobin9@huawei.com>; Saeed
> Mahameed <saeedm@mellanox.com>; Leon Romanovsky <leon@kernel.org>;
> Ido Schimmel <idosch@mellanox.com>; Danielle Ratson
> <danieller@mellanox.com>
> Subject: Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash
> update
>
> Tue, Jul 21, 2020 at 07:04:06PM CEST, kubakici@wp.pl wrote:
> >On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote:
> >> Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote:
> >> >On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
> >> >> This looks odd. You have a single image yet you somehow divide it
> >> >> into "program" and "config" areas. We already have infra in place to
> >> >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
> >> >> You should have 2 components:
> >> >> 1) "program"
> >> >> 2) "config"
> >> >>
> >> >> Then it is up to the user what he decides to flash.
> >> >
> >> >99.9% of the time users want to flash "all". To achieve "don't flash
> >> >config" with current infra users would have to flash each component
> >>
> >> Well you can have multiple component what would overlap:
> >> 1) "program" + "config" (default)
> >> 2) "program"
> >> 3) "config"
> >
> >Say I have FW component and UNDI driver. Now I'll have 4 components?
> >fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just
>
> Visible in which sense? We don't show components anywhere if I'm not
> mistaken. They are currently very rarely used. Basically we just ported
> it from ethtool without much thinking.
>
Component names are used in devlink info and displayed to end users along with versions, plus they're names passed by the user in devlink flash update. As far as documented, we shouldn't add new components without associated versions in the info report.
>
> >"implied"? If they are visible what version does the config have?
>
> Good question. we don't have per-component version so far. I think it
> would be good to have it alonside with the listing.
>
>
> >
> >Also (3) - flashing config from one firmware version and program from
> >another - makes a very limited amount of sense to me.
> >
> >> >one by one and then omit the one(s) which is config (guessing which
> >> >one that is based on the name).
> >> >
> >> >Wouldn't this be quite inconvenient?
> >>
> >> I see it as an extra knob that is actually somehow provides degradation
> >> of components.
> >
> >Hm. We have the exact opposite view on the matter. To me components
> >currently correspond to separate fw/hw entities, that's a very clear
> >meaning. PHY firmware, management FW, UNDI. Now we would add a
> >completely orthogonal meaning to the same API.
>
> I understand. My concern is, we would have a component with some
> "subparts". Now it is some fuzzy vagely defined "config part",
> in the future it might be something else. That is what I'm concerned
> about. Components have clear api.
>
> So perhaps we can introduce something like "component mask", which would
> allow to flash only part of the component. That is basically what Jacob
> has, I would just like to have it well defined.
>
>
So, we could make this selection a series of masked bits instead of a single enumeration value.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-22 15:30 ` Keller, Jacob E
@ 2020-07-22 16:52 ` Jakub Kicinski
2020-07-22 18:21 ` Jacob Keller
2020-07-29 22:49 ` Jacob Keller
2020-07-26 7:16 ` Jiri Pirko
1 sibling, 2 replies; 36+ messages in thread
From: Jakub Kicinski @ 2020-07-22 16:52 UTC (permalink / raw)
To: Keller, Jacob E
Cc: Jiri Pirko, netdev, Tom Herbert, Jiri Pirko, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote:
> > >> >one by one and then omit the one(s) which is config (guessing which
> > >> >one that is based on the name).
> > >> >
> > >> >Wouldn't this be quite inconvenient?
> > >>
> > >> I see it as an extra knob that is actually somehow provides degradation
> > >> of components.
> > >
> > >Hm. We have the exact opposite view on the matter. To me components
> > >currently correspond to separate fw/hw entities, that's a very clear
> > >meaning. PHY firmware, management FW, UNDI. Now we would add a
> > >completely orthogonal meaning to the same API.
> >
> > I understand. My concern is, we would have a component with some
> > "subparts". Now it is some fuzzy vagely defined "config part",
> > in the future it might be something else. That is what I'm concerned
> > about. Components have clear api.
> >
> > So perhaps we can introduce something like "component mask", which would
> > allow to flash only part of the component. That is basically what Jacob
> > has, I would just like to have it well defined.
>
> So, we could make this selection a series of masked bits instead of a
> single enumeration value.
I'd still argue that components (as defined in devlink info) and config
are pretty orthogonal. In my experience config is stored in its own
section of the flash, and some of the knobs are in no obvious way
associated with components (used by components).
That said, if we rename the "component mask" to "update mask" that's
fine with me.
Then we'd have
bit 0 - don't overwrite config
bit 1 - don't overwrite identifiers
?
Let's define a bit for "don't update program" when we actually need it.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-22 16:52 ` Jakub Kicinski
@ 2020-07-22 18:21 ` Jacob Keller
2020-07-26 7:18 ` Jiri Pirko
2020-07-29 22:49 ` Jacob Keller
1 sibling, 1 reply; 36+ messages in thread
From: Jacob Keller @ 2020-07-22 18:21 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jiri Pirko, netdev, Tom Herbert, Jiri Pirko, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
On 7/22/2020 9:52 AM, Jakub Kicinski wrote:
> On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote:
>>> So perhaps we can introduce something like "component mask", which would
>>> allow to flash only part of the component. That is basically what Jacob
>>> has, I would just like to have it well defined.
>>
>> So, we could make this selection a series of masked bits instead of a
>> single enumeration value.
>
> I'd still argue that components (as defined in devlink info) and config
> are pretty orthogonal. In my experience config is stored in its own
> section of the flash, and some of the knobs are in no obvious way
> associated with components (used by components).
>
> That said, if we rename the "component mask" to "update mask" that's
> fine with me.
>
> Then we'd have
>
> bit 0 - don't overwrite config
> bit 1 - don't overwrite identifiers
>
> ?
>
> Let's define a bit for "don't update program" when we actually need it.
>
Ok. And this can be later extended with additional bits with new
meanings should the need arise.
Additionally, drivers can ensure that the valid combination of bits is
set. the drivers can reject requests for combinations that they do not
support.
I can make that change.
My preference is that "0" for a bit means do not overwrite while "1"
means overwrite. This way, if/when additional bits are added, drivers
won't need to be updated to reject such requests. If we make "1" the "do
not overwrite" then we'd have a case where drivers must update to ensure
they reject requests which don't set the bit.
Thanks,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-22 18:21 ` Jacob Keller
@ 2020-07-26 7:18 ` Jiri Pirko
2020-07-27 18:11 ` Jacob Keller
0 siblings, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2020-07-26 7:18 UTC (permalink / raw)
To: Jacob Keller
Cc: Jakub Kicinski, netdev, Tom Herbert, Jiri Pirko, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
Wed, Jul 22, 2020 at 08:21:22PM CEST, jacob.e.keller@intel.com wrote:
>
>
>On 7/22/2020 9:52 AM, Jakub Kicinski wrote:
>> On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote:
>>>> So perhaps we can introduce something like "component mask", which would
>>>> allow to flash only part of the component. That is basically what Jacob
>>>> has, I would just like to have it well defined.
>>>
>>> So, we could make this selection a series of masked bits instead of a
>>> single enumeration value.
>>
>> I'd still argue that components (as defined in devlink info) and config
>> are pretty orthogonal. In my experience config is stored in its own
>> section of the flash, and some of the knobs are in no obvious way
>> associated with components (used by components).
>>
>> That said, if we rename the "component mask" to "update mask" that's
>> fine with me.
>>
>> Then we'd have
>>
>> bit 0 - don't overwrite config
>> bit 1 - don't overwrite identifiers
>>
>> ?
>>
>> Let's define a bit for "don't update program" when we actually need it.
>>
>
>
>Ok. And this can be later extended with additional bits with new
>meanings should the need arise.
>
>Additionally, drivers can ensure that the valid combination of bits is
>set. the drivers can reject requests for combinations that they do not
>support.
Makes sense.
>
>I can make that change.
>
>My preference is that "0" for a bit means do not overwrite while "1"
>means overwrite. This way, if/when additional bits are added, drivers
>won't need to be updated to reject such requests. If we make "1" the "do
>not overwrite" then we'd have a case where drivers must update to ensure
>they reject requests which don't set the bit.
0 should be default and driver should bahave accordingly.
>
>Thanks,
>Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-26 7:18 ` Jiri Pirko
@ 2020-07-27 18:11 ` Jacob Keller
0 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-27 18:11 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, netdev, Tom Herbert, Jiri Pirko, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
On 7/26/2020 12:18 AM, Jiri Pirko wrote:
> Wed, Jul 22, 2020 at 08:21:22PM CEST, jacob.e.keller@intel.com wrote:
>>
>>
>> On 7/22/2020 9:52 AM, Jakub Kicinski wrote:
>>> On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote:
>>>>> So perhaps we can introduce something like "component mask", which would
>>>>> allow to flash only part of the component. That is basically what Jacob
>>>>> has, I would just like to have it well defined.
>>>>
>>>> So, we could make this selection a series of masked bits instead of a
>>>> single enumeration value.
>>>
>>> I'd still argue that components (as defined in devlink info) and config
>>> are pretty orthogonal. In my experience config is stored in its own
>>> section of the flash, and some of the knobs are in no obvious way
>>> associated with components (used by components).
>>>
>>> That said, if we rename the "component mask" to "update mask" that's
>>> fine with me.
>>>
>>> Then we'd have
>>>
>>> bit 0 - don't overwrite config
>>> bit 1 - don't overwrite identifiers
>>>
>>> ?
>>>
>>> Let's define a bit for "don't update program" when we actually need it.
>>>
>>
>>
>> Ok. And this can be later extended with additional bits with new
>> meanings should the need arise.
>>
>> Additionally, drivers can ensure that the valid combination of bits is
>> set. the drivers can reject requests for combinations that they do not
>> support.
>
> Makes sense.
>
>>
>> I can make that change.
>>
>> My preference is that "0" for a bit means do not overwrite while "1"
>> means overwrite. This way, if/when additional bits are added, drivers
>> won't need to be updated to reject such requests. If we make "1" the "do
>> not overwrite" then we'd have a case where drivers must update to ensure
>> they reject requests which don't set the bit.
>
> 0 should be default and driver should bahave accordingly.
>
Correct, and it's good to spell that out more clearly.
Thanks,
Jake
>
>>
>> Thanks,
>> Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-22 16:52 ` Jakub Kicinski
2020-07-22 18:21 ` Jacob Keller
@ 2020-07-29 22:49 ` Jacob Keller
2020-07-29 23:16 ` Jakub Kicinski
1 sibling, 1 reply; 36+ messages in thread
From: Jacob Keller @ 2020-07-29 22:49 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jiri Pirko, netdev, Tom Herbert, Jiri Pirko, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
On 7/22/2020 9:52 AM, Jakub Kicinski wrote:
> On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote:
>>>>>> one by one and then omit the one(s) which is config (guessing which
>>>>>> one that is based on the name).
>>>>>>
>>>>>> Wouldn't this be quite inconvenient?
>>>>>
>>>>> I see it as an extra knob that is actually somehow provides degradation
>>>>> of components.
>>>>
>>>> Hm. We have the exact opposite view on the matter. To me components
>>>> currently correspond to separate fw/hw entities, that's a very clear
>>>> meaning. PHY firmware, management FW, UNDI. Now we would add a
>>>> completely orthogonal meaning to the same API.
>>>
>>> I understand. My concern is, we would have a component with some
>>> "subparts". Now it is some fuzzy vagely defined "config part",
>>> in the future it might be something else. That is what I'm concerned
>>> about. Components have clear api.
>>>
>>> So perhaps we can introduce something like "component mask", which would
>>> allow to flash only part of the component. That is basically what Jacob
>>> has, I would just like to have it well defined.
>>
>> So, we could make this selection a series of masked bits instead of a
>> single enumeration value.
>
> I'd still argue that components (as defined in devlink info) and config
> are pretty orthogonal. In my experience config is stored in its own
> section of the flash, and some of the knobs are in no obvious way
> associated with components (used by components).
>
> That said, if we rename the "component mask" to "update mask" that's
> fine with me.
>
> Then we'd have
>
> bit 0 - don't overwrite config
> bit 1 - don't overwrite identifiers
>
> ?
>
> Let's define a bit for "don't update program" when we actually need it.
>
One further wrinkle I was just reminded about. The ice hardware has a
section of the flash which defines a "minimum security revision". All
NVM images also have a "security revision". The firmware will fail to
load if the NVM image's security revision is less than the mimimum
security revision.
The minimum security revision is not updated automatically. Current
tools which had direct access have an optional "opt in to minimum
security revision update" which would optionally bump the minimum
security revision after an update. The intent is that once an image is
tested and verified to be stable, an administrator can opt in to prevent
downgrade below that security revision. (Thus preventing potential
downgrade to a known insecure image).
The folks adjusting our tools would like to continue to support this. I
think the best solution would be to have both the security revision and
minimum security revision become components, i.e.
"fw.mgmt.security_revision" and "fw.mgmt.min_security_revision" (maybe
shortened like "secrev or srev?), and then use the
fw.mgmt.min_security_revision component name in the flash update request.
The security revision is tied into the management firmware image and
would always be updated when an image is updated, but the minimum
revision is only updated on an explicit request request.
In theory this could be done as part of this overwrite, but since I
suspect this is somewhat device specific, (not sure other vendors have
something similar?), and because there is a valid/known version we can
report I think a component makes the most sense.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-29 22:49 ` Jacob Keller
@ 2020-07-29 23:16 ` Jakub Kicinski
2020-07-29 23:59 ` Jacob Keller
0 siblings, 1 reply; 36+ messages in thread
From: Jakub Kicinski @ 2020-07-29 23:16 UTC (permalink / raw)
To: Jacob Keller
Cc: Jiri Pirko, netdev, Tom Herbert, Jiri Pirko, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
On Wed, 29 Jul 2020 15:49:05 -0700 Jacob Keller wrote:
> The security revision is tied into the management firmware image and
> would always be updated when an image is updated, but the minimum
> revision is only updated on an explicit request request.
Does it have to be updated during FW flashing? Can't it be a devlink
param?
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-29 23:16 ` Jakub Kicinski
@ 2020-07-29 23:59 ` Jacob Keller
0 siblings, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-29 23:59 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jiri Pirko, netdev, Tom Herbert, Jiri Pirko, Jonathan Corbet,
Michael Chan, Bin Luo, Saeed Mahameed, Leon Romanovsky,
Ido Schimmel, Danielle Ratson
On 7/29/2020 4:16 PM, Jakub Kicinski wrote:
> On Wed, 29 Jul 2020 15:49:05 -0700 Jacob Keller wrote:
>> The security revision is tied into the management firmware image and
>> would always be updated when an image is updated, but the minimum
>> revision is only updated on an explicit request request.
>
> Does it have to be updated during FW flashing? Can't it be a devlink
> param?
>
Oh, right. I'd forgotten about that type of parameter. Makes sense. I'll
implement the current security revision as a component of flash info (so
that it can be reported via devlink info, and can't be changed) but the
minimum should be able to be a parameter just fine.
Thanks,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-22 15:30 ` Keller, Jacob E
2020-07-22 16:52 ` Jakub Kicinski
@ 2020-07-26 7:16 ` Jiri Pirko
2020-07-27 18:13 ` Jacob Keller
1 sibling, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2020-07-26 7:16 UTC (permalink / raw)
To: Keller, Jacob E
Cc: Jakub Kicinski, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
Wed, Jul 22, 2020 at 05:30:05PM CEST, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
>> Behalf Of Jiri Pirko
>> Sent: Wednesday, July 22, 2020 3:52 AM
>> To: Jakub Kicinski <kubakici@wp.pl>
>> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; Tom
>> Herbert <tom@herbertland.com>; Jiri Pirko <jiri@mellanox.com>; Jakub Kicinski
>> <kuba@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Michael Chan
>> <michael.chan@broadcom.com>; Bin Luo <luobin9@huawei.com>; Saeed
>> Mahameed <saeedm@mellanox.com>; Leon Romanovsky <leon@kernel.org>;
>> Ido Schimmel <idosch@mellanox.com>; Danielle Ratson
>> <danieller@mellanox.com>
>> Subject: Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash
>> update
>>
>> Tue, Jul 21, 2020 at 07:04:06PM CEST, kubakici@wp.pl wrote:
>> >On Tue, 21 Jul 2020 15:53:56 +0200 Jiri Pirko wrote:
>> >> Mon, Jul 20, 2020 at 05:51:59PM CEST, kubakici@wp.pl wrote:
>> >> >On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
>> >> >> This looks odd. You have a single image yet you somehow divide it
>> >> >> into "program" and "config" areas. We already have infra in place to
>> >> >> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
>> >> >> You should have 2 components:
>> >> >> 1) "program"
>> >> >> 2) "config"
>> >> >>
>> >> >> Then it is up to the user what he decides to flash.
>> >> >
>> >> >99.9% of the time users want to flash "all". To achieve "don't flash
>> >> >config" with current infra users would have to flash each component
>> >>
>> >> Well you can have multiple component what would overlap:
>> >> 1) "program" + "config" (default)
>> >> 2) "program"
>> >> 3) "config"
>> >
>> >Say I have FW component and UNDI driver. Now I'll have 4 components?
>> >fw.prog, fw.config, undi.prog etc? Are those extra ones visible or just
>>
>> Visible in which sense? We don't show components anywhere if I'm not
>> mistaken. They are currently very rarely used. Basically we just ported
>> it from ethtool without much thinking.
>>
>
>Component names are used in devlink info and displayed to end users along with versions, plus they're names passed by the user in devlink flash update. As far as documented, we shouldn't add new components without associated versions in the info report.
Okay. So it is loosely coupled. I think it would be nice to tight those
2 togeter so it is not up to the driver how he decides to implement it.
>
>>
>> >"implied"? If they are visible what version does the config have?
>>
>> Good question. we don't have per-component version so far. I think it
>> would be good to have it alonside with the listing.
>>
>>
>> >
>> >Also (3) - flashing config from one firmware version and program from
>> >another - makes a very limited amount of sense to me.
>> >
>> >> >one by one and then omit the one(s) which is config (guessing which
>> >> >one that is based on the name).
>> >> >
>> >> >Wouldn't this be quite inconvenient?
>> >>
>> >> I see it as an extra knob that is actually somehow provides degradation
>> >> of components.
>> >
>> >Hm. We have the exact opposite view on the matter. To me components
>> >currently correspond to separate fw/hw entities, that's a very clear
>> >meaning. PHY firmware, management FW, UNDI. Now we would add a
>> >completely orthogonal meaning to the same API.
>>
>> I understand. My concern is, we would have a component with some
>> "subparts". Now it is some fuzzy vagely defined "config part",
>> in the future it might be something else. That is what I'm concerned
>> about. Components have clear api.
>>
>> So perhaps we can introduce something like "component mask", which would
>> allow to flash only part of the component. That is basically what Jacob
>> has, I would just like to have it well defined.
>>
>>
>
>So, we could make this selection a series of masked bits instead of a single enumeration value.
Yeah.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-26 7:16 ` Jiri Pirko
@ 2020-07-27 18:13 ` Jacob Keller
2020-07-28 11:19 ` Jiri Pirko
0 siblings, 1 reply; 36+ messages in thread
From: Jacob Keller @ 2020-07-27 18:13 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
On 7/26/2020 12:16 AM, Jiri Pirko wrote:
> Wed, Jul 22, 2020 at 05:30:05PM CEST, jacob.e.keller@intel.com wrote:
>>
>>
>>> -----Original Message-----
>>> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
>>> Visible in which sense? We don't show components anywhere if I'm not
>>> mistaken. They are currently very rarely used. Basically we just ported
>>> it from ethtool without much thinking.
>>>
>>
>> Component names are used in devlink info and displayed to end users along with versions, plus they're names passed by the user in devlink flash update. As far as documented, we shouldn't add new components without associated versions in the info report.
>
> Okay. So it is loosely coupled. I think it would be nice to tight those
> 2 togeter so it is not up to the driver how he decides to implement it.
>
I felt the coupling was quite clear from Jakub's recent documentation
improvements in the devlink-flash.rst doc file.
Are you thinking find some way to tie these two lists more closely in code?
Thanks,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-27 18:13 ` Jacob Keller
@ 2020-07-28 11:19 ` Jiri Pirko
2020-07-28 16:58 ` Jacob Keller
0 siblings, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2020-07-28 11:19 UTC (permalink / raw)
To: Jacob Keller
Cc: Jakub Kicinski, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
Mon, Jul 27, 2020 at 08:13:12PM CEST, jacob.e.keller@intel.com wrote:
>
>
>On 7/26/2020 12:16 AM, Jiri Pirko wrote:
>> Wed, Jul 22, 2020 at 05:30:05PM CEST, jacob.e.keller@intel.com wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
>>>> Visible in which sense? We don't show components anywhere if I'm not
>>>> mistaken. They are currently very rarely used. Basically we just ported
>>>> it from ethtool without much thinking.
>>>>
>>>
>>> Component names are used in devlink info and displayed to end users along with versions, plus they're names passed by the user in devlink flash update. As far as documented, we shouldn't add new components without associated versions in the info report.
>>
>> Okay. So it is loosely coupled. I think it would be nice to tight those
>> 2 togeter so it is not up to the driver how he decides to implement it.
>>
>I felt the coupling was quite clear from Jakub's recent documentation
>improvements in the devlink-flash.rst doc file.
>
>Are you thinking find some way to tie these two lists more closely in code?
Yes. Documentation is very easy to ignore unfortunatelly. The driver
developer has to be tight up by the core code and api, I believe.
>
>Thanks,
>Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-28 11:19 ` Jiri Pirko
@ 2020-07-28 16:58 ` Jacob Keller
2020-07-28 17:09 ` Jakub Kicinski
0 siblings, 1 reply; 36+ messages in thread
From: Jacob Keller @ 2020-07-28 16:58 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
On 7/28/2020 4:19 AM, Jiri Pirko wrote:
> Yes. Documentation is very easy to ignore unfortunatelly. The driver
> developer has to be tight up by the core code and api, I believe.
>
So I'm not sure what the best proposal here is. We do have a list of
generic components, but given that each piece of HW has different
elements, it's not always feasible to have fully generic names. Some of
the names are driver specific.
I guess we could use some system where components are "registered" when
loading the devlink, so that they can be verified by the stack when used
as a parameter for flash update? Perhaps take something like the
table-driven approach used for infos and extend that into devlink core
so that drivers basically register a table of the components which
includes both a function callback that gets the version string as well
as an indication of whether that component can be updated via flash_update?
I know it would also be useful for ice to have a sort of "pre-info"
callback that generates a context structure that is passed to each of
the info callbacks. (that way a single up-front step could be to lookup
the relevant information, and this is then forwarded to each of the
formatter functions for each component).
Am I on the right track here or just over-engineering?
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-28 16:58 ` Jacob Keller
@ 2020-07-28 17:09 ` Jakub Kicinski
2020-07-28 17:43 ` Jacob Keller
2020-07-28 22:59 ` Jacob Keller
0 siblings, 2 replies; 36+ messages in thread
From: Jakub Kicinski @ 2020-07-28 17:09 UTC (permalink / raw)
To: Jacob Keller
Cc: Jiri Pirko, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
On Tue, 28 Jul 2020 09:58:44 -0700 Jacob Keller wrote:
> On 7/28/2020 4:19 AM, Jiri Pirko wrote:
> > Yes. Documentation is very easy to ignore unfortunatelly. The driver
> > developer has to be tight up by the core code and api, I believe.
>
> So I'm not sure what the best proposal here is. We do have a list of
> generic components, but given that each piece of HW has different
> elements, it's not always feasible to have fully generic names. Some of
> the names are driver specific.
>
> I guess we could use some system where components are "registered" when
> loading the devlink, so that they can be verified by the stack when used
> as a parameter for flash update? Perhaps take something like the
> table-driven approach used for infos and extend that into devlink core
> so that drivers basically register a table of the components which
> includes both a function callback that gets the version string as well
> as an indication of whether that component can be updated via flash_update?
>
> I know it would also be useful for ice to have a sort of "pre-info"
> callback that generates a context structure that is passed to each of
> the info callbacks. (that way a single up-front step could be to lookup
> the relevant information, and this is then forwarded to each of the
> formatter functions for each component).
>
> Am I on the right track here or just over-engineering?
I don't understand why we're having this conversation.
No driver right now uses the component name.
AFAIU we agreed not to use the component name for config vs code.
So you may as well remove the component name from the devlink op and
add a todo there saying "when adding component back, make sure it's
tightly coupled to info".
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-28 17:09 ` Jakub Kicinski
@ 2020-07-28 17:43 ` Jacob Keller
2020-07-28 22:59 ` Jacob Keller
1 sibling, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-28 17:43 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jiri Pirko, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
On 7/28/2020 10:09 AM, Jakub Kicinski wrote:
> On Tue, 28 Jul 2020 09:58:44 -0700 Jacob Keller wrote:
>> On 7/28/2020 4:19 AM, Jiri Pirko wrote:
>>> Yes. Documentation is very easy to ignore unfortunatelly. The driver
>>> developer has to be tight up by the core code and api, I believe.
>>
>> So I'm not sure what the best proposal here is. We do have a list of
>> generic components, but given that each piece of HW has different
>> elements, it's not always feasible to have fully generic names. Some of
>> the names are driver specific.
>>
>> I guess we could use some system where components are "registered" when
>> loading the devlink, so that they can be verified by the stack when used
>> as a parameter for flash update? Perhaps take something like the
>> table-driven approach used for infos and extend that into devlink core
>> so that drivers basically register a table of the components which
>> includes both a function callback that gets the version string as well
>> as an indication of whether that component can be updated via flash_update?
>>
>> I know it would also be useful for ice to have a sort of "pre-info"
>> callback that generates a context structure that is passed to each of
>> the info callbacks. (that way a single up-front step could be to lookup
>> the relevant information, and this is then forwarded to each of the
>> formatter functions for each component).
>>
>> Am I on the right track here or just over-engineering?
>
> I don't understand why we're having this conversation.
>
> No driver right now uses the component name.
>
> AFAIU we agreed not to use the component name for config vs code.
>
> So you may as well remove the component name from the devlink op and
> add a todo there saying "when adding component back, make sure it's
> tightly coupled to info".
>
Fair enough yea.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
2020-07-28 17:09 ` Jakub Kicinski
2020-07-28 17:43 ` Jacob Keller
@ 2020-07-28 22:59 ` Jacob Keller
1 sibling, 0 replies; 36+ messages in thread
From: Jacob Keller @ 2020-07-28 22:59 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jiri Pirko, netdev, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
On 7/28/2020 10:09 AM, Jakub Kicinski wrote:
> On Tue, 28 Jul 2020 09:58:44 -0700 Jacob Keller wrote:
>> On 7/28/2020 4:19 AM, Jiri Pirko wrote:
>>> Yes. Documentation is very easy to ignore unfortunatelly. The driver
>>> developer has to be tight up by the core code and api, I believe.
>>
>> So I'm not sure what the best proposal here is. We do have a list of
>> generic components, but given that each piece of HW has different
>> elements, it's not always feasible to have fully generic names. Some of
>> the names are driver specific.
>>
>> I guess we could use some system where components are "registered" when
>> loading the devlink, so that they can be verified by the stack when used
>> as a parameter for flash update? Perhaps take something like the
>> table-driven approach used for infos and extend that into devlink core
>> so that drivers basically register a table of the components which
>> includes both a function callback that gets the version string as well
>> as an indication of whether that component can be updated via flash_update?
>>
>> I know it would also be useful for ice to have a sort of "pre-info"
>> callback that generates a context structure that is passed to each of
>> the info callbacks. (that way a single up-front step could be to lookup
>> the relevant information, and this is then forwarded to each of the
>> formatter functions for each component).
>>
>> Am I on the right track here or just over-engineering?
>
> I don't understand why we're having this conversation.
>
> No driver right now uses the component name.
>
> AFAIU we agreed not to use the component name for config vs code.
>
> So you may as well remove the component name from the devlink op and
> add a todo there saying "when adding component back, make sure it's
> tightly coupled to info".
>
Another simpler option would be to just call .info_get and verify that
the requested component appeared in that list. Ofcourse we'd be making
this call every flash_update...
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library
2020-07-17 18:35 [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jacob Keller
` (5 preceding siblings ...)
2020-07-17 18:35 ` [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update Jacob Keller
@ 2020-07-17 19:58 ` Jakub Kicinski
2020-07-17 21:00 ` Keller, Jacob E
2020-07-17 21:08 ` Keller, Jacob E
6 siblings, 2 replies; 36+ messages in thread
From: Jakub Kicinski @ 2020-07-17 19:58 UTC (permalink / raw)
To: Jacob Keller
Cc: netdev, Jiri Pirko, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
On Fri, 17 Jul 2020 11:35:35 -0700 Jacob Keller wrote:
> This series goal is to enable support for updating the ice hardware flash
> using the devlink flash command.
Looks reasonable.
You have some left over references to ignore_pending_flash_update in
comments, and you should use NLA_POLICY_RANGE() for the new attr.
Taking and releasing the FW lock may be fun for multi-host devices if
you ever support those.
^ permalink raw reply [flat|nested] 36+ messages in thread* RE: [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library
2020-07-17 19:58 ` [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jakub Kicinski
@ 2020-07-17 21:00 ` Keller, Jacob E
2020-07-17 21:08 ` Keller, Jacob E
1 sibling, 0 replies; 36+ messages in thread
From: Keller, Jacob E @ 2020-07-17 21:00 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, Jiri Pirko, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Jakub Kicinski
> Sent: Friday, July 17, 2020 12:58 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Jiri Pirko <jiri@resnulli.us>; Tom Herbert
> <tom@herbertland.com>; Jiri Pirko <jiri@mellanox.com>; Jakub Kicinski
> <kuba@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Michael Chan
> <michael.chan@broadcom.com>; Bin Luo <luobin9@huawei.com>; Saeed
> Mahameed <saeedm@mellanox.com>; Leon Romanovsky <leon@kernel.org>;
> Ido Schimmel <idosch@mellanox.com>; Danielle Ratson
> <danieller@mellanox.com>
> Subject: Re: [RFC PATCH net-next v2 0/6] introduce PLDM firmware update
> library
>
> On Fri, 17 Jul 2020 11:35:35 -0700 Jacob Keller wrote:
> > This series goal is to enable support for updating the ice hardware flash
> > using the devlink flash command.
>
> Looks reasonable.
>
> You have some left over references to ignore_pending_flash_update in
> comments, and you should use NLA_POLICY_RANGE() for the new attr.
>
Ah, good point I'll make sure to fix those up, and switch the NLA_POLICY_RANGE.
> Taking and releasing the FW lock may be fun for multi-host devices if
> you ever support those.
Yea. The lib/pldm stuff assumes the driver will manage the locking. I'm not sure how the resource locks work in a multi-host environment at all..
Thanks,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library
2020-07-17 19:58 ` [RFC PATCH net-next v2 0/6] introduce PLDM firmware update library Jakub Kicinski
2020-07-17 21:00 ` Keller, Jacob E
@ 2020-07-17 21:08 ` Keller, Jacob E
1 sibling, 0 replies; 36+ messages in thread
From: Keller, Jacob E @ 2020-07-17 21:08 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, Jiri Pirko, Tom Herbert, Jiri Pirko, Jakub Kicinski,
Jonathan Corbet, Michael Chan, Bin Luo, Saeed Mahameed,
Leon Romanovsky, Ido Schimmel, Danielle Ratson
> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Jakub Kicinski
> Sent: Friday, July 17, 2020 12:58 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Jiri Pirko <jiri@resnulli.us>; Tom Herbert
> <tom@herbertland.com>; Jiri Pirko <jiri@mellanox.com>; Jakub Kicinski
> <kuba@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Michael Chan
> <michael.chan@broadcom.com>; Bin Luo <luobin9@huawei.com>; Saeed
> Mahameed <saeedm@mellanox.com>; Leon Romanovsky <leon@kernel.org>;
> Ido Schimmel <idosch@mellanox.com>; Danielle Ratson
> <danieller@mellanox.com>
> Subject: Re: [RFC PATCH net-next v2 0/6] introduce PLDM firmware update
> library
>
> On Fri, 17 Jul 2020 11:35:35 -0700 Jacob Keller wrote:
> > This series goal is to enable support for updating the ice hardware flash
> > using the devlink flash command.
>
> Looks reasonable.
>
> You have some left over references to ignore_pending_flash_update in
> comments, and you should use NLA_POLICY_RANGE() for the new attr.
>
Since the minimum value is zero, I switched the code to use NLA_POLICY_MAX.
Thanks,
Jake
^ permalink raw reply [flat|nested] 36+ messages in thread