[PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Ido Schimmel]

Jiri says:

This patchset is extending the line card model by three items:
1) line card devices
2) line card info
3) line card device info

First three patches are introducing the necessary changes in devlink
core.

Then, all three extensions are implemented in mlxsw alongside with
selftest.

Examples:
$ devlink lc show pci/0000:01:00.0 lc 8
pci/0000:01:00.0:
  lc 8 state active type 16x100G
    supported_types:
      16x100G
    devices:
      device 0
      device 1
      device 2
      device 3
$ devlink lc info pci/0000:01:00.0 lc 8
pci/0000:01:00.0:
  lc 8
    versions:
        fixed:
          hw.revision 0
        running:
          ini.version 4
    devices:
      device 0
        versions:
            running:
              fw 19.2010.1310
      device 1
        versions:
            running:
              fw 19.2010.1310
      device 2
        versions:
            running:
              fw 19.2010.1310
      device 3
        versions:
            running:
              fw 19.2010.1310

Note that device FW flashing is going to be implemented in the follow-up
patchset.

Jiri Pirko (11):
  devlink: introduce line card devices support
  devlink: introduce line card info get message
  devlink: introduce line card device info infrastructure
  mlxsw: reg: Extend MDDQ by device_info
  mlxsw: core_linecards: Probe provisioned line cards for devices and
    attach them
  selftests: mlxsw: Check devices on provisioned line card
  mlxsw: core_linecards: Expose HW revision and INI version
  selftests: mlxsw: Check line card info on provisioned line card
  mlxsw: reg: Extend MDDQ device_info by FW version fields
  mlxsw: core_linecards: Expose device FW version over device info
  selftests: mlxsw: Check device info on activated line card

 .../networking/devlink/devlink-linecard.rst   |   4 +
 Documentation/networking/devlink/mlxsw.rst    |  33 ++
 drivers/net/ethernet/mellanox/mlxsw/core.h    |   1 +
 .../ethernet/mellanox/mlxsw/core_linecards.c  | 237 +++++++++++++-
 drivers/net/ethernet/mellanox/mlxsw/reg.h     |  87 ++++-
 include/net/devlink.h                         |  18 +-
 include/uapi/linux/devlink.h                  |   5 +
 net/core/devlink.c                            | 303 +++++++++++++++++-
 .../drivers/net/mlxsw/devlink_linecard.sh     |  61 ++++
 9 files changed, 739 insertions(+), 10 deletions(-)

-- 
2.33.1

[PATCH net-next 01/11] devlink: introduce line card devices support

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Line card can contain one or more devices that makes sense to make
visible to the user. For example, this can be a gearbox with
flash memory, which could be updated.

Provide the driver possibility to attach such devices to a line card
and expose those to user.

Example:
$ devlink lc show pci/0000:01:00.0 lc 8
pci/0000:01:00.0:
  lc 8 state active type 16x100G
    supported_types:
      16x100G
    devices:
      device 0
      device 1
      device 2
      device 3

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 include/net/devlink.h        |   7 +++
 include/uapi/linux/devlink.h |   3 +
 net/core/devlink.c           | 104 ++++++++++++++++++++++++++++++++++-
 3 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 2a2a2a0c93f7..c84b52fb9ff0 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1578,6 +1578,13 @@ struct devlink_linecard *
 devlink_linecard_create(struct devlink *devlink, unsigned int linecard_index,
 			const struct devlink_linecard_ops *ops, void *priv);
 void devlink_linecard_destroy(struct devlink_linecard *linecard);
