netdev - Re: [PATCH net-next 1/2] devlink: add fw bank select parameter

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]

Date:   Mon, 12 Dec 2022 10:04:37 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Shannon Nelson <shannon.nelson@....com>, <netdev@...r.kernel.org>,
        <davem@...emloft.net>, <jiri@...dia.com>
Subject: Re: [PATCH net-next 1/2] devlink: add fw bank select parameter



On 12/8/2022 5:24 PM, Jakub Kicinski wrote:
> On Thu, 8 Dec 2022 16:47:31 -0800 Jacob Keller wrote:
>> This is what I was thinking of and looks good to me. As for how to add
>> attributes to get us from the current netlink API to this, I'm not 100%
>> sure.
>>
>> I think we can mostly just add the bank ID and flags to indicate which
>> one is active and which one will be programmed next.
> 
> Why flags, tho?
> 
> The current nesting is:
> 
>    DEVLINK_ATTR_INFO_DRIVER_NAME		[str]
>    DEVLINK_ATTR_INFO_SERIAL_NUMBER	[str]
>    DEVLINK_ATTR_INFO_BOARD_SERIAL_NUMBER	[str]
> 
>    DEVLINK_ATTR_INFO_VERSION_FIXED	[nest] // multiple VERSION_* nests follow
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_FIXED	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
> 
> 
> Now we'd throw the bank into the nests, and add root attrs for the
> current / flash / active as top level attrs:
> 
>    DEVLINK_ATTR_INFO_DRIVER_NAME		[str]
>    DEVLINK_ATTR_INFO_SERIAL_NUMBER	[str]
>    DEVLINK_ATTR_INFO_BOARD_SERIAL_NUMBER	[str]
>    DEVLINK_ATTR_INFO_BANK_ACTIVE		[u32] // << optional
>    DEVLINK_ATTR_INFO_BANK_UPDATE_TGT	[u32] // << optional
> 
>    DEVLINK_ATTR_INFO_VERSION_FIXED	[nest] // multiple VERSION_* nests follow
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_FIXED	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_RUNNING	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>    DEVLINK_ATTR_INFO_VERSION_STORED	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE	[str]
>      DEVLINK_ATTR_INFO_VERSION_BANK	[u32] // << optional
>    DEVLINK_ATTR_INFO_VERSION_STORED	[nest]
>      DEVLINK_ATTR_INFO_VERSION_NAME	[str]
>      DEVLINK_ATTR_INFO_VERSION_VALUE     [str]
>      DEVLINK_ATTR_INFO_VERSION_BANK	[u32] // << optional
> 


Yea this is what I was thinking. With this change we have:

old kernel, old devlink - behaves as today
old kernel, new devlink - prints "unknown bank"
new kernel, old devlink - old devlink should ignore the attribute
new kernel, new devlink - prints bank info along with version

So I don't see any issue with adding these attributes getting confused 
when working with old or new userspace.

>> I think we could also add a new attribute to both reload and flash which
>> specify which bank to use. For flash, this would be which bank to
>> program, and for update this would be which bank to load the firmware
>> from when doing a "fw_activate".
> 
> SG!
> 
>> Is that reasonable? Do you still need a permanent "use this bank by
>> default" parameter as well?
> 
> I hope we cover all cases, so no param needed?

The only reason one might want a parameter is if we want to change some 
default. For example I think I saw some devices load firmware during 
resets or initialization.

But I think that is something we can cross if the extra attributes for 
reload and flash are not sufficient. We can always add a parameter 
later. We can't easily take them away once added.