[PATCH v2 net-next 0/4] bnxt_en: Add new "allow_fw_live_reset" generic devlink parameter

[PATCH v2 net-next 0/4] bnxt_en: Add new "allow_fw_live_reset" generic devlink parameter

[Vasundhara Volam]

This patchset adds support for a "allow_fw_live_reset" generic devlink
parameter and use it in bnxt_en driver.

Firmware live reset allows users to reset the firmware in real time. For
example, after firmware upgrade, this feature can immediately reset to run
the new firmware without reloading the driver or rebooting 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.

User can initiate the fw reset by using "ethtool --reset ethX all" command.
Where ethX is any PF of the device with administrative privileges.

Firmware can initiate the live reset only when all the installed host
driver(s) also support the feature. For example, if a function is loaded
with a very old driver that is not aware of live reset capability,
firmware cannot initiate the reset until that driver is unloaded or
upgraded.

"allow_fw_live_reset" runtime configuration mode allows the user to control
this feature by enabling or disabling it in the host driver. And permanent
configuration mode allows the user to enable the firmware live reset
capability in NVRAM configuration of the device.

Also, firmware spec. is updated to 1.10.1.40.

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 (4):
  devlink: Add new "allow_fw_live_reset" generic device parameter.
  bnxt_en: Update firmware spec. to 1.10.1.40.
  bnxt_en: Use allow_fw_live_reset generic devlink parameter
  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET

 Documentation/networking/devlink/bnxt.rst          | 13 +++++
 .../networking/devlink/devlink-params.rst          |  6 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 61 +++++++++++++++++++++
 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                              |  4 ++
 net/core/devlink.c                                 |  5 ++
 10 files changed, 165 insertions(+), 36 deletions(-)

-- 
1.8.3.1

[PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

Add a new "allow_fw_live_reset" generic device bool parameter. When
parameter is set, user is allowed to reset the firmware in real time.

This parameter is employed to communicate user consent or dissent for
the live reset to happen. A separate command triggers the actual live
reset.

Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
v2: Rename param name to "allow_fw_live_reset" from
"enable_hot_fw_reset".
Update documentation for the param in devlink-params.rst file.
---
 Documentation/networking/devlink/devlink-params.rst | 6 ++++++
 include/net/devlink.h                               | 4 ++++
 net/core/devlink.c                                  | 5 +++++
 3 files changed, 15 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
index d075fd0..ad54dfb 100644
--- a/Documentation/networking/devlink/devlink-params.rst
+++ b/Documentation/networking/devlink/devlink-params.rst
@@ -108,3 +108,9 @@ own name.
    * - ``region_snapshot_enable``
      - Boolean
      - Enable capture of ``devlink-region`` snapshots.
+   * - ``allow_fw_live_reset``
+     - Boolean
+     - Firmware live reset allows users to reset the firmware in real time.
+       For example, after firmware upgrade, this feature can immediately reset
+       to run the new firmware without reloading the driver or rebooting the
+       system.
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
+		.name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
+		.type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
1.8.3.1

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
>Add a new "allow_fw_live_reset" generic device bool parameter. When
>parameter is set, user is allowed to reset the firmware in real time.
>
>This parameter is employed to communicate user consent or dissent for
>the live reset to happen. A separate command triggers the actual live
>reset.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>---
>v2: Rename param name to "allow_fw_live_reset" from
>"enable_hot_fw_reset".
>Update documentation for the param in devlink-params.rst file.
>---
> Documentation/networking/devlink/devlink-params.rst | 6 ++++++
> include/net/devlink.h                               | 4 ++++
> net/core/devlink.c                                  | 5 +++++
> 3 files changed, 15 insertions(+)
>
>diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>index d075fd0..ad54dfb 100644
>--- a/Documentation/networking/devlink/devlink-params.rst
>+++ b/Documentation/networking/devlink/devlink-params.rst
>@@ -108,3 +108,9 @@ own name.
>    * - ``region_snapshot_enable``
>      - Boolean
>      - Enable capture of ``devlink-region`` snapshots.
>+   * - ``allow_fw_live_reset``
>+     - Boolean
>+     - Firmware live reset allows users to reset the firmware in real time.
>+       For example, after firmware upgrade, this feature can immediately reset
>+       to run the new firmware without reloading the driver or rebooting the

This does not tell me anything about the reset being done on another
host. You need to emhasize that, in the name of the param too.



>+       system.
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
>+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
>+		.name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
>+		.type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
>+	},
> };
> 
> static int devlink_param_generic_verify(const struct devlink_param *param)
>-- 
>1.8.3.1
>

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >Add a new "allow_fw_live_reset" generic device bool parameter. When
> >parameter is set, user is allowed to reset the firmware in real time.
> >
> >This parameter is employed to communicate user consent or dissent for
> >the live reset to happen. A separate command triggers the actual live
> >reset.
> >
> >Cc: Jiri Pirko <jiri@mellanox.com>
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> >---
> >v2: Rename param name to "allow_fw_live_reset" from
> >"enable_hot_fw_reset".
> >Update documentation for the param in devlink-params.rst file.
> >---
> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
> > include/net/devlink.h                               | 4 ++++
> > net/core/devlink.c                                  | 5 +++++
> > 3 files changed, 15 insertions(+)
> >
> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
> >index d075fd0..ad54dfb 100644
> >--- a/Documentation/networking/devlink/devlink-params.rst
> >+++ b/Documentation/networking/devlink/devlink-params.rst
> >@@ -108,3 +108,9 @@ own name.
> >    * - ``region_snapshot_enable``
> >      - Boolean
> >      - Enable capture of ``devlink-region`` snapshots.
> >+   * - ``allow_fw_live_reset``
> >+     - Boolean
> >+     - Firmware live reset allows users to reset the firmware in real time.
> >+       For example, after firmware upgrade, this feature can immediately reset
> >+       to run the new firmware without reloading the driver or rebooting the
>
> This does not tell me anything about the reset being done on another
> host. You need to emhasize that, in the name of the param too.
I am not sure if I completely understand your query.

Reset is actually initiated by one of the PF/host of the device, which
resets the entire same device.

Reset is not initiated by any other remote device/host.

Thanks,
Vasundhara
>
>
>
> >+       system.
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
> >+      },
> > };
> >
> > static int devlink_param_generic_verify(const struct devlink_param *param)
> >--
> >1.8.3.1
> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >Add a new "allow_fw_live_reset" generic device bool parameter. When
>> >parameter is set, user is allowed to reset the firmware in real time.
>> >
>> >This parameter is employed to communicate user consent or dissent for
>> >the live reset to happen. A separate command triggers the actual live
>> >reset.
>> >
>> >Cc: Jiri Pirko <jiri@mellanox.com>
>> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> >---
>> >v2: Rename param name to "allow_fw_live_reset" from
>> >"enable_hot_fw_reset".
>> >Update documentation for the param in devlink-params.rst file.
>> >---
>> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
>> > include/net/devlink.h                               | 4 ++++
>> > net/core/devlink.c                                  | 5 +++++
>> > 3 files changed, 15 insertions(+)
>> >
>> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> >index d075fd0..ad54dfb 100644
>> >--- a/Documentation/networking/devlink/devlink-params.rst
>> >+++ b/Documentation/networking/devlink/devlink-params.rst
>> >@@ -108,3 +108,9 @@ own name.
>> >    * - ``region_snapshot_enable``
>> >      - Boolean
>> >      - Enable capture of ``devlink-region`` snapshots.
>> >+   * - ``allow_fw_live_reset``
>> >+     - Boolean
>> >+     - Firmware live reset allows users to reset the firmware in real time.
>> >+       For example, after firmware upgrade, this feature can immediately reset
>> >+       to run the new firmware without reloading the driver or rebooting the
>>
>> This does not tell me anything about the reset being done on another
>> host. You need to emhasize that, in the name of the param too.
>I am not sure if I completely understand your query.
>
>Reset is actually initiated by one of the PF/host of the device, which
>resets the entire same device.
>
>Reset is not initiated by any other remote device/host.

Well, in case of multihost system, it might be, right?


>
>Thanks,
>Vasundhara
>>
>>
>>
>> >+       system.
>> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
>> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
>> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
>> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
>> >+      },
>> > };
>> >
>> > static int devlink_param_generic_verify(const struct devlink_param *param)
>> >--
>> >1.8.3.1
>> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
> >> >parameter is set, user is allowed to reset the firmware in real time.
> >> >
> >> >This parameter is employed to communicate user consent or dissent for
> >> >the live reset to happen. A separate command triggers the actual live
> >> >reset.
> >> >
> >> >Cc: Jiri Pirko <jiri@mellanox.com>
> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> >> >---
> >> >v2: Rename param name to "allow_fw_live_reset" from
> >> >"enable_hot_fw_reset".
> >> >Update documentation for the param in devlink-params.rst file.
> >> >---
> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
> >> > include/net/devlink.h                               | 4 ++++
> >> > net/core/devlink.c                                  | 5 +++++
> >> > 3 files changed, 15 insertions(+)
> >> >
> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
> >> >index d075fd0..ad54dfb 100644
> >> >--- a/Documentation/networking/devlink/devlink-params.rst
> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
> >> >@@ -108,3 +108,9 @@ own name.
> >> >    * - ``region_snapshot_enable``
> >> >      - Boolean
> >> >      - Enable capture of ``devlink-region`` snapshots.
> >> >+   * - ``allow_fw_live_reset``
> >> >+     - Boolean
> >> >+     - Firmware live reset allows users to reset the firmware in real time.
> >> >+       For example, after firmware upgrade, this feature can immediately reset
> >> >+       to run the new firmware without reloading the driver or rebooting the
> >>
> >> This does not tell me anything about the reset being done on another
> >> host. You need to emhasize that, in the name of the param too.
> >I am not sure if I completely understand your query.
> >
> >Reset is actually initiated by one of the PF/host of the device, which
> >resets the entire same device.
> >
> >Reset is not initiated by any other remote device/host.
>
> Well, in case of multihost system, it might be, right?
>
In case of multi-host system also, it is one of the host that triggers
the reset, which resets the entire same device. I don't think this is
remote.

As the parameter is a device parameter, it is applicable to the entire
device. When a user initiates the reset from any of the host in case
of multi-host and any of the PF in case of stand-alone or smartNIC
device, the entire device goes for a reset.

I will be expanding the description to the following to make it more clear.

------------------------
- Firmware live reset allows users to reset the firmware in real time.
For example, after firmware upgrade, this feature can immediately
reset to run the new firmware without reloading the driver or
rebooting the system.
When a user initiates the reset from any of the host (in case of
multi-host system) / PF (in case of stand-alone or smartNIC device),
the entire device goes for a reset when the parameter is enabled.
------------------------

Thanks,
Vasundhara
>
> >
> >Thanks,
> >Vasundhara
> >>
> >>
> >>
> >> >+       system.
> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
> >> >+      },
> >> > };
> >> >
> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
> >> >--
> >> >1.8.3.1
> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
>> >> >parameter is set, user is allowed to reset the firmware in real time.
>> >> >
>> >> >This parameter is employed to communicate user consent or dissent for
>> >> >the live reset to happen. A separate command triggers the actual live
>> >> >reset.
>> >> >
>> >> >Cc: Jiri Pirko <jiri@mellanox.com>
>> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> >> >---
>> >> >v2: Rename param name to "allow_fw_live_reset" from
>> >> >"enable_hot_fw_reset".
>> >> >Update documentation for the param in devlink-params.rst file.
>> >> >---
>> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
>> >> > include/net/devlink.h                               | 4 ++++
>> >> > net/core/devlink.c                                  | 5 +++++
>> >> > 3 files changed, 15 insertions(+)
>> >> >
>> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> >> >index d075fd0..ad54dfb 100644
>> >> >--- a/Documentation/networking/devlink/devlink-params.rst
>> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
>> >> >@@ -108,3 +108,9 @@ own name.
>> >> >    * - ``region_snapshot_enable``
>> >> >      - Boolean
>> >> >      - Enable capture of ``devlink-region`` snapshots.
>> >> >+   * - ``allow_fw_live_reset``
>> >> >+     - Boolean
>> >> >+     - Firmware live reset allows users to reset the firmware in real time.
>> >> >+       For example, after firmware upgrade, this feature can immediately reset
>> >> >+       to run the new firmware without reloading the driver or rebooting the
>> >>
>> >> This does not tell me anything about the reset being done on another
>> >> host. You need to emhasize that, in the name of the param too.
>> >I am not sure if I completely understand your query.
>> >
>> >Reset is actually initiated by one of the PF/host of the device, which
>> >resets the entire same device.
>> >
>> >Reset is not initiated by any other remote device/host.
>>
>> Well, in case of multihost system, it might be, right?
>>
>In case of multi-host system also, it is one of the host that triggers
>the reset, which resets the entire same device. I don't think this is
>remote.
>
>As the parameter is a device parameter, it is applicable to the entire
>device. When a user initiates the reset from any of the host in case
>of multi-host and any of the PF in case of stand-alone or smartNIC
>device, the entire device goes for a reset.
>
>I will be expanding the description to the following to make it more clear.
>
>------------------------
>- Firmware live reset allows users to reset the firmware in real time.
>For example, after firmware upgrade, this feature can immediately
>reset to run the new firmware without reloading the driver or
>rebooting the system.
>When a user initiates the reset from any of the host (in case of
>multi-host system) / PF (in case of stand-alone or smartNIC device),
>the entire device goes for a reset when the parameter is enabled.

Sorry, this is still not clear. I think that you are mixing up two
different things:
1) option of devlink reload to indicate that user is interested in "live
   reset" of firmware without reloading driver
2) devlink param that would indicate "I am okay if someone else (not by
   my devlink instance) resets my firmware".

Could you please split?


>------------------------
>
>Thanks,
>Vasundhara
>>
>> >
>> >Thanks,
>> >Vasundhara
>> >>
>> >>
>> >>
>> >> >+       system.
>> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
>> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
>> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
>> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
>> >> >+      },
>> >> > };
>> >> >
>> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
>> >> >--
>> >> >1.8.3.1
>> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Tue, May 26, 2020 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >>
> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
> >> >> >parameter is set, user is allowed to reset the firmware in real time.
> >> >> >
> >> >> >This parameter is employed to communicate user consent or dissent for
> >> >> >the live reset to happen. A separate command triggers the actual live
> >> >> >reset.
> >> >> >
> >> >> >Cc: Jiri Pirko <jiri@mellanox.com>
> >> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> >> >> >---
> >> >> >v2: Rename param name to "allow_fw_live_reset" from
> >> >> >"enable_hot_fw_reset".
> >> >> >Update documentation for the param in devlink-params.rst file.
> >> >> >---
> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
> >> >> > include/net/devlink.h                               | 4 ++++
> >> >> > net/core/devlink.c                                  | 5 +++++
> >> >> > 3 files changed, 15 insertions(+)
> >> >> >
> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
> >> >> >index d075fd0..ad54dfb 100644
> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst
> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
> >> >> >@@ -108,3 +108,9 @@ own name.
> >> >> >    * - ``region_snapshot_enable``
> >> >> >      - Boolean
> >> >> >      - Enable capture of ``devlink-region`` snapshots.
> >> >> >+   * - ``allow_fw_live_reset``
> >> >> >+     - Boolean
> >> >> >+     - Firmware live reset allows users to reset the firmware in real time.
> >> >> >+       For example, after firmware upgrade, this feature can immediately reset
> >> >> >+       to run the new firmware without reloading the driver or rebooting the
> >> >>
> >> >> This does not tell me anything about the reset being done on another
> >> >> host. You need to emhasize that, in the name of the param too.
> >> >I am not sure if I completely understand your query.
> >> >
> >> >Reset is actually initiated by one of the PF/host of the device, which
> >> >resets the entire same device.
> >> >
> >> >Reset is not initiated by any other remote device/host.
> >>
> >> Well, in case of multihost system, it might be, right?
> >>
> >In case of multi-host system also, it is one of the host that triggers
> >the reset, which resets the entire same device. I don't think this is
> >remote.
> >
> >As the parameter is a device parameter, it is applicable to the entire
> >device. When a user initiates the reset from any of the host in case
> >of multi-host and any of the PF in case of stand-alone or smartNIC
> >device, the entire device goes for a reset.
> >
> >I will be expanding the description to the following to make it more clear.
> >
> >------------------------
> >- Firmware live reset allows users to reset the firmware in real time.
> >For example, after firmware upgrade, this feature can immediately
> >reset to run the new firmware without reloading the driver or
> >rebooting the system.
> >When a user initiates the reset from any of the host (in case of
> >multi-host system) / PF (in case of stand-alone or smartNIC device),
> >the entire device goes for a reset when the parameter is enabled.
>
> Sorry, this is still not clear. I think that you are mixing up two
> different things:
> 1) option of devlink reload to indicate that user is interested in "live
>    reset" of firmware without reloading driver

