Date:   Mon, 1 Jun 2020 08:43:23 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Vasundhara Volam <vasundhara-v.volam@...adcom.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        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 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.

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

Powered by blists - more mailing lists