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

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 8 Dec 2022 10:44:50 -0800 Shannon Nelson wrote:
> >> I think this is a nice guideline, but I'm not sure all physical devices
> >> will work this way.  
> > 
> > Shouldn't it be entirely in SW control? (possibly "FW" category of SW)  
> 
> Sadly, not all HW/FW works the way driver writers would like, nor gives 
> us all the features options we want.  Especially that FW that was built 
> before we driver writers had an opinion about how this should work.
> 
> My comment here mainly is that we need to be able to manage the older FW 
> as well as the newer, and be able to make allowances for FW that doesn't 
> play along as well.

How do we steer new folks towards this design, tho? 

The only idea I have would break backward compat for you - we keep what
I described as default, and for devices which can't do that we require
sort of a manual opt out, for example user must request "don't set to
active" if the driver can't auto-change the active. And explicitly
select the bank if the driver can't provide the stable next-flash
semantics?

IDK what exact pieces of info you're working with and how much of the
semantics you can "fake" in the driver?

> >> How about a new info item
> >>       DEVLINK_ATTR_INFO_ACTIVE_BANK
> >> which would need a new api function something like
> >>       devlink_info_active_bank_put()  
> > 
> > Yes, definitely. But I think the next-to-write is also needed, because
> > we will need to use the next-to-write bank to populate the JSON for
> > stored FW to keep backward compat.
> > 
> > In CLI we can be more loose but the algo in the docs must work and not
> > risk overwriting all the banks if machine gets multiple update cycles
> > before getting drained.  
> 
> If we are going to have multiple "stored" (banks) sections, then we need 
> an api that allows for signifying which stored section are we adding a 
> fw version to, and to be able to add the "active" and "flash-target" and 
> whatever other attributes can get added onto the stored bank.
> 
> One option is to assume a bank context gets set by a call to something 
> like devlink_info_stored_bank_put(), and add a bitmask of attributes 
> (ACTIVE, FLASH_TARGET, CURRENT, ...) that can be added to in the future 
> as needed.
>      int devlink_info_stored_bank_put(struct devlink_info_req *req,
>                                       uint bank_id,
>                                       u32 option_mask)

Yup, that's an option. Dunno if the mask is easier to use than just
separate call per attribute, but I guess you'll be the one to test
this API so you'll find out :)

At the netlink level we'd have a separate nla for active, target,
current banks, so no masks there.. right?