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

[patch net-next v2 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 currently using components so it is still 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 a new flag exposed to the use over netlink for versions.
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.

Example:
$ devlink dev info
netdevsim/netdevsim10:
  driver netdevsim
  versions:
      running:
        fw.mgmt 10.20.30
        fw 11.22.33
      flash_components:
        fw.mgmt
$ 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.

---
v1->v2:
- see changelog of individual patches, no code changes, just split patch
- removed patches that exposed "default flash target"

Jiri Pirko (4):
  net: devlink: extend info_get() version put to indicate a flash
    component
  netdevsim: add version fw.mgmt info info_get() and mark as a component
  net: devlink: limit flash component name to match version returned by
    info_get()
  net: devlink: expose the info about version representing a component

 drivers/net/netdevsim/dev.c  |  12 +++-
 include/net/devlink.h        |  15 +++-
 include/uapi/linux/devlink.h |   2 +
 net/core/devlink.c           | 133 +++++++++++++++++++++++++++++------
 4 files changed, 136 insertions(+), 26 deletions(-)

-- 
2.37.1

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

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

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.

This is going to be used in follow-up patch calling info_get() during
flash update command checking if version with this the version type
exists.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- split from v1 patch "net: devlink: extend info_get() version put to
  indicate a flash component", no code changes
---
 include/net/devlink.h | 12 ++++++++++++
 net/core/devlink.c    | 23 +++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 119ed1ffb988..5f47d5cefaa6 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1714,6 +1714,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 +1731,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..2682e968539e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6579,7 +6579,8 @@ 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;
@@ -6612,7 +6613,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 +6623,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,10 +6633,22 @@ 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,
-- 
2.37.1

[patch net-next v2 2/4] netdevsim: add version fw.mgmt info info_get() and mark as a component

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Fix the only component user which is netdevsim. It uses component named
"fw.mgmt" in selftests. So add this version to info_get() output with
version type component.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- split from v1 patch "net: devlink: extend info_get() version put to
  indicate a flash component", no code changes
---
 drivers/net/netdevsim/dev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index e88f783c297e..97fc17ffff93 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
-- 
2.37.1

[patch net-next v2 3/4] net: devlink: limit flash component name to match version returned by info_get()

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

Check VERSION_TYPE_COMPONENT version type 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.

Remove "UPDATE_COMPONENT" flag which becomes used.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- split from v1 patch "net: devlink: extend info_get() version put to
  indicate a flash component", no code changes
---
 drivers/net/netdevsim/dev.c |   3 +-
 include/net/devlink.h       |   3 +-
 net/core/devlink.c          | 105 ++++++++++++++++++++++++++++++------
 3 files changed, 90 insertions(+), 21 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 97fc17ffff93..cea130490dea 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -1319,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 5f47d5cefaa6..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;
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 2682e968539e..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);
 }
@@ -6585,6 +6649,13 @@ static int devlink_info_version_put(struct devlink_info_req *req, int attr,
 	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;
@@ -6654,7 +6725,7 @@ 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;
 
@@ -12321,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 v2 4/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

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

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Monday, August 22, 2022 10:03 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 v2 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 currently using components so it is still 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 a new flag exposed to the use over netlink for versions.
> 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.
> 
> Example:
> $ devlink dev info
> netdevsim/netdevsim10:
>   driver netdevsim
>   versions:
>       running:
>         fw.mgmt 10.20.30
>         fw 11.22.33
>       flash_components:
>         fw.mgmt
> $ 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.
> 
> ---
> v1->v2:
> - see changelog of individual patches, no code changes, just split patch
> - removed patches that exposed "default flash target"
> 

Thanks for splitting this. It was much easier to read and process the changes. This version looks great to me.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Jiri Pirko (4):
>   net: devlink: extend info_get() version put to indicate a flash
>     component
>   netdevsim: add version fw.mgmt info info_get() and mark as a component
>   net: devlink: limit flash component name to match version returned by
>     info_get()
>   net: devlink: expose the info about version representing a component
> 
>  drivers/net/netdevsim/dev.c  |  12 +++-
>  include/net/devlink.h        |  15 +++-
>  include/uapi/linux/devlink.h |   2 +
>  net/core/devlink.c           | 133 +++++++++++++++++++++++++++++------
>  4 files changed, 136 insertions(+), 26 deletions(-)
> 
> --
> 2.37.1

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

[Jakub Kicinski]

On Mon, 22 Aug 2022 19:02:44 +0200 Jiri Pirko wrote:
> +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);

