Re: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info

[Jacob Keller <jacob.e.keller@intel.com>]

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.