Re: [PATCH v3 net-next 2/2] ionic: add devlink firmware update

[Shannon Nelson <snelson@pensando.io>]

On 9/14/20 5:53 PM, Keller, Jacob E wrote:
>
>> -----Original Message-----
>> From: Shannon Nelson <snelson@pensando.io>
>> Sent: Monday, September 14, 2020 4:47 PM
>> To: Jakub Kicinski <kuba@kernel.org>; Keller, Jacob E <jacob.e.keller@intel.com>
>> Cc: netdev@vger.kernel.org; davem@davemloft.net
>> Subject: Re: [PATCH v3 net-next 2/2] ionic: add devlink firmware update
>>
>> On 9/14/20 4:36 PM, Jakub Kicinski wrote:
>>> On Mon, 14 Sep 2020 16:15:28 -0700 Jacob Keller wrote:
>>>> On 9/10/2020 10:56 AM, Jakub Kicinski wrote:
>>>>> IOW drop the component parameter from the normal helper, cause almost
>>>>> nobody uses that. The add a more full featured __ version, which would
>>>>> take the arg struct, the struct would include the timeout value.
>>>>>
>>>> I would point out that the ice driver does use it to help indicate which
>>>> section of the flash is currently being updated.
>>>>
>>>> i.e.
>>>>
>>>> $ devlink dev flash pci/0000:af:00.0 file firmware.bin
>>>> Preparing to flash
>>>> [fw.mgmt] Erasing
>>>> [fw.mgmt] Erasing done
>>>> [fw.mgmt] Flashing 100%
>>>> [fw.mgmt] Flashing done 100%
>>>> [fw.undi] Erasing
>>>> [fw.undi] Erasing done
>>>> [fw.undi] Flashing 100%
>>>> [fw.undi] Flashing done 100%
>>>> [fw.netlist] Erasing
>>>> [fw.netlist] Erasing done
>>>> [fw.netlist] Flashing 100%
>>>> [fw.netlist] Flashing done 100%
>>>>
>>>> I'd like to keep that, as it helps tell which component is currently
>>>> being updated. If we drop this, then either I have to manually build
>>>> strings which include the component name, or we lose this information on
>>>> display.
>>> Thanks for pointing that out. My recollection was that ice and netdevsim
>>> were the only two users, so I thought those could use the full __*
>>> helper and pass an arg struct. But no strong feelings.
>> Thanks, both.
>>
>> I'd been going back and forth all morning about whether a simple single
>> timeout or a timeout for each "chunk" would be appropriate. I'll try to
>> be back in another day or three with an RFC.
>>
>> sln
> For ice, a timeout for each message/chunk makes the most sense, but I could see  a different reasoning when you have multiple steps all bounded by the same timeout
>
> Thanks,
> Jake
>

So now we're beginning to dance around timeout boundaries - how can we 
define the beginning and end of a timeout boundary, and how do they 
relate to the component and label?  Currently, if either the component 
or status_msg changes, the devlink user program does a newline to start 
a new status line.  The done and total values are used from each notify 
message to create a % value displayed, but are not dependent on any 
previous done or total values, so the total doesn't need to be the same 
value from status message to status message, even if the component and 
label remain the same, devlink will just print whatever % gets 
calculated that time.

I'm thinking that the behavior of the timeout value should remain 
separate from the component and status_msg values, such that once given, 
then the userland countdown continues on that timeout.  Each subsequent 
notify, regardless of component or label changes, should continue 
reporting that same timeout value for as long as it applies to the 
action.  If a new timeout value is reported, the countdown starts over.  
This continues until either the countdown finishes or the driver reports 
the flash as completed.  I think this allows is the flexibility for 
multiple steps that Jake alludes to above.  Does this make sense?

What should the userland program do when the timeout expires?  Start 
counting backwards?  Stop waiting?  Do we care to define this at the moment?

sln