Why are we hooking into running, wouldn't stored make more sense?

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

[Jakub Kicinski]

On Mon, 22 Aug 2022 19:02:47 +0200 Jiri Pirko wrote:
> 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

I don't think we should add API without a clear use, let's drop this 
as well.

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

[Jiri Pirko]

Tue, Aug 23, 2022 at 05:00:26AM CEST, kuba@kernel.org wrote:
>On Mon, 22 Aug 2022 19:02:44 +0200 Jiri Pirko wrote:
>> +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);
>
>Why are we hooking into running, wouldn't stored make more sense?

I think eventually this should be in both. Netdevsim and mlxsw (which I
did this originally for) does not have "stored".

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

[Jiri Pirko]

Tue, Aug 23, 2022 at 05:01:16AM CEST, kuba@kernel.org wrote:
>On Mon, 22 Aug 2022 19:02:47 +0200 Jiri Pirko wrote:
>> 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
>
>I don't think we should add API without a clear use, let's drop this 
>as well.

What do you mean? This just simply lists components that are possible to
use with devlink dev flash. What is not clear? I don't follow.

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

[Jakub Kicinski]

On Tue, 23 Aug 2022 08:36:01 +0200 Jiri Pirko wrote:
> Tue, Aug 23, 2022 at 05:01:16AM CEST, kuba@kernel.org wrote:
> >I don't think we should add API without a clear use, let's drop this 
> >as well.  
> 
> What do you mean? This just simply lists components that are possible to
> use with devlink dev flash. What is not clear? I don't follow.

Dumping random internal bits of the kernel is not how we create uAPIs.

Again, what is the scenario in which user space needs to know 
the flashable component today ?

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

[Jakub Kicinski]

On Tue, 23 Aug 2022 08:35:04 +0200 Jiri Pirko wrote:
> Tue, Aug 23, 2022 at 05:00:26AM CEST, kuba@kernel.org wrote:
> >On Mon, 22 Aug 2022 19:02:44 +0200 Jiri Pirko wrote:  
> >> +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);  
> >
> >Why are we hooking into running, wouldn't stored make more sense?  
> 
> I think eventually this should be in both. Netdevsim and mlxsw (which I
> did this originally for) does not have "stored".

Well, netdevsim is just API dust, and mlxsw is incorrect :/

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

[Jiri Pirko]

Tue, Aug 23, 2022 at 09:31:21PM CEST, kuba@kernel.org wrote:
>On Tue, 23 Aug 2022 08:36:01 +0200 Jiri Pirko wrote:
>> Tue, Aug 23, 2022 at 05:01:16AM CEST, kuba@kernel.org wrote:
>> >I don't think we should add API without a clear use, let's drop this 
>> >as well.  
>> 
>> What do you mean? This just simply lists components that are possible to
>> use with devlink dev flash. What is not clear? I don't follow.
>
>Dumping random internal bits of the kernel is not how we create uAPIs.
>
>Again, what is the scenario in which user space needs to know 
>the flashable component today ?

Well, I thought it would be polite to let the user know what component
he can pass to the kernel. Now, it is try-fail/success game. But if you
think it is okay to let the user in the doubts, no problem. I will drop
the patch.

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

[Jiri Pirko]

