Message-ID: <20181108162238.GA5515@splinter.mtl.com>
Date:   Thu, 8 Nov 2018 18:22:38 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Ido Schimmel <idosch@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        Shalom Toledo <shalomt@...lanox.com>,
        Moshe Shemesh <moshe@...lanox.com>,
        "dsahern@...il.com" <dsahern@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        mlxsw <mlxsw@...lanox.com>
Subject: Re: [PATCH net-next 1/3] devlink: Add fw_version_check generic
 parameter

On Wed, Nov 07, 2018 at 11:05:18AM -0800, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 12:11:32 +0200, Ido Schimmel wrote:
> > 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:  
> > > > > 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. 
> 
> Correct.
> 
> > You potentially load the firmware from the disk, but never flash it?
> 
> You can flash it if you want, but default is to load the
> linux-firmware/disk one when system boots.  
> 
> Flashing is useful for example if you have some super special FW that
> you prefer over whatever comes from linux-firmware updates, or (most
> commonly) if distributing FWs is hard in your provisioning system (ugh).
> 
> > If so, why load it?
> 
> We need to load some FW..

OK. Got it now. mlxsw must flash a firmware in order to load it, but in
your case it's not necessary.

> > > 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.
> 
> Certainly, let me know if what I wrote above helps to understand the
> motivation.

Yes, it does. Thanks!

> 
> > 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.
> 
> 100% agree, you can choose the default for the parameter to be whatever
> you want, nobody will have to touch it in normal operation.
> 
> I'm just proposing widening the values of the parameter so it works for
> others (given you propose it as generic).
> 
> > 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.
> 
> Thanks, got it.
> 
> > I think this is both generic and simple, but I possibly didn't
> > understand the full scope of your use cases.
> 
> I agree, it is simple, but the semantics of the parameter you're adding
> unnecessarily involve firmware flashing, which is mlxsw quirk.  My
> impression (mostly from my WiFi driver days) is that the FW is either in
> flash or in /lib/firmware, but rarely /lib/firmware autofeeds the flash.
> 
> The commit message says "Many drivers checking the device's firmware
> version during the initialization flow and flashing a compatible
> version if the current version is not."  Do Ethernet drivers do that 
> or some other drivers?

Yes, the terminology is not accurate. The intention was that drivers
load (we used flashing...) a compatible firmware version during their
initialization routine.

> To reiterate I think the definition of the flag unnecessarily involves
> flashing.  It may make sense to mlxsw users, but I'd postulate it won't
> to almost everyone else :)
> 
> Therefore AFAICS we could make one of two improvements:
>  - make this a full-fledged flash update policy with clear operational
>    semantics which others can reuse; or 
>  - remove the flashing part and leave it unmentioned - definition of the
>    parameter becomes in a nutshell "ignore all FW version
>    incompatibilities"; the limitation of mlxsw having to flash a FW
>    image to load it is then an implementation detail.

Discussed the topic with Jakub during the bi-weekly switchdev call. We
will submit a v2 with a parameter called 'fw_load_policy' that will have
these options:

* driver: Load firmware version preferred by the driver (default in 
  mlxsw)
* flash: Load firmware currently stored in flash

Therefore, new sequence is:

1. Flash new firmware via ethtool
2. Set devlink 'fw_load_policy' parameter to 'flash'
3. Perform a reset via devlink to have changes take effect

The parameter can be later extended with more options such as 'newest'
and 'disk' that Jakub mentioned earlier in the thread.

We will not add the 'fw_flash_auto_update' parameter as it is not needed
for mlxsw, but can be added for nfp/others in the future.

Jakub, thanks again for your time!

Powered by blists - more mailing lists