[PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Vasundhara Volam]

Live device reset capability allows the users to reset the device in real
time. For example, after flashing a new firmware image, this feature allows
a user to initiate the reset immediately from a separate command, to load
the new firmware without reloading the driver or resetting the system.

When device reset is initiated, services running on the host interfaces
will momentarily pause and resume once reset is completed, which is very
similar to momentary network outage.

This patchset adds support for two new generic devlink parameters for
controlling the live device reset capability and use it in the bnxt_en
driver.

Users can initiate the reset from a separate command, for example,
'ethtool --reset ethX all' or 'devlink dev reload' to reset the
device.
Where ethX or dev is any PF with administrative privileges.

Patchset also updates firmware spec. to 1.10.1.40.


v2->v3: Split the param into two new params "enable_live_dev_reset" and
"allow_live_dev_reset".
- Expand the documentation of each param and update commit messages
 accordingly.
- Separated the permanent configuration mode code to another patch and
rename the callbacks of the "allow_live_dev_reset" parameter accordingly.

v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
- Update documentation files and commit messages with more details of the
 feature.

Vasundhara Volam (6):
  devlink: Add 'enable_live_dev_reset' generic parameter.
  devlink: Add 'allow_live_dev_reset' generic parameter.
  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
  bnxt_en: Update firmware spec. to 1.10.1.40.
  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET

 Documentation/networking/devlink/bnxt.rst          |  4 ++
 .../networking/devlink/devlink-params.rst          | 28 ++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
 include/net/devlink.h                              |  8 +++
 net/core/devlink.c                                 | 10 ++++
 10 files changed, 175 insertions(+), 36 deletions(-)

-- 
1.8.3.1

[PATCH v3 net-next 1/6] devlink: Add 'enable_live_dev_reset' generic parameter.

[Vasundhara Volam]

This parameter controls the device's live reset capability. When
enabled, a user can issue a separate command like 'ethtool --reset'
or 'devlink dev reload' to reset the entire device in real time.

This parameter can be useful, if the user wants to upgrade the
firmware quickly with a momentary network outage.

For example, if a user flashes a new firmware image and if this
parameter is enabled, the user can immediately initiate reset of
the device, to load the new firmware without reloading the driver
manually or resetting the system.

Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/devlink/devlink-params.rst | 8 ++++++++
 include/net/devlink.h                               | 4 ++++
 net/core/devlink.c                                  | 5 +++++
 3 files changed, 17 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
index d075fd0..8e12c83 100644
--- a/Documentation/networking/devlink/devlink-params.rst
+++ b/Documentation/networking/devlink/devlink-params.rst
@@ -108,3 +108,11 @@ own name.
    * - ``region_snapshot_enable``
      - Boolean
      - Enable capture of ``devlink-region`` snapshots.
+   * - ``enable_live_dev_reset``
+     - Boolean
+     - Controls the device's live reset capability. When the parameter is true,
+       the user can use a separate command to reset the entire device in real
+       time, that resets the firmware and driver entities.
+       For example, after flashing a new firmware image, this feature allows the
+       user to initiate the reset immediately from a separate command, to load
+       the new firmware without reloading the driver or resetting the system.
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 8ffc1b5c..eb28fa1 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -406,6 +406,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY,
 	DEVLINK_PARAM_GENERIC_ID_RESET_DEV_ON_DRV_PROBE,
 	DEVLINK_PARAM_GENERIC_ID_ENABLE_ROCE,
+	DEVLINK_PARAM_GENERIC_ID_ENABLE_LIVE_DEV_RESET,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -443,6 +444,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_ENABLE_ROCE_NAME "enable_roce"
 #define DEVLINK_PARAM_GENERIC_ENABLE_ROCE_TYPE DEVLINK_PARAM_TYPE_BOOL
 
+#define DEVLINK_PARAM_GENERIC_ENABLE_LIVE_DEV_RESET_NAME "enable_live_dev_reset"
+#define DEVLINK_PARAM_GENERIC_ENABLE_LIVE_DEV_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7b76e5f..7b52b38 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3011,6 +3011,11 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 		.name = DEVLINK_PARAM_GENERIC_ENABLE_ROCE_NAME,
 		.type = DEVLINK_PARAM_GENERIC_ENABLE_ROCE_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_ENABLE_LIVE_DEV_RESET,
+		.name = DEVLINK_PARAM_GENERIC_ENABLE_LIVE_DEV_RESET_NAME,
+		.type = DEVLINK_PARAM_GENERIC_ENABLE_LIVE_DEV_RESET_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
1.8.3.1

[PATCH v3 net-next 2/6] devlink: Add 'allow_live_dev_reset' generic parameter.

[Vasundhara Volam]

This parameter is supported only when 'enable_live_dev_reset' is
true. The purpose of this parameter is to allow the user on a
host to temporarily disable the live reset feature of the device.

For example, if a host is running a mission critical application,
a user from the host can set this parameter to false, to avoid
a potential live reset from disrupting it.

Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/devlink/devlink-params.rst | 20 ++++++++++++++++++++
 include/net/devlink.h                               |  4 ++++
 net/core/devlink.c                                  |  5 +++++
 3 files changed, 29 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
index 8e12c83..450fe18 100644
--- a/Documentation/networking/devlink/devlink-params.rst
+++ b/Documentation/networking/devlink/devlink-params.rst
@@ -116,3 +116,23 @@ own name.
        For example, after flashing a new firmware image, this feature allows the
        user to initiate the reset immediately from a separate command, to load
        the new firmware without reloading the driver or resetting the system.
+
+       A user can set the 'allow_live_dev_reset' parameter to false to
+       momentarily disable the live reset capability.
+   * - ``allow_live_dev_reset``
+     - Boolean
+     - This parameter is supported only when 'enable_live_dev_reset' is true.
+       The purpose of this parameter is to allow the user on a host to
+       temporarily disable the live reset feature of the device. When this
+       parameter is set to true from all the hosts in a multi-host environment
+       for example, a user from any host can initiate live device reset from any
+       of the host drivers.
+
+       For the parameter to be true, all the loaded host drivers must support
+       the live reset and the parameter must be set to true for all the host
+       drivers. For example, if any of the host (in case of multi-host NIC) is
+       loaded with an old driver which is not aware of the feature, then the
+       value of the parameter will be false until the old driver is upgraded
+       or unloaded. Also if the user has set the parameter to false on one of
+       the host (say A), the parameter will be false for all the hosts until the
+       user sets the parameter to true in the host (A).
diff --git a/include/net/devlink.h b/include/net/devlink.h
index eb28fa1..d922033 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -407,6 +407,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_RESET_DEV_ON_DRV_PROBE,
 	DEVLINK_PARAM_GENERIC_ID_ENABLE_ROCE,
 	DEVLINK_PARAM_GENERIC_ID_ENABLE_LIVE_DEV_RESET,
+	DEVLINK_PARAM_GENERIC_ID_ALLOW_LIVE_DEV_RESET,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -447,6 +448,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_ENABLE_LIVE_DEV_RESET_NAME "enable_live_dev_reset"
 #define DEVLINK_PARAM_GENERIC_ENABLE_LIVE_DEV_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
 
+#define DEVLINK_PARAM_GENERIC_ALLOW_LIVE_DEV_RESET_NAME "allow_live_dev_reset"
+#define DEVLINK_PARAM_GENERIC_ALLOW_LIVE_DEV_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7b52b38..e36f6c4 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3016,6 +3016,11 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 		.name = DEVLINK_PARAM_GENERIC_ENABLE_LIVE_DEV_RESET_NAME,
 		.type = DEVLINK_PARAM_GENERIC_ENABLE_LIVE_DEV_RESET_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_ALLOW_LIVE_DEV_RESET,
+		.name = DEVLINK_PARAM_GENERIC_ALLOW_LIVE_DEV_RESET_NAME,
+		.type = DEVLINK_PARAM_GENERIC_ALLOW_LIVE_DEV_RESET_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
1.8.3.1

[PATCH v3 net-next 3/6] bnxt_en: Use 'enable_live_dev_reset' devlink parameter.

[Vasundhara Volam]

