From: Jacob Keller <jacob.e.kel...@intel.com> Sent: Monday, February 1, 2021 3:09 PM To: Jakub Kicinski <k...@kernel.org> Cc: Nguyen, Anthony L <anthony.l.ngu...@intel.com>; da...@davemloft.net; netdev@vger.kernel.org; sassm...@redhat.com; Brelinski, TonyX <tonyx.brelin...@intel.com> 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: >> 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-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 <tonyx.brelin...@intel.com> A Contingent Worker at Intel
