diff mbox series

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

Message ID 20200801002159.3300425-3-jacob.e.keller@intel.com
State Changes Requested
Delegated to: David Miller
Headers show
Series devlink flash update overwrite mask | expand

Commit Message

Keller, Jacob E Aug. 1, 2020, 12:21 a.m. UTC 
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(-)

Comments

Jiri Pirko Aug. 3, 2020, 3:38 p.m. UTC | #1 
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.
Keller, Jacob E Aug. 3, 2020, 4:53 p.m. UTC | #2 
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.
Keller, Jacob E Aug. 3, 2020, 11:08 p.m. UTC | #3 
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
diff mbox series

Patch

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"