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

> -----Original Message-----
> From: Shannon Nelson <shnelson at amd.com>
> Sent: Monday, November 28, 2022 3:46 PM
> To: Jakub Kicinski <kuba at kernel.org>
> 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 11/28/22 3:33 PM, Jakub Kicinski wrote:
> > 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.
This could potentially be handled by having DELVINK_PARAM_FW_LOAD_POLICY_FLASH
be the automatic "select best version", and if a user has set a manual
value then don't allow flashing until a reboot or the value is set back to
POLICY_FLASH?
> 
> Yes, the action of flashing will set the new bank/slot to use on the
> next boot.  However, we have the ability to select from multiple valid
> images and we want to pass this flexibility to the user rather than
> force them to go through a whole flash sequence just to get to the other
> bank.
> 
> >
> > 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?
> >
> > 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?
> 
> We are listing the stored images in the devlink dev info output, just
> want to let the user choose which of those valid images to use next.
> 
> Cheers,
> sln
Technically I think we could do something similar in ice to switch between the
banks, at least as long as there is a valid image in the bank. The big trick is
that I am not sure we can verify ahead of time whether we have a valid image and
if you happen to boot into an invalid or blank image. There is some recovery
firmware that should activate in that case, but I think our current driver
doesn't implement enough of a recovery mode to actually handle this case to
allow user to switch back.

Still, I think the ability to select the bank is valuable, and finding the right
way to expose it is good.

Thanks,
Jake