This is the option we are trying to add. If a user is interested in
"live reset", he needs to enable the parameter to enable it in device
capabilities, which is achieved by permanent configuration mode. When
capability is enabled in the device, new firmware which is aware will
allocate the resources and exposes the capability to host drivers.

But firmware allows the "live reset" only when all the loaded drivers
are aware of/supports the capability. For example, if any of the host
is loaded with an old driver, "live reset" is not allowed until the
driver is upgraded or unloaded. or if the host driver turns it off,
then also "live reset" is not allowed.

In case of runtime parameter cmode, if any of the host turns off the
capability in the host driver, "live reset" is not allowed until the
driver is unloaded or the user enables it again.

To make it clear, I can add two parameters.

1. enable_fw_live_reset - To indicate that the user is interested in
"live reset". This will be a generic param.

2. allow_fw_live_reset - To indicate, if any of the host/PF turns it
off, "live reset" is not allowed. This serves the purpose of what we
are trying to add in runtime cmode.
Do you want me to keep it as a driver-specific param?

Please let me know if this is clear and makes less confusion.

Thanks,
Vasundhara

> 2) devlink param that would indicate "I am okay if someone else (not by
>    my devlink instance) resets my firmware".
>
> Could you please split?
>
>
> >------------------------
> >
> >Thanks,
> >Vasundhara
> >>
> >> >
> >> >Thanks,
> >> >Vasundhara
> >> >>
> >> >>
> >> >>
> >> >> >+       system.
> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
> >> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
> >> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
> >> >> >+      },
> >> >> > };
> >> >> >
> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
> >> >> >--
> >> >> >1.8.3.1
> >> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Tue, May 26, 2020 at 08:42:28AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Tue, May 26, 2020 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >>
>> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
>> >> >> >parameter is set, user is allowed to reset the firmware in real time.
>> >> >> >
>> >> >> >This parameter is employed to communicate user consent or dissent for
>> >> >> >the live reset to happen. A separate command triggers the actual live
>> >> >> >reset.
>> >> >> >
>> >> >> >Cc: Jiri Pirko <jiri@mellanox.com>
>> >> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> >> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> >> >> >---
>> >> >> >v2: Rename param name to "allow_fw_live_reset" from
>> >> >> >"enable_hot_fw_reset".
>> >> >> >Update documentation for the param in devlink-params.rst file.
>> >> >> >---
>> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
>> >> >> > include/net/devlink.h                               | 4 ++++
>> >> >> > net/core/devlink.c                                  | 5 +++++
>> >> >> > 3 files changed, 15 insertions(+)
>> >> >> >
>> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> >> >> >index d075fd0..ad54dfb 100644
>> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst
>> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
>> >> >> >@@ -108,3 +108,9 @@ own name.
>> >> >> >    * - ``region_snapshot_enable``
>> >> >> >      - Boolean
>> >> >> >      - Enable capture of ``devlink-region`` snapshots.
>> >> >> >+   * - ``allow_fw_live_reset``
>> >> >> >+     - Boolean
>> >> >> >+     - Firmware live reset allows users to reset the firmware in real time.
>> >> >> >+       For example, after firmware upgrade, this feature can immediately reset
>> >> >> >+       to run the new firmware without reloading the driver or rebooting the
>> >> >>
>> >> >> This does not tell me anything about the reset being done on another
>> >> >> host. You need to emhasize that, in the name of the param too.
>> >> >I am not sure if I completely understand your query.
>> >> >
>> >> >Reset is actually initiated by one of the PF/host of the device, which
>> >> >resets the entire same device.
>> >> >
>> >> >Reset is not initiated by any other remote device/host.
>> >>
>> >> Well, in case of multihost system, it might be, right?
>> >>
>> >In case of multi-host system also, it is one of the host that triggers
>> >the reset, which resets the entire same device. I don't think this is
>> >remote.
>> >
>> >As the parameter is a device parameter, it is applicable to the entire
>> >device. When a user initiates the reset from any of the host in case
>> >of multi-host and any of the PF in case of stand-alone or smartNIC
>> >device, the entire device goes for a reset.
>> >
>> >I will be expanding the description to the following to make it more clear.
>> >
>> >------------------------
>> >- Firmware live reset allows users to reset the firmware in real time.
>> >For example, after firmware upgrade, this feature can immediately
>> >reset to run the new firmware without reloading the driver or
>> >rebooting the system.
>> >When a user initiates the reset from any of the host (in case of
>> >multi-host system) / PF (in case of stand-alone or smartNIC device),
>> >the entire device goes for a reset when the parameter is enabled.
>>
>> Sorry, this is still not clear. I think that you are mixing up two
>> different things:
>> 1) option of devlink reload to indicate that user is interested in "live
>>    reset" of firmware without reloading driver
>
>This is the option we are trying to add. If a user is interested in
>"live reset", he needs to enable the parameter to enable it in device
>capabilities, which is achieved by permanent configuration mode. When
>capability is enabled in the device, new firmware which is aware will
>allocate the resources and exposes the capability to host drivers.
>
>But firmware allows the "live reset" only when all the loaded drivers
>are aware of/supports the capability. For example, if any of the host
>is loaded with an old driver, "live reset" is not allowed until the
>driver is upgraded or unloaded. or if the host driver turns it off,
>then also "live reset" is not allowed.
>
>In case of runtime parameter cmode, if any of the host turns off the
>capability in the host driver, "live reset" is not allowed until the
>driver is unloaded or the user enables it again.
>
>To make it clear, I can add two parameters.
>
>1. enable_fw_live_reset - To indicate that the user is interested in
>"live reset". This will be a generic param.

As I wrote above, I believe this should be an option
to "devlink dev reload", not a param.


>
>2. allow_fw_live_reset - To indicate, if any of the host/PF turns it
>off, "live reset" is not allowed. This serves the purpose of what we
>are trying to add in runtime cmode.

Yeah.

>Do you want me to keep it as a driver-specific param?

There is nothing driver-specific about this.


>
>Please let me know if this is clear and makes less confusion.
>
>Thanks,
>Vasundhara
>
>> 2) devlink param that would indicate "I am okay if someone else (not by
>>    my devlink instance) resets my firmware".
>>
>> Could you please split?
>>
>>
>> >------------------------
>> >
>> >Thanks,
>> >Vasundhara
>> >>
>> >> >
>> >> >Thanks,
>> >> >Vasundhara
>> >> >>
>> >> >>
>> >> >>
>> >> >> >+       system.
>> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
>> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
>> >> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
>> >> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
>> >> >> >+      },
>> >> >> > };
>> >> >> >
>> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
>> >> >> >--
>> >> >> >1.8.3.1
>> >> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Tue, May 26, 2020 at 7:10 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, May 26, 2020 at 08:42:28AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Tue, May 26, 2020 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >>
> >> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> >>
> >> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
> >> >> >> >parameter is set, user is allowed to reset the firmware in real time.
> >> >> >> >
> >> >> >> >This parameter is employed to communicate user consent or dissent for
> >> >> >> >the live reset to happen. A separate command triggers the actual live
> >> >> >> >reset.
> >> >> >> >
> >> >> >> >Cc: Jiri Pirko <jiri@mellanox.com>
> >> >> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >> >> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> >> >> >> >---
> >> >> >> >v2: Rename param name to "allow_fw_live_reset" from
> >> >> >> >"enable_hot_fw_reset".
> >> >> >> >Update documentation for the param in devlink-params.rst file.
> >> >> >> >---
> >> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
> >> >> >> > include/net/devlink.h                               | 4 ++++
> >> >> >> > net/core/devlink.c                                  | 5 +++++
> >> >> >> > 3 files changed, 15 insertions(+)
> >> >> >> >
> >> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
> >> >> >> >index d075fd0..ad54dfb 100644
> >> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst
> >> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
> >> >> >> >@@ -108,3 +108,9 @@ own name.
> >> >> >> >    * - ``region_snapshot_enable``
> >> >> >> >      - Boolean
> >> >> >> >      - Enable capture of ``devlink-region`` snapshots.
> >> >> >> >+   * - ``allow_fw_live_reset``
> >> >> >> >+     - Boolean
> >> >> >> >+     - Firmware live reset allows users to reset the firmware in real time.
> >> >> >> >+       For example, after firmware upgrade, this feature can immediately reset
> >> >> >> >+       to run the new firmware without reloading the driver or rebooting the
> >> >> >>
> >> >> >> This does not tell me anything about the reset being done on another
> >> >> >> host. You need to emhasize that, in the name of the param too.
> >> >> >I am not sure if I completely understand your query.
> >> >> >
> >> >> >Reset is actually initiated by one of the PF/host of the device, which
> >> >> >resets the entire same device.
> >> >> >
> >> >> >Reset is not initiated by any other remote device/host.
> >> >>
> >> >> Well, in case of multihost system, it might be, right?
> >> >>
> >> >In case of multi-host system also, it is one of the host that triggers
> >> >the reset, which resets the entire same device. I don't think this is
> >> >remote.
> >> >
> >> >As the parameter is a device parameter, it is applicable to the entire
> >> >device. When a user initiates the reset from any of the host in case
> >> >of multi-host and any of the PF in case of stand-alone or smartNIC
> >> >device, the entire device goes for a reset.
> >> >
> >> >I will be expanding the description to the following to make it more clear.
> >> >
> >> >------------------------
> >> >- Firmware live reset allows users to reset the firmware in real time.
> >> >For example, after firmware upgrade, this feature can immediately
> >> >reset to run the new firmware without reloading the driver or
> >> >rebooting the system.
> >> >When a user initiates the reset from any of the host (in case of
> >> >multi-host system) / PF (in case of stand-alone or smartNIC device),
> >> >the entire device goes for a reset when the parameter is enabled.
> >>
> >> Sorry, this is still not clear. I think that you are mixing up two
> >> different things:
> >> 1) option of devlink reload to indicate that user is interested in "live
> >>    reset" of firmware without reloading driver
> >
> >This is the option we are trying to add. If a user is interested in
> >"live reset", he needs to enable the parameter to enable it in device
> >capabilities, which is achieved by permanent configuration mode. When
> >capability is enabled in the device, new firmware which is aware will
> >allocate the resources and exposes the capability to host drivers.
> >
> >But firmware allows the "live reset" only when all the loaded drivers
> >are aware of/supports the capability. For example, if any of the host
> >is loaded with an old driver, "live reset" is not allowed until the
> >driver is upgraded or unloaded. or if the host driver turns it off,
> >then also "live reset" is not allowed.
> >
> >In case of runtime parameter cmode, if any of the host turns off the
> >capability in the host driver, "live reset" is not allowed until the
> >driver is unloaded or the user enables it again.
> >
> >To make it clear, I can add two parameters.
> >
> >1. enable_fw_live_reset - To indicate that the user is interested in
> >"live reset". This will be a generic param.
>
> As I wrote above, I believe this should be an option
> to "devlink dev reload", not a param.
I think you are still confused with enabling feature in NVRAM
configuration of the device and command to trigger reset. This param
will enable the feature in the device NVRAM configuration and does not
trigger the actual reset.

Only when the param is set, feature will be enabled in the device and
firmware supports the "live reset". When the param is disabled,
firmware cannot support "live reset" and user needs to do PCIe reset
after flashing the firmware for it to take effect..

Once feature is enabled in NVRAM configuration, it will be persistent
across reboots.

User still needs to use "devlink dev reload" command to do the "live reset".
>
>
> >
> >2. allow_fw_live_reset - To indicate, if any of the host/PF turns it
> >off, "live reset" is not allowed. This serves the purpose of what we
> >are trying to add in runtime cmode.
>
> Yeah.
And this param will enable the feature in the driver for driver to
allow the firmware to go for "live reset", where as above param will
enable the feature in NVRAM configuration of the device.
>
> >Do you want me to keep it as a driver-specific param?
>
> There is nothing driver-specific about this.
okay.
>
>
> >
> >Please let me know if this is clear and makes less confusion.
> >
> >Thanks,
> >Vasundhara
> >
> >> 2) devlink param that would indicate "I am okay if someone else (not by
> >>    my devlink instance) resets my firmware".
> >>
> >> Could you please split?
> >>
> >>
> >> >------------------------
> >> >
> >> >Thanks,
> >> >Vasundhara
> >> >>
> >> >> >
> >> >> >Thanks,
> >> >> >Vasundhara
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> >+       system.
> >> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
> >> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
> >> >> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
> >> >> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
> >> >> >> >+      },
> >> >> >> > };
> >> >> >> >
> >> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
> >> >> >> >--
> >> >> >> >1.8.3.1
> >> >> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Tue, May 26, 2020 at 7:53 PM Vasundhara Volam
<vasundhara-v.volam@broadcom.com> wrote:
>
> On Tue, May 26, 2020 at 7:10 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >
> > Tue, May 26, 2020 at 08:42:28AM CEST, vasundhara-v.volam@broadcom.com wrote:
> > >On Tue, May 26, 2020 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > >>
> > >> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
> > >> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
> > >> >>
> > >> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
> > >> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > >> >> >>
> > >> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
> > >> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
> > >> >> >> >parameter is set, user is allowed to reset the firmware in real time.
> > >> >> >> >
> > >> >> >> >This parameter is employed to communicate user consent or dissent for
> > >> >> >> >the live reset to happen. A separate command triggers the actual live
> > >> >> >> >reset.
> > >> >> >> >
> > >> >> >> >Cc: Jiri Pirko <jiri@mellanox.com>
> > >> >> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> > >> >> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > >> >> >> >---
> > >> >> >> >v2: Rename param name to "allow_fw_live_reset" from
> > >> >> >> >"enable_hot_fw_reset".
> > >> >> >> >Update documentation for the param in devlink-params.rst file.
> > >> >> >> >---
> > >> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
> > >> >> >> > include/net/devlink.h                               | 4 ++++
> > >> >> >> > net/core/devlink.c                                  | 5 +++++
> > >> >> >> > 3 files changed, 15 insertions(+)
> > >> >> >> >
> > >> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
> > >> >> >> >index d075fd0..ad54dfb 100644
> > >> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst
> > >> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
> > >> >> >> >@@ -108,3 +108,9 @@ own name.
> > >> >> >> >    * - ``region_snapshot_enable``
> > >> >> >> >      - Boolean
> > >> >> >> >      - Enable capture of ``devlink-region`` snapshots.
> > >> >> >> >+   * - ``allow_fw_live_reset``
> > >> >> >> >+     - Boolean
> > >> >> >> >+     - Firmware live reset allows users to reset the firmware in real time.
> > >> >> >> >+       For example, after firmware upgrade, this feature can immediately reset
> > >> >> >> >+       to run the new firmware without reloading the driver or rebooting the
> > >> >> >>
> > >> >> >> This does not tell me anything about the reset being done on another
> > >> >> >> host. You need to emhasize that, in the name of the param too.
> > >> >> >I am not sure if I completely understand your query.
> > >> >> >
> > >> >> >Reset is actually initiated by one of the PF/host of the device, which
> > >> >> >resets the entire same device.
> > >> >> >
> > >> >> >Reset is not initiated by any other remote device/host.
> > >> >>
> > >> >> Well, in case of multihost system, it might be, right?
> > >> >>
> > >> >In case of multi-host system also, it is one of the host that triggers
> > >> >the reset, which resets the entire same device. I don't think this is
> > >> >remote.
> > >> >
> > >> >As the parameter is a device parameter, it is applicable to the entire
> > >> >device. When a user initiates the reset from any of the host in case
> > >> >of multi-host and any of the PF in case of stand-alone or smartNIC
> > >> >device, the entire device goes for a reset.
> > >> >
> > >> >I will be expanding the description to the following to make it more clear.
> > >> >
> > >> >------------------------
> > >> >- Firmware live reset allows users to reset the firmware in real time.
> > >> >For example, after firmware upgrade, this feature can immediately
> > >> >reset to run the new firmware without reloading the driver or
> > >> >rebooting the system.
> > >> >When a user initiates the reset from any of the host (in case of
> > >> >multi-host system) / PF (in case of stand-alone or smartNIC device),
> > >> >the entire device goes for a reset when the parameter is enabled.
> > >>
> > >> Sorry, this is still not clear. I think that you are mixing up two
> > >> different things:
> > >> 1) option of devlink reload to indicate that user is interested in "live
> > >>    reset" of firmware without reloading driver
> > >
> > >This is the option we are trying to add. If a user is interested in
> > >"live reset", he needs to enable the parameter to enable it in device
> > >capabilities, which is achieved by permanent configuration mode. When
> > >capability is enabled in the device, new firmware which is aware will
> > >allocate the resources and exposes the capability to host drivers.
> > >
> > >But firmware allows the "live reset" only when all the loaded drivers
> > >are aware of/supports the capability. For example, if any of the host
> > >is loaded with an old driver, "live reset" is not allowed until the
> > >driver is upgraded or unloaded. or if the host driver turns it off,
> > >then also "live reset" is not allowed.
> > >
> > >In case of runtime parameter cmode, if any of the host turns off the
> > >capability in the host driver, "live reset" is not allowed until the
> > >driver is unloaded or the user enables it again.
> > >
> > >To make it clear, I can add two parameters.
> > >
> > >1. enable_fw_live_reset - To indicate that the user is interested in
> > >"live reset". This will be a generic param.
> >
> > As I wrote above, I believe this should be an option
> > to "devlink dev reload", not a param.
> I think you are still confused with enabling feature in NVRAM
> configuration of the device and command to trigger reset. This param
> will enable the feature in the device NVRAM configuration and does not
> trigger the actual reset.
>
> Only when the param is set, feature will be enabled in the device and
> firmware supports the "live reset". When the param is disabled,
> firmware cannot support "live reset" and user needs to do PCIe reset
> after flashing the firmware for it to take effect..
>
> Once feature is enabled in NVRAM configuration, it will be persistent
> across reboots.
>
> User still needs to use "devlink dev reload" command to do the "live reset".
Here is a sample sequence of commands to do a "live reset" to get some
clear idea.
Note that I am providing the examples based on the current patchset.

