Message-ID: <93b7cd42-0af7-6066-ba42-a18755f84863@intel.com>
Date:   Mon, 14 Sep 2020 16:19:09 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Shannon Nelson <snelson@...sando.io>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH v3 net-next 2/2] ionic: add devlink firmware update



On 9/10/2020 10:56 AM, Jakub Kicinski wrote:
> On Wed, 9 Sep 2020 18:34:57 -0700 Shannon Nelson wrote:
>> On 9/9/20 12:22 PM, Jakub Kicinski wrote:
>>> On Wed, 9 Sep 2020 10:58:19 -0700 Shannon Nelson wrote:  
>>>>
>>>> I'm suggesting that this implementation using the existing devlink
>>>> logging services should suffice until someone can design, implement, and
>>>> get accepted a different bit of plumbing.  Unfortunately, that's not a
>>>> job that I can get to right now.  
>>> This hack is too nasty to be accepted.  
>>
>> Your comment earlier was
>>
>>  > I wonder if we can steal a page from systemd's book and display
>>  > "time until timeout", or whatchamacallit, like systemd does when it's
>>  > waiting for processes to quit. All drivers have some timeout set on the
>>  > operation. If users knew the driver sets timeout to n minutes and they
>>  > see the timer ticking up they'd be less likely to think the command has
>>  > hanged..  
>>
>> I implemented the loop such that the timeout value was the 100%, and 
>> each time through the loop the elapsed time value is sent, so the user 
>> gets to see the % value increasing as the wait goes on, in the same way 
>> they see the download progress percentage ticking away. 
> 
> Right but you said that in most cases the value never goes up to 25min,
> so user will see the value increment from 0 to say 5% very slowly and
> then jump to 100%.
> 
>> This is how I approached the stated requirement of user seeing the
>> "timer ticking up", using the existing machinery.  This seems to be
>> how devlink_flash_update_status_notify() is expected to be used, so
>> I'm a little surprised at the critique.
> 
> Sorry, I thought the systemd reference would be clear enough, see the
> screenshot here:
> 
> https://images.app.goo.gl/gz1Uwg6mcHEd3D2m7
> 
> Systemd prints something link:
> 
> bla bla bla (XXs / YYs)
> 
> where XX is the timer ticking up, and YY is the timeout value.
> 
>>> So to be clear your options are:
>>>   - plumb the single extra netlink parameter through to devlink
>>>   - wait for someone else to do that for you, before you get
>>> firmware flashing accepted upstream.
>>>  
>>
>> Since you seem to have something else in mind, a little more detail 
>> would be helpful.
>>
>> We currently see devlink updating a percentage, something like:
>> Downloading:  56%
>> using backspaces to overwrite the value as the updates are published.
>>
>> How do you envision the userland interpretation of the timeout
>> ticking? Do you want to see something like:
>> Installing - timeout seconds:  23
>> as a countdown?
> 
> I was under the impression that the systemd format would be familiar 
> to users, hence:
> 
> Downloading:  56% (Xm Ys / Zm Vz)

FWIW, I like this approach. I think all we need to implement it is to
send the additional timeout parameter as part of the status notify
command. Then, devlink userspace, if it sees a timeout can choose when
to start displaying the "(Xm Ys / Zm Vs)" portion. Userspace could track
elapsed time changed in its event loop until the message changes.

This would also work for the ice driver, where we indicate that an erase
command could take up to several minutes as well.

Thanks,
Jake

Powered by blists - more mailing lists