[net-next PATCH 0/2] devlink: implement dry run support for flash update

[net-next PATCH 0/2] devlink: implement dry run support for flash update

[Jacob Keller]

This is a re-send of the dry run support I submitted nearly a year ago at
https://lore.kernel.org/netdev/CO1PR11MB50898047B9C0FAA520505AFDD6B59@CO1PR11MB5089.namprd11.prod.outlook.com/

I had delayed sending this because of conflicting work in the ice driver at
the time, but then forgot about it and never got around to resubmitting it.

This adds a DEVLINK_ATTR_DRY_RUN which is used to indicate a request to
validate a potentially destructive operation without performing the actions
yet. In theory it could be used for other devlink operations in the future.

For flash update, it allows the user to validate a flash image, including
ensuring the driver for the device is willing to program it, without
actually committing an update yet.

There is an accompanying series for iproute2 which allows adding the dry-run
attribute. It does as Jakub suggested and checks the maximum attribute
before allowing the dry run in order to avoid accidentally performing a real
update on older kernels.

Jacob Keller (2):
  devlink: add dry run attribute to flash update
  ice: support dry run of a flash update to validate firmware file

 Documentation/driver-api/pldmfw/index.rst     | 10 ++++++++
 .../networking/devlink/devlink-flash.rst      | 23 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_devlink.c  |  3 ++-
 .../net/ethernet/intel/ice/ice_fw_update.c    | 14 +++++++----
 include/linux/pldmfw.h                        |  5 ++++
 include/net/devlink.h                         |  2 ++
 include/uapi/linux/devlink.h                  |  8 +++++++
 lib/pldmfw/pldmfw.c                           | 12 ++++++++++
 net/core/devlink.c                            | 19 ++++++++++++++-
 9 files changed, 90 insertions(+), 6 deletions(-)


base-commit: 6e693a104207fbf5a22795c987e8964c0a1ffe2d
-- 
2.35.1.456.ga9c7032d4631

[net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jacob Keller]

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.
+	 */
+	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, &params, 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

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jiri Pirko]

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, &params, 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
>

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Wednesday, July 20, 2022 10:55 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 1/2] devlink: add dry run attribute to flash update

<...>

> > 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?
> 

Because other attributes may not have such a negative and unexpected side effect. In most cases the side effect will be "the thing you wanted doesn't happen", but in this case its "the thing you didn't want to happen does". I think that deserves some warning. A dry run is a request to *not* do something.

Thanks,
Jake

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jiri Pirko]

Thu, Jul 21, 2022 at 10:32:25PM CEST, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Wednesday, July 20, 2022 10:55 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 1/2] devlink: add dry run attribute to flash update
>
><...>
>
>> > 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?
>> 
>
>Because other attributes may not have such a negative and unexpected side effect. In most cases the side effect will be "the thing you wanted doesn't happen", but in this case its "the thing you didn't want to happen does". I think that deserves some warning. A dry run is a request to *not* do something.

Hmm. Another option, in order to be on the safe side, would be to have a
new cmd for this...


>
>Thanks,
>Jake

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Thursday, July 21, 2022 11:19 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 1/2] devlink: add dry run attribute to flash update
> 
> Thu, Jul 21, 2022 at 10:32:25PM CEST, jacob.e.keller@intel.com wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jiri Pirko <jiri@resnulli.us>
> >> Sent: Wednesday, July 20, 2022 10:55 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 1/2] devlink: add dry run attribute to flash
> update
> >
> ><...>
> >
> >> > 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?
> >>
> >
> >Because other attributes may not have such a negative and unexpected side
> effect. In most cases the side effect will be "the thing you wanted doesn't
> happen", but in this case its "the thing you didn't want to happen does". I think
> that deserves some warning. A dry run is a request to *not* do something.
> 
> Hmm. Another option, in order to be on the safe side, would be to have a
> new cmd for this...
> 

I think that the warning and implementation in the iproute2 devlink userspace is sufficient. The alternative would be to work towards converting devlink over to the explicit validation which rejects unknown parameters.. but that has its own backwards compatibility challenges as well.

I guess we could use the same code to implement the command so it wouldn't be too much of a burden to add, but that also means we'd have a pattern of using a new command for every future devlink operation that wants a "dry run". I was anticipating we might want this  kind of option for other commands such as port splitting and unsplitting.

If we were going to add new commands, I would rather we go to the extra trouble of updating all the commands to be strict validation.

Thanks,
Jake

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jiri Pirko]