1. FW live reset is disabled in the device/adapter. Here adapter has 2
physical ports.

$ devlink dev
pci/0000:3b:00.0
pci/0000:3b:00.1
pci/0000:af:00.0
$ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
pci/0000:3b:00.0:
  name allow_fw_live_reset type generic
    values:
      cmode runtime value false
      cmode permanent value false
$ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
pci/0000:3b:00.1:
  name allow_fw_live_reset type generic
    values:
      cmode runtime value false
      cmode permanent value false

2. If a user issues "ethtool --reset p1p1 all", the device cannot
perform "live reset" as capability is not enabled. User needs to do a
driver reload, for firmware to undergo reset.

$ ethtool --reset p1p1 all
ETHTOOL_RESET 0xffffffff
Components reset:     0xff0000
Components not reset: 0xff00ffff
$ dmesg
[  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
[  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset

3. Now enable the capability in the device and reboot for device to
enable the capability. Firmware does not get reset just by setting the
param to true.

$ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
value true cmode permanent

4. After reboot, values of param.

$ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
pci/0000:3b:00.1:
  name allow_fw_live_reset type generic
    values:
      cmode runtime value true
      cmode permanent value true
$ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
pci/0000:3b:00.0:
  name allow_fw_live_reset type generic
    values:
      cmode runtime value true
      cmode permanent value true

5. Now issue the "ethtool --reset p1p1 all" and device will undergo
the "live reset". Reloading the driver is not required.

$ ethtool --reset p1p1 all
ETHTOOL_RESET 0xffffffff
Components reset:     0xff0000
Components not reset: 0xff00ffff
$ dmesg
[  117.432013] bnxt_en 0000:3b:00.0 p1p1: Firmware non-fatal reset
event received, max wait time 4200 msec
[  117.432015] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
[  117.432032] bnxt_en 0000:3b:00.1 p1p2: Firmware non-fatal reset
event received, max wait time 4200 msec
$ devlink health show pci/0000:3b:00.0 reporter fw_reset
pci/0000:3b:00.0:
  reporter fw_reset
    state healthy error 1 recover 1 grace_period 0 auto_recover true

6. If one of the host/PF turns off runtime param to false, "ethtool
--reset p1p1 all" behaves similar to step 2, until it turns it back
on.

$ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
value false cmode runtime
$ ethtool --reset p1p1 all
ETHTOOL_RESET 0xffffffff
Components reset:     0xff0000
Components not reset: 0xff00ffff
$ dmesg
[  327.610814] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
[  327.610828] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset

Thanks.
> >
> >
> > >
> > >2. allow_fw_live_reset - To indicate, if any of the host/PF turns it
> > >off, "live reset" is not allowed. This serves the purpose of what we
> > >are trying to add in runtime cmode.
> >
> > Yeah.
> And this param will enable the feature in the driver for driver to
> allow the firmware to go for "live reset", where as above param will
> enable the feature in NVRAM configuration of the device.
> >
> > >Do you want me to keep it as a driver-specific param?
> >
> > There is nothing driver-specific about this.
> okay.
> >
> >
> > >
> > >Please let me know if this is clear and makes less confusion.
> > >
> > >Thanks,
> > >Vasundhara
> > >
> > >> 2) devlink param that would indicate "I am okay if someone else (not by
> > >>    my devlink instance) resets my firmware".
> > >>
> > >> Could you please split?
> > >>
> > >>
> > >> >------------------------
> > >> >
> > >> >Thanks,
> > >> >Vasundhara
> > >> >>
> > >> >> >
> > >> >> >Thanks,
> > >> >> >Vasundhara
> > >> >> >>
> > >> >> >>
> > >> >> >>
> > >> >> >> >+       system.
> > >> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> > >> >> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
> > >> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
> > >> >> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
> > >> >> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
> > >> >> >> >+      },
> > >> >> >> > };
> > >> >> >> >
> > >> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
> > >> >> >> >--
> > >> >> >> >1.8.3.1
> > >> >> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jakub Kicinski]

On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:
> Here is a sample sequence of commands to do a "live reset" to get some
> clear idea.
> Note that I am providing the examples based on the current patchset.
> 
> 1. FW live reset is disabled in the device/adapter. Here adapter has 2
> physical ports.
> 
> $ devlink dev
> pci/0000:3b:00.0
> pci/0000:3b:00.1
> pci/0000:af:00.0
> $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
> pci/0000:3b:00.0:
>   name allow_fw_live_reset type generic
>     values:
>       cmode runtime value false
>       cmode permanent value false
> $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> pci/0000:3b:00.1:
>   name allow_fw_live_reset type generic
>     values:
>       cmode runtime value false
>       cmode permanent value false

What's the permanent value? What if after reboot the driver is too old
to change this, is the reset still allowed?

> 2. If a user issues "ethtool --reset p1p1 all", the device cannot
> perform "live reset" as capability is not enabled.
>
> User needs to do a driver reload, for firmware to undergo reset.

Why does driver reload have anything to do with resetting a potentially
MH device?

> $ ethtool --reset p1p1 all

Reset probably needs to be done via devlink. In any case you need a new
reset level for resetting MH devices and smartnics, because the current
reset mask covers port local, and host local cases, not any form of MH.

