Message-ID: <20210201143404.7e4a093b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 1 Feb 2021 14:34:04 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
        netdev@...r.kernel.org, sassmann@...hat.com,
        Tony Brelinski <tonyx.brelinski@...el.com>
Subject: Re: [PATCH net-next 10/15] ice: display some stored NVM versions
 via devlink info

On Mon, 1 Feb 2021 13:40:27 -0800 Jacob Keller wrote:
> On 1/29/2021 10:37 PM, Jakub Kicinski wrote:
> > On Thu, 28 Jan 2021 16:43:27 -0800 Tony Nguyen wrote:  
> >> When reporting the versions via devlink info, first read the device
> >> capabilities. If there is a pending flash update, use this new function
> >> to extract the inactive flash versions. Add the stored fields to the
> >> flash version map structure so that they will be displayed when
> >> available.  
> > 
> > Why only report them when there is an update pending?
> > 
> > The expectation was that you'd always report what you can and user 
> > can tell the update is pending by comparing the fields.
> 
> If there is no pending update, what is the expected behavior? We report
> the currently active image version as both stored and running?
> 
> In our case, the device has 2 copies of each of the 3 modules: NVM,
> Netlist, and UNDI/OptionROM.
> 
> For each module, the device has a bit that indicates whether it will
> boot from the first or second bank of the image. When we update,
> whichever bank is not active is erased, and then populated with the new
> image contents. The bit indicating which bank to load is flipped. Once
> the device is rebooted (EMP reset), then the new bank is loaded, and the
> firmware performs some onetime initialization.
> 
> So for us, in theory we have up to 2 versions within the device for each
> bank: the version in the currently active bank, and a version in the
> inactive bank. In the inactive case, it may or may not be valid
> depending on if that banks contents were ever a valid image. On a fresh
> card, this might be empty or filled with garbage.
> 
> Presumably we do not want to report that we have "stored" a version
> which is not going to be activated next time that we boot?
> 
> The documentation indicated that stored should be the version which
> *will* be activated.
> 
> If I just blindly always reported what was inactive, then the following
> scenarios exist:
> 
> # Brand new card:
> 
> running:
>   fw.bundle_id: Version
> stored
>   fw.bundle_id: <zero or garbage>
> 
> # Do an update:
> 
> running:
>   fw.bundle_id: Version
> stored
>   fw.bundle_id: NewVersion
> 
> # reset/reboot
> 
> running:
>   fw.bundle_id: NewVersion
> stored:
>   fw.bundle_id: Version
> 
> 
> I could get behind that if we do not have a pending update we report the
> stored value as the same as the running value (i.e. from the active
> bank), where as if we have a pending update that will be triggered we
> would report the inactive bank. I didn't see the value in that before
> because it seemed like "if you don't have a pending update, you don't
> have a stored value, so just report the active version in the running
> category")
> 
> It's also plausibly useful to report the stored but not pending value in
> some cases, but I really don't want to report zeros or garbage data on
> accident. This would almost certainly lead to confusing support
> conversations.

Very good points. Please see the documentation for example workflow:

https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html#firmware-version-management

The FW update agent should be able to rely on 'stored' for checking if
flash update is needed.

If the FW update is not pending just report the same values as running.
You should not report old version after 2 flashings (3rd output in your
example) - that'd confuse the flow - as you said - the stored versions
would not be what will get activated.

Powered by blists - more mailing lists