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

From: Jakub Kicinski <[email-masked]>
Date: Thu, 8 Dec 2022 17:24:22 -0800

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

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