Message-ID: <CO1PR11MB5089455E719480341AC226B7D6909@CO1PR11MB5089.namprd11.prod.outlook.com>
Date:   Fri, 22 Jul 2022 21:13:27 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jonathan Corbet <corbet@....net>, Jiri Pirko <jiri@...dia.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        David Ahern <dsahern@...nel.org>,
        "Stephen Hemminger" <stephen@...workplumber.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
Subject: RE: [net-next v2 1/2] devlink: add dry run attribute to flash update



> -----Original Message-----
> From: Jiri Pirko <jiri@...nulli.us>
> Sent: Thursday, July 21, 2022 11:26 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: netdev@...r.kernel.org; Jonathan Corbet <corbet@....net>; Jiri Pirko
> <jiri@...dia.com>; David S. Miller <davem@...emloft.net>; Eric Dumazet
> <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni
> <pabeni@...hat.com>; Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> David Ahern <dsahern@...nel.org>; Stephen Hemminger
> <stephen@...workplumber.org>; linux-doc@...r.kernel.org; intel-wired-
> lan@...ts.osuosl.org
> Subject: Re: [net-next v2 1/2] devlink: add dry run attribute to flash update
> 
> Thu, Jul 21, 2022 at 11:14:46PM CEST, jacob.e.keller@...el.com wrote:
> >Users use the devlink flash interface to request a device driver program or
> >update the device flash chip. In some cases, a user (or script) may want to
> >verify that a given flash update command is supported without actually
> >committing to immediately updating the device. For example, a system
> >administrator may want to validate that a particular flash binary image
> >will be accepted by the device, or simply validate a command before finally
> >committing to it.
> >
> >The current flash update interface lacks a method to support such a dry
> >run. Add a new DEVLINK_ATTR_DRY_RUN attribute which shall be used by a
> >devlink command to indicate that a request is a dry run which should not
> >perform device configuration. Instead, the command should report whether
> >the command or configuration request is valid.
> >
> >While we can validate the initial arguments of the devlink command, a
> >proper dry run must be processed by the device driver. This is required
> >because only the driver can perform validation of the flash binary file.
> >
> >Add a new dry_run parameter to the devlink_flash_update_params struct,
> >along with the associated bit to indicate if a driver supports verifying a
> >dry run.
> >
> >We always check the dry run attribute last in order to allow as much
> >verification of other parameters as possible. For example, even if a driver
> >does not support the dry_run option, we can still validate the other
> >optional parameters such as the overwrite_mask and per-component update
> >name.
> >
> >Document that userspace should take care when issuing a dry run to older
> >kernels, as the flash update command is not strictly verified. Thus,
> >unknown attributes will be ignored and this could cause a request for a dry
> >run to perform an actual update. We can't fix old kernels to verify unknown
> >attributes, but userspace can check the maximum attribute and reject the
> >dry run request if it is not supported by the kernel.
> >
> >Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> >---
> >
> >Changes since v1:
> >* Add kernel doc comments to devlink_flash_update_params
> >* Reduce indentation by using nla_get_flag
> >
> > .../networking/devlink/devlink-flash.rst      | 23 +++++++++++++++++++
> > include/net/devlink.h                         |  4 ++++
> > include/uapi/linux/devlink.h                  |  8 +++++++
> > net/core/devlink.c                            | 17 +++++++++++++-
> > 4 files changed, 51 insertions(+), 1 deletion(-)
> >
> >diff --git a/Documentation/networking/devlink/devlink-flash.rst
> b/Documentation/networking/devlink/devlink-flash.rst
> >index 603e732f00cc..1dc373229a54 100644
> >--- a/Documentation/networking/devlink/devlink-flash.rst
> >+++ b/Documentation/networking/devlink/devlink-flash.rst
> >@@ -44,6 +44,29 @@ preserved across the update. A device may not support
> every combination and
> > the driver for such a device must reject any combination which cannot be
> > faithfully implemented.
> >
> >+Dry run
> >+=======
> >+
> >+Users can request a "dry run" of a flash update by adding the
> >+``DEVLINK_ATTR_DRY_RUN`` attribute to the ``DEVLINK_CMD_FLASH_UPDATE``
> >+command. If the attribute is present, the kernel will only verify that the
> >+provided command is valid. During a dry run, an update is not performed.
> >+
> >+If supported by the driver, the flash image contents are also validated and
> >+the driver may indicate whether the file is a valid flash image for the
> >+device.
> >+
> >+.. code:: shell
> >+
> >+   $ devlink dev flash pci/0000:af:00.0 file image.bin dry-run
> >+   Validating flash binary
> >+
> >+Note that user space should take care when adding this attribute. Older
> >+kernels which do not recognize the attribute may accept the command with an
> >+unknown attribute. This could lead to a request for a dry run which performs
> >+an unexpected update. To avoid this, user space should check the policy dump
> >+and verify that the attribute is recognized before adding it to the command.
> >+
> > Firmware Loading
> > ================
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 780744b550b8..47b86ccb85b0 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -613,6 +613,8 @@ enum devlink_param_generic_id {
> >  * struct devlink_flash_update_params - Flash Update parameters
> >  * @fw: pointer to the firmware data to update from
> >  * @component: the flash component to update
> >+ * @overwrite_mask: what sections of flash can be overwritten
> 
> Well, strictly speaking, this is not related to this patch and should be
> done in a separate one. But hey, it's a comment, so I guess noone really
> cares.
> 

Ah yep. I'll split this out since I need to send a new version to split the iproute stuff into its own thread anyways.

Powered by blists - more mailing lists