[patch net-next 0/4] net: devlink: sync flash and dev info commands

[patch net-next 0/4] net: devlink: sync flash and dev info commands

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Purpose of this patchset is to introduce consistency between two devlink
commands:
  devlink dev info
    Shows versions of running default flash target and components.
  devlink dev flash
    Flashes default flash target or component name (if specified
    on cmdline).

Currently it is up to the driver what versions to expose and what flash
update component names to accept. This is inconsistent. Thankfully, only
netdevsim is currently using components, so it is a good time
to sanitize this.

This patchset makes sure, that devlink.c calls into driver for
component flash update only in case the driver exposes the same version
name.

Also there are two flags exposed to the use over netlink for versions:

1) if driver considers the version represents flashable component,
   DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT is set.
   This provides a list of component names for the user.

2) if driver considers the version represents default flash target (w/o
   component name specified)
   DEVLINK_ATTR_INFO_VERSION_IS_FLASH_UPDATE_DEFAULT is set.
   This tells the user which version is going to be affected by flash
   command when no component name is passed.

Example:
$ devlink dev info
netdevsim/netdevsim10:
  driver netdevsim
  versions:
      running:
        fw.mgmt 10.20.30
        fw 11.22.33
      flash_components:
        fw.mgmt
    flash_update_default fw
$ devlink dev flash netdevsim/netdevsim10 file somefile.bin
[fw.mgmt] Preparing to flash
[fw.mgmt] Flashing 100%
[fw.mgmt] Flash select
[fw.mgmt] Flashing done
$ devlink dev flash netdevsim/netdevsim10 file somefile.bin component fw.mgmt
[fw.mgmt] Preparing to flash
[fw.mgmt] Flashing 100%
[fw.mgmt] Flash select
[fw.mgmt] Flashing done
$ devlink dev flash netdevsim/netdevsim10 file somefile.bin component dummy
Error: selected component is not supported by this device.

Jiri Pirko (4):
  net: devlink: extend info_get() version put to indicate a flash
    component
  net: devlink: expose the info about version representing a component
  netdevsim: expose version of default flash target
  net: devlink: expose default flash update target

 drivers/net/netdevsim/dev.c  |  17 +++-
 include/net/devlink.h        |  18 ++++-
 include/uapi/linux/devlink.h |   3 +
 net/core/devlink.c           | 145 ++++++++++++++++++++++++++++++-----
 4 files changed, 157 insertions(+), 26 deletions(-)

-- 
2.37.1

[patch net-next 1/4] net: devlink: extend info_get() version put to indicate a flash component

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Limit the acceptance of component name passed to cmd_flash_update() to
match one of the versions returned by info_get(), marked by version type.
This makes things clearer and enforces 1:1 mapping between exposed
version and accepted flash component.

Whenever the driver is called by his info_get() op, it may put multiple
version names and values to the netlink message. Extend by additional
helper devlink_info_version_running_put_ext() that allows to specify a
version type that indicates when particular version name represents
a flash component.

Use this indication during cmd_flash_update() execution by calling
info_get() with different "req" context. That causes info_get() to
lookup the component name instead of filling-up the netlink message.

Fix the only component user which is netdevsim. It uses component named
"fw.mgmt" in selftests.

Remove now outdated "UPDATE_COMPONENT" flag.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/netdevsim/dev.c |  12 +++-
 include/net/devlink.h       |  15 ++++-
 net/core/devlink.c          | 128 ++++++++++++++++++++++++++++++------
 3 files changed, 129 insertions(+), 26 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index e88f783c297e..cea130490dea 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -984,7 +984,14 @@ static int nsim_dev_info_get(struct devlink *devlink,
 			     struct devlink_info_req *req,
 			     struct netlink_ext_ack *extack)
 {
-	return devlink_info_driver_name_put(req, DRV_NAME);
+	int err;
+
+	err = devlink_info_driver_name_put(req, DRV_NAME);
+	if (err)
+		return err;
+
+	return devlink_info_version_running_put_ext(req, "fw.mgmt", "10.20.30",
+						    DEVLINK_INFO_VERSION_TYPE_COMPONENT);
 }
 
 #define NSIM_DEV_FLASH_SIZE 500000
@@ -1312,8 +1319,7 @@ nsim_dev_devlink_trap_drop_counter_get(struct devlink *devlink,
 static const struct devlink_ops nsim_dev_devlink_ops = {
 	.eswitch_mode_set = nsim_devlink_eswitch_mode_set,
 	.eswitch_mode_get = nsim_devlink_eswitch_mode_get,
-	.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT |
-					 DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
+	.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
 	.reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT),
 	.reload_down = nsim_dev_reload_down,
 	.reload_up = nsim_dev_reload_up,
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 119ed1ffb988..9bf4f03feca6 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -624,8 +624,7 @@ struct devlink_flash_update_params {
 	u32 overwrite_mask;
 };
 
-#define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT		BIT(0)
-#define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK	BIT(1)
+#define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK	BIT(0)
 
 struct devlink_region;
 struct devlink_info_req;
@@ -1714,6 +1713,14 @@ int devlink_info_driver_name_put(struct devlink_info_req *req,
 				 const char *name);
 int devlink_info_board_serial_number_put(struct devlink_info_req *req,
 					 const char *bsn);
+
+enum devlink_info_version_type {
+	DEVLINK_INFO_VERSION_TYPE_NONE,
+	DEVLINK_INFO_VERSION_TYPE_COMPONENT, /* May be used as flash update
+					      * component by name.
+					      */
+};
+
 int devlink_info_version_fixed_put(struct devlink_info_req *req,
 				   const char *version_name,
 				   const char *version_value);
@@ -1723,6 +1730,10 @@ int devlink_info_version_stored_put(struct devlink_info_req *req,
 int devlink_info_version_running_put(struct devlink_info_req *req,
 				     const char *version_name,
 				     const char *version_value);
+int devlink_info_version_running_put_ext(struct devlink_info_req *req,
+					 const char *version_name,
+					 const char *version_value,
+					 enum devlink_info_version_type version_type);
 
 int devlink_fmsg_obj_nest_start(struct devlink_fmsg *fmsg);
 int devlink_fmsg_obj_nest_end(struct devlink_fmsg *fmsg);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index b50bcc18b8d9..17b78123ad9d 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4742,10 +4742,76 @@ void devlink_flash_update_timeout_notify(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
 
+struct devlink_info_req {
+	struct sk_buff *msg;
+	void (*version_cb)(const char *version_name,
+			   enum devlink_info_version_type version_type,
+			   void *version_cb_priv);
+	void *version_cb_priv;
+};
+
+struct devlink_flash_component_lookup_ctx {
+	const char *lookup_name;
+	bool lookup_name_found;
+};
+
+static void
+devlink_flash_component_lookup_cb(const char *version_name,
+				  enum devlink_info_version_type version_type,
+				  void *version_cb_priv)
+{
+	struct devlink_flash_component_lookup_ctx *lookup_ctx = version_cb_priv;
+
+	if (version_type != DEVLINK_INFO_VERSION_TYPE_COMPONENT ||
+	    lookup_ctx->lookup_name_found)
+		return;
+
+	lookup_ctx->lookup_name_found =
+		!strcmp(lookup_ctx->lookup_name, version_name);
+}
+
+static int devlink_flash_component_get(struct devlink *devlink,
+				       struct nlattr *nla_component,
+				       const char **p_component,
+				       struct netlink_ext_ack *extack)
+{
+	struct devlink_flash_component_lookup_ctx lookup_ctx = {};
+	struct devlink_info_req req = {};
+	const char *component;
+	int ret;
+
+	if (!nla_component)
+		return 0;
+
+	component = nla_data(nla_component);
+
+	if (!devlink->ops->info_get) {
+		NL_SET_ERR_MSG_ATTR(extack, nla_component,
+				    "component update is not supported by this device");
+		return -EOPNOTSUPP;
+	}
+
+	lookup_ctx.lookup_name = component;
+	req.version_cb = devlink_flash_component_lookup_cb;
+	req.version_cb_priv = &lookup_ctx;
+
+	ret = devlink->ops->info_get(devlink, &req, NULL);
+	if (ret)
+		return ret;
+
+	if (!lookup_ctx.lookup_name_found) {
+		NL_SET_ERR_MSG_ATTR(extack, nla_component,
+				    "selected component is not supported by this device");
+		return -EINVAL;
+	}
+	*p_component = component;
+	return 0;
+}
+
 static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 				       struct genl_info *info)
 {
-	struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name;
+	struct nlattr *nla_overwrite_mask, *nla_file_name;
 	struct devlink_flash_update_params params = {};
 	struct devlink *devlink = info->user_ptr[0];
 	const char *file_name;
@@ -4758,17 +4824,13 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
 		return -EINVAL;
 
-	supported_params = devlink->ops->supported_flash_update_params;
+	ret = devlink_flash_component_get(devlink,
+					  info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT],
+					  &params.component, info->extack);
+	if (ret)
+		return ret;
 
-	nla_component = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
-	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 by this device");
-			return -EOPNOTSUPP;
-		}
-		params.component = nla_data(nla_component);
-	}
+	supported_params = devlink->ops->supported_flash_update_params;
 
 	nla_overwrite_mask = info->attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK];
 	if (nla_overwrite_mask) {
@@ -6553,18 +6615,18 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	return err;
 }
 
-struct devlink_info_req {
-	struct sk_buff *msg;
-};
-
 int devlink_info_driver_name_put(struct devlink_info_req *req, const char *name)
 {
+	if (!req->msg)
+		return 0;
 	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_DRIVER_NAME, name);
 }
 EXPORT_SYMBOL_GPL(devlink_info_driver_name_put);
 
 int devlink_info_serial_number_put(struct devlink_info_req *req, const char *sn)
 {
+	if (!req->msg)
+		return 0;
 	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER, sn);
 }
 EXPORT_SYMBOL_GPL(devlink_info_serial_number_put);
@@ -6572,6 +6634,8 @@ EXPORT_SYMBOL_GPL(devlink_info_serial_number_put);
 int devlink_info_board_serial_number_put(struct devlink_info_req *req,
 					 const char *bsn)
 {
+	if (!req->msg)
+		return 0;
 	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_BOARD_SERIAL_NUMBER,
 			      bsn);
 }
@@ -6579,11 +6643,19 @@ EXPORT_SYMBOL_GPL(devlink_info_board_serial_number_put);
 
 static int devlink_info_version_put(struct devlink_info_req *req, int attr,
 				    const char *version_name,
-				    const char *version_value)
+				    const char *version_value,
+				    enum devlink_info_version_type version_type)
 {
 	struct nlattr *nest;
 	int err;
 
+	if (req->version_cb)
+		req->version_cb(version_name, version_type,
+				req->version_cb_priv);
+
+	if (!req->msg)
+		return 0;
+
 	nest = nla_nest_start_noflag(req->msg, attr);
 	if (!nest)
 		return -EMSGSIZE;
@@ -6612,7 +6684,8 @@ int devlink_info_version_fixed_put(struct devlink_info_req *req,
 				   const char *version_value)
 {
 	return devlink_info_version_put(req, DEVLINK_ATTR_INFO_VERSION_FIXED,
-					version_name, version_value);
+					version_name, version_value,
+					DEVLINK_INFO_VERSION_TYPE_NONE);
 }
 EXPORT_SYMBOL_GPL(devlink_info_version_fixed_put);
 