> ETHTOOL_RESET 0xffffffff
> Components reset:     0xff0000
> Components not reset: 0xff00ffff
> $ dmesg
> [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
> [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset

You said the reset was not performed, yet there is no information to
that effect in the log?!

> 3. Now enable the capability in the device and reboot for device to
> enable the capability. Firmware does not get reset just by setting the
> param to true.
> 
> $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> value true cmode permanent
> 
> 4. After reboot, values of param.

Is the reboot required here?

> $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> pci/0000:3b:00.1:
>   name allow_fw_live_reset type generic
>     values:
>       cmode runtime value true

Why is runtime value true now?

>       cmode permanent value true
> $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
> pci/0000:3b:00.0:
>   name allow_fw_live_reset type generic
>     values:
>       cmode runtime value true
>       cmode permanent value true
> 
> 5. Now issue the "ethtool --reset p1p1 all" and device will undergo
> the "live reset". Reloading the driver is not required.
> 
> $ ethtool --reset p1p1 all
> ETHTOOL_RESET 0xffffffff
> Components reset:     0xff0000
> Components not reset: 0xff00ffff
> $ dmesg
> [  117.432013] bnxt_en 0000:3b:00.0 p1p1: Firmware non-fatal reset
> event received, max wait time 4200 msec
> [  117.432015] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
> [  117.432032] bnxt_en 0000:3b:00.1 p1p2: Firmware non-fatal reset
> event received, max wait time 4200 msec
> $ devlink health show pci/0000:3b:00.0 reporter fw_reset
> pci/0000:3b:00.0:
>   reporter fw_reset
>     state healthy error 1 recover 1 grace_period 0 auto_recover true
> 
> 6. If one of the host/PF turns off runtime param to false, "ethtool
> --reset p1p1 all" behaves similar to step 2, until it turns it back
> on.
> 
> $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> value false cmode runtime
> $ ethtool --reset p1p1 all
> ETHTOOL_RESET 0xffffffff
> Components reset:     0xff0000
> Components not reset: 0xff00ffff
> $ dmesg
> [  327.610814] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
> [  327.610828] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Michael Chan]

On Wed, May 27, 2020 at 1:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:
> > Here is a sample sequence of commands to do a "live reset" to get some
> > clear idea.
> > Note that I am providing the examples based on the current patchset.
> >
> > 1. FW live reset is disabled in the device/adapter. Here adapter has 2
> > physical ports.
> >
> > $ devlink dev
> > pci/0000:3b:00.0
> > pci/0000:3b:00.1
> > pci/0000:af:00.0
> > $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
> > pci/0000:3b:00.0:
> >   name allow_fw_live_reset type generic
> >     values:
> >       cmode runtime value false
> >       cmode permanent value false
> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> > pci/0000:3b:00.1:
> >   name allow_fw_live_reset type generic
> >     values:
> >       cmode runtime value false
> >       cmode permanent value false
>
> What's the permanent value? What if after reboot the driver is too old
> to change this, is the reset still allowed?

The permanent value should be the NVRAM value.  If the NVRAM value is
false, the feature is always and unconditionally disabled.  If the
permanent value is true, the feature will only be available when all
loaded drivers indicate support for it and set the runtime value to
true.  If an old driver is loaded afterwards, it wouldn't indicate
support for this feature and it wouldn't set the runtime value to
true.  So the feature will not be available until the old driver is
unloaded or upgraded.

>
> > 2. If a user issues "ethtool --reset p1p1 all", the device cannot
> > perform "live reset" as capability is not enabled.
> >
> > User needs to do a driver reload, for firmware to undergo reset.
>
> Why does driver reload have anything to do with resetting a potentially
> MH device?

I think she meant that all drivers have to be unloaded before the
reset would take place in case it's a MH device since live reset is
not supported.  If it's a single function device, unloading this
driver is sufficient.

>
> > $ ethtool --reset p1p1 all
>
> Reset probably needs to be done via devlink. In any case you need a new
> reset level for resetting MH devices and smartnics, because the current
> reset mask covers port local, and host local cases, not any form of MH.

RIght.  This reset could be just a single function reset in this example.

>
> > ETHTOOL_RESET 0xffffffff
> > Components reset:     0xff0000
> > Components not reset: 0xff00ffff
> > $ dmesg
> > [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
> > [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset
>
> You said the reset was not performed, yet there is no information to
> that effect in the log?!

The firmware has been requested to reset, but the reset hasn't taken
place yet because live reset cannot be done.  We can make the logs
more clear.

>
> > 3. Now enable the capability in the device and reboot for device to
> > enable the capability. Firmware does not get reset just by setting the
> > param to true.
> >
> > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> > value true cmode permanent
> >
> > 4. After reboot, values of param.
>
> Is the reboot required here?
>

In general, our new NVRAM permanent parameters will take effect after
reset (or reboot).

> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> > pci/0000:3b:00.1:
> >   name allow_fw_live_reset type generic
> >     values:
> >       cmode runtime value true
>
> Why is runtime value true now?
>

If the permanent (NVRAM) parameter is true, all loaded new drivers
will indicate support for this feature and set the runtime value to
true by default.  The runtime value would not be true if any loaded
driver is too old or has set the runtime value to false.

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jakub Kicinski]

On Wed, 27 May 2020 13:57:11 -0700 Michael Chan wrote:
> On Wed, May 27, 2020 at 1:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:  
> > > Here is a sample sequence of commands to do a "live reset" to get some
> > > clear idea.
> > > Note that I am providing the examples based on the current patchset.
> > >
> > > 1. FW live reset is disabled in the device/adapter. Here adapter has 2
> > > physical ports.
> > >
> > > $ devlink dev
> > > pci/0000:3b:00.0
> > > pci/0000:3b:00.1
> > > pci/0000:af:00.0
> > > $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
> > > pci/0000:3b:00.0:
> > >   name allow_fw_live_reset type generic
> > >     values:
> > >       cmode runtime value false
> > >       cmode permanent value false
> > > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> > > pci/0000:3b:00.1:
> > >   name allow_fw_live_reset type generic
> > >     values:
> > >       cmode runtime value false
> > >       cmode permanent value false  
> >
> > What's the permanent value? What if after reboot the driver is too old
> > to change this, is the reset still allowed?  
> 
> The permanent value should be the NVRAM value.  If the NVRAM value is
> false, the feature is always and unconditionally disabled.  If the
> permanent value is true, the feature will only be available when all
> loaded drivers indicate support for it and set the runtime value to
> true.  If an old driver is loaded afterwards, it wouldn't indicate
> support for this feature and it wouldn't set the runtime value to
> true.  So the feature will not be available until the old driver is
> unloaded or upgraded.

Setting this permanent value to false makes the FW's life easier?
Otherwise why not always have it enabled and just depend on hosts 
not opting in?

> > > 2. If a user issues "ethtool --reset p1p1 all", the device cannot
> > > perform "live reset" as capability is not enabled.
> > >
> > > User needs to do a driver reload, for firmware to undergo reset.  
> >
> > Why does driver reload have anything to do with resetting a potentially
> > MH device?  
> 
> I think she meant that all drivers have to be unloaded before the
> reset would take place in case it's a MH device since live reset is
> not supported.  If it's a single function device, unloading this
> driver is sufficient.

I see.

> > > $ ethtool --reset p1p1 all  
> >
> > Reset probably needs to be done via devlink. In any case you need a new
> > reset level for resetting MH devices and smartnics, because the current
> > reset mask covers port local, and host local cases, not any form of MH.  
> 
> RIght.  This reset could be just a single function reset in this example.

Well, for the single host scenario the parameter dance is not at all
needed, since there is only one domain of control. If user can issue a
reset they can as well change the value of the param or even reload the
driver. The runtime parameter only makes sense in MH/SmartNIC scenario,
so IMHO the param and devlink reset are strongly dependent.

> > > ETHTOOL_RESET 0xffffffff
> > > Components reset:     0xff0000
> > > Components not reset: 0xff00ffff
> > > $ dmesg
> > > [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
> > > [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset  
> >
> > You said the reset was not performed, yet there is no information to
> > that effect in the log?!  
> 
> The firmware has been requested to reset, but the reset hasn't taken
> place yet because live reset cannot be done.  We can make the logs
> more clear.

Thanks

> > > 3. Now enable the capability in the device and reboot for device to
> > > enable the capability. Firmware does not get reset just by setting the
> > > param to true.
> > >
> > > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> > > value true cmode permanent
> > >
> > > 4. After reboot, values of param.  
> >
> > Is the reboot required here?
> 
> In general, our new NVRAM permanent parameters will take effect after
> reset (or reboot).
>
> > > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> > > pci/0000:3b:00.1:
> > >   name allow_fw_live_reset type generic
> > >     values:
> > >       cmode runtime value true  
> >
> > Why is runtime value true now?
> >  
> 
> If the permanent (NVRAM) parameter is true, all loaded new drivers
> will indicate support for this feature and set the runtime value to
> true by default.  The runtime value would not be true if any loaded
> driver is too old or has set the runtime value to false.

Okay, the parameter has a bit of a dual role as it controls whether the
feature is available (false -> true transition requiring a reset/reboot)
and the default setting of the runtime parameter. Let's document that
more clearly.

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Thu, May 28, 2020 at 2:46 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 27 May 2020 13:57:11 -0700 Michael Chan wrote:
> > On Wed, May 27, 2020 at 1:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:
> > > > Here is a sample sequence of commands to do a "live reset" to get some
> > > > clear idea.
> > > > Note that I am providing the examples based on the current patchset.
> > > >
> > > > 1. FW live reset is disabled in the device/adapter. Here adapter has 2
> > > > physical ports.
> > > >
> > > > $ devlink dev
> > > > pci/0000:3b:00.0
> > > > pci/0000:3b:00.1
> > > > pci/0000:af:00.0
> > > > $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
> > > > pci/0000:3b:00.0:
> > > >   name allow_fw_live_reset type generic
> > > >     values:
> > > >       cmode runtime value false
> > > >       cmode permanent value false
> > > > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> > > > pci/0000:3b:00.1:
> > > >   name allow_fw_live_reset type generic
> > > >     values:
> > > >       cmode runtime value false
> > > >       cmode permanent value false
> > >
> > > What's the permanent value? What if after reboot the driver is too old
> > > to change this, is the reset still allowed?
> >
> > The permanent value should be the NVRAM value.  If the NVRAM value is
> > false, the feature is always and unconditionally disabled.  If the
> > permanent value is true, the feature will only be available when all
> > loaded drivers indicate support for it and set the runtime value to
> > true.  If an old driver is loaded afterwards, it wouldn't indicate
> > support for this feature and it wouldn't set the runtime value to
> > true.  So the feature will not be available until the old driver is
> > unloaded or upgraded.
>
> Setting this permanent value to false makes the FW's life easier?

It just disables the feature.

> Otherwise why not always have it enabled and just depend on hosts
> not opting in?

We are providing permanent value as a flexibility to user. We can
remove it, if it makes things easy and clear.

>
> > > > 2. If a user issues "ethtool --reset p1p1 all", the device cannot
> > > > perform "live reset" as capability is not enabled.
> > > >
> > > > User needs to do a driver reload, for firmware to undergo reset.
> > >
> > > Why does driver reload have anything to do with resetting a potentially
> > > MH device?
> >
> > I think she meant that all drivers have to be unloaded before the
> > reset would take place in case it's a MH device since live reset is
> > not supported.  If it's a single function device, unloading this
> > driver is sufficient.
yes.

>
> I see.
>
> > > > $ ethtool --reset p1p1 all
> > >
> > > Reset probably needs to be done via devlink. In any case you need a new
> > > reset level for resetting MH devices and smartnics, because the current
> > > reset mask covers port local, and host local cases, not any form of MH.
> >
> > RIght.  This reset could be just a single function reset in this example.
>
> Well, for the single host scenario the parameter dance is not at all
> needed, since there is only one domain of control. If user can issue a
> reset they can as well change the value of the param or even reload the
> driver. The runtime parameter only makes sense in MH/SmartNIC scenario,
> so IMHO the param and devlink reset are strongly dependent.
>
> > > > ETHTOOL_RESET 0xffffffff
> > > > Components reset:     0xff0000
> > > > Components not reset: 0xff00ffff
> > > > $ dmesg
> > > > [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
> > > > [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset
> > >
> > > You said the reset was not performed, yet there is no information to
> > > that effect in the log?!
> >
> > The firmware has been requested to reset, but the reset hasn't taken
> > place yet because live reset cannot be done.  We can make the logs
> > more clear.
>
> Thanks
>
> > > > 3. Now enable the capability in the device and reboot for device to
> > > > enable the capability. Firmware does not get reset just by setting the
> > > > param to true.
> > > >
> > > > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> > > > value true cmode permanent
> > > >
> > > > 4. After reboot, values of param.
> > >
> > > Is the reboot required here?
> >
> > In general, our new NVRAM permanent parameters will take effect after
> > reset (or reboot).
> >
> > > > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> > > > pci/0000:3b:00.1:
> > > >   name allow_fw_live_reset type generic
> > > >     values:
> > > >       cmode runtime value true
> > >
> > > Why is runtime value true now?
> > >
> >
> > If the permanent (NVRAM) parameter is true, all loaded new drivers
> > will indicate support for this feature and set the runtime value to
> > true by default.  The runtime value would not be true if any loaded
> > driver is too old or has set the runtime value to false.
>
> Okay, the parameter has a bit of a dual role as it controls whether the
> feature is available (false -> true transition requiring a reset/reboot)
> and the default setting of the runtime parameter. Let's document that
> more clearly.
Please look at the 3/4 patch for more documentation in the bnxt.rst
file. We can add more documentation, if needed, in the bnxt.rst file.

Thanks.

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jakub Kicinski]

On Thu, 28 May 2020 07:20:00 +0530 Vasundhara Volam wrote:
> > > The permanent value should be the NVRAM value.  If the NVRAM value is
> > > false, the feature is always and unconditionally disabled.  If the
> > > permanent value is true, the feature will only be available when all
> > > loaded drivers indicate support for it and set the runtime value to
> > > true.  If an old driver is loaded afterwards, it wouldn't indicate
> > > support for this feature and it wouldn't set the runtime value to
> > > true.  So the feature will not be available until the old driver is
> > > unloaded or upgraded.  
> >
> > Setting this permanent value to false makes the FW's life easier?  
> 
> It just disables the feature.
> 
> > Otherwise why not always have it enabled and just depend on hosts
> > not opting in?  
> 
> We are providing permanent value as a flexibility to user. We can
> remove it, if it makes things easy and clear.

I'd think less knobs means less to understand for the user and less
to test for the vendor. If disabling the feature doesn't buy FW
anything then perhaps it can just serve the purpose of setting the
default?

> > > > > 3. Now enable the capability in the device and reboot for device to
> > > > > enable the capability. Firmware does not get reset just by setting the
> > > > > param to true.
> > > > >
> > > > > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> > > > > value true cmode permanent
> > > > >
> > > > > 4. After reboot, values of param.  
> > > >
> > > > Is the reboot required here?  
> > >
> > > In general, our new NVRAM permanent parameters will take effect after
> > > reset (or reboot).
> > >  
> > > > > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> > > > > pci/0000:3b:00.1:
> > > > >   name allow_fw_live_reset type generic
> > > > >     values:
> > > > >       cmode runtime value true  
> > > >
> > > > Why is runtime value true now?
> > > >  
> > >
> > > If the permanent (NVRAM) parameter is true, all loaded new drivers
> > > will indicate support for this feature and set the runtime value to
> > > true by default.  The runtime value would not be true if any loaded
> > > driver is too old or has set the runtime value to false.  
> >
> > Okay, the parameter has a bit of a dual role as it controls whether the
> > feature is available (false -> true transition requiring a reset/reboot)
> > and the default setting of the runtime parameter. Let's document that
> > more clearly.  
> Please look at the 3/4 patch for more documentation in the bnxt.rst
> file. We can add more documentation, if needed, in the bnxt.rst file.

Ack, I read that, but it wasn't nearly clear enough. The command
examples helped much more.  I think the doc should be expanded.

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Fri, May 29, 2020 at 12:35 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 28 May 2020 07:20:00 +0530 Vasundhara Volam wrote:
> > > > The permanent value should be the NVRAM value.  If the NVRAM value is
> > > > false, the feature is always and unconditionally disabled.  If the
> > > > permanent value is true, the feature will only be available when all
> > > > loaded drivers indicate support for it and set the runtime value to
> > > > true.  If an old driver is loaded afterwards, it wouldn't indicate
> > > > support for this feature and it wouldn't set the runtime value to
> > > > true.  So the feature will not be available until the old driver is
> > > > unloaded or upgraded.
> > >
> > > Setting this permanent value to false makes the FW's life easier?
> >
> > It just disables the feature.
> >
> > > Otherwise why not always have it enabled and just depend on hosts
> > > not opting in?
> >
> > We are providing permanent value as a flexibility to user. We can
> > remove it, if it makes things easy and clear.
>
> I'd think less knobs means less to understand for the user and less
> to test for the vendor. If disabling the feature doesn't buy FW
> anything then perhaps it can just serve the purpose of setting the
> default?
>
> > > > > > 3. Now enable the capability in the device and reboot for device to
> > > > > > enable the capability. Firmware does not get reset just by setting the
> > > > > > param to true.
> > > > > >
> > > > > > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> > > > > > value true cmode permanent
> > > > > >
> > > > > > 4. After reboot, values of param.
> > > > >
> > > > > Is the reboot required here?
> > > >
> > > > In general, our new NVRAM permanent parameters will take effect after
> > > > reset (or reboot).
> > > >
> > > > > > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> > > > > > pci/0000:3b:00.1:
> > > > > >   name allow_fw_live_reset type generic
> > > > > >     values:
> > > > > >       cmode runtime value true
> > > > >
> > > > > Why is runtime value true now?
> > > > >
> > > >
> > > > If the permanent (NVRAM) parameter is true, all loaded new drivers
> > > > will indicate support for this feature and set the runtime value to
> > > > true by default.  The runtime value would not be true if any loaded
> > > > driver is too old or has set the runtime value to false.
> > >
> > > Okay, the parameter has a bit of a dual role as it controls whether the
> > > feature is available (false -> true transition requiring a reset/reboot)
> > > and the default setting of the runtime parameter. Let's document that
> > > more clearly.
> > Please look at the 3/4 patch for more documentation in the bnxt.rst
> > file. We can add more documentation, if needed, in the bnxt.rst file.
>
> Ack, I read that, but it wasn't nearly clear enough. The command
> examples helped much more.  I think the doc should be expanded.
Thank you for looking into it. I will soon send out the next version
of patchset with expanded documentation and param is split into two
with better renaming.

Thanks.

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Wed, May 27, 2020 at 11:16:08PM CEST, kuba@kernel.org wrote:
>On Wed, 27 May 2020 13:57:11 -0700 Michael Chan wrote:
>> On Wed, May 27, 2020 at 1:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
>> > On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:  
>> > > Here is a sample sequence of commands to do a "live reset" to get some
>> > > clear idea.
>> > > Note that I am providing the examples based on the current patchset.
>> > >
>> > > 1. FW live reset is disabled in the device/adapter. Here adapter has 2
>> > > physical ports.
>> > >
>> > > $ devlink dev
>> > > pci/0000:3b:00.0
>> > > pci/0000:3b:00.1
>> > > pci/0000:af:00.0
>> > > $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
>> > > pci/0000:3b:00.0:
>> > >   name allow_fw_live_reset type generic
>> > >     values:
>> > >       cmode runtime value false
>> > >       cmode permanent value false
>> > > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
>> > > pci/0000:3b:00.1:
>> > >   name allow_fw_live_reset type generic
>> > >     values:
>> > >       cmode runtime value false
>> > >       cmode permanent value false  
>> >
>> > What's the permanent value? What if after reboot the driver is too old
>> > to change this, is the reset still allowed?  
>> 
>> The permanent value should be the NVRAM value.  If the NVRAM value is
>> false, the feature is always and unconditionally disabled.  If the
>> permanent value is true, the feature will only be available when all
>> loaded drivers indicate support for it and set the runtime value to
>> true.  If an old driver is loaded afterwards, it wouldn't indicate
>> support for this feature and it wouldn't set the runtime value to
>> true.  So the feature will not be available until the old driver is
>> unloaded or upgraded.
>
>Setting this permanent value to false makes the FW's life easier?
>Otherwise why not always have it enabled and just depend on hosts 
>not opting in?
>
>> > > 2. If a user issues "ethtool --reset p1p1 all", the device cannot
>> > > perform "live reset" as capability is not enabled.
>> > >
>> > > User needs to do a driver reload, for firmware to undergo reset.  
>> >
>> > Why does driver reload have anything to do with resetting a potentially
>> > MH device?  
>> 
>> I think she meant that all drivers have to be unloaded before the
>> reset would take place in case it's a MH device since live reset is
>> not supported.  If it's a single function device, unloading this
>> driver is sufficient.
>
>I see.
>
>> > > $ ethtool --reset p1p1 all  
>> >
>> > Reset probably needs to be done via devlink. In any case you need a new
>> > reset level for resetting MH devices and smartnics, because the current
>> > reset mask covers port local, and host local cases, not any form of MH.  
>> 
>> RIght.  This reset could be just a single function reset in this example.
>
>Well, for the single host scenario the parameter dance is not at all
>needed, since there is only one domain of control. If user can issue a
>reset they can as well change the value of the param or even reload the
>driver. The runtime parameter only makes sense in MH/SmartNIC scenario,
>so IMHO the param and devlink reset are strongly dependent.
>
>> > > ETHTOOL_RESET 0xffffffff
>> > > Components reset:     0xff0000
>> > > Components not reset: 0xff00ffff
>> > > $ dmesg
>> > > [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
>> > > [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset  
>> >
>> > You said the reset was not performed, yet there is no information to
>> > that effect in the log?!  
>> 
>> The firmware has been requested to reset, but the reset hasn't taken
>> place yet because live reset cannot be done.  We can make the logs
>> more clear.
>
>Thanks
>
>> > > 3. Now enable the capability in the device and reboot for device to
>> > > enable the capability. Firmware does not get reset just by setting the
>> > > param to true.
>> > >
>> > > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
>> > > value true cmode permanent
>> > >
>> > > 4. After reboot, values of param.  
>> >
>> > Is the reboot required here?
>> 
>> In general, our new NVRAM permanent parameters will take effect after
>> reset (or reboot).
>>
>> > > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
>> > > pci/0000:3b:00.1:
>> > >   name allow_fw_live_reset type generic
>> > >     values:
>> > >       cmode runtime value true  
>> >
>> > Why is runtime value true now?
>> >  
>> 
>> If the permanent (NVRAM) parameter is true, all loaded new drivers
>> will indicate support for this feature and set the runtime value to
>> true by default.  The runtime value would not be true if any loaded
>> driver is too old or has set the runtime value to false.
>
>Okay, the parameter has a bit of a dual role as it controls whether the
>feature is available (false -> true transition requiring a reset/reboot)
>and the default setting of the runtime parameter. Let's document that
>more clearly.

Or, don't use the param for indication as I suggested in the other
email...

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Wed, May 27, 2020 at 10:57:11PM CEST, michael.chan@broadcom.com wrote:
>On Wed, May 27, 2020 at 1:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:
>> > Here is a sample sequence of commands to do a "live reset" to get some
>> > clear idea.
>> > Note that I am providing the examples based on the current patchset.
>> >
>> > 1. FW live reset is disabled in the device/adapter. Here adapter has 2
>> > physical ports.
>> >
>> > $ devlink dev
>> > pci/0000:3b:00.0
>> > pci/0000:3b:00.1
>> > pci/0000:af:00.0
>> > $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
>> > pci/0000:3b:00.0:
>> >   name allow_fw_live_reset type generic
>> >     values:
>> >       cmode runtime value false
>> >       cmode permanent value false
>> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
>> > pci/0000:3b:00.1:
>> >   name allow_fw_live_reset type generic
>> >     values:
>> >       cmode runtime value false
>> >       cmode permanent value false
>>
>> What's the permanent value? What if after reboot the driver is too old
>> to change this, is the reset still allowed?
>
>The permanent value should be the NVRAM value.  If the NVRAM value is
>false, the feature is always and unconditionally disabled.  If the
>permanent value is true, the feature will only be available when all
>loaded drivers indicate support for it and set the runtime value to
>true.  If an old driver is loaded afterwards, it wouldn't indicate
>support for this feature and it wouldn't set the runtime value to
>true.  So the feature will not be available until the old driver is
>unloaded or upgraded.
>
>>
>> > 2. If a user issues "ethtool --reset p1p1 all", the device cannot
>> > perform "live reset" as capability is not enabled.
>> >
>> > User needs to do a driver reload, for firmware to undergo reset.
>>
>> Why does driver reload have anything to do with resetting a potentially
>> MH device?
>
>I think she meant that all drivers have to be unloaded before the
>reset would take place in case it's a MH device since live reset is
>not supported.  If it's a single function device, unloading this
>driver is sufficient.
>
>>
>> > $ ethtool --reset p1p1 all
>>
>> Reset probably needs to be done via devlink. In any case you need a new
>> reset level for resetting MH devices and smartnics, because the current
>> reset mask covers port local, and host local cases, not any form of MH.
>
>RIght.  This reset could be just a single function reset in this example.
>
>>
>> > ETHTOOL_RESET 0xffffffff
>> > Components reset:     0xff0000
>> > Components not reset: 0xff00ffff
>> > $ dmesg
>> > [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
>> > [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset
>>
>> You said the reset was not performed, yet there is no information to
>> that effect in the log?!
>
>The firmware has been requested to reset, but the reset hasn't taken
>place yet because live reset cannot be done.  We can make the logs
>more clear.
>
>>
>> > 3. Now enable the capability in the device and reboot for device to
>> > enable the capability. Firmware does not get reset just by setting the
>> > param to true.
>> >
>> > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
>> > value true cmode permanent
>> >
>> > 4. After reboot, values of param.
>>
>> Is the reboot required here?
>>
>
>In general, our new NVRAM permanent parameters will take effect after
>reset (or reboot).

Ah, you need a reboot. I was not expecting that :/ So the "devlink dev
reload" attr would not work for you. MLNX hardware can change this on
runtime.


>
>> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
>> > pci/0000:3b:00.1:
>> >   name allow_fw_live_reset type generic
>> >     values:
>> >       cmode runtime value true
>>
>> Why is runtime value true now?
>>
>
>If the permanent (NVRAM) parameter is true, all loaded new drivers
>will indicate support for this feature and set the runtime value to
>true by default.  The runtime value would not be true if any loaded
>driver is too old or has set the runtime value to false.

This is a bit odd. It is a configuration, not an indication. When you
want to indicate what you support something, I think it should be done
in a different place. I think that "devlink dev info" is the place to
put it, I think that we need "capabilities" there.

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Mon, Jun 1, 2020 at 12:09 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wed, May 27, 2020 at 10:57:11PM CEST, michael.chan@broadcom.com wrote:
> >On Wed, May 27, 2020 at 1:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >>
> >> On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:
> >> > Here is a sample sequence of commands to do a "live reset" to get some
> >> > clear idea.
> >> > Note that I am providing the examples based on the current patchset.
> >> >
> >> > 1. FW live reset is disabled in the device/adapter. Here adapter has 2
> >> > physical ports.
> >> >
> >> > $ devlink dev
> >> > pci/0000:3b:00.0
> >> > pci/0000:3b:00.1
> >> > pci/0000:af:00.0
> >> > $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
> >> > pci/0000:3b:00.0:
> >> >   name allow_fw_live_reset type generic
> >> >     values:
> >> >       cmode runtime value false
> >> >       cmode permanent value false
> >> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> >> > pci/0000:3b:00.1:
> >> >   name allow_fw_live_reset type generic
> >> >     values:
> >> >       cmode runtime value false
> >> >       cmode permanent value false
> >>
> >> What's the permanent value? What if after reboot the driver is too old
> >> to change this, is the reset still allowed?
> >
> >The permanent value should be the NVRAM value.  If the NVRAM value is
> >false, the feature is always and unconditionally disabled.  If the
> >permanent value is true, the feature will only be available when all
> >loaded drivers indicate support for it and set the runtime value to
> >true.  If an old driver is loaded afterwards, it wouldn't indicate
> >support for this feature and it wouldn't set the runtime value to
> >true.  So the feature will not be available until the old driver is
> >unloaded or upgraded.
> >
> >>
> >> > 2. If a user issues "ethtool --reset p1p1 all", the device cannot
> >> > perform "live reset" as capability is not enabled.
> >> >
> >> > User needs to do a driver reload, for firmware to undergo reset.
> >>
> >> Why does driver reload have anything to do with resetting a potentially
> >> MH device?
> >
> >I think she meant that all drivers have to be unloaded before the
> >reset would take place in case it's a MH device since live reset is
> >not supported.  If it's a single function device, unloading this
> >driver is sufficient.
> >
> >>
> >> > $ ethtool --reset p1p1 all
> >>
> >> Reset probably needs to be done via devlink. In any case you need a new
> >> reset level for resetting MH devices and smartnics, because the current
> >> reset mask covers port local, and host local cases, not any form of MH.
> >
> >RIght.  This reset could be just a single function reset in this example.
> >
> >>
> >> > ETHTOOL_RESET 0xffffffff
> >> > Components reset:     0xff0000
> >> > Components not reset: 0xff00ffff
> >> > $ dmesg
> >> > [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
> >> > [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset
> >>
> >> You said the reset was not performed, yet there is no information to
> >> that effect in the log?!
> >
> >The firmware has been requested to reset, but the reset hasn't taken
> >place yet because live reset cannot be done.  We can make the logs
> >more clear.
> >
> >>
> >> > 3. Now enable the capability in the device and reboot for device to
> >> > enable the capability. Firmware does not get reset just by setting the
> >> > param to true.
> >> >
> >> > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> >> > value true cmode permanent
> >> >
> >> > 4. After reboot, values of param.
> >>
> >> Is the reboot required here?
> >>
> >
> >In general, our new NVRAM permanent parameters will take effect after
> >reset (or reboot).
>
> Ah, you need a reboot. I was not expecting that :/ So the "devlink dev
> reload" attr would not work for you. MLNX hardware can change this on
> runtime.

NVRAM parameter configuration will take effect only on reboot or on
"live reset" (except few). But to enable "live reset", system needs a
reboot.
>
>
> >
> >> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> >> > pci/0000:3b:00.1:
> >> >   name allow_fw_live_reset type generic
> >> >     values:
> >> >       cmode runtime value true
> >>
> >> Why is runtime value true now?
> >>
> >
> >If the permanent (NVRAM) parameter is true, all loaded new drivers
> >will indicate support for this feature and set the runtime value to
> >true by default.  The runtime value would not be true if any loaded
> >driver is too old or has set the runtime value to false.
>
> This is a bit odd. It is a configuration, not an indication. When you
> want to indicate what you support something, I think it should be done
> in a different place. I think that "devlink dev info" is the place to
> put it, I think that we need "capabilities" there.
>
Indication can be shown in 'devlink dev info', but users can configure
this parameter also to control the 'live reset' at runtime.

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Mon, Jun 01, 2020 at 10:50:50AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Mon, Jun 1, 2020 at 12:09 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Wed, May 27, 2020 at 10:57:11PM CEST, michael.chan@broadcom.com wrote:
>> >On Wed, May 27, 2020 at 1:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
>> >>
>> >> On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:
>> >> > Here is a sample sequence of commands to do a "live reset" to get some
>> >> > clear idea.
>> >> > Note that I am providing the examples based on the current patchset.
>> >> >
>> >> > 1. FW live reset is disabled in the device/adapter. Here adapter has 2
>> >> > physical ports.
>> >> >
>> >> > $ devlink dev
>> >> > pci/0000:3b:00.0
>> >> > pci/0000:3b:00.1
>> >> > pci/0000:af:00.0
>> >> > $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
>> >> > pci/0000:3b:00.0:
>> >> >   name allow_fw_live_reset type generic
>> >> >     values:
>> >> >       cmode runtime value false
>> >> >       cmode permanent value false
>> >> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
>> >> > pci/0000:3b:00.1:
>> >> >   name allow_fw_live_reset type generic
>> >> >     values:
>> >> >       cmode runtime value false
>> >> >       cmode permanent value false
>> >>
>> >> What's the permanent value? What if after reboot the driver is too old
>> >> to change this, is the reset still allowed?
>> >
>> >The permanent value should be the NVRAM value.  If the NVRAM value is
>> >false, the feature is always and unconditionally disabled.  If the
>> >permanent value is true, the feature will only be available when all
>> >loaded drivers indicate support for it and set the runtime value to
>> >true.  If an old driver is loaded afterwards, it wouldn't indicate
>> >support for this feature and it wouldn't set the runtime value to
>> >true.  So the feature will not be available until the old driver is
>> >unloaded or upgraded.
>> >
>> >>
>> >> > 2. If a user issues "ethtool --reset p1p1 all", the device cannot
>> >> > perform "live reset" as capability is not enabled.
>> >> >
>> >> > User needs to do a driver reload, for firmware to undergo reset.
>> >>
>> >> Why does driver reload have anything to do with resetting a potentially
>> >> MH device?
>> >
>> >I think she meant that all drivers have to be unloaded before the
>> >reset would take place in case it's a MH device since live reset is
>> >not supported.  If it's a single function device, unloading this
>> >driver is sufficient.
>> >
>> >>
>> >> > $ ethtool --reset p1p1 all
>> >>
>> >> Reset probably needs to be done via devlink. In any case you need a new
>> >> reset level for resetting MH devices and smartnics, because the current
>> >> reset mask covers port local, and host local cases, not any form of MH.
>> >
>> >RIght.  This reset could be just a single function reset in this example.
>> >
>> >>
>> >> > ETHTOOL_RESET 0xffffffff
>> >> > Components reset:     0xff0000
>> >> > Components not reset: 0xff00ffff
>> >> > $ dmesg
>> >> > [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
>> >> > [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset
>> >>
>> >> You said the reset was not performed, yet there is no information to
>> >> that effect in the log?!
>> >
>> >The firmware has been requested to reset, but the reset hasn't taken
>> >place yet because live reset cannot be done.  We can make the logs
>> >more clear.
>> >
>> >>
>> >> > 3. Now enable the capability in the device and reboot for device to
>> >> > enable the capability. Firmware does not get reset just by setting the
>> >> > param to true.
>> >> >
>> >> > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
>> >> > value true cmode permanent
>> >> >
>> >> > 4. After reboot, values of param.
>> >>
>> >> Is the reboot required here?
>> >>
>> >
>> >In general, our new NVRAM permanent parameters will take effect after
>> >reset (or reboot).
>>
>> Ah, you need a reboot. I was not expecting that :/ So the "devlink dev
>> reload" attr would not work for you. MLNX hardware can change this on
>> runtime.
>
>NVRAM parameter configuration will take effect only on reboot or on
>"live reset" (except few). But to enable "live reset", system needs a
>reboot.
>>
>>
>> >
>> >> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
>> >> > pci/0000:3b:00.1:
>> >> >   name allow_fw_live_reset type generic
>> >> >     values:
>> >> >       cmode runtime value true
>> >>
>> >> Why is runtime value true now?
>> >>
>> >
>> >If the permanent (NVRAM) parameter is true, all loaded new drivers
>> >will indicate support for this feature and set the runtime value to
>> >true by default.  The runtime value would not be true if any loaded
>> >driver is too old or has set the runtime value to false.
>>
>> This is a bit odd. It is a configuration, not an indication. When you
>> want to indicate what you support something, I think it should be done
>> in a different place. I think that "devlink dev info" is the place to
>> put it, I think that we need "capabilities" there.
>>
>Indication can be shown in 'devlink dev info', but users can configure
>this parameter also to control the 'live reset' at runtime.

I'm totally confused. You wrote above that you need reboot (cmode
permanent) for that. What is the usecase for cmode runtime?

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Mon, Jun 1, 2020 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 01, 2020 at 10:50:50AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Mon, Jun 1, 2020 at 12:09 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Wed, May 27, 2020 at 10:57:11PM CEST, michael.chan@broadcom.com wrote:
> >> >On Wed, May 27, 2020 at 1:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >> >>
> >> >> On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:
> >> >> > Here is a sample sequence of commands to do a "live reset" to get some
> >> >> > clear idea.
> >> >> > Note that I am providing the examples based on the current patchset.
> >> >> >
> >> >> > 1. FW live reset is disabled in the device/adapter. Here adapter has 2
> >> >> > physical ports.
> >> >> >
> >> >> > $ devlink dev
> >> >> > pci/0000:3b:00.0
> >> >> > pci/0000:3b:00.1
> >> >> > pci/0000:af:00.0
> >> >> > $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
> >> >> > pci/0000:3b:00.0:
> >> >> >   name allow_fw_live_reset type generic
> >> >> >     values:
> >> >> >       cmode runtime value false
> >> >> >       cmode permanent value false
> >> >> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> >> >> > pci/0000:3b:00.1:
> >> >> >   name allow_fw_live_reset type generic
> >> >> >     values:
> >> >> >       cmode runtime value false
> >> >> >       cmode permanent value false
> >> >>
> >> >> What's the permanent value? What if after reboot the driver is too old
> >> >> to change this, is the reset still allowed?
> >> >
> >> >The permanent value should be the NVRAM value.  If the NVRAM value is
> >> >false, the feature is always and unconditionally disabled.  If the
> >> >permanent value is true, the feature will only be available when all
> >> >loaded drivers indicate support for it and set the runtime value to
> >> >true.  If an old driver is loaded afterwards, it wouldn't indicate
> >> >support for this feature and it wouldn't set the runtime value to
> >> >true.  So the feature will not be available until the old driver is
> >> >unloaded or upgraded.
> >> >
> >> >>
> >> >> > 2. If a user issues "ethtool --reset p1p1 all", the device cannot
> >> >> > perform "live reset" as capability is not enabled.
> >> >> >
> >> >> > User needs to do a driver reload, for firmware to undergo reset.
> >> >>
> >> >> Why does driver reload have anything to do with resetting a potentially
> >> >> MH device?
> >> >
> >> >I think she meant that all drivers have to be unloaded before the
> >> >reset would take place in case it's a MH device since live reset is
> >> >not supported.  If it's a single function device, unloading this
> >> >driver is sufficient.
> >> >
> >> >>
> >> >> > $ ethtool --reset p1p1 all
> >> >>
> >> >> Reset probably needs to be done via devlink. In any case you need a new
> >> >> reset level for resetting MH devices and smartnics, because the current
> >> >> reset mask covers port local, and host local cases, not any form of MH.
> >> >
> >> >RIght.  This reset could be just a single function reset in this example.
> >> >
> >> >>
> >> >> > ETHTOOL_RESET 0xffffffff
> >> >> > Components reset:     0xff0000
> >> >> > Components not reset: 0xff00ffff
> >> >> > $ dmesg
> >> >> > [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
> >> >> > [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset
> >> >>
> >> >> You said the reset was not performed, yet there is no information to
> >> >> that effect in the log?!
> >> >
> >> >The firmware has been requested to reset, but the reset hasn't taken
> >> >place yet because live reset cannot be done.  We can make the logs
> >> >more clear.
> >> >
> >> >>
> >> >> > 3. Now enable the capability in the device and reboot for device to
> >> >> > enable the capability. Firmware does not get reset just by setting the
> >> >> > param to true.
> >> >> >
> >> >> > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> >> >> > value true cmode permanent
> >> >> >
> >> >> > 4. After reboot, values of param.
> >> >>
> >> >> Is the reboot required here?
> >> >>
> >> >
> >> >In general, our new NVRAM permanent parameters will take effect after
> >> >reset (or reboot).
> >>
> >> Ah, you need a reboot. I was not expecting that :/ So the "devlink dev
> >> reload" attr would not work for you. MLNX hardware can change this on
> >> runtime.
> >
> >NVRAM parameter configuration will take effect only on reboot or on
> >"live reset" (except few). But to enable "live reset", system needs a
> >reboot.
> >>
> >>
> >> >
> >> >> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> >> >> > pci/0000:3b:00.1:
> >> >> >   name allow_fw_live_reset type generic
> >> >> >     values:
> >> >> >       cmode runtime value true
> >> >>
> >> >> Why is runtime value true now?
> >> >>
> >> >
> >> >If the permanent (NVRAM) parameter is true, all loaded new drivers
> >> >will indicate support for this feature and set the runtime value to
> >> >true by default.  The runtime value would not be true if any loaded
> >> >driver is too old or has set the runtime value to false.
> >>
> >> This is a bit odd. It is a configuration, not an indication. When you
> >> want to indicate what you support something, I think it should be done
> >> in a different place. I think that "devlink dev info" is the place to
> >> put it, I think that we need "capabilities" there.
> >>
> >Indication can be shown in 'devlink dev info', but users can configure
> >this parameter also to control the 'live reset' at runtime.
>
> I'm totally confused. You wrote above that you need reboot (cmode
> permanent) for that. What is the usecase for cmode runtime?
>

Okay let me brief it completely, probably some repetition.

Permanent cmode, is to enable the "live reset" feature in the device,
which takes effect after reboot, as it is configuration the parameter
in NVRAM configuration. After reboot, if the parameter is enabled,
user can initiate the live reset from a separate command.

Runtime cmode, default value is true and available only when permanent
cmode is true. This allows the user to temporarily disallow the "live
reset", if the host is running any mission critical application.

Thanks.

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Mon, Jun 01, 2020 at 11:57:28AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Mon, Jun 1, 2020 at 3:19 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Mon, Jun 01, 2020 at 10:50:50AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >On Mon, Jun 1, 2020 at 12:09 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Wed, May 27, 2020 at 10:57:11PM CEST, michael.chan@broadcom.com wrote:
>> >> >On Wed, May 27, 2020 at 1:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
>> >> >>
>> >> >> On Wed, 27 May 2020 09:07:09 +0530 Vasundhara Volam wrote:
>> >> >> > Here is a sample sequence of commands to do a "live reset" to get some
>> >> >> > clear idea.
>> >> >> > Note that I am providing the examples based on the current patchset.
>> >> >> >
>> >> >> > 1. FW live reset is disabled in the device/adapter. Here adapter has 2
>> >> >> > physical ports.
>> >> >> >
>> >> >> > $ devlink dev
>> >> >> > pci/0000:3b:00.0
>> >> >> > pci/0000:3b:00.1
>> >> >> > pci/0000:af:00.0
>> >> >> > $ devlink dev param show pci/0000:3b:00.0 name allow_fw_live_reset
>> >> >> > pci/0000:3b:00.0:
>> >> >> >   name allow_fw_live_reset type generic
>> >> >> >     values:
>> >> >> >       cmode runtime value false
>> >> >> >       cmode permanent value false
>> >> >> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
>> >> >> > pci/0000:3b:00.1:
>> >> >> >   name allow_fw_live_reset type generic
>> >> >> >     values:
>> >> >> >       cmode runtime value false
>> >> >> >       cmode permanent value false
>> >> >>
>> >> >> What's the permanent value? What if after reboot the driver is too old
>> >> >> to change this, is the reset still allowed?
>> >> >
>> >> >The permanent value should be the NVRAM value.  If the NVRAM value is
>> >> >false, the feature is always and unconditionally disabled.  If the
>> >> >permanent value is true, the feature will only be available when all
>> >> >loaded drivers indicate support for it and set the runtime value to
>> >> >true.  If an old driver is loaded afterwards, it wouldn't indicate
>> >> >support for this feature and it wouldn't set the runtime value to
>> >> >true.  So the feature will not be available until the old driver is
>> >> >unloaded or upgraded.
>> >> >
>> >> >>
>> >> >> > 2. If a user issues "ethtool --reset p1p1 all", the device cannot
>> >> >> > perform "live reset" as capability is not enabled.
>> >> >> >
>> >> >> > User needs to do a driver reload, for firmware to undergo reset.
>> >> >>
>> >> >> Why does driver reload have anything to do with resetting a potentially
>> >> >> MH device?
>> >> >
>> >> >I think she meant that all drivers have to be unloaded before the
>> >> >reset would take place in case it's a MH device since live reset is
>> >> >not supported.  If it's a single function device, unloading this
>> >> >driver is sufficient.
>> >> >
>> >> >>
>> >> >> > $ ethtool --reset p1p1 all
>> >> >>
>> >> >> Reset probably needs to be done via devlink. In any case you need a new
>> >> >> reset level for resetting MH devices and smartnics, because the current
>> >> >> reset mask covers port local, and host local cases, not any form of MH.
>> >> >
>> >> >RIght.  This reset could be just a single function reset in this example.
>> >> >
>> >> >>
>> >> >> > ETHTOOL_RESET 0xffffffff
>> >> >> > Components reset:     0xff0000
>> >> >> > Components not reset: 0xff00ffff
>> >> >> > $ dmesg
>> >> >> > [  198.745822] bnxt_en 0000:3b:00.0 p1p1: Firmware reset request successful.
>> >> >> > [  198.745836] bnxt_en 0000:3b:00.0 p1p1: Reload driver to complete reset
>> >> >>
>> >> >> You said the reset was not performed, yet there is no information to
>> >> >> that effect in the log?!
>> >> >
>> >> >The firmware has been requested to reset, but the reset hasn't taken
>> >> >place yet because live reset cannot be done.  We can make the logs
>> >> >more clear.
>> >> >
>> >> >>
>> >> >> > 3. Now enable the capability in the device and reboot for device to
>> >> >> > enable the capability. Firmware does not get reset just by setting the
>> >> >> > param to true.
>> >> >> >
>> >> >> > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
>> >> >> > value true cmode permanent
>> >> >> >
>> >> >> > 4. After reboot, values of param.
>> >> >>
>> >> >> Is the reboot required here?
>> >> >>
>> >> >
>> >> >In general, our new NVRAM permanent parameters will take effect after
>> >> >reset (or reboot).
>> >>
>> >> Ah, you need a reboot. I was not expecting that :/ So the "devlink dev
>> >> reload" attr would not work for you. MLNX hardware can change this on
>> >> runtime.
>> >
>> >NVRAM parameter configuration will take effect only on reboot or on
>> >"live reset" (except few). But to enable "live reset", system needs a
>> >reboot.
>> >>
>> >>
>> >> >
>> >> >> > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
>> >> >> > pci/0000:3b:00.1:
>> >> >> >   name allow_fw_live_reset type generic
>> >> >> >     values:
>> >> >> >       cmode runtime value true
>> >> >>
>> >> >> Why is runtime value true now?
>> >> >>
>> >> >
>> >> >If the permanent (NVRAM) parameter is true, all loaded new drivers
>> >> >will indicate support for this feature and set the runtime value to
>> >> >true by default.  The runtime value would not be true if any loaded
>> >> >driver is too old or has set the runtime value to false.
>> >>
>> >> This is a bit odd. It is a configuration, not an indication. When you
>> >> want to indicate what you support something, I think it should be done
>> >> in a different place. I think that "devlink dev info" is the place to
>> >> put it, I think that we need "capabilities" there.
>> >>
>> >Indication can be shown in 'devlink dev info', but users can configure
>> >this parameter also to control the 'live reset' at runtime.
>>
>> I'm totally confused. You wrote above that you need reboot (cmode
>> permanent) for that. What is the usecase for cmode runtime?
>>
>
>Okay let me brief it completely, probably some repetition.
>
>Permanent cmode, is to enable the "live reset" feature in the device,
>which takes effect after reboot, as it is configuration the parameter
>in NVRAM configuration. After reboot, if the parameter is enabled,
>user can initiate the live reset from a separate command.
>
>Runtime cmode, default value is true and available only when permanent
>cmode is true. This allows the user to temporarily disallow the "live
>reset", if the host is running any mission critical application.

Okay. I understand now. Thanks.

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jakub Kicinski]

On Mon, 1 Jun 2020 08:39:18 +0200 Jiri Pirko wrote:
> > If the permanent (NVRAM) parameter is true, all loaded new drivers
> > will indicate support for this feature and set the runtime value to
> > true by default.  The runtime value would not be true if any loaded
> > driver is too old or has set the runtime value to false.  
> 
> This is a bit odd. It is a configuration, not an indication. When you
> want to indicate what you support something, I think it should be done
> in a different place. I think that "devlink dev info" is the place to
> put it, I think that we need "capabilities" there.

Could you explain the need for "capabilities" under dev info?

I don't like catch-all mechanisms in principle. Better if capabilities
are expressed by the API dedicated to configuration of a given feature.

In this particular example the ability to do live reset is clearly
expressed by the presence of the parameter (as implemented by this set).

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Mon, Jun 01, 2020 at 11:44:36PM CEST, kuba@kernel.org wrote:
>On Mon, 1 Jun 2020 08:39:18 +0200 Jiri Pirko wrote:
>> > If the permanent (NVRAM) parameter is true, all loaded new drivers
>> > will indicate support for this feature and set the runtime value to
>> > true by default.  The runtime value would not be true if any loaded
>> > driver is too old or has set the runtime value to false.  
>> 
>> This is a bit odd. It is a configuration, not an indication. When you
>> want to indicate what you support something, I think it should be done
>> in a different place. I think that "devlink dev info" is the place to
>> put it, I think that we need "capabilities" there.
>

First of all, I think we cleared that up, params are not used like that
in this patchset.


>Could you explain the need for "capabilities" under dev info?
>
>I don't like catch-all mechanisms in principle. Better if capabilities
>are expressed by the API dedicated to configuration of a given feature.

I see. That makes sense. I was thinking about that, some capabilities
are per port. I think that a simple attribute would do.

>
>In this particular example the ability to do live reset is clearly
>expressed by the presence of the parameter (as implemented by this set).
>

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Tue, May 26, 2020 at 04:23:48PM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Tue, May 26, 2020 at 7:10 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Tue, May 26, 2020 at 08:42:28AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >On Tue, May 26, 2020 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >>
>> >> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >> >>
>> >> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
>> >> >> >> >parameter is set, user is allowed to reset the firmware in real time.
>> >> >> >> >
>> >> >> >> >This parameter is employed to communicate user consent or dissent for
>> >> >> >> >the live reset to happen. A separate command triggers the actual live
>> >> >> >> >reset.
>> >> >> >> >
>> >> >> >> >Cc: Jiri Pirko <jiri@mellanox.com>
>> >> >> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> >> >> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> >> >> >> >---
>> >> >> >> >v2: Rename param name to "allow_fw_live_reset" from
>> >> >> >> >"enable_hot_fw_reset".
>> >> >> >> >Update documentation for the param in devlink-params.rst file.
>> >> >> >> >---
>> >> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
>> >> >> >> > include/net/devlink.h                               | 4 ++++
>> >> >> >> > net/core/devlink.c                                  | 5 +++++
>> >> >> >> > 3 files changed, 15 insertions(+)
>> >> >> >> >
>> >> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> >> >> >> >index d075fd0..ad54dfb 100644
>> >> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst
>> >> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
>> >> >> >> >@@ -108,3 +108,9 @@ own name.
>> >> >> >> >    * - ``region_snapshot_enable``
>> >> >> >> >      - Boolean
>> >> >> >> >      - Enable capture of ``devlink-region`` snapshots.
>> >> >> >> >+   * - ``allow_fw_live_reset``
>> >> >> >> >+     - Boolean
>> >> >> >> >+     - Firmware live reset allows users to reset the firmware in real time.
>> >> >> >> >+       For example, after firmware upgrade, this feature can immediately reset
>> >> >> >> >+       to run the new firmware without reloading the driver or rebooting the
>> >> >> >>
>> >> >> >> This does not tell me anything about the reset being done on another
>> >> >> >> host. You need to emhasize that, in the name of the param too.
>> >> >> >I am not sure if I completely understand your query.
>> >> >> >
>> >> >> >Reset is actually initiated by one of the PF/host of the device, which
>> >> >> >resets the entire same device.
>> >> >> >
>> >> >> >Reset is not initiated by any other remote device/host.
>> >> >>
>> >> >> Well, in case of multihost system, it might be, right?
>> >> >>
>> >> >In case of multi-host system also, it is one of the host that triggers
>> >> >the reset, which resets the entire same device. I don't think this is
>> >> >remote.
>> >> >
>> >> >As the parameter is a device parameter, it is applicable to the entire
>> >> >device. When a user initiates the reset from any of the host in case
>> >> >of multi-host and any of the PF in case of stand-alone or smartNIC
>> >> >device, the entire device goes for a reset.
>> >> >
>> >> >I will be expanding the description to the following to make it more clear.
>> >> >
>> >> >------------------------
>> >> >- Firmware live reset allows users to reset the firmware in real time.
>> >> >For example, after firmware upgrade, this feature can immediately
>> >> >reset to run the new firmware without reloading the driver or
>> >> >rebooting the system.
>> >> >When a user initiates the reset from any of the host (in case of
>> >> >multi-host system) / PF (in case of stand-alone or smartNIC device),
>> >> >the entire device goes for a reset when the parameter is enabled.
>> >>
>> >> Sorry, this is still not clear. I think that you are mixing up two
>> >> different things:
>> >> 1) option of devlink reload to indicate that user is interested in "live
>> >>    reset" of firmware without reloading driver
>> >
>> >This is the option we are trying to add. If a user is interested in
>> >"live reset", he needs to enable the parameter to enable it in device
>> >capabilities, which is achieved by permanent configuration mode. When
>> >capability is enabled in the device, new firmware which is aware will
>> >allocate the resources and exposes the capability to host drivers.
>> >
>> >But firmware allows the "live reset" only when all the loaded drivers
>> >are aware of/supports the capability. For example, if any of the host
>> >is loaded with an old driver, "live reset" is not allowed until the
>> >driver is upgraded or unloaded. or if the host driver turns it off,
>> >then also "live reset" is not allowed.
>> >
>> >In case of runtime parameter cmode, if any of the host turns off the
>> >capability in the host driver, "live reset" is not allowed until the
>> >driver is unloaded or the user enables it again.
>> >
>> >To make it clear, I can add two parameters.
>> >
>> >1. enable_fw_live_reset - To indicate that the user is interested in
>> >"live reset". This will be a generic param.
>>
>> As I wrote above, I believe this should be an option
>> to "devlink dev reload", not a param.
>I think you are still confused with enabling feature in NVRAM
>configuration of the device and command to trigger reset. This param
>will enable the feature in the device NVRAM configuration and does not
>trigger the actual reset.
>
>Only when the param is set, feature will be enabled in the device and
>firmware supports the "live reset". When the param is disabled,
>firmware cannot support "live reset" and user needs to do PCIe reset
>after flashing the firmware for it to take effect..

Does that mean that after reboot, when user triggers fw reset, it will
be always "live" is possible? Meaning, user will no have a way to
specify that per-reset?


>
>Once feature is enabled in NVRAM configuration, it will be persistent
>across reboots.
>
>User still needs to use "devlink dev reload" command to do the "live reset".
>>
>>
>> >
>> >2. allow_fw_live_reset - To indicate, if any of the host/PF turns it
>> >off, "live reset" is not allowed. This serves the purpose of what we
>> >are trying to add in runtime cmode.
>>
>> Yeah.
>And this param will enable the feature in the driver for driver to
>allow the firmware to go for "live reset", where as above param will
>enable the feature in NVRAM configuration of the device.
>>
>> >Do you want me to keep it as a driver-specific param?
>>
>> There is nothing driver-specific about this.
>okay.
>>
>>
>> >
>> >Please let me know if this is clear and makes less confusion.
>> >
>> >Thanks,
>> >Vasundhara
>> >
>> >> 2) devlink param that would indicate "I am okay if someone else (not by
>> >>    my devlink instance) resets my firmware".
>> >>
>> >> Could you please split?
>> >>
>> >>
>> >> >------------------------
>> >> >
>> >> >Thanks,
>> >> >Vasundhara
>> >> >>
>> >> >> >
>> >> >> >Thanks,
>> >> >> >Vasundhara
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >> >+       system.
>> >> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >> >> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
>> >> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
>> >> >> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
>> >> >> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
>> >> >> >> >+      },
>> >> >> >> > };
>> >> >> >> >
>> >> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
>> >> >> >> >--
>> >> >> >> >1.8.3.1
>> >> >> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Mon, Jun 1, 2020 at 12:48 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, May 26, 2020 at 04:23:48PM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Tue, May 26, 2020 at 7:10 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Tue, May 26, 2020 at 08:42:28AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >On Tue, May 26, 2020 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >>
> >> >> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> >>
> >> >> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> >> >>
> >> >> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
> >> >> >> >> >parameter is set, user is allowed to reset the firmware in real time.
> >> >> >> >> >
> >> >> >> >> >This parameter is employed to communicate user consent or dissent for
> >> >> >> >> >the live reset to happen. A separate command triggers the actual live
> >> >> >> >> >reset.
> >> >> >> >> >
> >> >> >> >> >Cc: Jiri Pirko <jiri@mellanox.com>
> >> >> >> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >> >> >> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> >> >> >> >> >---
> >> >> >> >> >v2: Rename param name to "allow_fw_live_reset" from
> >> >> >> >> >"enable_hot_fw_reset".
> >> >> >> >> >Update documentation for the param in devlink-params.rst file.
> >> >> >> >> >---
> >> >> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
> >> >> >> >> > include/net/devlink.h                               | 4 ++++
> >> >> >> >> > net/core/devlink.c                                  | 5 +++++
> >> >> >> >> > 3 files changed, 15 insertions(+)
> >> >> >> >> >
> >> >> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
> >> >> >> >> >index d075fd0..ad54dfb 100644
> >> >> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst
> >> >> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
> >> >> >> >> >@@ -108,3 +108,9 @@ own name.
> >> >> >> >> >    * - ``region_snapshot_enable``
> >> >> >> >> >      - Boolean
> >> >> >> >> >      - Enable capture of ``devlink-region`` snapshots.
> >> >> >> >> >+   * - ``allow_fw_live_reset``
> >> >> >> >> >+     - Boolean
> >> >> >> >> >+     - Firmware live reset allows users to reset the firmware in real time.
> >> >> >> >> >+       For example, after firmware upgrade, this feature can immediately reset
> >> >> >> >> >+       to run the new firmware without reloading the driver or rebooting the
> >> >> >> >>
> >> >> >> >> This does not tell me anything about the reset being done on another
> >> >> >> >> host. You need to emhasize that, in the name of the param too.
> >> >> >> >I am not sure if I completely understand your query.
> >> >> >> >
> >> >> >> >Reset is actually initiated by one of the PF/host of the device, which
> >> >> >> >resets the entire same device.
> >> >> >> >
> >> >> >> >Reset is not initiated by any other remote device/host.
> >> >> >>
> >> >> >> Well, in case of multihost system, it might be, right?
> >> >> >>
> >> >> >In case of multi-host system also, it is one of the host that triggers
> >> >> >the reset, which resets the entire same device. I don't think this is
> >> >> >remote.
> >> >> >
> >> >> >As the parameter is a device parameter, it is applicable to the entire
> >> >> >device. When a user initiates the reset from any of the host in case
> >> >> >of multi-host and any of the PF in case of stand-alone or smartNIC
> >> >> >device, the entire device goes for a reset.
> >> >> >
> >> >> >I will be expanding the description to the following to make it more clear.
> >> >> >
> >> >> >------------------------
> >> >> >- Firmware live reset allows users to reset the firmware in real time.
> >> >> >For example, after firmware upgrade, this feature can immediately
> >> >> >reset to run the new firmware without reloading the driver or
> >> >> >rebooting the system.
> >> >> >When a user initiates the reset from any of the host (in case of
> >> >> >multi-host system) / PF (in case of stand-alone or smartNIC device),
> >> >> >the entire device goes for a reset when the parameter is enabled.
> >> >>
> >> >> Sorry, this is still not clear. I think that you are mixing up two
> >> >> different things:
> >> >> 1) option of devlink reload to indicate that user is interested in "live
> >> >>    reset" of firmware without reloading driver
> >> >
> >> >This is the option we are trying to add. If a user is interested in
> >> >"live reset", he needs to enable the parameter to enable it in device
> >> >capabilities, which is achieved by permanent configuration mode. When
> >> >capability is enabled in the device, new firmware which is aware will
> >> >allocate the resources and exposes the capability to host drivers.
> >> >
> >> >But firmware allows the "live reset" only when all the loaded drivers
> >> >are aware of/supports the capability. For example, if any of the host
> >> >is loaded with an old driver, "live reset" is not allowed until the
> >> >driver is upgraded or unloaded. or if the host driver turns it off,
> >> >then also "live reset" is not allowed.
> >> >
> >> >In case of runtime parameter cmode, if any of the host turns off the
> >> >capability in the host driver, "live reset" is not allowed until the
> >> >driver is unloaded or the user enables it again.
> >> >
> >> >To make it clear, I can add two parameters.
> >> >
> >> >1. enable_fw_live_reset - To indicate that the user is interested in
> >> >"live reset". This will be a generic param.
> >>
> >> As I wrote above, I believe this should be an option
> >> to "devlink dev reload", not a param.
> >I think you are still confused with enabling feature in NVRAM
> >configuration of the device and command to trigger reset. This param
> >will enable the feature in the device NVRAM configuration and does not
> >trigger the actual reset.
> >
> >Only when the param is set, feature will be enabled in the device and
> >firmware supports the "live reset". When the param is disabled,
> >firmware cannot support "live reset" and user needs to do PCIe reset
> >after flashing the firmware for it to take effect..
>
> Does that mean that after reboot, when user triggers fw reset, it will
> be always "live" is possible? Meaning, user will no have a way to
> specify that per-reset?
Right now, there is no option for user to mention the type of reset.

As you suggested, we need to extend 'devlink dev reload' for users to
mention the type of reset.

>
>
> >
> >Once feature is enabled in NVRAM configuration, it will be persistent
> >across reboots.
> >
> >User still needs to use "devlink dev reload" command to do the "live reset".
> >>
> >>
> >> >
> >> >2. allow_fw_live_reset - To indicate, if any of the host/PF turns it
> >> >off, "live reset" is not allowed. This serves the purpose of what we
> >> >are trying to add in runtime cmode.
> >>
> >> Yeah.
> >And this param will enable the feature in the driver for driver to
> >allow the firmware to go for "live reset", where as above param will
> >enable the feature in NVRAM configuration of the device.
> >>
> >> >Do you want me to keep it as a driver-specific param?
> >>
> >> There is nothing driver-specific about this.
> >okay.
> >>
> >>
> >> >
> >> >Please let me know if this is clear and makes less confusion.
> >> >
> >> >Thanks,
> >> >Vasundhara
> >> >
> >> >> 2) devlink param that would indicate "I am okay if someone else (not by
> >> >>    my devlink instance) resets my firmware".
> >> >>
> >> >> Could you please split?
> >> >>
> >> >>
> >> >> >------------------------
> >> >> >
> >> >> >Thanks,
> >> >> >Vasundhara
> >> >> >>
> >> >> >> >
> >> >> >> >Thanks,
> >> >> >> >Vasundhara
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> >+       system.
> >> >> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >> >> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
> >> >> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
> >> >> >> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
> >> >> >> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
> >> >> >> >> >+      },
> >> >> >> >> > };
> >> >> >> >> >
> >> >> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
> >> >> >> >> >--
> >> >> >> >> >1.8.3.1
> >> >> >> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Mon, Jun 01, 2020 at 10:53:19AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Mon, Jun 1, 2020 at 12:48 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Tue, May 26, 2020 at 04:23:48PM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >On Tue, May 26, 2020 at 7:10 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Tue, May 26, 2020 at 08:42:28AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >On Tue, May 26, 2020 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >>
>> >> >> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >> >>
>> >> >> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >> >> >>
>> >> >> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
>> >> >> >> >> >parameter is set, user is allowed to reset the firmware in real time.
>> >> >> >> >> >
>> >> >> >> >> >This parameter is employed to communicate user consent or dissent for
>> >> >> >> >> >the live reset to happen. A separate command triggers the actual live
>> >> >> >> >> >reset.
>> >> >> >> >> >
>> >> >> >> >> >Cc: Jiri Pirko <jiri@mellanox.com>
>> >> >> >> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> >> >> >> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> >> >> >> >> >---
>> >> >> >> >> >v2: Rename param name to "allow_fw_live_reset" from
>> >> >> >> >> >"enable_hot_fw_reset".
>> >> >> >> >> >Update documentation for the param in devlink-params.rst file.
>> >> >> >> >> >---
>> >> >> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
>> >> >> >> >> > include/net/devlink.h                               | 4 ++++
>> >> >> >> >> > net/core/devlink.c                                  | 5 +++++
>> >> >> >> >> > 3 files changed, 15 insertions(+)
>> >> >> >> >> >
>> >> >> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> >> >> >> >> >index d075fd0..ad54dfb 100644
>> >> >> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst
>> >> >> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
>> >> >> >> >> >@@ -108,3 +108,9 @@ own name.
>> >> >> >> >> >    * - ``region_snapshot_enable``
>> >> >> >> >> >      - Boolean
>> >> >> >> >> >      - Enable capture of ``devlink-region`` snapshots.
>> >> >> >> >> >+   * - ``allow_fw_live_reset``
>> >> >> >> >> >+     - Boolean
>> >> >> >> >> >+     - Firmware live reset allows users to reset the firmware in real time.
>> >> >> >> >> >+       For example, after firmware upgrade, this feature can immediately reset
>> >> >> >> >> >+       to run the new firmware without reloading the driver or rebooting the
>> >> >> >> >>
>> >> >> >> >> This does not tell me anything about the reset being done on another
>> >> >> >> >> host. You need to emhasize that, in the name of the param too.
>> >> >> >> >I am not sure if I completely understand your query.
>> >> >> >> >
>> >> >> >> >Reset is actually initiated by one of the PF/host of the device, which
>> >> >> >> >resets the entire same device.
>> >> >> >> >
>> >> >> >> >Reset is not initiated by any other remote device/host.
>> >> >> >>
>> >> >> >> Well, in case of multihost system, it might be, right?
>> >> >> >>
>> >> >> >In case of multi-host system also, it is one of the host that triggers
>> >> >> >the reset, which resets the entire same device. I don't think this is
>> >> >> >remote.
>> >> >> >
>> >> >> >As the parameter is a device parameter, it is applicable to the entire
>> >> >> >device. When a user initiates the reset from any of the host in case
>> >> >> >of multi-host and any of the PF in case of stand-alone or smartNIC
>> >> >> >device, the entire device goes for a reset.
>> >> >> >
>> >> >> >I will be expanding the description to the following to make it more clear.
>> >> >> >
>> >> >> >------------------------
>> >> >> >- Firmware live reset allows users to reset the firmware in real time.
>> >> >> >For example, after firmware upgrade, this feature can immediately
>> >> >> >reset to run the new firmware without reloading the driver or
>> >> >> >rebooting the system.
>> >> >> >When a user initiates the reset from any of the host (in case of
>> >> >> >multi-host system) / PF (in case of stand-alone or smartNIC device),
>> >> >> >the entire device goes for a reset when the parameter is enabled.
>> >> >>
>> >> >> Sorry, this is still not clear. I think that you are mixing up two
>> >> >> different things:
>> >> >> 1) option of devlink reload to indicate that user is interested in "live
>> >> >>    reset" of firmware without reloading driver
>> >> >
>> >> >This is the option we are trying to add. If a user is interested in
>> >> >"live reset", he needs to enable the parameter to enable it in device
>> >> >capabilities, which is achieved by permanent configuration mode. When
>> >> >capability is enabled in the device, new firmware which is aware will
>> >> >allocate the resources and exposes the capability to host drivers.
>> >> >
>> >> >But firmware allows the "live reset" only when all the loaded drivers
>> >> >are aware of/supports the capability. For example, if any of the host
>> >> >is loaded with an old driver, "live reset" is not allowed until the
>> >> >driver is upgraded or unloaded. or if the host driver turns it off,
>> >> >then also "live reset" is not allowed.
>> >> >
>> >> >In case of runtime parameter cmode, if any of the host turns off the
>> >> >capability in the host driver, "live reset" is not allowed until the
>> >> >driver is unloaded or the user enables it again.
>> >> >
>> >> >To make it clear, I can add two parameters.
>> >> >
>> >> >1. enable_fw_live_reset - To indicate that the user is interested in
>> >> >"live reset". This will be a generic param.
>> >>
>> >> As I wrote above, I believe this should be an option
>> >> to "devlink dev reload", not a param.
>> >I think you are still confused with enabling feature in NVRAM
>> >configuration of the device and command to trigger reset. This param
>> >will enable the feature in the device NVRAM configuration and does not
>> >trigger the actual reset.
>> >
>> >Only when the param is set, feature will be enabled in the device and
>> >firmware supports the "live reset". When the param is disabled,
>> >firmware cannot support "live reset" and user needs to do PCIe reset
>> >after flashing the firmware for it to take effect..
>>
>> Does that mean that after reboot, when user triggers fw reset, it will
>> be always "live" is possible? Meaning, user will no have a way to
>> specify that per-reset?
>Right now, there is no option for user to mention the type of reset.
>
>As you suggested, we need to extend 'devlink dev reload' for users to
>mention the type of reset.