+struct devlink_linecard_device;
+struct devlink_linecard_device *
+devlink_linecard_device_create(struct devlink_linecard *linecard,
+			       unsigned int device_index);
+void
+devlink_linecard_device_destroy(struct devlink_linecard *linecard,
+				struct devlink_linecard_device *linecard_device);
 void devlink_linecard_provision_set(struct devlink_linecard *linecard,
 				    const char *type);
 void devlink_linecard_provision_clear(struct devlink_linecard *linecard);
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index b3d40a5d72ff..cd578645f94f 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -575,6 +575,9 @@ enum devlink_attr {
 	DEVLINK_ATTR_LINECARD_STATE,		/* u8 */
 	DEVLINK_ATTR_LINECARD_TYPE,		/* string */
 	DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,	/* nested */
+	DEVLINK_ATTR_LINECARD_DEVICE_LIST,	/* nested */
+	DEVLINK_ATTR_LINECARD_DEVICE,		/* nested */
+	DEVLINK_ATTR_LINECARD_DEVICE_INDEX,	/* u32 */
 
 	/* add new attributes above here, update the policy in devlink.c */
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 5cc88490f18f..41d9631ceada 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -83,10 +83,11 @@ struct devlink_linecard {
 	const struct devlink_linecard_ops *ops;
 	void *priv;
 	enum devlink_linecard_state state;
-	struct mutex state_lock; /* Protects state */
+	struct mutex state_lock; /* Protects state and device_list */
 	const char *type;
 	struct devlink_linecard_type *types;
 	unsigned int types_count;
+	struct list_head device_list;
 };
 
 /**
@@ -2058,6 +2059,55 @@ struct devlink_linecard_type {
 	const void *priv;
 };
 
+struct devlink_linecard_device {
+	struct list_head list;
+	unsigned int index;
+};
+
+static int
+devlink_nl_linecard_device_fill(struct sk_buff *msg,
+				struct devlink_linecard_device *linecard_device)
+{
+	struct nlattr *attr;
+
+	attr = nla_nest_start(msg, DEVLINK_ATTR_LINECARD_DEVICE);
+	if (!attr)
+		return -EMSGSIZE;
+	if (nla_put_u32(msg, DEVLINK_ATTR_LINECARD_DEVICE_INDEX,
+			linecard_device->index)) {
+		nla_nest_cancel(msg, attr);
+		return -EMSGSIZE;
+	}
+	nla_nest_end(msg, attr);
+
+	return 0;
+}
+
+static int devlink_nl_linecard_devices_fill(struct sk_buff *msg,
+					    struct devlink_linecard *linecard)
+{
+	struct devlink_linecard_device *linecard_device;
+	struct nlattr *attr;
+	int err;
+
+	if (list_empty(&linecard->device_list))
+		return 0;
+
+	attr = nla_nest_start(msg, DEVLINK_ATTR_LINECARD_DEVICE_LIST);
+	if (!attr)
+		return -EMSGSIZE;
+	list_for_each_entry(linecard_device, &linecard->device_list, list) {
+		err = devlink_nl_linecard_device_fill(msg, linecard_device);
+		if (err) {
+			nla_nest_cancel(msg, attr);
+			return err;
+		}
+	}
+	nla_nest_end(msg, attr);
+
+	return 0;
+}
+
 static int devlink_nl_linecard_fill(struct sk_buff *msg,
 				    struct devlink *devlink,
 				    struct devlink_linecard *linecard,
@@ -2068,6 +2118,7 @@ static int devlink_nl_linecard_fill(struct sk_buff *msg,
 	struct devlink_linecard_type *linecard_type;
 	struct nlattr *attr;
 	void *hdr;
+	int err;
 	int i;
 
 	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd);
@@ -2100,6 +2151,10 @@ static int devlink_nl_linecard_fill(struct sk_buff *msg,
 		nla_nest_end(msg, attr);
 	}
 
+	err = devlink_nl_linecard_devices_fill(msg, linecard);
+	if (err)
+		goto nla_put_failure;
+
 	genlmsg_end(msg, hdr);
 	return 0;
 
@@ -10264,6 +10319,7 @@ devlink_linecard_create(struct devlink *devlink, unsigned int linecard_index,
 	linecard->priv = priv;
 	linecard->state = DEVLINK_LINECARD_STATE_UNPROVISIONED;
 	mutex_init(&linecard->state_lock);
+	INIT_LIST_HEAD(&linecard->device_list);
 
 	err = devlink_linecard_types_init(linecard);
 	if (err) {
@@ -10291,6 +10347,7 @@ void devlink_linecard_destroy(struct devlink_linecard *linecard)
 	struct devlink *devlink = linecard->devlink;
 
 	devlink_linecard_notify(linecard, DEVLINK_CMD_LINECARD_DEL);
+	WARN_ON(!list_empty(&linecard->device_list));
 	mutex_lock(&devlink->linecards_lock);
 	list_del(&linecard->list);
 	devlink_linecard_types_fini(linecard);
@@ -10299,6 +10356,50 @@ void devlink_linecard_destroy(struct devlink_linecard *linecard)
 }
 EXPORT_SYMBOL_GPL(devlink_linecard_destroy);
 
+/**
+ *	devlink_linecard_device_create - Create a device on linecard
+ *
+ *	@linecard: devlink linecard
+ *	@device_index: index of the linecard device
+ *
+ *	Return: Line card device structure or an ERR_PTR() encoded error code.
+ */
+struct devlink_linecard_device *
+devlink_linecard_device_create(struct devlink_linecard *linecard,
+			       unsigned int device_index)
+{
+	struct devlink_linecard_device *linecard_device;
+
+	linecard_device = kzalloc(sizeof(*linecard_device), GFP_KERNEL);
+	if (!linecard_device)
+		return ERR_PTR(-ENOMEM);
+	linecard_device->index = device_index;
+	mutex_lock(&linecard->state_lock);
+	list_add_tail(&linecard_device->list, &linecard->device_list);
+	devlink_linecard_notify(linecard, DEVLINK_CMD_LINECARD_NEW);
+	mutex_unlock(&linecard->state_lock);
+	return linecard_device;
+}
+EXPORT_SYMBOL_GPL(devlink_linecard_device_create);
+
+/**
+ *	devlink_linecard_device_destroy - Destroy device on linecard
+ *
+ *	@linecard: devlink linecard
+ *	@linecard_device: devlink linecard device
+ */
+void
+devlink_linecard_device_destroy(struct devlink_linecard *linecard,
+				struct devlink_linecard_device *linecard_device)
+{
+	mutex_lock(&linecard->state_lock);
+	list_del(&linecard_device->list);
+	devlink_linecard_notify(linecard, DEVLINK_CMD_LINECARD_NEW);
+	mutex_unlock(&linecard->state_lock);
+	kfree(linecard_device);
+}
+EXPORT_SYMBOL_GPL(devlink_linecard_device_destroy);
+
 /**
  *	devlink_linecard_provision_set - Set provisioning on linecard
  *
@@ -10331,6 +10432,7 @@ EXPORT_SYMBOL_GPL(devlink_linecard_provision_set);
 void devlink_linecard_provision_clear(struct devlink_linecard *linecard)
 {
 	mutex_lock(&linecard->state_lock);
+	WARN_ON(!list_empty(&linecard->device_list));
 	linecard->state = DEVLINK_LINECARD_STATE_UNPROVISIONED;
 	linecard->type = NULL;
 	devlink_linecard_notify(linecard, DEVLINK_CMD_LINECARD_NEW);
-- 
2.33.1

[PATCH net-next 02/11] devlink: introduce line card info get message

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Allow the driver to provide per line card info get op to fill-up info,
similar to the "devlink dev info".

Example:

$ devlink lc info pci/0000:01:00.0 lc 8
pci/0000:01:00.0:
  lc 8
    versions:
        fixed:
          hw.revision 0
        running:
          ini.version 4

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../networking/devlink/devlink-linecard.rst   |   4 +
 include/net/devlink.h                         |   7 +-
 include/uapi/linux/devlink.h                  |   2 +
 net/core/devlink.c                            | 130 +++++++++++++++++-
 4 files changed, 138 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/devlink/devlink-linecard.rst b/Documentation/networking/devlink/devlink-linecard.rst
index 6c0b8928bc13..5a8d5989702a 100644
--- a/Documentation/networking/devlink/devlink-linecard.rst
+++ b/Documentation/networking/devlink/devlink-linecard.rst
@@ -14,6 +14,7 @@ system. Following operations are provided:
   * Get a list of supported line card types.
   * Provision of a slot with specific line card type.
   * Get and monitor of line card state and its change.
+  * Get information about line card versions.
 
 Line card according to the type may contain one or more gearboxes
 to mux the lanes with certain speed to multiple ports with lanes
@@ -120,3 +121,6 @@ Example usage
 
     # Set slot 8 to be unprovisioned:
     $ devlink lc set pci/0000:01:00.0 lc 8 notype
+
+    # Set info for slot 8:
+    $ devlink lc info pci/0000:01:00.0 lc 8
diff --git a/include/net/devlink.h b/include/net/devlink.h
index c84b52fb9ff0..f96dcb376630 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -150,6 +150,8 @@ struct devlink_port_new_attrs {
 	   sfnum_valid:1;
 };
 
+struct devlink_info_req;
+
 /**
  * struct devlink_linecard_ops - Linecard operations
  * @provision: callback to provision the linecard slot with certain
@@ -168,6 +170,7 @@ struct devlink_port_new_attrs {
  *                  provisioned.
  * @types_count: callback to get number of supported types
  * @types_get: callback to get next type in list
+ * @info_get: callback to get linecard info
  */
 struct devlink_linecard_ops {
 	int (*provision)(struct devlink_linecard *linecard, void *priv,
@@ -182,6 +185,9 @@ struct devlink_linecard_ops {
 	void (*types_get)(struct devlink_linecard *linecard,
 			  void *priv, unsigned int index, const char **type,
 			  const void **type_priv);
+	int (*info_get)(struct devlink_linecard *linecard, void *priv,
+			struct devlink_info_req *req,
+			struct netlink_ext_ack *extack);
 };
 
 struct devlink_sb_pool_info {
@@ -628,7 +634,6 @@ struct devlink_flash_update_params {
 #define DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK	BIT(1)
 
 struct devlink_region;
-struct devlink_info_req;
 
 /**
  * struct devlink_region_ops - Region operations
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index cd578645f94f..fb8c3864457f 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -136,6 +136,8 @@ enum devlink_command {
 	DEVLINK_CMD_LINECARD_NEW,
 	DEVLINK_CMD_LINECARD_DEL,
 
+	DEVLINK_CMD_LINECARD_INFO_GET,		/* can dump */
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 41d9631ceada..5facd10de64a 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2424,6 +2424,125 @@ static int devlink_nl_cmd_linecard_set_doit(struct sk_buff *skb,
 	return 0;
 }
 
+struct devlink_info_req {
+	struct sk_buff *msg;
+};
+
+static int
+devlink_nl_linecard_info_fill(struct sk_buff *msg, struct devlink *devlink,
+			      struct devlink_linecard *linecard,
+			      enum devlink_command cmd, u32 portid,
+			      u32 seq, int flags, struct netlink_ext_ack *extack)
+{
+	struct devlink_info_req req;
+	void *hdr;
+	int err;
+
+	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	err = -EMSGSIZE;
+	if (devlink_nl_put_handle(msg, devlink))
+		goto nla_put_failure;
+	if (nla_put_u32(msg, DEVLINK_ATTR_LINECARD_INDEX, linecard->index))
+		goto nla_put_failure;
+
+	req.msg = msg;
+	err = linecard->ops->info_get(linecard, linecard->priv, &req, extack);
+	if (err)
+		goto nla_put_failure;
+
+	genlmsg_end(msg, hdr);
+	return 0;
+
+nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+	return err;
+}
+
+static int devlink_nl_cmd_linecard_info_get_doit(struct sk_buff *skb,
+						 struct genl_info *info)
+{
+	struct devlink_linecard *linecard = info->user_ptr[1];
+	struct devlink *devlink = linecard->devlink;
+	struct sk_buff *msg;
+	int err;
+
+	if (!linecard->ops->info_get)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	mutex_lock(&linecard->state_lock);
+	err = devlink_nl_linecard_info_fill(msg, devlink, linecard,
+					    DEVLINK_CMD_LINECARD_INFO_GET,
+					    info->snd_portid, info->snd_seq, 0,
+					    info->extack);
+	mutex_unlock(&linecard->state_lock);
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
+static int devlink_nl_cmd_linecard_info_get_dumpit(struct sk_buff *msg,
+						   struct netlink_callback *cb)
+{
+	struct devlink_linecard *linecard;
+	struct devlink *devlink;
+	int start = cb->args[0];
+	unsigned long index;
+	int idx = 0;
+	int err = 0;
+
+	mutex_lock(&devlink_mutex);
+	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
+		if (!devlink_try_get(devlink))
+			continue;
+
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			goto retry;
+
+		mutex_lock(&devlink->linecards_lock);
+		list_for_each_entry(linecard, &devlink->linecard_list, list) {
+			if (idx < start || !linecard->ops->info_get) {
+				idx++;
+				continue;
+			}
+			mutex_lock(&linecard->state_lock);
+			err = devlink_nl_linecard_info_fill(msg, devlink, linecard,
+							    DEVLINK_CMD_LINECARD_INFO_GET,
+							    NETLINK_CB(cb->skb).portid,
+							    cb->nlh->nlmsg_seq,
+							    NLM_F_MULTI,
+							    cb->extack);
+			mutex_unlock(&linecard->state_lock);
+			if (err) {
+				mutex_unlock(&devlink->linecards_lock);
+				devlink_put(devlink);
+				goto out;
+			}
+			idx++;
+		}
+		mutex_unlock(&devlink->linecards_lock);
+retry:
+		devlink_put(devlink);
+	}
+out:
+	mutex_unlock(&devlink_mutex);
+
+	if (err != -EMSGSIZE)
+		return err;
+
+	cb->args[0] = idx;
+	return msg->len;
+}
+
 static int devlink_nl_sb_fill(struct sk_buff *msg, struct devlink *devlink,
 			      struct devlink_sb *devlink_sb,
 			      enum devlink_command cmd, u32 portid,
@@ -6416,10 +6535,6 @@ 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)
 {
 	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_DRIVER_NAME, name);
@@ -9139,6 +9254,13 @@ static const struct genl_small_ops devlink_nl_ops[] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_LINECARD,
 	},
+	{
+		.cmd = DEVLINK_CMD_LINECARD_INFO_GET,
+		.doit = devlink_nl_cmd_linecard_info_get_doit,
+		.dumpit = devlink_nl_cmd_linecard_info_get_dumpit,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_LINECARD,
+		/* can be retrieved by unprivileged users */
+	},
 	{
 		.cmd = DEVLINK_CMD_SB_GET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-- 
2.33.1

[PATCH net-next 03/11] devlink: introduce line card device info infrastructure

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Extend the line card info message with information (e.g., FW version)
about devices found on the line card.

Example:
$ devlink lc info pci/0000:01:00.0 lc 8
pci/0000:01:00.0:
  lc 8
    versions:
        fixed:
          hw.revision 0
        running:
          ini.version 4
    devices:
      device 0
        versions:
            running:
              fw 19.2010.1310
      device 1
        versions:
            running:
              fw 19.2010.1310
      device 2
        versions:
            running:
              fw 19.2010.1310
      device 3
        versions:
            running:
              fw 19.2010.1310

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../networking/devlink/devlink-linecard.rst   |  2 +-
 include/net/devlink.h                         |  8 ++-
 net/core/devlink.c                            | 71 ++++++++++++++++++-
 3 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/devlink/devlink-linecard.rst b/Documentation/networking/devlink/devlink-linecard.rst
index 5a8d5989702a..a98b468ad479 100644
--- a/Documentation/networking/devlink/devlink-linecard.rst
+++ b/Documentation/networking/devlink/devlink-linecard.rst
@@ -14,7 +14,7 @@ system. Following operations are provided:
   * Get a list of supported line card types.
   * Provision of a slot with specific line card type.
   * Get and monitor of line card state and its change.
-  * Get information about line card versions.
+  * Get information about line card versions and devices.
 
 Line card according to the type may contain one or more gearboxes
 to mux the lanes with certain speed to multiple ports with lanes
diff --git a/include/net/devlink.h b/include/net/devlink.h
index f96dcb376630..062895973656 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -151,6 +151,7 @@ struct devlink_port_new_attrs {
 };
 
 struct devlink_info_req;
+struct devlink_linecard_device;
 
 /**
  * struct devlink_linecard_ops - Linecard operations
@@ -171,6 +172,7 @@ struct devlink_info_req;
  * @types_count: callback to get number of supported types
  * @types_get: callback to get next type in list
  * @info_get: callback to get linecard info
+ * @device_info_get: callback to get linecard device info
  */
 struct devlink_linecard_ops {
 	int (*provision)(struct devlink_linecard *linecard, void *priv,
@@ -188,6 +190,9 @@ struct devlink_linecard_ops {
 	int (*info_get)(struct devlink_linecard *linecard, void *priv,
 			struct devlink_info_req *req,
 			struct netlink_ext_ack *extack);
+	int (*device_info_get)(struct devlink_linecard_device *device,
+			       void *priv, struct devlink_info_req *req,
+			       struct netlink_ext_ack *extack);
 };
 
 struct devlink_sb_pool_info {
@@ -1583,10 +1588,9 @@ struct devlink_linecard *
 devlink_linecard_create(struct devlink *devlink, unsigned int linecard_index,
 			const struct devlink_linecard_ops *ops, void *priv);
 void devlink_linecard_destroy(struct devlink_linecard *linecard);
-struct devlink_linecard_device;
 struct devlink_linecard_device *
 devlink_linecard_device_create(struct devlink_linecard *linecard,
-			       unsigned int device_index);
+			       unsigned int device_index, void *priv);
 void
 devlink_linecard_device_destroy(struct devlink_linecard *linecard,
 				struct devlink_linecard_device *linecard_device);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 5facd10de64a..5f441a0e34f4 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2062,6 +2062,7 @@ struct devlink_linecard_type {
 struct devlink_linecard_device {
 	struct list_head list;
 	unsigned int index;
+	void *priv;
 };
 
 static int
@@ -2428,6 +2429,68 @@ struct devlink_info_req {
 	struct sk_buff *msg;
 };
 
+static int
+devlink_nl_linecard_device_info_fill(struct sk_buff *msg,
+				     struct devlink_linecard *linecard,
+				     struct devlink_linecard_device *linecard_device,
+				     struct netlink_ext_ack *extack)
+{
+	struct nlattr *attr;
+
+	attr = nla_nest_start(msg, DEVLINK_ATTR_LINECARD_DEVICE);
+	if (!attr)
+		return -EMSGSIZE;
+	if (nla_put_u32(msg, DEVLINK_ATTR_LINECARD_DEVICE_INDEX,
+			linecard_device->index)) {
+		nla_nest_cancel(msg, attr);
+		return -EMSGSIZE;
+	}
+	if (linecard->ops->device_info_get) {
+		struct devlink_info_req req;
+		int err;
+
+		req.msg = msg;
+		err = linecard->ops->device_info_get(linecard_device,
+						     linecard_device->priv,
+						     &req, extack);
+		if (err) {
+			nla_nest_cancel(msg, attr);
+			return err;
+		}
+	}
+	nla_nest_end(msg, attr);
+
+	return 0;
+}
+
+static int devlink_nl_linecard_devices_info_fill(struct sk_buff *msg,
+						 struct devlink_linecard *linecard,
+						 struct netlink_ext_ack *extack)
+{
+	struct devlink_linecard_device *linecard_device;
+	struct nlattr *attr;
+	int err;
+
+	if (list_empty(&linecard->device_list))
+		return 0;
+
+	attr = nla_nest_start(msg, DEVLINK_ATTR_LINECARD_DEVICE_LIST);
+	if (!attr)
+		return -EMSGSIZE;
+	list_for_each_entry(linecard_device, &linecard->device_list, list) {
+		err = devlink_nl_linecard_device_info_fill(msg, linecard,
+							   linecard_device,
+							   extack);
+		if (err) {
+			nla_nest_cancel(msg, attr);
+			return err;
+		}
+	}
+	nla_nest_end(msg, attr);
+
+	return 0;
+}
+
 static int
 devlink_nl_linecard_info_fill(struct sk_buff *msg, struct devlink *devlink,
 			      struct devlink_linecard *linecard,
@@ -2453,6 +2516,10 @@ devlink_nl_linecard_info_fill(struct sk_buff *msg, struct devlink *devlink,
 	if (err)
 		goto nla_put_failure;
 
+	err = devlink_nl_linecard_devices_info_fill(msg, linecard, extack);
+	if (err)
+		goto nla_put_failure;
+
 	genlmsg_end(msg, hdr);
 	return 0;
 
@@ -10483,12 +10550,13 @@ EXPORT_SYMBOL_GPL(devlink_linecard_destroy);
  *
  *	@linecard: devlink linecard
  *	@device_index: index of the linecard device
+ *	@priv: user priv pointer
  *
  *	Return: Line card device structure or an ERR_PTR() encoded error code.
  */
 struct devlink_linecard_device *
 devlink_linecard_device_create(struct devlink_linecard *linecard,
-			       unsigned int device_index)
+			       unsigned int device_index, void *priv)
 {
 	struct devlink_linecard_device *linecard_device;
 
@@ -10496,6 +10564,7 @@ devlink_linecard_device_create(struct devlink_linecard *linecard,
 	if (!linecard_device)
 		return ERR_PTR(-ENOMEM);
 	linecard_device->index = device_index;
+	linecard_device->priv = priv;
 	mutex_lock(&linecard->state_lock);
 	list_add_tail(&linecard_device->list, &linecard->device_list);
 	devlink_linecard_notify(linecard, DEVLINK_CMD_LINECARD_NEW);
-- 
2.33.1

[PATCH net-next 04/11] mlxsw: reg: Extend MDDQ by device_info

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Extend existing MDDQ register by possibility to query information about
devices residing on a line card.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlxsw/reg.h | 62 ++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/reg.h b/drivers/net/ethernet/mellanox/mlxsw/reg.h
index 23589d3b160a..521c1b195a3e 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/reg.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/reg.h
@@ -11643,7 +11643,11 @@ MLXSW_ITEM32(reg, mddq, sie, 0x00, 31, 1);
 
 enum mlxsw_reg_mddq_query_type {
 	MLXSW_REG_MDDQ_QUERY_TYPE_SLOT_INFO = 1,
-	MLXSW_REG_MDDQ_QUERY_TYPE_SLOT_NAME = 3,
+	MLXSW_REG_MDDQ_QUERY_TYPE_DEVICE_INFO, /* If there are no devices
+						* on the slot, data_valid
+						* will be '0'.
+						*/
+	MLXSW_REG_MDDQ_QUERY_TYPE_SLOT_NAME,
 };
 
 /* reg_mddq_query_type
@@ -11657,6 +11661,28 @@ MLXSW_ITEM32(reg, mddq, query_type, 0x00, 16, 8);
  */
 MLXSW_ITEM32(reg, mddq, slot_index, 0x00, 0, 4);
 
+/* reg_mddq_response_msg_seq
+ * Response message sequential number. For a specific request, the response
+ * message sequential number is the following one. In addition, the last
+ * message should be 0.
+ * Access: RO
+ */
+MLXSW_ITEM32(reg, mddq, response_msg_seq, 0x04, 16, 8);
+
+/* reg_mddq_request_msg_seq
+ * Request message sequential number.
+ * The first message number should be 0.
+ * Access: Index
+ */
+MLXSW_ITEM32(reg, mddq, request_msg_seq, 0x04, 0, 8);
+
+/* reg_mddq_data_valid
+ * If set, the data in the data field is valid and contain the information
+ * for the queried index.
+ * Access: RO
+ */
+MLXSW_ITEM32(reg, mddq, data_valid, 0x08, 31, 1);
+
 /* reg_mddq_slot_info_provisioned
  * If set, the INI file is applied and the card is provisioned.
  * Access: RO
@@ -11743,6 +11769,40 @@ mlxsw_reg_mddq_slot_info_unpack(const char *payload, u8 *p_slot_index,
 	*p_card_type = mlxsw_reg_mddq_slot_info_card_type_get(payload);
 }
 
+/* reg_mddq_device_info_flash_owner
+ * If set, the device is the flash owner. Otherwise, a shared flash
+ * is used by this device (another device is the flash owner).
+ * Access: RO
+ */
+MLXSW_ITEM32(reg, mddq, device_info_flash_owner, 0x10, 30, 1);
+
+/* reg_mddq_device_info_device_index
+ * Device index. The first device should number 0.
+ * Access: RO
+ */
+MLXSW_ITEM32(reg, mddq, device_info_device_index, 0x10, 0, 8);
+
+static inline void
+mlxsw_reg_mddq_device_info_pack(char *payload, u8 slot_index,
+				u8 request_msg_seq)
+{
+	__mlxsw_reg_mddq_pack(payload, slot_index,
+			      MLXSW_REG_MDDQ_QUERY_TYPE_DEVICE_INFO);
+	mlxsw_reg_mddq_request_msg_seq_set(payload, request_msg_seq);
+}
+
+static inline void
+mlxsw_reg_mddq_device_info_unpack(const char *payload, u8 *p_response_msg_seq,
+				  bool *p_data_valid, bool *p_flash_owner,
+				  u8 *p_device_index)
+{
+	*p_response_msg_seq = mlxsw_reg_mddq_response_msg_seq_get(payload);
+	*p_data_valid = mlxsw_reg_mddq_data_valid_get(payload);
+	if (p_flash_owner)
+		*p_flash_owner = mlxsw_reg_mddq_device_info_flash_owner_get(payload);
+	*p_device_index = mlxsw_reg_mddq_device_info_device_index_get(payload);
+}
+
 #define MLXSW_REG_MDDQ_SLOT_ASCII_NAME_LEN 20
 
 /* reg_mddq_slot_ascii_name
-- 
2.33.1

[PATCH net-next 05/11] mlxsw: core_linecards: Probe provisioned line cards for devices and attach them

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

In case the line card is provisioned, go over all possible existing
devices (gearboxes) on it and attach them, so devlink core is aware of
them.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.h    |  1 +
 .../ethernet/mellanox/mlxsw/core_linecards.c  | 99 +++++++++++++++++++
 2 files changed, 100 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index c2a891287047..d008282d7f2e 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -581,6 +581,7 @@ struct mlxsw_linecard {
 	   active:1;
 	u16 hw_revision;
 	u16 ini_version;
+	struct list_head device_list;
 };
 
 struct mlxsw_linecard_types_info;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
index 5c9869dcf674..9dd8a56add4a 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
@@ -87,11 +87,101 @@ static const char *mlxsw_linecard_type_name(struct mlxsw_linecard *linecard)
 	return linecard->name;
 }
 
+struct mlxsw_linecard_device {
+	struct list_head list;
+	u8 index;
+	struct mlxsw_linecard *linecard;
+	struct devlink_linecard_device *devlink_device;
+};
+
+static int mlxsw_linecard_device_attach(struct mlxsw_core *mlxsw_core,
+					struct mlxsw_linecard *linecard,
+					u8 device_index, bool flash_owner)
+{
+	struct mlxsw_linecard_device *device;
+	int err;
+
+	device = kzalloc(sizeof(*device), GFP_KERNEL);
+	if (!device)
+		return -ENOMEM;
+	device->index = device_index;
+	device->linecard = linecard;
+
+	device->devlink_device = devlink_linecard_device_create(linecard->devlink_linecard,
+								device_index, NULL);
+	if (IS_ERR(device->devlink_device)) {
+		err = PTR_ERR(device->devlink_device);
+		goto err_devlink_linecard_device_attach;
+	}
+
+	list_add_tail(&device->list, &linecard->device_list);
+	return 0;
+
+err_devlink_linecard_device_attach:
+	kfree(device);
+	return err;
+}
+
+static void mlxsw_linecard_device_detach(struct mlxsw_core *mlxsw_core,
+					 struct mlxsw_linecard *linecard,
+					 struct mlxsw_linecard_device *device)
+{
+	list_del(&device->list);
+	devlink_linecard_device_destroy(linecard->devlink_linecard,
+					device->devlink_device);
+	kfree(device);
+}
+
+static void mlxsw_linecard_devices_detach(struct mlxsw_linecard *linecard)
+{
+	struct mlxsw_core *mlxsw_core = linecard->linecards->mlxsw_core;
+	struct mlxsw_linecard_device *device, *tmp;
+
+	list_for_each_entry_safe(device, tmp, &linecard->device_list, list)
+		mlxsw_linecard_device_detach(mlxsw_core, linecard, device);
+}
+
+static int mlxsw_linecard_devices_attach(struct mlxsw_linecard *linecard)
+{
+	struct mlxsw_core *mlxsw_core = linecard->linecards->mlxsw_core;
+	u8 msg_seq = 0;
+	int err;
+
+	do {
+		char mddq_pl[MLXSW_REG_MDDQ_LEN];
+		bool flash_owner;
+		bool data_valid;
+		u8 device_index;
+
+		mlxsw_reg_mddq_device_info_pack(mddq_pl, linecard->slot_index,
+						msg_seq);
+		err = mlxsw_reg_query(mlxsw_core, MLXSW_REG(mddq), mddq_pl);
+		if (err)
+			return err;
+		mlxsw_reg_mddq_device_info_unpack(mddq_pl, &msg_seq,
+						  &data_valid, &flash_owner,
+						  &device_index);
+		if (!data_valid)
+			break;
+		err = mlxsw_linecard_device_attach(mlxsw_core, linecard,
+						   device_index, flash_owner);
+		if (err)
+			goto rollback;
+	} while (msg_seq);
+
+	return 0;
+
+rollback:
+	mlxsw_linecard_devices_detach(linecard);
+	return err;
+}
+
 static void mlxsw_linecard_provision_fail(struct mlxsw_linecard *linecard)
 {
 	linecard->provisioned = false;
 	linecard->ready = false;
 	linecard->active = false;
+	mlxsw_linecard_devices_detach(linecard);
 	devlink_linecard_provision_fail(linecard->devlink_linecard);
 }
 
@@ -232,6 +322,7 @@ mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
 {
 	struct mlxsw_linecards *linecards = linecard->linecards;
 	const char *type;
+	int err;
 
 	type = mlxsw_linecard_types_lookup(linecards, card_type);
 	mlxsw_linecard_status_event_done(linecard,
@@ -249,6 +340,11 @@ mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
 			return PTR_ERR(type);
 		}
 	}
+	err = mlxsw_linecard_devices_attach(linecard);
+	if (err) {
+		mlxsw_linecard_provision_fail(linecard);
+		return err;
+	}
 	linecard->provisioned = true;
 	linecard->hw_revision = hw_revision;
 	linecard->ini_version = ini_version;
@@ -261,6 +357,7 @@ static void mlxsw_linecard_provision_clear(struct mlxsw_linecard *linecard)
 	mlxsw_linecard_status_event_done(linecard,
 					 MLXSW_LINECARD_STATUS_EVENT_TYPE_UNPROVISION);
 	linecard->provisioned = false;
+	mlxsw_linecard_devices_detach(linecard);
 	devlink_linecard_provision_clear(linecard->devlink_linecard);
 }
 
@@ -840,6 +937,7 @@ static int mlxsw_linecard_init(struct mlxsw_core *mlxsw_core,
 	linecard->slot_index = slot_index;
 	linecard->linecards = linecards;
 	mutex_init(&linecard->lock);
+	INIT_LIST_HEAD(&linecard->device_list);
 
 	devlink_linecard = devlink_linecard_create(priv_to_devlink(mlxsw_core),
 						   slot_index, &mlxsw_linecard_ops,
@@ -885,6 +983,7 @@ static void mlxsw_linecard_fini(struct mlxsw_core *mlxsw_core,
 	mlxsw_core_flush_owq();
 	if (linecard->active)
 		mlxsw_linecard_active_clear(linecard);
+	mlxsw_linecard_devices_detach(linecard);
 	devlink_linecard_destroy(linecard->devlink_linecard);
 	mutex_destroy(&linecard->lock);
 }
-- 
2.33.1

[PATCH net-next 06/11] selftests: mlxsw: Check devices on provisioned line card

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Once line card is provisioned, check the count of devices on it and
print them out.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../drivers/net/mlxsw/devlink_linecard.sh     | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh b/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh
index 08a922d8b86a..67b0e56cb413 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh
@@ -152,6 +152,7 @@ unprovision_test()
 
 LC_16X100G_TYPE="16x100G"
 LC_16X100G_PORT_COUNT=16
+LC_16X100G_DEVICE_COUNT=4
 
 supported_types_check()
 {
@@ -177,6 +178,26 @@ supported_types_check()
 	check_err $? "16X100G not found between supported types of linecard $lc"
 }
 
+lc_devices_check()
+{
+	local lc=$1
+	local expected_device_count=$2
+	local device_count
+	local device
+
+	device_count=$(devlink lc show $DEVLINK_DEV lc $lc -j | \
+		       jq -e -r ".[][][].devices |length")
+	check_err $? "Failed to get linecard $lc device count"
+	[ $device_count != 0 ]
+	check_err $? "No device found on linecard $lc"
+	[ $device_count == $expected_device_count ]
+	check_err $? "Unexpected device count on linecard $lc (got $expected_device_count, expected $device_count)"
+	for (( device=0; device<device_count; device++ ))
+	do
+		log_info "Linecard $lc device $device"
+	done
+}
+
 ports_check()
 {
 	local lc=$1
@@ -206,6 +227,7 @@ provision_test()
 		unprovision_one $lc
 	fi
 	provision_one $lc $LC_16X100G_TYPE
+	lc_devices_check $lc $LC_16X100G_DEVICE_COUNT
 	ports_check $lc $LC_16X100G_PORT_COUNT
 	log_test "Provision"
 }
-- 
2.33.1

[PATCH net-next 07/11] mlxsw: core_linecards: Expose HW revision and INI version

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Implement info_get() to expose HW revision of a linecard and loaded INI
version.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/devlink/mlxsw.rst    | 18 +++++++++++
 .../ethernet/mellanox/mlxsw/core_linecards.c  | 31 +++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/Documentation/networking/devlink/mlxsw.rst b/Documentation/networking/devlink/mlxsw.rst
index cf857cb4ba8f..da1fbb265a11 100644
--- a/Documentation/networking/devlink/mlxsw.rst
+++ b/Documentation/networking/devlink/mlxsw.rst
@@ -58,6 +58,24 @@ The ``mlxsw`` driver reports the following versions
      - running
      - Three digit firmware version
 
+Line card info versions
+=======================
+
+The ``mlxsw`` driver reports the following versions for line cards
+
+.. list-table:: devlink line card info versions implemented
+   :widths: 5 5 90
+
+   * - Name
+     - Type
+     - Description
+   * - ``hw.revision``
+     - fixed
+     - The hardware revision for this line card
+   * - ``ini.version``
+     - running
+     - Version of line card INI loaded
+
 Driver-specific Traps
 =====================
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
index 9dd8a56add4a..b5f5b31bd31e 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
@@ -834,12 +834,43 @@ static void mlxsw_linecard_types_get(struct devlink_linecard *devlink_linecard,
 	*type_priv = ini_file;
 }
 
+static int
+mlxsw_linecard_info_get(struct devlink_linecard *devlink_linecard, void *priv,
+			struct devlink_info_req *req,
+			struct netlink_ext_ack *extack)
+{
+	struct mlxsw_linecard *linecard = priv;
+	char buf[32];
+	int err;
+
+	mutex_lock(&linecard->lock);
+	if (!linecard->provisioned) {
+		err = 0;
+		goto unlock;
+	}
+
+	sprintf(buf, "%d", linecard->hw_revision);
+	err = devlink_info_version_fixed_put(req, "hw.revision", buf);
+	if (err)
+		goto unlock;
+
+	sprintf(buf, "%d", linecard->ini_version);
+	err = devlink_info_version_running_put(req, "ini.version", buf);
+	if (err)
+		goto unlock;
+
+unlock:
+	mutex_unlock(&linecard->lock);
+	return err;
+}
+
 static const struct devlink_linecard_ops mlxsw_linecard_ops = {
 	.provision = mlxsw_linecard_provision,
 	.unprovision = mlxsw_linecard_unprovision,
 	.same_provision = mlxsw_linecard_same_provision,
 	.types_count = mlxsw_linecard_types_count,
 	.types_get = mlxsw_linecard_types_get,
+	.info_get = mlxsw_linecard_info_get,
 };
 
 struct mlxsw_linecard_status_event {
-- 
2.33.1

[PATCH net-next 08/11] selftests: mlxsw: Check line card info on provisioned line card

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Once line card is provisioned, check if HW revision and INI version
are exposed.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../drivers/net/mlxsw/devlink_linecard.sh       | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh b/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh
index 67b0e56cb413..04bedd98eb8b 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh
@@ -178,6 +178,22 @@ supported_types_check()
 	check_err $? "16X100G not found between supported types of linecard $lc"
 }
 
+lc_info_check()
+{
+	local lc=$1
+	local fixed_hw_revision
+	local running_ini_version
+
+	fixed_hw_revision=$(devlink lc -v info $DEVLINK_DEV lc $lc -j | \
+			    jq -e -r '.[][][].versions.fixed."hw.revision"')
+	check_err $? "Failed to get linecard $lc fixed.hw.revision"
+	log_info "Linecard $lc fixed.hw.revision: \"$fixed_hw_revision\""
+	running_ini_version=$(devlink lc -v info $DEVLINK_DEV lc $lc -j | \
+			      jq -e -r '.[][][].versions.running."ini.version"')
+	check_err $? "Failed to get linecard $lc running.ini.version"
+	log_info "Linecard $lc running.ini.version: \"$running_ini_version\""
+}
+
 lc_devices_check()
 {
 	local lc=$1
@@ -228,6 +244,7 @@ provision_test()
 	fi
 	provision_one $lc $LC_16X100G_TYPE
 	lc_devices_check $lc $LC_16X100G_DEVICE_COUNT
+	lc_info_check $lc
 	ports_check $lc $LC_16X100G_PORT_COUNT
 	log_test "Provision"
 }
-- 
2.33.1

[PATCH net-next 09/11] mlxsw: reg: Extend MDDQ device_info by FW version fields

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Add FW version fields to MDDQ device_info.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../ethernet/mellanox/mlxsw/core_linecards.c  |  3 ++-
 drivers/net/ethernet/mellanox/mlxsw/reg.h     | 27 ++++++++++++++++++-
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
index b5f5b31bd31e..42fe93ac629d 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
@@ -160,7 +160,8 @@ static int mlxsw_linecard_devices_attach(struct mlxsw_linecard *linecard)
 			return err;
 		mlxsw_reg_mddq_device_info_unpack(mddq_pl, &msg_seq,
 						  &data_valid, &flash_owner,
-						  &device_index);
+						  &device_index, NULL,
+						  NULL, NULL);
 		if (!data_valid)
 			break;
 		err = mlxsw_linecard_device_attach(mlxsw_core, linecard,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/reg.h b/drivers/net/ethernet/mellanox/mlxsw/reg.h
index 521c1b195a3e..04c4d7a4bd83 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/reg.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/reg.h
@@ -11782,6 +11782,24 @@ MLXSW_ITEM32(reg, mddq, device_info_flash_owner, 0x10, 30, 1);
  */
 MLXSW_ITEM32(reg, mddq, device_info_device_index, 0x10, 0, 8);
 
+/* reg_mddq_device_info_fw_major
+ * Major FW version number.
+ * Access: RO
+ */
+MLXSW_ITEM32(reg, mddq, device_info_fw_major, 0x14, 16, 16);
+
+/* reg_mddq_device_info_fw_minor
+ * Minor FW version number.
+ * Access: RO
+ */
+MLXSW_ITEM32(reg, mddq, device_info_fw_minor, 0x18, 16, 16);
+
+/* reg_mddq_device_info_fw_sub_minor
+ * Sub-minor FW version number.
+ * Access: RO
+ */
+MLXSW_ITEM32(reg, mddq, device_info_fw_sub_minor, 0x18, 0, 16);
+
 static inline void
 mlxsw_reg_mddq_device_info_pack(char *payload, u8 slot_index,
 				u8 request_msg_seq)
@@ -11794,13 +11812,20 @@ mlxsw_reg_mddq_device_info_pack(char *payload, u8 slot_index,
 static inline void
 mlxsw_reg_mddq_device_info_unpack(const char *payload, u8 *p_response_msg_seq,
 				  bool *p_data_valid, bool *p_flash_owner,
-				  u8 *p_device_index)
+				  u8 *p_device_index, u16 *p_fw_major,
+				  u16 *p_fw_minor, u16 *p_fw_sub_minor)
 {
 	*p_response_msg_seq = mlxsw_reg_mddq_response_msg_seq_get(payload);
 	*p_data_valid = mlxsw_reg_mddq_data_valid_get(payload);
 	if (p_flash_owner)
 		*p_flash_owner = mlxsw_reg_mddq_device_info_flash_owner_get(payload);
 	*p_device_index = mlxsw_reg_mddq_device_info_device_index_get(payload);
+	if (p_fw_major)
+		*p_fw_major = mlxsw_reg_mddq_device_info_fw_major_get(payload);
+	if (p_fw_minor)
+		*p_fw_minor = mlxsw_reg_mddq_device_info_fw_minor_get(payload);
+	if (p_fw_sub_minor)
+		*p_fw_sub_minor = mlxsw_reg_mddq_device_info_fw_sub_minor_get(payload);
 }
 
 #define MLXSW_REG_MDDQ_SLOT_ASCII_NAME_LEN 20
-- 
2.33.1

[PATCH net-next 10/11] mlxsw: core_linecards: Expose device FW version over device info

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Extend MDDQ to obtain FW version of line card device and implement
device_info_get() op to fill up the info with that.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/devlink/mlxsw.rst    |  15 +++
 .../ethernet/mellanox/mlxsw/core_linecards.c  | 108 +++++++++++++++++-
 2 files changed, 119 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/devlink/mlxsw.rst b/Documentation/networking/devlink/mlxsw.rst
index da1fbb265a11..0af345680510 100644
--- a/Documentation/networking/devlink/mlxsw.rst
+++ b/Documentation/networking/devlink/mlxsw.rst
@@ -76,6 +76,21 @@ The ``mlxsw`` driver reports the following versions for line cards
      - running
      - Version of line card INI loaded
 
+Line card device info versions
+==============================
+
+The ``mlxsw`` driver reports the following versions for line card devices
+
+.. list-table:: devlink line card device info versions implemented
+   :widths: 5 5 90
+
+   * - Name
+     - Type
+     - Description
+   * - ``fw.version``
+     - running
+     - Three digit firmware version
+
 Driver-specific Traps
 =====================
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
index 42fe93ac629d..2abd31a62776 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
@@ -87,13 +87,31 @@ static const char *mlxsw_linecard_type_name(struct mlxsw_linecard *linecard)
 	return linecard->name;
 }
 
+struct mlxsw_linecard_device_info {
+	u16 fw_major;
+	u16 fw_minor;
+	u16 fw_sub_minor;
+};
+
 struct mlxsw_linecard_device {
 	struct list_head list;
 	u8 index;
 	struct mlxsw_linecard *linecard;
 	struct devlink_linecard_device *devlink_device;
+	struct mlxsw_linecard_device_info info;
 };
 
+static struct mlxsw_linecard_device *
+mlxsw_linecard_device_lookup(struct mlxsw_linecard *linecard, u8 index)
+{
+	struct mlxsw_linecard_device *device;
+
+	list_for_each_entry(device, &linecard->device_list, list)
+		if (device->index == index)
+			return device;
+	return NULL;
+}
+
 static int mlxsw_linecard_device_attach(struct mlxsw_core *mlxsw_core,
 					struct mlxsw_linecard *linecard,
 					u8 device_index, bool flash_owner)
@@ -108,7 +126,7 @@ static int mlxsw_linecard_device_attach(struct mlxsw_core *mlxsw_core,
 	device->linecard = linecard;
 
 	device->devlink_device = devlink_linecard_device_create(linecard->devlink_linecard,
-								device_index, NULL);
+								device_index, device);
 	if (IS_ERR(device->devlink_device)) {
 		err = PTR_ERR(device->devlink_device);
 		goto err_devlink_linecard_device_attach;
@@ -177,6 +195,77 @@ static int mlxsw_linecard_devices_attach(struct mlxsw_linecard *linecard)
 	return err;
 }
 
+static void mlxsw_linecard_device_update(struct mlxsw_linecard *linecard,
+					 u8 device_index,
+					 struct mlxsw_linecard_device_info *info)
+{
+	struct mlxsw_linecard_device *device;
+
+	device = mlxsw_linecard_device_lookup(linecard, device_index);
+	if (!device)
+		return;
+	device->info = *info;
+}
+
+static int mlxsw_linecard_devices_update(struct mlxsw_linecard *linecard)
+{
+	struct mlxsw_core *mlxsw_core = linecard->linecards->mlxsw_core;
+	u8 msg_seq = 0;
+
+	do {
+		struct mlxsw_linecard_device_info info;
+		char mddq_pl[MLXSW_REG_MDDQ_LEN];
+		bool data_valid;
+		u8 device_index;
+		int err;
+
+		mlxsw_reg_mddq_device_info_pack(mddq_pl, linecard->slot_index,
+						msg_seq);
+		err = mlxsw_reg_query(mlxsw_core, MLXSW_REG(mddq), mddq_pl);
+		if (err)
+			return err;
+		mlxsw_reg_mddq_device_info_unpack(mddq_pl, &msg_seq,
+						  &data_valid, NULL,
+						  &device_index,
+						  &info.fw_major,
+						  &info.fw_minor,
+						  &info.fw_sub_minor);
+		if (!data_valid)
+			break;
+		mlxsw_linecard_device_update(linecard, device_index, &info);
+	} while (msg_seq);
+
+	return 0;
+}
+
+static int
+mlxsw_linecard_device_info_get(struct devlink_linecard_device *devlink_linecard_device,
+			       void *priv, struct devlink_info_req *req,
+			       struct netlink_ext_ack *extack)
+{
+	struct mlxsw_linecard_device *device = priv;
+	struct mlxsw_linecard_device_info *info;
+	struct mlxsw_linecard *linecard;
+	char buf[32];
+
+	linecard = device->linecard;
+	mutex_lock(&linecard->lock);
+	if (!linecard->active) {
+		mutex_unlock(&linecard->lock);
+		return 0;
+	}
+
+	info = &device->info;
+
+	sprintf(buf, "%u.%u.%u", info->fw_major, info->fw_minor,
+		info->fw_sub_minor);
+	mutex_unlock(&linecard->lock);
+
+	return devlink_info_version_running_put(req,
+						DEVLINK_INFO_VERSION_GENERIC_FW,
+						buf);
+}
+
 static void mlxsw_linecard_provision_fail(struct mlxsw_linecard *linecard)
 {
 	linecard->provisioned = false;
@@ -390,11 +479,18 @@ static int mlxsw_linecard_ready_clear(struct mlxsw_linecard *linecard)
 	return 0;
 }
 
-static void mlxsw_linecard_active_set(struct mlxsw_linecard *linecard)
+static int mlxsw_linecard_active_set(struct mlxsw_linecard *linecard)
 {
+	int err;
+
+	err = mlxsw_linecard_devices_update(linecard);
+	if (err)
+		return err;
+
 	mlxsw_linecard_active_ops_call(linecard);
 	linecard->active = true;
 	devlink_linecard_activate(linecard->devlink_linecard);
+	return 0;
 }
 
 static void mlxsw_linecard_active_clear(struct mlxsw_linecard *linecard)
@@ -443,8 +539,11 @@ static int mlxsw_linecard_status_process(struct mlxsw_linecards *linecards,
 			goto out;
 	}
 
-	if (active && linecard->active != active)
-		mlxsw_linecard_active_set(linecard);
+	if (active && linecard->active != active) {
+		err = mlxsw_linecard_active_set(linecard);
+		if (err)
+			goto out;
+	}
 
 	if (!active && linecard->active != active)
 		mlxsw_linecard_active_clear(linecard);
@@ -872,6 +971,7 @@ static const struct devlink_linecard_ops mlxsw_linecard_ops = {
 	.types_count = mlxsw_linecard_types_count,
 	.types_get = mlxsw_linecard_types_get,
 	.info_get = mlxsw_linecard_info_get,
+	.device_info_get = mlxsw_linecard_device_info_get,
 };
 
 struct mlxsw_linecard_status_event {
-- 
2.33.1

[PATCH net-next 11/11] selftests: mlxsw: Check device info on activated line card

[Ido Schimmel]

From: Jiri Pirko <jiri@nvidia.com>

Once line card is activated, check the device FW version is exposed.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../drivers/net/mlxsw/devlink_linecard.sh     | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh b/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh
index 04bedd98eb8b..53a65f416770 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/devlink_linecard.sh
@@ -259,6 +259,26 @@ interface_check()
 	setup_wait
 }
 
+lc_devices_info_check()
+{
+	local lc=$1
+	local expected_device_count=$2
+	local device_count
+	local device
+	local running_device_fw
+
+	device_count=$(devlink lc info $DEVLINK_DEV lc $lc -j | \
+		       jq -e -r ".[][][].devices |length")
+	check_err $? "Failed to get linecard $lc device count"
+	for (( device=0; device<device_count; device++ ))
+	do
+		running_device_fw=$(devlink lc -v info $DEVLINK_DEV lc $lc -j | \
+				    jq -e -r ".[][][].devices[$device].versions.running.fw")
+		check_err $? "Failed to get linecard $lc device $device running fw version"
+		log_info "Linecard $lc device $device running.fw: \"$running_device_fw\""
+	done
+}
+
 activation_16x100G_test()
 {
 	RET=0
@@ -275,6 +295,8 @@ activation_16x100G_test()
 		$ACTIVATION_TIMEOUT)
 	check_err $? "Failed to get linecard $lc activated (timeout)"
 
+	lc_devices_info_check $lc $LC_16X100G_DEVICE_COUNT
+
 	interface_check
 
 	log_test "Activation 16x100G"
-- 
2.33.1

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon, 25 Apr 2022 06:44:20 +0300 you wrote:
> Jiri says:
> 
> This patchset is extending the line card model by three items:
> 1) line card devices
> 2) line card info
> 3) line card device info
> 
> [...]

Here is the summary with links:
  - [net-next,01/11] devlink: introduce line card devices support
    https://git.kernel.org/netdev/net-next/c/8d92e4fbcf0f
  - [net-next,02/11] devlink: introduce line card info get message
    https://git.kernel.org/netdev/net-next/c/276910aecc6a
  - [net-next,03/11] devlink: introduce line card device info infrastructure
    https://git.kernel.org/netdev/net-next/c/28b2d1f1ac41
  - [net-next,04/11] mlxsw: reg: Extend MDDQ by device_info
    https://git.kernel.org/netdev/net-next/c/798e2df5067c
  - [net-next,05/11] mlxsw: core_linecards: Probe provisioned line cards for devices and attach them
    https://git.kernel.org/netdev/net-next/c/8e2e10f65112
  - [net-next,06/11] selftests: mlxsw: Check devices on provisioned line card
    https://git.kernel.org/netdev/net-next/c/5e2229891825
  - [net-next,07/11] mlxsw: core_linecards: Expose HW revision and INI version
    https://git.kernel.org/netdev/net-next/c/3b37130f4855
  - [net-next,08/11] selftests: mlxsw: Check line card info on provisioned line card
    https://git.kernel.org/netdev/net-next/c/08682c9e58cd
  - [net-next,09/11] mlxsw: reg: Extend MDDQ device_info by FW version fields
    https://git.kernel.org/netdev/net-next/c/c38e9bf33812
  - [net-next,10/11] mlxsw: core_linecards: Expose device FW version over device info
    https://git.kernel.org/netdev/net-next/c/e932b4bdbd7c
  - [net-next,11/11] selftests: mlxsw: Check device info on activated line card
    https://git.kernel.org/netdev/net-next/c/002defd576a3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Mon, 25 Apr 2022 06:44:20 +0300 Ido Schimmel wrote:
> This patchset is extending the line card model by three items:
> 1) line card devices
> 2) line card info
> 3) line card device info
> 
> First three patches are introducing the necessary changes in devlink
> core.
> 
> Then, all three extensions are implemented in mlxsw alongside with
> selftest.

:/ what is a line card device? You must provide document what you're
doing, this:

 .../networking/devlink/devlink-linecard.rst   |   4 +

is not enough.

How many operations and attributes are you going to copy&paste?

Is linking devlink instances into a hierarchy a better approach?

Would you mind if I revert this? I don't understand why the line card
patches are applied in 6h on Sunday night, especially that RFCv1 had
quite a long discussion. But really any uAPI additions should warrant
longer review time, IMHO.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Ido Schimmel]

On Mon, Apr 25, 2022 at 09:00:21AM -0700, Jakub Kicinski wrote:
> On Mon, 25 Apr 2022 06:44:20 +0300 Ido Schimmel wrote:
> > This patchset is extending the line card model by three items:
> > 1) line card devices
> > 2) line card info
> > 3) line card device info
> > 
> > First three patches are introducing the necessary changes in devlink
> > core.
> > 
> > Then, all three extensions are implemented in mlxsw alongside with
> > selftest.
> 
> :/ what is a line card device? You must provide document what you're
> doing, this:
> 
>  .../networking/devlink/devlink-linecard.rst   |   4 +
> 
> is not enough.
> 
> How many operations and attributes are you going to copy&paste?
> 
> Is linking devlink instances into a hierarchy a better approach?