When enabled, device will enable the live reset capability in
NVRAM configuration.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/devlink/bnxt.rst         | 2 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 6 ++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 1 +
 3 files changed, 9 insertions(+)

diff --git a/Documentation/networking/devlink/bnxt.rst b/Documentation/networking/devlink/bnxt.rst
index 3dfd84c..ae0a69d 100644
--- a/Documentation/networking/devlink/bnxt.rst
+++ b/Documentation/networking/devlink/bnxt.rst
@@ -22,6 +22,8 @@ Parameters
      - Permanent
    * - ``msix_vec_per_pf_min``
      - Permanent
+   * - ``enable_live_dev_reset``
+     - Permanent
 
 The ``bnxt`` driver also implements the following driver-specific
 parameters.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index a812beb..3e1a4ef 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -314,6 +314,8 @@ enum bnxt_dl_param_id {
 	 NVM_OFF_MSIX_VEC_PER_PF_MIN, BNXT_NVM_SHARED_CFG, 7, 4},
 	{BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, NVM_OFF_DIS_GRE_VER_CHECK,
 	 BNXT_NVM_SHARED_CFG, 1, 1},
+	{DEVLINK_PARAM_GENERIC_ID_ENABLE_LIVE_DEV_RESET,
+	 NVM_OFF_FW_LIVE_RESET, BNXT_NVM_SHARED_CFG, 1, 1},
 };
 
 union bnxt_nvm_data {
@@ -640,6 +642,10 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
 			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
 			     bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
 			     NULL),