@@ -6621,7 +6694,8 @@ int devlink_info_version_stored_put(struct devlink_info_req *req,
 				    const char *version_value)
 {
 	return devlink_info_version_put(req, DEVLINK_ATTR_INFO_VERSION_STORED,
-					version_name, version_value);
+					version_name, version_value,
+					DEVLINK_INFO_VERSION_TYPE_NONE);
 }
 EXPORT_SYMBOL_GPL(devlink_info_version_stored_put);
 
@@ -6630,16 +6704,28 @@ int devlink_info_version_running_put(struct devlink_info_req *req,
 				     const char *version_value)
 {
 	return devlink_info_version_put(req, DEVLINK_ATTR_INFO_VERSION_RUNNING,
-					version_name, version_value);
+					version_name, version_value,
+					DEVLINK_INFO_VERSION_TYPE_NONE);
 }
 EXPORT_SYMBOL_GPL(devlink_info_version_running_put);
 
+int devlink_info_version_running_put_ext(struct devlink_info_req *req,
+					 const char *version_name,
+					 const char *version_value,
+					 enum devlink_info_version_type version_type)
+{
+	return devlink_info_version_put(req, DEVLINK_ATTR_INFO_VERSION_RUNNING,
+					version_name, version_value,
+					version_type);
+}
+EXPORT_SYMBOL_GPL(devlink_info_version_running_put_ext);
+
 static int
 devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink,
 		     enum devlink_command cmd, u32 portid,
 		     u32 seq, int flags, struct netlink_ext_ack *extack)
 {
-	struct devlink_info_req req;
+	struct devlink_info_req req = {};
 	void *hdr;
 	int err;
 
@@ -12306,8 +12392,8 @@ EXPORT_SYMBOL_GPL(devl_trap_policers_unregister);
 static void __devlink_compat_running_version(struct devlink *devlink,
 					     char *buf, size_t len)
 {
+	struct devlink_info_req req = {};
 	const struct nlattr *nlattr;
-	struct devlink_info_req req;
 	struct sk_buff *msg;
 	int rem, err;
 
-- 
2.37.1

[patch net-next 2/4] net: devlink: expose the info about version representing a component

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

If certain version exposed by a driver is marked to be representing a
component, expose this info to the user.

Example:
$ devlink dev info
netdevsim/netdevsim10:
  driver netdevsim
  versions:
      running:
        fw.mgmt 10.20.30
      flash_components:
        fw.mgmt

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/uapi/linux/devlink.h | 2 ++
 net/core/devlink.c           | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 2f24b53a87a5..7f2874189188 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -607,6 +607,8 @@ enum devlink_attr {
 
 	DEVLINK_ATTR_SELFTESTS,			/* nested */
 
+	DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT,	/* u8 0 or 1 */
+
 	/* 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 17b78123ad9d..23a5fd92ecaa 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6670,6 +6670,11 @@ static int devlink_info_version_put(struct devlink_info_req *req, int attr,
 	if (err)
 		goto nla_put_failure;
 
+	err = nla_put_u8(req->msg, DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT,
+			 version_type == DEVLINK_INFO_VERSION_TYPE_COMPONENT);
+	if (err)
+		goto nla_put_failure;
+
 	nla_nest_end(req->msg, nest);
 
 	return 0;
-- 
2.37.1

[patch net-next 3/4] netdevsim: expose version of default flash target

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Add version named "fw" to represent version of default flash target.

Example:

$ devlink dev info
netdevsim/netdevsim10:
  driver netdevsim
  versions:
      running:
        fw.mgmt 10.20.30
        fw 11.22.33
      flash_components:
        fw.mgmt

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/netdevsim/dev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index cea130490dea..97281b6aa41f 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -990,8 +990,12 @@ static int nsim_dev_info_get(struct devlink *devlink,
 	if (err)
 		return err;
 
-	return devlink_info_version_running_put_ext(req, "fw.mgmt", "10.20.30",
-						    DEVLINK_INFO_VERSION_TYPE_COMPONENT);
+	err = devlink_info_version_running_put_ext(req, "fw.mgmt", "10.20.30",
+						   DEVLINK_INFO_VERSION_TYPE_COMPONENT);
+	if (err)
+		return err;
+
+	return devlink_info_version_running_put(req, "fw", "11.22.33");
 }
 
 #define NSIM_DEV_FLASH_SIZE 500000
-- 
2.37.1

[patch net-next 4/4] net: devlink: expose default flash update target

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Allow driver to mark certain version obtained by info_get() op as
"flash update default". Expose this information to user which allows him
to understand what version is going to be affected if he does flash
update without specifying the component. Implement this in netdevsim.

Example:

$ devlink dev info
netdevsim/netdevsim10:
  driver netdevsim
versions:
  running:
    fw.mgmt 10.20.30
    fw 11.22.33
    flash_components:
      fw.mgmt
  flash_update_default fw

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/netdevsim/dev.c  |  3 ++-
 include/net/devlink.h        |  3 +++
 include/uapi/linux/devlink.h |  1 +
 net/core/devlink.c           | 12 ++++++++++++
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 97281b6aa41f..f999b9477a26 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -995,7 +995,8 @@ static int nsim_dev_info_get(struct devlink *devlink,
 	if (err)
 		return err;
 
-	return devlink_info_version_running_put(req, "fw", "11.22.33");
+	return devlink_info_version_running_put_ext(req, "fw", "11.22.33",
+						    DEVLINK_INFO_VERSION_TYPE_FLASH_UPDATE_DEFAULT);
 }
 
 #define NSIM_DEV_FLASH_SIZE 500000
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 9bf4f03feca6..bfe988a56067 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1719,6 +1719,9 @@ enum devlink_info_version_type {
 	DEVLINK_INFO_VERSION_TYPE_COMPONENT, /* May be used as flash update
 					      * component by name.
 					      */
+	DEVLINK_INFO_VERSION_TYPE_FLASH_UPDATE_DEFAULT, /* Is default flash
+							 * update target.
+							 */
 };
 
 int devlink_info_version_fixed_put(struct devlink_info_req *req,
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 7f2874189188..01e348177f60 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -608,6 +608,7 @@ enum devlink_attr {
 	DEVLINK_ATTR_SELFTESTS,			/* nested */
 
 	DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT,	/* u8 0 or 1 */
+	DEVLINK_ATTR_INFO_VERSION_IS_FLASH_UPDATE_DEFAULT,	/* u8 0 or 1 */
 
 	/* add new attributes above here, update the policy in devlink.c */
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 23a5fd92ecaa..de583fb2ed12 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4748,6 +4748,7 @@ struct devlink_info_req {
 			   enum devlink_info_version_type version_type,
 			   void *version_cb_priv);
 	void *version_cb_priv;
+	unsigned int flash_update_default_count;
 };
 
 struct devlink_flash_component_lookup_ctx {
@@ -6656,6 +6657,12 @@ static int devlink_info_version_put(struct devlink_info_req *req, int attr,
 	if (!req->msg)
 		return 0;
 
+	if (version_type == DEVLINK_INFO_VERSION_TYPE_FLASH_UPDATE_DEFAULT) {
+		if (WARN_ON(req->flash_update_default_count++))
+			/* Max one flash update default is allowed. */
+			return -EINVAL;
+	}
+
 	nest = nla_nest_start_noflag(req->msg, attr);
 	if (!nest)
 		return -EMSGSIZE;
@@ -6675,6 +6682,11 @@ static int devlink_info_version_put(struct devlink_info_req *req, int attr,
 	if (err)
 		goto nla_put_failure;
 
+	err = nla_put_u8(req->msg, DEVLINK_ATTR_INFO_VERSION_IS_FLASH_UPDATE_DEFAULT,
+			 version_type == DEVLINK_INFO_VERSION_TYPE_FLASH_UPDATE_DEFAULT);
+	if (err)
+		goto nla_put_failure;
+
 	nla_nest_end(req->msg, nest);
 
 	return 0;
-- 
2.37.1

RE: [patch net-next 0/4] net: devlink: sync flash and dev info commands

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Thursday, August 18, 2022 6:01 AM
> To: netdev@vger.kernel.org
> Cc: davem@davemloft.net; kuba@kernel.org; idosch@nvidia.com;
> pabeni@redhat.com; edumazet@google.com; saeedm@nvidia.com; Keller, Jacob
> E <jacob.e.keller@intel.com>; vikas.gupta@broadcom.com;
> gospo@broadcom.com
> Subject: [patch net-next 0/4] net: devlink: sync flash and dev info commands
> 
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Purpose of this patchset is to introduce consistency between two devlink
> commands:
>   devlink dev info
>     Shows versions of running default flash target and components.
>   devlink dev flash
>     Flashes default flash target or component name (if specified
>     on cmdline).
> 
> Currently it is up to the driver what versions to expose and what flash
> update component names to accept. This is inconsistent. Thankfully, only
> netdevsim is currently using components, so it is a good time
> to sanitize this.
> 
> This patchset makes sure, that devlink.c calls into driver for
> component flash update only in case the driver exposes the same version
> name.

Makes sense.

> 
> Also there are two flags exposed to the use over netlink for versions:
> 
> 1) if driver considers the version represents flashable component,
>    DEVLINK_ATTR_INFO_VERSION_IS_COMPONENT is set.
>    This provides a list of component names for the user.
> 
> 2) if driver considers the version represents default flash target (w/o
>    component name specified)
>    DEVLINK_ATTR_INFO_VERSION_IS_FLASH_UPDATE_DEFAULT is set.
>    This tells the user which version is going to be affected by flash
>    command when no component name is passed.
> 

This is great. I've been meaning to get around to adding single component update to the ice driver, so this would be a good time to do so along with implementing support for this.

> Example:
> $ devlink dev info
> netdevsim/netdevsim10:
>   driver netdevsim
>   versions:
>       running:
>         fw.mgmt 10.20.30
>         fw 11.22.33
>       flash_components:
>         fw.mgmt
>     flash_update_default fw
> $ devlink dev flash netdevsim/netdevsim10 file somefile.bin
> [fw.mgmt] Preparing to flash
> [fw.mgmt] Flashing 100%
> [fw.mgmt] Flash select
> [fw.mgmt] Flashing done
> $ devlink dev flash netdevsim/netdevsim10 file somefile.bin component fw.mgmt
> [fw.mgmt] Preparing to flash
> [fw.mgmt] Flashing 100%
> [fw.mgmt] Flash select
> [fw.mgmt] Flashing done
> $ devlink dev flash netdevsim/netdevsim10 file somefile.bin component dummy
> Error: selected component is not supported by this device.
> 
> Jiri Pirko (4):
>   net: devlink: extend info_get() version put to indicate a flash
>     component
>   net: devlink: expose the info about version representing a component
>   netdevsim: expose version of default flash target
>   net: devlink: expose default flash update target
> 
>  drivers/net/netdevsim/dev.c  |  17 +++-
>  include/net/devlink.h        |  18 ++++-
>  include/uapi/linux/devlink.h |   3 +
>  net/core/devlink.c           | 145 ++++++++++++++++++++++++++++++-----
>  4 files changed, 157 insertions(+), 26 deletions(-)
> 
> --
> 2.37.1

RE: [patch net-next 1/4] net: devlink: extend info_get() version put to indicate a flash component

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Thursday, August 18, 2022 6:01 AM
> To: netdev@vger.kernel.org
> Cc: davem@davemloft.net; kuba@kernel.org; idosch@nvidia.com;
> pabeni@redhat.com; edumazet@google.com; saeedm@nvidia.com; Keller, Jacob
> E <jacob.e.keller@intel.com>; vikas.gupta@broadcom.com;
> gospo@broadcom.com
> Subject: [patch net-next 1/4] net: devlink: extend info_get() version put to
> indicate a flash component
> 
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Limit the acceptance of component name passed to cmd_flash_update() to
> match one of the versions returned by info_get(), marked by version type.
> This makes things clearer and enforces 1:1 mapping between exposed
> version and accepted flash component.
> 

I feel like this commit does quite a bit and could be separated into one part that only adds the components flagging in info and another which uses this in the cmd_flash_update.

> Whenever the driver is called by his info_get() op, it may put multiple
> version names and values to the netlink message. Extend by additional
> helper devlink_info_version_running_put_ext() that allows to specify a
> version type that indicates when particular version name represents
> a flash component.
> 
> Use this indication during cmd_flash_update() execution by calling
> info_get() with different "req" context. That causes info_get() to
> lookup the component name instead of filling-up the netlink message.
> 
> Fix the only component user which is netdevsim. It uses component named
> "fw.mgmt" in selftests.
> 
> Remove now outdated "UPDATE_COMPONENT" flag.

Is this flag outdated? I believe we're supposed to use this to indicate when a driver supports per-component update, which we would do once another driver actually uses the interface? I guess now instead we just check to see if any of the flash fields have the component flag set instead? Ok
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>


Code looks ok to me, but it would be easier to review if this was separated into one commit for addng the info changes, one for enforcing them, and one for updating netdevsim.

I'll try to find the patches I had for ice to implement this per-component update and specifying which components are default once this gets merged.

Thanks,
Jake

> ---
>  drivers/net/netdevsim/dev.c |  12 +++-
>  include/net/devlink.h       |  15 ++++-
>  net/core/devlink.c          | 128 ++++++++++++++++++++++++++++++------
>  3 files changed, 129 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index e88f783c297e..cea130490dea 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -984,7 +984,14 @@ static int nsim_dev_info_get(struct devlink *devlink,
>  			     struct devlink_info_req *req,
>  			     struct netlink_ext_ack *extack)
>  {
> -	return devlink_info_driver_name_put(req, DRV_NAME);
> +	int err;
> +
> +	err = devlink_info_driver_name_put(req, DRV_NAME);
> +	if (err)
> +		return err;
> +
> +	return devlink_info_version_running_put_ext(req, "fw.mgmt",
> "10.20.30",
> +
> DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>  }
> 
>  #define NSIM_DEV_FLASH_SIZE 500000
> @@ -1312,8 +1319,7 @@ nsim_dev_devlink_trap_drop_counter_get(struct
> devlink *devlink,
>  static const struct devlink_ops nsim_dev_devlink_ops = {
>  	.eswitch_mode_set = nsim_devlink_eswitch_mode_set,
>  	.eswitch_mode_get = nsim_devlink_eswitch_mode_get,
> -	.supported_flash_update_params =
> DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT |
> -
> DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
> +	.supported_flash_update_params =
> DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
>  	.reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT),
>  	.reload_down = nsim_dev_reload_down,
>  	.reload_up = nsim_dev_reload_up,
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 119ed1ffb988..9bf4f03feca6 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -624,8 +624,7 @@ struct devlink_flash_update_params {
>  	u32 overwrite_mask;
>  };
> 
> -#define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT		BIT(0)
> -#define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK	BIT(1)
> +#define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK	BIT(0)
> 

This stays in the kernel so there's no issue with changing the bits. Ok.

>  struct devlink_region;
>  struct devlink_info_req;
> @@ -1714,6 +1713,14 @@ int devlink_info_driver_name_put(struct
> devlink_info_req *req,
>  				 const char *name);
>  int devlink_info_board_serial_number_put(struct devlink_info_req *req,
>  					 const char *bsn);
> +
> +enum devlink_info_version_type {
> +	DEVLINK_INFO_VERSION_TYPE_NONE,
> +	DEVLINK_INFO_VERSION_TYPE_COMPONENT, /* May be used as flash
> update
> +					      * component by name.
> +					      */
> +};
> +
>  int devlink_info_version_fixed_put(struct devlink_info_req *req,
>  				   const char *version_name,
>  				   const char *version_value);
> @@ -1723,6 +1730,10 @@ int devlink_info_version_stored_put(struct
> devlink_info_req *req,
>  int devlink_info_version_running_put(struct devlink_info_req *req,
>  				     const char *version_name,
>  				     const char *version_value);
> +int devlink_info_version_running_put_ext(struct devlink_info_req *req,
> +					 const char *version_name,
> +					 const char *version_value,
> +					 enum devlink_info_version_type
> version_type);
> 
>  int devlink_fmsg_obj_nest_start(struct devlink_fmsg *fmsg);
>  int devlink_fmsg_obj_nest_end(struct devlink_fmsg *fmsg);
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index b50bcc18b8d9..17b78123ad9d 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4742,10 +4742,76 @@ void devlink_flash_update_timeout_notify(struct
> devlink *devlink,
>  }
>  EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
> 
> +struct devlink_info_req {
> +	struct sk_buff *msg;
> +	void (*version_cb)(const char *version_name,
> +			   enum devlink_info_version_type version_type,
> +			   void *version_cb_priv);
> +	void *version_cb_priv;
> +};
> +
> +struct devlink_flash_component_lookup_ctx {
> +	const char *lookup_name;
> +	bool lookup_name_found;
> +};
> +
> +static void
> +devlink_flash_component_lookup_cb(const char *version_name,
> +				  enum devlink_info_version_type version_type,
> +				  void *version_cb_priv)
> +{
> +	struct devlink_flash_component_lookup_ctx *lookup_ctx =
> version_cb_priv;
> +
> +	if (version_type != DEVLINK_INFO_VERSION_TYPE_COMPONENT ||
> +	    lookup_ctx->lookup_name_found)
> +		return;
> +
> +	lookup_ctx->lookup_name_found =
> +		!strcmp(lookup_ctx->lookup_name, version_name);
> +}
> +
> +static int devlink_flash_component_get(struct devlink *devlink,
> +				       struct nlattr *nla_component,
> +				       const char **p_component,
> +				       struct netlink_ext_ack *extack)
> +{
> +	struct devlink_flash_component_lookup_ctx lookup_ctx = {};
> +	struct devlink_info_req req = {};
> +	const char *component;
> +	int ret;
> +
> +	if (!nla_component)
> +		return 0;
> +
> +	component = nla_data(nla_component);
> +
> +	if (!devlink->ops->info_get) {
> +		NL_SET_ERR_MSG_ATTR(extack, nla_component,
> +				    "component update is not supported by this
> device");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	lookup_ctx.lookup_name = component;
> +	req.version_cb = devlink_flash_component_lookup_cb;
> +	req.version_cb_priv = &lookup_ctx;
> +
> +	ret = devlink->ops->info_get(devlink, &req, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (!lookup_ctx.lookup_name_found) {
> +		NL_SET_ERR_MSG_ATTR(extack, nla_component,
> +				    "selected component is not supported by this
> device");
> +		return -EINVAL;
> +	}
> +	*p_component = component;
> +	return 0;
> +}
> +
>  static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
>  				       struct genl_info *info)
>  {
> -	struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name;
> +	struct nlattr *nla_overwrite_mask, *nla_file_name;
>  	struct devlink_flash_update_params params = {};
>  	struct devlink *devlink = info->user_ptr[0];
>  	const char *file_name;
> @@ -4758,17 +4824,13 @@ static int devlink_nl_cmd_flash_update(struct
> sk_buff *skb,
>  	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
>  		return -EINVAL;
> 
> -	supported_params = devlink->ops->supported_flash_update_params;
> +	ret = devlink_flash_component_get(devlink,
> +					  info-
> >attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT],
> +					  &params.component, info->extack);
> +	if (ret)
> +		return ret;
> 
> -	nla_component = info-
> >attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
> -	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
> by this device");
> -			return -EOPNOTSUPP;
> -		}
> -		params.component = nla_data(nla_component);
> -	}
> +	supported_params = devlink->ops->supported_flash_update_params;
> 
>  	nla_overwrite_mask = info-
> >attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK];
>  	if (nla_overwrite_mask) {
> @@ -6553,18 +6615,18 @@ static int
> devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>  	return err;
>  }
> 
> -struct devlink_info_req {
> -	struct sk_buff *msg;
> -};
> -
>  int devlink_info_driver_name_put(struct devlink_info_req *req, const char
> *name)
>  {
> +	if (!req->msg)
> +		return 0;
>  	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_DRIVER_NAME,
> name);
>  }
>  EXPORT_SYMBOL_GPL(devlink_info_driver_name_put);
> 
>  int devlink_info_serial_number_put(struct devlink_info_req *req, const char
> *sn)
>  {
> +	if (!req->msg)
> +		return 0;
>  	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER,
> sn);
>  }
>  EXPORT_SYMBOL_GPL(devlink_info_serial_number_put);
> @@ -6572,6 +6634,8 @@
> EXPORT_SYMBOL_GPL(devlink_info_serial_number_put);
>  int devlink_info_board_serial_number_put(struct devlink_info_req *req,
>  					 const char *bsn)
>  {
> +	if (!req->msg)
> +		return 0;
>  	return nla_put_string(req->msg,
> DEVLINK_ATTR_INFO_BOARD_SERIAL_NUMBER,
>  			      bsn);
>  }
> @@ -6579,11 +6643,19 @@
> EXPORT_SYMBOL_GPL(devlink_info_board_serial_number_put);
> 
>  static int devlink_info_version_put(struct devlink_info_req *req, int attr,
>  				    const char *version_name,
> -				    const char *version_value)
> +				    const char *version_value,
> +				    enum devlink_info_version_type version_type)
>  {
>  	struct nlattr *nest;
>  	int err;
> 
> +	if (req->version_cb)
> +		req->version_cb(version_name, version_type,
> +				req->version_cb_priv);
> +
> +	if (!req->msg)
> +		return 0;
> +
>  	nest = nla_nest_start_noflag(req->msg, attr);
>  	if (!nest)
>  		return -EMSGSIZE;
> @@ -6612,7 +6684,8 @@ int devlink_info_version_fixed_put(struct
> devlink_info_req *req,
>  				   const char *version_value)
>  {
>  	return devlink_info_version_put(req,
> DEVLINK_ATTR_INFO_VERSION_FIXED,
> -					version_name, version_value);
> +					version_name, version_value,
> +					DEVLINK_INFO_VERSION_TYPE_NONE);
>  }
>  EXPORT_SYMBOL_GPL(devlink_info_version_fixed_put);
> 
> @@ -6621,7 +6694,8 @@ int devlink_info_version_stored_put(struct
> devlink_info_req *req,
>  				    const char *version_value)
>  {
>  	return devlink_info_version_put(req,
> DEVLINK_ATTR_INFO_VERSION_STORED,
> -					version_name, version_value);
> +					version_name, version_value,
> +					DEVLINK_INFO_VERSION_TYPE_NONE);
>  }
>  EXPORT_SYMBOL_GPL(devlink_info_version_stored_put);
> 
> @@ -6630,16 +6704,28 @@ int devlink_info_version_running_put(struct
> devlink_info_req *req,
>  				     const char *version_value)
>  {
>  	return devlink_info_version_put(req,
> DEVLINK_ATTR_INFO_VERSION_RUNNING,
> -					version_name, version_value);
> +					version_name, version_value,
> +					DEVLINK_INFO_VERSION_TYPE_NONE);
>  }
>  EXPORT_SYMBOL_GPL(devlink_info_version_running_put);
> 
> +int devlink_info_version_running_put_ext(struct devlink_info_req *req,
> +					 const char *version_name,
> +					 const char *version_value,
> +					 enum devlink_info_version_type
> version_type)
> +{
> +	return devlink_info_version_put(req,
> DEVLINK_ATTR_INFO_VERSION_RUNNING,
> +					version_name, version_value,
> +					version_type);
> +}
> +EXPORT_SYMBOL_GPL(devlink_info_version_running_put_ext);
> +
>  static int
>  devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink,
>  		     enum devlink_command cmd, u32 portid,
>  		     u32 seq, int flags, struct netlink_ext_ack *extack)
>  {
> -	struct devlink_info_req req;
> +	struct devlink_info_req req = {};
>  	void *hdr;
>  	int err;
> 
> @@ -12306,8 +12392,8 @@
> EXPORT_SYMBOL_GPL(devl_trap_policers_unregister);
>  static void __devlink_compat_running_version(struct devlink *devlink,
>  					     char *buf, size_t len)
>  {
> +	struct devlink_info_req req = {};
>  	const struct nlattr *nlattr;
> -	struct devlink_info_req req;
>  	struct sk_buff *msg;
>  	int rem, err;
> 
> --
> 2.37.1

Re: [patch net-next 0/4] net: devlink: sync flash and dev info commands

[Jakub Kicinski]

On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote:
> Currently it is up to the driver what versions to expose and what flash
> update component names to accept. This is inconsistent. Thankfully, only
> netdevsim is currently using components, so it is a good time
> to sanitize this.

Please take a look at recently merged code - 5417197dd516 ("Merge branch
'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions
there so I think you're gonna break them?

Re: [patch net-next 4/4] net: devlink: expose default flash update target

[Jakub Kicinski]

On Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote:
> Allow driver to mark certain version obtained by info_get() op as
> "flash update default". Expose this information to user which allows him
> to understand what version is going to be affected if he does flash
> update without specifying the component. Implement this in netdevsim.

My intuition would be that if you specify no component you're flashing
the entire device. Is that insufficient? Can you explain the use case?

Also Documentation/ needs to be updated.

Re: [patch net-next 1/4] net: devlink: extend info_get() version put to indicate a flash component

[Jiri Pirko]

Thu, Aug 18, 2022 at 11:23:44PM CEST, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Thursday, August 18, 2022 6:01 AM
>> To: netdev@vger.kernel.org
>> Cc: davem@davemloft.net; kuba@kernel.org; idosch@nvidia.com;
>> pabeni@redhat.com; edumazet@google.com; saeedm@nvidia.com; Keller, Jacob
>> E <jacob.e.keller@intel.com>; vikas.gupta@broadcom.com;
>> gospo@broadcom.com
>> Subject: [patch net-next 1/4] net: devlink: extend info_get() version put to
>> indicate a flash component
>> 
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Limit the acceptance of component name passed to cmd_flash_update() to
>> match one of the versions returned by info_get(), marked by version type.
>> This makes things clearer and enforces 1:1 mapping between exposed
>> version and accepted flash component.
>> 
>
>I feel like this commit does quite a bit and could be separated into one part that only adds the components flagging in info and another which uses this in the cmd_flash_update.

I thought about that, but felt like having it in one
patch makes more sense.
Okay, will split.


>
>> Whenever the driver is called by his info_get() op, it may put multiple
>> version names and values to the netlink message. Extend by additional
>> helper devlink_info_version_running_put_ext() that allows to specify a
>> version type that indicates when particular version name represents
>> a flash component.
>> 
>> Use this indication during cmd_flash_update() execution by calling
>> info_get() with different "req" context. That causes info_get() to
>> lookup the component name instead of filling-up the netlink message.
>> 
>> Fix the only component user which is netdevsim. It uses component named
>> "fw.mgmt" in selftests.
>> 
>> Remove now outdated "UPDATE_COMPONENT" flag.
>
>Is this flag outdated? I believe we're supposed to use this to indicate when a driver supports per-component update, which we would do once another driver actually uses the interface? I guess now instead we just check to see if any of the flash fields have the component flag set instead? Ok

You answered yourself :)


>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>
>
>Code looks ok to me, but it would be easier to review if this was separated into one commit for addng the info changes, one for enforcing them, and one for updating netdevsim.

Okay.



>
>I'll try to find the patches I had for ice to implement this per-component update and specifying which components are default once this gets merged.

Good.

>
>Thanks,
>Jake
>
>> ---
>>  drivers/net/netdevsim/dev.c |  12 +++-
>>  include/net/devlink.h       |  15 ++++-
>>  net/core/devlink.c          | 128 ++++++++++++++++++++++++++++++------
>>  3 files changed, 129 insertions(+), 26 deletions(-)
>> 
>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> index e88f783c297e..cea130490dea 100644
>> --- a/drivers/net/netdevsim/dev.c
>> +++ b/drivers/net/netdevsim/dev.c
>> @@ -984,7 +984,14 @@ static int nsim_dev_info_get(struct devlink *devlink,
>>  			     struct devlink_info_req *req,
>>  			     struct netlink_ext_ack *extack)
>>  {
>> -	return devlink_info_driver_name_put(req, DRV_NAME);
>> +	int err;
>> +
>> +	err = devlink_info_driver_name_put(req, DRV_NAME);
>> +	if (err)
>> +		return err;
>> +
>> +	return devlink_info_version_running_put_ext(req, "fw.mgmt",
>> "10.20.30",
>> +
>> DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>  }
>> 
>>  #define NSIM_DEV_FLASH_SIZE 500000
>> @@ -1312,8 +1319,7 @@ nsim_dev_devlink_trap_drop_counter_get(struct
>> devlink *devlink,
>>  static const struct devlink_ops nsim_dev_devlink_ops = {
>>  	.eswitch_mode_set = nsim_devlink_eswitch_mode_set,
>>  	.eswitch_mode_get = nsim_devlink_eswitch_mode_get,
>> -	.supported_flash_update_params =
>> DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT |
>> -
>> DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
>> +	.supported_flash_update_params =
>> DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
>>  	.reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT),
>>  	.reload_down = nsim_dev_reload_down,
>>  	.reload_up = nsim_dev_reload_up,
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index 119ed1ffb988..9bf4f03feca6 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -624,8 +624,7 @@ struct devlink_flash_update_params {
>>  	u32 overwrite_mask;
>>  };
>> 
>> -#define DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT		BIT(0)
>> -#define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK	BIT(1)
>> +#define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK	BIT(0)
>> 
>
>This stays in the kernel so there's no issue with changing the bits. Ok.
>
>>  struct devlink_region;
>>  struct devlink_info_req;
>> @@ -1714,6 +1713,14 @@ int devlink_info_driver_name_put(struct
>> devlink_info_req *req,
>>  				 const char *name);
>>  int devlink_info_board_serial_number_put(struct devlink_info_req *req,
>>  					 const char *bsn);
>> +
>> +enum devlink_info_version_type {
>> +	DEVLINK_INFO_VERSION_TYPE_NONE,
>> +	DEVLINK_INFO_VERSION_TYPE_COMPONENT, /* May be used as flash
>> update
>> +					      * component by name.
>> +					      */
>> +};
>> +
>>  int devlink_info_version_fixed_put(struct devlink_info_req *req,
>>  				   const char *version_name,
>>  				   const char *version_value);
>> @@ -1723,6 +1730,10 @@ int devlink_info_version_stored_put(struct
>> devlink_info_req *req,
>>  int devlink_info_version_running_put(struct devlink_info_req *req,
>>  				     const char *version_name,
>>  				     const char *version_value);
>> +int devlink_info_version_running_put_ext(struct devlink_info_req *req,
>> +					 const char *version_name,
>> +					 const char *version_value,
>> +					 enum devlink_info_version_type
>> version_type);
>> 
>>  int devlink_fmsg_obj_nest_start(struct devlink_fmsg *fmsg);
>>  int devlink_fmsg_obj_nest_end(struct devlink_fmsg *fmsg);
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index b50bcc18b8d9..17b78123ad9d 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -4742,10 +4742,76 @@ void devlink_flash_update_timeout_notify(struct
>> devlink *devlink,
>>  }
>>  EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
>> 
>> +struct devlink_info_req {
>> +	struct sk_buff *msg;
>> +	void (*version_cb)(const char *version_name,
>> +			   enum devlink_info_version_type version_type,
>> +			   void *version_cb_priv);
>> +	void *version_cb_priv;
>> +};
>> +
>> +struct devlink_flash_component_lookup_ctx {
>> +	const char *lookup_name;
>> +	bool lookup_name_found;
>> +};
>> +
>> +static void
>> +devlink_flash_component_lookup_cb(const char *version_name,
>> +				  enum devlink_info_version_type version_type,
>> +				  void *version_cb_priv)
>> +{
>> +	struct devlink_flash_component_lookup_ctx *lookup_ctx =
>> version_cb_priv;
>> +
>> +	if (version_type != DEVLINK_INFO_VERSION_TYPE_COMPONENT ||
>> +	    lookup_ctx->lookup_name_found)
>> +		return;
>> +
>> +	lookup_ctx->lookup_name_found =
>> +		!strcmp(lookup_ctx->lookup_name, version_name);
>> +}
>> +
>> +static int devlink_flash_component_get(struct devlink *devlink,
>> +				       struct nlattr *nla_component,
>> +				       const char **p_component,
>> +				       struct netlink_ext_ack *extack)
>> +{
>> +	struct devlink_flash_component_lookup_ctx lookup_ctx = {};
>> +	struct devlink_info_req req = {};
>> +	const char *component;
>> +	int ret;
>> +
>> +	if (!nla_component)
>> +		return 0;
>> +
>> +	component = nla_data(nla_component);
>> +
>> +	if (!devlink->ops->info_get) {
>> +		NL_SET_ERR_MSG_ATTR(extack, nla_component,
>> +				    "component update is not supported by this
>> device");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	lookup_ctx.lookup_name = component;
>> +	req.version_cb = devlink_flash_component_lookup_cb;
>> +	req.version_cb_priv = &lookup_ctx;
>> +
>> +	ret = devlink->ops->info_get(devlink, &req, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!lookup_ctx.lookup_name_found) {
>> +		NL_SET_ERR_MSG_ATTR(extack, nla_component,
>> +				    "selected component is not supported by this
>> device");
>> +		return -EINVAL;
>> +	}
>> +	*p_component = component;
>> +	return 0;
>> +}
>> +
>>  static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
>>  				       struct genl_info *info)
>>  {
>> -	struct nlattr *nla_component, *nla_overwrite_mask, *nla_file_name;
>> +	struct nlattr *nla_overwrite_mask, *nla_file_name;
>>  	struct devlink_flash_update_params params = {};
>>  	struct devlink *devlink = info->user_ptr[0];
>>  	const char *file_name;
>> @@ -4758,17 +4824,13 @@ static int devlink_nl_cmd_flash_update(struct
>> sk_buff *skb,
>>  	if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
>>  		return -EINVAL;
>> 
>> -	supported_params = devlink->ops->supported_flash_update_params;
>> +	ret = devlink_flash_component_get(devlink,
>> +					  info-
>> >attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT],
>> +					  &params.component, info->extack);
>> +	if (ret)
>> +		return ret;
>> 
>> -	nla_component = info-
>> >attrs[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT];
>> -	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
>> by this device");
>> -			return -EOPNOTSUPP;
>> -		}
>> -		params.component = nla_data(nla_component);
>> -	}
>> +	supported_params = devlink->ops->supported_flash_update_params;
>> 
>>  	nla_overwrite_mask = info-
>> >attrs[DEVLINK_ATTR_FLASH_UPDATE_OVERWRITE_MASK];
>>  	if (nla_overwrite_mask) {
>> @@ -6553,18 +6615,18 @@ static int
>> devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>>  	return err;
>>  }
>> 
>> -struct devlink_info_req {
>> -	struct sk_buff *msg;
>> -};
>> -
>>  int devlink_info_driver_name_put(struct devlink_info_req *req, const char
>> *name)
>>  {
>> +	if (!req->msg)
>> +		return 0;
>>  	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_DRIVER_NAME,
>> name);
>>  }
>>  EXPORT_SYMBOL_GPL(devlink_info_driver_name_put);
>> 
>>  int devlink_info_serial_number_put(struct devlink_info_req *req, const char
>> *sn)
>>  {
>> +	if (!req->msg)
>> +		return 0;
>>  	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER,
>> sn);
>>  }
>>  EXPORT_SYMBOL_GPL(devlink_info_serial_number_put);
>> @@ -6572,6 +6634,8 @@
>> EXPORT_SYMBOL_GPL(devlink_info_serial_number_put);
>>  int devlink_info_board_serial_number_put(struct devlink_info_req *req,
>>  					 const char *bsn)
>>  {
>> +	if (!req->msg)
>> +		return 0;
>>  	return nla_put_string(req->msg,
>> DEVLINK_ATTR_INFO_BOARD_SERIAL_NUMBER,
>>  			      bsn);
>>  }
>> @@ -6579,11 +6643,19 @@
>> EXPORT_SYMBOL_GPL(devlink_info_board_serial_number_put);
>> 
>>  static int devlink_info_version_put(struct devlink_info_req *req, int attr,
>>  				    const char *version_name,
>> -				    const char *version_value)
>> +				    const char *version_value,
>> +				    enum devlink_info_version_type version_type)
>>  {
>>  	struct nlattr *nest;
>>  	int err;
>> 
>> +	if (req->version_cb)
>> +		req->version_cb(version_name, version_type,
>> +				req->version_cb_priv);
>> +
>> +	if (!req->msg)
>> +		return 0;
>> +
>>  	nest = nla_nest_start_noflag(req->msg, attr);
>>  	if (!nest)
>>  		return -EMSGSIZE;
>> @@ -6612,7 +6684,8 @@ int devlink_info_version_fixed_put(struct
>> devlink_info_req *req,
>>  				   const char *version_value)
>>  {
>>  	return devlink_info_version_put(req,
>> DEVLINK_ATTR_INFO_VERSION_FIXED,
>> -					version_name, version_value);
>> +					version_name, version_value,
>> +					DEVLINK_INFO_VERSION_TYPE_NONE);
>>  }
>>  EXPORT_SYMBOL_GPL(devlink_info_version_fixed_put);
>> 
>> @@ -6621,7 +6694,8 @@ int devlink_info_version_stored_put(struct
>> devlink_info_req *req,
>>  				    const char *version_value)
>>  {
>>  	return devlink_info_version_put(req,
>> DEVLINK_ATTR_INFO_VERSION_STORED,
>> -					version_name, version_value);
>> +					version_name, version_value,
>> +					DEVLINK_INFO_VERSION_TYPE_NONE);
>>  }
>>  EXPORT_SYMBOL_GPL(devlink_info_version_stored_put);
>> 
>> @@ -6630,16 +6704,28 @@ int devlink_info_version_running_put(struct
>> devlink_info_req *req,
>>  				     const char *version_value)
>>  {
>>  	return devlink_info_version_put(req,
>> DEVLINK_ATTR_INFO_VERSION_RUNNING,
>> -					version_name, version_value);
>> +					version_name, version_value,
>> +					DEVLINK_INFO_VERSION_TYPE_NONE);
>>  }
>>  EXPORT_SYMBOL_GPL(devlink_info_version_running_put);
>> 
>> +int devlink_info_version_running_put_ext(struct devlink_info_req *req,
>> +					 const char *version_name,
>> +					 const char *version_value,
>> +					 enum devlink_info_version_type
>> version_type)
>> +{
>> +	return devlink_info_version_put(req,
>> DEVLINK_ATTR_INFO_VERSION_RUNNING,
>> +					version_name, version_value,
>> +					version_type);
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_info_version_running_put_ext);
>> +
>>  static int
>>  devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink,
>>  		     enum devlink_command cmd, u32 portid,
>>  		     u32 seq, int flags, struct netlink_ext_ack *extack)
>>  {
>> -	struct devlink_info_req req;
>> +	struct devlink_info_req req = {};
>>  	void *hdr;
>>  	int err;
>> 
>> @@ -12306,8 +12392,8 @@
>> EXPORT_SYMBOL_GPL(devl_trap_policers_unregister);
>>  static void __devlink_compat_running_version(struct devlink *devlink,
>>  					     char *buf, size_t len)
>>  {
>> +	struct devlink_info_req req = {};
>>  	const struct nlattr *nlattr;
>> -	struct devlink_info_req req;
>>  	struct sk_buff *msg;
>>  	int rem, err;
>> 
>> --
>> 2.37.1
>

Re: [patch net-next 4/4] net: devlink: expose default flash update target

[Jiri Pirko]

Fri, Aug 19, 2022 at 04:53:01AM CEST, kuba@kernel.org wrote:
>On Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote:
>> Allow driver to mark certain version obtained by info_get() op as
>> "flash update default". Expose this information to user which allows him
>> to understand what version is going to be affected if he does flash
>> update without specifying the component. Implement this in netdevsim.
>
>My intuition would be that if you specify no component you're flashing
>the entire device. Is that insufficient? Can you explain the use case?

I guess that it up to the driver implementation. I can imagine arguments
for both ways. Anyway, there is no way to restrict this in kernel, so
let that up to the driver.


>
>Also Documentation/ needs to be updated.

Okay.

Re: [patch net-next 0/4] net: devlink: sync flash and dev info command

[Jiri Pirko]

Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote:
>On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote:
>> Currently it is up to the driver what versions to expose and what flash
>> update component names to accept. This is inconsistent. Thankfully, only
>> netdevsim is currently using components, so it is a good time
>> to sanitize this.
>
>Please take a look at recently merged code - 5417197dd516 ("Merge branch
>'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions
>there so I think you're gonna break them?

Ah, crap. Too late :/ They are passing the string to FW (cmd is
the component name here):
static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
{
        char flash_command[T7XX_FB_COMMAND_SIZE];

        snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
        return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
}

This breaks the pairing with info.versions assumption. Any possibility
to revert this and let them redo?

Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com,
soumya.prakash.mishra@intel.com

Guys, could you expose one version for component you are flashing? We
need 1:1 mapping here.

Thanks!

RE: [patch net-next 4/4] net: devlink: expose default flash update target

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, August 18, 2022 7:53 PM
> To: Jiri Pirko <jiri@resnulli.us>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; idosch@nvidia.com;
> pabeni@redhat.com; edumazet@google.com; saeedm@nvidia.com; Keller, Jacob
> E <jacob.e.keller@intel.com>; vikas.gupta@broadcom.com;
> gospo@broadcom.com
> Subject: Re: [patch net-next 4/4] net: devlink: expose default flash update target
> 
> On Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote:
> > Allow driver to mark certain version obtained by info_get() op as
> > "flash update default". Expose this information to user which allows him
> > to understand what version is going to be affected if he does flash
> > update without specifying the component. Implement this in netdevsim.
> 
> My intuition would be that if you specify no component you're flashing
> the entire device. Is that insufficient? Can you explain the use case?
> 
> Also Documentation/ needs to be updated.

Some of the components in ice include the DDP which has an info version, but which is not part of the flash but is loaded by the driver during initialization.

Thanks,
Jake

Re: [patch net-next 4/4] net: devlink: expose default flash update target

[Jakub Kicinski]

On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote:
> > My intuition would be that if you specify no component you're flashing
> > the entire device. Is that insufficient? Can you explain the use case?
> > 
> > Also Documentation/ needs to be updated.  
> 
> Some of the components in ice include the DDP which has an info
> version, but which is not part of the flash but is loaded by the
> driver during initialization.

Right "entire device" as in "everything in 'stored'". Runtime loaded
stuff should not be listed in "stored" and therefore not be considered
"flashable". Correct?

Re: [patch net-next 4/4] net: devlink: expose default flash update target

[Jakub Kicinski]

On Fri, 19 Aug 2022 10:12:16 +0200 Jiri Pirko wrote:
> Fri, Aug 19, 2022 at 04:53:01AM CEST, kuba@kernel.org wrote:
> >On Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote:  
> >> Allow driver to mark certain version obtained by info_get() op as
> >> "flash update default". Expose this information to user which allows him
> >> to understand what version is going to be affected if he does flash
> >> update without specifying the component. Implement this in netdevsim.  
> >
> >My intuition would be that if you specify no component you're flashing
> >the entire device. Is that insufficient? Can you explain the use case?  
> 
> I guess that it up to the driver implementation. I can imagine arguments
> for both ways. Anyway, there is no way to restrict this in kernel, so
> let that up to the driver.

To be clear - your intent is to impose more structure on the relation
between the dev info and dev flash, right? But just "to be safe",
there's no immediate need to do this?

The entire dev info / dev flash interface was driven by practical needs
of the fleet management team @Facebook / Meta.

What would make the changes you're making more useful here would be if
instead of declaring the "default" component, we declared "overall"
component. I.e. the component which is guaranteed to encompass all the
other versions in "stored", and coincidentally is also the default
flashed one.

That way the FW version reporting can be simplified to store only one
version.

RE: [patch net-next 4/4] net: devlink: expose default flash update target

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, August 19, 2022 2:46 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org; davem@davemloft.net;
> idosch@nvidia.com; pabeni@redhat.com; edumazet@google.com;
> saeedm@nvidia.com; vikas.gupta@broadcom.com; gospo@broadcom.com
> Subject: Re: [patch net-next 4/4] net: devlink: expose default flash update target
> 
> On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote:
> > > My intuition would be that if you specify no component you're flashing
> > > the entire device. Is that insufficient? Can you explain the use case?
> > >
> > > Also Documentation/ needs to be updated.
> >
> > Some of the components in ice include the DDP which has an info
> > version, but which is not part of the flash but is loaded by the
> > driver during initialization.
> 
> Right "entire device" as in "everything in 'stored'". Runtime loaded
> stuff should not be listed in "stored" and therefore not be considered
> "flashable". Correct?

Yes I believe we don't list those as stored.

We do have some extra version information that is reported through multiple info lines, i.e. we report:

fw.mgmt
fw.mgmt.api
fw.mgmt.build

where the .api and .build are sub-version fields of the fw.mgmt and can potentially give further information but are just a part of the fw.mgmt component. They can't be flashed separately.

Thanks,
Jake

Re: [patch net-next 4/4] net: devlink: expose default flash update target

[Jiri Pirko]

Fri, Aug 19, 2022 at 11:54:59PM CEST, kuba@kernel.org wrote:
>On Fri, 19 Aug 2022 10:12:16 +0200 Jiri Pirko wrote:
>> Fri, Aug 19, 2022 at 04:53:01AM CEST, kuba@kernel.org wrote:
>> >On Thu, 18 Aug 2022 15:00:42 +0200 Jiri Pirko wrote:  
>> >> Allow driver to mark certain version obtained by info_get() op as
>> >> "flash update default". Expose this information to user which allows him
>> >> to understand what version is going to be affected if he does flash
>> >> update without specifying the component. Implement this in netdevsim.  
>> >
>> >My intuition would be that if you specify no component you're flashing
>> >the entire device. Is that insufficient? Can you explain the use case?  
>> 
>> I guess that it up to the driver implementation. I can imagine arguments
>> for both ways. Anyway, there is no way to restrict this in kernel, so
>> let that up to the driver.
>
>To be clear - your intent is to impose more structure on the relation
>between the dev info and dev flash, right? But just "to be safe",

Correct. Basically I want to make things clear for the user in terms of
what he can flash, what component names he can pass, what happens during
flash without component. Also, I want to sanitize drivers so they cannot
accept *any* component name.

>there's no immediate need to do this?

Nope.

>
>The entire dev info / dev flash interface was driven by practical needs
>of the fleet management team @Facebook / Meta.
>
>What would make the changes you're making more useful here would be if
>instead of declaring the "default" component, we declared "overall"
>component. I.e. the component which is guaranteed to encompass all the
>other versions in "stored", and coincidentally is also the default
>flashed one.

It is just semantics. Default is what we have now and drivers are using
it. How, that is up to the driver. I see no way how to enforce this, do
you?

But anyway, I can split the patchset in 2:
1) sanitize components
2) default/overall/whatever
If that would help.


>
>That way the FW version reporting can be simplified to store only one
>version.

Re: [patch net-next 4/4] net: devlink: expose default flash update target

[Jiri Pirko]

Sat, Aug 20, 2022 at 12:07:41AM CEST, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Jakub Kicinski <kuba@kernel.org>
>> Sent: Friday, August 19, 2022 2:46 PM
>> To: Keller, Jacob E <jacob.e.keller@intel.com>
>> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org; davem@davemloft.net;
>> idosch@nvidia.com; pabeni@redhat.com; edumazet@google.com;
>> saeedm@nvidia.com; vikas.gupta@broadcom.com; gospo@broadcom.com
>> Subject: Re: [patch net-next 4/4] net: devlink: expose default flash update target
>> 
>> On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote:
>> > > My intuition would be that if you specify no component you're flashing
>> > > the entire device. Is that insufficient? Can you explain the use case?
>> > >
>> > > Also Documentation/ needs to be updated.
>> >
>> > Some of the components in ice include the DDP which has an info
>> > version, but which is not part of the flash but is loaded by the
>> > driver during initialization.
>> 
>> Right "entire device" as in "everything in 'stored'". Runtime loaded
>> stuff should not be listed in "stored" and therefore not be considered
>> "flashable". Correct?
>
>Yes I believe we don't list those as stored.
>
>We do have some extra version information that is reported through multiple info lines, i.e. we report:
>
>fw.mgmt
>fw.mgmt.api
>fw.mgmt.build
>
>where the .api and .build are sub-version fields of the fw.mgmt and can potentially give further information but are just a part of the fw.mgmt component. They can't be flashed separately.

Yep, in my patchset, this is accounted for. The driver can say if the
"version" is flashable (passed as a compenent name) or not. In this case,
it is not and it only tells the user version of some fw part.

>
>Thanks,
>Jake

Re: [patch net-next 4/4] net: devlink: expose default flash update target

[Jakub Kicinski]

On Sat, 20 Aug 2022 07:44:41 +0200 Jiri Pirko wrote:
> >The entire dev info / dev flash interface was driven by practical needs
> >of the fleet management team @Facebook / Meta.
> >
> >What would make the changes you're making more useful here would be if
> >instead of declaring the "default" component, we declared "overall"
> >component. I.e. the component which is guaranteed to encompass all the
> >other versions in "stored", and coincidentally is also the default
> >flashed one.  
> 
> It is just semantics.

You say that like it's a synonym for irrelevance. Semantics are
*everything* for the uAPI :)

> Default is what we have now and drivers are using
> it. How, that is up to the driver. I see no way how to enforce this, do
> you?

Not sure I understand. We document the thing clearly as "this is the
overall and must include all parts of stored FW", name it appropriately.
If someone sneaks in code which abuses the value for something else,
nothing we can do, like with every driver API we have.

The "default" gives user information but to interpret that information
user is presupposed to know the semantics of the components. Of what
use is knowing that default is component A if I don't know that it's
the component I want to flash. And if so why don't I just say
"component A"?

> But anyway, I can split the patchset in 2:
> 1) sanitize components
> 2) default/overall/whatever
> If that would help.