In this particular case, these devices are gearboxes. They are running
their own firmware and we want user space to be able to query and update
the running firmware version.

The idea (implemented in the next patchset) is to let these devices
expose their own "component name", which can then be plugged into the
existing flash command:

    $ devlink lc show pci/0000:01:00.0 lc 8
    pci/0000:01:00.0:
      lc 8 state active type 16x100G
        supported_types:
           16x100G
        devices:
          device 0 flashable true component lc8_dev0
          device 1 flashable false
          device 2 flashable false
          device 3 flashable false
    $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2 component lc8_dev0

Registering a separate devlink instance for these devices sounds like an
overkill to me. If you are not OK with a separate command (e.g.,
DEVLINK_CMD_LINECARD_INFO_GET), then extending DEVLINK_CMD_INFO_GET is
also an option. We discussed this during internal review, but felt that
the current approach is cleaner.

> Would you mind if I revert this?

I can't stop you, but keep in mind that it's already late here and that
tomorrow I'm AFK (reserve duty) and won't be able to tag it. Jiri should
be available to continue this discussion tomorrow morning, so probably
best to wait for his feedback.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Mon, 25 Apr 2022 22:39:57 +0300 Ido Schimmel wrote:
> > :/ what is a line card device? You must provide document what you're
> > doing, this:
> > 
> >  .../networking/devlink/devlink-linecard.rst   |   4 +
> > 
> > is not enough.
> > 
> > How many operations and attributes are you going to copy&paste?
> > 
> > Is linking devlink instances into a hierarchy a better approach?  
> 
> In this particular case, these devices are gearboxes. They are running
> their own firmware and we want user space to be able to query and update
> the running firmware version.

Nothing too special, then, we don't create "devices" for every
component of the system which can have a separate FW. That's where
"components" are intended to be used..

> The idea (implemented in the next patchset) is to let these devices
> expose their own "component name", which can then be plugged into the
> existing flash command:
> 
>     $ devlink lc show pci/0000:01:00.0 lc 8
>     pci/0000:01:00.0:
>       lc 8 state active type 16x100G
>         supported_types:
>            16x100G
>         devices:
>           device 0 flashable true component lc8_dev0
>           device 1 flashable false
>           device 2 flashable false
>           device 3 flashable false
>     $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2 component lc8_dev0

IDK if it's just me or this assumes deep knowledge of the system.
I don't understand why we need to list devices 1-3 at all. And they
don't even have names. No information is exposed. 

There are many components on any networking device, including plenty
40G-R4 -> 25G-R1 gearboxes out there.

> Registering a separate devlink instance for these devices sounds like an
> overkill to me. If you are not OK with a separate command (e.g.,
> DEVLINK_CMD_LINECARD_INFO_GET), then extending DEVLINK_CMD_INFO_GET is
> also an option. We discussed this during internal review, but felt that
> the current approach is cleaner.

I don't know what you have queued, so if you don't need a full devlink
instance (IOW line cards won't need more individual config) that's fine.
For just FW flashing you can list many devices and update the
components... no need to introduce new objects or uAPI.

> > Would you mind if I revert this?  
> 
> I can't stop you, but keep in mind that it's already late here and that
> tomorrow I'm AFK (reserve duty) and won't be able to tag it. Jiri should
> be available to continue this discussion tomorrow morning, so probably
> best to wait for his feedback.

Sure, no rush.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Mon, Apr 25, 2022 at 09:39:57PM CEST, idosch@idosch.org wrote:
>On Mon, Apr 25, 2022 at 09:00:21AM -0700, Jakub Kicinski wrote:
>> On Mon, 25 Apr 2022 06:44:20 +0300 Ido Schimmel wrote:
>> > This patchset is extending the line card model by three items:
>> > 1) line card devices
>> > 2) line card info
>> > 3) line card device info
>> > 
>> > First three patches are introducing the necessary changes in devlink
>> > core.
>> > 
>> > Then, all three extensions are implemented in mlxsw alongside with
>> > selftest.
>> 
>> :/ what is a line card device? You must provide document what you're
>> doing, this:
>> 
>>  .../networking/devlink/devlink-linecard.rst   |   4 +
>> 
>> is not enough.
>> 
>> How many operations and attributes are you going to copy&paste?
>> 
>> Is linking devlink instances into a hierarchy a better approach?
>
>In this particular case, these devices are gearboxes. They are running
>their own firmware and we want user space to be able to query and update
>the running firmware version.
>
>The idea (implemented in the next patchset) is to let these devices
>expose their own "component name", which can then be plugged into the
>existing flash command:
>
>    $ devlink lc show pci/0000:01:00.0 lc 8
>    pci/0000:01:00.0:
>      lc 8 state active type 16x100G
>        supported_types:
>           16x100G
>        devices:
>          device 0 flashable true component lc8_dev0
>          device 1 flashable false
>          device 2 flashable false
>          device 3 flashable false
>    $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2 component lc8_dev0
>
>Registering a separate devlink instance for these devices sounds like an

It is not a separate devlink device, not removetely. A devlink device is
attached to some bus on the host, it contains entities like netdevices,
etc.

Line card devices, on contrary, are accessible over ASIC FW interface,
they reside on line cards. ASIC FW is using build-in SDK to communicate
with them. There is really nothing to expose, except for the face they
are there, with some FW version and later on (follow-up patchset) to be
able to flash FW on them.

It's a gearbox. I found it odd to name it gearbox as in theory, there
might be some other single purpose device on the line card of other type
than gearbox. Therefore, "device". I admit it is a bit misleading. Idea
for a better name?


>overkill to me. If you are not OK with a separate command (e.g.,
>DEVLINK_CMD_LINECARD_INFO_GET), then extending DEVLINK_CMD_INFO_GET is
>also an option. We discussed this during internal review, but felt that

We would need to add all the line card hierarchy into info_get. That
would look a bit odd to me.


>the current approach is cleaner.
>
>> Would you mind if I revert this?

Let's see what you need to change and change it in place, if possible.


>
>I can't stop you, but keep in mind that it's already late here and that
>tomorrow I'm AFK (reserve duty) and won't be able to tag it. Jiri should
>be available to continue this discussion tomorrow morning, so probably
>best to wait for his feedback.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Mon, Apr 25, 2022 at 09:52:18PM CEST, kuba@kernel.org wrote:
>On Mon, 25 Apr 2022 22:39:57 +0300 Ido Schimmel wrote:
>> > :/ what is a line card device? You must provide document what you're
>> > doing, this:
>> > 
>> >  .../networking/devlink/devlink-linecard.rst   |   4 +
>> > 
>> > is not enough.
>> > 
>> > How many operations and attributes are you going to copy&paste?
>> > 
>> > Is linking devlink instances into a hierarchy a better approach?  
>> 
>> In this particular case, these devices are gearboxes. They are running
>> their own firmware and we want user space to be able to query and update
>> the running firmware version.
>
>Nothing too special, then, we don't create "devices" for every
>component of the system which can have a separate FW. That's where
>"components" are intended to be used..

*
Sure, that is why I re-used components :)
But you have to somehow link the component to the particular gearbox on
particular line card. Say, you need to flash GB on line card 8. This is
basically providing a way to expose this relationship to user. Also, the
"lc info" shows the FW version for gearboxes. As Ido mentioned, the GB
versions could be listed in "devlink dev info" in theory. But then, you
need to somehow expose the relationship with line card as well.

I don't see any simpler iface than this.


>
>> The idea (implemented in the next patchset) is to let these devices
>> expose their own "component name", which can then be plugged into the
>> existing flash command:
>> 
>>     $ devlink lc show pci/0000:01:00.0 lc 8
>>     pci/0000:01:00.0:
>>       lc 8 state active type 16x100G
>>         supported_types:
>>            16x100G
>>         devices:
>>           device 0 flashable true component lc8_dev0
>>           device 1 flashable false
>>           device 2 flashable false
>>           device 3 flashable false
>>     $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2 component lc8_dev0
>
>IDK if it's just me or this assumes deep knowledge of the system.
>I don't understand why we need to list devices 1-3 at all. And they
>don't even have names. No information is exposed. 

There are 4 gearboxes on the line card. They share the same flash. So if
you flash gearbox 0, the rest will use the same FW.
I'm exposing them for the sake of completeness. Also, the interface
needs to be designed as a list anyway, as different line cards may have
separate flash per gearbox.

What's is the harm in exposing devices 1-3? If you insist, we can hide
them.


>
>There are many components on any networking device, including plenty
>40G-R4 -> 25G-R1 gearboxes out there.
>
>> Registering a separate devlink instance for these devices sounds like an
>> overkill to me. If you are not OK with a separate command (e.g.,
>> DEVLINK_CMD_LINECARD_INFO_GET), then extending DEVLINK_CMD_INFO_GET is
>> also an option. We discussed this during internal review, but felt that
>> the current approach is cleaner.
>
>I don't know what you have queued, so if you don't need a full devlink
>instance (IOW line cards won't need more individual config) that's fine.

Yeah, incoparable, the devlink dev and line card device - gearbox.


>For just FW flashing you can list many devices and update the
>components... no need to introduce new objects or uAPI.

Please see * above.


>
>> > Would you mind if I revert this?  
>> 
>> I can't stop you, but keep in mind that it's already late here and that
>> tomorrow I'm AFK (reserve duty) and won't be able to tag it. Jiri should
>> be available to continue this discussion tomorrow morning, so probably
>> best to wait for his feedback.
>
>Sure, no rush.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Andrew Lunn]