Fri, Jul 22, 2022 at 11:12:27PM CEST, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Thursday, July 21, 2022 11:19 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 1/2] devlink: add dry run attribute to flash update
>> 
>> Thu, Jul 21, 2022 at 10:32:25PM CEST, jacob.e.keller@intel.com wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Jiri Pirko <jiri@resnulli.us>
>> >> Sent: Wednesday, July 20, 2022 10:55 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 1/2] devlink: add dry run attribute to flash
>> update
>> >
>> ><...>
>> >
>> >> > 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?
>> >>
>> >
>> >Because other attributes may not have such a negative and unexpected side
>> effect. In most cases the side effect will be "the thing you wanted doesn't
>> happen", but in this case its "the thing you didn't want to happen does". I think
>> that deserves some warning. A dry run is a request to *not* do something.
>> 
>> Hmm. Another option, in order to be on the safe side, would be to have a
>> new cmd for this...
>> 
>
>I think that the warning and implementation in the iproute2 devlink userspace is sufficient. The alternative would be to work towards converting devlink over to the explicit validation which rejects unknown parameters.. but that has its own backwards compatibility challenges as well.
>
>I guess we could use the same code to implement the command so it wouldn't be too much of a burden to add, but that also means we'd have a pattern of using a new command for every future devlink operation that wants a "dry run". I was anticipating we might want this  kind of option for other commands such as port splitting and unsplitting.
>
>If we were going to add new commands, I would rather we go to the extra trouble of updating all the commands to be strict validation.

I think it is good idea. We would prevent many surprises.

>
>Thanks,
>Jake

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Saturday, July 23, 2022 8:42 AM
> To: Keller, Jacob E <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
> 
> Fri, Jul 22, 2022 at 11:12:27PM CEST, jacob.e.keller@intel.com wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jiri Pirko <jiri@resnulli.us>
> >> Sent: Thursday, July 21, 2022 11:19 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 1/2] devlink: add dry run attribute to flash
> update
> >>
> >> Thu, Jul 21, 2022 at 10:32:25PM CEST, jacob.e.keller@intel.com wrote:
> >> >
> >> >
> >> >> -----Original Message-----
> >> >> From: Jiri Pirko <jiri@resnulli.us>
> >> >> Sent: Wednesday, July 20, 2022 10:55 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 1/2] devlink: add dry run attribute to flash
> >> update
> >> >
> >> ><...>
> >> >
> >> >> > 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?
> >> >>
> >> >
> >> >Because other attributes may not have such a negative and unexpected side
> >> effect. In most cases the side effect will be "the thing you wanted doesn't
> >> happen", but in this case its "the thing you didn't want to happen does". I
> think
> >> that deserves some warning. A dry run is a request to *not* do something.
> >>
> >> Hmm. Another option, in order to be on the safe side, would be to have a
> >> new cmd for this...
> >>
> >
> >I think that the warning and implementation in the iproute2 devlink userspace is
> sufficient. The alternative would be to work towards converting devlink over to
> the explicit validation which rejects unknown parameters.. but that has its own
> backwards compatibility challenges as well.
> >
> >I guess we could use the same code to implement the command so it wouldn't
> be too much of a burden to add, but that also means we'd have a pattern of
> using a new command for every future devlink operation that wants a "dry run".
> I was anticipating we might want this  kind of option for other commands such as
> port splitting and unsplitting.
> >
> >If we were going to add new commands, I would rather we go to the extra
> trouble of updating all the commands to be strict validation.
> 
> I think it is good idea. We would prevent many surprises.
> 

I'm not sure exactly what the process would be here. Maybe something like:

1. identify all of the commands which aren't yet strict
2. introduce new command IDs for these commands with something like _STRICT as a suffix? (or something shorter like _2?)
3. make all of those commands strict validation..

but now that I think about that, i am not sure it would work. We use the same attribute list for all devlink commands. This means that strict validation would only check that its passed existing/known attributes? But that doesn't necessarily mean the kernel will process that particular attribute for a given command does it?

Like, once we introduce DEVLINK_ATTR_DRY_RUN support for flash, if we then want to introduce it later to something like port splitting.. it would be a valid attribute to send from kernels which support flash but would still be ignored on kernels that don't yet support it for port splitting?

Wouldn't we want each individual command to have its own validation of what attributes are valid?

I do think its probably a good idea to migrate to strict mode, but I am not sure it solves the problem of dry run. Thoughts? Am I missing something obvious?

Would we instead have to convert from genl_small_ops to genl_ops and introduce a policy for each command? I think that sounds like the proper approach here....

Thanks,
Jake

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jakub Kicinski]