Sure thing, seems like a practical approach.

On second thought the overall may not be practical, there are sometimes
critical components on the board you don't really want to flash unless
you really have to. CPLDs and stuff. So perhaps we should scratch (2)
altogether until we have a clear need...

RE: [patch net-next 4/4] net: devlink: expose default flash update target

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Friday, August 19, 2022 10:47 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org;
> davem@davemloft.net; idosch@nvidia.com; pabeni@redhat.com;
> edumazet@google.com; saeedm@nvidia.com; vikas.gupta@broadcom.com;
> gospo@broadcom.com
> Subject: Re: [patch net-next 4/4] net: devlink: expose default flash update target
> 
> Sat, Aug 20, 2022 at 12:07:41AM CEST, jacob.e.keller@intel.com wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jakub Kicinski <kuba@kernel.org>
> >> Sent: Friday, August 19, 2022 2:46 PM
> >> To: Keller, Jacob E <jacob.e.keller@intel.com>
> >> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org;
> davem@davemloft.net;
> >> idosch@nvidia.com; pabeni@redhat.com; edumazet@google.com;
> >> saeedm@nvidia.com; vikas.gupta@broadcom.com; gospo@broadcom.com
> >> Subject: Re: [patch net-next 4/4] net: devlink: expose default flash update
> target
> >>
> >> On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote:
> >> > > My intuition would be that if you specify no component you're flashing
> >> > > the entire device. Is that insufficient? Can you explain the use case?
> >> > >
> >> > > Also Documentation/ needs to be updated.
> >> >
> >> > Some of the components in ice include the DDP which has an info
> >> > version, but which is not part of the flash but is loaded by the
> >> > driver during initialization.
> >>
> >> Right "entire device" as in "everything in 'stored'". Runtime loaded
> >> stuff should not be listed in "stored" and therefore not be considered
> >> "flashable". Correct?
> >
> >Yes I believe we don't list those as stored.
> >
> >We do have some extra version information that is reported through multiple
> info lines, i.e. we report:
> >
> >fw.mgmt
> >fw.mgmt.api
> >fw.mgmt.build
> >
> >where the .api and .build are sub-version fields of the fw.mgmt and can
> potentially give further information but are just a part of the fw.mgmt
> component. They can't be flashed separately.
> 
> Yep, in my patchset, this is accounted for. The driver can say if the
> "version" is flashable (passed as a compenent name) or not. In this case,
> it is not and it only tells the user version of some fw part.
> 