Does your fw support it? The option of "I want live reset now/I
don't want live reset now"?



>
>>
>>
>> >
>> >Once feature is enabled in NVRAM configuration, it will be persistent
>> >across reboots.
>> >
>> >User still needs to use "devlink dev reload" command to do the "live reset".
>> >>
>> >>
>> >> >
>> >> >2. allow_fw_live_reset - To indicate, if any of the host/PF turns it
>> >> >off, "live reset" is not allowed. This serves the purpose of what we
>> >> >are trying to add in runtime cmode.
>> >>
>> >> Yeah.
>> >And this param will enable the feature in the driver for driver to
>> >allow the firmware to go for "live reset", where as above param will
>> >enable the feature in NVRAM configuration of the device.
>> >>
>> >> >Do you want me to keep it as a driver-specific param?
>> >>
>> >> There is nothing driver-specific about this.
>> >okay.
>> >>
>> >>
>> >> >
>> >> >Please let me know if this is clear and makes less confusion.
>> >> >
>> >> >Thanks,
>> >> >Vasundhara
>> >> >
>> >> >> 2) devlink param that would indicate "I am okay if someone else (not by
>> >> >>    my devlink instance) resets my firmware".
>> >> >>
>> >> >> Could you please split?
>> >> >>
>> >> >>
>> >> >> >------------------------
>> >> >> >
>> >> >> >Thanks,
>> >> >> >Vasundhara
>> >> >> >>
>> >> >> >> >
>> >> >> >> >Thanks,
>> >> >> >> >Vasundhara
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> >+       system.
>> >> >> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >> >> >> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
>> >> >> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
>> >> >> >> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
>> >> >> >> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
>> >> >> >> >> >+      },
>> >> >> >> >> > };
>> >> >> >> >> >
>> >> >> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
>> >> >> >> >> >--
>> >> >> >> >> >1.8.3.1
>> >> >> >> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Vasundhara Volam]