> >> The idea (implemented in the next patchset) is to let these devices
> >> expose their own "component name", which can then be plugged into the
> >> existing flash command:
> >> 
> >>     $ devlink lc show pci/0000:01:00.0 lc 8
> >>     pci/0000:01:00.0:
> >>       lc 8 state active type 16x100G
> >>         supported_types:
> >>            16x100G
> >>         devices:
> >>           device 0 flashable true component lc8_dev0
> >>           device 1 flashable false
> >>           device 2 flashable false
> >>           device 3 flashable false
> >>     $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2 component lc8_dev0
> >
> >IDK if it's just me or this assumes deep knowledge of the system.
> >I don't understand why we need to list devices 1-3 at all. And they
> >don't even have names. No information is exposed. 
> 
> There are 4 gearboxes on the line card. They share the same flash. So if
> you flash gearbox 0, the rest will use the same FW.

Is the flash big enough to hold four copies of the firmware? Listing
all four devices gives you a path forward to allowing each device to
have its own firmware.

On the flip side, flashable false is not really true. I don't know
this API at all, but are there any flags? Can you at least indicate it
is shared? You could then have an output something like:

           device 0 flashable true shared component lc8_dev0
           device 1 flashable true shared component lc8_dev0
           device 2 flashable true shared component lc8_dev0
           device 3 flashable true shared component lc8_dev0

so you get to see the sharing relationship.

   Andrew

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Andrew Lunn]

> It is not a separate devlink device, not removetely. A devlink device is
> attached to some bus on the host, it contains entities like netdevices,
> etc.
> 
> Line card devices, on contrary, are accessible over ASIC FW interface,
> they reside on line cards. ASIC FW is using build-in SDK to communicate
> with them. There is really nothing to expose, except for the face they
> are there, with some FW version and later on (follow-up patchset) to be
> able to flash FW on them.

But isn't this just an implementation detail?

Say the flash was directly accessible to the host? It is just another
mtd devices? The gearbox is just another bunch of MMIO registers. You
can access the SFP socket via a host i2c bus, etc. More of a SoC like
implementation, which the enterprise routers are like.

This is a completely different set of implementation details, but i
still have the same basic building blocks. Should it look the same,
and the implementation details are hidden, or do you want to expose
your implementation details?

	Andrew

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, Apr 26, 2022 at 02:11:43PM CEST, andrew@lunn.ch wrote:
>> >> The idea (implemented in the next patchset) is to let these devices
>> >> expose their own "component name", which can then be plugged into the
>> >> existing flash command:
>> >> 
>> >>     $ devlink lc show pci/0000:01:00.0 lc 8
>> >>     pci/0000:01:00.0:
>> >>       lc 8 state active type 16x100G
>> >>         supported_types:
>> >>            16x100G
>> >>         devices:
>> >>           device 0 flashable true component lc8_dev0
>> >>           device 1 flashable false
>> >>           device 2 flashable false
>> >>           device 3 flashable false
>> >>     $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2 component lc8_dev0
>> >
>> >IDK if it's just me or this assumes deep knowledge of the system.
>> >I don't understand why we need to list devices 1-3 at all. And they
>> >don't even have names. No information is exposed. 
>> 
>> There are 4 gearboxes on the line card. They share the same flash. So if
>> you flash gearbox 0, the rest will use the same FW.
>
>Is the flash big enough to hold four copies of the firmware? Listing

One copy, all 4 are using it.


>all four devices gives you a path forward to allowing each device to
>have its own firmware.

Yes.

>
>On the flip side, flashable false is not really true. I don't know
>this API at all, but are there any flags? Can you at least indicate it
>is shared? You could then have an output something like:

Yes, that I can do.

>
>           device 0 flashable true shared component lc8_dev0
>           device 1 flashable true shared component lc8_dev0
>           device 2 flashable true shared component lc8_dev0
>           device 3 flashable true shared component lc8_dev0
>
>so you get to see the sharing relationship.

Agreed.

>
>   Andrew

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, Apr 26, 2022 at 02:27:54PM CEST, andrew@lunn.ch wrote:
>> It is not a separate devlink device, not removetely. A devlink device is
>> attached to some bus on the host, it contains entities like netdevices,
>> etc.
>> 
>> Line card devices, on contrary, are accessible over ASIC FW interface,
>> they reside on line cards. ASIC FW is using build-in SDK to communicate
>> with them. There is really nothing to expose, except for the face they
>> are there, with some FW version and later on (follow-up patchset) to be
>> able to flash FW on them.
>
>But isn't this just an implementation detail?
>
>Say the flash was directly accessible to the host? It is just another
>mtd devices? The gearbox is just another bunch of MMIO registers. You

Not really, the ASIC FW is also not mtd device. ASIC FW exposes set of
registers to work with the ASIC FW flash. The linecard gearbox
implements the same set of registers, tunneled over MDDT register.

Also, the gearbox is not accessable from the host. The ASIC FW is the
only one able to access it.


>can access the SFP socket via a host i2c bus, etc. More of a SoC like
>implementation, which the enterprise routers are like.
>
>This is a completely different set of implementation details, but i
>still have the same basic building blocks. Should it look the same,
>and the implementation details are hidden, or do you want to expose
>your implementation details?

Well, I got your point. If the HW would be designed in the way the
building blocks are exposed to the host, that would work. However, that
is not the case here, unfortunatelly.


>
>	Andrew

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Tue, 26 Apr 2022 08:57:15 +0200 Jiri Pirko wrote:
> >> In this particular case, these devices are gearboxes. They are running
> >> their own firmware and we want user space to be able to query and update
> >> the running firmware version.  
> >
> >Nothing too special, then, we don't create "devices" for every
> >component of the system which can have a separate FW. That's where
> >"components" are intended to be used..  
> 
> *
> Sure, that is why I re-used components :)

Well, right, I guess you did reuse them a little :)

> But you have to somehow link the component to the particular gearbox on
> particular line card. Say, you need to flash GB on line card 8. This is
> basically providing a way to expose this relationship to user.
> 
> Also, the "lc info" shows the FW version for gearboxes. As Ido
> mentioned, the GB versions could be listed in "devlink dev info" in
> theory. But then, you need to somehow expose the relationship with
> line card as well.

Why would the automation which comes to update the firmware care 
at all where the component is? Humans can see what the component 
is by looking at the name.

If we do need to know (*if*!) you can list FW components as a lc
attribute, no need for new commands and objects.

IMHO we should either keep lc objects simple and self contained or 
give them a devlink instance. Creating sub-objects from them is very
worrying. If there is _any_ chance we'll need per-lc health reporters 
or sbs or params(🤢) etc. etc. - let's bite the bullet _now_ and create
full devlink sub-instances!

> I don't see any simpler iface than this.

Based on the assumptions you've made, maybe, but the uAPI should
abstract away the irrelevant details. I'm questioning the assumptions.

> >> The idea (implemented in the next patchset) is to let these devices
> >> expose their own "component name", which can then be plugged into
> >> the existing flash command:
> >> 
> >>     $ devlink lc show pci/0000:01:00.0 lc 8
> >>     pci/0000:01:00.0:
> >>       lc 8 state active type 16x100G
> >>         supported_types:
> >>            16x100G
> >>         devices:
> >>           device 0 flashable true component lc8_dev0
> >>           device 1 flashable false
> >>           device 2 flashable false
> >>           device 3 flashable false
> >>     $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2
> >> component lc8_dev0  
> >
> >IDK if it's just me or this assumes deep knowledge of the system.
> >I don't understand why we need to list devices 1-3 at all. And they
> >don't even have names. No information is exposed.   
> 
> There are 4 gearboxes on the line card. They share the same flash. So
> if you flash gearbox 0, the rest will use the same FW.

o_0 so the FW component is called lcX_dev0 and yet it applies to _all_
devices, not just dev0?! Looking at the output above I thought other
devices simply don't have FW ("flashable false").

> I'm exposing them for the sake of completeness. Also, the interface
> needs to be designed as a list anyway, as different line cards may
> have separate flash per gearbox.
> 
> What's is the harm in exposing devices 1-3? If you insist, we can hide
> them.

Well, they are unnecessary (this is uAPI), and coming from the outside
I misinterpreted what the information reported means, so yeah, I'd
classify it as harmful :(

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Andrew Lunn]

> Well, I got your point. If the HW would be designed in the way the
> building blocks are exposed to the host, that would work. However, that
> is not the case here, unfortunatelly.

I'm with Jakub. It is the uAPI which matters here. It should look the
same for a SoC style enterprise router and your discombobulated TOR
router. How you talk to the different building blocks is an
implementation detail.

	Andrew

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, Apr 26, 2022 at 02:41:30PM CEST, kuba@kernel.org wrote:
>On Tue, 26 Apr 2022 08:57:15 +0200 Jiri Pirko wrote:
>> >> In this particular case, these devices are gearboxes. They are running
>> >> their own firmware and we want user space to be able to query and update
>> >> the running firmware version.  
>> >
>> >Nothing too special, then, we don't create "devices" for every
>> >component of the system which can have a separate FW. That's where
>> >"components" are intended to be used..  
>> 
>> *
>> Sure, that is why I re-used components :)
>
>Well, right, I guess you did reuse them a little :)

I use them a lot. It is not visible in this patchset, but in the
flashing follow-up patchset.


>
>> But you have to somehow link the component to the particular gearbox on
>> particular line card. Say, you need to flash GB on line card 8. This is
>> basically providing a way to expose this relationship to user.
>> 
>> Also, the "lc info" shows the FW version for gearboxes. As Ido
>> mentioned, the GB versions could be listed in "devlink dev info" in
>> theory. But then, you need to somehow expose the relationship with
>> line card as well.
>
>Why would the automation which comes to update the firmware care 
>at all where the component is? Humans can see what the component 
>is by looking at the name.

The relationship-by-name sounds a bit fragile to me. The names of
components are up to the individual drivers.


>
>If we do need to know (*if*!) you can list FW components as a lc
>attribute, no need for new commands and objects.

There is no new command for that, only one nested attribute which
carries the device list added to the existing command. They are no new
objects, they are just few nested values.


>
>IMHO we should either keep lc objects simple and self contained or 
>give them a devlink instance. Creating sub-objects from them is very

Give them a devlink instance? I don't understand how. LC is not a
separate device, far from that. That does not make any sense to me.


>worrying. If there is _any_ chance we'll need per-lc health reporters 
>or sbs or params(🤢) etc. etc. - let's bite the bullet _now_ and create
>full devlink sub-instances!

Does not make sense to me at all. Line cards are detachable PHY sets in
essence, very basic functionality. They does not have buffers, health
and params, I don't think so. 


>
>> I don't see any simpler iface than this.
>
>Based on the assumptions you've made, maybe, but the uAPI should
>abstract away the irrelevant details. I'm questioning the assumptions.

Is the FW version of gearbox on a line card irrelevand detail?
If so, how does the user know if/when to flash it?
If not, where would you list it if devices nest is not the correct place?


>
>> >> The idea (implemented in the next patchset) is to let these devices
>> >> expose their own "component name", which can then be plugged into
>> >> the existing flash command:
>> >> 
>> >>     $ devlink lc show pci/0000:01:00.0 lc 8
>> >>     pci/0000:01:00.0:
>> >>       lc 8 state active type 16x100G
>> >>         supported_types:
>> >>            16x100G
>> >>         devices:
>> >>           device 0 flashable true component lc8_dev0
>> >>           device 1 flashable false
>> >>           device 2 flashable false
>> >>           device 3 flashable false
>> >>     $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2
>> >> component lc8_dev0  
>> >
>> >IDK if it's just me or this assumes deep knowledge of the system.
>> >I don't understand why we need to list devices 1-3 at all. And they
>> >don't even have names. No information is exposed.   
>> 
>> There are 4 gearboxes on the line card. They share the same flash. So
>> if you flash gearbox 0, the rest will use the same FW.
>
>o_0 so the FW component is called lcX_dev0 and yet it applies to _all_
>devices, not just dev0?! Looking at the output above I thought other
>devices simply don't have FW ("flashable false").

Yes, device 0 is "flash master" (RW). 1-3 are RO. I know it is a bit
confusing. Maybe Andy's suggestion of "shared" flag of some sort might
help.


>
>> I'm exposing them for the sake of completeness. Also, the interface
>> needs to be designed as a list anyway, as different line cards may
>> have separate flash per gearbox.
>> 
>> What's is the harm in exposing devices 1-3? If you insist, we can hide
>> them.
>
>Well, they are unnecessary (this is uAPI), and coming from the outside
>I misinterpreted what the information reported means, so yeah, I'd
>classify it as harmful :(

UAPI is the "devices nest". It has to be list one way or another
(we may need to expose more gearboxes anyway). So what is differently
harmful with having list [0] or list [0,1,2,3] ?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, Apr 26, 2022 at 03:45:48PM CEST, andrew@lunn.ch wrote:
>> Well, I got your point. If the HW would be designed in the way the
>> building blocks are exposed to the host, that would work. However, that
>> is not the case here, unfortunatelly.
>
>I'm with Jakub. It is the uAPI which matters here. It should look the
>same for a SoC style enterprise router and your discombobulated TOR
>router. How you talk to the different building blocks is an
>implementation detail.

It's not that simple. Take the gearbox for example. You say bunch of
MDIO registers. ASIC FW has a custom SDK internally that is used to
talk to the gearbox.

The flash, you say expose by MTD, but there is no access to it directly
from host. Can't be done. There are HW design limitations that are
blocking your concept.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Tue, 26 Apr 2022 16:00:10 +0200 Jiri Pirko wrote:
> >> But you have to somehow link the component to the particular gearbox on
> >> particular line card. Say, you need to flash GB on line card 8. This is
> >> basically providing a way to expose this relationship to user.
> >> 
> >> Also, the "lc info" shows the FW version for gearboxes. As Ido
> >> mentioned, the GB versions could be listed in "devlink dev info" in
> >> theory. But then, you need to somehow expose the relationship with
> >> line card as well.  
> >
> >Why would the automation which comes to update the firmware care 
> >at all where the component is? Humans can see what the component 
> >is by looking at the name.  
> 
> The relationship-by-name sounds a bit fragile to me. The names of
> components are up to the individual drivers.

I asked you how the automation will operate. You must answer questions
if you want to have a discussion. Automation is the relevant part.
You're not designing an interface for SDK users but for end users.

> >If we do need to know (*if*!) you can list FW components as a lc
> >attribute, no need for new commands and objects.  
> 
> There is no new command for that, only one nested attribute which
> carries the device list added to the existing command. They are no new
> objects, they are just few nested values.

DEVLINK_CMD_LINECARD_INFO_GET

> >IMHO we should either keep lc objects simple and self contained or 
> >give them a devlink instance. Creating sub-objects from them is very  
> 
> Give them a devlink instance? I don't understand how. LC is not a
> separate device, far from that. That does not make any sense to me.

You can put a name of another devlink instance as an attribute of a lc.
See below.

> >worrying. If there is _any_ chance we'll need per-lc health reporters 
> >or sbs or params(🤢) etc. etc. - let's bite the bullet _now_ and create
> >full devlink sub-instances!  
> 
> Does not make sense to me at all. Line cards are detachable PHY sets in
> essence, very basic functionality. They does not have buffers, health
> and params, I don't think so. 

I guess the definition of a "line card" has become somewhat murky over
the years, since the olden days of serial lines.

Perhaps David and others can enlighten us but what I'm used to hearing
about as a line card these days in a chassis system is a full-on switch.
Chassis being effectively a Clos network in a box, the main difference
being the line cards talk cells to the backplane, not full packets.

Back in my Netronome days we called those detachable front panel gear
boxes "phy mods". Those had nowhere near the complexity of a real line
card. Sounds like that's more aligned with what you have.

To summarize, since your definition of a line card is a bit special,
the less uAPI we add to fit your definition we add the better.

> >> I don't see any simpler iface than this.  
> >
> >Based on the assumptions you've made, maybe, but the uAPI should
> >abstract away the irrelevant details. I'm questioning the assumptions.  
> 
> Is the FW version of gearbox on a line card irrelevand detail?

Not what I said.

> If so, how does the user know if/when to flash it?
> If not, where would you list it if devices nest is not the correct place?

Let me mock up what I had in mind for you since it did not come thru 
in the explanation:

$ devlink dev info show pci/0000:01:00.0
    versions:
        fixed:
          hw.revision 0
          lc2.hw.revision a
          lc8.hw.revision b
        running:
          ini.version 4
          lc2.gearbox 1.1.3
          lc8.gearbox 1.2.3

$ devlink lc show pci/0000:01:00.0 lc 8
pci/0000:01:00.0:
  lc 8 state active type 16x100G
    supported_types:
      16x100G
    versions: 
      lc8.hw.revision (a) 
      lc8.gearbox (1.2.3)

Where the data in the brackets is optionally fetched thru the existing
"dev info" API, but rendered together by the user space.

> >> There are 4 gearboxes on the line card. They share the same flash. So
> >> if you flash gearbox 0, the rest will use the same FW.  
> >
> >o_0 so the FW component is called lcX_dev0 and yet it applies to _all_
> >devices, not just dev0?! Looking at the output above I thought other
> >devices simply don't have FW ("flashable false").  
> 
> Yes, device 0 is "flash master" (RW). 1-3 are RO. I know it is a bit
> confusing. Maybe Andy's suggestion of "shared" flag of some sort might
> help.
> 
> >> I'm exposing them for the sake of completeness. Also, the interface
> >> needs to be designed as a list anyway, as different line cards may
> >> have separate flash per gearbox.
> >> 
> >> What's is the harm in exposing devices 1-3? If you insist, we can hide
> >> them.  
> >
> >Well, they are unnecessary (this is uAPI), and coming from the outside
> >I misinterpreted what the information reported means, so yeah, I'd
> >classify it as harmful :(  
> 
> UAPI is the "devices nest". It has to be list one way or another
> (we may need to expose more gearboxes anyway). So what is differently
> harmful with having list [0] or list [0,1,2,3] ?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Andrew Lunn]

> Does not make sense to me at all. Line cards are detachable PHY sets in
> essence, very basic functionality. They does not have buffers, health
> and params, I don't think so. 

Ido recently added support to ethtool to upgrade the flash in SFPs.
They are far from simple devices. Some of the GPON ones have linux
running in them, that you can log in to.

The real question is, can you do everything you need to do via
existing uAPIs like ethtool and hwmon.

	Andrew

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Andrew Lunn]

On Tue, Apr 26, 2022 at 04:05:43PM +0200, Jiri Pirko wrote:
> Tue, Apr 26, 2022 at 03:45:48PM CEST, andrew@lunn.ch wrote:
> >> Well, I got your point. If the HW would be designed in the way the
> >> building blocks are exposed to the host, that would work. However, that
> >> is not the case here, unfortunatelly.
> >
> >I'm with Jakub. It is the uAPI which matters here. It should look the
> >same for a SoC style enterprise router and your discombobulated TOR
> >router. How you talk to the different building blocks is an
> >implementation detail.
> 
> It's not that simple. Take the gearbox for example. You say bunch of
> MDIO registers. ASIC FW has a custom SDK internally that is used to
> talk to the gearbox.
> 
> The flash, you say expose by MTD, but there is no access to it directly
> from host. Can't be done. There are HW design limitations that are
> blocking your concept.

The MTD API and your SDK API are abstractions. You give it a blob of
data and ask it to write it to the storage. Somehow that happens. Does
user space need to know MTD or an SDK is being used? Does user space
care? I would expect the same uAPI for both, here is a firmware blob,
write it to storage. The driver knows if it needs to use the MTD API
or the SDK API, it is all abstracted away in the driver.

	Andrew

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, Apr 26, 2022 at 04:51:33PM CEST, kuba@kernel.org wrote:
>On Tue, 26 Apr 2022 16:00:10 +0200 Jiri Pirko wrote:
>> >> But you have to somehow link the component to the particular gearbox on
>> >> particular line card. Say, you need to flash GB on line card 8. This is
>> >> basically providing a way to expose this relationship to user.
>> >> 
>> >> Also, the "lc info" shows the FW version for gearboxes. As Ido
>> >> mentioned, the GB versions could be listed in "devlink dev info" in
>> >> theory. But then, you need to somehow expose the relationship with
>> >> line card as well.  
>> >
>> >Why would the automation which comes to update the firmware care 
>> >at all where the component is? Humans can see what the component 
>> >is by looking at the name.  
>> 
>> The relationship-by-name sounds a bit fragile to me. The names of
>> components are up to the individual drivers.
>
>I asked you how the automation will operate. You must answer questions
>if you want to have a discussion. Automation is the relevant part.

Automation, not sure. It would probably just see type of gearbox and
flash it. Not sure I understand the question, perhaps you could explain?
Plus, the possibility is to auto-flash the GB from driver directly.


>You're not designing an interface for SDK users but for end users.

Sure, that is the aim of this API. Human end user. That is why I wanted
the user to see the relationships between devlink dev, line cards and
the gearboxes on them. If you want to limit the visibility, sure, just
tell me how.


>
>> >If we do need to know (*if*!) you can list FW components as a lc
>> >attribute, no need for new commands and objects.  
>> 
>> There is no new command for that, only one nested attribute which
>> carries the device list added to the existing command. They are no new
>> objects, they are just few nested values.
>
>DEVLINK_CMD_LINECARD_INFO_GET

Okay, that is not only to expose devices. That is also to expose info
about linecards, like HW revision, INI version etc. Where else to put
it? I can perhaps embed it into devlink dev info, but I thought separate
command would be more suitable. object cmd, object info cmd. It is
more clear I believe.


>
>> >IMHO we should either keep lc objects simple and self contained or 
>> >give them a devlink instance. Creating sub-objects from them is very  
>> 
>> Give them a devlink instance? I don't understand how. LC is not a
>> separate device, far from that. That does not make any sense to me.
>
>You can put a name of another devlink instance as an attribute of a lc.
>See below.
>
>> >worrying. If there is _any_ chance we'll need per-lc health reporters 
>> >or sbs or params(🤢) etc. etc. - let's bite the bullet _now_ and create
>> >full devlink sub-instances!  
>> 
>> Does not make sense to me at all. Line cards are detachable PHY sets in
>> essence, very basic functionality. They does not have buffers, health
>> and params, I don't think so. 
>
>I guess the definition of a "line card" has become somewhat murky over
>the years, since the olden days of serial lines.
>
>Perhaps David and others can enlighten us but what I'm used to hearing
>about as a line card these days in a chassis system is a full-on switch.
>Chassis being effectively a Clos network in a box, the main difference
>being the line cards talk cells to the backplane, not full packets.

