Message-ID: <20200601100411.GL2282@nanopsycho>
Date:   Mon, 1 Jun 2020 12:04:11 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Vasundhara Volam <vasundhara-v.volam@...adcom.com>
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.

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.
>
>>
>> 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
>> >>

Powered by blists - more mailing lists