Tue, May 19, 2020 at 01:43:09AM CEST, k...@kernel.org wrote:
>On Mon, 18 May 2020 13:01:52 +0200 Jiri Pirko wrote:
>> Mon, May 18, 2020 at 10:27:15AM CEST, vasundhara-v.vo...@broadcom.com wrote:
>> >This patchset adds support for a "enable_hot_fw_reset" generic devlink
>> >parameter and use it in bnxt_en driver.
>> >
>> >Also, firmware spec. is updated to 1.10.1.40.
>>
>> Hi.
>>
>> We've been discussing this internally for some time.
>> I don't like to use params for this purpose.
>> We already have "devlink dev flash" and "devlink dev reload" commands.
>> Combination of these two with appropriate attributes should provide what
>> you want. The "param" you are introducing is related to either "flash"
>> or "reload", so I don't think it is good to have separate param, when we
>> can extend the command attributes.
>>
>> How does flash&reload work for mlxsw now:
>>
>> # devlink flash
>> Now new version is pending, old FW is running
>> # devlink reload
>> Driver resets the device, new FW is loaded
>>
>> I propose to extend reload like this:
>>
>> devlink dev reload DEV [ level { driver-default | fw-reset | driver-only |
>> fw-live-patch } ]
>> driver-default - means one of following to, according to what is
>> default for the driver
>> fw-reset - does FW reset and driver entities re-instantiation
>> driver-only - does driver entities re-instantiation only
>> fw-live-patch - does only FW live patching - no effect on kernel
>>
>> Could be an enum or bitfield. Does not matter. The point is to use
>> reload with attribute to achieve what user wants. In your usecase, user
>> would do:
>>
>> # devlink flash
>> # devlink reload level fw-live-patch
>
>Unfortunately for SmartNICs and MultiHost systems the reset may not be
>initiated locally. I agree it'd be great to have a normal netlink knob
I don't follow. Locally initiated or not, why what I suggested is not
enough to cover that?
>for this instead of a param. But it has to be some form of a policy of
>allowing the reset to happen, rather than an action/trigger kind of
>thing.
The "host" allows to reset himself by the "smartnic", right? For that, I
can imagine a param. But that is not the case of this patchset.
>
>Also user space notification should be generated when reset happens,
>IMO. devlink dev info contents will likely change after reset, if
>nothing else.
I agree.
>
>Plus this functionality will need proper documentation.
Also agreed.
>
>FWIW - I am unconvinced that applications will be happy to experience
>network black outs, rather than being fully killed and re-spawned. For
>a micro-service worker shutdown + re-spawn should be bread and butter.
>But we already have ionic doing this, so seems like vendors are
>convinced otherwise, so a common interface is probably a good step.
Hmm, not sure I follow what you mean by this para in context of this
patchset. Could you please explain? Thanks!