That is a different device model comparing what we have.
These are like "nested switches". If they are separate devices with
everything, they can be modeled as separate devlink device instances.
No problem. Different case though.


>
>Back in my Netronome days we called those detachable front panel gear
>boxes "phy mods". Those had nowhere near the complexity of a real line
>card. Sounds like that's more aligned with what you have.

Yep.


>
>To summarize, since your definition of a line card is a bit special,
>the less uAPI we add to fit your definition we add the better.

Tell me where to cut it so it still makes sense.


>
>> >> I don't see any simpler iface than this.  
>> >
>> >Based on the assumptions you've made, maybe, but the uAPI should
>> >abstract away the irrelevant details. I'm questioning the assumptions.  
>> 
>> Is the FW version of gearbox on a line card irrelevand detail?
>
>Not what I said.

That is why I'm asking :)


>
>> If so, how does the user know if/when to flash it?
>> If not, where would you list it if devices nest is not the correct place?
>
>Let me mock up what I had in mind for you since it did not come thru 
>in the explanation:
>
>$ devlink dev info show pci/0000:01:00.0
>    versions:
>        fixed:
>          hw.revision 0
>          lc2.hw.revision a
>          lc8.hw.revision b
>        running:
>          ini.version 4
>          lc2.gearbox 1.1.3
>          lc8.gearbox 1.2.3

Would be rather:

          lc2.gearbox0 1.1.3
          lc2.gearbox1 1.2.4
          lc8.gearbox0 1.2.3

Okay, I see. So instead of having clear api with relationships and
clear human+machine readability we have squahed indexes into strings.
I fail to see the benefit, other than no-api-extension :/ On contrary.


>
>$ devlink lc show pci/0000:01:00.0 lc 8
>pci/0000:01:00.0:
>  lc 8 state active type 16x100G
>    supported_types:
>      16x100G
>    versions: 
>      lc8.hw.revision (a) 
>      lc8.gearbox (1.2.3)
>
>Where the data in the brackets is optionally fetched thru the existing
>"dev info" API, but rendered together by the user space.

Quite odd. I find it questionable to say at least to mix multiple
command netlink outputs into one output. The processing of it would
be a small nightmare considering the way how the netlink message
processing works in iproute2 :/


>
>> >> There are 4 gearboxes on the line card. They share the same flash. So
>> >> if you flash gearbox 0, the rest will use the same FW.  
>> >
>> >o_0 so the FW component is called lcX_dev0 and yet it applies to _all_
>> >devices, not just dev0?! Looking at the output above I thought other
>> >devices simply don't have FW ("flashable false").  
>> 
>> Yes, device 0 is "flash master" (RW). 1-3 are RO. I know it is a bit
>> confusing. Maybe Andy's suggestion of "shared" flag of some sort might
>> help.
>> 
>> >> I'm exposing them for the sake of completeness. Also, the interface
>> >> needs to be designed as a list anyway, as different line cards may
>> >> have separate flash per gearbox.
>> >> 
>> >> What's is the harm in exposing devices 1-3? If you insist, we can hide
>> >> them.  
>> >
>> >Well, they are unnecessary (this is uAPI), and coming from the outside
>> >I misinterpreted what the information reported means, so yeah, I'd
>> >classify it as harmful :(  
>> 
>> UAPI is the "devices nest". It has to be list one way or another
>> (we may need to expose more gearboxes anyway). So what is differently
>> harmful with having list [0] or list [0,1,2,3] ?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, Apr 26, 2022 at 05:24:41PM CEST, andrew@lunn.ch wrote:
>> Does not make sense to me at all. Line cards are detachable PHY sets in
>> essence, very basic functionality. They does not have buffers, health
>> and params, I don't think so. 
>
>Ido recently added support to ethtool to upgrade the flash in SFPs.
>They are far from simple devices. Some of the GPON ones have linux
>running in them, that you can log in to.
>
>The real question is, can you do everything you need to do via
>existing uAPIs like ethtool and hwmon.

No, it does not fit. Ethtool works with netdevices which are
instantiated for separate port. The disconnection between what we can do
with netdev as a handle and how the devices are modeled was the reason
for devlink introduction in the past.

>
>	Andrew

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Wed, 27 Apr 2022 09:35:34 +0200 Jiri Pirko wrote:
> >> The relationship-by-name sounds a bit fragile to me. The names of
> >> components are up to the individual drivers.  
> >
> >I asked you how the automation will operate. You must answer questions
> >if you want to have a discussion. Automation is the relevant part.  
> 
> Automation, not sure. It would probably just see type of gearbox and
> flash it. Not sure I understand the question, perhaps you could explain?
> Plus, the possibility is to auto-flash the GB from driver directly.
> 
> 
> >You're not designing an interface for SDK users but for end users.  
> 
> Sure, that is the aim of this API. Human end user. That is why I wanted
> the user to see the relationships between devlink dev, line cards and
> the gearboxes on them. If you want to limit the visibility, sure, just
> tell me how.

Okay, we have completely different views on what the goals should be.
Perhaps that explains the differences in the design.

Of the three API levels (SDK, automation, human) I think automation
is the only one that's interesting to us in Linux. SDK interfaces are
necessarily too low level as they expose too much of internal details
to standardize. Humans are good with dealing with uncertainty and
diverse so there's no a good benchmark.

The benchmark for automation is - can a machine use this API across
different vendors to reliably achieve its goals. For FW info/flashing
the goal is keeping the FW versions up to date. This is documented:

https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html#firmware-version-management

What would the pseudo code look like with "line cards" in the picture?
Apply RFC1925 truth 12.

> >> There is no new command for that, only one nested attribute which
> >> carries the device list added to the existing command. They are no new
> >> objects, they are just few nested values.  
> >
> >DEVLINK_CMD_LINECARD_INFO_GET  
> 
> Okay, that is not only to expose devices. That is also to expose info
> about linecards, like HW revision, INI version etc. Where else to put
> it? I can perhaps embed it into devlink dev info, but I thought separate
> command would be more suitable. object cmd, object info cmd. It is
> more clear I believe.

> >> If so, how does the user know if/when to flash it?
> >> If not, where would you list it if devices nest is not the correct place?  
> >
> >Let me mock up what I had in mind for you since it did not come thru 
> >in the explanation:
> >
> >$ devlink dev info show pci/0000:01:00.0
> >    versions:
> >        fixed:
> >          hw.revision 0
> >          lc2.hw.revision a
> >          lc8.hw.revision b
> >        running:
> >          ini.version 4
> >          lc2.gearbox 1.1.3
> >          lc8.gearbox 1.2.3  
> 
> Would be rather:
> 
>           lc2.gearbox0 1.1.3
>           lc2.gearbox1 1.2.4

I thought you said your gearboxes all the the same FW? 
Theoretically, yes. Theoretically, I can also have nested "line cards".

>           lc8.gearbox0 1.2.3
> 
> Okay, I see. So instead of having clear api with relationships and
> clear human+machine readability we have squahed indexes into strings.
> I fail to see the benefit, other than no-api-extension :/ On contrary.

Show me the real life use for all the "clear api with relationships"
and I'll shut up.

I would not take falling back to physical (HW) hierarchy for the API
design as a point of pride. Seems lazy if I'm completely honest.
Someone else's HW may have a different hierarchy, and you're just
forcing the automation engineer iterate over irrelevant structures
("devices").

My hunch is that automation will not want to deal with line cards
separately, and flash the entire devices in one go to a tested and
verified bundle blob provided by the vendor. If they do want to poke 
at line cards - the information is still there in what I described.

> >$ devlink lc show pci/0000:01:00.0 lc 8
> >pci/0000:01:00.0:
> >  lc 8 state active type 16x100G
> >    supported_types:
> >      16x100G
> >    versions: 
> >      lc8.hw.revision (a) 
> >      lc8.gearbox (1.2.3)
> >
> >Where the data in the brackets is optionally fetched thru the existing
> >"dev info" API, but rendered together by the user space.  
> 
> Quite odd. I find it questionable to say at least to mix multiple
> command netlink outputs into one output.

Really? So we're going to be designing kernel APIs so that each message
contains complete information and can't contain references now?

> The processing of it would be a small nightmare considering the way
> how the netlink message processing works in iproute2 :/

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Wed, Apr 27, 2022 at 04:14:47PM CEST, kuba@kernel.org wrote:
>On Wed, 27 Apr 2022 09:35:34 +0200 Jiri Pirko wrote:
>> >> The relationship-by-name sounds a bit fragile to me. The names of
>> >> components are up to the individual drivers.  
>> >
>> >I asked you how the automation will operate. You must answer questions
>> >if you want to have a discussion. Automation is the relevant part.  
>> 
>> Automation, not sure. It would probably just see type of gearbox and
>> flash it. Not sure I understand the question, perhaps you could explain?
>> Plus, the possibility is to auto-flash the GB from driver directly.
>> 
>> 
>> >You're not designing an interface for SDK users but for end users.  
>> 
>> Sure, that is the aim of this API. Human end user. That is why I wanted
>> the user to see the relationships between devlink dev, line cards and
>> the gearboxes on them. If you want to limit the visibility, sure, just
>> tell me how.
>
>Okay, we have completely different views on what the goals should be.
>Perhaps that explains the differences in the design.

I don't think so. I'm just a bit confused that you say "You're not designing
an interface for SDK users but for *end users*" and now you say that
primary is automation. Nevertheless, both are equally important and the
iface should work for both or them, I believe.


>
>Of the three API levels (SDK, automation, human) I think automation
>is the only one that's interesting to us in Linux. SDK interfaces are
>necessarily too low level as they expose too much of internal details
>to standardize. Humans are good with dealing with uncertainty and
>diverse so there's no a good benchmark.
>
>The benchmark for automation is - can a machine use this API across
>different vendors to reliably achieve its goals. For FW info/flashing
>the goal is keeping the FW versions up to date. This is documented:
>
>https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html#firmware-version-management
>
>What would the pseudo code look like with "line cards" in the picture?
>Apply RFC1925 truth 12.

Something like this:

$lc_count = array_size(devlink-lc-info[$handle])

for ($lcnum = 0; $lcnum < $lc_count; lcnum++):
    $dev_count = array_size(devlink-lc-info[$handle][$lcnum])

    for ($devnum = 0; $devnum < $dev_count; $devnum++):
    
        # Get unique HW design identifier (gearbox id)
        $hw_id = devlink-lc-info[$handle][$lcnum][$devnum]['fw.psid']

        # Find out which FW flash we want to use for this device
        $want_flash_vers = some-db-backed.lookup($hw_id, 'flash')

        # Update flash if necessary
        if $want_flash_vers != devlink-lc-info[$handle][$lcnum][$devnum]['fw']:
            $file = some-db-backed.download($hw_id, 'flash')
            $component = devlink-lc[$handle][$lcnum][$devnum]['component']
            devlink-dev-flash($handle, $component, $file)

devlink-reload()

Clear indexes, not squashed somewhere in middle of string.


>
>> >> There is no new command for that, only one nested attribute which
>> >> carries the device list added to the existing command. They are no new
>> >> objects, they are just few nested values.  
>> >
>> >DEVLINK_CMD_LINECARD_INFO_GET  
>> 
>> Okay, that is not only to expose devices. That is also to expose info
>> about linecards, like HW revision, INI version etc. Where else to put
>> it? I can perhaps embed it into devlink dev info, but I thought separate
>> command would be more suitable. object cmd, object info cmd. It is
>> more clear I believe.
>
>> >> If so, how does the user know if/when to flash it?
>> >> If not, where would you list it if devices nest is not the correct place?  
>> >
>> >Let me mock up what I had in mind for you since it did not come thru 
>> >in the explanation:
>> >
>> >$ devlink dev info show pci/0000:01:00.0
>> >    versions:
>> >        fixed:
>> >          hw.revision 0
>> >          lc2.hw.revision a
>> >          lc8.hw.revision b
>> >        running:
>> >          ini.version 4
>> >          lc2.gearbox 1.1.3
>> >          lc8.gearbox 1.2.3  
>> 
>> Would be rather:
>> 
>>           lc2.gearbox0 1.1.3
>>           lc2.gearbox1 1.2.4
>
>I thought you said your gearboxes all the the same FW? 
>Theoretically, yes. Theoretically, I can also have nested "line cards".

Well, yeah. I was under impresion that possibility of having multiple
devices on the same LC is not close to 0. But I get your point.

Let's try to figure out he iface as you want it:
We will have devlink dev info extended to be able to provide info
about the LC/gearbox. Let's work with same prefix "lcX." for all
info related to line card X.

First problem is, who is going to enforce this prefix. It is driver's
responsibility to provide the string which is put out. The solution
would be to have a helper similar to devlink_info_version_*_put()
called devlink_info_lc_version_*_put() that would accept lc pointer and
add the prefix. Does it make sense to you?

We need 3 things:
1) current version of gearbox FW 
   That is easy, we have it - "versions"
   (DEVLINK_ATTR_INFO_VERSION_* nested attrs). We can have multiple
   nests that would carry the versions for individiual line cards.
   Example:
       versions:
           fixed:
             hw.revision 0
             lc2.hw.revision a
             lc8.hw.revision b
           running:
             ini.version 4
             lc2.gearbox.fw.version 1.1.3
             lc8.gearbox.fw.version 1.2.3
2) HW id (as you have it in your pseudocode), it is PSID in case of
   mlxsw. We already have PSID for ASIC:
   ....
   This should be also easy, as this is exposed as "fixed version" in a
   same way as previous example.
   Example:
       versions:
           fixed:
             lc2.gearbox.fw.psid XXX
             lc8.gearbox.fw.psid YYY
3) Component name
   This one is a bit tricky. It is not a version, so put it under
   "versions" does not make much sense.
   Also, there are more than one. Looking at how versions are done,
   multiple nests of attr type DEVLINK_ATTR_INFO_VERSION_* are put to
   the message. So we can introduce new attr DEVLINK_ATTR_INFO_FLASH_COMPONENT
   and put one per linecard/gearbox.
   Here arain we need some kind of helper to driver to call to put this
   into msg: devlink_info_lc_flash_component_put()
   Example:
     pci/0000:01:00.0:
       driver mlxsw_spectrum3
       flash_components:
         lc2.gearbox.fw
         lc8.gearbox.fw

    Then the flashing of the gearbox will be done by:
    # devlink dev flash pci/0000:01:00.0 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2 component lc2.gearbox.fw

    Maybe this would call for some sort of API between driver and devlink to
    register individial components. devlink.c can then sanitize the
    input value of the component according to the registered list.

    Even withot that I think this would be valuable anyway to let the
    user know what are the supported component names.

What do you think? If you agree, I can jump into implementing this and
you can feel free to revert this patchset.



>
>>           lc8.gearbox0 1.2.3
>> 
>> Okay, I see. So instead of having clear api with relationships and
>> clear human+machine readability we have squahed indexes into strings.
>> I fail to see the benefit, other than no-api-extension :/ On contrary.
>
>Show me the real life use for all the "clear api with relationships"
>and I'll shut up.

See the pseudo-code above.


>
>I would not take falling back to physical (HW) hierarchy for the API
>design as a point of pride. Seems lazy if I'm completely honest.

I got that. I spent a lot of time to find a good abstraction though.


>Someone else's HW may have a different hierarchy, and you're just
>forcing the automation engineer iterate over irrelevant structures
>("devices").

Well, "linecard device" could be anything on th LC, from gearbox to
whatever. It should fit. But I get your point.


>
>My hunch is that automation will not want to deal with line cards
>separately, and flash the entire devices in one go to a tested and
>verified bundle blob provided by the vendor. If they do want to poke 
>at line cards - the information is still there in what I described.
>
>> >$ devlink lc show pci/0000:01:00.0 lc 8
>> >pci/0000:01:00.0:
>> >  lc 8 state active type 16x100G
>> >    supported_types:
>> >      16x100G
>> >    versions: 
>> >      lc8.hw.revision (a) 
>> >      lc8.gearbox (1.2.3)
>> >
>> >Where the data in the brackets is optionally fetched thru the existing
>> >"dev info" API, but rendered together by the user space.  
>> 
>> Quite odd. I find it questionable to say at least to mix multiple
>> command netlink outputs into one output.
>
>Really? So we're going to be designing kernel APIs so that each message
>contains complete information and can't contain references now?

Can you give me an exapmple of devlink or any other iproute2 app cmd
that actually does something similar to this?



>
>> The processing of it would be a small nightmare considering the way
>> how the netlink message processing works in iproute2 :/

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Fri, 29 Apr 2022 13:51:33 +0200 Jiri Pirko wrote:
> >Of the three API levels (SDK, automation, human) I think automation
> >is the only one that's interesting to us in Linux. SDK interfaces are
> >necessarily too low level as they expose too much of internal details
> >to standardize. Humans are good with dealing with uncertainty and
> >diverse so there's no a good benchmark.
> >
> >The benchmark for automation is - can a machine use this API across
> >different vendors to reliably achieve its goals. For FW info/flashing
> >the goal is keeping the FW versions up to date. This is documented:
> >
> >https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html#firmware-version-management
> >
> >What would the pseudo code look like with "line cards" in the picture?
> >Apply RFC1925 truth 12.  
> 
> Something like this:
> 
> $lc_count = array_size(devlink-lc-info[$handle])
> 
> for ($lcnum = 0; $lcnum < $lc_count; lcnum++):
>     $dev_count = array_size(devlink-lc-info[$handle][$lcnum])
> 
>     for ($devnum = 0; $devnum < $dev_count; $devnum++):

Here goes the iteration I complained about in my previous message.
Tracking FW versions makes most sense at the level of a product (as 
in the unit of HW one can purchase from the system vendor). That
integrates well with system tracking HW in the fleet. Product in your
case will be a line card or populated chassis, I believe.

>         # Get unique HW design identifier (gearbox id)
>         $hw_id = devlink-lc-info[$handle][$lcnum][$devnum]['fw.psid']

1) you can't use 'fw.psid' in generic logic, it's a Melvidia's invention
2) looking at your cover letter there's no fw.psid for the device
   reported, the automation will not work, Q.E.D.

>         # Find out which FW flash we want to use for this device
>         $want_flash_vers = some-db-backed.lookup($hw_id, 'flash')
> 
>         # Update flash if necessary
>         if $want_flash_vers != devlink-lc-info[$handle][$lcnum][$devnum]['fw']:
>             $file = some-db-backed.download($hw_id, 'flash')
>             $component = devlink-lc[$handle][$lcnum][$devnum]['component']
>             devlink-dev-flash($handle, $component, $file)
> 
> devlink-reload()
> 
> Clear indexes, not squashed somewhere in middle of string.
> 
> >I thought you said your gearboxes all the the same FW? 
> >Theoretically, yes. Theoretically, I can also have nested "line cards".  
> 
> Well, yeah. I was under impresion that possibility of having multiple
> devices on the same LC is not close to 0. But I get your point.
> 
> Let's try to figure out he iface as you want it:
> We will have devlink dev info extended to be able to provide info
> about the LC/gearbox. Let's work with same prefix "lcX." for all
> info related to line card X.
> 
> First problem is, who is going to enforce this prefix. It is driver's
> responsibility to provide the string which is put out. The solution
> would be to have a helper similar to devlink_info_version_*_put()
> called devlink_info_lc_version_*_put() that would accept lc pointer and
> add the prefix. Does it make sense to you?
> 
> We need 3 things:
> 1) current version of gearbox FW 
>    That is easy, we have it - "versions"
>    (DEVLINK_ATTR_INFO_VERSION_* nested attrs). We can have multiple
>    nests that would carry the versions for individiual line cards.
>    Example:
>        versions:
>            fixed:
>              hw.revision 0
>              lc2.hw.revision a
>              lc8.hw.revision b
>            running:
>              ini.version 4
>              lc2.gearbox.fw.version 1.1.3
>              lc8.gearbox.fw.version 1.2.3
> 2) HW id (as you have it in your pseudocode), it is PSID in case of
>    mlxsw. We already have PSID for ASIC:
>    ....
>    This should be also easy, as this is exposed as "fixed version" in a
>    same way as previous example.
>    Example:
>        versions:
>            fixed:
>              lc2.gearbox.fw.psid XXX
>              lc8.gearbox.fw.psid YYY
> 3) Component name
>    This one is a bit tricky. It is not a version, so put it under
>    "versions" does not make much sense.
>    Also, there are more than one. Looking at how versions are done,
>    multiple nests of attr type DEVLINK_ATTR_INFO_VERSION_* are put to
>    the message. So we can introduce new attr DEVLINK_ATTR_INFO_FLASH_COMPONENT
>    and put one per linecard/gearbox.
>    Here arain we need some kind of helper to driver to call to put this
>    into msg: devlink_info_lc_flash_component_put()
>    Example:
>      pci/0000:01:00.0:
>        driver mlxsw_spectrum3
>        flash_components:
>          lc2.gearbox.fw
>          lc8.gearbox.fw
> 
>     Then the flashing of the gearbox will be done by:
>     # devlink dev flash pci/0000:01:00.0 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2 component lc2.gearbox.fw

The main question to me is whether users will want to flash the entire
device, or update line cards individually.

What's inside mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2? Doesn't
sound like it's FW just for a single gearbox?

> >Really? So we're going to be designing kernel APIs so that each message
> >contains complete information and can't contain references now?  
> 
> Can you give me an exapmple of devlink or any other iproute2 app cmd
> that actually does something similar to this?