On Mon, 25 Jul 2022 19:15:10 +0000 Keller, Jacob E wrote:
> I'm not sure exactly what the process would be here. Maybe something
> like:
> 
> 1. identify all of the commands which aren't yet strict
> 2. introduce new command IDs for these commands with something like
> _STRICT as a suffix? (or something shorter like _2?) 3. make all of
> those commands strict validation..
> 
> but now that I think about that, i am not sure it would work. We use
> the same attribute list for all devlink commands. This means that
> strict validation would only check that its passed existing/known
> attributes? But that doesn't necessarily mean the kernel will process
> that particular attribute for a given command does it?
> 
> Like, once we introduce DEVLINK_ATTR_DRY_RUN support for flash, if we
> then want to introduce it later to something like port splitting.. it
> would be a valid attribute to send from kernels which support flash
> but would still be ignored on kernels that don't yet support it for
> port splitting?
> 
> Wouldn't we want each individual command to have its own validation
> of what attributes are valid?
> 
> I do think its probably a good idea to migrate to strict mode, but I
> am not sure it solves the problem of dry run. Thoughts? Am I missing
> something obvious?
> 
> Would we instead have to convert from genl_small_ops to genl_ops and
> introduce a policy for each command? I think that sounds like the
> proper approach here....

...or repost without the comment and move on. IDK if Jiri would like 
to see the general problem of attr rejection solved right now but IMHO
it's perfectly fine to just make the user space DTRT.

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, July 25, 2022 12:39 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> On Mon, 25 Jul 2022 19:15:10 +0000 Keller, Jacob E wrote:
> > I'm not sure exactly what the process would be here. Maybe something
> > like:
> >
> > 1. identify all of the commands which aren't yet strict
> > 2. introduce new command IDs for these commands with something like
> > _STRICT as a suffix? (or something shorter like _2?) 3. make all of
> > those commands strict validation..
> >
> > but now that I think about that, i am not sure it would work. We use
> > the same attribute list for all devlink commands. This means that
> > strict validation would only check that its passed existing/known
> > attributes? But that doesn't necessarily mean the kernel will process
> > that particular attribute for a given command does it?
> >
> > Like, once we introduce DEVLINK_ATTR_DRY_RUN support for flash, if we
> > then want to introduce it later to something like port splitting.. it
> > would be a valid attribute to send from kernels which support flash
> > but would still be ignored on kernels that don't yet support it for
> > port splitting?
> >
> > Wouldn't we want each individual command to have its own validation
> > of what attributes are valid?
> >
> > I do think its probably a good idea to migrate to strict mode, but I
> > am not sure it solves the problem of dry run. Thoughts? Am I missing
> > something obvious?
> >
> > Would we instead have to convert from genl_small_ops to genl_ops and
> > introduce a policy for each command? I think that sounds like the
> > proper approach here....
> 
> ...or repost without the comment and move on. IDK if Jiri would like
> to see the general problem of attr rejection solved right now but IMHO
> it's perfectly fine to just make the user space DTRT.

Its probably worth fixing policy, but would like to come up with a proper path that doesn't break compatibility and that will require discussion to figure out what approach works.

I'll remove the comment though since this problem affects all attributes.

Thanks,
Jake

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jakub Kicinski]

On Mon, 25 Jul 2022 20:27:02 +0000 Keller, Jacob E wrote:
> > ...or repost without the comment and move on. IDK if Jiri would like
> > to see the general problem of attr rejection solved right now but IMHO
> > it's perfectly fine to just make the user space DTRT.  
> 
> Its probably worth fixing policy, but would like to come up with a
> proper path that doesn't break compatibility and that will require
> discussion to figure out what approach works.

The problem does not exist for new commands, right? Because we don't
set GENL_DONT_VALIDATE_STRICT for new additions. For that reason I
don't think we are in the "sooner we fix the better" situation.

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, July 25, 2022 1:33 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> On Mon, 25 Jul 2022 20:27:02 +0000 Keller, Jacob E wrote:
> > > ...or repost without the comment and move on. IDK if Jiri would like
> > > to see the general problem of attr rejection solved right now but IMHO
> > > it's perfectly fine to just make the user space DTRT.
> >
> > Its probably worth fixing policy, but would like to come up with a
> > proper path that doesn't break compatibility and that will require
> > discussion to figure out what approach works.
> 
> The problem does not exist for new commands, right? Because we don't
> set GENL_DONT_VALIDATE_STRICT for new additions. For that reason I
> don't think we are in the "sooner we fix the better" situation.

There are two problems, and only one of them is solved by strict validation right now:

1) Does the kernel know this attribute?

This is the question of whether the kernel is new enough to have the attribute, i.e. does the DEVLINK_ATTR_DRY_RUN even exist in the kernel's uapi yet.

This is straight forward, and usually good enough for most attributes. This is what is solved by not setting GENL_DONT_VALIDATE_STRICT.

However, consider what happens once we add  DEVLINK_ATTR_DRY_RUN and support it in flash update, in version X. This leads us to the next problem.