I think we can just go with "is this flashable or not?" and then document that if no component is flashed, the driver should be flashing all marked components?

Then we don't need a "default" since the default without component is to flash everything.

> >
> >Thanks,
> >Jake

Re: [patch net-next 4/4] net: devlink: expose default flash update target

[Jiri Pirko]

Mon, Aug 22, 2022 at 07:09:35PM CEST, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Friday, August 19, 2022 10:47 PM
>> To: Keller, Jacob E <jacob.e.keller@intel.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org;
>> davem@davemloft.net; idosch@nvidia.com; pabeni@redhat.com;
>> edumazet@google.com; saeedm@nvidia.com; vikas.gupta@broadcom.com;
>> gospo@broadcom.com
>> Subject: Re: [patch net-next 4/4] net: devlink: expose default flash update target
>> 
>> Sat, Aug 20, 2022 at 12:07:41AM CEST, jacob.e.keller@intel.com wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Jakub Kicinski <kuba@kernel.org>
>> >> Sent: Friday, August 19, 2022 2:46 PM
>> >> To: Keller, Jacob E <jacob.e.keller@intel.com>
>> >> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org;
>> davem@davemloft.net;
>> >> idosch@nvidia.com; pabeni@redhat.com; edumazet@google.com;
>> >> saeedm@nvidia.com; vikas.gupta@broadcom.com; gospo@broadcom.com
>> >> Subject: Re: [patch net-next 4/4] net: devlink: expose default flash update
>> target
>> >>
>> >> On Fri, 19 Aug 2022 20:59:28 +0000 Keller, Jacob E wrote:
>> >> > > My intuition would be that if you specify no component you're flashing
>> >> > > the entire device. Is that insufficient? Can you explain the use case?
>> >> > >
>> >> > > Also Documentation/ needs to be updated.
>> >> >
>> >> > Some of the components in ice include the DDP which has an info
>> >> > version, but which is not part of the flash but is loaded by the
>> >> > driver during initialization.
>> >>
>> >> Right "entire device" as in "everything in 'stored'". Runtime loaded
>> >> stuff should not be listed in "stored" and therefore not be considered
>> >> "flashable". Correct?
>> >
>> >Yes I believe we don't list those as stored.
>> >
>> >We do have some extra version information that is reported through multiple
>> info lines, i.e. we report:
>> >
>> >fw.mgmt
>> >fw.mgmt.api
>> >fw.mgmt.build
>> >
>> >where the .api and .build are sub-version fields of the fw.mgmt and can
>> potentially give further information but are just a part of the fw.mgmt
>> component. They can't be flashed separately.
>> 
>> Yep, in my patchset, this is accounted for. The driver can say if the
>> "version" is flashable (passed as a compenent name) or not. In this case,
>> it is not and it only tells the user version of some fw part.
>> 
>
>I think we can just go with "is this flashable or not?" and then document that if no component is flashed, the driver should be flashing all marked components?
>
>Then we don't need a "default" since the default without component is to flash everything.