Off the top of my head - doesn't ip has some caches for name resolution
etc.? Either way current code in iproute2.git is hardly written on the
stone tablets.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Fri, Apr 29, 2022 at 08:45:35PM CEST, kuba@kernel.org wrote:
>On Fri, 29 Apr 2022 13:51:33 +0200 Jiri Pirko wrote:
>> >Of the three API levels (SDK, automation, human) I think automation
>> >is the only one that's interesting to us in Linux. SDK interfaces are
>> >necessarily too low level as they expose too much of internal details
>> >to standardize. Humans are good with dealing with uncertainty and
>> >diverse so there's no a good benchmark.
>> >
>> >The benchmark for automation is - can a machine use this API across
>> >different vendors to reliably achieve its goals. For FW info/flashing
>> >the goal is keeping the FW versions up to date. This is documented:
>> >
>> >https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html#firmware-version-management
>> >
>> >What would the pseudo code look like with "line cards" in the picture?
>> >Apply RFC1925 truth 12.  
>> 
>> Something like this:
>> 
>> $lc_count = array_size(devlink-lc-info[$handle])
>> 
>> for ($lcnum = 0; $lcnum < $lc_count; lcnum++):
>>     $dev_count = array_size(devlink-lc-info[$handle][$lcnum])
>> 
>>     for ($devnum = 0; $devnum < $dev_count; $devnum++):
>
>Here goes the iteration I complained about in my previous message.
>Tracking FW versions makes most sense at the level of a product (as 
>in the unit of HW one can purchase from the system vendor). That
>integrates well with system tracking HW in the fleet. Product in your
>case will be a line card or populated chassis, I believe.

Well, most probably, you are right. In theory, there migth de 2 types of
devices/gearboxes on a single line card. I admit I find it unlikely now.


>
>>         # Get unique HW design identifier (gearbox id)
>>         $hw_id = devlink-lc-info[$handle][$lcnum][$devnum]['fw.psid']
>
>1) you can't use 'fw.psid' in generic logic, it's a Melvidia's invention
>2) looking at your cover letter there's no fw.psid for the device
>   reported, the automation will not work, Q.E.D.

We can make is a "symlink" to fw.hw_id or whatever. But that is not the
point here. For ASIC FW, we currently have also fw.psid.


>
>>         # Find out which FW flash we want to use for this device
>>         $want_flash_vers = some-db-backed.lookup($hw_id, 'flash')
>> 
>>         # Update flash if necessary
>>         if $want_flash_vers != devlink-lc-info[$handle][$lcnum][$devnum]['fw']:
>>             $file = some-db-backed.download($hw_id, 'flash')
>>             $component = devlink-lc[$handle][$lcnum][$devnum]['component']
>>             devlink-dev-flash($handle, $component, $file)
>> 
>> devlink-reload()
>> 
>> Clear indexes, not squashed somewhere in middle of string.
>> 
>> >I thought you said your gearboxes all the the same FW? 
>> >Theoretically, yes. Theoretically, I can also have nested "line cards".  
>> 
>> Well, yeah. I was under impresion that possibility of having multiple
>> devices on the same LC is not close to 0. But I get your point.
>> 
>> Let's try to figure out he iface as you want it:
>> We will have devlink dev info extended to be able to provide info
>> about the LC/gearbox. Let's work with same prefix "lcX." for all
>> info related to line card X.
>> 
>> First problem is, who is going to enforce this prefix. It is driver's
>> responsibility to provide the string which is put out. The solution
>> would be to have a helper similar to devlink_info_version_*_put()
>> called devlink_info_lc_version_*_put() that would accept lc pointer and
>> add the prefix. Does it make sense to you?
>> 
>> We need 3 things:
>> 1) current version of gearbox FW 
>>    That is easy, we have it - "versions"
>>    (DEVLINK_ATTR_INFO_VERSION_* nested attrs). We can have multiple
>>    nests that would carry the versions for individiual line cards.
>>    Example:
>>        versions:
>>            fixed:
>>              hw.revision 0
>>              lc2.hw.revision a
>>              lc8.hw.revision b
>>            running:
>>              ini.version 4
>>              lc2.gearbox.fw.version 1.1.3
>>              lc8.gearbox.fw.version 1.2.3
>> 2) HW id (as you have it in your pseudocode), it is PSID in case of
>>    mlxsw. We already have PSID for ASIC:
>>    ....
>>    This should be also easy, as this is exposed as "fixed version" in a
>>    same way as previous example.
>>    Example:
>>        versions:
>>            fixed:
>>              lc2.gearbox.fw.psid XXX
>>              lc8.gearbox.fw.psid YYY
>> 3) Component name
>>    This one is a bit tricky. It is not a version, so put it under
>>    "versions" does not make much sense.
>>    Also, there are more than one. Looking at how versions are done,
>>    multiple nests of attr type DEVLINK_ATTR_INFO_VERSION_* are put to
>>    the message. So we can introduce new attr DEVLINK_ATTR_INFO_FLASH_COMPONENT
>>    and put one per linecard/gearbox.
>>    Here arain we need some kind of helper to driver to call to put this
>>    into msg: devlink_info_lc_flash_component_put()
>>    Example:
>>      pci/0000:01:00.0:
>>        driver mlxsw_spectrum3
>>        flash_components:
>>          lc2.gearbox.fw
>>          lc8.gearbox.fw
>> 
>>     Then the flashing of the gearbox will be done by:
>>     # devlink dev flash pci/0000:01:00.0 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2 component lc2.gearbox.fw
>
>The main question to me is whether users will want to flash the entire
>device, or update line cards individually.

I think it makes sense to update them individually. The versions are
also reported individually. What's the benefit of not doing that. Also,
how would you name the "group" component. Sounds odd to me.


>
>What's inside mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2? Doesn't
>sound like it's FW just for a single gearbox?
>
>> >Really? So we're going to be designing kernel APIs so that each message
>> >contains complete information and can't contain references now?  
>> 
>> Can you give me an exapmple of devlink or any other iproute2 app cmd
>> that actually does something similar to this?
>
>Off the top of my head - doesn't ip has some caches for name resolution
>etc.? Either way current code in iproute2.git is hardly written on the
>stone tablets.

Not sure, that is why I thought you might have an example. Anyway, I
don't think it is important. I think that all 3 values exposed I
described above can be just in devlink dev info.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Fri, 29 Apr 2022 21:29:16 +0200 Jiri Pirko wrote:
> >The main question to me is whether users will want to flash the entire
> >device, or update line cards individually.  
> 
> I think it makes sense to update them individually. The versions are
> also reported individually.

Okay, but neither I want that, nor does it match what Ido described as
the direction for mlxsw, quoting:

  The idea (implemented in the next patchset) is to let these devices
  expose their own "component name", which can then be plugged into the
  existing flash command:

    $ devlink lc show pci/0000:01:00.0 lc 8
    pci/0000:01:00.0:
      lc 8 state active type 16x100G
        supported_types:
           16x100G
        devices:
          device 0 flashable true component lc8_dev0
          device 1 flashable false
          device 2 flashable false
          device 3 flashable false
    $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2 component lc8_dev0

Your "devices" are _not_ individually flashable. It seems natural for
single-board devices like a NIC or a line card to have a single flash
with all the images burned together.

> What's the benefit of not doing that.

As already mentioned in my previous reply the user will likely have 
a database of all their networking assets, and having to break them
up further than the physical piece of gear they order from the supplier
is a pain. Plus the vendor will likely also prefer to ship a single
validated image rather than a blob for every board component with FW.

> Also, how would you name the "group" component. Sounds odd to me.

To flash the whole device we skip the component.

> >What's inside mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2? Doesn't
> >sound like it's FW just for a single gearbox?

Please answer questions. I already complained about this once in 
this thread.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Sat, Apr 30, 2022 at 12:38:45AM CEST, kuba@kernel.org wrote:
>On Fri, 29 Apr 2022 21:29:16 +0200 Jiri Pirko wrote:
>> >The main question to me is whether users will want to flash the entire
>> >device, or update line cards individually.  
>> 
>> I think it makes sense to update them individually. The versions are
>> also reported individually.
>
>Okay, but neither I want that, nor does it match what Ido described as
>the direction for mlxsw, quoting:
>
>  The idea (implemented in the next patchset) is to let these devices
>  expose their own "component name", which can then be plugged into the
>  existing flash command:
>
>    $ devlink lc show pci/0000:01:00.0 lc 8
>    pci/0000:01:00.0:
>      lc 8 state active type 16x100G
>        supported_types:
>           16x100G
>        devices:
>          device 0 flashable true component lc8_dev0
>          device 1 flashable false
>          device 2 flashable false
>          device 3 flashable false
>    $ devlink dev flash pci/0000:01:00.0 file some_file.mfa2 component lc8_dev0
>
>Your "devices" are _not_ individually flashable. It seems natural for
>single-board devices like a NIC or a line card to have a single flash
>with all the images burned together.

Wait a second. I think that we don't understand each other. Currently,
we have a single device to flash on a linecard, the gearbox.
There is one file to flash it. So 1:1 between line card and file to
flash. That is exactly as I described in the proposal. 1 component name
per line card.


>
>> What's the benefit of not doing that.
>
>As already mentioned in my previous reply the user will likely have 
>a database of all their networking assets, and having to break them
>up further than the physical piece of gear they order from the supplier
>is a pain. Plus the vendor will likely also prefer to ship a single
>validated image rather than a blob for every board component with FW.

Depends on the vendor :) But it is hypothetical, let's see what the
future brings. But I agree with you.


>
>> Also, how would you name the "group" component. Sounds odd to me.
>
>To flash the whole device we skip the component.

Sec. I think these is a misunderstanding here. The component it what we
already have in devlink dev flash. Quoting devlink-dev man page:
   devlink dev flash - write device's non-volatile memory.
       DEV - specifies the devlink device to write to.

       file PATH - Path to the file which will be written into device's flash.
       The path needs to be relative to one of the directories searched by the
       kernel firmware loaded, such as /lib/firmware.

---->  component NAME - If device stores multiple firmware images in non-
       volatile memory, this parameter may be used to indicate which firmware
       image should be written.  The value of NAME should match the component
       names from devlink dev info and may be driver-dependent.

This is currently not used in devlink capable drivers. It is a concept
taken from ethtool (I think you were the one that requested to take it,
but my memory could be wrong).
Anyway, the default is component NULL. In case of mlxsw, in that case
the target is ASIC FW.

Now I just want to use this component name to target individual line
cards. I see it is a nice fit. Don't you think?

I see that the manpage is mentioning "the component names from devlink dev info"
which is not actually implemented, but exactly what I proposed.


>
>> >What's inside mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2? Doesn't
>> >sound like it's FW just for a single gearbox?
>
>Please answer questions. I already complained about this once in 
>this thread.

Sorry, I missed this one. The file IS a FW just for a SINGLE gearbox.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Sat, 30 Apr 2022 08:27:35 +0200 Jiri Pirko wrote:
> Now I just want to use this component name to target individual line
> cards. I see it is a nice fit. Don't you think?

Still on the fence.

> I see that the manpage is mentioning "the component names from devlink dev info"
> which is not actually implemented, but exactly what I proposed.

How do you tie the line card to the component name? lc8_dev0 from 
the flashing example is not present in the lc info output.

> >Please answer questions. I already complained about this once in 
> >this thread.  
> 
> Sorry, I missed this one. The file IS a FW just for a SINGLE gearbox.

I see.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

First of all, sorry for delay :/


Mon, May 02, 2022 at 04:39:33PM CEST, kuba@kernel.org wrote:
>On Sat, 30 Apr 2022 08:27:35 +0200 Jiri Pirko wrote:
>> Now I just want to use this component name to target individual line
>> cards. I see it is a nice fit. Don't you think?
>
>Still on the fence.

Why?

>
>> I see that the manpage is mentioning "the component names from devlink dev info"
>> which is not actually implemented, but exactly what I proposed.
>
>How do you tie the line card to the component name? lc8_dev0 from 
>the flashing example is not present in the lc info output.

Okay, I will move it there. Makes sense.


>
>> >Please answer questions. I already complained about this once in 
>> >this thread.  
>> 
>> Sorry, I missed this one. The file IS a FW just for a SINGLE gearbox.
>
>I see.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Mon, 23 May 2022 11:42:07 +0200 Jiri Pirko wrote:
> Mon, May 02, 2022 at 04:39:33PM CEST, kuba@kernel.org wrote:
> >On Sat, 30 Apr 2022 08:27:35 +0200 Jiri Pirko wrote:  
> >> Now I just want to use this component name to target individual line
> >> cards. I see it is a nice fit. Don't you think?  
> >
> >Still on the fence.  
> 
> Why?

IIRC my concern was mixing objects. We have component name coming from
lc info, but then use it in dev flash.

> >> I see that the manpage is mentioning "the component names from devlink dev info"
> >> which is not actually implemented, but exactly what I proposed.  
> >
> >How do you tie the line card to the component name? lc8_dev0 from 
> >the flashing example is not present in the lc info output.  
> 
> Okay, I will move it there. Makes sense.

FWIW I think I meant my comment as a way to underline that what you
argue for is not what's implemented (assuming your "not actually
implemented" referred to the flashing). I was trying to send you back 
to the drawing board rather than break open a box of band-aides.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Mon, May 23, 2022 at 07:56:40PM CEST, kuba@kernel.org wrote:
>On Mon, 23 May 2022 11:42:07 +0200 Jiri Pirko wrote:
>> Mon, May 02, 2022 at 04:39:33PM CEST, kuba@kernel.org wrote:
>> >On Sat, 30 Apr 2022 08:27:35 +0200 Jiri Pirko wrote:  
>> >> Now I just want to use this component name to target individual line
>> >> cards. I see it is a nice fit. Don't you think?  
>> >
>> >Still on the fence.  
>> 
>> Why?
>
>IIRC my concern was mixing objects. We have component name coming from
>lc info, but then use it in dev flash.

Sure. I considered that. The thing is, even if you put the lc component
names to output of "devlink dev info", you would need to provide lc
objects as well (somehow) - to contain the versions.

But the component name is related to lc object listed in "devlink lc",
so "devlink lc info" sounds line the correct place to put it.

If you are concern about "devlink dev flash" using component name from
"devlink lc info", I would rather introduce "devlink lc flash" so you
have a match. But from what I see, I don't really see the necessity for
this match. Do you?


>
>> >> I see that the manpage is mentioning "the component names from devlink dev info"
>> >> which is not actually implemented, but exactly what I proposed.  
>> >
>> >How do you tie the line card to the component name? lc8_dev0 from 
>> >the flashing example is not present in the lc info output.  
>> 
>> Okay, I will move it there. Makes sense.
>
>FWIW I think I meant my comment as a way to underline that what you
>argue for is not what's implemented (assuming your "not actually
>implemented" referred to the flashing). I was trying to send you back 
>to the drawing board rather than break open a box of band-aides.

Sure, lets do this right, I don't want to band-aide anything...

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, May 24, 2022 at 08:46:38AM CEST, jiri@resnulli.us wrote:
>Mon, May 23, 2022 at 07:56:40PM CEST, kuba@kernel.org wrote:
>>On Mon, 23 May 2022 11:42:07 +0200 Jiri Pirko wrote:
>>> Mon, May 02, 2022 at 04:39:33PM CEST, kuba@kernel.org wrote:
>>> >On Sat, 30 Apr 2022 08:27:35 +0200 Jiri Pirko wrote:  
>>> >> Now I just want to use this component name to target individual line
>>> >> cards. I see it is a nice fit. Don't you think?  
>>> >
>>> >Still on the fence.  
>>> 
>>> Why?
>>
>>IIRC my concern was mixing objects. We have component name coming from
>>lc info, but then use it in dev flash.
>
>Sure. I considered that. The thing is, even if you put the lc component
>names to output of "devlink dev info", you would need to provide lc
>objects as well (somehow) - to contain the versions.
>
>But the component name is related to lc object listed in "devlink lc",
>so "devlink lc info" sounds line the correct place to put it.
>
>If you are concern about "devlink dev flash" using component name from
>"devlink lc info", I would rather introduce "devlink lc flash" so you
>have a match. But from what I see, I don't really see the necessity for
>this match. Do you?

Okay, we can eventually avoid using component name at all for now,
considering one flash object per linecard (with possibility to extend by
component later on). This would look like:

$ devlink lc info pci/0000:01:00.0 lc 8
pci/0000:01:00.0:
  lc 8
    versions:
        fixed:
          hw.revision 0
	  fw.psid MT_0000000749
        running:
          ini.version 4
          fw 19.2010.1310

$ devlink lc flash pci/0000:01:00.0 lc 8 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2

I have to admit I like this.
We would reuse the existing DEVLINK_CMD_FLASH_UPDATE cmd and when
DEVLINK_ATTR_LINECARD_INDEX attribute is present, we call the lc-flash
op. How does this sound?


>
>
>>
>>> >> I see that the manpage is mentioning "the component names from devlink dev info"
>>> >> which is not actually implemented, but exactly what I proposed.  
>>> >
>>> >How do you tie the line card to the component name? lc8_dev0 from 
>>> >the flashing example is not present in the lc info output.  
>>> 
>>> Okay, I will move it there. Makes sense.
>>
>>FWIW I think I meant my comment as a way to underline that what you
>>argue for is not what's implemented (assuming your "not actually
>>implemented" referred to the flashing). I was trying to send you back 
>>to the drawing board rather than break open a box of band-aides.
>
>Sure, lets do this right, I don't want to band-aide anything...

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Tue, 24 May 2022 16:31:45 +0200 Jiri Pirko wrote:
> >Sure. I considered that. The thing is, even if you put the lc component
> >names to output of "devlink dev info", you would need to provide lc
> >objects as well (somehow) - to contain the versions.
> >
> >But the component name is related to lc object listed in "devlink lc",
> >so "devlink lc info" sounds line the correct place to put it.
> >
> >If you are concern about "devlink dev flash" using component name from
> >"devlink lc info", I would rather introduce "devlink lc flash" so you
> >have a match. But from what I see, I don't really see the necessity for
> >this match. Do you?  
> 
> Okay, we can eventually avoid using component name at all for now,
> considering one flash object per linecard (with possibility to extend by
> component later on). This would look like:
> 
> $ devlink lc info pci/0000:01:00.0 lc 8
> pci/0000:01:00.0:
>   lc 8
>     versions:
>         fixed:
>           hw.revision 0
> 	  fw.psid MT_0000000749
>         running:
>           ini.version 4
>           fw 19.2010.1310
> 
> $ devlink lc flash pci/0000:01:00.0 lc 8 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
> 
> I have to admit I like this.
> We would reuse the existing DEVLINK_CMD_FLASH_UPDATE cmd and when
> DEVLINK_ATTR_LINECARD_INDEX attribute is present, we call the lc-flash
> op. How does this sound?

We talked about this earlier in the thread, I think. If you need both
info and flash per LC just make them a separate devlink instance and
let them have all the objects they need. Then just put the instance
name under lc info.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, May 24, 2022 at 08:00:57PM CEST, kuba@kernel.org wrote:
>On Tue, 24 May 2022 16:31:45 +0200 Jiri Pirko wrote:
>> >Sure. I considered that. The thing is, even if you put the lc component
>> >names to output of "devlink dev info", you would need to provide lc
>> >objects as well (somehow) - to contain the versions.
>> >
>> >But the component name is related to lc object listed in "devlink lc",
>> >so "devlink lc info" sounds line the correct place to put it.
>> >
>> >If you are concern about "devlink dev flash" using component name from
>> >"devlink lc info", I would rather introduce "devlink lc flash" so you
>> >have a match. But from what I see, I don't really see the necessity for
>> >this match. Do you?  
>> 
>> Okay, we can eventually avoid using component name at all for now,
>> considering one flash object per linecard (with possibility to extend by
>> component later on). This would look like:
>> 
>> $ devlink lc info pci/0000:01:00.0 lc 8
>> pci/0000:01:00.0:
>>   lc 8
>>     versions:
>>         fixed:
>>           hw.revision 0
>> 	  fw.psid MT_0000000749
>>         running:
>>           ini.version 4
>>           fw 19.2010.1310
>> 
>> $ devlink lc flash pci/0000:01:00.0 lc 8 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
>> 
>> I have to admit I like this.
>> We would reuse the existing DEVLINK_CMD_FLASH_UPDATE cmd and when
>> DEVLINK_ATTR_LINECARD_INDEX attribute is present, we call the lc-flash
>> op. How does this sound?
>
>We talked about this earlier in the thread, I think. If you need both
>info and flash per LC just make them a separate devlink instance and
>let them have all the objects they need. Then just put the instance
>name under lc info.

I don't follow :/ What do you mean be "separate devlink instance" here?
Could you draw me an example?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Wed, 25 May 2022 08:20:45 +0200 Jiri Pirko wrote:
> >We talked about this earlier in the thread, I think. If you need both
> >info and flash per LC just make them a separate devlink instance and
> >let them have all the objects they need. Then just put the instance
> >name under lc info.  
> 
> I don't follow :/ What do you mean be "separate devlink instance" here?
> Could you draw me an example?

Separate instance:

	for (i = 0; i < sw->num_lcs; i++) {
		devlink_register(&sw->lc_dl[i]);
		devlink_line_card_link(&sw->lc[i], &sw->lc_dl[i]);
	}

then report that under the linecard

	nla_nest_start(msg, DEVLINK_SUBORDINATE_INSTANCE);
	devlink_nl_put_handle(msg, lc->devlink);
	nla_nest_end(msg...)

then user can update the linecard like any devlink instance, switch,
NIC etc. It's better code reuse and I don't see any downside, TBH.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Wed, May 25, 2022 at 05:50:54PM CEST, kuba@kernel.org wrote:
>On Wed, 25 May 2022 08:20:45 +0200 Jiri Pirko wrote:
>> >We talked about this earlier in the thread, I think. If you need both
>> >info and flash per LC just make them a separate devlink instance and
>> >let them have all the objects they need. Then just put the instance
>> >name under lc info.  
>> 
>> I don't follow :/ What do you mean be "separate devlink instance" here?
>> Could you draw me an example?
>
>Separate instance:
>
>	for (i = 0; i < sw->num_lcs; i++) {
>		devlink_register(&sw->lc_dl[i]);
>		devlink_line_card_link(&sw->lc[i], &sw->lc_dl[i]);
>	}
>
>then report that under the linecard
>
>	nla_nest_start(msg, DEVLINK_SUBORDINATE_INSTANCE);
>	devlink_nl_put_handle(msg, lc->devlink);
>	nla_nest_end(msg...)
>
>then user can update the linecard like any devlink instance, switch,
>NIC etc. It's better code reuse and I don't see any downside, TBH.

I think you already suggested something like that. What you mean is that
linecard would be modeled as any other devlink instance. Sorry, but that
does not make any sense to me at all :/ Should devlink port not be
devlink port and rather modeled as a separate devlink instance too? Same
to the rest of the objects we already have. Should we have a tree of
same devlink objects each overloaded to some specific type. If yes, that
is completely changing the devlink modelling.

