Message-ID: <20181107110518.6502665a@cakuba.netronome.com>
Date:   Wed, 7 Nov 2018 11:05:18 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Ido Schimmel <idosch@...sch.org>
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, 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..

> > 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.

> 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?

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.

Powered by blists - more mailing lists