On Mon, Jun 1, 2020 at 3:20 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 01, 2020 at 10:53:19AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Mon, Jun 1, 2020 at 12:48 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Tue, May 26, 2020 at 04:23:48PM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >On Tue, May 26, 2020 at 7:10 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >>
> >> >> Tue, May 26, 2020 at 08:42:28AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >On Tue, May 26, 2020 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> >>
> >> >> >> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> >> >>
> >> >> >> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> >> >> >>
> >> >> >> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >> >> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
> >> >> >> >> >> >parameter is set, user is allowed to reset the firmware in real time.
> >> >> >> >> >> >
> >> >> >> >> >> >This parameter is employed to communicate user consent or dissent for
> >> >> >> >> >> >the live reset to happen. A separate command triggers the actual live
> >> >> >> >> >> >reset.
> >> >> >> >> >> >
> >> >> >> >> >> >Cc: Jiri Pirko <jiri@mellanox.com>
> >> >> >> >> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >> >> >> >> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> >> >> >> >> >> >---
> >> >> >> >> >> >v2: Rename param name to "allow_fw_live_reset" from
> >> >> >> >> >> >"enable_hot_fw_reset".
> >> >> >> >> >> >Update documentation for the param in devlink-params.rst file.
> >> >> >> >> >> >---
> >> >> >> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
> >> >> >> >> >> > include/net/devlink.h                               | 4 ++++
> >> >> >> >> >> > net/core/devlink.c                                  | 5 +++++
> >> >> >> >> >> > 3 files changed, 15 insertions(+)
> >> >> >> >> >> >
> >> >> >> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
> >> >> >> >> >> >index d075fd0..ad54dfb 100644
> >> >> >> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst
> >> >> >> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
> >> >> >> >> >> >@@ -108,3 +108,9 @@ own name.
> >> >> >> >> >> >    * - ``region_snapshot_enable``
> >> >> >> >> >> >      - Boolean
> >> >> >> >> >> >      - Enable capture of ``devlink-region`` snapshots.
> >> >> >> >> >> >+   * - ``allow_fw_live_reset``
> >> >> >> >> >> >+     - Boolean
> >> >> >> >> >> >+     - Firmware live reset allows users to reset the firmware in real time.
> >> >> >> >> >> >+       For example, after firmware upgrade, this feature can immediately reset
> >> >> >> >> >> >+       to run the new firmware without reloading the driver or rebooting the
> >> >> >> >> >>
> >> >> >> >> >> This does not tell me anything about the reset being done on another
> >> >> >> >> >> host. You need to emhasize that, in the name of the param too.
> >> >> >> >> >I am not sure if I completely understand your query.
> >> >> >> >> >
> >> >> >> >> >Reset is actually initiated by one of the PF/host of the device, which
> >> >> >> >> >resets the entire same device.
> >> >> >> >> >
> >> >> >> >> >Reset is not initiated by any other remote device/host.
> >> >> >> >>
> >> >> >> >> Well, in case of multihost system, it might be, right?
> >> >> >> >>
> >> >> >> >In case of multi-host system also, it is one of the host that triggers
> >> >> >> >the reset, which resets the entire same device. I don't think this is
> >> >> >> >remote.
> >> >> >> >
> >> >> >> >As the parameter is a device parameter, it is applicable to the entire
> >> >> >> >device. When a user initiates the reset from any of the host in case
> >> >> >> >of multi-host and any of the PF in case of stand-alone or smartNIC
> >> >> >> >device, the entire device goes for a reset.
> >> >> >> >
> >> >> >> >I will be expanding the description to the following to make it more clear.
> >> >> >> >
> >> >> >> >------------------------
> >> >> >> >- Firmware live reset allows users to reset the firmware in real time.
> >> >> >> >For example, after firmware upgrade, this feature can immediately
> >> >> >> >reset to run the new firmware without reloading the driver or
> >> >> >> >rebooting the system.
> >> >> >> >When a user initiates the reset from any of the host (in case of
> >> >> >> >multi-host system) / PF (in case of stand-alone or smartNIC device),
> >> >> >> >the entire device goes for a reset when the parameter is enabled.
> >> >> >>
> >> >> >> Sorry, this is still not clear. I think that you are mixing up two
> >> >> >> different things:
> >> >> >> 1) option of devlink reload to indicate that user is interested in "live
> >> >> >>    reset" of firmware without reloading driver
> >> >> >
> >> >> >This is the option we are trying to add. If a user is interested in
> >> >> >"live reset", he needs to enable the parameter to enable it in device
> >> >> >capabilities, which is achieved by permanent configuration mode. When
> >> >> >capability is enabled in the device, new firmware which is aware will
> >> >> >allocate the resources and exposes the capability to host drivers.
> >> >> >
> >> >> >But firmware allows the "live reset" only when all the loaded drivers
> >> >> >are aware of/supports the capability. For example, if any of the host
> >> >> >is loaded with an old driver, "live reset" is not allowed until the
> >> >> >driver is upgraded or unloaded. or if the host driver turns it off,
> >> >> >then also "live reset" is not allowed.
> >> >> >
> >> >> >In case of runtime parameter cmode, if any of the host turns off the
> >> >> >capability in the host driver, "live reset" is not allowed until the
> >> >> >driver is unloaded or the user enables it again.
> >> >> >
> >> >> >To make it clear, I can add two parameters.
> >> >> >
> >> >> >1. enable_fw_live_reset - To indicate that the user is interested in
> >> >> >"live reset". This will be a generic param.
> >> >>
> >> >> As I wrote above, I believe this should be an option
> >> >> to "devlink dev reload", not a param.
> >> >I think you are still confused with enabling feature in NVRAM
> >> >configuration of the device and command to trigger reset. This param
> >> >will enable the feature in the device NVRAM configuration and does not
> >> >trigger the actual reset.
> >> >
> >> >Only when the param is set, feature will be enabled in the device and
> >> >firmware supports the "live reset". When the param is disabled,
> >> >firmware cannot support "live reset" and user needs to do PCIe reset
> >> >after flashing the firmware for it to take effect..
> >>
> >> Does that mean that after reboot, when user triggers fw reset, it will
> >> be always "live" is possible? Meaning, user will no have a way to
> >> specify that per-reset?
> >Right now, there is no option for user to mention the type of reset.
> >
> >As you suggested, we need to extend 'devlink dev reload' for users to
> >mention the type of reset.
>
> Does your fw support it? The option of "I want live reset now/I
> don't want live reset now"?

