mbox series

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

Message ID 1590908625-10952-1-git-send-email-vasundhara-v.volam@broadcom.com
Headers show
Series bnxt_en: Add 'enable_live_dev_reset' and 'allow_live_dev_reset' generic devlink params. | expand

Message

Vasundhara Volam May 31, 2020, 7:03 a.m. UTC
Live device reset capability allows the users to reset the device in real
time. For example, after flashing a new firmware image, this feature allows
a user to initiate the reset immediately from a separate command, to load
the new firmware without reloading the driver or resetting the system.

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

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

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

Patchset also updates firmware spec. to 1.10.1.40.


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

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

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

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

Comments

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

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

Thanks!


>"allow_live_dev_reset".
>- Expand the documentation of each param and update commit messages
> accordingly.
>- Separated the permanent configuration mode code to another patch and
>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
>
>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
>- Update documentation files and commit messages with more details of the
> feature.
>
>Vasundhara Volam (6):
>  devlink: Add 'enable_live_dev_reset' generic parameter.
>  devlink: Add 'allow_live_dev_reset' generic parameter.
>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
>  bnxt_en: Update firmware spec. to 1.10.1.40.
>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
>
> Documentation/networking/devlink/bnxt.rst          |  4 ++
> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> include/net/devlink.h                              |  8 +++
> net/core/devlink.c                                 | 10 ++++
> 10 files changed, 175 insertions(+), 36 deletions(-)
>
>-- 
>1.8.3.1
>
Jiri Pirko June 1, 2020, 6:43 a.m. UTC | #2
Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
>Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
>>Live device reset capability allows the users to reset the device in real
>>time. For example, after flashing a new firmware image, this feature allows
>>a user to initiate the reset immediately from a separate command, to load
>>the new firmware without reloading the driver or resetting the system.
>>
>>When device reset is initiated, services running on the host interfaces
>>will momentarily pause and resume once reset is completed, which is very
>>similar to momentary network outage.
>>
>>This patchset adds support for two new generic devlink parameters for
>>controlling the live device reset capability and use it in the bnxt_en
>>driver.
>>
>>Users can initiate the reset from a separate command, for example,
>>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
>>device.
>>Where ethX or dev is any PF with administrative privileges.
>>
>>Patchset also updates firmware spec. to 1.10.1.40.
>>
>>
>>v2->v3: Split the param into two new params "enable_live_dev_reset" and
>
>Vasundhara, I asked you multiple times for this to be "devlink dev reload"
>attribute. I don't recall you telling any argument against it. I belive
>that this should not be paramater. This is very tightly related to
>reload, could you please have it as an attribute of reload, as I
>suggested?

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

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

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


>
>Thanks!
>
>
>>"allow_live_dev_reset".
>>- Expand the documentation of each param and update commit messages
>> accordingly.
>>- Separated the permanent configuration mode code to another patch and
>>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
>>
>>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
>>- Update documentation files and commit messages with more details of the
>> feature.
>>
>>Vasundhara Volam (6):
>>  devlink: Add 'enable_live_dev_reset' generic parameter.
>>  devlink: Add 'allow_live_dev_reset' generic parameter.
>>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
>>  bnxt_en: Update firmware spec. to 1.10.1.40.
>>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
>>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
>>
>> Documentation/networking/devlink/bnxt.rst          |  4 ++
>> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
>> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
>> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
>> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
>> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
>> include/net/devlink.h                              |  8 +++
>> net/core/devlink.c                                 | 10 ++++
>> 10 files changed, 175 insertions(+), 36 deletions(-)
>>
>>-- 
>>1.8.3.1
>>
Vasundhara Volam June 1, 2020, 8:58 a.m. UTC | #3
On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >>Live device reset capability allows the users to reset the device in real
> >>time. For example, after flashing a new firmware image, this feature allows
> >>a user to initiate the reset immediately from a separate command, to load
> >>the new firmware without reloading the driver or resetting the system.
> >>
> >>When device reset is initiated, services running on the host interfaces
> >>will momentarily pause and resume once reset is completed, which is very
> >>similar to momentary network outage.
> >>
> >>This patchset adds support for two new generic devlink parameters for
> >>controlling the live device reset capability and use it in the bnxt_en
> >>driver.
> >>
> >>Users can initiate the reset from a separate command, for example,
> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
> >>device.
> >>Where ethX or dev is any PF with administrative privileges.
> >>
> >>Patchset also updates firmware spec. to 1.10.1.40.
> >>
> >>
> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
> >
> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
> >attribute. I don't recall you telling any argument against it. I belive
> >that this should not be paramater. This is very tightly related to
> >reload, could you please have it as an attribute of reload, as I
> >suggested?
>
> I just wrote the thread to the previous version. I understand now why
> you need param as you require reboot to activate the feature.

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

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

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

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

