Re: [PATCH v2 net-next 1/4] devlink: Add new "allow_fw_live_reset" generic device parameter.

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 28 May 2020 07:20:00 +0530 Vasundhara Volam wrote:
> > > The permanent value should be the NVRAM value.  If the NVRAM value is
> > > false, the feature is always and unconditionally disabled.  If the
> > > permanent value is true, the feature will only be available when all
> > > loaded drivers indicate support for it and set the runtime value to
> > > true.  If an old driver is loaded afterwards, it wouldn't indicate
> > > support for this feature and it wouldn't set the runtime value to
> > > true.  So the feature will not be available until the old driver is
> > > unloaded or upgraded.  
> >
> > Setting this permanent value to false makes the FW's life easier?  
> 
> It just disables the feature.
> 
> > Otherwise why not always have it enabled and just depend on hosts
> > not opting in?  
> 
> We are providing permanent value as a flexibility to user. We can
> remove it, if it makes things easy and clear.

I'd think less knobs means less to understand for the user and less
to test for the vendor. If disabling the feature doesn't buy FW
anything then perhaps it can just serve the purpose of setting the
default?

> > > > > 3. Now enable the capability in the device and reboot for device to
> > > > > enable the capability. Firmware does not get reset just by setting the
> > > > > param to true.
> > > > >
> > > > > $ devlink dev param set pci/0000:3b:00.1 name allow_fw_live_reset
> > > > > value true cmode permanent
> > > > >
> > > > > 4. After reboot, values of param.  
> > > >
> > > > Is the reboot required here?  
> > >
> > > In general, our new NVRAM permanent parameters will take effect after
> > > reset (or reboot).
> > >  
> > > > > $ devlink dev param show pci/0000:3b:00.1 name allow_fw_live_reset
> > > > > pci/0000:3b:00.1:
> > > > >   name allow_fw_live_reset type generic
> > > > >     values:
> > > > >       cmode runtime value true  
> > > >
> > > > Why is runtime value true now?
> > > >  
> > >
> > > If the permanent (NVRAM) parameter is true, all loaded new drivers
> > > will indicate support for this feature and set the runtime value to
> > > true by default.  The runtime value would not be true if any loaded
> > > driver is too old or has set the runtime value to false.  
> >
> > Okay, the parameter has a bit of a dual role as it controls whether the
> > feature is available (false -> true transition requiring a reset/reboot)
> > and the default setting of the runtime parameter. Let's document that
> > more clearly.  
> Please look at the 3/4 patch for more documentation in the bnxt.rst
> file. We can add more documentation, if needed, in the bnxt.rst file.

Ack, I read that, but it wasn't nearly clear enough. The command
examples helped much more.  I think the doc should be expanded.