The handle of the devlink object is bus_name/dev_name. In other words,
there is a device on a bus and for each you can create devlink instance.
Linecards is not on a any bus, is is not modeled in /sys. What would be
the handle?

What you suggest seems to me like completely broken :/

I don't understand the motivation :/ The only I can think of that you
will have the same "devlink dev flash" mechanism, but that's it.
I'm not sure I understand why we cannot resolve the flashing otherwise,
for example as I suggested with "devlink lc info" and "devlink lc
flash". What is wrong with that?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Thu, May 26, 2022 at 11:05:53AM CEST, jiri@resnulli.us wrote:
>Wed, May 25, 2022 at 05:50:54PM CEST, kuba@kernel.org wrote:
>>On Wed, 25 May 2022 08:20:45 +0200 Jiri Pirko wrote:
>>> >We talked about this earlier in the thread, I think. If you need both
>>> >info and flash per LC just make them a separate devlink instance and
>>> >let them have all the objects they need. Then just put the instance
>>> >name under lc info.  
>>> 
>>> I don't follow :/ What do you mean be "separate devlink instance" here?
>>> Could you draw me an example?
>>
>>Separate instance:
>>
>>	for (i = 0; i < sw->num_lcs; i++) {
>>		devlink_register(&sw->lc_dl[i]);
>>		devlink_line_card_link(&sw->lc[i], &sw->lc_dl[i]);
>>	}
>>
>>then report that under the linecard
>>
>>	nla_nest_start(msg, DEVLINK_SUBORDINATE_INSTANCE);
>>	devlink_nl_put_handle(msg, lc->devlink);
>>	nla_nest_end(msg...)
>>
>>then user can update the linecard like any devlink instance, switch,
>>NIC etc. It's better code reuse and I don't see any downside, TBH.
>
>I think you already suggested something like that. What you mean is that
>linecard would be modeled as any other devlink instance. Sorry, but that
>does not make any sense to me at all :/ Should devlink port not be
>devlink port and rather modeled as a separate devlink instance too? Same
>to the rest of the objects we already have. Should we have a tree of
>same devlink objects each overloaded to some specific type. If yes, that
>is completely changing the devlink modelling.
>
>The handle of the devlink object is bus_name/dev_name. In other words,
>there is a device on a bus and for each you can create devlink instance.
>Linecards is not on a any bus, is is not modeled in /sys. What would be
>the handle?
>
>What you suggest seems to me like completely broken :/

After some thinking I think I understand why you suggest this.
The versions in dev info and dev flash is there on devlink instance level,
so you would like to reuse it.

But it does not click. Having devlink instance just for this feels very
artificial, unfitting and heavy. IDK.


>
>I don't understand the motivation :/ The only I can think of that you
>will have the same "devlink dev flash" mechanism, but that's it.
>I'm not sure I understand why we cannot resolve the flashing otherwise,
>for example as I suggested with "devlink lc info" and "devlink lc
>flash". What is wrong with that?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Wed, May 25, 2022 at 05:50:54PM CEST, kuba@kernel.org wrote:
>On Wed, 25 May 2022 08:20:45 +0200 Jiri Pirko wrote:
>> >We talked about this earlier in the thread, I think. If you need both
>> >info and flash per LC just make them a separate devlink instance and
>> >let them have all the objects they need. Then just put the instance
>> >name under lc info.  
>> 
>> I don't follow :/ What do you mean be "separate devlink instance" here?
>> Could you draw me an example?
>
>Separate instance:
>
>	for (i = 0; i < sw->num_lcs; i++) {
>		devlink_register(&sw->lc_dl[i]);
>		devlink_line_card_link(&sw->lc[i], &sw->lc_dl[i]);
>	}
>
>then report that under the linecard
>
>	nla_nest_start(msg, DEVLINK_SUBORDINATE_INSTANCE);
>	devlink_nl_put_handle(msg, lc->devlink);
>	nla_nest_end(msg...)
>
>then user can update the linecard like any devlink instance, switch,
>NIC etc. It's better code reuse and I don't see any downside, TBH.

Okay, I was thinking about this a litle bit more, and I would like to
explore extending the components path. Exposing the components in
"devlink dev info" and then using them in "devlink dev flash". LC could
be just one of multiple potential users of components. Will send RFC
soon.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Thu, 26 May 2022 13:45:49 +0200 Jiri Pirko wrote:
> >Separate instance:
> >
> >	for (i = 0; i < sw->num_lcs; i++) {
> >		devlink_register(&sw->lc_dl[i]);
> >		devlink_line_card_link(&sw->lc[i], &sw->lc_dl[i]);
> >	}
> >
> >then report that under the linecard
> >
> >	nla_nest_start(msg, DEVLINK_SUBORDINATE_INSTANCE);
> >	devlink_nl_put_handle(msg, lc->devlink);
> >	nla_nest_end(msg...)
> >
> >then user can update the linecard like any devlink instance, switch,
> >NIC etc. It's better code reuse and I don't see any downside, TBH.  
> 
> Okay, I was thinking about this a litle bit more, and I would like to
> explore extending the components path. Exposing the components in
> "devlink dev info" and then using them in "devlink dev flash". LC could
> be just one of multiple potential users of components. Will send RFC
> soon.

Feel free to send a mockup of the devlink user space outputs.
The core code for devlink is just meaningless marshaling anyway.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Thu, May 26, 2022 at 07:35:39PM CEST, kuba@kernel.org wrote:
>On Thu, 26 May 2022 13:45:49 +0200 Jiri Pirko wrote:
>> >Separate instance:
>> >
>> >	for (i = 0; i < sw->num_lcs; i++) {
>> >		devlink_register(&sw->lc_dl[i]);
>> >		devlink_line_card_link(&sw->lc[i], &sw->lc_dl[i]);
>> >	}
>> >
>> >then report that under the linecard
>> >
>> >	nla_nest_start(msg, DEVLINK_SUBORDINATE_INSTANCE);
>> >	devlink_nl_put_handle(msg, lc->devlink);
>> >	nla_nest_end(msg...)
>> >
>> >then user can update the linecard like any devlink instance, switch,
>> >NIC etc. It's better code reuse and I don't see any downside, TBH.  
>> 
>> Okay, I was thinking about this a litle bit more, and I would like to
>> explore extending the components path. Exposing the components in
>> "devlink dev info" and then using them in "devlink dev flash". LC could
>> be just one of multiple potential users of components. Will send RFC
>> soon.
>
>Feel free to send a mockup of the devlink user space outputs.
>The core code for devlink is just meaningless marshaling anyway.

Okay. So the output of devlink dev info would be extended by
"components" nest. This nest would carry array of components which
contain versions. The name of the component is openin each array member
nest:

$ devlink dev info
pci/0000:01:00.0:
  driver mlxsw_spectrum2
  versions:
      fixed:
        hw.revision A0
        fw.psid MT_0000000199
      running:
        fw.version 29.2010.2302
        fw 29.2010.2302
  components:
    lc1:
      versions:
        fixed:
          hw.revision 0
          fw.psid MT_0000000111
        running:
          fw 19.2010.1310
          ini.version 4
    lc2:
      versions:
        fixed:
          hw.revision 0
          fw.psid MT_0000000111
        running:
          fw 19.2010.1310
          ini.version 4
    someothercomponentname:
      versions:
        running:
	   fw: 888

Now on top of exsisting "devlink dev flash" cmd without component, user
may specify the component name from the array above:

$ devlink dev flash pci/0000:01:00.0 component lc1 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2

$ devlink dev flash pci/0000:01:00.0 component someothercomponentname file foo.bin

Note this is generic vehicle, line cards would benefit but it is usable
for multiple ASIC FW partitions for example.

Note that on "devlink dev flash" there is no change. This is implemented
currently. Only "devlink dev info" is extended to show the component
list.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Fri, 27 May 2022 09:27:47 +0200 Jiri Pirko wrote:
> Okay. So the output of devlink dev info would be extended by
> "components" nest. This nest would carry array of components which
> contain versions. The name of the component is openin each array member
> nest:
> 
> $ devlink dev info
> pci/0000:01:00.0:
>   driver mlxsw_spectrum2
>   versions:
>       fixed:
>         hw.revision A0
>         fw.psid MT_0000000199
>       running:
>         fw.version 29.2010.2302
>         fw 29.2010.2302
>   components:
>     lc1:

Is the "lc1" free-form or generated by the core based on subobjects?
Is it carried as a string or object type + id?

I guess my suggestion of a CLI mockup has proven its weakness :)

>       versions:
>         fixed:
>           hw.revision 0
>           fw.psid MT_0000000111
>         running:
>           fw 19.2010.1310
>           ini.version 4
>     lc2:
>       versions:
>         fixed:
>           hw.revision 0
>           fw.psid MT_0000000111
>         running:
>           fw 19.2010.1310
>           ini.version 4
>     someothercomponentname:
>       versions:
>         running:
> 	   fw: 888
> 
> Now on top of exsisting "devlink dev flash" cmd without component, user
> may specify the component name from the array above:
> 
> $ devlink dev flash pci/0000:01:00.0 component lc1 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
> 
> $ devlink dev flash pci/0000:01:00.0 component someothercomponentname file foo.bin
> 
> Note this is generic vehicle, line cards would benefit but it is usable
> for multiple ASIC FW partitions for example.
> 
> Note that on "devlink dev flash" there is no change. This is implemented
> currently. Only "devlink dev info" is extended to show the component
> list.

I sort of assumed that the DEVLINK_ATTR_INFO_VERSION_NAME is the
component, the docs also use the word "component" for it. 

For the nfp for instance we had "fw.app" for the datapath microcode and
"fw.mgmt" for the control processor. These are separate partitions on
the flash. I don't think we ever implemented writing them separately
but it's certainly was our internal plan at some point.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Sat, May 28, 2022 at 02:10:38AM CEST, kuba@kernel.org wrote:
>On Fri, 27 May 2022 09:27:47 +0200 Jiri Pirko wrote:
>> Okay. So the output of devlink dev info would be extended by
>> "components" nest. This nest would carry array of components which
>> contain versions. The name of the component is openin each array member
>> nest:
>> 
>> $ devlink dev info
>> pci/0000:01:00.0:
>>   driver mlxsw_spectrum2
>>   versions:
>>       fixed:
>>         hw.revision A0
>>         fw.psid MT_0000000199
>>       running:
>>         fw.version 29.2010.2302
>>         fw 29.2010.2302
>>   components:
>>     lc1:
>
>Is the "lc1" free-form or generated by the core based on subobjects?
>Is it carried as a string or object type + id?

It could be both:
1) for line cards I plan to have a helper to have this generated by core
2) for other FW objects, it is up to the driver.


>
>I guess my suggestion of a CLI mockup has proven its weakness :)

I'm not sure I understand what you mean by this sentence. Could you
please be more blunt? You know, my english is not so good to understand
some hidden meanings :)



>
>>       versions:
>>         fixed:
>>           hw.revision 0
>>           fw.psid MT_0000000111
>>         running:
>>           fw 19.2010.1310
>>           ini.version 4
>>     lc2:
>>       versions:
>>         fixed:
>>           hw.revision 0
>>           fw.psid MT_0000000111
>>         running:
>>           fw 19.2010.1310
>>           ini.version 4
>>     someothercomponentname:
>>       versions:
>>         running:
>> 	   fw: 888
>> 
>> Now on top of exsisting "devlink dev flash" cmd without component, user
>> may specify the component name from the array above:
>> 
>> $ devlink dev flash pci/0000:01:00.0 component lc1 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
>> 
>> $ devlink dev flash pci/0000:01:00.0 component someothercomponentname file foo.bin
>> 
>> Note this is generic vehicle, line cards would benefit but it is usable
>> for multiple ASIC FW partitions for example.
>> 
>> Note that on "devlink dev flash" there is no change. This is implemented
>> currently. Only "devlink dev info" is extended to show the component
>> list.
>
>I sort of assumed that the DEVLINK_ATTR_INFO_VERSION_NAME is the
>component, the docs also use the word "component" for it. 

Okay, that I didn't see.

>
>For the nfp for instance we had "fw.app" for the datapath microcode and
>"fw.mgmt" for the control processor. These are separate partitions on
>the flash. I don't think we ever implemented writing them separately
>but it's certainly was our internal plan at some point.

Okay, so what you say it, we already have components in "devlink dev
info". Like you pointed out as an example:
  fw.app
  fw.mgmt
so the flash comment would be:
  devlink dev flash pci/0000:01:00.0 component fw.app file foo.bin
  devlink dev flash pci/0000:01:00.0 component fw.mgmt file bar.bin
?

If yes, what should be the default in case component is not defined? Do
we need to expose it in "devlink dev info"? How?

So to extend this existing facility with my line card example, we would
have:

$ devlink dev info
pci/0000:01:00.0:
   driver mlxsw_spectrum2
   versions:
       fixed:
         hw.revision A0
         fw.psid MT_0000000199
	 lc1.hw.revision 0
	 lc1.fw.psid MT_0000000111
	 lc2.hw.revision 0
	 lc2.fw.psid MT_0000000111
       running:
         fw.version 29.2010.2302
         fw 29.2010.2302
	 lc1.fw 19.2010.1310
	 lc1.ini.version 4
	 lc2.fw 19.2010.1310
	 lc2.ini.version 4

And then:
devlink dev flash pci/0000:01:00.0 component lc1.fw file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2

Does this sound correct?

Also, to avoid free-form, I can imagine to have per-linecard info_get() op
which would be called for each line card from devlink_nl_info_fill() and
prefix the "lcX" automatically without driver being involved.

Sounds good?

Thanks!

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[David Ahern]

On 5/24/22 8:31 AM, Jiri Pirko wrote:
> 
> $ devlink lc info pci/0000:01:00.0 lc 8
> pci/0000:01:00.0:

...

> 
> $ devlink lc flash pci/0000:01:00.0 lc 8 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2


A lot of your proposed syntax for devlink commands has 'lc' twice. If
'lc' is the subcommand, then you are managing a linecard making 'lc'
before the '8' redundant. How about 'slot 8' or something along those lines?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Sat, 28 May 2022 11:09:01 +0200 Jiri Pirko wrote:
> Sat, May 28, 2022 at 02:10:38AM CEST, kuba@kernel.org wrote:
> >
> >Is the "lc1" free-form or generated by the core based on subobjects?
> >Is it carried as a string or object type + id?  
> 
> It could be both:
> 1) for line cards I plan to have a helper to have this generated by core
> 2) for other FW objects, it is up to the driver.

Did you mean "either" or "both"?

> >I guess my suggestion of a CLI mockup has proven its weakness :)  
> 
> I'm not sure I understand what you mean by this sentence. Could you
> please be more blunt? You know, my english is not so good to understand
> some hidden meanings :)

The question of what kind of attribute "lc1" is carried in would had
been answered in posting of a code, while CLI mockup doesn't provide
such detail.

> >
> >I sort of assumed that the DEVLINK_ATTR_INFO_VERSION_NAME is the
> >component, the docs also use the word "component" for it.   
> 
> Okay, that I didn't see.
> 
> >
> >For the nfp for instance we had "fw.app" for the datapath microcode and
> >"fw.mgmt" for the control processor. These are separate partitions on
> >the flash. I don't think we ever implemented writing them separately
> >but it's certainly was our internal plan at some point.  
> 
> Okay, so what you say it, we already have components in "devlink dev
> info". Like you pointed out as an example:
>   fw.app
>   fw.mgmt
> so the flash comment would be:
>   devlink dev flash pci/0000:01:00.0 component fw.app file foo.bin
>   devlink dev flash pci/0000:01:00.0 component fw.mgmt file bar.bin
> ?

Correct.

> If yes, what should be the default in case component is not defined? Do
> we need to expose it in "devlink dev info"? How?

Not defined as in someone tries to flash component X but there is no
version for X in info?

> So to extend this existing facility with my line card example, we would
> have:
> 
> $ devlink dev info
> pci/0000:01:00.0:
>    driver mlxsw_spectrum2
>    versions:
>        fixed:
>          hw.revision A0
>          fw.psid MT_0000000199
> 	 lc1.hw.revision 0
> 	 lc1.fw.psid MT_0000000111
> 	 lc2.hw.revision 0
> 	 lc2.fw.psid MT_0000000111
>        running:
>          fw.version 29.2010.2302
>          fw 29.2010.2302
> 	 lc1.fw 19.2010.1310
> 	 lc1.ini.version 4
> 	 lc2.fw 19.2010.1310
> 	 lc2.ini.version 4
> 
> And then:
> devlink dev flash pci/0000:01:00.0 component lc1.fw file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
> 
> Does this sound correct?

I think I suggested something like that in the past, but back then 
I was assuming that lc FW would come from the same large FW bundle
file as the control plan FW, and we would not have to use the component.

Let's step back and look from the automation perspective again.
Assuming we don't want to hardcode matching "lc$i" there how can 
a generic FW update service scan the dev info and decide on what
dev flash command to fire off?

> Also, to avoid free-form, I can imagine to have per-linecard info_get() op
> which would be called for each line card from devlink_nl_info_fill() and
> prefix the "lcX" automatically without driver being involved.
> 
> Sounds good?

Hm. That's moving the matryoshka-ing of the objects from the uAPI level
to the internals. 

If we don't do the string prefix but instead pass the subobject info to
the user space as an attribute per version we can at least avoid
per-subobject commands (DEVLINK_CMD_LINECARD_INFO_GET). Much closer to
how health reporters are implemented than how params are done, so I
think it is a good direction.

We still need to iron out how the automation can go over the main FW
and sub-objects in a generic way.

I still think full devlink sub-instance is better because we will end
up needing params or health. Fake devices can be made with auxbus or
otherwise. But if you really don't want sub-instances we can explore 
the above.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Sat, May 28, 2022 at 09:02:53PM CEST, kuba@kernel.org wrote:
>On Sat, 28 May 2022 11:09:01 +0200 Jiri Pirko wrote:
>> Sat, May 28, 2022 at 02:10:38AM CEST, kuba@kernel.org wrote:
>> >
>> >Is the "lc1" free-form or generated by the core based on subobjects?
>> >Is it carried as a string or object type + id?  
>> 
>> It could be both:
>> 1) for line cards I plan to have a helper to have this generated by core
>> 2) for other FW objects, it is up to the driver.
>
>Did you mean "either" or "both"?

Both.


>
>> >I guess my suggestion of a CLI mockup has proven its weakness :)  
>> 
>> I'm not sure I understand what you mean by this sentence. Could you
>> please be more blunt? You know, my english is not so good to understand
>> some hidden meanings :)
>
>The question of what kind of attribute "lc1" is carried in would had
>been answered in posting of a code, while CLI mockup doesn't provide
>such detail.
>
>> >
>> >I sort of assumed that the DEVLINK_ATTR_INFO_VERSION_NAME is the
>> >component, the docs also use the word "component" for it.   
>> 
>> Okay, that I didn't see.
>> 
>> >
>> >For the nfp for instance we had "fw.app" for the datapath microcode and
>> >"fw.mgmt" for the control processor. These are separate partitions on
>> >the flash. I don't think we ever implemented writing them separately
>> >but it's certainly was our internal plan at some point.  
>> 
>> Okay, so what you say it, we already have components in "devlink dev
>> info". Like you pointed out as an example:
>>   fw.app
>>   fw.mgmt
>> so the flash comment would be:
>>   devlink dev flash pci/0000:01:00.0 component fw.app file foo.bin
>>   devlink dev flash pci/0000:01:00.0 component fw.mgmt file bar.bin
>> ?
>
>Correct.
>
>> If yes, what should be the default in case component is not defined? Do
>> we need to expose it in "devlink dev info"? How?
>
>Not defined as in someone tries to flash component X but there is no
>version for X in info?
>
>> So to extend this existing facility with my line card example, we would
>> have:
>> 
>> $ devlink dev info
>> pci/0000:01:00.0:
>>    driver mlxsw_spectrum2
>>    versions:
>>        fixed:
>>          hw.revision A0
>>          fw.psid MT_0000000199
>> 	 lc1.hw.revision 0
>> 	 lc1.fw.psid MT_0000000111
>> 	 lc2.hw.revision 0
>> 	 lc2.fw.psid MT_0000000111
>>        running:
>>          fw.version 29.2010.2302
>>          fw 29.2010.2302
>> 	 lc1.fw 19.2010.1310
>> 	 lc1.ini.version 4
>> 	 lc2.fw 19.2010.1310
>> 	 lc2.ini.version 4
>> 
>> And then:
>> devlink dev flash pci/0000:01:00.0 component lc1.fw file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
>> 
>> Does this sound correct?
>
>I think I suggested something like that in the past, but back then 

Yes, you did.


>I was assuming that lc FW would come from the same large FW bundle
>file as the control plan FW, and we would not have to use the component.
>
>Let's step back and look from the automation perspective again.
>Assuming we don't want to hardcode matching "lc$i" there how can 
>a generic FW update service scan the dev info and decide on what
>dev flash command to fire off?

Hardcode matching lc$i? I don't follow. It is a part of the
version/component name.
So if devlink dev info outputs:
lc2.fw 19.2010.1310
then you use for devlink dev flash:
devlink dev flash pci/0000:01:00.0 component lc2.fw file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
Same name, same string.

What am I missing?



>
>> Also, to avoid free-form, I can imagine to have per-linecard info_get() op
>> which would be called for each line card from devlink_nl_info_fill() and
>> prefix the "lcX" automatically without driver being involved.
>> 
>> Sounds good?
>
>Hm. That's moving the matryoshka-ing of the objects from the uAPI level
>to the internals. 
>
>If we don't do the string prefix but instead pass the subobject info to
>the user space as an attribute per version we can at least avoid
>per-subobject commands (DEVLINK_CMD_LINECARD_INFO_GET). Much closer to
>how health reporters are implemented than how params are done, so I
>think it is a good direction.

Sorry, I'm a bit lost. Could you please provide some example about how
you envision it? For me it is a guessing game :/
My guess is you would like to add to the version nest where
DEVLINK_ATTR_INFO_VERSION_NAME resides for example
DEVLINK_ATTR_LINECARD_INDEX?

Correct?