+	DEVLINK_PARAM_GENERIC(ENABLE_LIVE_DEV_RESET,
+			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			      bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
+			      NULL),
 };
 
 static const struct devlink_param bnxt_dl_port_params[] = {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index d5c8bd4..0c786fb 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -39,6 +39,7 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
 #define NVM_OFF_DIS_GRE_VER_CHECK	171
 #define NVM_OFF_ENABLE_SRIOV		401
 #define NVM_OFF_NVM_CFG_VER		602
+#define NVM_OFF_FW_LIVE_RESET		917
 
 #define BNXT_NVM_CFG_VER_BITS		24
 #define BNXT_NVM_CFG_VER_BYTES		4
-- 
1.8.3.1

[PATCH v3 net-next 4/6] bnxt_en: Update firmware spec. to 1.10.1.40.

[Vasundhara Volam]

Major changes are to add additional flags to configure hot firmware
reset.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h | 64 ++++++++++++++++-----------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
index 7e9235c..0a6e60e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
@@ -367,6 +367,8 @@ struct cmd_nums {
 	#define HWRM_TF_EXT_EM_OP                         0x2ddUL
 	#define HWRM_TF_EXT_EM_CFG                        0x2deUL
 	#define HWRM_TF_EXT_EM_QCFG                       0x2dfUL
+	#define HWRM_TF_EM_INSERT                         0x2e0UL
+	#define HWRM_TF_EM_DELETE                         0x2e1UL
 	#define HWRM_TF_TCAM_SET                          0x2eeUL
 	#define HWRM_TF_TCAM_GET                          0x2efUL
 	#define HWRM_TF_TCAM_MOVE                         0x2f0UL
@@ -391,6 +393,7 @@ struct cmd_nums {
 	#define HWRM_DBG_QCAPS                            0xff20UL
 	#define HWRM_DBG_QCFG                             0xff21UL
 	#define HWRM_DBG_CRASHDUMP_MEDIUM_CFG             0xff22UL
+	#define HWRM_NVM_REQ_ARBITRATION                  0xffedUL
 	#define HWRM_NVM_FACTORY_DEFAULTS                 0xffeeUL
 	#define HWRM_NVM_VALIDATE_OPTION                  0xffefUL
 	#define HWRM_NVM_FLUSH                            0xfff0UL
@@ -464,8 +467,8 @@ struct hwrm_err_output {
 #define HWRM_VERSION_MAJOR 1
 #define HWRM_VERSION_MINOR 10
 #define HWRM_VERSION_UPDATE 1
-#define HWRM_VERSION_RSVD 33
-#define HWRM_VERSION_STR "1.10.1.33"
+#define HWRM_VERSION_RSVD 40
+#define HWRM_VERSION_STR "1.10.1.40"
 
 /* hwrm_ver_get_input (size:192b/24B) */
 struct hwrm_ver_get_input {
@@ -1192,6 +1195,7 @@ struct hwrm_func_qcaps_output {
 	#define FUNC_QCAPS_RESP_FLAGS_EXT_ECN_MARK_SUPPORTED         0x1UL
 	#define FUNC_QCAPS_RESP_FLAGS_EXT_ECN_STATS_SUPPORTED        0x2UL
 	#define FUNC_QCAPS_RESP_FLAGS_EXT_EXT_HW_STATS_SUPPORTED     0x4UL
+	#define FUNC_QCAPS_RESP_FLAGS_EXT_HOT_RESET_IF_SUPPORT       0x8UL
 	u8	unused_1[3];
 	u8	valid;
 };
@@ -1226,6 +1230,7 @@ struct hwrm_func_qcfg_output {
 	#define FUNC_QCFG_RESP_FLAGS_TRUSTED_VF                   0x40UL
 	#define FUNC_QCFG_RESP_FLAGS_SECURE_MODE_ENABLED          0x80UL
 	#define FUNC_QCFG_RESP_FLAGS_PREBOOT_LEGACY_L2_RINGS      0x100UL
+	#define FUNC_QCFG_RESP_FLAGS_HOT_RESET_ALLOWED            0x200UL
 	u8	mac_address[6];
 	__le16	pci_id;
 	__le16	alloc_rsscos_ctx;
@@ -1352,30 +1357,32 @@ struct hwrm_func_cfg_input {
 	#define FUNC_CFG_REQ_FLAGS_NQ_ASSETS_TEST                 0x800000UL
 	#define FUNC_CFG_REQ_FLAGS_TRUSTED_VF_DISABLE             0x1000000UL
 	#define FUNC_CFG_REQ_FLAGS_PREBOOT_LEGACY_L2_RINGS        0x2000000UL
+	#define FUNC_CFG_REQ_FLAGS_HOT_RESET_IF_EN_DIS            0x4000000UL
 	__le32	enables;
-	#define FUNC_CFG_REQ_ENABLES_MTU                     0x1UL
-	#define FUNC_CFG_REQ_ENABLES_MRU                     0x2UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_RSSCOS_CTXS         0x4UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_CMPL_RINGS          0x8UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_TX_RINGS            0x10UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_RX_RINGS            0x20UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_L2_CTXS             0x40UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_VNICS               0x80UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_STAT_CTXS           0x100UL
-	#define FUNC_CFG_REQ_ENABLES_DFLT_MAC_ADDR           0x200UL
-	#define FUNC_CFG_REQ_ENABLES_DFLT_VLAN               0x400UL
-	#define FUNC_CFG_REQ_ENABLES_DFLT_IP_ADDR            0x800UL
-	#define FUNC_CFG_REQ_ENABLES_MIN_BW                  0x1000UL
-	#define FUNC_CFG_REQ_ENABLES_MAX_BW                  0x2000UL
-	#define FUNC_CFG_REQ_ENABLES_ASYNC_EVENT_CR          0x4000UL
-	#define FUNC_CFG_REQ_ENABLES_VLAN_ANTISPOOF_MODE     0x8000UL
-	#define FUNC_CFG_REQ_ENABLES_ALLOWED_VLAN_PRIS       0x10000UL
-	#define FUNC_CFG_REQ_ENABLES_EVB_MODE                0x20000UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_MCAST_FILTERS       0x40000UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_HW_RING_GRPS        0x80000UL
-	#define FUNC_CFG_REQ_ENABLES_CACHE_LINESIZE          0x100000UL
-	#define FUNC_CFG_REQ_ENABLES_NUM_MSIX                0x200000UL
-	#define FUNC_CFG_REQ_ENABLES_ADMIN_LINK_STATE        0x400000UL
+	#define FUNC_CFG_REQ_ENABLES_MTU                      0x1UL
+	#define FUNC_CFG_REQ_ENABLES_MRU                      0x2UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_RSSCOS_CTXS          0x4UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_CMPL_RINGS           0x8UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_TX_RINGS             0x10UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_RX_RINGS             0x20UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_L2_CTXS              0x40UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_VNICS                0x80UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_STAT_CTXS            0x100UL
+	#define FUNC_CFG_REQ_ENABLES_DFLT_MAC_ADDR            0x200UL
+	#define FUNC_CFG_REQ_ENABLES_DFLT_VLAN                0x400UL
+	#define FUNC_CFG_REQ_ENABLES_DFLT_IP_ADDR             0x800UL
+	#define FUNC_CFG_REQ_ENABLES_MIN_BW                   0x1000UL
+	#define FUNC_CFG_REQ_ENABLES_MAX_BW                   0x2000UL
+	#define FUNC_CFG_REQ_ENABLES_ASYNC_EVENT_CR           0x4000UL
+	#define FUNC_CFG_REQ_ENABLES_VLAN_ANTISPOOF_MODE      0x8000UL
+	#define FUNC_CFG_REQ_ENABLES_ALLOWED_VLAN_PRIS        0x10000UL
+	#define FUNC_CFG_REQ_ENABLES_EVB_MODE                 0x20000UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_MCAST_FILTERS        0x40000UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_HW_RING_GRPS         0x80000UL
+	#define FUNC_CFG_REQ_ENABLES_CACHE_LINESIZE           0x100000UL
+	#define FUNC_CFG_REQ_ENABLES_NUM_MSIX                 0x200000UL
+	#define FUNC_CFG_REQ_ENABLES_ADMIN_LINK_STATE         0x400000UL
+	#define FUNC_CFG_REQ_ENABLES_HOT_RESET_IF_SUPPORT     0x800000UL
 	__le16	mtu;
 	__le16	mru;
 	__le16	num_rsscos_ctxs;
@@ -7620,7 +7627,8 @@ struct hwrm_dbg_ring_info_get_input {
 	#define DBG_RING_INFO_GET_REQ_RING_TYPE_L2_CMPL 0x0UL
 	#define DBG_RING_INFO_GET_REQ_RING_TYPE_TX      0x1UL
 	#define DBG_RING_INFO_GET_REQ_RING_TYPE_RX      0x2UL
-	#define DBG_RING_INFO_GET_REQ_RING_TYPE_LAST   DBG_RING_INFO_GET_REQ_RING_TYPE_RX
+	#define DBG_RING_INFO_GET_REQ_RING_TYPE_NQ      0x3UL
+	#define DBG_RING_INFO_GET_REQ_RING_TYPE_LAST   DBG_RING_INFO_GET_REQ_RING_TYPE_NQ
 	u8	unused_0[3];
 	__le32	fw_ring_id;
 };
@@ -7633,7 +7641,8 @@ struct hwrm_dbg_ring_info_get_output {
 	__le16	resp_len;
 	__le32	producer_index;
 	__le32	consumer_index;
-	u8	unused_0[7];
+	__le32	cag_vector_ctrl;
+	u8	unused_0[3];
 	u8	valid;
 };
 
@@ -7922,6 +7931,7 @@ struct hwrm_nvm_install_update_input {
 	#define NVM_INSTALL_UPDATE_REQ_FLAGS_ERASE_UNUSED_SPACE     0x1UL
 	#define NVM_INSTALL_UPDATE_REQ_FLAGS_REMOVE_UNUSED_PKG      0x2UL
 	#define NVM_INSTALL_UPDATE_REQ_FLAGS_ALLOWED_TO_DEFRAG      0x4UL
+	#define NVM_INSTALL_UPDATE_REQ_FLAGS_VERIFY_ONLY            0x8UL
 	u8	unused_0[2];
 };
 
-- 
1.8.3.1

[PATCH v3 net-next 5/6] bnxt_en: Use 'allow_live_dev_reset' devlink parameter.

[Vasundhara Volam]

This parameter allows the user to prevent live reset of the device
temporarily by setting it to false.
For example, if the host is running a mission critical application,
user can set this parameter to false until the application has
completed, to avoid a potential device reset disrupting it.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
v2->v3: Rename parameter name to "allow_live_dev_reset".
- Moved permanent configuration mode code to a separate patch as the
param is renamed to "enable_live_dev_reset".
- Rename the get/set callbacks of the param accordingly.
- Moved the documentation completely to devlink-params.rst file.
---
 Documentation/networking/devlink/bnxt.rst         |  2 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 28 ++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h         |  2 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 43 +++++++++++++++++++++++
 4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/devlink/bnxt.rst b/Documentation/networking/devlink/bnxt.rst
index ae0a69d..5573663 100644
--- a/Documentation/networking/devlink/bnxt.rst
+++ b/Documentation/networking/devlink/bnxt.rst
@@ -24,6 +24,8 @@ Parameters
      - Permanent
    * - ``enable_live_dev_reset``
      - Permanent
+   * - ``allow_live_dev_reset``
+     - Runtime
 
 The ``bnxt`` driver also implements the following driver-specific
 parameters.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index f86b621..535fe8f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -6955,7 +6955,7 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 	struct hwrm_func_qcaps_input req = {0};
 	struct hwrm_func_qcaps_output *resp = bp->hwrm_cmd_resp_addr;
 	struct bnxt_hw_resc *hw_resc = &bp->hw_resc;
-	u32 flags;
+	u32 flags, flags_ext;
 
 	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_FUNC_QCAPS, -1, -1);
 	req.fid = cpu_to_le16(0xffff);
@@ -6985,6 +6985,10 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 	if (flags & FUNC_QCAPS_RESP_FLAGS_PUSH_MODE_SUPPORTED)
 		bp->tx_push_thresh = BNXT_TX_PUSH_THRESH;
 
+	flags_ext = le32_to_cpu(resp->flags_ext);
+	if (flags_ext & FUNC_QCAPS_RESP_FLAGS_EXT_HOT_RESET_IF_SUPPORT)
+		bp->fw_cap |= BNXT_FW_CAP_HOT_RESET_IF_SUPPORT;
+
 	hw_resc->max_rsscos_ctxs = le16_to_cpu(resp->max_rsscos_ctx);
 	hw_resc->max_cp_rings = le16_to_cpu(resp->max_cmpl_rings);
 	hw_resc->max_tx_rings = le16_to_cpu(resp->max_tx_rings);
@@ -8773,6 +8777,28 @@ static int bnxt_hwrm_shutdown_link(struct bnxt *bp)
 
 static int bnxt_fw_init_one(struct bnxt *bp);
 
+int bnxt_hwrm_get_hot_reset(struct bnxt *bp, bool *hot_reset_allowed)
+{
+	struct hwrm_func_qcfg_output *resp = bp->hwrm_cmd_resp_addr;
+	struct hwrm_func_qcfg_input req = {0};
+	int rc;
+
+	if (!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET_IF_SUPPORT)) {
+		*hot_reset_allowed = !!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET);
+		return 0;
+	}
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_FUNC_QCFG, -1, -1);
+	req.fid = cpu_to_le16(0xffff);
+	mutex_lock(&bp->hwrm_cmd_lock);
+	rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+	if (!rc)
+		*hot_reset_allowed = !!(le16_to_cpu(resp->flags) &
+					FUNC_QCFG_RESP_FLAGS_HOT_RESET_ALLOWED);
+	mutex_unlock(&bp->hwrm_cmd_lock);
+	return rc;
+}
+
 static int bnxt_hwrm_if_change(struct bnxt *bp, bool up)
 {
 	struct hwrm_func_drv_if_change_output *resp = bp->hwrm_cmd_resp_addr;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index c04ac4a..fd6592e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1710,6 +1710,7 @@ struct bnxt {
 	#define BNXT_FW_CAP_ERR_RECOVER_RELOAD		0x00100000
 	#define BNXT_FW_CAP_HOT_RESET			0x00200000
 	#define BNXT_FW_CAP_SHARED_PORT_CFG		0x00400000
+	#define BNXT_FW_CAP_HOT_RESET_IF_SUPPORT	0x08000000
 
 #define BNXT_NEW_RM(bp)		((bp)->fw_cap & BNXT_FW_CAP_NEW_RM)
 	u32			hwrm_spec_code;
@@ -2062,5 +2063,6 @@ int bnxt_get_port_parent_id(struct net_device *dev,
 			    struct netdev_phys_item_id *ppid);
 void bnxt_dim_work(struct work_struct *work);
 int bnxt_hwrm_set_ring_coal(struct bnxt *bp, struct bnxt_napi *bnapi);
+int bnxt_hwrm_get_hot_reset(struct bnxt *bp, bool *hot_reset_allowed);
 
 #endif
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 3e1a4ef..da31351 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -620,6 +620,45 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
 	return 0;
 }
 
+static int bnxt_live_dev_reset_get(struct devlink *dl, u32 id,
+				   struct devlink_param_gset_ctx *ctx)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+
+	return bnxt_hwrm_get_hot_reset(bp, &ctx->val.vbool);
+}
+
+static int bnxt_live_dev_reset_set(struct devlink *dl, u32 id,
+				   struct devlink_param_gset_ctx *ctx)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+	struct hwrm_func_cfg_input req = {0};
+	bool hot_reset_allowed;
+	int rc;
+
+	if (!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET_IF_SUPPORT))
+		return -EOPNOTSUPP;
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_FUNC_CFG, -1, -1);
+	req.fid = cpu_to_le16(0xffff);
+	req.enables = cpu_to_le32(FUNC_CFG_REQ_ENABLES_HOT_RESET_IF_SUPPORT);
+	if (ctx->val.vbool)
+		req.flags = cpu_to_le32(FUNC_CFG_REQ_FLAGS_HOT_RESET_IF_EN_DIS);
+
+	rc = hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+	if (rc)
+		return rc;
+
+	rc = bnxt_hwrm_get_hot_reset(bp, &hot_reset_allowed);
+	if (rc)
+		return rc;
+
+	if (ctx->val.vbool && !hot_reset_allowed)
+		netdev_info(bp->dev, "Live reset enabled, but is disallowed as it is disabled on other interface(s) of this device.\n");
+
+	return 0;
+}
+
 static const struct devlink_param bnxt_dl_params[] = {
 	DEVLINK_PARAM_GENERIC(ENABLE_SRIOV,
 			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
@@ -646,6 +685,10 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
 			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
 			      bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
 			      NULL),
+	DEVLINK_PARAM_GENERIC(ALLOW_LIVE_DEV_RESET,
+			      BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			      bnxt_live_dev_reset_get, bnxt_live_dev_reset_set,
+			      NULL),
 };
 
 static const struct devlink_param bnxt_dl_port_params[] = {
-- 
1.8.3.1

[PATCH v3 net-next 6/6] bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET

[Vasundhara Volam]

If device does not allow fw_live_reset, issue FW_RESET command
without graceful flag, which requires a driver reload to reset
the firmware.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index dd0c3f2..e5eb8d2 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1888,12 +1888,11 @@ static int bnxt_firmware_reset(struct net_device *dev,
 	return bnxt_hwrm_firmware_reset(dev, proc_type, self_reset, flags);
 }
 
-static int bnxt_firmware_reset_chip(struct net_device *dev)
+static int bnxt_firmware_reset_chip(struct net_device *dev, bool hot_reset)
 {
-	struct bnxt *bp = netdev_priv(dev);
 	u8 flags = 0;
 
-	if (bp->fw_cap & BNXT_FW_CAP_HOT_RESET)
+	if (hot_reset)
 		flags = FW_RESET_REQ_FLAGS_RESET_GRACEFUL;
 
 	return bnxt_hwrm_firmware_reset(dev,
@@ -3082,7 +3081,7 @@ static void bnxt_self_test(struct net_device *dev, struct ethtool_test *etest,
 static int bnxt_reset(struct net_device *dev, u32 *flags)
 {
 	struct bnxt *bp = netdev_priv(dev);
-	bool reload = false;
+	bool reload = false, hot_reset;
 	u32 req = *flags;
 
 	if (!req)
@@ -3093,8 +3092,10 @@ static int bnxt_reset(struct net_device *dev, u32 *flags)
 		return -EOPNOTSUPP;
 	}
 
-	if (pci_vfs_assigned(bp->pdev) &&
-	    !(bp->fw_cap & BNXT_FW_CAP_HOT_RESET)) {
+	if (bnxt_hwrm_get_hot_reset(bp, &hot_reset))
+		hot_reset = !!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET);
+
+	if (pci_vfs_assigned(bp->pdev) && !hot_reset) {
 		netdev_err(dev,
 			   "Reset not allowed when VFs are assigned to VMs\n");
 		return -EBUSY;
@@ -3103,9 +3104,9 @@ static int bnxt_reset(struct net_device *dev, u32 *flags)
 	if ((req & BNXT_FW_RESET_CHIP) == BNXT_FW_RESET_CHIP) {
 		/* This feature is not supported in older firmware versions */
 		if (bp->hwrm_spec_code >= 0x10803) {
-			if (!bnxt_firmware_reset_chip(dev)) {
+			if (!bnxt_firmware_reset_chip(dev, hot_reset)) {
 				netdev_info(dev, "Firmware reset request successful.\n");
-				if (!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET))
+				if (!hot_reset)
 					reload = true;
 				*flags &= ~BNXT_FW_RESET_CHIP;
 			}
-- 
1.8.3.1

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Jiri Pirko]

Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
>Live device reset capability allows the users to reset the device in real
>time. For example, after flashing a new firmware image, this feature allows
>a user to initiate the reset immediately from a separate command, to load
>the new firmware without reloading the driver or resetting the system.
>
>When device reset is initiated, services running on the host interfaces
>will momentarily pause and resume once reset is completed, which is very
>similar to momentary network outage.
>
>This patchset adds support for two new generic devlink parameters for
>controlling the live device reset capability and use it in the bnxt_en
>driver.
>
>Users can initiate the reset from a separate command, for example,
>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
>device.
>Where ethX or dev is any PF with administrative privileges.
>
>Patchset also updates firmware spec. to 1.10.1.40.
>
>
>v2->v3: Split the param into two new params "enable_live_dev_reset" and

Vasundhara, I asked you multiple times for this to be "devlink dev reload"
attribute. I don't recall you telling any argument against it. I belive
that this should not be paramater. This is very tightly related to
reload, could you please have it as an attribute of reload, as I
suggested?

Thanks!


>"allow_live_dev_reset".
>- Expand the documentation of each param and update commit messages
> accordingly.
>- Separated the permanent configuration mode code to another patch and
>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
>
>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
>- Update documentation files and commit messages with more details of the
> feature.
>
>Vasundhara Volam (6):
>  devlink: Add 'enable_live_dev_reset' generic parameter.
>  devlink: Add 'allow_live_dev_reset' generic parameter.
>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
>  bnxt_en: Update firmware spec. to 1.10.1.40.
>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
>
> Documentation/networking/devlink/bnxt.rst          |  4 ++
> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> include/net/devlink.h                              |  8 +++
> net/core/devlink.c                                 | 10 ++++
> 10 files changed, 175 insertions(+), 36 deletions(-)
>
>-- 
>1.8.3.1
>

Re: [PATCH v3 net-next 1/6] devlink: Add 'enable_live_dev_reset' generic parameter.

[Jiri Pirko]

Sun, May 31, 2020 at 09:03:40AM CEST, vasundhara-v.volam@broadcom.com wrote:
>This parameter controls the device's live reset capability. When
>enabled, a user can issue a separate command like 'ethtool --reset'
>or 'devlink dev reload' to reset the entire device in real time.
>
>This parameter can be useful, if the user wants to upgrade the
>firmware quickly with a momentary network outage.
>
>For example, if a user flashes a new firmware image and if this
>parameter is enabled, the user can immediately initiate reset of
>the device, to load the new firmware without reloading the driver
>manually or resetting the system.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Cc: Jakub Kicinski <kuba@kernel.org>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>Signed-off-by: Michael Chan <michael.chan@broadcom.com>


NAck, please see the comment I wrote as a reply to the cover letter.
Thanks.

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Jiri Pirko]

Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
>Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
>>Live device reset capability allows the users to reset the device in real
>>time. For example, after flashing a new firmware image, this feature allows
>>a user to initiate the reset immediately from a separate command, to load
>>the new firmware without reloading the driver or resetting the system.
>>
>>When device reset is initiated, services running on the host interfaces
>>will momentarily pause and resume once reset is completed, which is very
>>similar to momentary network outage.
>>
>>This patchset adds support for two new generic devlink parameters for
>>controlling the live device reset capability and use it in the bnxt_en
>>driver.
>>
>>Users can initiate the reset from a separate command, for example,
>>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
>>device.
>>Where ethX or dev is any PF with administrative privileges.
>>
>>Patchset also updates firmware spec. to 1.10.1.40.
>>
>>
>>v2->v3: Split the param into two new params "enable_live_dev_reset" and
>
>Vasundhara, I asked you multiple times for this to be "devlink dev reload"
>attribute. I don't recall you telling any argument against it. I belive
>that this should not be paramater. This is very tightly related to
>reload, could you please have it as an attribute of reload, as I
>suggested?

I just wrote the thread to the previous version. I understand now why
you need param as you require reboot to activate the feature.

However, I don't think it is correct to use enable_live_dev_reset to
indicate the live-reset capability to the user. Params serve for
configuration only. Could you please move the indication some place
else? ("devlink dev info" seems fitting).

I think that you can do the indication in a follow-up patchset. But
please remove it from this one where you do it with params.


>
>Thanks!
>
>
>>"allow_live_dev_reset".
>>- Expand the documentation of each param and update commit messages
>> accordingly.
>>- Separated the permanent configuration mode code to another patch and
>>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
>>
>>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
>>- Update documentation files and commit messages with more details of the
>> feature.
>>
>>Vasundhara Volam (6):
>>  devlink: Add 'enable_live_dev_reset' generic parameter.
>>  devlink: Add 'allow_live_dev_reset' generic parameter.
>>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
>>  bnxt_en: Update firmware spec. to 1.10.1.40.
>>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
>>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
>>
>> Documentation/networking/devlink/bnxt.rst          |  4 ++
>> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
>> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
>> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
>> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
>> include/net/devlink.h                              |  8 +++
>> net/core/devlink.c                                 | 10 ++++
>> 10 files changed, 175 insertions(+), 36 deletions(-)
>>
>>-- 
>>1.8.3.1
>>

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Vasundhara Volam]

On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >>Live device reset capability allows the users to reset the device in real
> >>time. For example, after flashing a new firmware image, this feature allows
> >>a user to initiate the reset immediately from a separate command, to load
> >>the new firmware without reloading the driver or resetting the system.
> >>
> >>When device reset is initiated, services running on the host interfaces
> >>will momentarily pause and resume once reset is completed, which is very
> >>similar to momentary network outage.
> >>
> >>This patchset adds support for two new generic devlink parameters for
> >>controlling the live device reset capability and use it in the bnxt_en
> >>driver.
> >>
> >>Users can initiate the reset from a separate command, for example,
> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
> >>device.
> >>Where ethX or dev is any PF with administrative privileges.
> >>
> >>Patchset also updates firmware spec. to 1.10.1.40.
> >>
> >>
> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
> >
> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
> >attribute. I don't recall you telling any argument against it. I belive
> >that this should not be paramater. This is very tightly related to
> >reload, could you please have it as an attribute of reload, as I
> >suggested?
>
> I just wrote the thread to the previous version. I understand now why
> you need param as you require reboot to activate the feature.

Okay.
>
> However, I don't think it is correct to use enable_live_dev_reset to
> indicate the live-reset capability to the user. Params serve for
> configuration only. Could you please move the indication some place
> else? ("devlink dev info" seems fitting).

Here we are not indicating the support. If the parameter is set to
true, we are enabling the feature in firmware and driver after reboot.
Users can disable the feature by setting the parameter to false and
reboot. This is the configuration which is enabling or disabling the
feature in the device.

>
> I think that you can do the indication in a follow-up patchset. But
> please remove it from this one where you do it with params.

Could you please see the complete patchset and use it bnxt_en driver
to get a clear picture? We are not indicating the support.

Thanks.

>
>
> >
> >Thanks!
> >
> >
> >>"allow_live_dev_reset".
> >>- Expand the documentation of each param and update commit messages
> >> accordingly.
> >>- Separated the permanent configuration mode code to another patch and
> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
> >>
> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
> >>- Update documentation files and commit messages with more details of the
> >> feature.
> >>
> >>Vasundhara Volam (6):
> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
> >>
> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> >> include/net/devlink.h                              |  8 +++
> >> net/core/devlink.c                                 | 10 ++++
> >> 10 files changed, 175 insertions(+), 36 deletions(-)
> >>
> >>--
> >>1.8.3.1
> >>

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Jiri Pirko]

Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
>> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >>Live device reset capability allows the users to reset the device in real
>> >>time. For example, after flashing a new firmware image, this feature allows
>> >>a user to initiate the reset immediately from a separate command, to load
>> >>the new firmware without reloading the driver or resetting the system.
>> >>
>> >>When device reset is initiated, services running on the host interfaces
>> >>will momentarily pause and resume once reset is completed, which is very
>> >>similar to momentary network outage.
>> >>
>> >>This patchset adds support for two new generic devlink parameters for
>> >>controlling the live device reset capability and use it in the bnxt_en
>> >>driver.
>> >>
>> >>Users can initiate the reset from a separate command, for example,
>> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
>> >>device.
>> >>Where ethX or dev is any PF with administrative privileges.
>> >>
>> >>Patchset also updates firmware spec. to 1.10.1.40.
>> >>
>> >>
>> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
>> >
>> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
>> >attribute. I don't recall you telling any argument against it. I belive
>> >that this should not be paramater. This is very tightly related to
>> >reload, could you please have it as an attribute of reload, as I
>> >suggested?
>>
>> I just wrote the thread to the previous version. I understand now why
>> you need param as you require reboot to activate the feature.
>
>Okay.
>>
>> However, I don't think it is correct to use enable_live_dev_reset to
>> indicate the live-reset capability to the user. Params serve for
>> configuration only. Could you please move the indication some place
>> else? ("devlink dev info" seems fitting).
>
>Here we are not indicating the support. If the parameter is set to
>true, we are enabling the feature in firmware and driver after reboot.
>Users can disable the feature by setting the parameter to false and
>reboot. This is the configuration which is enabling or disabling the
>feature in the device.

You are talking about cmode permanent here. I'm talking about cmode
runtime.


>
>>
>> I think that you can do the indication in a follow-up patchset. But
>> please remove it from this one where you do it with params.
>
>Could you please see the complete patchset and use it bnxt_en driver
>to get a clear picture? We are not indicating the support.
>
>Thanks.
>
>>
>>
>> >
>> >Thanks!
>> >
>> >
>> >>"allow_live_dev_reset".
>> >>- Expand the documentation of each param and update commit messages
>> >> accordingly.
>> >>- Separated the permanent configuration mode code to another patch and
>> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
>> >>
>> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
>> >>- Update documentation files and commit messages with more details of the
>> >> feature.
>> >>
>> >>Vasundhara Volam (6):
>> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
>> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
>> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
>> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
>> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
>> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
>> >>
>> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
>> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
>> >> include/net/devlink.h                              |  8 +++
>> >> net/core/devlink.c                                 | 10 ++++
>> >> 10 files changed, 175 insertions(+), 36 deletions(-)
>> >>
>> >>--
>> >>1.8.3.1
>> >>

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Jiri Pirko]

Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:

[...]

> Documentation/networking/devlink/bnxt.rst          |  4 ++
> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> include/net/devlink.h                              |  8 +++
> net/core/devlink.c                                 | 10 ++++

Could you please cc me to this patchset? use scripts/maintainers to get
the cc list.

It is also customary to cc people that replied to the previous patchset
versions.


> 10 files changed, 175 insertions(+), 36 deletions(-)
>
>-- 
>1.8.3.1
>

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Jiri Pirko]

Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
>> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >>Live device reset capability allows the users to reset the device in real
>> >>time. For example, after flashing a new firmware image, this feature allows
>> >>a user to initiate the reset immediately from a separate command, to load
>> >>the new firmware without reloading the driver or resetting the system.
>> >>
>> >>When device reset is initiated, services running on the host interfaces
>> >>will momentarily pause and resume once reset is completed, which is very
>> >>similar to momentary network outage.
>> >>
>> >>This patchset adds support for two new generic devlink parameters for
>> >>controlling the live device reset capability and use it in the bnxt_en
>> >>driver.
>> >>
>> >>Users can initiate the reset from a separate command, for example,
>> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
>> >>device.
>> >>Where ethX or dev is any PF with administrative privileges.
>> >>
>> >>Patchset also updates firmware spec. to 1.10.1.40.
>> >>
>> >>
>> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
>> >
>> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
>> >attribute. I don't recall you telling any argument against it. I belive
>> >that this should not be paramater. This is very tightly related to
>> >reload, could you please have it as an attribute of reload, as I
>> >suggested?
>>
>> I just wrote the thread to the previous version. I understand now why
>> you need param as you require reboot to activate the feature.
>
>Okay.
>>
>> However, I don't think it is correct to use enable_live_dev_reset to
>> indicate the live-reset capability to the user. Params serve for
>> configuration only. Could you please move the indication some place
>> else? ("devlink dev info" seems fitting).
>
>Here we are not indicating the support. If the parameter is set to
>true, we are enabling the feature in firmware and driver after reboot.
>Users can disable the feature by setting the parameter to false and
>reboot. This is the configuration which is enabling or disabling the
>feature in the device.
>
>>
>> I think that you can do the indication in a follow-up patchset. But
>> please remove it from this one where you do it with params.
>
>Could you please see the complete patchset and use it bnxt_en driver
>to get a clear picture? We are not indicating the support.

Right. I see.

There is still one thing that I see problematic. There is no clear
semantics on when the "live fw update" is going to be performed. You
enable the feature in NVRAM and you set to "allow" it from all the host.
Now the user does reset, the driver has 2 options:
1) do the live fw reset
2) do the ordinary fw reset

