Message-ID: <CAACQVJr4eOkW9PQwZBxPN_jVz_+P94xTDJ0sMa1wi-8n_s=ghA@mail.gmail.com>
Date:   Mon, 1 Jun 2020 15:38:14 +0530
From:   Vasundhara Volam <vasundhara-v.volam@...adcom.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Michael Chan <michael.chan@...adcom.com>
Subject: Re: [PATCH v3 net-next 0/6] bnxt_en: Add 'enable_live_dev_reset' and
 'allow_live_dev_reset' generic devlink params.

On Mon, Jun 1, 2020 at 3:22 PM Jiri Pirko <jiri@...nulli.us> wrote:
>
> Mon, Jun 01, 2020 at 10:58:09AM CEST, vasundhara-v.volam@...adcom.com wrote:
> >On Mon, Jun 1, 2020 at 12:13 PM Jiri Pirko <jiri@...nulli.us> wrote:
> >>
> >> Mon, Jun 01, 2020 at 08:18:19AM CEST, jiri@...nulli.us wrote:
> >> >Sun, May 31, 2020 at 09:03:39AM CEST, vasundhara-v.volam@...adcom.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
> >> >>

Powered by blists - more mailing lists