>
>We still need to iron out how the automation can go over the main FW
>and sub-objects in a generic way.
>
>I still think full devlink sub-instance is better because we will end
>up needing params or health. Fake devices can be made with auxbus or
>otherwise. But if you really don't want sub-instances we can explore 
>the above.

I really don't.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Sat, May 28, 2022 at 05:58:56PM CEST, dsahern@gmail.com wrote:
>On 5/24/22 8:31 AM, Jiri Pirko wrote:
>> 
>> $ devlink lc info pci/0000:01:00.0 lc 8
>> pci/0000:01:00.0:
>
>...
>
>> 
>> $ devlink lc flash pci/0000:01:00.0 lc 8 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
>
>
>A lot of your proposed syntax for devlink commands has 'lc' twice. If
>'lc' is the subcommand, then you are managing a linecard making 'lc'
>before the '8' redundant. How about 'slot 8' or something along those lines?

Well, there is 1:1 match between cmd line options and output, as always.

Object name is one thing, the option name is different. It is quite
common to name them both the same. I'm not sure I understand why it
would be an issue.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Sun, 29 May 2022 11:23:01 +0200 Jiri Pirko wrote:
> >Let's step back and look from the automation perspective again.
> >Assuming we don't want to hardcode matching "lc$i" there how can 
> >a generic FW update service scan the dev info and decide on what
> >dev flash command to fire off?  
> 
> Hardcode matching lc$i? I don't follow. It is a part of the
> version/component name.
> So if devlink dev info outputs:
> lc2.fw 19.2010.1310
> then you use for devlink dev flash:
> devlink dev flash pci/0000:01:00.0 component lc2.fw file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
> Same name, same string.
> 
> What am I missing?

Nevermind, I think we can iterate over all the groupings.
Since I hope you agreed that component has an established
meaning can we use group instead?

> >> Also, to avoid free-form, I can imagine to have per-linecard info_get() op
> >> which would be called for each line card from devlink_nl_info_fill() and
> >> prefix the "lcX" automatically without driver being involved.
> >> 
> >> Sounds good?  
> >
> >Hm. That's moving the matryoshka-ing of the objects from the uAPI level
> >to the internals. 
> >
> >If we don't do the string prefix but instead pass the subobject info to
> >the user space as an attribute per version we can at least avoid
> >per-subobject commands (DEVLINK_CMD_LINECARD_INFO_GET). Much closer to
> >how health reporters are implemented than how params are done, so I
> >think it is a good direction.  
> 
> Sorry, I'm a bit lost. Could you please provide some example about how
> you envision it? For me it is a guessing game :/
> My guess is you would like to add to the version nest where
> DEVLINK_ATTR_INFO_VERSION_NAME resides for example
> DEVLINK_ATTR_LINECARD_INDEX?
> 
> Correct?

Yup.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[David Ahern]

On 5/29/22 3:24 AM, Jiri Pirko wrote:
> Sat, May 28, 2022 at 05:58:56PM CEST, dsahern@gmail.com wrote:
>> On 5/24/22 8:31 AM, Jiri Pirko wrote:
>>>
>>> $ devlink lc info pci/0000:01:00.0 lc 8
>>> pci/0000:01:00.0:
>>
>> ...
>>
>>>
>>> $ devlink lc flash pci/0000:01:00.0 lc 8 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
>>
>>
>> A lot of your proposed syntax for devlink commands has 'lc' twice. If
>> 'lc' is the subcommand, then you are managing a linecard making 'lc'
>> before the '8' redundant. How about 'slot 8' or something along those lines?
> 
> Well, there is 1:1 match between cmd line options and output, as always.
> 
> Object name is one thing, the option name is different. It is quite
> common to name them both the same. I'm not sure I understand why it
> would be an issue.
> 

example? To me it says something is off in your model when you want to
use the same keyword twice in a command line.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, May 31, 2022 at 04:11:16AM CEST, dsahern@gmail.com wrote:
>On 5/29/22 3:24 AM, Jiri Pirko wrote:
>> Sat, May 28, 2022 at 05:58:56PM CEST, dsahern@gmail.com wrote:
>>> On 5/24/22 8:31 AM, Jiri Pirko wrote:
>>>>
>>>> $ devlink lc info pci/0000:01:00.0 lc 8
>>>> pci/0000:01:00.0:
>>>
>>> ...
>>>
>>>>
>>>> $ devlink lc flash pci/0000:01:00.0 lc 8 file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
>>>
>>>
>>> A lot of your proposed syntax for devlink commands has 'lc' twice. If
>>> 'lc' is the subcommand, then you are managing a linecard making 'lc'
>>> before the '8' redundant. How about 'slot 8' or something along those lines?
>> 
>> Well, there is 1:1 match between cmd line options and output, as always.
>> 
>> Object name is one thing, the option name is different. It is quite
>> common to name them both the same. I'm not sure I understand why it
>> would be an issue.
>> 
>
>example? To me it says something is off in your model when you want to
>use the same keyword twice in a command line.

man devlink-trap
man devlink-sb

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Mon, May 30, 2022 at 09:54:08PM CEST, kuba@kernel.org wrote:
>On Sun, 29 May 2022 11:23:01 +0200 Jiri Pirko wrote:
>> >Let's step back and look from the automation perspective again.
>> >Assuming we don't want to hardcode matching "lc$i" there how can 
>> >a generic FW update service scan the dev info and decide on what
>> >dev flash command to fire off?  
>> 
>> Hardcode matching lc$i? I don't follow. It is a part of the
>> version/component name.
>> So if devlink dev info outputs:
>> lc2.fw 19.2010.1310
>> then you use for devlink dev flash:
>> devlink dev flash pci/0000:01:00.0 component lc2.fw file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
>> Same name, same string.
>> 
>> What am I missing?
>
>Nevermind, I think we can iterate over all the groupings.
>Since I hope you agreed that component has an established

Yeah, component=version. I will send a RFC soon that tights it together.

>meaning can we use group instead?

Group of what? Could you provide me example what you mean?


>
>> >> Also, to avoid free-form, I can imagine to have per-linecard info_get() op
>> >> which would be called for each line card from devlink_nl_info_fill() and
>> >> prefix the "lcX" automatically without driver being involved.
>> >> 
>> >> Sounds good?  
>> >
>> >Hm. That's moving the matryoshka-ing of the objects from the uAPI level
>> >to the internals. 
>> >
>> >If we don't do the string prefix but instead pass the subobject info to
>> >the user space as an attribute per version we can at least avoid
>> >per-subobject commands (DEVLINK_CMD_LINECARD_INFO_GET). Much closer to
>> >how health reporters are implemented than how params are done, so I
>> >think it is a good direction.  
>> 
>> Sorry, I'm a bit lost. Could you please provide some example about how
>> you envision it? For me it is a guessing game :/
>> My guess is you would like to add to the version nest where
>> DEVLINK_ATTR_INFO_VERSION_NAME resides for example
>> DEVLINK_ATTR_LINECARD_INDEX?
>> 
>> Correct?
>
>Yup.

Hmm, in that case, I'm not sure how to do this. As cmd options and       
outputs should match, we would have:                                     
                                                                         
devlink dev info                                                         
lc2.fw 19.2010.1310                                                      
                                                                         
here lc2 and fw are concatenated from DEVLINK_ATTR_LINECARD_INDEX and DEVLINK_ATTR_INFO_VERSION_NAME
                                                                         
Now on devlink dev flash side, when I pass "component lc2.fw", how could 
the "devlink dev flash" know to divide it to DEVLINK_ATTR_LINECARD_INDEX 
and FLASH_COMPONENT? Should I parse the cmd line option and figure the
"lcX." prefix into an attribute?
                                                                         
Or, we would have to have something like:                                    
devlink dev flash pci/0000:01:00.0 lc 2 component fw file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
                                                                         
But to be consistent with the output, we would have to change "devlink   
dev info" to something like:                                             
pci/0000:01:00.0:                                                        
  versions:                                                              
      running:                                                           
        fw 1.2.3                                                         
        fw.mgmt 10.20.30                                                 
        lc 2 fw 19.2010.1310                                             
                                                                         
But that would break the existing JSON output, because "running" is an array:
                "running": {                                             
                    "fw": "1.2.3",                                       
                    "fw.mgmt": "10.20.30"                                
                },                                                       

So probably better to stick to "lcx.y" notation in both devlink dev info
and flash and split/squash to attributes internally. What do you think?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Tue, 31 May 2022 09:11:27 +0200 Jiri Pirko wrote:
> >Nevermind, I think we can iterate over all the groupings.
> >Since I hope you agreed that component has an established  
> 
> Yeah, component=version. I will send a RFC soon that tights it together.
> 
> >meaning can we use group instead?  
> 
> Group of what? Could you provide me example what you mean?

Group of components. As explained component has an existing meaning,
we can't reuse the term with a different one now.

> >> Sorry, I'm a bit lost. Could you please provide some example about how
> >> you envision it? For me it is a guessing game :/
> >> My guess is you would like to add to the version nest where
> >> DEVLINK_ATTR_INFO_VERSION_NAME resides for example
> >> DEVLINK_ATTR_LINECARD_INDEX?
> >> 
> >> Correct?  
> >
> >Yup.  
> 
> Hmm, in that case, I'm not sure how to do this. As cmd options and       
> outputs should match, we would have:                                     
>                                                           
> devlink dev info                                                         
> lc2.fw 19.2010.1310                                                      
>                                                                          
> here lc2 and fw are concatenated from DEVLINK_ATTR_LINECARD_INDEX and DEVLINK_ATTR_INFO_VERSION_NAME

lc2 is the group name.
                                                     
> Now on devlink dev flash side, when I pass "component lc2.fw", how could 
> the "devlink dev flash" know to divide it to DEVLINK_ATTR_LINECARD_INDEX 
> and FLASH_COMPONENT? Should I parse the cmd line option and figure the
> "lcX." prefix into an attribute?
>                                                        
> Or, we would have to have something like:                                    
> devlink dev flash pci/0000:01:00.0 lc 2 component fw file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2

Yup, it'll make DaveA happy as well.

> But to be consistent with the output, we would have to change "devlink   
> dev info" to something like:                                             
> pci/0000:01:00.0:                                                        
>   versions:                                                              
>       running:                                                           
>         fw 1.2.3                                                         
>         fw.mgmt 10.20.30                                                 
>         lc 2 fw 19.2010.1310                                             

Yup.
                                                            
> But that would break the existing JSON output, because "running" is an array:
>                 "running": {                                             
>                     "fw": "1.2.3",                                       
>                     "fw.mgmt": "10.20.30"                                
>                 },                                                       

No, the lc versions should be in separate nests. Since they are not
updated when flashing main FW mixing them into existing versions would
break uAPI.

> So probably better to stick to "lcx.y" notation in both devlink dev info
> and flash and split/squash to attributes internally. What do you think?

BTW how do you intend to activate the new FW? Extend the reload command?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, May 31, 2022 at 05:05:55PM CEST, kuba@kernel.org wrote:
>On Tue, 31 May 2022 09:11:27 +0200 Jiri Pirko wrote:
>> >Nevermind, I think we can iterate over all the groupings.
>> >Since I hope you agreed that component has an established  
>> 
>> Yeah, component=version. I will send a RFC soon that tights it together.
>> 
>> >meaning can we use group instead?  
>> 
>> Group of what? Could you provide me example what you mean?
>
>Group of components. As explained component has an existing meaning,
>we can't reuse the term with a different one now.

I still don't follow. I don't want to reuse it.
Really, could you be more specific and show examples, please?


>
>> >> Sorry, I'm a bit lost. Could you please provide some example about how
>> >> you envision it? For me it is a guessing game :/
>> >> My guess is you would like to add to the version nest where
>> >> DEVLINK_ATTR_INFO_VERSION_NAME resides for example
>> >> DEVLINK_ATTR_LINECARD_INDEX?
>> >> 
>> >> Correct?  
>> >
>> >Yup.  
>> 
>> Hmm, in that case, I'm not sure how to do this. As cmd options and       
>> outputs should match, we would have:                                     
>>                                                           
>> devlink dev info                                                         
>> lc2.fw 19.2010.1310                                                      
>>                                                                          
>> here lc2 and fw are concatenated from DEVLINK_ATTR_LINECARD_INDEX and DEVLINK_ATTR_INFO_VERSION_NAME
>
>lc2 is the group name.
>                                                     
>> Now on devlink dev flash side, when I pass "component lc2.fw", how could 
>> the "devlink dev flash" know to divide it to DEVLINK_ATTR_LINECARD_INDEX 
>> and FLASH_COMPONENT? Should I parse the cmd line option and figure the
>> "lcX." prefix into an attribute?
>>                                                        
>> Or, we would have to have something like:                                    
>> devlink dev flash pci/0000:01:00.0 lc 2 component fw file mellanox/fw-AGB-rel-19_2010_1312-022-EVB.mfa2
>
>Yup, it'll make DaveA happy as well.
>
>> But to be consistent with the output, we would have to change "devlink   
>> dev info" to something like:                                             
>> pci/0000:01:00.0:                                                        
>>   versions:                                                              
>>       running:                                                           
>>         fw 1.2.3                                                         
>>         fw.mgmt 10.20.30                                                 
>>         lc 2 fw 19.2010.1310                                             
>
>Yup.

Set, you say "yup" but below you say it should be in a separate nest.
That is confusing me.


>                                                            
>> But that would break the existing JSON output, because "running" is an array:
>>                 "running": {                                             
>>                     "fw": "1.2.3",                                       
>>                     "fw.mgmt": "10.20.30"                                
>>                 },                                                       
>
>No, the lc versions should be in separate nests. Since they are not
>updated when flashing main FW mixing them into existing versions would
>break uAPI.

Could you please draw it out for me exacly as you envision it? We are
dancing around it, I can't really understand what exactly do you mean.


>
>> So probably better to stick to "lcx.y" notation in both devlink dev info
>> and flash and split/squash to attributes internally. What do you think?
>
>BTW how do you intend to activate the new FW? Extend the reload command?

Not sure now. Extending reload is an option. Have to think about it.

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Tue, 31 May 2022 17:51:36 +0200 Jiri Pirko wrote:
> Tue, May 31, 2022 at 05:05:55PM CEST, kuba@kernel.org wrote:
> >> Group of what? Could you provide me example what you mean?  
> >
> >Group of components. As explained component has an existing meaning,
> >we can't reuse the term with a different one now.  
> 
> I still don't follow. I don't want to reuse it.
> Really, could you be more specific and show examples, please?

What you had in your previous examples, just don't call it components
but come up with a new term:

$ devlink dev info
pci/0000:01:00.0:
  driver mlxsw_spectrum2
  versions:
      fixed:
        hw.revision A0
        fw.psid MT_0000000199
      running:
        fw.version 29.2010.2302
        fw 29.2010.2302
  groups? sections? subordinates? :                         <= here
    lc1:
      versions:
        fixed:
          hw.revision 0
          fw.psid MT_0000000111
        running:
          fw 19.2010.1310
          ini.version 4

Note that lc1 is not a nest at netlink level but user space can group
the attrs pretty trivially.

> >> But to be consistent with the output, we would have to change "devlink   
> >> dev info" to something like:                                             
> >> pci/0000:01:00.0:                                                        
> >>   versions:                                                              
> >>       running:                                                           
> >>         fw 1.2.3                                                         
> >>         fw.mgmt 10.20.30                                                 
> >>         lc 2 fw 19.2010.1310                                               
> >
> >Yup.  
> 
> Set, you say "yup" but below you say it should be in a separate nest.
> That is confusing me.

Ah, sorry. I hope the above is clear.
                                                  
> >> But that would break the existing JSON output, because "running" is an array:
> >>                 "running": {                                             
> >>                     "fw": "1.2.3",                                       
> >>                     "fw.mgmt": "10.20.30"                                
> >>                 },                                                         
> >
> >No, the lc versions should be in separate nests. Since they are not
> >updated when flashing main FW mixing them into existing versions would
> >break uAPI.  
> 
> Could you please draw it out for me exacly as you envision it? We are
> dancing around it, I can't really understand what exactly do you mean.

Why would I prototype your feature for you? I prefer a separate dl
instance. If you want to explore other options the "drawing out" is
on you :/

> >> So probably better to stick to "lcx.y" notation in both devlink dev info
> >> and flash and split/squash to attributes internally. What do you think?  
> >
> >BTW how do you intend to activate the new FW? Extend the reload command?  
> 
> Not sure now. Extending reload is an option. Have to think about it.

😑

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Tue, May 31, 2022 at 06:08:52PM CEST, kuba@kernel.org wrote:
>On Tue, 31 May 2022 17:51:36 +0200 Jiri Pirko wrote:
>> Tue, May 31, 2022 at 05:05:55PM CEST, kuba@kernel.org wrote:
>> >> Group of what? Could you provide me example what you mean?  
>> >
>> >Group of components. As explained component has an existing meaning,
>> >we can't reuse the term with a different one now.  
>> 
>> I still don't follow. I don't want to reuse it.
>> Really, could you be more specific and show examples, please?
>
>What you had in your previous examples, just don't call it components
>but come up with a new term:
>
>$ devlink dev info
>pci/0000:01:00.0:
>  driver mlxsw_spectrum2
>  versions:
>      fixed:
>        hw.revision A0
>        fw.psid MT_0000000199
>      running:
>        fw.version 29.2010.2302
>        fw 29.2010.2302
>  groups? sections? subordinates? :                         <= here
>    lc1:
>      versions:
>        fixed:
>          hw.revision 0
>          fw.psid MT_0000000111
>        running:
>          fw 19.2010.1310
>          ini.version 4
>
>Note that lc1 is not a nest at netlink level but user space can group
>the attrs pretty trivially.

Awesome! I think now it is clearer. To be in sync with devlink dev
flash cmd option, we have to have "lc 1" here, have to think how that
can be done.


>
>> >> But to be consistent with the output, we would have to change "devlink   
>> >> dev info" to something like:                                             
>> >> pci/0000:01:00.0:                                                        
>> >>   versions:                                                              
>> >>       running:                                                           
>> >>         fw 1.2.3                                                         
>> >>         fw.mgmt 10.20.30                                                 
>> >>         lc 2 fw 19.2010.1310                                               
>> >
>> >Yup.  
>> 
>> Set, you say "yup" but below you say it should be in a separate nest.
>> That is confusing me.
>
>Ah, sorry. I hope the above is clear.
>                                                  
>> >> But that would break the existing JSON output, because "running" is an array:
>> >>                 "running": {                                             
>> >>                     "fw": "1.2.3",                                       
>> >>                     "fw.mgmt": "10.20.30"                                
>> >>                 },                                                         
>> >
>> >No, the lc versions should be in separate nests. Since they are not
>> >updated when flashing main FW mixing them into existing versions would
>> >break uAPI.  
>> 
>> Could you please draw it out for me exacly as you envision it? We are
>> dancing around it, I can't really understand what exactly do you mean.
>
>Why would I prototype your feature for you? I prefer a separate dl
>instance. If you want to explore other options the "drawing out" is
>on you :/

Well, you are basically leading my arm when I'm drawing the thing. Looks
like you exactly know what you are looking for. That is why.
If you let me to the stuff my way, we would be already done.
You have to decide which way you want this.

And again, for the record, I strongly believe that a separate dl
instance for this does not make any sense at all :/ I wonder why you
still think it does.


>
>> >> So probably better to stick to "lcx.y" notation in both devlink dev info
>> >> and flash and split/squash to attributes internally. What do you think?  
>> >
>> >BTW how do you intend to activate the new FW? Extend the reload command?  
>> 
>> Not sure now. Extending reload is an option. Have to think about it.
>
>😑

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jakub Kicinski]

On Tue, 31 May 2022 21:34:42 +0200 Jiri Pirko wrote:
> And again, for the record, I strongly believe that a separate dl
> instance for this does not make any sense at all :/ I wonder why you
> still think it does.

For purely software reuse reasons. I think the line cards will require
a lot of the same attributes as the full devlink instance, so making
them a subobject which can have all the same attributes is poor SW arch.
Think about it from OOP perspective, you'd definitely factor all that
stuff out to an abstract class. We can't do that in netlink but whatever
just make it a full dl instance and describe the link between the two.

Most NIC vendors (everyone excluding Netronome?) decided that devlink
instance is equivalent to a bus device which IIUC it was not supposed
to be. It was supposed to be the whole ASIC. If we're okay to stretch
the definition of a dl instance to be "any independently controllable
unit of HW" for NICs then IDK why we can't make a line card a dl
instance.

Are you afraid of hiding dependencies?

Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info

[Jiri Pirko]

Wed, Jun 01, 2022 at 12:41:59AM CEST, kuba@kernel.org wrote:
>On Tue, 31 May 2022 21:34:42 +0200 Jiri Pirko wrote:
>> And again, for the record, I strongly believe that a separate dl
>> instance for this does not make any sense at all :/ I wonder why you
>> still think it does.
>
>For purely software reuse reasons. I think the line cards will require
>a lot of the same attributes as the full devlink instance, so making
>them a subobject which can have all the same attributes is poor SW arch.

Sure, I understand the motivation.


>Think about it from OOP perspective, you'd definitely factor all that
>stuff out to an abstract class. We can't do that in netlink but whatever
>just make it a full dl instance and describe the link between the two.
>
>Most NIC vendors (everyone excluding Netronome?) decided that devlink
>instance is equivalent to a bus device which IIUC it was not supposed
>to be. It was supposed to be the whole ASIC. If we're okay to stretch

I agree, that is incorrect. That is why I was thinking about sort of
"alias" to make it right (2 PF devlink instances would be one connected
by alias). Not implemented yet though :/


>the definition of a dl instance to be "any independently controllable
>unit of HW" for NICs then IDK why we can't make a line card a dl
>instance.

Well, it is not independently controllable. Well, truth is, that in our
current implementation, there is one independent "configuration", and
that is flash burn of the gearbox. It is done using a "tunnelling"
register which encapsulates register communication what is done during
flash burning.


>
>Are you afraid of hiding dependencies?

Not really, I'm just not sure I see it is worth the excercise.

In czech, we have this saying: "kanon na vrabce". I think that the
following picture is better than any translation :)
https://i.iinfo.cz/images/72/shutterstock-com-kanon-delo-ptak-vrabec-strilet-1.jpg

Will think about it some more.