This specification is missing and I believe it should be part of this
patchset, otherwise the behaviour might not be deterministic between
drivers and driver versions.

I think that the legacy ethtool should stick with the "ordinary fw reset",
becase that is what user expects. You should add an attribute to
"devlink dev reload" to trigger the "live fw reset"



>
>Thanks.
>
>>
>>
>> >
>> >Thanks!
>> >
>> >
>> >>"allow_live_dev_reset".
>> >>- Expand the documentation of each param and update commit messages
>> >> accordingly.
>> >>- Separated the permanent configuration mode code to another patch and
>> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
>> >>
>> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
>> >>- Update documentation files and commit messages with more details of the
>> >> feature.
>> >>
>> >>Vasundhara Volam (6):
>> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
>> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
>> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
>> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
>> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
>> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
>> >>
>> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
>> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
>> >> include/net/devlink.h                              |  8 +++
>> >> net/core/devlink.c                                 | 10 ++++
>> >> 10 files changed, 175 insertions(+), 36 deletions(-)
>> >>
>> >>--
>> >>1.8.3.1
>> >>

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Vasundhara Volam]

On Mon, Jun 1, 2020 at 3:22 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
> >> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >>Live device reset capability allows the users to reset the device in real
> >> >>time. For example, after flashing a new firmware image, this feature allows
> >> >>a user to initiate the reset immediately from a separate command, to load
> >> >>the new firmware without reloading the driver or resetting the system.
> >> >>
> >> >>When device reset is initiated, services running on the host interfaces
> >> >>will momentarily pause and resume once reset is completed, which is very
> >> >>similar to momentary network outage.
> >> >>
> >> >>This patchset adds support for two new generic devlink parameters for
> >> >>controlling the live device reset capability and use it in the bnxt_en
> >> >>driver.
> >> >>
> >> >>Users can initiate the reset from a separate command, for example,
> >> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
> >> >>device.
> >> >>Where ethX or dev is any PF with administrative privileges.
> >> >>
> >> >>Patchset also updates firmware spec. to 1.10.1.40.
> >> >>
> >> >>
> >> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
> >> >
> >> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
> >> >attribute. I don't recall you telling any argument against it. I belive
> >> >that this should not be paramater. This is very tightly related to
> >> >reload, could you please have it as an attribute of reload, as I
> >> >suggested?
> >>
> >> I just wrote the thread to the previous version. I understand now why
> >> you need param as you require reboot to activate the feature.
> >
> >Okay.
> >>
> >> However, I don't think it is correct to use enable_live_dev_reset to
> >> indicate the live-reset capability to the user. Params serve for
> >> configuration only. Could you please move the indication some place
> >> else? ("devlink dev info" seems fitting).
> >
> >Here we are not indicating the support. If the parameter is set to
> >true, we are enabling the feature in firmware and driver after reboot.
> >Users can disable the feature by setting the parameter to false and
> >reboot. This is the configuration which is enabling or disabling the
> >feature in the device.
>
> You are talking about cmode permanent here. I'm talking about cmode
> runtime.
For cmode runtime, I have renamed it to "allow_live_dev_reset". As I
see the comment under "enable_live_dev_reset", I thought you are
talking about permanent cmode.

