| Message ID | 1590214105-10430-2-git-send-email-vasundhara-v.volam@broadcom.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | bnxt_en: Add new "allow_fw_live_reset" generic devlink parameter | expand |
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 >
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 > >
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 >> >
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 > >> >
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 >> >> >
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 > >> >> >
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 >> >> >> >
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 > >> >> >> >
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 > > >> >> >> >
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
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.
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.
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.
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.
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.
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.
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...
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 >> >> >> >> >
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.
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 > >> >> >> >> >
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?
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 >> >> >> >> >> >
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.
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 > >> >> >> >> >> >
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.
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 >> >> >> >> >> >> >
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).
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). >
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)