[net-next v2 0/5] devlink flash update overwrite mask

[net-next v2 0/5] devlink flash update overwrite mask

[Jacob Keller]

This series introduces support for a new attribute to the flash update
command: DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK. This attribute is a u32
value that represents a bitmask of which subsections of flash to
request/allow overwriting when performing a flash update.

The intent is to support the ability to control overwriting options of the
ice hardware flash update. Specifically, the ice flash components combine
settings and identifiers within the firmware flash section. This series
introduces the two subsections, "identifiers" and "settings". With the new
attribute, users can request to overwrite these subsections when performing
a flash update. By existing convention, it is assumed that flash program
binaries are always updated (and thus overwritten), and no mask bit is
provided to control this.

First, the .flash_update command is modified to take a parameter structure.
A new supported_flash_update_params field is also provided to allow drivers
to opt-in to the parameters they support rather than opting out. This is
similar to the recently added supported_coalesc_params field in ethtool.

Following this, the new overwrite mask parameter is added, along with the
associated supported bit. The netdevsim driver is updated to support this
parameter, along with a few self tests to help verify the interface is
working as expected.

Finally, the ice driver is modified to support the parameter, converting it
into the firmware preservation level request.

Patches to enable support for specifying the overwrite sections are also
provided for iproute2-next. This is done primarily in order to enable the
tests for netdevsim. As discussed previously on the list, the primary
motivations for the overwrite mode are two-fold.

First, supporting update with a customized image that has pre-configured
settings and identifiers, used with overwrite of both settings and
identifiers. This enables an initial update to overwrite default values and
customize the adapter with a new serial ID and fresh settings. Second, it
may sometimes be useful to allow overwriting of settings when updating in
order to guarantee that the settings in the flash section are "known good".

Changes since v1
* Added supported_flash_update_params field, removing some boilerplate in
  each driver. This also makes it easier to add new parameters in the future
  without fear of accidentally breaking an existing driver, due to opt-in
  behavior instead of forcing drivers to opt-out.
* Split the ice changes to a separate patch.

Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: Bin Luo <luobin9@huawei.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Ido Schimmel <idosch@mellanox.com>
Cc: Danielle Ratson <danieller@mellanox.com>

Jacob Keller (3):
  devlink: convert flash_update to use params structure
  devlink: introduce flash update overwrite mask
  ice: add support for flash update overwrite mask

 .../networking/devlink/devlink-flash.rst      | 29 ++++++++++++++
 Documentation/networking/devlink/ice.rst      | 31 +++++++++++++++
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 19 ++++-----
 .../net/ethernet/huawei/hinic/hinic_devlink.c |  8 +---
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 33 +++++++++++-----
 .../net/ethernet/intel/ice/ice_fw_update.c    | 16 +++++++-
 .../net/ethernet/intel/ice/ice_fw_update.h    |  2 +-
 .../net/ethernet/mellanox/mlx5/core/devlink.c |  8 +---
 drivers/net/ethernet/mellanox/mlxsw/core.c    |  6 +--
 drivers/net/ethernet/mellanox/mlxsw/core.h    |  2 +-
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  7 +---
 .../net/ethernet/netronome/nfp/nfp_devlink.c  |  9 ++---
 drivers/net/netdevsim/dev.c                   | 21 +++++++---
 drivers/net/netdevsim/netdevsim.h             |  1 +
 include/net/devlink.h                         | 35 ++++++++++++++++-
 include/uapi/linux/devlink.h                  | 24 ++++++++++++
 net/core/devlink.c                            | 39 +++++++++++++++----
 .../drivers/net/netdevsim/devlink.sh          | 21 ++++++++++
 18 files changed, 244 insertions(+), 67 deletions(-)

Jacob Keller (2):
  Update devlink header for overwrite mask attribute
  devlink: support setting the overwrite mask

 devlink/devlink.c            | 37 ++++++++++++++++++++++++++++++++++--
 include/uapi/linux/devlink.h | 24 +++++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)

base-commit: bd69058f50d5ffa659423bcfa6fe6280ce9c760a
-- 
2.28.0.163.g6104cc2f0b60

[net-next v2 1/5] devlink: convert flash_update to use params structure

[Jacob Keller]

A future change is going to introduce a new parameter for specifying how
devices should handle overwrite behavior when updating the flash. This
will introduce a new argument specifying a bitmask of component
subsections to allow overwriting behavior.

Prepare for this by converting flash_update to use a new
devlink_flash_update_params structure. For now this just holds the
file_name and component, but this enables more easily extending the
function with new parameters in the future.

To avoid the need for modifying every driver when a new parameter is
introduced, the supported_flash_update_params field is added to
devlink_ops. Drivers must opt-in to supported parameters by setting the
appropriate bits in this field. This allows dropping the check in each
driver that doesn't support component updates.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: Bin Luo <luobin9@huawei.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Ido Schimmel <idosch@mellanox.com>
Cc: Danielle Ratson <danieller@mellanox.com>
---

Changes since v1
* Add supported_flash_update_params field, to allow drivers to opt-in to the
  set of supported parameters. This is similar to supported_coalesc_params
  and was suggested by Jakub. This also makes adding future parameters
  easier by reducing the need to modify existing drivers! Due to this, the
  boilerplate check for component is simply removed.

* netdevsim is modified to support the component parameter, and a new simple
  selftest is added to verify that this works.

 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 19 ++++-------
 .../net/ethernet/huawei/hinic/hinic_devlink.c |  8 ++---
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 18 ++++------
 .../net/ethernet/mellanox/mlx5/core/devlink.c |  8 ++---
 drivers/net/ethernet/mellanox/mlxsw/core.c    |  6 ++--
 drivers/net/ethernet/mellanox/mlxsw/core.h    |  2 +-
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  7 ++--
 .../net/ethernet/netronome/nfp/nfp_devlink.c  |  9 +++--
 drivers/net/netdevsim/dev.c                   | 13 ++++----
 include/net/devlink.h                         | 33 +++++++++++++++++--
 net/core/devlink.c                            | 25 ++++++++++----
 .../drivers/net/netdevsim/devlink.sh          |  3 ++
 12 files changed, 87 insertions(+), 64 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 3a854195d5b0..d436134bdc40 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -17,15 +17,13 @@
 #include "bnxt_ethtool.h"
 
 static int