"allow_live_dev_reset" is runtime cmode, which will allow users to
disable the "live reset" feature temporarily. It just not only
indicate the support but user can configure it to control the "live
reset" at runtime.
>
>
> >
> >>
> >> I think that you can do the indication in a follow-up patchset. But
> >> please remove it from this one where you do it with params.
> >
> >Could you please see the complete patchset and use it bnxt_en driver
> >to get a clear picture? We are not indicating the support.
> >
> >Thanks.
> >
> >>
> >>
> >> >
> >> >Thanks!
> >> >
> >> >
> >> >>"allow_live_dev_reset".
> >> >>- Expand the documentation of each param and update commit messages
> >> >> accordingly.
> >> >>- Separated the permanent configuration mode code to another patch and
> >> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
> >> >>
> >> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
> >> >>- Update documentation files and commit messages with more details of the
> >> >> feature.
> >> >>
> >> >>Vasundhara Volam (6):
> >> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
> >> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
> >> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
> >> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
> >> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
> >> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
> >> >>
> >> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
> >> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> >> >> include/net/devlink.h                              |  8 +++
> >> >> net/core/devlink.c                                 | 10 ++++
> >> >> 10 files changed, 175 insertions(+), 36 deletions(-)
> >> >>
> >> >>--
> >> >>1.8.3.1
> >> >>

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Vasundhara Volam]

