RE: [net-next PATCH 2/2] ice: support dry run of a flash update to validate firmware file

["Keller, Jacob E" <jacob.e.keller@intel.com>]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Wednesday, July 20, 2022 10:56 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Jakub Kicinski <kuba@kernel.org>
> Subject: Re: [net-next PATCH 2/2] ice: support dry run of a flash update to
> validate firmware file
> 
> Wed, Jul 20, 2022 at 08:34:33PM CEST, jacob.e.keller@intel.com wrote:
> >Now that devlink core flash update can handle dry run requests, update
> >the ice driver to allow validating a PLDM file in dry_run mode.
> >
> >First, add a new dry_run field to the pldmfw context structure. This
> >indicates that the PLDM firmware file library should only validate the
> >file and verify that it has a matching record. Update the pldmfw
> >documentation to indicate this "dry run" mode.
> >
> >In the ice driver, let the stack know that we support the dry run
> >attribute for flash update by setting the appropriate bit in the
> >.supported_flash_update_params field.
> >
> >If the dry run is requested, notify the PLDM firmware library by setting
> >the context bit appropriately. Don't cancel a pending update during
> >a dry run.
> >
> >Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> >---
> > Documentation/driver-api/pldmfw/index.rst      | 10 ++++++++++
> > drivers/net/ethernet/intel/ice/ice_devlink.c   |  3 ++-
> > drivers/net/ethernet/intel/ice/ice_fw_update.c | 14 ++++++++++----
> > include/linux/pldmfw.h                         |  5 +++++
> > lib/pldmfw/pldmfw.c                            | 12 ++++++++++++
> > 5 files changed, 39 insertions(+), 5 deletions(-)
> >
> >diff --git a/Documentation/driver-api/pldmfw/index.rst
> b/Documentation/driver-api/pldmfw/index.rst
> >index ad2c33ece30f..454b3ed6576a 100644
> >--- a/Documentation/driver-api/pldmfw/index.rst
> >+++ b/Documentation/driver-api/pldmfw/index.rst
> >@@ -51,6 +51,16 @@ unaligned access of multi-byte fields, and to properly
> convert from Little
> > Endian to CPU host format. Additionally the records, descriptors, and
> > components are stored in linked lists.
> >
> >+Validating a PLDM firmware file
> >+===============================
> >+
> >+To simply validate a PLDM firmware file, and verify whether it applies to
> >+the device, set the ``dry_run`` flag in the ``pldmfw`` context structure.
> >+If this flag is set, the library will parse the file, validating its UUID
> >+and checking if any record matches the device. Note that in a dry run, the
> >+library will *not* issue any ops besides ``match_record``. It will not
> >+attempt to send the component table or package data to the device firmware.
> >+
> > Performing a flash update
> > =========================
> >
> >diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c
> b/drivers/net/ethernet/intel/ice/ice_devlink.c
> >index 3337314a7b35..18214ea33e2d 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> >+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> >@@ -467,7 +467,8 @@ ice_devlink_reload_empr_finish(struct devlink *devlink,
> > }
> >
> > static const struct devlink_ops ice_devlink_ops = {
> >-	.supported_flash_update_params =
> DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
> >+	.supported_flash_update_params =
> DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK |
> >+
> DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN,
> > 	.reload_actions = BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE),
> > 	/* The ice driver currently does not support driver reinit */
> > 	.reload_down = ice_devlink_reload_empr_start,
> >diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c
> b/drivers/net/ethernet/intel/ice/ice_fw_update.c
> >index 3dc5662d62a6..63317ae88186 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
> >+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
> >@@ -1015,15 +1015,21 @@ int ice_devlink_flash_update(struct devlink
> *devlink,
> > 	else
> > 		priv.context.ops = &ice_fwu_ops_e810;
> > 	priv.context.dev = dev;
> >+	priv.context.dry_run = params->dry_run;
> > 	priv.extack = extack;
> > 	priv.pf = pf;
> > 	priv.activate_flags = preservation;
> >
> >-	devlink_flash_update_status_notify(devlink, "Preparing to flash", NULL,
> 0, 0);
> >+	if (params->dry_run)
> >+		devlink_flash_update_status_notify(devlink, "Validating flash
> binary", NULL, 0, 0);
> 
> You do validation of the binary instead of the actual flash. Why is it
> called "dry-run" then? Perhaps "validate" would be more suitable?
> 

I had it as dry-run to match the naming of the devlink, but validate might make more sense for what we are able to do with PLDM here.

I don't believe we have  a method to actually load it and have firmware perform any further validation without actually updating.