-bnxt_dl_flash_update(struct devlink *dl, const char *filename,
-		     const char *region, struct netlink_ext_ack *extack)
+bnxt_dl_flash_update(struct devlink *dl,
+		     struct devlink_flash_update_params *params,
+		     struct netlink_ext_ack *extack)
 {
 	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
 	int rc;
 
-	if (region)
-		return -EOPNOTSUPP;
-
 	if (!BNXT_PF(bp)) {
 		NL_SET_ERR_MSG_MOD(extack,
 				   "flash update not supported from a VF");
@@ -33,15 +31,12 @@ bnxt_dl_flash_update(struct devlink *dl, const char *filename,
 	}
 
 	devlink_flash_update_begin_notify(dl);
-	devlink_flash_update_status_notify(dl, "Preparing to flash", region, 0,
-					   0);
-	rc = bnxt_flash_package_from_file(bp->dev, filename, 0);
+	devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0, 0);
+	rc = bnxt_flash_package_from_file(bp->dev, params->file_name, 0);
 	if (!rc)
-		devlink_flash_update_status_notify(dl, "Flashing done", region,
-						   0, 0);
+		devlink_flash_update_status_notify(dl, "Flashing done", NULL, 0, 0);
 	else
-		devlink_flash_update_status_notify(dl, "Flashing failed",
-						   region, 0, 0);
+		devlink_flash_update_status_notify(dl, "Flashing failed", NULL, 0, 0);
 	devlink_flash_update_end_notify(dl);
 	return rc;
 }
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
index c6adc776f3c8..1f45766107e4 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
@@ -281,18 +281,14 @@ static int hinic_firmware_update(struct hinic_devlink_priv *priv,
 }
 
 static int hinic_devlink_flash_update(struct devlink *devlink,
-				      const char *file_name,
-				      const char *component,
+				      struct devlink_flash_update_params *params,
 				      struct netlink_ext_ack *extack)
 {
 	struct hinic_devlink_priv *priv = devlink_priv(devlink);
 	const struct firmware *fw;
 	int err;
 
-	if (component)
-		return -EOPNOTSUPP;
-
-	err = request_firmware_direct(&fw, file_name,
+	err = request_firmware_direct(&fw, params->file_name,
 				      &priv->hwdev->hwif->pdev->dev);
 	if (err)
 		return err;
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index dbbd8b6f9d1a..c8255235f7c4 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -233,8 +233,7 @@ static int ice_devlink_info_get(struct devlink *devlink,
 /**
  * ice_devlink_flash_update - Update firmware stored in flash on the device
  * @devlink: pointer to devlink associated with device to update
- * @path: the path of the firmware file to use via request_firmware
- * @component: name of the component to update, or NULL
+ * @params: flash update parameters
  * @extack: netlink extended ACK structure
  *
  * Perform a device flash update. The bulk of the update logic is contained
@@ -243,8 +242,9 @@ static int ice_devlink_info_get(struct devlink *devlink,
  * Returns: zero on success, or an error code on failure.
  */
 static int
-ice_devlink_flash_update(struct devlink *devlink, const char *path,
-			 const char *component, struct netlink_ext_ack *extack)
+ice_devlink_flash_update(struct devlink *devlink,
+			 struct devlink_flash_update_params *params,
+			 struct netlink_ext_ack *extack)
 {
 	struct ice_pf *pf = devlink_priv(devlink);
 	struct device *dev = &pf->pdev->dev;
@@ -252,20 +252,16 @@ ice_devlink_flash_update(struct devlink *devlink, const char *path,
 	const struct firmware *fw;
 	int err;
 
-	/* individual component update is not yet supported */
-	if (component)
-		return -EOPNOTSUPP;
-
 	if (!hw->dev_caps.common_cap.nvm_unified_update) {
 		NL_SET_ERR_MSG_MOD(extack, "Current firmware does not support unified update");
 		return -EOPNOTSUPP;
 	}
 
-	err = ice_check_for_pending_update(pf, component, extack);
+	err = ice_check_for_pending_update(pf, NULL, extack);
 	if (err)
 		return err;
 
-	err = request_firmware(&fw, path, dev);
+	err = request_firmware(&fw, params->file_name, dev);
 	if (err) {
 		NL_SET_ERR_MSG_MOD(extack, "Unable to read file from disk");
 		return err;
@@ -273,7 +269,7 @@ ice_devlink_flash_update(struct devlink *devlink, const char *path,
 
 	devlink_flash_update_begin_notify(devlink);
 	devlink_flash_update_status_notify(devlink, "Preparing to flash",
-					   component, 0, 0);
+					   NULL, 0, 0);
 	err = ice_flash_pldm_image(pf, fw, extack);
 	devlink_flash_update_end_notify(devlink);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
index c709e9a385f6..9b14e3f805a2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
@@ -8,18 +8,14 @@
 #include "eswitch.h"
 
 static int mlx5_devlink_flash_update(struct devlink *devlink,
-				     const char *file_name,
-				     const char *component,
+				     struct devlink_flash_update_params *params,
 				     struct netlink_ext_ack *extack)
 {
 	struct mlx5_core_dev *dev = devlink_priv(devlink);
 	const struct firmware *fw;
 	int err;
 
-	if (component)
-		return -EOPNOTSUPP;
-
-	err = request_firmware_direct(&fw, file_name, &dev->pdev->dev);
+	err = request_firmware_direct(&fw, params->file_name, &dev->pdev->dev);
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index b01f8f2fab63..6db938708a0d 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -1138,8 +1138,7 @@ mlxsw_devlink_core_bus_device_reload_up(struct devlink *devlink,
 }
 
 static int mlxsw_devlink_flash_update(struct devlink *devlink,
-				      const char *file_name,
-				      const char *component,
+				      struct devlink_flash_update_params *params,
 				      struct netlink_ext_ack *extack)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
@@ -1147,8 +1146,7 @@ static int mlxsw_devlink_flash_update(struct devlink *devlink,
 
 	if (!mlxsw_driver->flash_update)
 		return -EOPNOTSUPP;
-	return mlxsw_driver->flash_update(mlxsw_core, file_name,
-					  component, extack);
+	return mlxsw_driver->flash_update(mlxsw_core, params, extack);
 }
 
 static int mlxsw_devlink_trap_init(struct devlink *devlink,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index c1c1e039323a..b6e3faf38305 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -318,7 +318,7 @@ struct mlxsw_driver {
 				       enum devlink_sb_pool_type pool_type,
 				       u32 *p_cur, u32 *p_max);
 	int (*flash_update)(struct mlxsw_core *mlxsw_core,
-			    const char *file_name, const char *component,
+			    struct devlink_flash_update_params *params,
 			    struct netlink_ext_ack *extack);
 	int (*trap_init)(struct mlxsw_core *mlxsw_core,
 			 const struct devlink_trap *trap, void *trap_ctx);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 519eb44e4097..6bbf0ab7794c 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -418,17 +418,14 @@ static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp)
 }
 
 static int mlxsw_sp_flash_update(struct mlxsw_core *mlxsw_core,
-				 const char *file_name, const char *component,
+				 struct devlink_flash_update_params *params,
 				 struct netlink_ext_ack *extack)
 {
 	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	const struct firmware *firmware;
 	int err;
 
-	if (component)
-		return -EOPNOTSUPP;
-
-	err = request_firmware_direct(&firmware, file_name,
+	err = request_firmware_direct(&firmware, params->file_name,
 				      mlxsw_sp->bus_info->dev);
 	if (err)
 		return err;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index be52510d446b..97d2b03208de 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -329,12 +329,11 @@ nfp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 }
 
 static int
-nfp_devlink_flash_update(struct devlink *devlink, const char *path,
-			 const char *component, struct netlink_ext_ack *extack)
+nfp_devlink_flash_update(struct devlink *devlink,
+			 struct devlink_flash_update_params *params,
+			 struct netlink_ext_ack *extack)
 {
-	if (component)
-		return -EOPNOTSUPP;
-	return nfp_flash_update_common(devlink_priv(devlink), path, extack);
+	return nfp_flash_update_common(devlink_priv(devlink), params->file_name, extack);
 }
 
 const struct devlink_ops nfp_devlink_ops = {
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index ce719c830a77..29bb25f0f069 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -740,8 +740,8 @@ static int nsim_dev_info_get(struct devlink *devlink,
 #define NSIM_DEV_FLASH_CHUNK_SIZE 1000
 #define NSIM_DEV_FLASH_CHUNK_TIME_MS 10
 
-static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
-				 const char *component,
+static int nsim_dev_flash_update(struct devlink *devlink,
+				 struct devlink_flash_update_params *params,
 				 struct netlink_ext_ack *extack)
 {
 	struct nsim_dev *nsim_dev = devlink_priv(devlink);
@@ -751,13 +751,13 @@ static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
 		devlink_flash_update_begin_notify(devlink);
 		devlink_flash_update_status_notify(devlink,
 						   "Preparing to flash",
-						   component, 0, 0);
+						   params->component, 0, 0);
 	}
 
 	for (i = 0; i < NSIM_DEV_FLASH_SIZE / NSIM_DEV_FLASH_CHUNK_SIZE; i++) {
 		if (nsim_dev->fw_update_status)
 			devlink_flash_update_status_notify(devlink, "Flashing",
-							   component,
+							   params->component,
 							   i * NSIM_DEV_FLASH_CHUNK_SIZE,
 							   NSIM_DEV_FLASH_SIZE);
 		msleep(NSIM_DEV_FLASH_CHUNK_TIME_MS);
@@ -765,11 +765,11 @@ static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
 
 	if (nsim_dev->fw_update_status) {
 		devlink_flash_update_status_notify(devlink, "Flashing",
-						   component,
+						   params->component,
 						   NSIM_DEV_FLASH_SIZE,
 						   NSIM_DEV_FLASH_SIZE);
 		devlink_flash_update_status_notify(devlink, "Flashing done",
-						   component, 0, 0);
+						   params->component, 0, 0);
 		devlink_flash_update_end_notify(devlink);
 	}
 
@@ -873,6 +873,7 @@ nsim_dev_devlink_trap_policer_counter_get(struct devlink *devlink,
 }
 
 static const struct devlink_ops nsim_dev_devlink_ops = {
+	.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT,
 	.reload_down = nsim_dev_reload_down,
 	.reload_up = nsim_dev_reload_up,
 	.info_get = nsim_dev_info_get,
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 19d990c8edcc..192a2c5b6e82 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -511,6 +511,22 @@ enum devlink_param_generic_id {
 /* Firmware bundle identifier */
 #define DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID	"fw.bundle_id"
 
+/**
+ * struct devlink_flash_update_params - Flash Update parameters
+ * @file_name: the name of the flash firmware file to update from
+ * @component: the flash component to update
+ *
+ * With the exception of file_name, drivers must opt-in to parameters by
+ * setting the appropriate bit in the supported_flash_update_params field in
+ * their devlink_ops structure.
+ */
+struct devlink_flash_update_params {
+	const char *file_name;
+	const char *component;
+};
+
+#define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT	BIT(0)
+
 struct devlink_region;
 struct devlink_info_req;
 
@@ -985,6 +1001,12 @@ enum devlink_trap_group_generic_id {
 	}
 
 struct devlink_ops {
+	/**
+	 * @supported_flash_update_params:
+	 * mask of parameters supported by the driver's .flash_update
+	 * implemementation.
+	 */
+	u32 supported_flash_update_params;
 	int (*reload_down)(struct devlink *devlink, bool netns_change,
 			   struct netlink_ext_ack *extack);
 	int (*reload_up)(struct devlink *devlink,
@@ -1045,8 +1067,15 @@ struct devlink_ops {
 				      struct netlink_ext_ack *extack);
 	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
 			struct netlink_ext_ack *extack);
-	int (*flash_update)(struct devlink *devlink, const char *file_name,
-			    const char *component,
+	/**
+	 * @flash_update: Device flash update function
+	 *
+	 * Used to perform a flash update for the device. The set of
+	 * parameters supported by the driver should be set in
+	 * supported_flash_update_params.
+	 */
+	int (*flash_update)(struct devlink *devlink,
+			    struct devlink_flash_update_params *params,
 			    struct netlink_ext_ack *extack);
 	/**
 	 * @trap_init: Trap initialization function.
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 0ca89196a367..3ccba85f85c7 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3113,22 +3113,32 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
 static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 				       struct genl_info *info)
 {
+	struct devlink_flash_update_params params = {};
 	struct devlink *devlink = info->user_ptr[0];
-	const char *file_name, *component;
 	struct nlattr *nla_component;
+	u32 supported_params;
 
 	if (!devlink->ops->flash_update)
 		return -EOPNOTSUPP;
 
 	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
 		return -EINVAL;
-	file_name = nla_data(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]);
+
+	supported_params = devlink->ops->supported_flash_update_params;
+
+	params.file_name = nla_data(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]);
 
 	nla_component = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
-	component = nla_component ? nla_data(nla_component) : NULL;
+	if (nla_component) {
+		if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT)) {
+			NL_SET_ERR_MSG_ATTR(info->extack, nla_component,
+					    "component update is not supported");
+			return -EOPNOTSUPP;
+		}
+		params.component = nla_data(nla_component);
+	}
 
-	return devlink->ops->flash_update(devlink, file_name, component,
-					  info->extack);
+	return devlink->ops->flash_update(devlink, &params, info->extack);
 }
 
 static const struct devlink_param devlink_param_generic[] = {
@@ -9527,6 +9537,7 @@ void devlink_compat_running_version(struct net_device *dev,
 
 int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
 {
+	struct devlink_flash_update_params params = {};
 	struct devlink *devlink;
 	int ret;
 
@@ -9539,8 +9550,10 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
 		goto out;
 	}
 
+	params.file_name = file_name;
+
 	mutex_lock(&devlink->lock);
-	ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL);
+	ret = devlink->ops->flash_update(devlink, &params, NULL);
 	mutex_unlock(&devlink->lock);
 
 out:
diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
index de4b32fc4223..1e7541688978 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
@@ -23,6 +23,9 @@ fw_flash_test()
 	devlink dev flash $DL_HANDLE file dummy
 	check_err $? "Failed to flash with status updates on"
 
+	devlink dev flash $DL_HANDLE file dummy component fw.mgmt
+	check_err $? "Failed to flash with component attribute"
+
 	echo "n"> $DEBUGFS_DIR/fw_update_status
 	check_err $? "Failed to disable status updates"
 
-- 
2.28.0.163.g6104cc2f0b60

Re: [net-next v2 1/5] devlink: convert flash_update to use params structure

[Jiri Pirko]

Sat, Aug 01, 2020 at 02:21:55AM CEST, jacob.e.keller@intel.com wrote:
>A future change is going to introduce a new parameter for specifying how
>devices should handle overwrite behavior when updating the flash. This
>will introduce a new argument specifying a bitmask of component
>subsections to allow overwriting behavior.
>
>Prepare for this by converting flash_update to use a new
>devlink_flash_update_params structure. For now this just holds the
>file_name and component, but this enables more easily extending the
>function with new parameters in the future.
>
>To avoid the need for modifying every driver when a new parameter is
>introduced, the supported_flash_update_params field is added to

This is not direstly related to the "params" introduction.
Please split at least to 2 patches.


>devlink_ops. Drivers must opt-in to supported parameters by setting the
>appropriate bits in this field. This allows dropping the check in each
>driver that doesn't support component updates.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Cc: Jakub Kicinski <kuba@kernel.org>
>Cc: Jonathan Corbet <corbet@lwn.net>
>Cc: Michael Chan <michael.chan@broadcom.com>
>Cc: Bin Luo <luobin9@huawei.com>
>Cc: Saeed Mahameed <saeedm@mellanox.com>
>Cc: Leon Romanovsky <leon@kernel.org>
>Cc: Ido Schimmel <idosch@mellanox.com>
>Cc: Danielle Ratson <danieller@mellanox.com>
>---
>
>Changes since v1
>* Add supported_flash_update_params field, to allow drivers to opt-in to the
>  set of supported parameters. This is similar to supported_coalesc_params
>  and was suggested by Jakub. This also makes adding future parameters
>  easier by reducing the need to modify existing drivers! Due to this, the
>  boilerplate check for component is simply removed.
>
>* netdevsim is modified to support the component parameter, and a new simple
>  selftest is added to verify that this works.
>
> .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 19 ++++-------
> .../net/ethernet/huawei/hinic/hinic_devlink.c |  8 ++---
> drivers/net/ethernet/intel/ice/ice_devlink.c  | 18 ++++------
> .../net/ethernet/mellanox/mlx5/core/devlink.c |  8 ++---
> drivers/net/ethernet/mellanox/mlxsw/core.c    |  6 ++--
> drivers/net/ethernet/mellanox/mlxsw/core.h    |  2 +-
> .../net/ethernet/mellanox/mlxsw/spectrum.c    |  7 ++--
> .../net/ethernet/netronome/nfp/nfp_devlink.c  |  9 +++--
> drivers/net/netdevsim/dev.c                   | 13 ++++----
> include/net/devlink.h                         | 33 +++++++++++++++++--
> net/core/devlink.c                            | 25 ++++++++++----
> .../drivers/net/netdevsim/devlink.sh          |  3 ++
> 12 files changed, 87 insertions(+), 64 deletions(-)
>
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>index 3a854195d5b0..d436134bdc40 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>@@ -17,15 +17,13 @@
> #include "bnxt_ethtool.h"
> 
> static int
>-bnxt_dl_flash_update(struct devlink *dl, const char *filename,
>-		     const char *region, struct netlink_ext_ack *extack)
>+bnxt_dl_flash_update(struct devlink *dl,
>+		     struct devlink_flash_update_params *params,
>+		     struct netlink_ext_ack *extack)
> {
> 	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
> 	int rc;
> 
>-	if (region)
>-		return -EOPNOTSUPP;
>-
> 	if (!BNXT_PF(bp)) {
> 		NL_SET_ERR_MSG_MOD(extack,
> 				   "flash update not supported from a VF");
>@@ -33,15 +31,12 @@ bnxt_dl_flash_update(struct devlink *dl, const char *filename,
> 	}
> 
> 	devlink_flash_update_begin_notify(dl);
>-	devlink_flash_update_status_notify(dl, "Preparing to flash", region, 0,
>-					   0);
>-	rc = bnxt_flash_package_from_file(bp->dev, filename, 0);
>+	devlink_flash_update_status_notify(dl, "Preparing to flash", NULL, 0, 0);
>+	rc = bnxt_flash_package_from_file(bp->dev, params->file_name, 0);
> 	if (!rc)
>-		devlink_flash_update_status_notify(dl, "Flashing done", region,
>-						   0, 0);
>+		devlink_flash_update_status_notify(dl, "Flashing done", NULL, 0, 0);
> 	else
>-		devlink_flash_update_status_notify(dl, "Flashing failed",
>-						   region, 0, 0);
>+		devlink_flash_update_status_notify(dl, "Flashing failed", NULL, 0, 0);
> 	devlink_flash_update_end_notify(dl);
> 	return rc;
> }
>diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>index c6adc776f3c8..1f45766107e4 100644
>--- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>+++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
>@@ -281,18 +281,14 @@ static int hinic_firmware_update(struct hinic_devlink_priv *priv,
> }
> 
> static int hinic_devlink_flash_update(struct devlink *devlink,
>-				      const char *file_name,
>-				      const char *component,
>+				      struct devlink_flash_update_params *params,
> 				      struct netlink_ext_ack *extack)
> {
> 	struct hinic_devlink_priv *priv = devlink_priv(devlink);
> 	const struct firmware *fw;
> 	int err;
> 
>-	if (component)
>-		return -EOPNOTSUPP;
>-
>-	err = request_firmware_direct(&fw, file_name,
>+	err = request_firmware_direct(&fw, params->file_name,
> 				      &priv->hwdev->hwif->pdev->dev);
> 	if (err)
> 		return err;
>diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>index dbbd8b6f9d1a..c8255235f7c4 100644
>--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>@@ -233,8 +233,7 @@ static int ice_devlink_info_get(struct devlink *devlink,
> /**
>  * ice_devlink_flash_update - Update firmware stored in flash on the device
>  * @devlink: pointer to devlink associated with device to update
>- * @path: the path of the firmware file to use via request_firmware
>- * @component: name of the component to update, or NULL
>+ * @params: flash update parameters
>  * @extack: netlink extended ACK structure
>  *
>  * Perform a device flash update. The bulk of the update logic is contained
>@@ -243,8 +242,9 @@ static int ice_devlink_info_get(struct devlink *devlink,
>  * Returns: zero on success, or an error code on failure.
>  */
> static int
>-ice_devlink_flash_update(struct devlink *devlink, const char *path,
>-			 const char *component, struct netlink_ext_ack *extack)
>+ice_devlink_flash_update(struct devlink *devlink,
>+			 struct devlink_flash_update_params *params,
>+			 struct netlink_ext_ack *extack)
> {
> 	struct ice_pf *pf = devlink_priv(devlink);
> 	struct device *dev = &pf->pdev->dev;
>@@ -252,20 +252,16 @@ ice_devlink_flash_update(struct devlink *devlink, const char *path,
> 	const struct firmware *fw;
> 	int err;
> 
>-	/* individual component update is not yet supported */
>-	if (component)
>-		return -EOPNOTSUPP;
>-
> 	if (!hw->dev_caps.common_cap.nvm_unified_update) {
> 		NL_SET_ERR_MSG_MOD(extack, "Current firmware does not support unified update");
> 		return -EOPNOTSUPP;
> 	}
> 
>-	err = ice_check_for_pending_update(pf, component, extack);
>+	err = ice_check_for_pending_update(pf, NULL, extack);
> 	if (err)
> 		return err;
> 
>-	err = request_firmware(&fw, path, dev);
>+	err = request_firmware(&fw, params->file_name, dev);
> 	if (err) {
> 		NL_SET_ERR_MSG_MOD(extack, "Unable to read file from disk");
> 		return err;
>@@ -273,7 +269,7 @@ ice_devlink_flash_update(struct devlink *devlink, const char *path,
> 
> 	devlink_flash_update_begin_notify(devlink);
> 	devlink_flash_update_status_notify(devlink, "Preparing to flash",
>-					   component, 0, 0);
>+					   NULL, 0, 0);
> 	err = ice_flash_pldm_image(pf, fw, extack);
> 	devlink_flash_update_end_notify(devlink);
> 
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
>index c709e9a385f6..9b14e3f805a2 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
>@@ -8,18 +8,14 @@
> #include "eswitch.h"
> 
> static int mlx5_devlink_flash_update(struct devlink *devlink,
>-				     const char *file_name,
>-				     const char *component,
>+				     struct devlink_flash_update_params *params,
> 				     struct netlink_ext_ack *extack)
> {
> 	struct mlx5_core_dev *dev = devlink_priv(devlink);
> 	const struct firmware *fw;
> 	int err;
> 
>-	if (component)
>-		return -EOPNOTSUPP;
>-
>-	err = request_firmware_direct(&fw, file_name, &dev->pdev->dev);
>+	err = request_firmware_direct(&fw, params->file_name, &dev->pdev->dev);
> 	if (err)
> 		return err;
> 
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
>index b01f8f2fab63..6db938708a0d 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
>@@ -1138,8 +1138,7 @@ mlxsw_devlink_core_bus_device_reload_up(struct devlink *devlink,
> }
> 
> static int mlxsw_devlink_flash_update(struct devlink *devlink,
>-				      const char *file_name,
>-				      const char *component,
>+				      struct devlink_flash_update_params *params,
> 				      struct netlink_ext_ack *extack)
> {
> 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>@@ -1147,8 +1146,7 @@ static int mlxsw_devlink_flash_update(struct devlink *devlink,
> 
> 	if (!mlxsw_driver->flash_update)
> 		return -EOPNOTSUPP;
>-	return mlxsw_driver->flash_update(mlxsw_core, file_name,
>-					  component, extack);
>+	return mlxsw_driver->flash_update(mlxsw_core, params, extack);
> }
> 
> static int mlxsw_devlink_trap_init(struct devlink *devlink,
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
>index c1c1e039323a..b6e3faf38305 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
>+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
>@@ -318,7 +318,7 @@ struct mlxsw_driver {
> 				       enum devlink_sb_pool_type pool_type,
> 				       u32 *p_cur, u32 *p_max);
> 	int (*flash_update)(struct mlxsw_core *mlxsw_core,
>-			    const char *file_name, const char *component,
>+			    struct devlink_flash_update_params *params,
> 			    struct netlink_ext_ack *extack);
> 	int (*trap_init)(struct mlxsw_core *mlxsw_core,
> 			 const struct devlink_trap *trap, void *trap_ctx);
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>index 519eb44e4097..6bbf0ab7794c 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>@@ -418,17 +418,14 @@ static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp)
> }
> 
> static int mlxsw_sp_flash_update(struct mlxsw_core *mlxsw_core,
>-				 const char *file_name, const char *component,
>+				 struct devlink_flash_update_params *params,
> 				 struct netlink_ext_ack *extack)
> {
> 	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> 	const struct firmware *firmware;
> 	int err;
> 
>-	if (component)
>-		return -EOPNOTSUPP;
>-
>-	err = request_firmware_direct(&firmware, file_name,
>+	err = request_firmware_direct(&firmware, params->file_name,
> 				      mlxsw_sp->bus_info->dev);
> 	if (err)
> 		return err;
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>index be52510d446b..97d2b03208de 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>@@ -329,12 +329,11 @@ nfp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
> }
> 
> static int
>-nfp_devlink_flash_update(struct devlink *devlink, const char *path,
>-			 const char *component, struct netlink_ext_ack *extack)
>+nfp_devlink_flash_update(struct devlink *devlink,
>+			 struct devlink_flash_update_params *params,
>+			 struct netlink_ext_ack *extack)
> {
>-	if (component)
>-		return -EOPNOTSUPP;
>-	return nfp_flash_update_common(devlink_priv(devlink), path, extack);
>+	return nfp_flash_update_common(devlink_priv(devlink), params->file_name, extack);
> }
> 
> const struct devlink_ops nfp_devlink_ops = {
>diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>index ce719c830a77..29bb25f0f069 100644
>--- a/drivers/net/netdevsim/dev.c
>+++ b/drivers/net/netdevsim/dev.c
>@@ -740,8 +740,8 @@ static int nsim_dev_info_get(struct devlink *devlink,
> #define NSIM_DEV_FLASH_CHUNK_SIZE 1000
> #define NSIM_DEV_FLASH_CHUNK_TIME_MS 10
> 
>-static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
>-				 const char *component,
>+static int nsim_dev_flash_update(struct devlink *devlink,
>+				 struct devlink_flash_update_params *params,
> 				 struct netlink_ext_ack *extack)
> {
> 	struct nsim_dev *nsim_dev = devlink_priv(devlink);
>@@ -751,13 +751,13 @@ static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
> 		devlink_flash_update_begin_notify(devlink);
> 		devlink_flash_update_status_notify(devlink,
> 						   "Preparing to flash",
>-						   component, 0, 0);
>+						   params->component, 0, 0);
> 	}
> 
> 	for (i = 0; i < NSIM_DEV_FLASH_SIZE / NSIM_DEV_FLASH_CHUNK_SIZE; i++) {
> 		if (nsim_dev->fw_update_status)
> 			devlink_flash_update_status_notify(devlink, "Flashing",
>-							   component,
>+							   params->component,
> 							   i * NSIM_DEV_FLASH_CHUNK_SIZE,
> 							   NSIM_DEV_FLASH_SIZE);
> 		msleep(NSIM_DEV_FLASH_CHUNK_TIME_MS);
>@@ -765,11 +765,11 @@ static int nsim_dev_flash_update(struct devlink *devlink, const char *file_name,
> 
> 	if (nsim_dev->fw_update_status) {
> 		devlink_flash_update_status_notify(devlink, "Flashing",
>-						   component,
>+						   params->component,
> 						   NSIM_DEV_FLASH_SIZE,
> 						   NSIM_DEV_FLASH_SIZE);
> 		devlink_flash_update_status_notify(devlink, "Flashing done",
>-						   component, 0, 0);
>+						   params->component, 0, 0);
> 		devlink_flash_update_end_notify(devlink);
> 	}
> 
>@@ -873,6 +873,7 @@ nsim_dev_devlink_trap_policer_counter_get(struct devlink *devlink,
> }
> 
> static const struct devlink_ops nsim_dev_devlink_ops = {
>+	.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT,
> 	.reload_down = nsim_dev_reload_down,
> 	.reload_up = nsim_dev_reload_up,
> 	.info_get = nsim_dev_info_get,
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 19d990c8edcc..192a2c5b6e82 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -511,6 +511,22 @@ enum devlink_param_generic_id {
> /* Firmware bundle identifier */
> #define DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID	"fw.bundle_id"
> 
>+/**
>+ * struct devlink_flash_update_params - Flash Update parameters
>+ * @file_name: the name of the flash firmware file to update from
>+ * @component: the flash component to update
>+ *
>+ * With the exception of file_name, drivers must opt-in to parameters by
>+ * setting the appropriate bit in the supported_flash_update_params field in
>+ * their devlink_ops structure.
>+ */
>+struct devlink_flash_update_params {
>+	const char *file_name;
>+	const char *component;
>+};
>+
>+#define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT	BIT(0)
>+
> struct devlink_region;
> struct devlink_info_req;
> 
>@@ -985,6 +1001,12 @@ enum devlink_trap_group_generic_id {
> 	}
> 
> struct devlink_ops {
>+	/**
>+	 * @supported_flash_update_params:
>+	 * mask of parameters supported by the driver's .flash_update
>+	 * implemementation.
>+	 */
>+	u32 supported_flash_update_params;
> 	int (*reload_down)(struct devlink *devlink, bool netns_change,
> 			   struct netlink_ext_ack *extack);
> 	int (*reload_up)(struct devlink *devlink,
>@@ -1045,8 +1067,15 @@ struct devlink_ops {
> 				      struct netlink_ext_ack *extack);
> 	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
> 			struct netlink_ext_ack *extack);
>-	int (*flash_update)(struct devlink *devlink, const char *file_name,
>-			    const char *component,
>+	/**
>+	 * @flash_update: Device flash update function
>+	 *
>+	 * Used to perform a flash update for the device. The set of
>+	 * parameters supported by the driver should be set in
>+	 * supported_flash_update_params.
>+	 */
>+	int (*flash_update)(struct devlink *devlink,
>+			    struct devlink_flash_update_params *params,
> 			    struct netlink_ext_ack *extack);
> 	/**
> 	 * @trap_init: Trap initialization function.
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 0ca89196a367..3ccba85f85c7 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3113,22 +3113,32 @@ EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
> static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> 				       struct genl_info *info)
> {
>+	struct devlink_flash_update_params params = {};
> 	struct devlink *devlink = info->user_ptr[0];
>-	const char *file_name, *component;
> 	struct nlattr *nla_component;
>+	u32 supported_params;
> 
> 	if (!devlink->ops->flash_update)
> 		return -EOPNOTSUPP;
> 
> 	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
> 		return -EINVAL;
>-	file_name = nla_data(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]);
>+
>+	supported_params = devlink->ops->supported_flash_update_params;

It is a bit odd to have this "flash_update" specific. Perhaps better to
have it as generic devlink "cap"? I can easily imagine this being handy
for other ops too.



>+
>+	params.file_name = nla_data(info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]);
> 
> 	nla_component = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
>-	component = nla_component ? nla_data(nla_component) : NULL;
>+	if (nla_component) {
>+		if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT)) {
>+			NL_SET_ERR_MSG_ATTR(info->extack, nla_component,
>+					    "component update is not supported");
>+			return -EOPNOTSUPP;
>+		}
>+		params.component = nla_data(nla_component);
>+	}
> 
>-	return devlink->ops->flash_update(devlink, file_name, component,
>-					  info->extack);
>+	return devlink->ops->flash_update(devlink, &params, info->extack);
> }
> 
> static const struct devlink_param devlink_param_generic[] = {
>@@ -9527,6 +9537,7 @@ void devlink_compat_running_version(struct net_device *dev,
> 
> int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
> {
>+	struct devlink_flash_update_params params = {};
> 	struct devlink *devlink;
> 	int ret;
> 
>@@ -9539,8 +9550,10 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
> 		goto out;
> 	}
> 
>+	params.file_name = file_name;
>+
> 	mutex_lock(&devlink->lock);
>-	ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL);
>+	ret = devlink->ops->flash_update(devlink, &params, NULL);
> 	mutex_unlock(&devlink->lock);
> 
> out:
>diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>index de4b32fc4223..1e7541688978 100755
>--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
>@@ -23,6 +23,9 @@ fw_flash_test()
> 	devlink dev flash $DL_HANDLE file dummy
> 	check_err $? "Failed to flash with status updates on"
> 
>+	devlink dev flash $DL_HANDLE file dummy component fw.mgmt

Perhaps you can rather use something made up instead of "fw.mgmt" which
is looking legit and therefor confusing.


>+	check_err $? "Failed to flash with component attribute"
>+
> 	echo "n"> $DEBUGFS_DIR/fw_update_status
> 	check_err $? "Failed to disable status updates"
> 
>-- 
>2.28.0.163.g6104cc2f0b60
>

[net-next v2 2/5] devlink: introduce flash update overwrite mask

[Jacob Keller]

Sections of device flash may contain settings or device identifying
information. When performing a flash update, it is generally expected
that these settings and identifiers are not overwritten.

Sometimes it is useful to be able to overwrite these fields when
performing a flash update. To support this, a new attribute, the
DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK is defined. This mask defines
the subsections of flash components that should be overwritten when
updating.

By default, only the flash binaries are updated. Two bits are defined
for specifying overwriting of the settings and device identifiers.

I chose to use a u32 instead of an nla_bitfield32 primarily because we
do not need the selector. This isn't a request to set bits in a stored
bitmask. Also, nla_bitfields aren't supported by libmnl currently, so it
would complicate enabling this in iproute2/devlink.

Modify the netdevsim driver to enable support for this parameter and add
a few self tests used to make sure the attribute works. A new debugfs
hook is used to control what set of overwrite mask values the netdevsim
driver will accept.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1
* separate the ice driver changes to a follow-on patch
* use the new supported_flash_update_params

 .../networking/devlink/devlink-flash.rst      | 29 +++++++++++++++++++
 drivers/net/netdevsim/dev.c                   | 10 ++++++-
 drivers/net/netdevsim/netdevsim.h             |  1 +
 include/net/devlink.h                         |  4 ++-
 include/uapi/linux/devlink.h                  | 24 +++++++++++++++
 net/core/devlink.c                            | 14 ++++++++-
 .../drivers/net/netdevsim/devlink.sh          | 18 ++++++++++++
 7 files changed, 97 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/devlink/devlink-flash.rst b/Documentation/networking/devlink/devlink-flash.rst
index 40a87c0222cb..bf97f5c8d4bd 100644
--- a/Documentation/networking/devlink/devlink-flash.rst
+++ b/Documentation/networking/devlink/devlink-flash.rst
@@ -16,6 +16,35 @@ Note that the file name is a path relative to the firmware loading path
 (usually ``/lib/firmware/``). Drivers may send status updates to inform
 user space about the progress of the update operation.
 
+Overwrite Mask
+==============
+
+The ``devlink-flash`` command allows optionally specifying a mask indicating
+the how the device should handle subsections of flash components when
+updating. This mask indicates the set of sections which are allowed to be
+overwritten.
+
+.. list-table:: List of overwrite mask bits
+   :widths: 5 95
+
+   * - Name
+     - Description
+   * - ``DEVLINK_FLASH_OVERWRITE_SETTINGS``
+     - Indicates that the device should overwrite settings in the components
+       being updated with the settings found in the provided image.
+   * - ``DEVLINK_FLASH_OVERWRITE_IDENTIFIERS``
+     - Indicates that the device should overwrite identifiers in the
+       components being updated with the identifiers found in the provided
+       image. This includes MAC addresses, serial IDs, and similar device
+       identifiers.
+
+Multiple overwrite bits may be combined and requested together. If no bits
+are provided, it is expected that the device only update firmware binaries
+in the components being updated. Settings and identifiers are expected to be
+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.
+
 Firmware Loading
 ================
 
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 29bb25f0f069..3b378238a091 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -201,6 +201,8 @@ static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev)
 		return PTR_ERR(nsim_dev->ports_ddir);
 	debugfs_create_bool("fw_update_status", 0600, nsim_dev->ddir,
 			    &nsim_dev->fw_update_status);
+	debugfs_create_u32("fw_update_overwrite_mask", 0600, nsim_dev->ddir,
+			    &nsim_dev->fw_update_overwrite_mask);
 	debugfs_create_u32("max_macs", 0600, nsim_dev->ddir,
 			   &nsim_dev->max_macs);
 	debugfs_create_bool("test1", 0600, nsim_dev->ddir,
@@ -747,6 +749,9 @@ static int nsim_dev_flash_update(struct devlink *devlink,
 	struct nsim_dev *nsim_dev = devlink_priv(devlink);
 	int i;
 
+	if ((params->overwrite_mask & ~nsim_dev->fw_update_overwrite_mask) != 0)
+		return -EOPNOTSUPP;
+
 	if (nsim_dev->fw_update_status) {
 		devlink_flash_update_begin_notify(devlink);
 		devlink_flash_update_status_notify(devlink,
@@ -873,7 +878,8 @@ nsim_dev_devlink_trap_policer_counter_get(struct devlink *devlink,
 }
 
 static const struct devlink_ops nsim_dev_devlink_ops = {
-	.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT,
+	.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT |
+					 DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
 	.reload_down = nsim_dev_reload_down,
 	.reload_up = nsim_dev_reload_up,
 	.info_get = nsim_dev_info_get,
@@ -988,6 +994,7 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
 	INIT_LIST_HEAD(&nsim_dev->port_list);
 	mutex_init(&nsim_dev->port_list_lock);
 	nsim_dev->fw_update_status = true;
+	nsim_dev->fw_update_overwrite_mask = 0;
 
 	nsim_dev->fib_data = nsim_fib_create(devlink, extack);
 	if (IS_ERR(nsim_dev->fib_data))
@@ -1046,6 +1053,7 @@ int nsim_dev_probe(struct nsim_bus_dev *nsim_bus_dev)
 	INIT_LIST_HEAD(&nsim_dev->port_list);
 	mutex_init(&nsim_dev->port_list_lock);
 	nsim_dev->fw_update_status = true;
+	nsim_dev->fw_update_overwrite_mask = 0;
 	nsim_dev->max_macs = NSIM_DEV_MAX_MACS_DEFAULT;
 	nsim_dev->test1 = NSIM_DEV_TEST1_DEFAULT;
 	spin_lock_init(&nsim_dev->fa_cookie_lock);
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index d164052e0393..6ad1250c9362 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -185,6 +185,7 @@ struct nsim_dev {
 	struct list_head port_list;
 	struct mutex port_list_lock; /* protects port list */
 	bool fw_update_status;
+	u32 fw_update_overwrite_mask;
 	u32 max_macs;
 	bool test1;
 	bool dont_allow_reload;
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 192a2c5b6e82..072810d05812 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -523,9 +523,11 @@ enum devlink_param_generic_id {
 struct devlink_flash_update_params {
 	const char *file_name;
 	const char *component;
+	u32 overwrite_mask;
 };
 
-#define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT	BIT(0)
+#define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT		BIT(0)
+#define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK	BIT(1)
 
 struct devlink_region;
 struct devlink_info_req;
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index cfef4245ea5a..12cfd84c6956 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -228,6 +228,28 @@ enum {
 	DEVLINK_ATTR_STATS_MAX = __DEVLINK_ATTR_STATS_MAX - 1
 };
 
+/* Specify what sections of a flash component can be overwritten when
+ * performing an update. Overwriting of firmware binary sections is always
+ * implicitly assumed to be allowed.
+ *
+ * Each section must be documented in
+ * Documentation/networking/devlink/devlink-flash.rst
+ *
+ */
+enum {
+	DEVLINK_FLASH_OVERWRITE_SETTINGS_BIT,
+	DEVLINK_FLASH_OVERWRITE_IDENTIFIERS_BIT,
+
+	__DEVLINK_FLASH_OVERWRITE_MAX_BIT,
+	DEVLINK_FLASH_OVERWRITE_MAX_BIT = __DEVLINK_FLASH_OVERWRITE_MAX_BIT - 1
+};
+
+#define DEVLINK_FLASH_OVERWRITE_SETTINGS BIT(DEVLINK_FLASH_OVERWRITE_SETTINGS_BIT)
+#define DEVLINK_FLASH_OVERWRITE_IDENTIFIERS BIT(DEVLINK_FLASH_OVERWRITE_IDENTIFIERS_BIT)
+
+#define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
+	GENMASK(DEVLINK_FLASH_OVERWRITE_MAX_BIT, 0)
+
 /**
  * enum devlink_trap_action - Packet trap action.
  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
@@ -458,6 +480,8 @@ enum devlink_attr {
 	DEVLINK_ATTR_PORT_LANES,			/* u32 */
 	DEVLINK_ATTR_PORT_SPLITTABLE,			/* u8 */
 
+	DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK,	/* u32 */
+
 	/* 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 3ccba85f85c7..cbaafaf3147b 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3115,7 +3115,7 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 {
 	struct devlink_flash_update_params params = {};
 	struct devlink *devlink = info->user_ptr[0];
-	struct nlattr *nla_component;
+	struct nlattr *nla_component, *nla_mask;
 	u32 supported_params;
 
 	if (!devlink->ops->flash_update)
@@ -3138,6 +3138,16 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 		params.component = nla_data(nla_component);
 	}
 
+	nla_mask = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK];
+	if (nla_mask) {
+		if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK)) {
+			NL_SET_ERR_MSG_ATTR(info->extack, nla_mask,
+					    "overwrite is not supported");
+			return -EOPNOTSUPP;
+		}
+		params.overwrite_mask = nla_get_u32(nla_mask);
+	}
+
 	return devlink->ops->flash_update(devlink, &params, info->extack);
 }
 
@@ -7025,6 +7035,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER] = { .type = NLA_U8 },
 	[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT] = { .type = NLA_NUL_STRING },
+	[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK] =
+		NLA_POLICY_MAX(NLA_U32, DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS),
 	[DEVLINK_ATTR_TRAP_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_TRAP_ACTION] = { .type = NLA_U8 },
 	[DEVLINK_ATTR_TRAP_GROUP_NAME] = { .type = NLA_NUL_STRING },
diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
index 1e7541688978..40909c254365 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
@@ -26,6 +26,24 @@ fw_flash_test()
 	devlink dev flash $DL_HANDLE file dummy component fw.mgmt
 	check_err $? "Failed to flash with component attribute"
 
+	devlink dev flash $DL_HANDLE file dummy overwrite settings
+	check_fail $? "Flash with overwrite settings should be rejected"
+
+	echo "1"> $DEBUGFS_DIR/fw_update_overwrite_mask
+	check_err $? "Failed to change allowed overwrite mask"
+
+	devlink dev flash $DL_HANDLE file dummy overwrite settings
+	check_err $? "Failed to flash with settings overwrite enabled"
+
+	devlink dev flash $DL_HANDLE file dummy overwrite identifiers
+	check_fail $? "Flash with overwrite settings should be identifiers"
+
+	echo "3"> $DEBUGFS_DIR/fw_update_overwrite_mask
+	check_err $? "Failed to change allowed overwrite mask"
+
+	devlink dev flash $DL_HANDLE file dummy overwrite identifiers overwrite settings
+	check_err $? "Failed to flash with settings and identifiers overwrite enabled"
+
 	echo "n"> $DEBUGFS_DIR/fw_update_status
 	check_err $? "Failed to disable status updates"
 
-- 
2.28.0.163.g6104cc2f0b60

Re: [net-next v2 2/5] devlink: introduce flash update overwrite mask

[Jiri Pirko]

Sat, Aug 01, 2020 at 02:21:56AM CEST, jacob.e.keller@intel.com wrote:

[...]

>+	nla_mask = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK];
>+	if (nla_mask) {
>+		if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK)) {
>+			NL_SET_ERR_MSG_ATTR(info->extack, nla_mask,
>+					    "overwrite is not supported");
>+			return -EOPNOTSUPP;
>+		}
>+		params.overwrite_mask = nla_get_u32(nla_mask);

It's a bitfield, should be NL_ATTR_TYPE_BITFIELD32.

Re: [net-next v2 2/5] devlink: introduce flash update overwrite mask

[Jacob Keller]

On 8/3/2020 8:38 AM, Jiri Pirko wrote:
> Sat, Aug 01, 2020 at 02:21:56AM CEST, jacob.e.keller@intel.com wrote:
> 
> [...]
> 
>> +	nla_mask = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK];
>> +	if (nla_mask) {
>> +		if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK)) {
>> +			NL_SET_ERR_MSG_ATTR(info->extack, nla_mask,
>> +					    "overwrite is not supported");
>> +			return -EOPNOTSUPP;
>> +		}
>> +		params.overwrite_mask = nla_get_u32(nla_mask);
> 
> It's a bitfield, should be NL_ATTR_TYPE_BITFIELD32.
> 

I disagree. BITFIELD32 has both a mask and a field. This doesn't have
the notion of a mask. The bits you allow are set, the bits you don't
allow are not set. Having both a mask and a field over complicates this.

Re: [net-next v2 2/5] devlink: introduce flash update overwrite mask

[Jacob Keller]

On 8/3/2020 8:38 AM, Jiri Pirko wrote:
> Sat, Aug 01, 2020 at 02:21:56AM CEST, jacob.e.keller@intel.com wrote:
> 
> [...]
> 
>> +	nla_mask = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK];
>> +	if (nla_mask) {
>> +		if (!(supported_params & DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK)) {
>> +			NL_SET_ERR_MSG_ATTR(info->extack, nla_mask,
>> +					    "overwrite is not supported");
>> +			return -EOPNOTSUPP;
>> +		}
>> +		params.overwrite_mask = nla_get_u32(nla_mask);
> 
> It's a bitfield, should be NL_ATTR_TYPE_BITFIELD32.
> 

v3 will use a bitfield, despite I don't believe it needs this. The
overwrite mask provided to the driver will be the bitwise AND of the
selector and the value (i.e. userspace will have to set the bit in both
the selector and value to get it enabled).

Thanks,
Jake

[net-next v2 3/5] ice: add support for flash update overwrite mask

[Jacob Keller]

The ice driver has the ability to inform firmware of what sections of
flash to preserve when performing an update. Expose this support via the
recently added overwrite_mask support. Based on which sections are
specified, determine the equivalent preservation level and inform
firmware.

Reject combinations of sections that cannot be handled by the
preservation levels. Specifically, a request to overwrite identifiers
but not settings cannot be done, so it is rejected as not supported.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1
* add this patch containing the ice changes as a separate commit

 Documentation/networking/devlink/ice.rst      | 31 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 19 +++++++++++-
 .../net/ethernet/intel/ice/ice_fw_update.c    | 16 ++++++++--
 .../net/ethernet/intel/ice/ice_fw_update.h    |  2 +-
 4 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
index 237848d56f9b..8eb50ba41f1a 100644
--- a/Documentation/networking/devlink/ice.rst
+++ b/Documentation/networking/devlink/ice.rst
@@ -81,6 +81,37 @@ The ``ice`` driver reports the following versions
       - 0xee16ced7
       - The first 4 bytes of the hash of the netlist module contents.
 
+Flash Update
+============
+
+The ``ice`` driver implements support for flash update using the
+``devlink-flash`` interface. It supports updating the device flash using a
+combined flash image that contains the ``fw.mgmt``, ``fw.undi``, and
+``fw.netlist`` components.
+
+.. list-table:: List of supported overwrite modes
+   :widths: 5 95
+
+   * - Bits
+     - Behavior
+   * - ``DEVLINK_FLASH_OVERWRITE_SETTINGS``
+     - Do not preserve settings stored in the flash components being
+       updated. This includes overwriting the port configuration that
+       determines the number of physical functions the device will
+       initialize with.
+   * - ``DEVLINK_FLASH_OVERWRITE_SETTINGS`` and ``DEVLINK_FLASH_OVERWRITE_IDENTIFIERS``
+     - Do not preserve either settings or identifiers. Overwrite everything
+       in the flash with the contents from the provided image, without
+       performing any preservation. This includes overwriting device
+       identifying fields such as the MAC address, VPD area, and device
+       serial number. It is expected that this combination be used with an
+       image customized for the specific device.
+
+The ice hardware does not support overwriting only identifiers while
+preserving settings, and thus ``DEVLINK_FLASH_OVERWRITE_IDENTIFIERS`` on its
+own will be rejected. If no overwrite mask is provided, the firmware will be
+instructed to preserve all settings and identifying fields when updating.
+
 Regions
 =======
 
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index c8255235f7c4..d2cb9144c63f 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -250,8 +250,24 @@ ice_devlink_flash_update(struct devlink *devlink,
 	struct device *dev = &pf->pdev->dev;
 	struct ice_hw *hw = &pf->hw;
 	const struct firmware *fw;
+	u8 preservation;
 	int err;
 
+	if (!params->overwrite_mask) {
+		/* preserve all settings and identifiers */
+		preservation = ICE_AQC_NVM_PRESERVE_ALL;
+	} else if (params->overwrite_mask == DEVLINK_FLASH_OVERWRITE_SETTINGS) {
+		/* overwrite settings, but preserve the vital device identifiers */
+		preservation = ICE_AQC_NVM_PRESERVE_SELECTED;
+	} else if (params->overwrite_mask == (DEVLINK_FLASH_OVERWRITE_SETTINGS |
+					      DEVLINK_FLASH_OVERWRITE_IDENTIFIERS)) {
+		/* overwrite both settings and identifiers, preserve nothing */
+		preservation = ICE_AQC_NVM_NO_PRESERVATION;
+	} else {
+		NL_SET_ERR_MSG_MOD(extack, "Requested overwrite mask is not supported");
+		return -EOPNOTSUPP;
+	}
+
 	if (!hw->dev_caps.common_cap.nvm_unified_update) {
 		NL_SET_ERR_MSG_MOD(extack, "Current firmware does not support unified update");
 		return -EOPNOTSUPP;
@@ -270,7 +286,7 @@ ice_devlink_flash_update(struct devlink *devlink,
 	devlink_flash_update_begin_notify(devlink);
 	devlink_flash_update_status_notify(devlink, "Preparing to flash",
 					   NULL, 0, 0);
-	err = ice_flash_pldm_image(pf, fw, extack);
+	err = ice_flash_pldm_image(pf, fw, preservation, extack);
 	devlink_flash_update_end_notify(devlink);
 
 	release_firmware(fw);
@@ -279,6 +295,7 @@ ice_devlink_flash_update(struct devlink *devlink,
 }
 
 static const struct devlink_ops ice_devlink_ops = {
+	.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
 	.info_get = ice_devlink_info_get,
 	.flash_update = ice_devlink_flash_update,
 };
diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
index deaefe00c9c0..c81273000d88 100644
--- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
@@ -630,6 +630,7 @@ static const struct pldmfw_ops ice_fwu_ops = {
  * ice_flash_pldm_image - Write a PLDM-formatted firmware image to the device
  * @pf: private device driver structure
  * @fw: firmware object pointing to the relevant firmware file
+ * @preservation: preservation level to request from firmware
  * @extack: netlink extended ACK structure
  *
  * Parse the data for a given firmware file, verifying that it is a valid PLDM
@@ -643,7 +644,7 @@ static const struct pldmfw_ops ice_fwu_ops = {
  * Returns: zero on success or a negative error code on failure.
  */
 int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw,
-			 struct netlink_ext_ack *extack)
+			 u8 preservation, struct netlink_ext_ack *extack)
 {
 	struct device *dev = ice_pf_to_dev(pf);
 	struct ice_hw *hw = &pf->hw;
@@ -651,13 +652,24 @@ int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw,
 	enum ice_status status;
 	int err;
 
+	switch (preservation) {
+	case ICE_AQC_NVM_PRESERVE_ALL:
+	case ICE_AQC_NVM_PRESERVE_SELECTED:
+	case ICE_AQC_NVM_NO_PRESERVATION:
+	case ICE_AQC_NVM_FACTORY_DEFAULT:
+		break;
+	default:
+		WARN(1, "Unexpected preservation level request %u", preservation);
+		return -EINVAL;
+	}
+
 	memset(&priv, 0, sizeof(priv));
 
 	priv.context.ops = &ice_fwu_ops;
 	priv.context.dev = dev;
 	priv.extack = extack;
 	priv.pf = pf;
-	priv.activate_flags = ICE_AQC_NVM_PRESERVE_ALL;
+	priv.activate_flags = preservation;
 
 	status = ice_acquire_nvm(hw, ICE_RES_WRITE);
 	if (status) {
diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.h b/drivers/net/ethernet/intel/ice/ice_fw_update.h
index 79472cc618b4..c6390f6851ff 100644
--- a/drivers/net/ethernet/intel/ice/ice_fw_update.h
+++ b/drivers/net/ethernet/intel/ice/ice_fw_update.h
@@ -5,7 +5,7 @@
 #define _ICE_FW_UPDATE_H_
 
 int ice_flash_pldm_image(struct ice_pf *pf, const struct firmware *fw,
-			 struct netlink_ext_ack *extack);
+			 u8 preservation, struct netlink_ext_ack *extack);
 int ice_check_for_pending_update(struct ice_pf *pf, const char *component,
 				 struct netlink_ext_ack *extack);
 
-- 
2.28.0.163.g6104cc2f0b60

[iproute2-next v2 4/5] Update devlink header for overwrite mask attribute

[Jacob Keller]

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 include/uapi/linux/devlink.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index b7f23faae901..3d006ad2fdaf 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -228,6 +228,28 @@ enum {
 	DEVLINK_ATTR_STATS_MAX = __DEVLINK_ATTR_STATS_MAX - 1
 };
 
+/* Specify what sections of a flash component can be overwritten when
+ * performing an update. Overwriting of firmware binary sections is always
+ * implicitly assumed to be allowed.
+ *
+ * Each section must be documented in
+ * Documentation/networking/devlink/devlink-flash.rst
+ *
+ */
+enum {
+	DEVLINK_FLASH_OVERWRITE_SETTINGS_BIT,
+	DEVLINK_FLASH_OVERWRITE_IDENTIFIERS_BIT,
+
+	__DEVLINK_FLASH_OVERWRITE_MAX_BIT,
+	DEVLINK_FLASH_OVERWRITE_MAX_BIT = __DEVLINK_FLASH_OVERWRITE_MAX_BIT - 1
+};
+
+#define DEVLINK_FLASH_OVERWRITE_SETTINGS BIT(DEVLINK_FLASH_OVERWRITE_SETTINGS_BIT)
+#define DEVLINK_FLASH_OVERWRITE_IDENTIFIERS BIT(DEVLINK_FLASH_OVERWRITE_IDENTIFIERS_BIT)
+
+#define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
+	GENMASK(DEVLINK_FLASH_OVERWRITE_MAX_BIT, 0)
+
 /**
  * enum devlink_trap_action - Packet trap action.
  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
@@ -458,6 +480,8 @@ enum devlink_attr {
 	DEVLINK_ATTR_PORT_LANES,			/* u32 */
 	DEVLINK_ATTR_PORT_SPLITTABLE,			/* u8 */
 
+	DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK,	/* u32 */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
-- 
2.28.0.163.g6104cc2f0b60

[iproute2-next v2 5/5] devlink: support setting the overwrite mask

[Jacob Keller]

Add support for specifying the overwrite sections to allow in the flash
update command. This is done by adding a new "overwrite" option which
can take either "settings" or "identifiers" passing the overwrite mode
multiple times will combine the fields using bitwise-OR.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 devlink/devlink.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 7dbe9c7e07a8..a3360a09898b 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -302,6 +302,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
 #define DL_OPT_TRAP_POLICER_BURST	BIT(36)
 #define DL_OPT_HEALTH_REPORTER_AUTO_DUMP     BIT(37)
 #define DL_OPT_PORT_FUNCTION_HW_ADDR BIT(38)
+#define DL_OPT_FLASH_OVERWRITE		BIT(39)
 
 struct dl_opts {
 	uint64_t present; /* flags of present items */
@@ -349,6 +350,7 @@ struct dl_opts {
 	uint64_t trap_policer_burst;
 	char port_function_hw_addr[MAX_ADDR_LEN];
 	uint32_t port_function_hw_addr_len;
+	uint32_t overwrite_mask;
 };
 
 struct dl {
@@ -1282,6 +1284,19 @@ eswitch_encap_mode_get(const char *typestr,
 	return 0;
 }
 
+static int flash_overwrite_mask_get(const char *sectionstr, uint32_t *mask)
+{
+	if (strcmp(sectionstr, "settings") == 0) {
+		*mask |= DEVLINK_FLASH_OVERWRITE_SETTINGS;
+	} else if (strcmp(sectionstr, "identifiers") == 0) {
+		*mask |= DEVLINK_FLASH_OVERWRITE_IDENTIFIERS;
+	} else {
+		pr_err("Unknown overwrite section \"%s\"\n", sectionstr);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int param_cmode_get(const char *cmodestr,
 			   enum devlink_param_cmode *cmode)
 {
@@ -1624,6 +1639,21 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
 			if (err)
 				return err;
 			o_found |= DL_OPT_FLASH_COMPONENT;
+
+		} else if (dl_argv_match(dl, "overwrite") &&
+				(o_all & DL_OPT_FLASH_OVERWRITE)) {
+			const char *sectionstr;
+
+			dl_arg_inc(dl);
+			err = dl_argv_str(dl, &sectionstr);
+			if(err)
+				return err;
+			err = flash_overwrite_mask_get(sectionstr,
+							&opts->overwrite_mask);
+			if (err)
+				return err;
+			o_found |= DL_OPT_FLASH_OVERWRITE;
+
 		} else if (dl_argv_match(dl, "reporter") &&
 			   (o_all & DL_OPT_HEALTH_REPORTER_NAME)) {
 			dl_arg_inc(dl);
@@ -1851,6 +1881,9 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
 	if (opts->present & DL_OPT_FLASH_COMPONENT)
 		mnl_attr_put_strz(nlh, DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,
 				  opts->flash_component);
+	if (opts->present & DL_OPT_FLASH_OVERWRITE)
+		mnl_attr_put_u32(nlh, DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK,
+				 opts->overwrite_mask);
 	if (opts->present & DL_OPT_HEALTH_REPORTER_NAME)
 		mnl_attr_put_strz(nlh, DEVLINK_ATTR_HEALTH_REPORTER_NAME,
 				  opts->reporter_name);
@@ -1951,7 +1984,7 @@ static void cmd_dev_help(void)
 	pr_err("       devlink dev param show [DEV name PARAMETER]\n");
 	pr_err("       devlink dev reload DEV [ netns { PID | NAME | ID } ]\n");
 	pr_err("       devlink dev info [ DEV ]\n");
-	pr_err("       devlink dev flash DEV file PATH [ component NAME ]\n");
+	pr_err("       devlink dev flash DEV file PATH [ component NAME ] [ overwrite SECTION ]\n");
 }
 
 static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
@@ -3205,7 +3238,7 @@ static int cmd_dev_flash(struct dl *dl)
 			       NLM_F_REQUEST | NLM_F_ACK);
 
 	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE | DL_OPT_FLASH_FILE_NAME,
-				DL_OPT_FLASH_COMPONENT);
+				DL_OPT_FLASH_COMPONENT | DL_OPT_FLASH_OVERWRITE);
 	if (err)
 		return err;
 
-- 
2.28.0.163.g6104cc2f0b60

Re: [iproute2-next v2 5/5] devlink: support setting the overwrite mask

[David Ahern]

On 7/31/20 6:21 PM, Jacob Keller wrote:
> Add support for specifying the overwrite sections to allow in the flash
> update command. This is done by adding a new "overwrite" option which
> can take either "settings" or "identifiers" passing the overwrite mode
> multiple times will combine the fields using bitwise-OR.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  devlink/devlink.c | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 

5/5? I only see 2 - 4/5 and 5/5. Please re-send against latest
iproute2-next.

Re: [iproute2-next v2 5/5] devlink: support setting the overwrite mask

[Jiri Pirko]

Mon, Aug 03, 2020 at 05:53:16PM CEST, dsahern@gmail.com wrote:
>On 7/31/20 6:21 PM, Jacob Keller wrote:
>> Add support for specifying the overwrite sections to allow in the flash
>> update command. This is done by adding a new "overwrite" option which
>> can take either "settings" or "identifiers" passing the overwrite mode
>> multiple times will combine the fields using bitwise-OR.
>> 
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  devlink/devlink.c | 37 +++++++++++++++++++++++++++++++++++--
>>  1 file changed, 35 insertions(+), 2 deletions(-)
>> 
>
>5/5? I only see 2 - 4/5 and 5/5. Please re-send against latest
>iproute2-next.

1-3 are kernel.

>

Re: [iproute2-next v2 5/5] devlink: support setting the overwrite mask

[Jacob Keller]

On 8/3/2020 8:53 AM, David Ahern wrote:
> On 7/31/20 6:21 PM, Jacob Keller wrote:
>> Add support for specifying the overwrite sections to allow in the flash
>> update command. This is done by adding a new "overwrite" option which
>> can take either "settings" or "identifiers" passing the overwrite mode
>> multiple times will combine the fields using bitwise-OR.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  devlink/devlink.c | 37 +++++++++++++++++++++++++++++++++++--
>>  1 file changed, 35 insertions(+), 2 deletions(-)
>>
> 
> 5/5? I only see 2 - 4/5 and 5/5. Please re-send against latest
> iproute2-next.
> 

Sorry for the confusion here. I sent both the iproute2 and net-next
changes to implement it in the kernel.

Re: [iproute2-next v2 5/5] devlink: support setting the overwrite mask

[David Ahern]

On 8/3/20 10:56 AM, Jacob Keller wrote:
> Sorry for the confusion here. I sent both the iproute2 and net-next
> changes to implement it in the kernel.

please re-send the iproute2 patch; I already marked it in patchworks. I
get sending the patches in 1 go, but kernel and iproute2 patch numbering
should be separate.

Re: [iproute2-next v2 5/5] devlink: support setting the overwrite mask

[Jacob Keller]

On 8/3/2020 2:20 PM, David Ahern wrote:
> On 8/3/20 10:56 AM, Jacob Keller wrote:
>> Sorry for the confusion here. I sent both the iproute2 and net-next
>> changes to implement it in the kernel.
> 
> please re-send the iproute2 patch; I already marked it in patchworks. I
> get sending the patches in 1 go, but kernel and iproute2 patch numbering
> should be separate.
> 

Yep, I'll do that in the future, an will be re-sending a v2 shortly with
changes on the kernel side from Jiri's review.

Thanks,
Jake

Re: [iproute2-next v2 5/5] devlink: support setting the overwrite mask

[Jacob Keller]

On 8/3/2020 8:53 AM, David Ahern wrote:
> On 7/31/20 6:21 PM, Jacob Keller wrote:
>> Add support for specifying the overwrite sections to allow in the flash
>> update command. This is done by adding a new "overwrite" option which
>> can take either "settings" or "identifiers" passing the overwrite mode
>> multiple times will combine the fields using bitwise-OR.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  devlink/devlink.c | 37 +++++++++++++++++++++++++++++++++++--
>>  1 file changed, 35 insertions(+), 2 deletions(-)
>>
> 
> 5/5? I only see 2 - 4/5 and 5/5. Please re-send against latest
> iproute2-next.
> 

Slightly unrelated: but the recent change to using a bitfield32 results
in a "GENMASK is undefined".. I'm not sure what the proper way to fix
this is, since we'd like to still use GENMASK to define the supported
bitfields. I guess we need to pull in more headers? Or define something
in include/utils.h?

Thanks,
Jake

Re: [iproute2-next v2 5/5] devlink: support setting the overwrite mask

[David Ahern]

On 8/3/20 5:30 PM, Jacob Keller wrote:
> 
> Slightly unrelated: but the recent change to using a bitfield32 results
> in a "GENMASK is undefined".. I'm not sure what the proper way to fix
> this is, since we'd like to still use GENMASK to define the supported
> bitfields. I guess we need to pull in more headers? Or define something
> in include/utils.h?
> 

I see that include/linux/bits.h has been pulled into the tools directory
for perf and power tools (ie., works fine in userspace).

iproute2 is GPL so should be good from a licensing perspective to copy
into iproute2. Stephen: any objections?

Re: [iproute2-next v2 5/5] devlink: support setting the overwrite mask

[Jacob Keller]

On 8/3/2020 4:54 PM, David Ahern wrote:
> On 8/3/20 5:30 PM, Jacob Keller wrote:
>>
>> Slightly unrelated: but the recent change to using a bitfield32 results
>> in a "GENMASK is undefined".. I'm not sure what the proper way to fix
>> this is, since we'd like to still use GENMASK to define the supported
>> bitfields. I guess we need to pull in more headers? Or define something
>> in include/utils.h?
>>
> 
> I see that include/linux/bits.h has been pulled into the tools directory
> for perf and power tools (ie., works fine in userspace).
> 
> iproute2 is GPL so should be good from a licensing perspective to copy
> into iproute2. Stephen: any objections?
> 


Hmm... Actually, no other uapi header uses GENMASK.. Perhaps its better
to just avoid using it in the uapi/linux/devlink.h header...

Re: [net-next v2 0/5] devlink flash update overwrite mask

[Jiri Pirko]

Sat, Aug 01, 2020 at 02:21:54AM CEST, jacob.e.keller@intel.com wrote:
>This series introduces support for a new attribute to the flash update
>command: DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK. This attribute is a u32
>value that represents a bitmask of which subsections of flash to
>request/allow overwriting when performing a flash update.
>
>The intent is to support the ability to control overwriting options of the
>ice hardware flash update. Specifically, the ice flash components combine
>settings and identifiers within the firmware flash section. This series
>introduces the two subsections, "identifiers" and "settings". With the new
>attribute, users can request to overwrite these subsections when performing
>a flash update. By existing convention, it is assumed that flash program
>binaries are always updated (and thus overwritten), and no mask bit is
>provided to control this.
>
>First, the .flash_update command is modified to take a parameter structure.
>A new supported_flash_update_params field is also provided to allow drivers
>to opt-in to the parameters they support rather than opting out. This is
>similar to the recently added supported_coalesc_params field in ethtool.
>
>Following this, the new overwrite mask parameter is added, along with the
>associated supported bit. The netdevsim driver is updated to support this
>parameter, along with a few self tests to help verify the interface is
>working as expected.
>
>Finally, the ice driver is modified to support the parameter, converting it
>into the firmware preservation level request.
>
>Patches to enable support for specifying the overwrite sections are also
>provided for iproute2-next. This is done primarily in order to enable the
>tests for netdevsim. As discussed previously on the list, the primary
>motivations for the overwrite mode are two-fold.
>
>First, supporting update with a customized image that has pre-configured
>settings and identifiers, used with overwrite of both settings and
>identifiers. This enables an initial update to overwrite default values and
>customize the adapter with a new serial ID and fresh settings. Second, it
>may sometimes be useful to allow overwriting of settings when updating in
>order to guarantee that the settings in the flash section are "known good".


I'm missing examples in the cover letter. It is much easier to
understand the nature of the patchset with examples. Could you please
repost with them?

Thanks!



>
>Changes since v1
>* Added supported_flash_update_params field, removing some boilerplate in
>  each driver. This also makes it easier to add new parameters in the future
>  without fear of accidentally breaking an existing driver, due to opt-in
>  behavior instead of forcing drivers to opt-out.
>* Split the ice changes to a separate patch.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Cc: Jakub Kicinski <kuba@kernel.org>
>Cc: Jonathan Corbet <corbet@lwn.net>
>Cc: Michael Chan <michael.chan@broadcom.com>
>Cc: Bin Luo <luobin9@huawei.com>
>Cc: Saeed Mahameed <saeedm@mellanox.com>
>Cc: Leon Romanovsky <leon@kernel.org>
>Cc: Ido Schimmel <idosch@mellanox.com>
>Cc: Danielle Ratson <danieller@mellanox.com>
>
>Jacob Keller (3):
>  devlink: convert flash_update to use params structure
>  devlink: introduce flash update overwrite mask
>  ice: add support for flash update overwrite mask
>
> .../networking/devlink/devlink-flash.rst      | 29 ++++++++++++++
> Documentation/networking/devlink/ice.rst      | 31 +++++++++++++++
> .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 19 ++++-----
> .../net/ethernet/huawei/hinic/hinic_devlink.c |  8 +---
> drivers/net/ethernet/intel/ice/ice_devlink.c  | 33 +++++++++++-----
> .../net/ethernet/intel/ice/ice_fw_update.c    | 16 +++++++-
> .../net/ethernet/intel/ice/ice_fw_update.h    |  2 +-
> .../net/ethernet/mellanox/mlx5/core/devlink.c |  8 +---
> drivers/net/ethernet/mellanox/mlxsw/core.c    |  6 +--
> drivers/net/ethernet/mellanox/mlxsw/core.h    |  2 +-
> .../net/ethernet/mellanox/mlxsw/spectrum.c    |  7 +---
> .../net/ethernet/netronome/nfp/nfp_devlink.c  |  9 ++---
> drivers/net/netdevsim/dev.c                   | 21 +++++++---
> drivers/net/netdevsim/netdevsim.h             |  1 +
> include/net/devlink.h                         | 35 ++++++++++++++++-
> include/uapi/linux/devlink.h                  | 24 ++++++++++++
> net/core/devlink.c                            | 39 +++++++++++++++----
> .../drivers/net/netdevsim/devlink.sh          | 21 ++++++++++
> 18 files changed, 244 insertions(+), 67 deletions(-)
>
>Jacob Keller (2):
>  Update devlink header for overwrite mask attribute
>  devlink: support setting the overwrite mask
>
> devlink/devlink.c            | 37 ++++++++++++++++++++++++++++++++++--
> include/uapi/linux/devlink.h | 24 +++++++++++++++++++++++
> 2 files changed, 59 insertions(+), 2 deletions(-)
>
>base-commit: bd69058f50d5ffa659423bcfa6fe6280ce9c760a
>-- 
>2.28.0.163.g6104cc2f0b60
>

Re: [net-next v2 0/5] devlink flash update overwrite mask

[Jacob Keller]

On 8/3/2020 8:28 AM, Jiri Pirko wrote:

> 
> I'm missing examples in the cover letter. It is much easier to
> understand the nature of the patchset with examples. Could you please
> repost with them?
> 
> Thanks!
> 


I'm not sure what you're asking for here. If by example of the command
line interface in devlink, there are some in the tests for netdevsim
which I could copy to the cover letter I guess?