On Mon, Jun 1, 2020 at 3:28 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
>
> [...]
>
> > Documentation/networking/devlink/bnxt.rst          |  4 ++
> > .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> > drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> > drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> > drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> > include/net/devlink.h                              |  8 +++
> > net/core/devlink.c                                 | 10 ++++
>
> Could you please cc me to this patchset? use scripts/maintainers to get
> the cc list.
>
> It is also customary to cc people that replied to the previous patchset
> versions.
I am sorry. I copied you only on devlink patches. I will keep in mind
to copy on the entire patchset from next time.

Thanks.
>
>
> > 10 files changed, 175 insertions(+), 36 deletions(-)
> >
> >--
> >1.8.3.1
> >

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Jiri Pirko]

Mon, Jun 01, 2020 at 12:08:14PM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Mon, Jun 1, 2020 at 3:22 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
>> >> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >>Live device reset capability allows the users to reset the device in real
>> >> >>time. For example, after flashing a new firmware image, this feature allows
>> >> >>a user to initiate the reset immediately from a separate command, to load
>> >> >>the new firmware without reloading the driver or resetting the system.
>> >> >>
>> >> >>When device reset is initiated, services running on the host interfaces
>> >> >>will momentarily pause and resume once reset is completed, which is very
>> >> >>similar to momentary network outage.
>> >> >>
>> >> >>This patchset adds support for two new generic devlink parameters for
>> >> >>controlling the live device reset capability and use it in the bnxt_en
>> >> >>driver.
>> >> >>
>> >> >>Users can initiate the reset from a separate command, for example,
>> >> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
>> >> >>device.
>> >> >>Where ethX or dev is any PF with administrative privileges.
>> >> >>
>> >> >>Patchset also updates firmware spec. to 1.10.1.40.
>> >> >>
>> >> >>
>> >> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
>> >> >
>> >> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
>> >> >attribute. I don't recall you telling any argument against it. I belive
>> >> >that this should not be paramater. This is very tightly related to
>> >> >reload, could you please have it as an attribute of reload, as I
>> >> >suggested?
>> >>
>> >> I just wrote the thread to the previous version. I understand now why
>> >> you need param as you require reboot to activate the feature.
>> >
>> >Okay.
>> >>
>> >> However, I don't think it is correct to use enable_live_dev_reset to
>> >> indicate the live-reset capability to the user. Params serve for
>> >> configuration only. Could you please move the indication some place
>> >> else? ("devlink dev info" seems fitting).
>> >
>> >Here we are not indicating the support. If the parameter is set to
>> >true, we are enabling the feature in firmware and driver after reboot.
>> >Users can disable the feature by setting the parameter to false and
>> >reboot. This is the configuration which is enabling or disabling the
>> >feature in the device.
>>
>> You are talking about cmode permanent here. I'm talking about cmode
>> runtime.
>For cmode runtime, I have renamed it to "allow_live_dev_reset". As I
>see the comment under "enable_live_dev_reset", I thought you are
>talking about permanent cmode.
>
>"allow_live_dev_reset" is runtime cmode, which will allow users to
>disable the "live reset" feature temporarily. It just not only
>indicate the support but user can configure it to control the "live
>reset" at runtime.

