Date:   Mon, 20 Jul 2020 11:52:58 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kubakici@...pl>, Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, Tom Herbert <tom@...bertland.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Michael Chan <michael.chan@...adcom.com>,
        Bin Luo <luobin9@...wei.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Leon Romanovsky <leon@...nel.org>,
        Ido Schimmel <idosch@...lanox.com>,
        Danielle Ratson <danieller@...lanox.com>
Subject: Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash
 update



On 7/20/2020 8:51 AM, Jakub Kicinski wrote:
> On Mon, 20 Jul 2020 12:09:53 +0200 Jiri Pirko wrote:
>> This looks odd. You have a single image yet you somehow divide it
>> into "program" and "config" areas. We already have infra in place to
>> take care of this. See DEVLINK_ATTR_FLASH_UPDATE_COMPONENT.
>> You should have 2 components:
>> 1) "program"
>> 2) "config"
>>

First off, unfortunately at least for ice, the "main" section of NVM
contains both the management firmware as well as config settings. I
don't really have a way to split it up.

This series includes support for updating the main NVM section
containing the management firmware (and some config) "fw.mgmt", as well
as "fw.undi" which contains the OptionROM, and "fw.netlist" which
contains additional configuration TLVs.

The firmware interface allows me to separate the three components, but
does not let me separate the "fw binary" from the "config settings" that
are stored within the main NVM bank. (These fields include other data
like the device MAC address and VPD area of the device too, so using
"config" is a bit of a misnomer).

>> Then it is up to the user what he decides to flash.
> 
> 99.9% of the time users want to flash "all". To achieve "don't flash
> config" with current infra users would have to flash each component 
> one by one and then omit the one(s) which is config (guessing which 
> one that is based on the name).
> 
> Wouldn't this be quite inconvenient?
> 

I also agree here, I'd like to be able to make the "update with the
complete file" just work in the most straight forward  way (i.e. without
erasing stuff by accident) be the default.

The option I'm proposing here is to enable allowing tools to optionally
specify handling this type of overwrite. The goal is to support these
use cases:

a) (default) just update the image, but keep the config and vital data
the same as before the update.

b) overwrite config fields, but keep vital fields the same. Intended to
allow returning configuration to "image defaults". This mostly is
intended in case regular update caused some issues like if somehow the
config preservation didn't work properly.

c) overwrite all fields. The intention here is to allow programming a
customized image during initial setup that would contain new IDs etc. It
is not expected to be used in general, as this does overwrite vital data
like the MAC addresses and such.

So the problem is that the vital data, config data, and firmware
binaries are stored in the same section, without a good way to separate
between them. We program all of these together as one chunk to the
"secondary NVM bank"  and then ask firmware to update. It reads through
and based on our "preservation" setting will update the binaries and
merge the configuration sections.

> In case of MLX is PSID considered config?
> 

Powered by blists - more mailing lists