From: Jiri Pirko <jiri@resnulli.us>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: netdev@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>
Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
Date: Thu, 21 Jul 2022 07:54:46 +0200 [thread overview]
Message-ID: <YtjqJjIceW+fProb@nanopsycho> (raw)
In-Reply-To: <20220720183433.2070122-2-jacob.e.keller@intel.com>
Wed, Jul 20, 2022 at 08:34:32PM CEST, jacob.e.keller@intel.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@intel.com>
>---
> .../networking/devlink/devlink-flash.rst | 23 +++++++++++++++++++
> include/net/devlink.h | 2 ++
> include/uapi/linux/devlink.h | 8 +++++++
> net/core/devlink.c | 19 ++++++++++++++-
> 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 88c701b375a2..ff5b1e60ad6a 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -622,10 +622,12 @@ struct devlink_flash_update_params {
> const struct firmware *fw;
> const char *component;
> u32 overwrite_mask;
>+ bool dry_run;
> };
>
> #define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT BIT(0)
> #define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK BIT(1)
>+#define DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN BIT(2)
>
> struct devlink_region;
> struct devlink_info_req;
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index b3d40a5d72ff..e24a5a808a12 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -576,6 +576,14 @@ enum devlink_attr {
> DEVLINK_ATTR_LINECARD_TYPE, /* string */
> DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */
>
>+ /* Before adding this attribute to a command, user space should check
>+ * the policy dump and verify the kernel recognizes the attribute.
>+ * Otherwise older kernels which do not recognize the attribute may
>+ * silently accept the unknown attribute while not actually performing
>+ * a dry run.
Why this comment is needed? Isn't that something generic which applies
to all new attributes what userspace may pass and kernel may ignore?
>+ */
>+ DEVLINK_ATTR_DRY_RUN, /* flag */
>+
> /* add new attributes above here, update the policy in devlink.c */
>
> __DEVLINK_ATTR_MAX,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index a9776ea923ae..7d403151bee2 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4736,7 +4736,8 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
> static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> struct genl_info *info)
> {
>- struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name;
>+ struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name,
>+ *nla_dry_run;
> struct devlink_flash_update_params params = {};
> struct devlink *devlink = info->user_ptr[0];
> const char *file_name;
>@@ -4782,6 +4783,21 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> return ret;
> }
>
>+ /* Always check dry run last, in order to allow verification of other
>+ * parameter support even if the particular driver does not yet
>+ * support a full dry-run
>+ */
>+ nla_dry_run = info->attrs[DEVLINK_ATTR_DRY_RUN];
>+ if (nla_dry_run) {
>+ if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN)) {
>+ NL_SET_ERR_MSG_ATTR(info->extack, nla_dry_run,
>+ "flash update is supported, but dry run is not supported for this device");
>+ release_firmware(params.fw);
>+ return -EOPNOTSUPP;
>+ }
>+ params.dry_run = true;
>+ }
>+
> devlink_flash_update_begin_notify(devlink);
> ret = devlink->ops->flash_update(devlink, ¶ms, info->extack);
> devlink_flash_update_end_notify(devlink);
>@@ -8997,6 +9013,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
> [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
> [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
>+ [DEVLINK_ATTR_DRY_RUN] = { .type = NLA_FLAG },
> };
>
> static const struct genl_small_ops devlink_nl_ops[] = {
>--
>2.35.1.456.ga9c7032d4631
>
next prev parent reply other threads:[~2022-07-21 5:54 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-20 18:34 [net-next PATCH 0/2] devlink: implement dry run support for flash update Jacob Keller
2022-07-20 18:34 ` [net-next PATCH 1/2] devlink: add dry run attribute to " Jacob Keller
2022-07-21 5:54 ` Jiri Pirko [this message]
2022-07-21 20:32 ` Keller, Jacob E
2022-07-22 6:18 ` Jiri Pirko
2022-07-22 21:12 ` Keller, Jacob E
2022-07-23 15:42 ` Jiri Pirko
2022-07-25 19:15 ` Keller, Jacob E
2022-07-25 19:39 ` Jakub Kicinski
2022-07-25 20:27 ` Keller, Jacob E
2022-07-25 20:32 ` Jakub Kicinski
2022-07-25 20:46 ` Keller, Jacob E
2022-07-26 1:13 ` Jakub Kicinski
2022-07-26 17:23 ` Keller, Jacob E
2022-07-26 18:48 ` Jakub Kicinski
2022-07-26 18:49 ` Keller, Jacob E
2022-07-26 18:21 ` Keller, Jacob E
2022-08-05 16:32 ` Keller, Jacob E
2022-08-05 18:51 ` Jakub Kicinski
2022-08-05 19:50 ` Keller, Jacob E
2022-07-25 20:33 ` Keller, Jacob E
2022-07-21 16:47 ` Jakub Kicinski
2022-07-21 18:57 ` Keller, Jacob E
2022-07-21 16:48 ` Jakub Kicinski
2022-07-21 18:57 ` Keller, Jacob E
2022-07-20 18:34 ` [net-next PATCH 2/2] ice: support dry run of a flash update to validate firmware file Jacob Keller
2022-07-21 5:56 ` Jiri Pirko
2022-07-21 18:57 ` Keller, Jacob E
2022-07-21 7:53 ` Leon Romanovsky
2022-07-21 5:57 ` [net-next PATCH 0/2] devlink: implement dry run support for flash update Jiri Pirko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YtjqJjIceW+fProb@nanopsycho \
--to=jiri@resnulli.us \
--cc=jacob.e.keller@intel.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).