Okay, that looks fine to me now.


>>
>>
>> >
>> >>
>> >> I think that you can do the indication in a follow-up patchset. But
>> >> please remove it from this one where you do it with params.
>> >
>> >Could you please see the complete patchset and use it bnxt_en driver
>> >to get a clear picture? We are not indicating the support.
>> >
>> >Thanks.
>> >
>> >>
>> >>
>> >> >
>> >> >Thanks!
>> >> >
>> >> >
>> >> >>"allow_live_dev_reset".
>> >> >>- Expand the documentation of each param and update commit messages
>> >> >> accordingly.
>> >> >>- Separated the permanent configuration mode code to another patch and
>> >> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
>> >> >>
>> >> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
>> >> >>- Update documentation files and commit messages with more details of the
>> >> >> feature.
>> >> >>
>> >> >>Vasundhara Volam (6):
>> >> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
>> >> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
>> >> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
>> >> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
>> >> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
>> >> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
>> >> >>
>> >> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
>> >> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
>> >> >> include/net/devlink.h                              |  8 +++
>> >> >> net/core/devlink.c                                 | 10 ++++
>> >> >> 10 files changed, 175 insertions(+), 36 deletions(-)
>> >> >>
>> >> >>--
>> >> >>1.8.3.1
>> >> >>

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Vasundhara Volam]

On Mon, Jun 1, 2020 at 3:34 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
> >> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >>Live device reset capability allows the users to reset the device in real
> >> >>time. For example, after flashing a new firmware image, this feature allows
> >> >>a user to initiate the reset immediately from a separate command, to load
> >> >>the new firmware without reloading the driver or resetting the system.
> >> >>
> >> >>When device reset is initiated, services running on the host interfaces
> >> >>will momentarily pause and resume once reset is completed, which is very
> >> >>similar to momentary network outage.
> >> >>
> >> >>This patchset adds support for two new generic devlink parameters for
> >> >>controlling the live device reset capability and use it in the bnxt_en
> >> >>driver.
> >> >>
> >> >>Users can initiate the reset from a separate command, for example,
> >> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
> >> >>device.
> >> >>Where ethX or dev is any PF with administrative privileges.
> >> >>
> >> >>Patchset also updates firmware spec. to 1.10.1.40.
> >> >>
> >> >>
> >> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
> >> >
> >> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
> >> >attribute. I don't recall you telling any argument against it. I belive
> >> >that this should not be paramater. This is very tightly related to
> >> >reload, could you please have it as an attribute of reload, as I
> >> >suggested?
> >>
> >> I just wrote the thread to the previous version. I understand now why
> >> you need param as you require reboot to activate the feature.
> >
> >Okay.
> >>
> >> However, I don't think it is correct to use enable_live_dev_reset to
> >> indicate the live-reset capability to the user. Params serve for
> >> configuration only. Could you please move the indication some place
> >> else? ("devlink dev info" seems fitting).
> >
> >Here we are not indicating the support. If the parameter is set to
> >true, we are enabling the feature in firmware and driver after reboot.
> >Users can disable the feature by setting the parameter to false and
> >reboot. This is the configuration which is enabling or disabling the
> >feature in the device.
> >
> >>
> >> I think that you can do the indication in a follow-up patchset. But
> >> please remove it from this one where you do it with params.
> >
> >Could you please see the complete patchset and use it bnxt_en driver
> >to get a clear picture? We are not indicating the support.
>
> Right. I see.
>
> There is still one thing that I see problematic. There is no clear
> semantics on when the "live fw update" is going to be performed. You
> enable the feature in NVRAM and you set to "allow" it from all the host.
> Now the user does reset, the driver has 2 options:
> 1) do the live fw reset
> 2) do the ordinary fw reset
>
> This specification is missing and I believe it should be part of this
> patchset, otherwise the behaviour might not be deterministic between
> drivers and driver versions.

I see, this makes sense. It takes little time for me to extend
"devlink dev reload". I will spend time on it and send the next
version with 'devlink dev reload' patches included.

>
> I think that the legacy ethtool should stick with the "ordinary fw reset",
> becase that is what user expects. You should add an attribute to
> "devlink dev reload" to trigger the "live fw reset"

Okay.

I am planning to add a type field with "driver-only | fw-reset |
live-fw-reset | live-fw-patch" to "devlink dev reload" command.

driver-only - Resets host driver instance of the 'devlink dev'
(current behaviour). This will be default, if the user does not
provide the type option.
fw-reset - Initiate the reset command for the currently running
firmware and wait for the driver reload for completing the reset.
(This is similar to the legacy "ethtool --reset all" command).
live-fw-reset - Resets the currently running firmware and driver entities.
live-fw-patch - Loads the currently pending flashed firmware and
reloads all driver entities. If no pending flashed firmware, resets
currently loaded firmware.

Thanks.
>
>
>
> >
> >Thanks.
> >
> >>
> >>
> >> >
> >> >Thanks!
> >> >
> >> >
> >> >>"allow_live_dev_reset".
> >> >>- Expand the documentation of each param and update commit messages
> >> >> accordingly.
> >> >>- Separated the permanent configuration mode code to another patch and
> >> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
> >> >>
> >> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
> >> >>- Update documentation files and commit messages with more details of the
> >> >> feature.
> >> >>
> >> >>Vasundhara Volam (6):
> >> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
> >> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
> >> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
> >> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
> >> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
> >> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
> >> >>
> >> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
> >> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> >> >> include/net/devlink.h                              |  8 +++
> >> >> net/core/devlink.c                                 | 10 ++++
> >> >> 10 files changed, 175 insertions(+), 36 deletions(-)
> >> >>
> >> >>--
> >> >>1.8.3.1
> >> >>

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Jakub Kicinski]

On Mon, 1 Jun 2020 11:58:38 +0200 Jiri Pirko wrote:
> > Documentation/networking/devlink/bnxt.rst          |  4 ++
> > .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> > drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> > drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> > drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> > include/net/devlink.h                              |  8 +++
> > net/core/devlink.c                                 | 10 ++++  
> 
> Could you please cc me to this patchset? use scripts/maintainers to get
> the cc list.
> 
> It is also customary to cc people that replied to the previous patchset
> versions.

+1

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Jakub Kicinski]

On Mon, 1 Jun 2020 21:01:42 +0530 Vasundhara Volam wrote:
> > I think that the legacy ethtool should stick with the "ordinary fw reset",
> > becase that is what user expects. You should add an attribute to
> > "devlink dev reload" to trigger the "live fw reset"  
> 
> Okay.
> 
> I am planning to add a type field with "driver-only | fw-reset |
> live-fw-reset | live-fw-patch" to "devlink dev reload" command.
> 
> driver-only - Resets host driver instance of the 'devlink dev'
> (current behaviour). This will be default, if the user does not
> provide the type option.
> fw-reset - Initiate the reset command for the currently running
> firmware and wait for the driver reload for completing the reset.
> (This is similar to the legacy "ethtool --reset all" command).
> live-fw-reset - Resets the currently running firmware and driver entities.
> live-fw-patch - Loads the currently pending flashed firmware and
> reloads all driver entities. If no pending flashed firmware, resets
> currently loaded firmware.