Thanks.

>
>
> >
> >Thanks!
> >
> >
> >>"allow_live_dev_reset".
> >>- Expand the documentation of each param and update commit messages
> >> accordingly.
> >>- Separated the permanent configuration mode code to another patch and
> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
> >>
> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
> >>- Update documentation files and commit messages with more details of the
> >> feature.
> >>
> >>Vasundhara Volam (6):
> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
> >>
> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> >> include/net/devlink.h                              |  8 +++
> >> net/core/devlink.c                                 | 10 ++++
> >> 10 files changed, 175 insertions(+), 36 deletions(-)
> >>
> >>--
> >>1.8.3.1
> >>
Jiri Pirko June 1, 2020, 9:52 a.m. UTC | #4
Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
>> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
>> >>Live device reset capability allows the users to reset the device in real
>> >>time. For example, after flashing a new firmware image, this feature allows
>> >>a user to initiate the reset immediately from a separate command, to load
>> >>the new firmware without reloading the driver or resetting the system.
>> >>
>> >>When device reset is initiated, services running on the host interfaces
>> >>will momentarily pause and resume once reset is completed, which is very
>> >>similar to momentary network outage.
>> >>
>> >>This patchset adds support for two new generic devlink parameters for
>> >>controlling the live device reset capability and use it in the bnxt_en
>> >>driver.
>> >>
>> >>Users can initiate the reset from a separate command, for example,
>> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
>> >>device.
>> >>Where ethX or dev is any PF with administrative privileges.
>> >>
>> >>Patchset also updates firmware spec. to 1.10.1.40.
>> >>
>> >>
>> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
>> >
>> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
>> >attribute. I don't recall you telling any argument against it. I belive
>> >that this should not be paramater. This is very tightly related to
>> >reload, could you please have it as an attribute of reload, as I
>> >suggested?
>>
>> I just wrote the thread to the previous version. I understand now why
>> you need param as you require reboot to activate the feature.
>
>Okay.
>>
>> However, I don't think it is correct to use enable_live_dev_reset to
>> indicate the live-reset capability to the user. Params serve for
>> configuration only. Could you please move the indication some place
>> else? ("devlink dev info" seems fitting).
>
>Here we are not indicating the support. If the parameter is set to
>true, we are enabling the feature in firmware and driver after reboot.
>Users can disable the feature by setting the parameter to false and
>reboot. This is the configuration which is enabling or disabling the
>feature in the device.

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


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

[...]

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

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

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


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

Right. I see.

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

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

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



>
>Thanks.
>
>>
>>
>> >
>> >Thanks!
>> >
>> >
>> >>"allow_live_dev_reset".
>> >>- Expand the documentation of each param and update commit messages
>> >> accordingly.
>> >>- Separated the permanent configuration mode code to another patch and
>> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
>> >>
>> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
>> >>- Update documentation files and commit messages with more details of the
>> >> feature.
>> >>
>> >>Vasundhara Volam (6):
>> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
>> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
>> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
>> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
>> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
>> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
>> >>
>> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
>> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
>> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
>> >> include/net/devlink.h                              |  8 +++
>> >> net/core/devlink.c                                 | 10 ++++
>> >> 10 files changed, 175 insertions(+), 36 deletions(-)
>> >>
>> >>--
>> >>1.8.3.1
>> >>
Vasundhara Volam June 1, 2020, 10:08 a.m. UTC | #7
On Mon, Jun 1, 2020 at 3:22 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
> >> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >>Live device reset capability allows the users to reset the device in real
> >> >>time. For example, after flashing a new firmware image, this feature allows
> >> >>a user to initiate the reset immediately from a separate command, to load
> >> >>the new firmware without reloading the driver or resetting the system.
> >> >>
> >> >>When device reset is initiated, services running on the host interfaces
> >> >>will momentarily pause and resume once reset is completed, which is very
> >> >>similar to momentary network outage.
> >> >>
> >> >>This patchset adds support for two new generic devlink parameters for
> >> >>controlling the live device reset capability and use it in the bnxt_en
> >> >>driver.
> >> >>
> >> >>Users can initiate the reset from a separate command, for example,
> >> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
> >> >>device.
> >> >>Where ethX or dev is any PF with administrative privileges.
> >> >>
> >> >>Patchset also updates firmware spec. to 1.10.1.40.
> >> >>
> >> >>
> >> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
> >> >
> >> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
> >> >attribute. I don't recall you telling any argument against it. I belive
> >> >that this should not be paramater. This is very tightly related to
> >> >reload, could you please have it as an attribute of reload, as I
> >> >suggested?
> >>
> >> I just wrote the thread to the previous version. I understand now why
> >> you need param as you require reboot to activate the feature.
> >
> >Okay.
> >>
> >> However, I don't think it is correct to use enable_live_dev_reset to
> >> indicate the live-reset capability to the user. Params serve for
> >> configuration only. Could you please move the indication some place
> >> else? ("devlink dev info" seems fitting).
> >
> >Here we are not indicating the support. If the parameter is set to
> >true, we are enabling the feature in firmware and driver after reboot.
> >Users can disable the feature by setting the parameter to false and
> >reboot. This is the configuration which is enabling or disabling the
> >feature in the device.
>
> You are talking about cmode permanent here. I'm talking about cmode
> runtime.
For cmode runtime, I have renamed it to "allow_live_dev_reset". As I
see the comment under "enable_live_dev_reset", I thought you are
talking about permanent cmode.