I dropped "default" from the patchset. We need to document the
semanticts for default at least. Is it always "everything"? Idk.

>
>> >
>> >Thanks,
>> >Jake

Re: [patch net-next 0/4] net: devlink: sync flash and dev info command

[Kumar, M Chetan]

On 8/19/2022 1:55 PM, Jiri Pirko wrote:
> Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote:
>> On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote:
>>> Currently it is up to the driver what versions to expose and what flash
>>> update component names to accept. This is inconsistent. Thankfully, only
>>> netdevsim is currently using components, so it is a good time
>>> to sanitize this.
>>
>> Please take a look at recently merged code - 5417197dd516 ("Merge branch
>> 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions
>> there so I think you're gonna break them?
> 
> Ah, crap. Too late :/ They are passing the string to FW (cmd is
> the component name here):
> static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
> {
>          char flash_command[T7XX_FB_COMMAND_SIZE];
> 
>          snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
>          return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
> }
> 
> This breaks the pairing with info.versions assumption. Any possibility
> to revert this and let them redo?
> 
> Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com,
> soumya.prakash.mishra@intel.com
> 
> Guys, could you expose one version for component you are flashing? We
> need 1:1 mapping here.

Thanks for the heads-up.
I had a look at the patch & my understanding is driver is supposed
to expose flash update component name & version details via
devlink_info_version_running_put_ext().

Is version value a must ? Internally version value is not used for 
making any decision so in case driver/device doesn't support it should 
be ok to pass empty string ?

Ex:
devlink_info_version_running_put_ext(req, "fw", "",
  DEVLINK_INFO_VERSION_TYPE_COMPONENT);

One observation:-
While testing I noticed "flash_components:" is not getting displayed as 
mentioned in cover note.

Below is the snapshot for mtk_t7xx driver. Am I missing something here ?

# devlink dev info
pci/0000:55:00.0:
driver mtk_t7xx
versions:
        running:
            boot

-- 
Chetan

Re: [patch net-next 0/4] net: devlink: sync flash and dev info command

[Jiri Pirko]

Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote:
>On 8/19/2022 1:55 PM, Jiri Pirko wrote:
>> Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote:
>> > On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote:
>> > > Currently it is up to the driver what versions to expose and what flash
>> > > update component names to accept. This is inconsistent. Thankfully, only
>> > > netdevsim is currently using components, so it is a good time
>> > > to sanitize this.
>> > 
>> > Please take a look at recently merged code - 5417197dd516 ("Merge branch
>> > 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions
>> > there so I think you're gonna break them?
>> 
>> Ah, crap. Too late :/ They are passing the string to FW (cmd is
>> the component name here):
>> static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
>> {
>>          char flash_command[T7XX_FB_COMMAND_SIZE];
>> 
>>          snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
>>          return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
>> }
>> 
>> This breaks the pairing with info.versions assumption. Any possibility
>> to revert this and let them redo?
>> 
>> Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com,
>> soumya.prakash.mishra@intel.com
>> 
>> Guys, could you expose one version for component you are flashing? We
>> need 1:1 mapping here.
>
>Thanks for the heads-up.
>I had a look at the patch & my understanding is driver is supposed
>to expose flash update component name & version details via
>devlink_info_version_running_put_ext().

Yes.

>
>Is version value a must ? Internally version value is not used for making any
>decision so in case driver/device doesn't support it should be ok to pass
>empty string ?

No.

>
>Ex:
>devlink_info_version_running_put_ext(req, "fw", "",
> DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>
>One observation:-
>While testing I noticed "flash_components:" is not getting displayed as
>mentioned in cover note.

You need iproute2 patch for that which is still in my queue:
https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683


>
>Below is the snapshot for mtk_t7xx driver. Am I missing something here ?
>
># devlink dev info
>pci/0000:55:00.0:
>driver mtk_t7xx
>versions:
>       running:
>           boot
>
>-- 
>Chetan

Re: [patch net-next 0/4] net: devlink: sync flash and dev info command

[Kumar, M Chetan]

On 8/23/2022 5:50 PM, Jiri Pirko wrote:
> Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote:
>> On 8/19/2022 1:55 PM, Jiri Pirko wrote:
>>> Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote:
>>>> On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote:
>>>>> Currently it is up to the driver what versions to expose and what flash
>>>>> update component names to accept. This is inconsistent. Thankfully, only
>>>>> netdevsim is currently using components, so it is a good time
>>>>> to sanitize this.
>>>>
>>>> Please take a look at recently merged code - 5417197dd516 ("Merge branch
>>>> 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions
>>>> there so I think you're gonna break them?
>>>
>>> Ah, crap. Too late :/ They are passing the string to FW (cmd is
>>> the component name here):
>>> static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
>>> {
>>>           char flash_command[T7XX_FB_COMMAND_SIZE];
>>>
>>>           snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
>>>           return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
>>> }
>>>
>>> This breaks the pairing with info.versions assumption. Any possibility
>>> to revert this and let them redo?
>>>
>>> Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com,
>>> soumya.prakash.mishra@intel.com
>>>
>>> Guys, could you expose one version for component you are flashing? We
>>> need 1:1 mapping here.
>>
>> Thanks for the heads-up.
>> I had a look at the patch & my understanding is driver is supposed
>> to expose flash update component name & version details via
>> devlink_info_version_running_put_ext().
> 
> Yes.
> 
>>
>> Is version value a must ? Internally version value is not used for making any
>> decision so in case driver/device doesn't support it should be ok to pass
>> empty string ?
> 
> No.
> 
>>
>> Ex:
>> devlink_info_version_running_put_ext(req, "fw", "",
>> DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>
>> One observation:-
>> While testing I noticed "flash_components:" is not getting displayed as
>> mentioned in cover note.
> 
> You need iproute2 patch for that which is still in my queue:
> https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683

Thanks. After applying this patch "flash_components" details are getting 
displayed.

Another observation is if NULL is passed for version_value there is a 
crash. Below is the backtrace.

3187.556637] BUG: kernel NULL pointer dereference, address: 0000000000000000
[ 3187.556659] #PF: supervisor read access in kernel mode
[ 3187.556666] #PF: error_code(0x0000) - not-present page
3187.556791] Call Trace:
[ 3187.556796]  <TASK>
[ 3187.556801]  ? devlink_info_version_put+0x112/0x1d0
[ 3187.556823]  ? __nla_put+0x20/0x30
[ 3187.556833]  devlink_info_version_running_put_ext+0x1c/0x30
[ 3187.556851]  t7xx_devlink_info_get+0x37/0x40 [mtk_t7xx]
[ 3187.556880]  devlink_nl_info_fill.constprop.0+0xa1/0x120
[ 3187.556892]  devlink_nl_cmd_info_get_dumpit+0xa8/0x140
[ 3187.556901]  netlink_dump+0x1a3/0x340
[ 3187.556913]  __netlink_dump_start+0x1d0/0x290

Is driver expected to set version number along with component name ?

mtk_t7xx WWAN driver is using the devlink interface for flashing the fw 
to WWAN device. If WWAN device is not capable of supporting the 
versioning for each component how should we handle ? Please suggest.

-- 
Chetan

Re: [patch net-next 0/4] net: devlink: sync flash and dev info command

[Jiri Pirko]

Tue, Aug 23, 2022 at 06:29:48PM CEST, m.chetan.kumar@linux.intel.com wrote:
>On 8/23/2022 5:50 PM, Jiri Pirko wrote:
>> Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote:
>> > On 8/19/2022 1:55 PM, Jiri Pirko wrote:
>> > > Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote:
>> > > > On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote:
>> > > > > Currently it is up to the driver what versions to expose and what flash
>> > > > > update component names to accept. This is inconsistent. Thankfully, only
>> > > > > netdevsim is currently using components, so it is a good time
>> > > > > to sanitize this.
>> > > > 
>> > > > Please take a look at recently merged code - 5417197dd516 ("Merge branch
>> > > > 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions
>> > > > there so I think you're gonna break them?
>> > > 
>> > > Ah, crap. Too late :/ They are passing the string to FW (cmd is
>> > > the component name here):
>> > > static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
>> > > {
>> > >           char flash_command[T7XX_FB_COMMAND_SIZE];
>> > > 
>> > >           snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
>> > >           return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
>> > > }
>> > > 
>> > > This breaks the pairing with info.versions assumption. Any possibility
>> > > to revert this and let them redo?
>> > > 
>> > > Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com,
>> > > soumya.prakash.mishra@intel.com
>> > > 
>> > > Guys, could you expose one version for component you are flashing? We
>> > > need 1:1 mapping here.
>> > 
>> > Thanks for the heads-up.
>> > I had a look at the patch & my understanding is driver is supposed
>> > to expose flash update component name & version details via
>> > devlink_info_version_running_put_ext().
>> 
>> Yes.
>> 
>> > 
>> > Is version value a must ? Internally version value is not used for making any
>> > decision so in case driver/device doesn't support it should be ok to pass
>> > empty string ?
>> 
>> No.
>> 
>> > 
>> > Ex:
>> > devlink_info_version_running_put_ext(req, "fw", "",
>> > DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>> > 
>> > One observation:-
>> > While testing I noticed "flash_components:" is not getting displayed as
>> > mentioned in cover note.
>> 
>> You need iproute2 patch for that which is still in my queue:
>> https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683
>
>Thanks. After applying this patch "flash_components" details are getting
>displayed.
>
>Another observation is if NULL is passed for version_value there is a crash.

So don't pass NULL :)


>Below is the backtrace.
>
>3187.556637] BUG: kernel NULL pointer dereference, address: 0000000000000000
>[ 3187.556659] #PF: supervisor read access in kernel mode
>[ 3187.556666] #PF: error_code(0x0000) - not-present page
>3187.556791] Call Trace:
>[ 3187.556796]  <TASK>
>[ 3187.556801]  ? devlink_info_version_put+0x112/0x1d0
>[ 3187.556823]  ? __nla_put+0x20/0x30
>[ 3187.556833]  devlink_info_version_running_put_ext+0x1c/0x30
>[ 3187.556851]  t7xx_devlink_info_get+0x37/0x40 [mtk_t7xx]
>[ 3187.556880]  devlink_nl_info_fill.constprop.0+0xa1/0x120
>[ 3187.556892]  devlink_nl_cmd_info_get_dumpit+0xa8/0x140
>[ 3187.556901]  netlink_dump+0x1a3/0x340
>[ 3187.556913]  __netlink_dump_start+0x1d0/0x290
>
>Is driver expected to set version number along with component name ?

Of course.


>
>mtk_t7xx WWAN driver is using the devlink interface for flashing the fw to
>WWAN device. If WWAN device is not capable of supporting the versioning for
>each component how should we handle ? Please suggest.

The user should have a visibility about what version is currently
stored/running in the device. You should expose it.


>
>-- 
>Chetan

Re: [patch net-next 0/4] net: devlink: sync flash and dev info command

[Kumar, M Chetan]

Looped hua.yang@mediatek.com to email.

On 8/24/2022 2:17 PM, Jiri Pirko wrote:
> Tue, Aug 23, 2022 at 06:29:48PM CEST, m.chetan.kumar@linux.intel.com wrote:
>> On 8/23/2022 5:50 PM, Jiri Pirko wrote:
>>> Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote:
>>>> On 8/19/2022 1:55 PM, Jiri Pirko wrote:
>>>>> Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote:
>>>>>> On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote:
>>>>>>> Currently it is up to the driver what versions to expose and what flash
>>>>>>> update component names to accept. This is inconsistent. Thankfully, only
>>>>>>> netdevsim is currently using components, so it is a good time
>>>>>>> to sanitize this.
>>>>>>
>>>>>> Please take a look at recently merged code - 5417197dd516 ("Merge branch
>>>>>> 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions
>>>>>> there so I think you're gonna break them?
>>>>>
>>>>> Ah, crap. Too late :/ They are passing the string to FW (cmd is
>>>>> the component name here):
>>>>> static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
>>>>> {
>>>>>            char flash_command[T7XX_FB_COMMAND_SIZE];
>>>>>
>>>>>            snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
>>>>>            return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
>>>>> }
>>>>>
>>>>> This breaks the pairing with info.versions assumption. Any possibility
>>>>> to revert this and let them redo?
>>>>>
>>>>> Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com,
>>>>> soumya.prakash.mishra@intel.com
>>>>>
>>>>> Guys, could you expose one version for component you are flashing? We
>>>>> need 1:1 mapping here.
>>>>
>>>> Thanks for the heads-up.
>>>> I had a look at the patch & my understanding is driver is supposed
>>>> to expose flash update component name & version details via
>>>> devlink_info_version_running_put_ext().
>>>
>>> Yes.
>>>
>>>>
>>>> Is version value a must ? Internally version value is not used for making any
>>>> decision so in case driver/device doesn't support it should be ok to pass
>>>> empty string ?
>>>
>>> No.
>>>
>>>>
>>>> Ex:
>>>> devlink_info_version_running_put_ext(req, "fw", "",
>>>> DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>>>
>>>> One observation:-
>>>> While testing I noticed "flash_components:" is not getting displayed as
>>>> mentioned in cover note.
>>>
>>> You need iproute2 patch for that which is still in my queue:
>>> https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683
>>
>> Thanks. After applying this patch "flash_components" details are getting
>> displayed.
>>
>> Another observation is if NULL is passed for version_value there is a crash.
> 
> So don't pass NULL :)
> 
> 
>> Below is the backtrace.
>>
>> 3187.556637] BUG: kernel NULL pointer dereference, address: 0000000000000000
>> [ 3187.556659] #PF: supervisor read access in kernel mode
>> [ 3187.556666] #PF: error_code(0x0000) - not-present page
>> 3187.556791] Call Trace:
>> [ 3187.556796]  <TASK>
>> [ 3187.556801]  ? devlink_info_version_put+0x112/0x1d0
>> [ 3187.556823]  ? __nla_put+0x20/0x30
>> [ 3187.556833]  devlink_info_version_running_put_ext+0x1c/0x30
>> [ 3187.556851]  t7xx_devlink_info_get+0x37/0x40 [mtk_t7xx]
>> [ 3187.556880]  devlink_nl_info_fill.constprop.0+0xa1/0x120
>> [ 3187.556892]  devlink_nl_cmd_info_get_dumpit+0xa8/0x140
>> [ 3187.556901]  netlink_dump+0x1a3/0x340
>> [ 3187.556913]  __netlink_dump_start+0x1d0/0x290
>>
>> Is driver expected to set version number along with component name ?
> 
> Of course.
> 
> 
>>
>> mtk_t7xx WWAN driver is using the devlink interface for flashing the fw to
>> WWAN device. If WWAN device is not capable of supporting the versioning for
>> each component how should we handle ? Please suggest.
> 
> The user should have a visibility about what version is currently
> stored/running in the device. You should expose it.

If the only intention of this component version is to give a visbility 
to user, the WWAN Driver exposes the AT & MBIM control ports. 
Applications like Modem Manager uses AT/MBIM commands to obtain fw 
version info.

So would it be ok to keep component version as an optional for WWAN 
drivers ?

-- 
Chetan

Re: [patch net-next 0/4] net: devlink: sync flash and dev info command

[Jiri Pirko]

Fri, Aug 26, 2022 at 10:54:52AM CEST, m.chetan.kumar@linux.intel.com wrote:
>Looped hua.yang@mediatek.com to email.
>
>On 8/24/2022 2:17 PM, Jiri Pirko wrote:
>> Tue, Aug 23, 2022 at 06:29:48PM CEST, m.chetan.kumar@linux.intel.com wrote:
>> > On 8/23/2022 5:50 PM, Jiri Pirko wrote:
>> > > Tue, Aug 23, 2022 at 12:09:06PM CEST, m.chetan.kumar@linux.intel.com wrote:
>> > > > On 8/19/2022 1:55 PM, Jiri Pirko wrote:
>> > > > > Fri, Aug 19, 2022 at 04:49:40AM CEST, kuba@kernel.org wrote:
>> > > > > > On Thu, 18 Aug 2022 15:00:38 +0200 Jiri Pirko wrote:
>> > > > > > > Currently it is up to the driver what versions to expose and what flash
>> > > > > > > update component names to accept. This is inconsistent. Thankfully, only
>> > > > > > > netdevsim is currently using components, so it is a good time
>> > > > > > > to sanitize this.
>> > > > > > 
>> > > > > > Please take a look at recently merged code - 5417197dd516 ("Merge branch
>> > > > > > 'wwan-t7xx-fw-flashing-and-coredump-support'"), I don't see any versions
>> > > > > > there so I think you're gonna break them?
>> > > > > 
>> > > > > Ah, crap. Too late :/ They are passing the string to FW (cmd is
>> > > > > the component name here):
>> > > > > static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
>> > > > > {
>> > > > >            char flash_command[T7XX_FB_COMMAND_SIZE];
>> > > > > 
>> > > > >            snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
>> > > > >            return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
>> > > > > }
>> > > > > 
>> > > > > This breaks the pairing with info.versions assumption. Any possibility
>> > > > > to revert this and let them redo?
>> > > > > 
>> > > > > Ccing m.chetan.kumar@linux.intel.com, chandrashekar.devegowda@intel.com,
>> > > > > soumya.prakash.mishra@intel.com
>> > > > > 
>> > > > > Guys, could you expose one version for component you are flashing? We
>> > > > > need 1:1 mapping here.
>> > > > 
>> > > > Thanks for the heads-up.
>> > > > I had a look at the patch & my understanding is driver is supposed
>> > > > to expose flash update component name & version details via
>> > > > devlink_info_version_running_put_ext().
>> > > 
>> > > Yes.
>> > > 
>> > > > 
>> > > > Is version value a must ? Internally version value is not used for making any
>> > > > decision so in case driver/device doesn't support it should be ok to pass
>> > > > empty string ?
>> > > 
>> > > No.
>> > > 
>> > > > 
>> > > > Ex:
>> > > > devlink_info_version_running_put_ext(req, "fw", "",
>> > > > DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>> > > > 
>> > > > One observation:-
>> > > > While testing I noticed "flash_components:" is not getting displayed as
>> > > > mentioned in cover note.
>> > > 
>> > > You need iproute2 patch for that which is still in my queue:
>> > > https://github.com/jpirko/iproute2_mlxsw/commit/e1d36409362257cc42a435f6695d2058ab7ab683
>> > 
>> > Thanks. After applying this patch "flash_components" details are getting
>> > displayed.
>> > 
>> > Another observation is if NULL is passed for version_value there is a crash.
>> 
>> So don't pass NULL :)
>> 
>> 
>> > Below is the backtrace.
>> > 
>> > 3187.556637] BUG: kernel NULL pointer dereference, address: 0000000000000000
>> > [ 3187.556659] #PF: supervisor read access in kernel mode
>> > [ 3187.556666] #PF: error_code(0x0000) - not-present page
>> > 3187.556791] Call Trace:
>> > [ 3187.556796]  <TASK>
>> > [ 3187.556801]  ? devlink_info_version_put+0x112/0x1d0
>> > [ 3187.556823]  ? __nla_put+0x20/0x30
>> > [ 3187.556833]  devlink_info_version_running_put_ext+0x1c/0x30
>> > [ 3187.556851]  t7xx_devlink_info_get+0x37/0x40 [mtk_t7xx]
>> > [ 3187.556880]  devlink_nl_info_fill.constprop.0+0xa1/0x120
>> > [ 3187.556892]  devlink_nl_cmd_info_get_dumpit+0xa8/0x140
>> > [ 3187.556901]  netlink_dump+0x1a3/0x340
>> > [ 3187.556913]  __netlink_dump_start+0x1d0/0x290
>> > 
>> > Is driver expected to set version number along with component name ?
>> 
>> Of course.
>> 
>> 
>> > 
>> > mtk_t7xx WWAN driver is using the devlink interface for flashing the fw to
>> > WWAN device. If WWAN device is not capable of supporting the versioning for
>> > each component how should we handle ? Please suggest.
>> 
>> The user should have a visibility about what version is currently
>> stored/running in the device. You should expose it.
>
>If the only intention of this component version is to give a visbility to
>user, the WWAN Driver exposes the AT & MBIM control ports. Applications like
>Modem Manager uses AT/MBIM commands to obtain fw version info.
>
>So would it be ok to keep component version as an optional for WWAN drivers ?

Nope. It is mandatory now for the devlink flash component name to match
version of component reported. If you cannot do that, there is
most likely something wrong with your design