FWIW I'd prefer to extend the ethtool semantics. Ethtool reset has two
reset "depths" already - single port, entire adapter, we could just add
"entire sled" here. IOW we'd have reset which can affect only given
port, then reset which can affect multiple ports, and reset which may
affect multiple systems.

The mechanism of the reset and whether old or new version of FW is
activated is a detail, which I believe will be entirely uninteresting 
to the user. Whether other systems or ports are affected is _very_
important, OTOH.

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Jiri Pirko]

Tue, Jun 02, 2020 at 01:24:16AM CEST, kuba@kernel.org wrote:
>On Mon, 1 Jun 2020 21:01:42 +0530 Vasundhara Volam wrote:
>> > I think that the legacy ethtool should stick with the "ordinary fw reset",
>> > becase that is what user expects. You should add an attribute to
>> > "devlink dev reload" to trigger the "live fw reset"  
>> 
>> Okay.
>> 
>> I am planning to add a type field with "driver-only | fw-reset |
>> live-fw-reset | live-fw-patch" to "devlink dev reload" command.
>> 
>> driver-only - Resets host driver instance of the 'devlink dev'
>> (current behaviour). This will be default, if the user does not
>> provide the type option.
>> fw-reset - Initiate the reset command for the currently running
>> firmware and wait for the driver reload for completing the reset.
>> (This is similar to the legacy "ethtool --reset all" command).
>> live-fw-reset - Resets the currently running firmware and driver entities.
>> live-fw-patch - Loads the currently pending flashed firmware and
>> reloads all driver entities. If no pending flashed firmware, resets
>> currently loaded firmware.
>
>FWIW I'd prefer to extend the ethtool semantics. Ethtool reset has two
>reset "depths" already - single port, entire adapter, we could just add
>"entire sled" here. IOW we'd have reset which can affect only given
>port, then reset which can affect multiple ports, and reset which may
>affect multiple systems.

Hmm, I think that one way or another, we need to implement this in
devlink and have compat fallback from ethtool there (as we have for
other things too).


>
>The mechanism of the reset and whether old or new version of FW is
>activated is a detail, which I believe will be entirely uninteresting 
>to the user. Whether other systems or ports are affected is _very_
>important, OTOH.

Wait. So you say that user is not interested if the reset is fw "live"
or not? There might be all sorts of issues when the reset happens under
working driver instance. I think that user should be able to indicate if
he is willing to take the risk.

Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params.

[Vasundhara Volam]

On Mon, Jun 1, 2020 at 9:01 PM Vasundhara Volam
<vasundhara-v.volam@broadcom.com> wrote:
>
> On Mon, Jun 1, 2020 at 3:34 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >
> > Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@broadcom.com wrote:
> > >On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
> > >>
> > >> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
> > >> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
> > >> >>Live device reset capability allows the users to reset the device in real
> > >> >>time. For example, after flashing a new firmware image, this feature allows
> > >> >>a user to initiate the reset immediately from a separate command, to load
> > >> >>the new firmware without reloading the driver or resetting the system.
> > >> >>
> > >> >>When device reset is initiated, services running on the host interfaces
> > >> >>will momentarily pause and resume once reset is completed, which is very
> > >> >>similar to momentary network outage.
> > >> >>
> > >> >>This patchset adds support for two new generic devlink parameters for
> > >> >>controlling the live device reset capability and use it in the bnxt_en
> > >> >>driver.
> > >> >>
> > >> >>Users can initiate the reset from a separate command, for example,
> > >> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
> > >> >>device.
> > >> >>Where ethX or dev is any PF with administrative privileges.
> > >> >>
> > >> >>Patchset also updates firmware spec. to 1.10.1.40.
> > >> >>
> > >> >>
> > >> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
> > >> >
> > >> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
> > >> >attribute. I don't recall you telling any argument against it. I belive
> > >> >that this should not be paramater. This is very tightly related to
> > >> >reload, could you please have it as an attribute of reload, as I
> > >> >suggested?
> > >>
> > >> I just wrote the thread to the previous version. I understand now why
> > >> you need param as you require reboot to activate the feature.
> > >
> > >Okay.
> > >>
> > >> However, I don't think it is correct to use enable_live_dev_reset to
> > >> indicate the live-reset capability to the user. Params serve for
> > >> configuration only. Could you please move the indication some place
> > >> else? ("devlink dev info" seems fitting).
> > >
> > >Here we are not indicating the support. If the parameter is set to
> > >true, we are enabling the feature in firmware and driver after reboot.
> > >Users can disable the feature by setting the parameter to false and
> > >reboot. This is the configuration which is enabling or disabling the
> > >feature in the device.
> > >
> > >>
> > >> I think that you can do the indication in a follow-up patchset. But
> > >> please remove it from this one where you do it with params.
> > >
> > >Could you please see the complete patchset and use it bnxt_en driver
> > >to get a clear picture? We are not indicating the support.
> >
> > Right. I see.
> >
> > There is still one thing that I see problematic. There is no clear
> > semantics on when the "live fw update" is going to be performed. You
> > enable the feature in NVRAM and you set to "allow" it from all the host.
> > Now the user does reset, the driver has 2 options:
> > 1) do the live fw reset
> > 2) do the ordinary fw reset
> >
> > This specification is missing and I believe it should be part of this
> > patchset, otherwise the behaviour might not be deterministic between
> > drivers and driver versions.
>
> I see, this makes sense. It takes little time for me to extend
> "devlink dev reload". I will spend time on it and send the next
> version with 'devlink dev reload' patches included.
>
> >
> > I think that the legacy ethtool should stick with the "ordinary fw reset",
> > becase that is what user expects. You should add an attribute to
> > "devlink dev reload" to trigger the "live fw reset"
>
> Okay.
>
> I am planning to add a type field with "driver-only | fw-reset |
> live-fw-reset | live-fw-patch" to "devlink dev reload" command.
>
> driver-only - Resets host driver instance of the 'devlink dev'
> (current behaviour). This will be default, if the user does not
> provide the type option.
> fw-reset - Initiate the reset command for the currently running
> firmware and wait for the driver reload for completing the reset.
> (This is similar to the legacy "ethtool --reset all" command).
> live-fw-reset - Resets the currently running firmware and driver entities.
> live-fw-patch - Loads the currently pending flashed firmware and
> reloads all driver entities. If no pending flashed firmware, resets
> currently loaded firmware.
I take back my proposal after taking a closer look at 'devlink dev
reload' implementation. 'devlink dev reload' is a synchronous
mechanism, which calls the reload_down and reload_up similar to remove
and probe callbacks respectively, per my understanding. This is not
what 'ethtool --reset' does.

'ethtool --reset' invokes driver callback, which in turn issues a
firmware command for reset.

We need to either extend ethtool for users to provide additional
entire-sled depth and type of reset as live/no-live. Or add a new
devlink command for fw-reset and add fallback to ethtool from there.

Thanks.
>
> Thanks.
> >
> >
> >
> > >
> > >Thanks.
> > >
> > >>
> > >>
> > >> >
> > >> >Thanks!
> > >> >
> > >> >
> > >> >>"allow_live_dev_reset".
> > >> >>- Expand the documentation of each param and update commit messages
> > >> >> accordingly.
> > >> >>- Separated the permanent configuration mode code to another patch and
> > >> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
> > >> >>
> > >> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
> > >> >>- Update documentation files and commit messages with more details of the
> > >> >> feature.
> > >> >>
> > >> >>Vasundhara Volam (6):
> > >> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
> > >> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
> > >> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
> > >> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
> > >> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
> > >> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
> > >> >>
> > >> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
> > >> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> > >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> > >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> > >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> > >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> > >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> > >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> > >> >> include/net/devlink.h                              |  8 +++
> > >> >> net/core/devlink.c                                 | 10 ++++
> > >> >> 10 files changed, 175 insertions(+), 36 deletions(-)
> > >> >>
> > >> >>--
> > >> >>1.8.3.1
> > >> >>