Virtualization - Nov 2022 - [RFC PATCH net-next 06/19] pds_core: add FW update feature to devlink

> -----Original Message-----
> From: Jakub Kicinski <kuba at kernel.org>
> Sent: Monday, November 28, 2022 3:33 PM
> To: Shannon Nelson <shnelson at amd.com>
> Cc: Shannon Nelson <snelson at pensando.io>; netdev at
vger.kernel.org;
> davem at davemloft.net; mst at redhat.com; jasowang at redhat.com;
> virtualization at lists.linux-foundation.org; drivers at pensando.io;
Keller, Jacob E
> <jacob.e.keller at intel.com>
> Subject: Re: [RFC PATCH net-next 06/19] pds_core: add FW update feature to
> devlink
> 
> On Mon, 28 Nov 2022 14:25:46 -0800 Shannon Nelson wrote:
> > I don't think Intel selects which FW image to boot, but it looks
like
> > mlxsw and nfp use the PARAM_GENERIC_FW_LOAD_POLICY to select between
> > DRIVER, FLASH, or DISK.  Shall I add a couple of generic SLOT_x items
to
> > the enum devlink_param_fw_load_policy_value and use this API?  For
example:
> >
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_0,
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_1,
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_2,
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_3,
> 
> Not the worst idea, although I presume normal FW flashing should switch
> between slots to activate the new image by default? Which means the
> action of fw flashing would alter the policy set by the user. A little
> awkward from an API purist standpoint.
> 
> I'd just expose the active "bank" via netlink directly.
> 
> > I could then modify the devlink dev info printed to refer to
fw_slot_0,
> > fw.slot_1, and fw.slot_2 instead of our vendor specific names.
> 
> Jake, didn't you have a similar capability in ice?
> 
We have two banks of flash, the active bank, and an inactive bank used for
updates. We can determine the active bank from the Shadow RAM contents which are
generated as the EMP firmware boots up.
> Knowing my memory I may have acquiesced to something in another driver
> already. That said - I think it's cleaner if we just list the stored
> versions per bank, no?
I think it would make sense to store them per bank and make the bank number some
index instead of something separate as like this
DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_<X> where each <X> makes a
separate parameter.

Currently devlink info reports "stored" and "active", which
aligns with our current use of the active vs inactive flash bank. We could be
explicit and indicate which bank it is, though its a bit tricky since most of
the firmware interface deals with it in terms of "active" and
"inactive" rather than the absolute position of "bank 0 or bank
1".

Especially if another device has more than 2 banks I think its a good extension
to devlink info, and we could probably get away with something like a new info
attribute that specifies which bank index it is, and an attribute which
indicates whether its active or not.

Thanks,
Jake