Re: [PATCH net-next 1/3] devlink: Add fw_version_check generic parameter

[Ido Schimmel <idosch@idosch.org>]

On Tue, Nov 06, 2018 at 02:47:13PM -0800, Jakub Kicinski wrote:
> On Tue, 6 Nov 2018 22:37:51 +0200, Ido Schimmel wrote:
> > On Tue, Nov 06, 2018 at 12:19:13PM -0800, Jakub Kicinski wrote:
> > > On Tue, 6 Nov 2018 20:05:00 +0000, Ido Schimmel wrote:  
> > > > From: Shalom Toledo <shalomt@mellanox.com>
> > > > 
> > > > Many drivers checking the device's firmware version during the
> > > > initialization flow and flashing a compatible version if the current
> > > > version is not.
> > > > 
> > > > fw_version_check gives the ability to skip this check which allows to run
> > > > the device with a different firmware version than required by the driver
> > > > for testing and/or debugging purposes.
> > > > 
> > > > Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
> > > > Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> > > > Signed-off-by: Ido Schimmel <idosch@mellanox.com>  
> > > 
> > > The documentation is missing, so it's hard to comment on the definition
> > > of the parameter...    
> > 
> > I assume you mean Documentation/networking/devlink-params.txt ?
> 
> Yes
> 
> > > We have a FW loading policy for NFP, too, so it'd be good to see if we
> > > can find a common ground.  
> > 
> > If the parameter is set, then device runs with whatever firmware version
> > was last flashed (via ethtool, for example). Otherwise, the driver will
> > flash a version according to its policy. In mlxsw, it is a specific
> > version.
> > 
> > Will that work for you?
> 
> Our FW is always backward compatible so there is no need to downgrade.
> 
> What we have is this more along these lines: there are two images one
> on disk and second in the flash.  The FW loading policy can decide
> which of those should be preferred, or should the versions be compared
> and the newer one win (default).  But we don't flash the newer FW, just
> potentially load it from disk today.

Not sure I understand. You have a currently flashed firmware and another
firmware image on disk. You potentially load the firmware from the disk,
but never flash it? If so, why load it?

> I'm not sure whether 'fw_version_check' describes the general behaviour
> of not updating the FW in flash.  The policy of updating the FW in the
> flash if the one on disk is newer seems to be something we could adopt
> as well.  Can we come up with a more general parameter which could
> select FW loading policy that'd for both cases?
> 
> Would values like these make any sense to you?
>  - driver preferred (your default behaviour, we don't support since
>    driver doesn't care);
>  - newest (our default, device compares images and picks newer);
>  - always disk (always run with what's on the disk, regardless of
>    versions);
>  - always flash (always run with what's already in flash, don't look at
>    disk);
> 
> Separate bool parameter 'fw_flash_auto_update' would decide whether the
> selected FW should be flashed to the device (always true for you AFAIU).
> 
> Let me know if that makes sense, it would be nice if we could converge
> on a common solution, or at least name our parameters sufficiently
> distinctly to avoid confusion :)

I think that the above scheme is a bit too complicated and I'm not sure
this is warranted. I'll try to better explain the motivation for this
parameter and where we are coming from.

We want to keep things as simple as possible. This means we don't want
users to fiddle with devlink parameter unless they have to. Things
should just work. This parameter should only be used in exceptional
cases.

For example, when user reports a problem with current firmware version
enforced by the driver. Assuming we have a new firmware version with a
fix, we would like the user to try it and confirm bug was fixed.
Ideally, the user would do something like this:

1. Flash new firmware via ethtool
2. Perform a reset via devlink to have changes take effect

Problem is that after the reset the driver's init sequence will run and
overwrite the new firmware version with the one specified in its source
as a compatible version. The driver needs to enforce a specific version
because newer versions are not necessarily backward compatible.

Therefore, we added this new parameter that gives the user the ability
to explicitly run with a different version than what was specified as
compatible. New sequence is therefore:

1. Flash new firmware via ethtool
2. Toggle devlink parameter
3. Perform a reset via devlink to have changes take effect

Firmware loading policy is basically always go with what the driver is
enforcing (it knows best), unless user specified he/she knows better.

I think this is both generic and simple, but I possibly didn't
understand the full scope of your use cases.