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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3d75c4be-ae5d-43b0-407c-5df1e7645447@pensando.io>
Date:   Wed, 9 Sep 2020 18:34:57 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH v3 net-next 2/2] ionic: add devlink firmware update

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. 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.

> 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?

So, maybe a flag parameter that can tell the UI to use the raw value and 
not massage it into a percentage?

Do you see this new netlink parameter to be a boolean switch between the 
percentage and raw, or maybe a bitflag parameter that might end up with 
several bits of context information for userland to interpret?

Are you thinking of a new flags parameter in 
devlink_flash_update_status_notify(), or a new function to service this?

If a new parameter to devlink_flash_update_status_notify(), maybe it is 
time to make a struct for flash update data rather than adding more 
parameters to the function?

Should we add yet another parameter to replace the '%' with some other 
label, so devlink could print something like
Installing - timeout in:  23 secs

Or could we use a 0 value for total to signify using a raw value and not 
need to plumb a new parameter?  Although this might not get along well 
with older devlink utilities.

Thoughts?

sln