Yes, our firmware supports both. Even when "live reset" is enabled,
users can choose the option of "live reset" / "reset on driver
reload".

>
>
>
> >
> >>
> >>
> >> >
> >> >Once feature is enabled in NVRAM configuration, it will be persistent
> >> >across reboots.
> >> >
> >> >User still needs to use "devlink dev reload" command to do the "live reset".
> >> >>
> >> >>
> >> >> >
> >> >> >2. allow_fw_live_reset - To indicate, if any of the host/PF turns it
> >> >> >off, "live reset" is not allowed. This serves the purpose of what we
> >> >> >are trying to add in runtime cmode.
> >> >>
> >> >> Yeah.
> >> >And this param will enable the feature in the driver for driver to
> >> >allow the firmware to go for "live reset", where as above param will
> >> >enable the feature in NVRAM configuration of the device.
> >> >>
> >> >> >Do you want me to keep it as a driver-specific param?
> >> >>
> >> >> There is nothing driver-specific about this.
> >> >okay.
> >> >>
> >> >>
> >> >> >
> >> >> >Please let me know if this is clear and makes less confusion.
> >> >> >
> >> >> >Thanks,
> >> >> >Vasundhara
> >> >> >
> >> >> >> 2) devlink param that would indicate "I am okay if someone else (not by
> >> >> >>    my devlink instance) resets my firmware".
> >> >> >>
> >> >> >> Could you please split?
> >> >> >>
> >> >> >>
> >> >> >> >------------------------
> >> >> >> >
> >> >> >> >Thanks,
> >> >> >> >Vasundhara
> >> >> >> >>
> >> >> >> >> >
> >> >> >> >> >Thanks,
> >> >> >> >> >Vasundhara
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >> >> >+       system.
> >> >> >> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >> >> >> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
> >> >> >> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
> >> >> >> >> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
> >> >> >> >> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
> >> >> >> >> >> >+      },
> >> >> >> >> >> > };
> >> >> >> >> >> >
> >> >> >> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
> >> >> >> >> >> >--
> >> >> >> >> >> >1.8.3.1
> >> >> >> >> >> >

Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jiri Pirko]