Tue, Aug 23, 2022 at 09:32:08PM CEST, kuba@kernel.org wrote:
>On Tue, 23 Aug 2022 08:35:04 +0200 Jiri Pirko wrote:
>> Tue, Aug 23, 2022 at 05:00:26AM CEST, kuba@kernel.org wrote:
>> >On Mon, 22 Aug 2022 19:02:44 +0200 Jiri Pirko wrote:  
>> >> +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);  
>> >
>> >Why are we hooking into running, wouldn't stored make more sense?  
>> 
>> I think eventually this should be in both. Netdevsim and mlxsw (which I
>> did this originally for) does not have "stored".
>
>Well, netdevsim is just API dust, and mlxsw is incorrect :/

Okay, will add "stored".

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

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Wednesday, August 24, 2022 1:50 AM
> To: Jakub Kicinski <kuba@kernel.org>
> 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 v2 4/4] net: devlink: expose the info about version
> representing a component
> 
> Tue, Aug 23, 2022 at 09:31:21PM CEST, kuba@kernel.org wrote:
> >On Tue, 23 Aug 2022 08:36:01 +0200 Jiri Pirko wrote:
> >> Tue, Aug 23, 2022 at 05:01:16AM CEST, kuba@kernel.org wrote:
> >> >I don't think we should add API without a clear use, let's drop this
> >> >as well.
> >>
> >> What do you mean? This just simply lists components that are possible to
> >> use with devlink dev flash. What is not clear? I don't follow.
> >
> >Dumping random internal bits of the kernel is not how we create uAPIs.
> >
> >Again, what is the scenario in which user space needs to know
> >the flashable component today ?
> 
> Well, I thought it would be polite to let the user know what component
> he can pass to the kernel. Now, it is try-fail/success game. But if you
> think it is okay to let the user in the doubts, no problem. I will drop
> the patch.

I would prefer exposing this as well since it lets the user know which names are valid for flashing.

I do have some patches for ice to support individual component update as well I can post soon.

Thanks,
Jake

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

[Jakub Kicinski]

On Wed, 24 Aug 2022 17:31:46 +0000 Keller, Jacob E wrote:
> > Well, I thought it would be polite to let the user know what component
> > he can pass to the kernel. Now, it is try-fail/success game. But if you
> > think it is okay to let the user in the doubts, no problem. I will drop
> > the patch.  
> 
> I would prefer exposing this as well since it lets the user know which names are valid for flashing.
> 
> I do have some patches for ice to support individual component update as well I can post soon.

Gentlemen, I had multiple false starts myself adding information 
to device info, flashing and health reporters. Adding APIs which
will actually be _useful_ in production is not trivial. I have 
the advantage of being able to talk to Meta's production team first
so none of my patches made it to the list. 

To be clear I'm not saying (nor believe) that Meta's needs or processes 
are in any way "the right way to go" or otherwise should dictate
the APIs. It's just an example I have direct access to.

I don't think I'm out of line asking you for a clear use case.
Just knowing something is flashable is not sufficient information,
the user needs to know what the component actually describes and
what binary to use to update it.

Since we have no use of component flashing now it's all cart
before the horse.

Coincidentally I doubt anyone is making serious use of the health
infrastructure.

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

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, August 24, 2022 11:12 AM
> 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 v2 4/4] net: devlink: expose the info about version
> representing a component
> 
> On Wed, 24 Aug 2022 17:31:46 +0000 Keller, Jacob E wrote:
> > > Well, I thought it would be polite to let the user know what component
> > > he can pass to the kernel. Now, it is try-fail/success game. But if you
> > > think it is okay to let the user in the doubts, no problem. I will drop
> > > the patch.
> >
> > I would prefer exposing this as well since it lets the user know which names are
> valid for flashing.
> >
> > I do have some patches for ice to support individual component update as well
> I can post soon.
> 
> Gentlemen, I had multiple false starts myself adding information
> to device info, flashing and health reporters. Adding APIs which
> will actually be _useful_ in production is not trivial. I have
> the advantage of being able to talk to Meta's production team first
> so none of my patches made it to the list.
> 
> To be clear I'm not saying (nor believe) that Meta's needs or processes
> are in any way "the right way to go" or otherwise should dictate
> the APIs. It's just an example I have direct access to.
> 
> I don't think I'm out of line asking you for a clear use case.
> Just knowing something is flashable is not sufficient information,
> the user needs to know what the component actually describes and
> what binary to use to update it.
> 

