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

From: Jacob Keller <jacob.e.keller@intel.com> 
Sent: Monday, February 1, 2021 3:09 PM
To: Jakub Kicinski <kuba@kernel.org>
Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; netdev@vger.kernel.org; sassmann@redhat.com; Brelinski, TonyX <redacted>
Subject: Re: [PATCH net-next 10/15] ice: display some stored NVM versions via devlink info



On 2/1/2021 2:34 PM, Jakub Kicinski wrote:

On Mon, 1 Feb 2021 13:40:27 -0800 Jacob Keller wrote:

quoted

On 1/29/2021 10:37 PM, Jakub Kicinski wrote:

quoted

On Thu, 28 Jan 2021 16:43:27 -0800 Tony Nguyen wrote:  

quoted

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

Sure, ok. I can add that when implementing this in the v2 submission (along with extracting the security revision patches to a separate series).

Thanks,
Jake

------------------------------------------------------------------
I tested this revision:
Tested-by: Tony Brelinski <redacted> A Contingent Worker at Intel