2) does the *command* recognize and support DEVLINK_ATTR_DRY_RUN

Since the kernel in this example already supports DEVLINK_ATTR_DRY_RUN, it will be recognized and the current setup the policy for attributes is the same for every command. Thus the kernel will accept DEVLINK_ATTR_DRY_RUN for any command, strict or not.

But if the command itself doesn't honor DEVLINK_ATTR_DRY_RUN, it will once again be silently ignored.

We currently use the same policy and the same attribute list for every command, so we already silently ignore unexpected attributes, even in strict validation, at least as far as I can tell when analyzing the code. You could try to send an attribute for the wrong command. Obviously existing iproute2 user space doesn't' do this.. but nothing stops it.

For some attributes, its not a problem. I.e. all flash update attributes are only used for DEVLINK_CMD_FLASH_UPDATE, and passing them to another command is meaningless and will likely stay meaningless forever. Obviously I think we would prefer if the kernel rejected the input anyways, but its at least not that surprising and a smaller problem.

But for something generic like DRY_RUN, this is problematic because we might want to add support for dry run in the future for other commands. I didn't really analyze every existing command today to see which ones make sense. We could minimize this problem for now by checking DRY_RUN for every command that might want to support it in the future...

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jakub Kicinski]

On Mon, 25 Jul 2022 20:46:01 +0000 Keller, Jacob E wrote:
> There are two problems, and only one of them is solved by strict
> validation right now:
> 
> 1) Does the kernel know this attribute?
> 
> This is the question of whether the kernel is new enough to have the
> attribute, i.e. does the DEVLINK_ATTR_DRY_RUN even exist in the
> kernel's uapi yet.
> 
> This is straight forward, and usually good enough for most
> attributes. This is what is solved by not setting
> GENL_DONT_VALIDATE_STRICT.
> 
> However, consider what happens once we add  DEVLINK_ATTR_DRY_RUN and
> support it in flash update, in version X. This leads us to the next
> problem.
> 
> 2) does the *command* recognize and support DEVLINK_ATTR_DRY_RUN
> 
> Since the kernel in this example already supports
> DEVLINK_ATTR_DRY_RUN, it will be recognized and the current setup the
> policy for attributes is the same for every command. Thus the kernel
> will accept DEVLINK_ATTR_DRY_RUN for any command, strict or not.
> 
> But if the command itself doesn't honor DEVLINK_ATTR_DRY_RUN, it will
> once again be silently ignored.
> 
> We currently use the same policy and the same attribute list for
> every command, so we already silently ignore unexpected attributes,
> even in strict validation, at least as far as I can tell when
> analyzing the code. You could try to send an attribute for the wrong
> command. Obviously existing iproute2 user space doesn't' do this..
> but nothing stops it.
> 
> For some attributes, its not a problem. I.e. all flash update
> attributes are only used for DEVLINK_CMD_FLASH_UPDATE, and passing
> them to another command is meaningless and will likely stay
> meaningless forever. Obviously I think we would prefer if the kernel
> rejected the input anyways, but its at least not that surprising and
> a smaller problem.
> 
> But for something generic like DRY_RUN, this is problematic because
> we might want to add support for dry run in the future for other
> commands. I didn't really analyze every existing command today to see
> which ones make sense. We could minimize this problem for now by
> checking DRY_RUN for every command that might want to support it in
> the future...