Mon, Jun 01, 2020 at 11:59:38AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Mon, Jun 1, 2020 at 3:20 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Mon, Jun 01, 2020 at 10:53:19AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >On Mon, Jun 1, 2020 at 12:48 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Tue, May 26, 2020 at 04:23:48PM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >On Tue, May 26, 2020 at 7:10 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >>
>> >> >> Tue, May 26, 2020 at 08:42:28AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >On Tue, May 26, 2020 at 10:17 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >> >>
>> >> >> >> Tue, May 26, 2020 at 06:28:59AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >> >On Mon, May 25, 2020 at 10:56 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >> >> >>
>> >> >> >> >> Sun, May 24, 2020 at 08:29:56AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >> >> >On Sun, May 24, 2020 at 10:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >> >> >> >>
>> >> >> >> >> >> Sat, May 23, 2020 at 08:08:22AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >> >> >> >> >> >Add a new "allow_fw_live_reset" generic device bool parameter. When
>> >> >> >> >> >> >parameter is set, user is allowed to reset the firmware in real time.
>> >> >> >> >> >> >
>> >> >> >> >> >> >This parameter is employed to communicate user consent or dissent for
>> >> >> >> >> >> >the live reset to happen. A separate command triggers the actual live
>> >> >> >> >> >> >reset.
>> >> >> >> >> >> >
>> >> >> >> >> >> >Cc: Jiri Pirko <jiri@mellanox.com>
>> >> >> >> >> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> >> >> >> >> >> >Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>> >> >> >> >> >> >---
>> >> >> >> >> >> >v2: Rename param name to "allow_fw_live_reset" from
>> >> >> >> >> >> >"enable_hot_fw_reset".
>> >> >> >> >> >> >Update documentation for the param in devlink-params.rst file.
>> >> >> >> >> >> >---
>> >> >> >> >> >> > Documentation/networking/devlink/devlink-params.rst | 6 ++++++
>> >> >> >> >> >> > include/net/devlink.h                               | 4 ++++
>> >> >> >> >> >> > net/core/devlink.c                                  | 5 +++++
>> >> >> >> >> >> > 3 files changed, 15 insertions(+)
>> >> >> >> >> >> >
>> >> >> >> >> >> >diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
>> >> >> >> >> >> >index d075fd0..ad54dfb 100644
>> >> >> >> >> >> >--- a/Documentation/networking/devlink/devlink-params.rst
>> >> >> >> >> >> >+++ b/Documentation/networking/devlink/devlink-params.rst
>> >> >> >> >> >> >@@ -108,3 +108,9 @@ own name.
>> >> >> >> >> >> >    * - ``region_snapshot_enable``
>> >> >> >> >> >> >      - Boolean
>> >> >> >> >> >> >      - Enable capture of ``devlink-region`` snapshots.
>> >> >> >> >> >> >+   * - ``allow_fw_live_reset``
>> >> >> >> >> >> >+     - Boolean
>> >> >> >> >> >> >+     - Firmware live reset allows users to reset the firmware in real time.
>> >> >> >> >> >> >+       For example, after firmware upgrade, this feature can immediately reset
>> >> >> >> >> >> >+       to run the new firmware without reloading the driver or rebooting the
>> >> >> >> >> >>
>> >> >> >> >> >> This does not tell me anything about the reset being done on another
>> >> >> >> >> >> host. You need to emhasize that, in the name of the param too.
>> >> >> >> >> >I am not sure if I completely understand your query.
>> >> >> >> >> >
>> >> >> >> >> >Reset is actually initiated by one of the PF/host of the device, which
>> >> >> >> >> >resets the entire same device.
>> >> >> >> >> >
>> >> >> >> >> >Reset is not initiated by any other remote device/host.
>> >> >> >> >>
>> >> >> >> >> Well, in case of multihost system, it might be, right?
>> >> >> >> >>
>> >> >> >> >In case of multi-host system also, it is one of the host that triggers
>> >> >> >> >the reset, which resets the entire same device. I don't think this is
>> >> >> >> >remote.
>> >> >> >> >
>> >> >> >> >As the parameter is a device parameter, it is applicable to the entire
>> >> >> >> >device. When a user initiates the reset from any of the host in case
>> >> >> >> >of multi-host and any of the PF in case of stand-alone or smartNIC
>> >> >> >> >device, the entire device goes for a reset.
>> >> >> >> >
>> >> >> >> >I will be expanding the description to the following to make it more clear.
>> >> >> >> >
>> >> >> >> >------------------------
>> >> >> >> >- Firmware live reset allows users to reset the firmware in real time.
>> >> >> >> >For example, after firmware upgrade, this feature can immediately
>> >> >> >> >reset to run the new firmware without reloading the driver or
>> >> >> >> >rebooting the system.
>> >> >> >> >When a user initiates the reset from any of the host (in case of
>> >> >> >> >multi-host system) / PF (in case of stand-alone or smartNIC device),
>> >> >> >> >the entire device goes for a reset when the parameter is enabled.
>> >> >> >>
>> >> >> >> Sorry, this is still not clear. I think that you are mixing up two
>> >> >> >> different things:
>> >> >> >> 1) option of devlink reload to indicate that user is interested in "live
>> >> >> >>    reset" of firmware without reloading driver
>> >> >> >
>> >> >> >This is the option we are trying to add. If a user is interested in
>> >> >> >"live reset", he needs to enable the parameter to enable it in device
>> >> >> >capabilities, which is achieved by permanent configuration mode. When
>> >> >> >capability is enabled in the device, new firmware which is aware will
>> >> >> >allocate the resources and exposes the capability to host drivers.
>> >> >> >
>> >> >> >But firmware allows the "live reset" only when all the loaded drivers
>> >> >> >are aware of/supports the capability. For example, if any of the host
>> >> >> >is loaded with an old driver, "live reset" is not allowed until the
>> >> >> >driver is upgraded or unloaded. or if the host driver turns it off,
>> >> >> >then also "live reset" is not allowed.
>> >> >> >
>> >> >> >In case of runtime parameter cmode, if any of the host turns off the
>> >> >> >capability in the host driver, "live reset" is not allowed until the
>> >> >> >driver is unloaded or the user enables it again.
>> >> >> >
>> >> >> >To make it clear, I can add two parameters.
>> >> >> >
>> >> >> >1. enable_fw_live_reset - To indicate that the user is interested in
>> >> >> >"live reset". This will be a generic param.
>> >> >>
>> >> >> As I wrote above, I believe this should be an option
>> >> >> to "devlink dev reload", not a param.
>> >> >I think you are still confused with enabling feature in NVRAM
>> >> >configuration of the device and command to trigger reset. This param
>> >> >will enable the feature in the device NVRAM configuration and does not
>> >> >trigger the actual reset.
>> >> >
>> >> >Only when the param is set, feature will be enabled in the device and
>> >> >firmware supports the "live reset". When the param is disabled,
>> >> >firmware cannot support "live reset" and user needs to do PCIe reset
>> >> >after flashing the firmware for it to take effect..
>> >>
>> >> Does that mean that after reboot, when user triggers fw reset, it will
>> >> be always "live" is possible? Meaning, user will no have a way to
>> >> specify that per-reset?
>> >Right now, there is no option for user to mention the type of reset.
>> >
>> >As you suggested, we need to extend 'devlink dev reload' for users to
>> >mention the type of reset.
>>
>> Does your fw support it? The option of "I want live reset now/I
>> don't want live reset now"?
>
>Yes, our firmware supports both. Even when "live reset" is enabled,
>users can choose the option of "live reset" / "reset on driver
>reload".

Good, I believe you need to expose this option now.


>
>>
>>
>>
>> >
>> >>
>> >>
>> >> >
>> >> >Once feature is enabled in NVRAM configuration, it will be persistent
>> >> >across reboots.
>> >> >
>> >> >User still needs to use "devlink dev reload" command to do the "live reset".
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >2. allow_fw_live_reset - To indicate, if any of the host/PF turns it
>> >> >> >off, "live reset" is not allowed. This serves the purpose of what we
>> >> >> >are trying to add in runtime cmode.
>> >> >>
>> >> >> Yeah.
>> >> >And this param will enable the feature in the driver for driver to
>> >> >allow the firmware to go for "live reset", where as above param will
>> >> >enable the feature in NVRAM configuration of the device.
>> >> >>
>> >> >> >Do you want me to keep it as a driver-specific param?
>> >> >>
>> >> >> There is nothing driver-specific about this.
>> >> >okay.
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >Please let me know if this is clear and makes less confusion.
>> >> >> >
>> >> >> >Thanks,
>> >> >> >Vasundhara
>> >> >> >
>> >> >> >> 2) devlink param that would indicate "I am okay if someone else (not by
>> >> >> >>    my devlink instance) resets my firmware".
>> >> >> >>
>> >> >> >> Could you please split?
>> >> >> >>
>> >> >> >>
>> >> >> >> >------------------------
>> >> >> >> >
>> >> >> >> >Thanks,
>> >> >> >> >Vasundhara
>> >> >> >> >>
>> >> >> >> >> >
>> >> >> >> >> >Thanks,
>> >> >> >> >> >Vasundhara
>> >> >> >> >> >>
>> >> >> >> >> >>
>> >> >> >> >> >>
>> >> >> >> >> >> >+       system.
>> >> >> >> >> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >> >> >> >> >> >index 8ffc1b5c..488b61c 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_ALLOW_FW_LIVE_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_ALLOW_FW_LIVE_RESET_NAME "allow_fw_live_reset"
>> >> >> >> >> >> >+#define DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_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..8544f23 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_ALLOW_FW_LIVE_RESET,
>> >> >> >> >> >> >+              .name = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_NAME,
>> >> >> >> >> >> >+              .type = DEVLINK_PARAM_GENERIC_ALLOW_FW_LIVE_RESET_TYPE,
>> >> >> >> >> >> >+      },
>> >> >> >> >> >> > };
>> >> >> >> >> >> >
>> >> >> >> >> >> > static int devlink_param_generic_verify(const struct devlink_param *param)
>> >> >> >> >> >> >--
>> >> >> >> >> >> >1.8.3.1
>> >> >> >> >> >> >

[PATCH v2 net-next 2/4] 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 v2 net-next 3/4] bnxt_en: Use allow_fw_live_reset generic devlink parameter

[Vasundhara Volam]

allow_fw_live_reset parameter supports both permanent and runtime
configuration modes. Permanent configuration mode allows user to
enable the firmware live reset device capability in NVRAM
configuration.

Runtime configuration mode allows the user to prevent firmware
live reset by setting it to false in a host driver. For parameter
to be true, all the installed host driver(s) must support the
feature, and the runtime value must also be true for all of them.
For example, if the host is running a mission critical application,
it can disable this parameter until the application has completed
to avoid a potential firmware live reset disrupting it.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
v2: Update commit message. Rename devlink param name to
"allow_fw_live_reset".
Add more documentation for param in bnxt.rst file.
---
 Documentation/networking/devlink/bnxt.rst         | 13 +++++
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 28 ++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h         |  2 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 61 +++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  1 +
 5 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/devlink/bnxt.rst b/Documentation/networking/devlink/bnxt.rst
index 3dfd84c..c4f84c9 100644
--- a/Documentation/networking/devlink/bnxt.rst
+++ b/Documentation/networking/devlink/bnxt.rst
@@ -14,6 +14,7 @@ Parameters
 
    * - Name
      - Mode
+     - Description
    * - ``enable_sriov``
      - Permanent
    * - ``ignore_ari``
@@ -22,6 +23,18 @@ Parameters
      - Permanent
    * - ``msix_vec_per_pf_min``
      - Permanent
+   * - ``allow_fw_live_reset``
+     - Permanent, runtime
+     - The parameter's runtime configuration mode allows the user to prevent
+       firmware live reset by setting it to false in a host driver. For the
+       runtime parameter to be true, all the installed host driver(s) must
+       support the feature, and the runtime value must also be true for all of
+       them. For example, if the host is running a mission critical application,
+       user can disable this parameter until the application has completed to
+       avoid a potential firmware live reset disrupting it.
+
+       The parameter's permanent configuration mode allows user to control if
+       firmware live reset device capability is advertised to the drivers.
 
 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 a812beb..9d679a8 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_ALLOW_FW_LIVE_RESET,
+	 NVM_OFF_FW_LIVE_RESET, BNXT_NVM_SHARED_CFG, 1, 1},
 };
 
 union bnxt_nvm_data {
@@ -618,6 +620,61 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
 	return 0;
 }
 
+static int bnxt_dl_param_get(struct devlink *dl, u32 id,
+			     struct devlink_param_gset_ctx *ctx)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+
+	if (ctx->cmode == DEVLINK_PARAM_CMODE_PERMANENT)
+		return bnxt_dl_nvm_param_get(dl, id, ctx);
+	else if (ctx->cmode == DEVLINK_PARAM_CMODE_RUNTIME)
+		return bnxt_hwrm_get_hot_reset(bp, &ctx->val.vbool);
+
+	return -EOPNOTSUPP;
+}
+
+static int bnxt_hwrm_set_hot_reset(struct bnxt *bp, bool hot_reset_if_support)
+{
+	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 (hot_reset_if_support)
+		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 (hot_reset_if_support && !hot_reset_allowed)
+		netdev_info(bp->dev, "FW live reset enabled, but is disallowed as it is disabled on other interface(s) of this device.\n");
+
+	return 0;
+}
+
+static int bnxt_dl_param_set(struct devlink *dl, u32 id,
+			     struct devlink_param_gset_ctx *ctx)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+
+	if (ctx->cmode == DEVLINK_PARAM_CMODE_PERMANENT)
+		return bnxt_dl_nvm_param_set(dl, id, ctx);
+	else if (ctx->cmode == DEVLINK_PARAM_CMODE_RUNTIME)
+		return bnxt_hwrm_set_hot_reset(bp, ctx->val.vbool);
+
+	return -EOPNOTSUPP;
+}
+
 static const struct devlink_param bnxt_dl_params[] = {
 	DEVLINK_PARAM_GENERIC(ENABLE_SRIOV,
 			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
@@ -640,6 +697,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_FW_LIVE_RESET,
+			      BIT(DEVLINK_PARAM_CMODE_PERMANENT) |
+			      BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			      bnxt_dl_param_get, bnxt_dl_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 v2 net-next 4/4] 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>
---
v2: Rephrase the subject and elaborate commit message
---
 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