"allow_live_dev_reset" is runtime cmode, which will allow users to
disable the "live reset" feature temporarily. It just not only
indicate the support but user can configure it to control the "live
reset" at runtime.
>
>
> >
> >>
> >> I think that you can do the indication in a follow-up patchset. But
> >> please remove it from this one where you do it with params.
> >
> >Could you please see the complete patchset and use it bnxt_en driver
> >to get a clear picture? We are not indicating the support.
> >
> >Thanks.
> >
> >>
> >>
> >> >
> >> >Thanks!
> >> >
> >> >
> >> >>"allow_live_dev_reset".
> >> >>- Expand the documentation of each param and update commit messages
> >> >> accordingly.
> >> >>- Separated the permanent configuration mode code to another patch and
> >> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
> >> >>
> >> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
> >> >>- Update documentation files and commit messages with more details of the
> >> >> feature.
> >> >>
> >> >>Vasundhara Volam (6):
> >> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
> >> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
> >> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
> >> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
> >> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
> >> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
> >> >>
> >> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
> >> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> >> >> include/net/devlink.h                              |  8 +++
> >> >> net/core/devlink.c                                 | 10 ++++
> >> >> 10 files changed, 175 insertions(+), 36 deletions(-)
> >> >>
> >> >>--
> >> >>1.8.3.1
> >> >>
Vasundhara Volam June 1, 2020, 10:12 a.m. UTC | #8
On Mon, Jun 1, 2020 at 3:28 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
>
> [...]
>
> > Documentation/networking/devlink/bnxt.rst          |  4 ++
> > .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> > drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> > drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> > drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> > include/net/devlink.h                              |  8 +++
> > net/core/devlink.c                                 | 10 ++++
>
> Could you please cc me to this patchset? use scripts/maintainers to get
> the cc list.
>
> It is also customary to cc people that replied to the previous patchset
> versions.
I am sorry. I copied you only on devlink patches. I will keep in mind
to copy on the entire patchset from next time.

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

Okay, that looks fine to me now.