Hm, yes. Don't invest too much effort into rendering per-cmd policies
right now, tho. I've started working on putting the parsing policies 
in YAML last Friday. This way we can auto-gen the policy for the kernel
and user space can auto-gen the parser/nl TLV writer. Long story short
we can kill two birds with one stone if you hold off until I have the
format ironed out. For now maybe just fork the policies into two - 
with and without dry run attr. We'll improve the granularity later 
when doing the YAML conversion.

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, July 25, 2022 6:14 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> On Mon, 25 Jul 2022 20:46:01 +0000 Keller, Jacob E wrote:
> > There are two problems, and only one of them is solved by strict
> > validation right now:
> >
> > 1) Does the kernel know this attribute?
> >
> > This is the question of whether the kernel is new enough to have the
> > attribute, i.e. does the DEVLINK_ATTR_DRY_RUN even exist in the
> > kernel's uapi yet.
> >
> > This is straight forward, and usually good enough for most
> > attributes. This is what is solved by not setting
> > GENL_DONT_VALIDATE_STRICT.
> >
> > However, consider what happens once we add  DEVLINK_ATTR_DRY_RUN and
> > support it in flash update, in version X. This leads us to the next
> > problem.
> >
> > 2) does the *command* recognize and support DEVLINK_ATTR_DRY_RUN
> >
> > Since the kernel in this example already supports
> > DEVLINK_ATTR_DRY_RUN, it will be recognized and the current setup the
> > policy for attributes is the same for every command. Thus the kernel
> > will accept DEVLINK_ATTR_DRY_RUN for any command, strict or not.
> >
> > But if the command itself doesn't honor DEVLINK_ATTR_DRY_RUN, it will
> > once again be silently ignored.
> >
> > We currently use the same policy and the same attribute list for
> > every command, so we already silently ignore unexpected attributes,
> > even in strict validation, at least as far as I can tell when
> > analyzing the code. You could try to send an attribute for the wrong
> > command. Obviously existing iproute2 user space doesn't' do this..
> > but nothing stops it.
> >
> > For some attributes, its not a problem. I.e. all flash update
> > attributes are only used for DEVLINK_CMD_FLASH_UPDATE, and passing
> > them to another command is meaningless and will likely stay
> > meaningless forever. Obviously I think we would prefer if the kernel
> > rejected the input anyways, but its at least not that surprising and
> > a smaller problem.
> >
> > But for something generic like DRY_RUN, this is problematic because
> > we might want to add support for dry run in the future for other
> > commands. I didn't really analyze every existing command today to see
> > which ones make sense. We could minimize this problem for now by
> > checking DRY_RUN for every command that might want to support it in
> > the future...
> 
> Hm, yes. Don't invest too much effort into rendering per-cmd policies
> right now, tho. I've started working on putting the parsing policies
> in YAML last Friday. This way we can auto-gen the policy for the kernel
> and user space can auto-gen the parser/nl TLV writer. Long story short
> we can kill two birds with one stone if you hold off until I have the
> format ironed out.

Makes sense, this would definitely simplify writing policy and avoid some duplication that would occur otherwise.

> For now maybe just fork the policies into two -
> with and without dry run attr. We'll improve the granularity later
> when doing the YAML conversion.

Not quite sure I follow this. I guess just add a separate policy array with dry_run and then make that the policy for the flash update command? I don't think flash update is strict yet, and I'm not sure what the impact of changing it to strict is in terms of backwards compatibility with the interface.

Thanks,
Jake

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jakub Kicinski]

On Tue, 26 Jul 2022 17:23:53 +0000 Keller, Jacob E wrote:
> > For now maybe just fork the policies into two -
> > with and without dry run attr. We'll improve the granularity later
> > when doing the YAML conversion.  
> 
> Not quite sure I follow this. I guess just add a separate policy
> array with dry_run and then make that the policy for the flash update
> command? I don't think flash update is strict yet, and I'm not sure
> what the impact of changing it to strict is in terms of backwards
> compatibility with the interface.

Strictness is a separate issue, the policy split addresses just 
the support discoverability question.

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, July 26, 2022 11:49 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> On Tue, 26 Jul 2022 17:23:53 +0000 Keller, Jacob E wrote:
> > > For now maybe just fork the policies into two -
> > > with and without dry run attr. We'll improve the granularity later
> > > when doing the YAML conversion.
> >
> > Not quite sure I follow this. I guess just add a separate policy
> > array with dry_run and then make that the policy for the flash update
> > command? I don't think flash update is strict yet, and I'm not sure
> > what the impact of changing it to strict is in terms of backwards
> > compatibility with the interface.
> 
> Strictness is a separate issue, the policy split addresses just
> the support discoverability question.

Right, ok.

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, July 25, 2022 6:14 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> Hm, yes. Don't invest too much effort into rendering per-cmd policies
> right now, tho. I've started working on putting the parsing policies
> in YAML last Friday. This way we can auto-gen the policy for the kernel
> and user space can auto-gen the parser/nl TLV writer. Long story short
> we can kill two birds with one stone if you hold off until I have the
> format ironed out. For now maybe just fork the policies into two -
> with and without dry run attr. We'll improve the granularity later
> when doing the YAML conversion.

I'm also worried about the process for transitioning from the existing non-strict policy to a strict validation of per-command policies. How does that impact backwards compatibilty, and will we need to introduce new ops or not?

I know we had to introduce new ops for the strict versions of the PTP ioctls. However those were dealing with (possibly) uninitialized values, where-in userspace may have been accidentally sending values so we could no longer extend the reserved fields.

For netlink, in this case the userspace code would need to be intentionally adding netlink attributes. I would think that well behaved userspace would rather get an error when the command isn't honoring an attribute...

But in a technical sense that would still be breaking backwards compatibility, since it would cause an application that used to "work" to break. Ofcourse in the case of something like DEVLINK_ATTR_DRY_RUN, the userspace may not be working as intended, and it might be considered a bug.

