Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update

From: Jacob Keller <jacob.e.keller@intel.com>
To: Jiri Pirko <jiri@resnulli.us>
Cc: Jakub Kicinski <kuba@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Tom Herbert <tom@herbertland.com>, Jiri Pirko <jiri@mellanox.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Michael Chan <michael.chan@broadcom.com>,
	Bin Luo <luobin9@huawei.com>,
	Saeed Mahameed <saeedm@mellanox.com>,
	Leon Romanovsky <leon@kernel.org>,
	Ido Schimmel <idosch@mellanox.com>,
	Danielle Ratson <danieller@mellanox.com>
Subject: Re: [RFC PATCH net-next v2 6/6] devlink: add overwrite mode to flash update
Date: Mon, 27 Jul 2020 11:11:43 -0700	[thread overview]
Message-ID: <9f33b220-6f2c-4f67-6c1d-c676e02d9904@intel.com> (raw)
In-Reply-To: <20200726071834.GC2216@nanopsycho>

On 7/26/2020 12:18 AM, Jiri Pirko wrote:
> Wed, Jul 22, 2020 at 08:21:22PM CEST, jacob.e.keller@intel.com wrote:
>>
>>
>> On 7/22/2020 9:52 AM, Jakub Kicinski wrote:
>>> On Wed, 22 Jul 2020 15:30:05 +0000 Keller, Jacob E wrote:
>>>>> So perhaps we can introduce something like "component mask", which would
>>>>> allow to flash only part of the component. That is basically what Jacob
>>>>> has, I would just like to have it well defined.
>>>>
>>>> So, we could make this selection a series of masked bits instead of a
>>>> single enumeration value.
>>>
>>> I'd still argue that components (as defined in devlink info) and config
>>> are pretty orthogonal. In my experience config is stored in its own
>>> section of the flash, and some of the knobs are in no obvious way
>>> associated with components (used by components).
>>>
>>> That said, if we rename the "component mask" to "update mask" that's
>>> fine with me.
>>>
>>> Then we'd have
>>>
>>> bit 0 - don't overwrite config
>>> bit 1 - don't overwrite identifiers
>>>
>>> ? 
>>>
>>> Let's define a bit for "don't update program" when we actually need it.
>>>
>>
>>
>> Ok. And this can be later extended with additional bits with new
>> meanings should the need arise.
>>
>> Additionally, drivers can ensure that the valid combination of bits is
>> set. the drivers can reject requests for combinations that they do not
>> support.
> 
> Makes sense.
> 
>>
>> I can make that change.
>>
>> My preference is that "0" for a bit means do not overwrite while "1"
>> means overwrite. This way, if/when additional bits are added, drivers
>> won't need to be updated to reject such requests. If we make "1" the "do
>> not overwrite" then we'd have a case where drivers must update to ensure
>> they reject requests which don't set the bit.
> 
> 0 should be default and driver should bahave accordingly.
> 

Correct, and it's good to spell that out more clearly.

Thanks,
Jake

> 
>>
>> Thanks,
>> Jake