On Tue, 19 May 2020 07:24:00 +0200 Jiri Pirko wrote:
> 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?
Hopefully clear now after Michael's explanation :)
> >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!
I'm saying I'm dubious users will actually enable the async remote
reset.