In short: for backwards compatibility, it seems like we might not be able to migrate existing ops to strict validation and would need to replace them? That seems like a very big step...

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

Hi,

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, July 25, 2022 6:14 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
>
> Hm, yes. Don't invest too much effort into rendering per-cmd policies
> right now, tho. I've started working on putting the parsing policies
> in YAML last Friday. This way we can auto-gen the policy for the kernel
> and user space can auto-gen the parser/nl TLV writer. Long story short
> we can kill two birds with one stone if you hold off until I have the
> format ironed out. For now maybe just fork the policies into two -
> with and without dry run attr. We'll improve the granularity later
> when doing the YAML conversion.

Any update on this?

FWIW I started looking at iproute2 code to dump policy and check whether a specific attribute is accepted by the kernel.

Thanks,
Jake 

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jakub Kicinski]

On Fri, 5 Aug 2022 16:32:30 +0000 Keller, Jacob E wrote:
> > Hm, yes. Don't invest too much effort into rendering per-cmd policies
> > right now, tho. I've started working on putting the parsing policies
> > in YAML last Friday. This way we can auto-gen the policy for the kernel
> > and user space can auto-gen the parser/nl TLV writer. Long story short
> > we can kill two birds with one stone if you hold off until I have the
> > format ironed out. For now maybe just fork the policies into two -
> > with and without dry run attr. We'll improve the granularity later
> > when doing the YAML conversion.  
> 
> Any update on this?
> 
> FWIW I started looking at iproute2 code to dump policy and check
> whether a specific attribute is accepted by the kernel.

Yes and no, I coded a little bit of it up, coincidentally I have a YAML
policy for genetlink policy querying if that's helpful:

https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/tree/tools/net/ynl/samples/nlctrl.c?h=gnl-gen-dpll

I'll try to wrap up the YAML format by today / tomorrow and send an
early RFC, but the codegen part (and everything else really) still
requires much work. Probably another month until I can post the first
non-RFC with error checking, kernel policy generation, uAPI generation
etc.

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, August 05, 2022 11:51 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> On Fri, 5 Aug 2022 16:32:30 +0000 Keller, Jacob E wrote:
> > > Hm, yes. Don't invest too much effort into rendering per-cmd policies
> > > right now, tho. I've started working on putting the parsing policies
> > > in YAML last Friday. This way we can auto-gen the policy for the kernel
> > > and user space can auto-gen the parser/nl TLV writer. Long story short
> > > we can kill two birds with one stone if you hold off until I have the
> > > format ironed out. For now maybe just fork the policies into two -
> > > with and without dry run attr. We'll improve the granularity later
> > > when doing the YAML conversion.
> >
> > Any update on this?
> >
> > FWIW I started looking at iproute2 code to dump policy and check
> > whether a specific attribute is accepted by the kernel.
> 
> Yes and no, I coded a little bit of it up, coincidentally I have a YAML
> policy for genetlink policy querying if that's helpful:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/tree/tools/net/ynl
> /samples/nlctrl.c?h=gnl-gen-dpll
> 

I'll take a look at this.

> I'll try to wrap up the YAML format by today / tomorrow and send an
> early RFC, but the codegen part (and everything else really) still
> requires much work.

I'd like to see it and provide some early review.

> Probably another month until I can post the first
> non-RFC with error checking, kernel policy generation, uAPI generation
> etc.

Ya, I figured this would take quite a lot of effort to get to completion.

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com>
> Sent: Monday, July 25, 2022 1:27 PM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> Subject: RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> 
> 
> > -----Original Message-----
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Monday, July 25, 2022 12:39 PM
> > To: Keller, Jacob E <jacob.e.keller@intel.com>
> > Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
> > Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> >
> > On Mon, 25 Jul 2022 19:15:10 +0000 Keller, Jacob E wrote:
> > > I'm not sure exactly what the process would be here. Maybe something
> > > like:
> > >
> > > 1. identify all of the commands which aren't yet strict
> > > 2. introduce new command IDs for these commands with something like
> > > _STRICT as a suffix? (or something shorter like _2?) 3. make all of
> > > those commands strict validation..
> > >
> > > but now that I think about that, i am not sure it would work. We use
> > > the same attribute list for all devlink commands. This means that
> > > strict validation would only check that its passed existing/known
> > > attributes? But that doesn't necessarily mean the kernel will process
> > > that particular attribute for a given command does it?
> > >
> > > Like, once we introduce DEVLINK_ATTR_DRY_RUN support for flash, if we
> > > then want to introduce it later to something like port splitting.. it
> > > would be a valid attribute to send from kernels which support flash
> > > but would still be ignored on kernels that don't yet support it for
> > > port splitting?
> > >
> > > Wouldn't we want each individual command to have its own validation
> > > of what attributes are valid?
> > >
> > > I do think its probably a good idea to migrate to strict mode, but I
> > > am not sure it solves the problem of dry run. Thoughts? Am I missing
> > > something obvious?
> > >
> > > Would we instead have to convert from genl_small_ops to genl_ops and
> > > introduce a policy for each command? I think that sounds like the
> > > proper approach here....
> >
> > ...or repost without the comment and move on. IDK if Jiri would like
> > to see the general problem of attr rejection solved right now but IMHO
> > it's perfectly fine to just make the user space DTRT.
> 
> Its probably worth fixing policy, but would like to come up with a proper path
> that doesn't break compatibility and that will require discussion to figure out
> what approach works.
> 
> I'll remove the comment though since this problem affects all attributes.
> 
> Thanks,
> Jake