>>
>>
>> >
>> >>
>> >> I think that you can do the indication in a follow-up patchset. But
>> >> please remove it from this one where you do it with params.
>> >
>> >Could you please see the complete patchset and use it bnxt_en driver
>> >to get a clear picture? We are not indicating the support.
>> >
>> >Thanks.
>> >
>> >>
>> >>
>> >> >
>> >> >Thanks!
>> >> >
>> >> >
>> >> >>"allow_live_dev_reset".
>> >> >>- Expand the documentation of each param and update commit messages
>> >> >> accordingly.
>> >> >>- Separated the permanent configuration mode code to another patch and
>> >> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
>> >> >>
>> >> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
>> >> >>- Update documentation files and commit messages with more details of the
>> >> >> feature.
>> >> >>
>> >> >>Vasundhara Volam (6):
>> >> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
>> >> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
>> >> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
>> >> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
>> >> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
>> >> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
>> >> >>
>> >> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
>> >> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
>> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
>> >> >> include/net/devlink.h                              |  8 +++
>> >> >> net/core/devlink.c                                 | 10 ++++
>> >> >> 10 files changed, 175 insertions(+), 36 deletions(-)
>> >> >>
>> >> >>--
>> >> >>1.8.3.1
>> >> >>
Vasundhara Volam June 1, 2020, 3:31 p.m. UTC | #10
On Mon, Jun 1, 2020 at 3:34 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@resnulli.us wrote:
> >> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >> >>Live device reset capability allows the users to reset the device in real
> >> >>time. For example, after flashing a new firmware image, this feature allows
> >> >>a user to initiate the reset immediately from a separate command, to load
> >> >>the new firmware without reloading the driver or resetting the system.
> >> >>
> >> >>When device reset is initiated, services running on the host interfaces
> >> >>will momentarily pause and resume once reset is completed, which is very
> >> >>similar to momentary network outage.
> >> >>
> >> >>This patchset adds support for two new generic devlink parameters for
> >> >>controlling the live device reset capability and use it in the bnxt_en
> >> >>driver.
> >> >>
> >> >>Users can initiate the reset from a separate command, for example,
> >> >>'ethtool --reset ethX all' or 'devlink dev reload' to reset the
> >> >>device.
> >> >>Where ethX or dev is any PF with administrative privileges.
> >> >>
> >> >>Patchset also updates firmware spec. to 1.10.1.40.
> >> >>
> >> >>
> >> >>v2->v3: Split the param into two new params "enable_live_dev_reset" and
> >> >
> >> >Vasundhara, I asked you multiple times for this to be "devlink dev reload"
> >> >attribute. I don't recall you telling any argument against it. I belive
> >> >that this should not be paramater. This is very tightly related to
> >> >reload, could you please have it as an attribute of reload, as I
> >> >suggested?
> >>
> >> I just wrote the thread to the previous version. I understand now why
> >> you need param as you require reboot to activate the feature.
> >
> >Okay.
> >>
> >> However, I don't think it is correct to use enable_live_dev_reset to
> >> indicate the live-reset capability to the user. Params serve for
> >> configuration only. Could you please move the indication some place
> >> else? ("devlink dev info" seems fitting).
> >
> >Here we are not indicating the support. If the parameter is set to
> >true, we are enabling the feature in firmware and driver after reboot.
> >Users can disable the feature by setting the parameter to false and
> >reboot. This is the configuration which is enabling or disabling the
> >feature in the device.
> >
> >>
> >> I think that you can do the indication in a follow-up patchset. But
> >> please remove it from this one where you do it with params.
> >
> >Could you please see the complete patchset and use it bnxt_en driver
> >to get a clear picture? We are not indicating the support.
>
> Right. I see.
>
> There is still one thing that I see problematic. There is no clear
> semantics on when the "live fw update" is going to be performed. You
> enable the feature in NVRAM and you set to "allow" it from all the host.
> Now the user does reset, the driver has 2 options:
> 1) do the live fw reset
> 2) do the ordinary fw reset
>
> This specification is missing and I believe it should be part of this
> patchset, otherwise the behaviour might not be deterministic between
> drivers and driver versions.

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

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

Okay.

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

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

Thanks.
>
>
>
> >
> >Thanks.
> >
> >>
> >>
> >> >
> >> >Thanks!
> >> >
> >> >
> >> >>"allow_live_dev_reset".
> >> >>- Expand the documentation of each param and update commit messages
> >> >> accordingly.
> >> >>- Separated the permanent configuration mode code to another patch and
> >> >>rename the callbacks of the "allow_live_dev_reset" parameter accordingly.
> >> >>
> >> >>v1->v2: Rename param to "allow_fw_live_reset" from "enable_hot_fw_reset".
> >> >>- Update documentation files and commit messages with more details of the
> >> >> feature.
> >> >>
> >> >>Vasundhara Volam (6):
> >> >>  devlink: Add 'enable_live_dev_reset' generic parameter.
> >> >>  devlink: Add 'allow_live_dev_reset' generic parameter.
> >> >>  bnxt_en: Use 'enable_live_dev_reset' devlink parameter.
> >> >>  bnxt_en: Update firmware spec. to 1.10.1.40.
> >> >>  bnxt_en: Use 'allow_live_dev_reset' devlink parameter.
> >> >>  bnxt_en: Check if fw_live_reset is allowed before doing ETHTOOL_RESET
> >> >>
> >> >> Documentation/networking/devlink/bnxt.rst          |  4 ++
> >> >> .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> >> >> include/net/devlink.h                              |  8 +++
> >> >> net/core/devlink.c                                 | 10 ++++
> >> >> 10 files changed, 175 insertions(+), 36 deletions(-)
> >> >>
> >> >>--
> >> >>1.8.3.1
> >> >>
Jakub Kicinski June 1, 2020, 11:20 p.m. UTC | #11
On Mon, 1 Jun 2020 11:58:38 +0200 Jiri Pirko wrote:
> > Documentation/networking/devlink/bnxt.rst          |  4 ++
> > .../networking/devlink/devlink-params.rst          | 28 ++++++++++
> > drivers/net/ethernet/broadcom/bnxt/bnxt.c          | 28 +++++++++-
> > drivers/net/ethernet/broadcom/bnxt/bnxt.h          |  2 +
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c  | 49 +++++++++++++++++
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h  |  1 +
> > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  | 17 +++---
> > drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h      | 64 +++++++++++++---------
> > include/net/devlink.h                              |  8 +++
> > net/core/devlink.c                                 | 10 ++++  
> 
> Could you please cc me to this patchset? use scripts/maintainers to get
> the cc list.
> 
> It is also customary to cc people that replied to the previous patchset
> versions.

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

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

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

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


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

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

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

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

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