At least for ice, the same binary would be used for individual component update. the PLDM firmware binary header describes where each component is within it, and is decoded by lib/pldmfw, we just need to translate the PLDM header codes to the userspace names.

The old tools which Intel supports do have support for such an individual component update, but the demand wasn't very high, so I never got around to posting the patches to support this. There are some corner cases where it might be helpful to flash (or reflash) a single component, but it seems somewhat less useful for most end-users and mostly would be useful for internal engineering and debugging.

Users would still need to know what each component is, and there isn't much the kernel API itself can do here. We do document a short description, but that is going to be limited in usefulness since it likely depends on a lot of related knowledge.

Thanks,
Jake

> Since we have no use of component flashing now it's all cart
> before the horse.
> 
> Coincidentally I doubt anyone is making serious use of the health
> infrastructure.

There haven't been a lot of implementations here, which makes it hard to develop tooling, and then without tooling no one wants to write the implementation....

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

[Andy Gospodarek]

[-- Attachment #1: Type: text/plain, Size: 3047 bytes --]

On Wed, Aug 24, 2022 at 06:46:13PM +0000, Keller, Jacob E wrote:
> 
> 
> > -----Original Message-----
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Wednesday, August 24, 2022 11:12 AM
> > 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 v2 4/4] net: devlink: expose the info about version
> > representing a component
> > 
> > On Wed, 24 Aug 2022 17:31:46 +0000 Keller, Jacob E wrote:
> > > > Well, I thought it would be polite to let the user know what component
> > > > he can pass to the kernel. Now, it is try-fail/success game. But if you
> > > > think it is okay to let the user in the doubts, no problem. I will drop
> > > > the patch.
> > >
> > > I would prefer exposing this as well since it lets the user know which names are
> > valid for flashing.
> > >
> > > I do have some patches for ice to support individual component update as well
> > I can post soon.
> > 
> > Gentlemen, I had multiple false starts myself adding information
> > to device info, flashing and health reporters. Adding APIs which
> > will actually be _useful_ in production is not trivial. I have
> > the advantage of being able to talk to Meta's production team first
> > so none of my patches made it to the list.
> > 
> > To be clear I'm not saying (nor believe) that Meta's needs or processes
> > are in any way "the right way to go" or otherwise should dictate
> > the APIs. It's just an example I have direct access to.
> > 
> > I don't think I'm out of line asking you for a clear use case.
> > Just knowing something is flashable is not sufficient information,
> > the user needs to know what the component actually describes and
> > what binary to use to update it.
> > 
> 
> At least for ice, the same binary would be used for individual component update. the PLDM firmware binary header describes where each component is within it, and is decoded by lib/pldmfw, we just need to translate the PLDM header codes to the userspace names.
> 
> The old tools which Intel supports do have support for such an individual component update, but the demand wasn't very high, so I never got around to posting the patches to support this. There are some corner cases where it might be helpful to flash (or reflash) a single component, but it seems somewhat less useful for most end-users and mostly would be useful for internal engineering and debugging.
> 
> Users would still need to know what each component is, and there isn't much the kernel API itself can do here. We do document a short description, but that is going to be limited in usefulness since it likely depends on a lot of related knowledge.
> 

I agree with this.  Individual component updates are most useful in a
dev/debug environment rather than in production.  I'm not sure there is
value in exporting this all the way out to kernel APIs.



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4222 bytes --]