Hmm. The more I think about it, the more it seems that per-command policy is the only way to make the addition of dry_run to new commands safe. Without it, we'd run the risk of a future kernel supporting dry_run but a command not supporting it yet.

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jakub Kicinski]

On Wed, 20 Jul 2022 11:34:32 -0700 Jacob Keller wrote:
> +	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;
> +	}

Looks over-indented. You can do this, right?

	params.dry_run = nla_get_flag(info->attrs[DEVLINK_ATTR_DRY_RUN]);
	if (params.dry_run &&
	    !(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN)) {
		/* error handling */
	}

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, July 21, 2022 9:48 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> On Wed, 20 Jul 2022 11:34:32 -0700 Jacob Keller wrote:
> > +	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;
> > +	}
> 
> Looks over-indented. You can do this, right?
> 
> 	params.dry_run = nla_get_flag(info->attrs[DEVLINK_ATTR_DRY_RUN]);
> 	if (params.dry_run &&
> 	    !(supported_params &
> DEVLINK_SUPPORT_FLASH_UPDATE_DRY_RUN)) {
> 		/* error handling */
> 	}

Yea that makes more sense.

Thanks,
Jake

Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Jakub Kicinski]

On Wed, 20 Jul 2022 11:34:32 -0700 Jacob Keller 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.

Also

include/net/devlink.h:627: warning: Function parameter or member 'dry_run' not described in 'devlink_flash_update_params'

RE: [net-next PATCH 1/2] devlink: add dry run attribute to flash update

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, July 21, 2022 9:48 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org
> Subject: Re: [net-next PATCH 1/2] devlink: add dry run attribute to flash update
> 
> On Wed, 20 Jul 2022 11:34:32 -0700 Jacob Keller 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.
> 
> Also
> 
> include/net/devlink.h:627: warning: Function parameter or member 'dry_run'
> not described in 'devlink_flash_update_params'

Will fix.

Thanks,
Jake

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

[Jacob Keller]

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);
+	else
+		devlink_flash_update_status_notify(devlink, "Preparing to flash", NULL, 0, 0);
 
