Re: [Intel-wired-lan] [PATCH net v1] ice: Fix inventory failed error during flash update

From: Tony Nguyen <anthony.l.nguyen@intel.com>
To: "Dziedziuch, SylwesterX" <sylwesterx.dziedziuch@intel.com>,
	"Palczewski, Mateusz" <mateusz.palczewski@intel.com>,
	"intel-wired-lan@lists.osuosl.org"
	<intel-wired-lan@lists.osuosl.org>,
	"Keller, Jacob E" <jacob.e.keller@intel.com>
Subject: Re: [Intel-wired-lan] [PATCH net v1] ice: Fix inventory failed error during flash update
Date: Fri, 19 Aug 2022 14:38:16 -0700	[thread overview]
Message-ID: <f3ef82cc-e372-443a-7f3b-8fd854332763@intel.com> (raw)
In-Reply-To: <PH7PR11MB5768669AC890675FE5E87034E66C9@PH7PR11MB5768.namprd11.prod.outlook.com>

On 8/19/2022 12:21 AM, Dziedziuch, SylwesterX wrote:
>> On 8/11/2022 4:45 AM, Mateusz Palczewski wrote:
>>> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
>>>
>>> After updating flash image on E810 card with NVM update tool there was
>>> an error: The inventory process failed.
>>>
>>> This was reported at bugzilla thread #2114483 and caused by the tool
>>> trying to read devlink parameters fw.mgmt.minsrev and fw.undi.minsrev
>>> but those parameters were not registered by the driver.
>>
>> Pointing to an issue when using with our userspace tool is not a good
>> justification of why this should be accepted into the kernel.
>>
>>> The ice NVM flash has a security revision field for the main NVM bank
>>> and the Option ROM bank. In addition to the revision within the
>>> module, the device also has a minimum security revision TLV area. This
>>> minimum security revision field indicates the minimum value that will
>>> be accepted for the associated security revision when loading the NVM
>> bank.
>>>
>>> These parameters are permanent (i.e. stored in flash), and are used to
>>> indicate the minimum security revision of the associated NVM bank. If
>>> the image in the bank has a lower security revision, then the flash
>>> loader will not continue loading that flash bank.
>>>
>>> Fix this by adding two new devlink parameters fw.mgmt.minsrev and
>>> fw.undi.minsrev and function to read they respective values.
>>>
>>> This idea was proposed before with both write and read funcionality
>>> but was rejected by community. This patch focuses on read only.
>>
>> How is this different/addresses the issues that caused it to be rejected
>> initially? What makes it acceptable now?
> 
> One of the concerns in the previous review was that we give the ability to change those values manually which might cause security issues. So in this change we are not allowing to modify those values only to read them for the update process to finish without errors.

Let's put this patch on pause for the moment and discuss internally. I 
think, at the very least, the commit message would need some updates to 
it so review could continue on a v2.

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan