Message-ID: <20200715162329.4224fa6f@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 15 Jul 2020 16:23:29 -0700
From:   Jakub Kicinski <kubakici@...pl>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Tom Herbert <tom@...bertland.com>
Subject: Re: [RFC PATCH net-next 6/6] ice: implement devlink parameters to
 control flash update

On Wed, 15 Jul 2020 14:41:04 -0700 Jacob Keller wrote:
> To summarize this discussion, the next spin will have the following changes:
> 
> 1) remove all parameters except for the preservation_level. Both
> ignore_pending_flash_update and allow_downgrade_on_flash_update will be
> removed and change the default behavior to the most accepting case:
> updates will always be tried even if firmware says its a downgrade, and
> we will always cancel a pending update. We will now expect user space
> tools to be aware of this and handle the equivalent options themselves
> if they desire.
> 
> 2) reset_after_flash_update will be removed, and we will replace it with
> a new interface, perhaps like the devlink reset command suggested in
> another thread.
> 
> 3) preservation_level will remain, but I have updated the documentation
> slightly.

Okay, then. But let's make it a parameter to the flash update operation
(extend the uAPI), rather than a devlink param, shall we?

> Unfortunately it looks like FACTORY_SETTINGS option is not directly
> available without doing an update. It may work with a sort of "update to
> the same version" but I'm not sure if or how we could implement that
> silently in the driver. There's no other way to ask firmware to perform
> factory reset though. Otherwise I would remove this and make it part of
> a new command.

If the settings are restored from within the device and not the flashed
image - it sounds like it's a matter of a FW change to add this
functionality. Right? Maybe it's not immediately necessary if we go
with the new option to flashing.

> I'd also like to clarify the reasoning behind all of the options. The
> preservation is referring to "what to keep in the existing NVM", so
> "PRESERVE_ALL" is the one where the most fields and data are kept by the
> firmware when updating. In this mode, we do not change any settings,
> device-specific fields, or other configuration. This is the default.
> With "PRESERVE_LIMITED" the limited subset of device-specific fields are
> preserved, but all of the settings and configuration are overwritten.
> With PRESERVE_NONE, we simply write what is in the image.

IMO we need to come up with names for the reset levels which
correspond to what's being reset more directly. Ones which will
actually be meaningful without a 4-email-exchange with the developer :) 

It makes immediate sense to me to reset any subset of { settings,
identifying information }. But I don't see how reset to factory
defaults fits into this. Is it controlling the source of the defaults?
I.e. reset to factory defaults vs reset to FW build defaults?

> The intent behind this parameter is to enable our existing tools to
> learn the devlink tool while being able to maintain existing behavior.
> For other operating systems, these tools support the preservation level,
> so without this parameter, we would not be able to support it. The
> expectation is that most of the time PRESERVE_ALL is the correct mode.
> However, the other options do have some usefulness, either when
> debugging or to recover from bad situations such as if the firmware
> preservation doesn't behave properly as expected.
> 
> I hope this information further clarifies our goals and why I believe
> the parameter is valuable.

Powered by blists - more mailing lists