-	err = ice_cancel_pending_update(pf, NULL, extack);
-	if (err)
-		return err;
+	if (!params->dry_run) {
+		err = ice_cancel_pending_update(pf, NULL, extack);
+		if (err)
+			return err;
+	}
 
 	err = ice_acquire_nvm(hw, ICE_RES_WRITE);
 	if (err) {
diff --git a/include/linux/pldmfw.h b/include/linux/pldmfw.h
index 0fc831338226..d9add301582b 100644
--- a/include/linux/pldmfw.h
+++ b/include/linux/pldmfw.h
@@ -124,10 +124,15 @@ struct pldmfw_ops;
  * should embed this in a private structure and use container_of to obtain
  * a pointer to their own data, used to implement the device specific
  * operations.
+ *
+ * @ops: function pointers used as callbacks from the PLDMFW library
+ * @dev: pointer to the device being updated
+ * @dry_run: if true, only validate the file, do not perform an update.
  */
 struct pldmfw {
 	const struct pldmfw_ops *ops;
 	struct device *dev;
+	bool dry_run;
 };
 
 bool pldmfw_op_pci_match_record(struct pldmfw *context, struct pldmfw_record *record);
diff --git a/lib/pldmfw/pldmfw.c b/lib/pldmfw/pldmfw.c
index 6e77eb6d8e72..29a132a39876 100644
--- a/lib/pldmfw/pldmfw.c
+++ b/lib/pldmfw/pldmfw.c
@@ -827,6 +827,10 @@ static int pldm_finalize_update(struct pldmfw_priv *data)
  * to the device firmware. Extract and write the flash data for each of the
  * components indicated in the firmware file.
  *
+ * If the context->dry_run is set, this is a request for a dry run, i.e. to
+ * only validate the PLDM firmware file. In this case, stop and exit after we
+ * find a valid matching record.
+ *
  * Returns: zero on success, or a negative error code on failure.
  */
 int pldmfw_flash_image(struct pldmfw *context, const struct firmware *fw)
@@ -844,14 +848,22 @@ int pldmfw_flash_image(struct pldmfw *context, const struct firmware *fw)
 	data->fw = fw;
 	data->context = context;
 
+	/* Parse the image and make sure it is a valid PLDM firmware binary */
 	err = pldm_parse_image(data);
 	if (err)
 		goto out_release_data;
 
+	/* Search for a record matching the device */
 	err = pldm_find_matching_record(data);
 	if (err)
 		goto out_release_data;
 
+	/* If this is a dry run, do not perform an update */
+	if (context->dry_run)
+		goto out_release_data;
+
+	/* Perform the device update */
+
 	err = pldm_send_package_data(data);
 	if (err)
 		goto out_release_data;
-- 
2.35.1.456.ga9c7032d4631

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

[Jiri Pirko]

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?



>+	else
>+		devlink_flash_update_status_notify(devlink, "Preparing to flash", NULL, 0, 0);
> 
>-	err = ice_cancel_pending_update(pf, NULL, extack);
>-	if (err)
>-		return err;
>+	if (!params->dry_run) {
>+		err = ice_cancel_pending_update(pf, NULL, extack);
>+		if (err)
>+			return err;
>+	}
> 
> 	err = ice_acquire_nvm(hw, ICE_RES_WRITE);
> 	if (err) {
>diff --git a/include/linux/pldmfw.h b/include/linux/pldmfw.h
>index 0fc831338226..d9add301582b 100644
>--- a/include/linux/pldmfw.h
>+++ b/include/linux/pldmfw.h
>@@ -124,10 +124,15 @@ struct pldmfw_ops;
>  * should embed this in a private structure and use container_of to obtain
>  * a pointer to their own data, used to implement the device specific
>  * operations.
>+ *
>+ * @ops: function pointers used as callbacks from the PLDMFW library
>+ * @dev: pointer to the device being updated
>+ * @dry_run: if true, only validate the file, do not perform an update.
>  */
> struct pldmfw {
> 	const struct pldmfw_ops *ops;
> 	struct device *dev;
>+	bool dry_run;
> };
> 
> bool pldmfw_op_pci_match_record(struct pldmfw *context, struct pldmfw_record *record);
>diff --git a/lib/pldmfw/pldmfw.c b/lib/pldmfw/pldmfw.c
>index 6e77eb6d8e72..29a132a39876 100644
>--- a/lib/pldmfw/pldmfw.c
>+++ b/lib/pldmfw/pldmfw.c
>@@ -827,6 +827,10 @@ static int pldm_finalize_update(struct pldmfw_priv *data)
>  * to the device firmware. Extract and write the flash data for each of the
>  * components indicated in the firmware file.
>  *
>+ * If the context->dry_run is set, this is a request for a dry run, i.e. to
>+ * only validate the PLDM firmware file. In this case, stop and exit after we
>+ * find a valid matching record.
>+ *
>  * Returns: zero on success, or a negative error code on failure.
>  */
> int pldmfw_flash_image(struct pldmfw *context, const struct firmware *fw)
>@@ -844,14 +848,22 @@ int pldmfw_flash_image(struct pldmfw *context, const struct firmware *fw)
> 	data->fw = fw;
> 	data->context = context;
> 
>+	/* Parse the image and make sure it is a valid PLDM firmware binary */
> 	err = pldm_parse_image(data);
> 	if (err)
> 		goto out_release_data;
> 
>+	/* Search for a record matching the device */
> 	err = pldm_find_matching_record(data);
> 	if (err)
> 		goto out_release_data;
> 
>+	/* If this is a dry run, do not perform an update */
>+	if (context->dry_run)
>+		goto out_release_data;
>+
>+	/* Perform the device update */
>+
> 	err = pldm_send_package_data(data);
> 	if (err)
> 		goto out_release_data;
>-- 
>2.35.1.456.ga9c7032d4631
>

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

[Keller, Jacob E]

> -----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.

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

[Leon Romanovsky]

On Wed, Jul 20, 2022 at 11:34:33AM -0700, Jacob Keller 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(-)

<...>

>  struct pldmfw {
>  	const struct pldmfw_ops *ops;
>  	struct device *dev;
> +	bool dry_run;
>  };

Just a nitpick, it is better to write "u8 dry_run : 1;" and not bool.

Thanks

Re: [net-next PATCH 0/2] devlink: implement dry run support for flash update

[Jiri Pirko]

Did you use get_maintainers script to get cc list?