[PATCH net-next v2 0/7] mv88e6xxx: Add per port devlink regions

[PATCH net-next v2 0/7] mv88e6xxx: Add per port devlink regions

[Andrew Lunn]

This patchset extends devlink regions to support per port regions, and
them makes use of them to support the ports of the mv88e6xxx switches.

root@rap:~# devlink region show
mdio_bus/gpio-0:00/global1: size 64 snapshot []
mdio_bus/gpio-0:00/global2: size 64 snapshot []
mdio_bus/gpio-0:00/atu: size 49152 snapshot []
mdio_bus/gpio-0:00/0/port: size 64 snapshot []
mdio_bus/gpio-0:00/1/port: size 64 snapshot []
mdio_bus/gpio-0:00/2/port: size 64 snapshot []
mdio_bus/gpio-0:00/3/port: size 64 snapshot []
mdio_bus/gpio-0:00/4/port: size 64 snapshot []
mdio_bus/gpio-0:00/5/port: size 64 snapshot []
mdio_bus/gpio-0:00/6/port: size 64 snapshot []
mdio_bus/gpio-0:00/7/port: size 64 snapshot []
mdio_bus/gpio-0:00/8/port: size 64 snapshot []
mdio_bus/gpio-0:00/9/port: size 64 snapshot []
mdio_bus/gpio-0:00/10/port: size 64 snapshot []

root@rap:~# devlink region new mdio_bus/gpio-0:00/1/port snapshot 42
root@rap:~# devlink region dump mdio_bus/gpio-0:00/1/port snapshot 42
0000000000000000 4f 1e 3e 20 00 01 01 39 3f 05 00 00 fd 07 00 00
0000000000000010 80 00 01 00 00 00 00 00 00 00 00 00 00 00 00 91
0000000000000020 00 00 00 00 00 00 00 00 00 00 00 00 22 00 00 00
0000000000000030 07 3e 00 00 00 00 00 80 00 00 00 00 00 00 5b 00

In order to support all ports of the switch, a new devlink flavour has
been added for unused ports:

# devlink port show
mdio_bus/gpio-0:00/0: type notset flavour unused splittable false
mdio_bus/gpio-0:00/1: type notset flavour cpu port 1 splittable false
mdio_bus/gpio-0:00/2: type eth netdev red flavour physical port 2 splittable fae
mdio_bus/gpio-0:00/3: type eth netdev blue flavour physical port 3 splittable fe
mdio_bus/gpio-0:00/4: type eth netdev green flavour physical port 4 splittable e
mdio_bus/gpio-0:00/5: type notset flavour unused splittable false
mdio_bus/gpio-0:00/6: type notset flavour unused splittable false
mdio_bus/gpio-0:00/7: type notset flavour unused splittable false
mdio_bus/gpio-0:00/8: type eth netdev waic0 flavour physical port 8 splittable e
mdio_bus/gpio-0:00/9: type notset flavour unused splittable false
mdio_bus/gpio-0:00/10: type notset flavour unused splittable false

The DSA core now creates the devlink port instances earlier, so that
the driver setup function can make use of them.

Andrew Lunn (7):
  net: devlink: Add unused port flavour
  net: dsa: Make use of devlink port flavour unused
  net: dsa: Register devlink ports before calling DSA driver setup()
  net: devlink: Add support for port regions
  net: dsa: Add devlink port regions support to DSA
  net: dsa: Add helper for converting devlink port to ds and port
  net: dsa: mv88e6xxx: Add per port devlink regions

 drivers/net/dsa/mv88e6xxx/devlink.c | 109 +++++++++++-
 include/net/devlink.h               |  27 +++
 include/net/dsa.h                   |  20 +++
 include/uapi/linux/devlink.h        |   3 +
 net/core/devlink.c                  | 254 +++++++++++++++++++++++++---
 net/dsa/dsa.c                       |  14 ++
 net/dsa/dsa2.c                      | 123 +++++++++-----
 7 files changed, 478 insertions(+), 72 deletions(-)

-- 
2.28.0

[PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Andrew Lunn]

Not all ports of a switch need to be used, particularly in embedded
systems. Add a port flavour for ports which physically exist in the
switch, but are not connected to the front panel etc, and so are
unused.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/uapi/linux/devlink.h | 3 +++
 net/core/devlink.c           | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index a2ecc8b00611..e1f209feac74 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -195,6 +195,9 @@ enum devlink_port_flavour {
 				      * port that faces the PCI VF.
 				      */
 	DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */
+	DEVLINK_PORT_FLAVOUR_UNUSED, /* Port which exists in the switch, but
+				      *	is not used in any way.
+				      */
 };
 
 enum devlink_param_cmode {
diff --git a/net/core/devlink.c b/net/core/devlink.c
index ac32b672a04b..fc9589eb4115 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -7569,7 +7569,8 @@ static bool devlink_port_type_should_warn(struct devlink_port *devlink_port)
 {
 	/* Ignore CPU and DSA flavours. */
 	return devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_CPU &&
-	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA;
+	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA &&
+	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_UNUSED;
 }
 
 #define DEVLINK_PORT_TYPE_WARN_TIMEOUT (HZ * 3600)
@@ -7854,6 +7855,7 @@ static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
 		break;
 	case DEVLINK_PORT_FLAVOUR_CPU:
 	case DEVLINK_PORT_FLAVOUR_DSA:
+	case DEVLINK_PORT_FLAVOUR_UNUSED:
 		/* As CPU and DSA ports do not have a netdevice associated
 		 * case should not ever happen.
 		 */
-- 
2.28.0

[PATCH net-next v2 2/7] net: dsa: Make use of devlink port flavour unused

[Andrew Lunn]

If a port is unused, still create a devlink port for it, but set the
flavour to unused. This allows us to attach devlink regions to the
port, etc.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/dsa2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 3cf67f5fe54a..2c149fb36928 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -272,6 +272,15 @@ static int dsa_port_setup(struct dsa_port *dp)
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
+		memset(dlp, 0, sizeof(*dlp));
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;
+		devlink_port_attrs_set(dlp, &attrs);
+		err = devlink_port_register(dl, dlp, dp->index);
+		if (err)
+			break;
+
+		devlink_port_registered = true;
+
 		dsa_port_disable(dp);
 		break;
 	case DSA_PORT_TYPE_CPU:
@@ -355,6 +364,7 @@ static void dsa_port_teardown(struct dsa_port *dp)
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
+		devlink_port_unregister(dlp);
 		break;
 	case DSA_PORT_TYPE_CPU:
 		dsa_port_disable(dp);
-- 
2.28.0

[PATCH net-next v2 3/7] net: dsa: Register devlink ports before calling DSA driver setup()

[Andrew Lunn]

DSA drivers want to create regions on devlink ports as well as the
devlink device instance, in order to export registers and other tables
per port. To keep all this code together in the drivers, have the
devlink ports registered early, so the setup() method can setup both
device and port devlink regions.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/dsa.h |   1 +
 net/dsa/dsa2.c    | 133 ++++++++++++++++++++++++++++------------------
 2 files changed, 83 insertions(+), 51 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index d16057c5987a..9aa44dc8ecdb 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -208,6 +208,7 @@ struct dsa_port {
 	u8			stp_state;
 	struct net_device	*bridge_dev;
 	struct devlink_port	devlink_port;
+	bool			devlink_port_setup;
 	struct phylink		*pl;
 	struct phylink_config	pl_config;
 
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 2c149fb36928..90cc70bd7c22 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -251,47 +251,19 @@ static void dsa_tree_teardown_default_cpu(struct dsa_switch_tree *dst)
 
 static int dsa_port_setup(struct dsa_port *dp)
 {
-	struct dsa_switch *ds = dp->ds;
-	struct dsa_switch_tree *dst = ds->dst;
-	const unsigned char *id = (const unsigned char *)&dst->index;
-	const unsigned char len = sizeof(dst->index);
 	struct devlink_port *dlp = &dp->devlink_port;
 	bool dsa_port_link_registered = false;
-	bool devlink_port_registered = false;
-	struct devlink_port_attrs attrs = {};
-	struct devlink *dl = ds->devlink;
 	bool dsa_port_enabled = false;
 	int err = 0;
 
-	attrs.phys.port_number = dp->index;
-	memcpy(attrs.switch_id.id, id, len);
-	attrs.switch_id.id_len = len;
-
 	if (dp->setup)
 		return 0;
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
-		memset(dlp, 0, sizeof(*dlp));
-		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;
-		devlink_port_attrs_set(dlp, &attrs);
-		err = devlink_port_register(dl, dlp, dp->index);
-		if (err)
-			break;
-
-		devlink_port_registered = true;
-
 		dsa_port_disable(dp);
 		break;
 	case DSA_PORT_TYPE_CPU:
-		memset(dlp, 0, sizeof(*dlp));
-		attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU;
-		devlink_port_attrs_set(dlp, &attrs);
-		err = devlink_port_register(dl, dlp, dp->index);
-		if (err)
-			break;
-		devlink_port_registered = true;
-
 		err = dsa_port_link_register_of(dp);
 		if (err)
 			break;
@@ -304,14 +276,6 @@ static int dsa_port_setup(struct dsa_port *dp)
 
 		break;
 	case DSA_PORT_TYPE_DSA:
-		memset(dlp, 0, sizeof(*dlp));
-		attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA;
-		devlink_port_attrs_set(dlp, &attrs);
-		err = devlink_port_register(dl, dlp, dp->index);
-		if (err)
-			break;
-		devlink_port_registered = true;
-
 		err = dsa_port_link_register_of(dp);
 		if (err)
 			break;
@@ -324,14 +288,6 @@ static int dsa_port_setup(struct dsa_port *dp)
 
 		break;
 	case DSA_PORT_TYPE_USER:
-		memset(dlp, 0, sizeof(*dlp));
-		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
-		devlink_port_attrs_set(dlp, &attrs);
-		err = devlink_port_register(dl, dlp, dp->index);
-		if (err)
-			break;
-		devlink_port_registered = true;
-
 		dp->mac = of_get_mac_address(dp->dn);
 		err = dsa_slave_create(dp);
 		if (err)
@@ -345,8 +301,6 @@ static int dsa_port_setup(struct dsa_port *dp)
 		dsa_port_disable(dp);
 	if (err && dsa_port_link_registered)
 		dsa_port_link_unregister_of(dp);
-	if (err && devlink_port_registered)
-		devlink_port_unregister(dlp);
 	if (err)
 		return err;
 
@@ -355,30 +309,77 @@ static int dsa_port_setup(struct dsa_port *dp)
 	return 0;
 }
 
-static void dsa_port_teardown(struct dsa_port *dp)
+static int dsa_port_devlink_setup(struct dsa_port *dp)
 {
 	struct devlink_port *dlp = &dp->devlink_port;
+	struct dsa_switch_tree *dst = dp->ds->dst;
+	struct devlink_port_attrs attrs = {};
+	struct devlink *dl = dp->ds->devlink;
+	const unsigned char *id;
+	unsigned char len;
+	int err;
+
+	id = (const unsigned char *)&dst->index;
+	len = sizeof(dst->index);
+
+	attrs.phys.port_number = dp->index;
+	memcpy(attrs.switch_id.id, id, len);
+	attrs.switch_id.id_len = len;
+
+	if (dp->setup)
+		return 0;
 
+	switch (dp->type) {
+	case DSA_PORT_TYPE_UNUSED:
+		memset(dlp, 0, sizeof(*dlp));
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;
+		devlink_port_attrs_set(dlp, &attrs);
+		err = devlink_port_register(dl, dlp, dp->index);
+		break;
+	case DSA_PORT_TYPE_CPU:
+		memset(dlp, 0, sizeof(*dlp));
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU;
+		devlink_port_attrs_set(dlp, &attrs);
+		err = devlink_port_register(dl, dlp, dp->index);
+		break;
+	case DSA_PORT_TYPE_DSA:
+		memset(dlp, 0, sizeof(*dlp));
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA;
+		devlink_port_attrs_set(dlp, &attrs);
+		err = devlink_port_register(dl, dlp, dp->index);
+		break;
+	case DSA_PORT_TYPE_USER:
+		memset(dlp, 0, sizeof(*dlp));
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
+		devlink_port_attrs_set(dlp, &attrs);
+		err = devlink_port_register(dl, dlp, dp->index);
+		break;
+	}
+
+	if (!err)
+		dp->devlink_port_setup = true;
+
+	return err;
+}
+
+static void dsa_port_teardown(struct dsa_port *dp)
+{
 	if (!dp->setup)
 		return;
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
-		devlink_port_unregister(dlp);
 		break;
 	case DSA_PORT_TYPE_CPU:
 		dsa_port_disable(dp);
 		dsa_tag_driver_put(dp->tag_ops);
-		devlink_port_unregister(dlp);
 		dsa_port_link_unregister_of(dp);
 		break;
 	case DSA_PORT_TYPE_DSA:
 		dsa_port_disable(dp);
-		devlink_port_unregister(dlp);
 		dsa_port_link_unregister_of(dp);
 		break;
 	case DSA_PORT_TYPE_USER:
-		devlink_port_unregister(dlp);
 		if (dp->slave) {
 			dsa_slave_destroy(dp->slave);
 			dp->slave = NULL;
@@ -389,6 +390,15 @@ static void dsa_port_teardown(struct dsa_port *dp)
 	dp->setup = false;
 }
 
+static void dsa_port_devlink_teardown(struct dsa_port *dp)
+{
+	struct devlink_port *dlp = &dp->devlink_port;
+
+	if (dp->devlink_port_setup)
+		devlink_port_unregister(dlp);
+	dp->devlink_port_setup = false;
+}
+
 static int dsa_devlink_info_get(struct devlink *dl,
 				struct devlink_info_req *req,
 				struct netlink_ext_ack *extack)
@@ -408,6 +418,7 @@ static const struct devlink_ops dsa_devlink_ops = {
 static int dsa_switch_setup(struct dsa_switch *ds)
 {
 	struct dsa_devlink_priv *dl_priv;
+	struct dsa_port *dp;
 	int err;
 
 	if (ds->setup)
@@ -433,6 +444,17 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 	if (err)
 		goto free_devlink;
 
+	/* Setup devlink port instances now, so that the switch
+	 * setup() can register regions etc, against the ports
+	 */
+	list_for_each_entry(dp, &ds->dst->ports, list) {
+		if (dp->ds == ds) {
+			err = dsa_port_devlink_setup(dp);
+			if (err)
+				goto unregister_devlink_ports;
+		}
+	}
+
 	err = dsa_switch_register_notifier(ds);
 	if (err)
 		goto unregister_devlink;
@@ -463,6 +485,10 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 
 unregister_notifier:
 	dsa_switch_unregister_notifier(ds);
+unregister_devlink_ports:
+	list_for_each_entry(dp, &ds->dst->ports, list)
+		if (dp->ds == ds)
+			dsa_port_devlink_teardown(dp);
 unregister_devlink:
 	devlink_unregister(ds->devlink);
 free_devlink:
@@ -474,6 +500,8 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 
 static void dsa_switch_teardown(struct dsa_switch *ds)
 {
+	struct dsa_port *dp;
+
 	if (!ds->setup)
 		return;
 
@@ -486,6 +514,9 @@ static void dsa_switch_teardown(struct dsa_switch *ds)
 		ds->ops->teardown(ds);
 
 	if (ds->devlink) {
+		list_for_each_entry(dp, &ds->dst->ports, list)
+			if (dp->ds == ds)
+				dsa_port_devlink_teardown(dp);
 		devlink_unregister(ds->devlink);
 		devlink_free(ds->devlink);
 		ds->devlink = NULL;
-- 
2.28.0

[PATCH net-next v2 4/7] net: devlink: Add support for port regions

[Andrew Lunn]

Allow regions to be registered to a devlink port. The same netlink API
is used, but the port index is provided to indicate when a region is a
port region as opposed to a device region.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/devlink.h |  27 +++++
 net/core/devlink.c    | 250 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 251 insertions(+), 26 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 4883dbae7faf..d9ce317ae060 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -110,6 +110,7 @@ struct devlink_port_attrs {
 struct devlink_port {
 	struct list_head list;
 	struct list_head param_list;
+	struct list_head region_list;
 	struct devlink *devlink;
 	unsigned int index;
 	bool registered;
@@ -573,6 +574,26 @@ struct devlink_region_ops {
 	void *priv;
 };
 
+/**
+ * struct devlink_port_region_ops - Region operations for a port
+ * @name: region name
+ * @destructor: callback used to free snapshot memory when deleting
+ * @snapshot: callback to request an immediate snapshot. On success,
+ *            the data variable must be updated to point to the snapshot data.
+ *            The function will be called while the devlink instance lock is
+ *            held.
+ * @priv: Pointer to driver private data for the region operation
+ */
+struct devlink_port_region_ops {
+	const char *name;
+	void (*destructor)(const void *data);
+	int (*snapshot)(struct devlink_port *port,
+			const struct devlink_port_region_ops *ops,
+			struct netlink_ext_ack *extack,
+			u8 **data);
+	void *priv;
+};
+
 struct devlink_fmsg;
 struct devlink_health_reporter;
 
@@ -1336,7 +1357,13 @@ struct devlink_region *
 devlink_region_create(struct devlink *devlink,
 		      const struct devlink_region_ops *ops,
 		      u32 region_max_snapshots, u64 region_size);
+struct devlink_region *
+devlink_port_region_create(struct devlink_port *port,
+			   const struct devlink_port_region_ops *ops,
+			   u32 region_max_snapshots, u64 region_size);
 void devlink_region_destroy(struct devlink_region *region);
+void devlink_port_region_destroy(struct devlink_region *region);
+
 int devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id);
 void devlink_region_snapshot_id_put(struct devlink *devlink, u32 id);
 int devlink_region_snapshot_create(struct devlink_region *region,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index fc9589eb4115..19d1e07cf9d3 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -347,8 +347,12 @@ devlink_sb_tc_index_get_from_info(struct devlink_sb *devlink_sb,
 
 struct devlink_region {
 	struct devlink *devlink;
+	struct devlink_port *port;
 	struct list_head list;
-	const struct devlink_region_ops *ops;
+	union {
+		const struct devlink_region_ops *ops;
+		const struct devlink_port_region_ops *port_ops;
+	};
 	struct list_head snapshot_list;
 	u32 max_snapshots;
 	u32 cur_snapshots;
@@ -374,6 +378,19 @@ devlink_region_get_by_name(struct devlink *devlink, const char *region_name)
 	return NULL;
 }
 
+static struct devlink_region *
+devlink_port_region_get_by_name(struct devlink_port *port,
+				const char *region_name)
+{
+	struct devlink_region *region;
+
+	list_for_each_entry(region, &port->region_list, list)
+		if (!strcmp(region->ops->name, region_name))
+			return region;
+
+	return NULL;
+}
+
 static struct devlink_snapshot *
 devlink_region_snapshot_get_by_id(struct devlink_region *region, u32 id)
 {
@@ -3903,6 +3920,11 @@ static int devlink_nl_region_fill(struct sk_buff *msg, struct devlink *devlink,
 	if (err)
 		goto nla_put_failure;
 
+	if (region->port)
+		if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX,
+				region->port->index))
+			goto nla_put_failure;
+
 	err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->ops->name);
 	if (err)
 		goto nla_put_failure;
@@ -3950,6 +3972,11 @@ devlink_nl_region_notify_build(struct devlink_region *region,
 	if (err)
 		goto out_cancel_msg;
 
+	if (region->port)
+		if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX,
+				region->port->index))
+			goto out_cancel_msg;
+
 	err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME,
 			     region->ops->name);
 	if (err)
@@ -4196,16 +4223,30 @@ static int devlink_nl_cmd_region_get_doit(struct sk_buff *skb,
 					  struct genl_info *info)
 {
 	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_port *port = NULL;
 	struct devlink_region *region;
 	const char *region_name;
 	struct sk_buff *msg;
+	unsigned int index;
 	int err;
 
 	if (!info->attrs[DEVLINK_ATTR_REGION_NAME])
 		return -EINVAL;
 
+	if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
+		index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+
+		port = devlink_port_get_by_index(devlink, index);
+		if (!port)
+			return -ENODEV;
+	}
+
 	region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
-	region = devlink_region_get_by_name(devlink, region_name);
+	if (port)
+		region = devlink_port_region_get_by_name(port, region_name);
+	else
+		region = devlink_region_get_by_name(devlink, region_name);
+
 	if (!region)
 		return -EINVAL;
 
@@ -4224,10 +4265,75 @@ static int devlink_nl_cmd_region_get_doit(struct sk_buff *skb,
 	return genlmsg_reply(msg, info);
 }
 
+static int devlink_nl_cmd_region_get_port_dumpit(struct sk_buff *msg,
+						 struct netlink_callback *cb,
+						 struct devlink_port *port,
+						 int *idx,
+						 int start)
+{
+	struct devlink_region *region;
+	int err = 0;
+
+	list_for_each_entry(region, &port->region_list, list) {
+		if (*idx < start) {
+			(*idx)++;
+			continue;
+		}
+		err = devlink_nl_region_fill(msg, port->devlink,
+					     DEVLINK_CMD_REGION_GET,
+					     NETLINK_CB(cb->skb).portid,
+					     cb->nlh->nlmsg_seq,
+					     NLM_F_MULTI, region);
+		if (err)
+			goto out;
+		(*idx)++;
+	}
+
+out:
+	return err;
+}
+
+static int devlink_nl_cmd_region_get_devlink_dumpit(struct sk_buff *msg,
+						    struct netlink_callback *cb,
+						    struct devlink *devlink,
+						    int *idx,
+						    int start)
+{
+	struct devlink_region *region;
+	struct devlink_port *port;
+	int err = 0;
+
+	mutex_lock(&devlink->lock);
+	list_for_each_entry(region, &devlink->region_list, list) {
+		if (*idx < start) {
+			(*idx)++;
+			continue;
+		}
+		err = devlink_nl_region_fill(msg, devlink,
+					     DEVLINK_CMD_REGION_GET,
+					     NETLINK_CB(cb->skb).portid,
+					     cb->nlh->nlmsg_seq,
+					     NLM_F_MULTI, region);
+		if (err)
+			goto out;
+		(*idx)++;
+	}
+
+	list_for_each_entry(port, &devlink->port_list, list) {
+		err = devlink_nl_cmd_region_get_port_dumpit(msg, cb, port, idx,
+							    start);
+		if (err)
+			goto out;
+	}
+
+out:
+	mutex_unlock(&devlink->lock);
+	return err;
+}
+
 static int devlink_nl_cmd_region_get_dumpit(struct sk_buff *msg,
 					    struct netlink_callback *cb)
 {
-	struct devlink_region *region;
 	struct devlink *devlink;
 	int start = cb->args[0];
 	int idx = 0;
@@ -4237,25 +4343,10 @@ static int devlink_nl_cmd_region_get_dumpit(struct sk_buff *msg,
 	list_for_each_entry(devlink, &devlink_list, list) {
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			continue;
-
-		mutex_lock(&devlink->lock);
-		list_for_each_entry(region, &devlink->region_list, list) {
-			if (idx < start) {
-				idx++;
-				continue;
-			}
-			err = devlink_nl_region_fill(msg, devlink,
-						     DEVLINK_CMD_REGION_GET,
-						     NETLINK_CB(cb->skb).portid,
-						     cb->nlh->nlmsg_seq,
-						     NLM_F_MULTI, region);
-			if (err) {
-				mutex_unlock(&devlink->lock);
-				goto out;
-			}
-			idx++;
-		}
-		mutex_unlock(&devlink->lock);
+		err = devlink_nl_cmd_region_get_devlink_dumpit(msg, cb, devlink,
+							       &idx, start);
+		if (err)
+			goto out;
 	}
 out:
 	mutex_unlock(&devlink_mutex);
@@ -4268,8 +4359,10 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_snapshot *snapshot;
+	struct devlink_port *port = NULL;
 	struct devlink_region *region;
 	const char *region_name;
+	unsigned int index;
 	u32 snapshot_id;
 
 	if (!info->attrs[DEVLINK_ATTR_REGION_NAME] ||
@@ -4279,7 +4372,19 @@ static int devlink_nl_cmd_region_del(struct sk_buff *skb,
 	region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
 	snapshot_id = nla_get_u32(info->attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
 
-	region = devlink_region_get_by_name(devlink, region_name);
+	if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
+		index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+
+		port = devlink_port_get_by_index(devlink, index);
+		if (!port)
+			return -ENODEV;
+	}
+
+	if (port)
+		region = devlink_port_region_get_by_name(port, region_name);
+	else
+		region = devlink_region_get_by_name(devlink, region_name);
+
 	if (!region)
 		return -EINVAL;
 
@@ -4296,9 +4401,11 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_snapshot *snapshot;
+	struct devlink_port *port = NULL;
 	struct nlattr *snapshot_id_attr;
 	struct devlink_region *region;
 	const char *region_name;
+	unsigned int index;
 	u32 snapshot_id;
 	u8 *data;
 	int err;
@@ -4309,7 +4416,20 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
-	region = devlink_region_get_by_name(devlink, region_name);
+
+	if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
+		index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+
+		port = devlink_port_get_by_index(devlink, index);
+		if (!port)
+			return -ENODEV;
+	}
+
+	if (port)
+		region = devlink_port_region_get_by_name(port, region_name);
+	else
+		region = devlink_region_get_by_name(devlink, region_name);
+
 	if (!region) {
 		NL_SET_ERR_MSG_MOD(info->extack, "The requested region does not exist");
 		return -EINVAL;
@@ -4345,7 +4465,12 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 		}
 	}
 
-	err = region->ops->snapshot(devlink, region->ops, info->extack, &data);
+	if (port)
+		err = region->port_ops->snapshot(port, region->port_ops,
+						 info->extack, &data);
+	else
+		err = region->ops->snapshot(devlink, region->ops,
+					    info->extack, &data);
 	if (err)
 		goto err_snapshot_capture;
 
@@ -4467,10 +4592,12 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
 	u64 ret_offset, start_offset, end_offset = U64_MAX;
 	struct nlattr **attrs = info->attrs;
+	struct devlink_port *port = NULL;
 	struct devlink_region *region;
 	struct nlattr *chunks_attr;
 	const char *region_name;
 	struct devlink *devlink;
+	unsigned int index;
 	void *hdr;
 	int err;
 
@@ -4491,8 +4618,21 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		goto out_unlock;
 	}
 
+	if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
+		index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+
+		port = devlink_port_get_by_index(devlink, index);
+		if (!port)
+			return -ENODEV;
+	}
+
 	region_name = nla_data(attrs[DEVLINK_ATTR_REGION_NAME]);
-	region = devlink_region_get_by_name(devlink, region_name);
+
+	if (port)
+		region = devlink_port_region_get_by_name(port, region_name);
+	else
+		region = devlink_region_get_by_name(devlink, region_name);
+
 	if (!region) {
 		err = -EINVAL;
 		goto out_unlock;
@@ -4529,6 +4669,11 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	if (err)
 		goto nla_put_failure;
 
+	if (region->port)
+		if (nla_put_u32(skb, DEVLINK_ATTR_PORT_INDEX,
+				region->port->index))
+			goto nla_put_failure;
+
 	err = nla_put_string(skb, DEVLINK_ATTR_REGION_NAME, region_name);
 	if (err)
 		goto nla_put_failure;
@@ -7623,6 +7768,7 @@ int devlink_port_register(struct devlink *devlink,
 	mutex_init(&devlink_port->reporters_lock);
 	list_add_tail(&devlink_port->list, &devlink->port_list);
 	INIT_LIST_HEAD(&devlink_port->param_list);
+	INIT_LIST_HEAD(&devlink_port->region_list);
 	mutex_unlock(&devlink->lock);
 	INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
 	devlink_port_type_warn_schedule(devlink_port);
@@ -7646,6 +7792,7 @@ void devlink_port_unregister(struct devlink_port *devlink_port)
 	list_del(&devlink_port->list);
 	mutex_unlock(&devlink->lock);
 	WARN_ON(!list_empty(&devlink_port->reporter_list));
+	WARN_ON(!list_empty(&devlink_port->region_list));
 	mutex_destroy(&devlink_port->reporters_lock);
 }
 EXPORT_SYMBOL_GPL(devlink_port_unregister);
@@ -8725,6 +8872,57 @@ devlink_region_create(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devlink_region_create);
 
+/**
+ *	devlink_port_region_create - create a new address region for a port
+ *
+ *	@port: devlink port
+ *	@ops: region operations and name
+ *	@region_max_snapshots: Maximum supported number of snapshots for region
+ *	@region_size: size of region
+ */
+struct devlink_region *
+devlink_port_region_create(struct devlink_port *port,
+			   const struct devlink_port_region_ops *ops,
+			   u32 region_max_snapshots, u64 region_size)
+{
+	struct devlink *devlink = port->devlink;
+	struct devlink_region *region;
+	int err = 0;
+
+	if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&devlink->lock);
+
+	if (devlink_port_region_get_by_name(port, ops->name)) {
+		err = -EEXIST;
+		goto unlock;
+	}
+
+	region = kzalloc(sizeof(*region), GFP_KERNEL);
+	if (!region) {
+		err = -ENOMEM;
+		goto unlock;
+	}
+
+	region->devlink = devlink;
+	region->port = port;
+	region->max_snapshots = region_max_snapshots;
+	region->port_ops = ops;
+	region->size = region_size;
+	INIT_LIST_HEAD(&region->snapshot_list);
+	list_add_tail(&region->list, &port->region_list);
+	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
+
+	mutex_unlock(&devlink->lock);
+	return region;
+
+unlock:
+	mutex_unlock(&devlink->lock);
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(devlink_port_region_create);
+
 /**
  *	devlink_region_destroy - destroy address region
  *
-- 
2.28.0

[PATCH net-next v2 5/7] net: dsa: Add devlink port regions support to DSA

[Andrew Lunn]

Allow DSA drivers to make use of devlink port regions, via simple
wrappers.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/dsa.h |  5 +++++
 net/dsa/dsa.c     | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 9aa44dc8ecdb..f0bb64e5002f 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -666,6 +666,11 @@ struct devlink_region *
 dsa_devlink_region_create(struct dsa_switch *ds,
 			  const struct devlink_region_ops *ops,
 			  u32 region_max_snapshots, u64 region_size);
+struct devlink_region *
+dsa_devlink_port_region_create(struct dsa_switch *ds,
+			       int port,
+			       const struct devlink_port_region_ops *ops,
+			       u32 region_max_snapshots, u64 region_size);
 void dsa_devlink_region_destroy(struct devlink_region *region);
 
 struct dsa_port *dsa_port_from_netdev(struct net_device *netdev);
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 5c18c0214aac..97fcabdeccec 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -414,6 +414,20 @@ dsa_devlink_region_create(struct dsa_switch *ds,
 }
 EXPORT_SYMBOL_GPL(dsa_devlink_region_create);
 
+struct devlink_region *
+dsa_devlink_port_region_create(struct dsa_switch *ds,
+			       int port,
+			       const struct devlink_port_region_ops *ops,
+			       u32 region_max_snapshots, u64 region_size)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+
+	return devlink_port_region_create(&dp->devlink_port, ops,
+					  region_max_snapshots,
+					  region_size);
+}
+EXPORT_SYMBOL_GPL(dsa_devlink_port_region_create);
+
 void dsa_devlink_region_destroy(struct devlink_region *region)
 {
 	devlink_region_destroy(region);
-- 
2.28.0

[PATCH net-next v2 6/7] net: dsa: Add helper for converting devlink port to ds and port

[Andrew Lunn]

Hide away from DSA drivers how devlink works.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/dsa.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index f0bb64e5002f..24c925c192ec 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -686,6 +686,20 @@ static inline struct dsa_switch *dsa_devlink_to_ds(struct devlink *dl)
 	return dl_priv->ds;
 }
 
+static inline
+struct dsa_switch *dsa_devlink_port_to_ds(struct devlink_port *port)
+{
+	struct devlink *dl = port->devlink;
+	struct dsa_devlink_priv *dl_priv = devlink_priv(dl);
+
+	return dl_priv->ds;
+}
+
+static inline int dsa_devlink_port_to_port(struct devlink_port *port)
+{
+	return port->index;
+}
+
 struct dsa_switch_driver {
 	struct list_head	list;
 	const struct dsa_switch_ops *ops;
-- 
2.28.0

[PATCH net-next v2 7/7] net: dsa: mv88e6xxx: Add per port devlink regions

[Andrew Lunn]

Add a devlink region to return the per port registers.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/devlink.c | 109 +++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/devlink.c b/drivers/net/dsa/mv88e6xxx/devlink.c
index 81e1560db206..ed74b075de84 100644
--- a/drivers/net/dsa/mv88e6xxx/devlink.c
+++ b/drivers/net/dsa/mv88e6xxx/devlink.c
@@ -415,6 +415,36 @@ static int mv88e6xxx_region_atu_snapshot(struct devlink *dl,
 	return err;
 }
 
+static int mv88e6xxx_region_port_snapshot(struct devlink_port *devlink_port,
+					  const struct devlink_port_region_ops *ops,
+					  struct netlink_ext_ack *extack,
+					  u8 **data)
+{
+	struct dsa_switch *ds = dsa_devlink_port_to_ds(devlink_port);
+	int port = dsa_devlink_port_to_port(devlink_port);
+	struct mv88e6xxx_chip *chip = ds->priv;
+	u16 *registers;
+	int i, err;
+
+	registers = kmalloc_array(32, sizeof(u16), GFP_KERNEL);
+	if (!registers)
+		return -ENOMEM;
+
+	mv88e6xxx_reg_lock(chip);
+	for (i = 0; i < 32; i++) {
+		err = mv88e6xxx_port_read(chip, port, i, &registers[i]);
+		if (err) {
+			kfree(registers);
+			goto out;
+		}
+	}
+	*data = (u8 *)registers;
+out:
+	mv88e6xxx_reg_unlock(chip);
+
+	return err;
+}
+
 static struct mv88e6xxx_region_priv mv88e6xxx_region_global1_priv = {
 	.id = MV88E6XXX_REGION_GLOBAL1,
 };
@@ -443,6 +473,12 @@ static struct devlink_region_ops mv88e6xxx_region_atu_ops = {
 	.destructor = kfree,
 };
 
+static const struct devlink_port_region_ops mv88e6xxx_region_port_ops = {
+	.name = "port",
+	.snapshot = mv88e6xxx_region_port_snapshot,
+	.destructor = kfree,
+};
+
 struct mv88e6xxx_region {
 	struct devlink_region_ops *ops;
 	u64 size;
@@ -471,11 +507,59 @@ mv88e6xxx_teardown_devlink_regions_global(struct mv88e6xxx_chip *chip)
 		dsa_devlink_region_destroy(chip->regions[i]);
 }
 
-void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds)
+static void
+mv88e6xxx_teardown_devlink_regions_port(struct mv88e6xxx_chip *chip,
+					int port)
 {
-	struct mv88e6xxx_chip *chip = ds->priv;
+	dsa_devlink_region_destroy(chip->ports[port].region);
+}
 
-	mv88e6xxx_teardown_devlink_regions_global(chip);
+static int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds,
+						struct mv88e6xxx_chip *chip,
+						int port)
+{
+	struct devlink_region *region;
+
+	region = dsa_devlink_port_region_create(ds,
+						port,
+						&mv88e6xxx_region_port_ops, 1,
+						32 * sizeof(u16));
+	if (IS_ERR(region))
+		return PTR_ERR(region);
+
+	chip->ports[port].region = region;
+
+	return 0;
+}
+
+static void
+mv88e6xxx_teardown_devlink_regions_ports(struct mv88e6xxx_chip *chip)
+{
+	int port;
+
+	for (port = 0; port < mv88e6xxx_num_ports(chip); port++)
+		mv88e6xxx_teardown_devlink_regions_port(chip, port);
+}
+
+static int mv88e6xxx_setup_devlink_regions_ports(struct dsa_switch *ds,
+						 struct mv88e6xxx_chip *chip)
+{
+	int port, port_err;
+	int err;
+
+	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
+		err = mv88e6xxx_setup_devlink_regions_port(ds, chip, port);
+		if (err)
+			goto out;
+	}
+
+	return 0;
+
+out:
+	for (port_err = 0; port_err < port; port_err++)
+		mv88e6xxx_teardown_devlink_regions_port(chip, port_err);
+
+	return err;
 }
 
 static int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds,
@@ -511,8 +595,25 @@ static int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds,
 int mv88e6xxx_setup_devlink_regions(struct dsa_switch *ds)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
+	int err;
+
+	err = mv88e6xxx_setup_devlink_regions_global(ds, chip);
+	if (err)
+		return err;
+
+	err = mv88e6xxx_setup_devlink_regions_ports(ds, chip);
+	if (err)
+		mv88e6xxx_teardown_devlink_regions_global(chip);
 
-	return mv88e6xxx_setup_devlink_regions_global(ds, chip);
+	return err;
+}
+
+void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	mv88e6xxx_teardown_devlink_regions_ports(chip);
+	mv88e6xxx_teardown_devlink_regions_global(chip);
 }
 
 int mv88e6xxx_devlink_info_get(struct dsa_switch *ds,
-- 
2.28.0

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Vladimir Oltean]

On Sat, Sep 26, 2020 at 11:06:26PM +0200, Andrew Lunn wrote:
> Not all ports of a switch need to be used, particularly in embedded
> systems. Add a port flavour for ports which physically exist in the
> switch, but are not connected to the front panel etc, and so are
> unused.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  include/uapi/linux/devlink.h | 3 +++
>  net/core/devlink.c           | 4 +++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index a2ecc8b00611..e1f209feac74 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -195,6 +195,9 @@ enum devlink_port_flavour {
>  				      * port that faces the PCI VF.
>  				      */
>  	DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */
> +	DEVLINK_PORT_FLAVOUR_UNUSED, /* Port which exists in the switch, but
> +				      *	is not used in any way.

Nitpicking: there is an extraneous tab character here between "*" and "is".

> +				      */
>  };
>  
>  enum devlink_param_cmode {
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index ac32b672a04b..fc9589eb4115 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -7569,7 +7569,8 @@ static bool devlink_port_type_should_warn(struct devlink_port *devlink_port)
>  {
>  	/* Ignore CPU and DSA flavours. */
>  	return devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_CPU &&
> -	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA;
> +	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA &&
> +	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_UNUSED;
>  }
>  
>  #define DEVLINK_PORT_TYPE_WARN_TIMEOUT (HZ * 3600)
> @@ -7854,6 +7855,7 @@ static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
>  		break;
>  	case DEVLINK_PORT_FLAVOUR_CPU:
>  	case DEVLINK_PORT_FLAVOUR_DSA:
> +	case DEVLINK_PORT_FLAVOUR_UNUSED:
>  		/* As CPU and DSA ports do not have a netdevice associated
>  		 * case should not ever happen.
>  		 */
> -- 
> 2.28.0
> 

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Vladimir Oltean]

On Sat, Sep 26, 2020 at 11:06:26PM +0200, Andrew Lunn wrote:
> Not all ports of a switch need to be used, particularly in embedded
> systems. Add a port flavour for ports which physically exist in the
> switch, but are not connected to the front panel etc, and so are
> unused.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---

You also have an iproute2 patch prepared, right?

$ devlink port
spi/spi0.1/0: type notset flavour <unknown flavour>  <- this should read "unused"
spi/spi0.1/1: type eth netdev swp2 flavour physical port 1
spi/spi0.1/2: type eth netdev swp3 flavour physical port 2
spi/spi0.1/3: type eth netdev swp4 flavour physical port 3
spi/spi0.1/4: type notset flavour cpu port 4

Re: [PATCH net-next v2 0/7] mv88e6xxx: Add per port devlink regions

[Marek Behun]

Andrew, can this be used to write the registers from userspace, or only
to read it?

Marek

Re: [PATCH net-next v2 0/7] mv88e6xxx: Add per port devlink regions

[Vladimir Oltean]

Hi Marek,

On Sun, Sep 27, 2020 at 12:02:19AM +0200, Marek Behun wrote:
> Andrew, can this be used to write the registers from userspace, or only
> to read it?

It is intended for reading only.
https://www.kernel.org/doc/html/latest/networking/devlink/devlink-region.html

Thanks,
-Vladimir

Re: [PATCH net-next v2 2/7] net: dsa: Make use of devlink port flavour unused

[Vladimir Oltean]

On Sat, Sep 26, 2020 at 11:06:27PM +0200, Andrew Lunn wrote:
> If a port is unused, still create a devlink port for it, but set the
> flavour to unused. This allows us to attach devlink regions to the
> port, etc.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Tested-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v2 3/7] net: dsa: Register devlink ports before calling DSA driver setup()

[Vladimir Oltean]

On Sat, Sep 26, 2020 at 11:06:28PM +0200, Andrew Lunn wrote:
> DSA drivers want to create regions on devlink ports as well as the
> devlink device instance, in order to export registers and other tables
> per port. To keep all this code together in the drivers, have the
> devlink ports registered early, so the setup() method can setup both
> device and port devlink regions.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  include/net/dsa.h |   1 +
>  net/dsa/dsa2.c    | 133 ++++++++++++++++++++++++++++------------------
>  2 files changed, 83 insertions(+), 51 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index d16057c5987a..9aa44dc8ecdb 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -208,6 +208,7 @@ struct dsa_port {
>  	u8			stp_state;
>  	struct net_device	*bridge_dev;
>  	struct devlink_port	devlink_port;
> +	bool			devlink_port_setup;
>  	struct phylink		*pl;
>  	struct phylink_config	pl_config;
>  
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 2c149fb36928..90cc70bd7c22 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -251,47 +251,19 @@ static void dsa_tree_teardown_default_cpu(struct dsa_switch_tree *dst)
>  
>  static int dsa_port_setup(struct dsa_port *dp)
>  {
> -	struct dsa_switch *ds = dp->ds;
> -	struct dsa_switch_tree *dst = ds->dst;
> -	const unsigned char *id = (const unsigned char *)&dst->index;
> -	const unsigned char len = sizeof(dst->index);
>  	struct devlink_port *dlp = &dp->devlink_port;
>  	bool dsa_port_link_registered = false;
> -	bool devlink_port_registered = false;
> -	struct devlink_port_attrs attrs = {};
> -	struct devlink *dl = ds->devlink;
>  	bool dsa_port_enabled = false;
>  	int err = 0;
>  
> -	attrs.phys.port_number = dp->index;
> -	memcpy(attrs.switch_id.id, id, len);
> -	attrs.switch_id.id_len = len;
> -
>  	if (dp->setup)
>  		return 0;
>  
>  	switch (dp->type) {
>  	case DSA_PORT_TYPE_UNUSED:
> -		memset(dlp, 0, sizeof(*dlp));
> -		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;
> -		devlink_port_attrs_set(dlp, &attrs);
> -		err = devlink_port_register(dl, dlp, dp->index);
> -		if (err)
> -			break;
> -
> -		devlink_port_registered = true;
> -
>  		dsa_port_disable(dp);
>  		break;
>  	case DSA_PORT_TYPE_CPU:
> -		memset(dlp, 0, sizeof(*dlp));
> -		attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU;
> -		devlink_port_attrs_set(dlp, &attrs);
> -		err = devlink_port_register(dl, dlp, dp->index);
> -		if (err)
> -			break;
> -		devlink_port_registered = true;
> -
>  		err = dsa_port_link_register_of(dp);
>  		if (err)
>  			break;
> @@ -304,14 +276,6 @@ static int dsa_port_setup(struct dsa_port *dp)
>  
>  		break;
>  	case DSA_PORT_TYPE_DSA:
> -		memset(dlp, 0, sizeof(*dlp));
> -		attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA;
> -		devlink_port_attrs_set(dlp, &attrs);
> -		err = devlink_port_register(dl, dlp, dp->index);
> -		if (err)
> -			break;
> -		devlink_port_registered = true;
> -
>  		err = dsa_port_link_register_of(dp);
>  		if (err)
>  			break;
> @@ -324,14 +288,6 @@ static int dsa_port_setup(struct dsa_port *dp)
>  
>  		break;
>  	case DSA_PORT_TYPE_USER:
> -		memset(dlp, 0, sizeof(*dlp));
> -		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
> -		devlink_port_attrs_set(dlp, &attrs);
> -		err = devlink_port_register(dl, dlp, dp->index);
> -		if (err)
> -			break;
> -		devlink_port_registered = true;
> -
>  		dp->mac = of_get_mac_address(dp->dn);
>  		err = dsa_slave_create(dp);
>  		if (err)
> @@ -345,8 +301,6 @@ static int dsa_port_setup(struct dsa_port *dp)
>  		dsa_port_disable(dp);
>  	if (err && dsa_port_link_registered)
>  		dsa_port_link_unregister_of(dp);
> -	if (err && devlink_port_registered)
> -		devlink_port_unregister(dlp);
>  	if (err)
>  		return err;
>  
> @@ -355,30 +309,77 @@ static int dsa_port_setup(struct dsa_port *dp)
>  	return 0;
>  }
>  
> -static void dsa_port_teardown(struct dsa_port *dp)
> +static int dsa_port_devlink_setup(struct dsa_port *dp)
>  {
>  	struct devlink_port *dlp = &dp->devlink_port;
> +	struct dsa_switch_tree *dst = dp->ds->dst;
> +	struct devlink_port_attrs attrs = {};
> +	struct devlink *dl = dp->ds->devlink;
> +	const unsigned char *id;
> +	unsigned char len;
> +	int err;
> +
> +	id = (const unsigned char *)&dst->index;
> +	len = sizeof(dst->index);
> +
> +	attrs.phys.port_number = dp->index;
> +	memcpy(attrs.switch_id.id, id, len);
> +	attrs.switch_id.id_len = len;
> +
> +	if (dp->setup)
> +		return 0;
>  

I wonder what this is protecting against? I ran on a multi-switch tree
without these 2 lines and I didn't get anything like multiple
registration or things like that. What is the call path that would call
dsa_port_devlink_setup twice?

> +	switch (dp->type) {
> +	case DSA_PORT_TYPE_UNUSED:
> +		memset(dlp, 0, sizeof(*dlp));
> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;

> +		devlink_port_attrs_set(dlp, &attrs);
> +		err = devlink_port_register(dl, dlp, dp->index);

These 2 lines are common everywhere. Could you move them out of the
switch-case statement?

> +		break;
> +	case DSA_PORT_TYPE_CPU:
> +		memset(dlp, 0, sizeof(*dlp));
> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU;
> +		devlink_port_attrs_set(dlp, &attrs);
> +		err = devlink_port_register(dl, dlp, dp->index);
> +		break;
> +	case DSA_PORT_TYPE_DSA:
> +		memset(dlp, 0, sizeof(*dlp));
> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA;
> +		devlink_port_attrs_set(dlp, &attrs);
> +		err = devlink_port_register(dl, dlp, dp->index);
> +		break;
> +	case DSA_PORT_TYPE_USER:
> +		memset(dlp, 0, sizeof(*dlp));
> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
> +		devlink_port_attrs_set(dlp, &attrs);
> +		err = devlink_port_register(dl, dlp, dp->index);
> +		break;
> +	}
> +
> +	if (!err)
> +		dp->devlink_port_setup = true;
> +
> +	return err;
> +}
> +
> +static void dsa_port_teardown(struct dsa_port *dp)
> +{
>  	if (!dp->setup)
>  		return;
>  
>  	switch (dp->type) {
>  	case DSA_PORT_TYPE_UNUSED:
> -		devlink_port_unregister(dlp);
>  		break;
>  	case DSA_PORT_TYPE_CPU:
>  		dsa_port_disable(dp);
>  		dsa_tag_driver_put(dp->tag_ops);
> -		devlink_port_unregister(dlp);
>  		dsa_port_link_unregister_of(dp);
>  		break;
>  	case DSA_PORT_TYPE_DSA:
>  		dsa_port_disable(dp);
> -		devlink_port_unregister(dlp);
>  		dsa_port_link_unregister_of(dp);
>  		break;
>  	case DSA_PORT_TYPE_USER:
> -		devlink_port_unregister(dlp);
>  		if (dp->slave) {
>  			dsa_slave_destroy(dp->slave);
>  			dp->slave = NULL;
> @@ -389,6 +390,15 @@ static void dsa_port_teardown(struct dsa_port *dp)
>  	dp->setup = false;
>  }
>  
> +static void dsa_port_devlink_teardown(struct dsa_port *dp)
> +{
> +	struct devlink_port *dlp = &dp->devlink_port;
> +
> +	if (dp->devlink_port_setup)
> +		devlink_port_unregister(dlp);
> +	dp->devlink_port_setup = false;
> +}
> +
>  static int dsa_devlink_info_get(struct devlink *dl,
>  				struct devlink_info_req *req,
>  				struct netlink_ext_ack *extack)
> @@ -408,6 +418,7 @@ static const struct devlink_ops dsa_devlink_ops = {
>  static int dsa_switch_setup(struct dsa_switch *ds)
>  {
>  	struct dsa_devlink_priv *dl_priv;
> +	struct dsa_port *dp;
>  	int err;
>  
>  	if (ds->setup)
> @@ -433,6 +444,17 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  	if (err)
>  		goto free_devlink;
>  
> +	/* Setup devlink port instances now, so that the switch
> +	 * setup() can register regions etc, against the ports
> +	 */
> +	list_for_each_entry(dp, &ds->dst->ports, list) {
> +		if (dp->ds == ds) {
> +			err = dsa_port_devlink_setup(dp);
> +			if (err)
> +				goto unregister_devlink_ports;
> +		}
> +	}
> +
>  	err = dsa_switch_register_notifier(ds);
>  	if (err)
>  		goto unregister_devlink;

Need to use goto unregister_devlink_ports here.

> @@ -463,6 +485,10 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  
>  unregister_notifier:
>  	dsa_switch_unregister_notifier(ds);
> +unregister_devlink_ports:
> +	list_for_each_entry(dp, &ds->dst->ports, list)
> +		if (dp->ds == ds)
> +			dsa_port_devlink_teardown(dp);
>  unregister_devlink:
>  	devlink_unregister(ds->devlink);
>  free_devlink:
> @@ -474,6 +500,8 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  
>  static void dsa_switch_teardown(struct dsa_switch *ds)
>  {
> +	struct dsa_port *dp;
> +
>  	if (!ds->setup)
>  		return;
>  
> @@ -486,6 +514,9 @@ static void dsa_switch_teardown(struct dsa_switch *ds)
>  		ds->ops->teardown(ds);
>  
>  	if (ds->devlink) {
> +		list_for_each_entry(dp, &ds->dst->ports, list)
> +			if (dp->ds == ds)
> +				dsa_port_devlink_teardown(dp);
>  		devlink_unregister(ds->devlink);
>  		devlink_free(ds->devlink);
>  		ds->devlink = NULL;
> -- 
> 2.28.0
> 

Re: [PATCH net-next v2 5/7] net: dsa: Add devlink port regions support to DSA

[Vladimir Oltean]

On Sat, Sep 26, 2020 at 11:06:30PM +0200, Andrew Lunn wrote:
> Allow DSA drivers to make use of devlink port regions, via simple
> wrappers.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Tested-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v2 7/7] net: dsa: mv88e6xxx: Add per port devlink regions

[Vladimir Oltean]

On Sat, Sep 26, 2020 at 11:06:32PM +0200, Andrew Lunn wrote:
> Add a devlink region to return the per port registers.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mv88e6xxx/devlink.c | 109 +++++++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/devlink.c b/drivers/net/dsa/mv88e6xxx/devlink.c
> index 81e1560db206..ed74b075de84 100644
> --- a/drivers/net/dsa/mv88e6xxx/devlink.c
> +++ b/drivers/net/dsa/mv88e6xxx/devlink.c
> @@ -415,6 +415,36 @@ static int mv88e6xxx_region_atu_snapshot(struct devlink *dl,
>  	return err;
>  }
>  
> +static int mv88e6xxx_region_port_snapshot(struct devlink_port *devlink_port,
> +					  const struct devlink_port_region_ops *ops,
> +					  struct netlink_ext_ack *extack,
> +					  u8 **data)
> +{
> +	struct dsa_switch *ds = dsa_devlink_port_to_ds(devlink_port);
> +	int port = dsa_devlink_port_to_port(devlink_port);
> +	struct mv88e6xxx_chip *chip = ds->priv;

> +	u16 *registers;

I was meaning to ask this since the global regions patchset, but I
forgot.

Do we not expect to see, under the same circumstances, the same region
snapshot on a big endian and on a little endian system?

> +	int i, err;
> +
> +	registers = kmalloc_array(32, sizeof(u16), GFP_KERNEL);
> +	if (!registers)
> +		return -ENOMEM;
> +
> +	mv88e6xxx_reg_lock(chip);
> +	for (i = 0; i < 32; i++) {
> +		err = mv88e6xxx_port_read(chip, port, i, &registers[i]);
> +		if (err) {
> +			kfree(registers);
> +			goto out;
> +		}
> +	}
> +	*data = (u8 *)registers;
> +out:
> +	mv88e6xxx_reg_unlock(chip);
> +
> +	return err;
> +}
> +
>  static struct mv88e6xxx_region_priv mv88e6xxx_region_global1_priv = {
>  	.id = MV88E6XXX_REGION_GLOBAL1,
>  };
> @@ -443,6 +473,12 @@ static struct devlink_region_ops mv88e6xxx_region_atu_ops = {
>  	.destructor = kfree,
>  };
>  
> +static const struct devlink_port_region_ops mv88e6xxx_region_port_ops = {
> +	.name = "port",
> +	.snapshot = mv88e6xxx_region_port_snapshot,
> +	.destructor = kfree,
> +};
> +
>  struct mv88e6xxx_region {
>  	struct devlink_region_ops *ops;
>  	u64 size;
> @@ -471,11 +507,59 @@ mv88e6xxx_teardown_devlink_regions_global(struct mv88e6xxx_chip *chip)
>  		dsa_devlink_region_destroy(chip->regions[i]);
>  }
>  
> -void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds)
> +static void
> +mv88e6xxx_teardown_devlink_regions_port(struct mv88e6xxx_chip *chip,
> +					int port)
>  {
> -	struct mv88e6xxx_chip *chip = ds->priv;
> +	dsa_devlink_region_destroy(chip->ports[port].region);
> +}
>  
> -	mv88e6xxx_teardown_devlink_regions_global(chip);
> +static int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds,
> +						struct mv88e6xxx_chip *chip,
> +						int port)
> +{
> +	struct devlink_region *region;
> +
> +	region = dsa_devlink_port_region_create(ds,
> +						port,
> +						&mv88e6xxx_region_port_ops, 1,
> +						32 * sizeof(u16));
> +	if (IS_ERR(region))
> +		return PTR_ERR(region);
> +
> +	chip->ports[port].region = region;
> +
> +	return 0;
> +}
> +
> +static void
> +mv88e6xxx_teardown_devlink_regions_ports(struct mv88e6xxx_chip *chip)
> +{
> +	int port;
> +
> +	for (port = 0; port < mv88e6xxx_num_ports(chip); port++)
> +		mv88e6xxx_teardown_devlink_regions_port(chip, port);
> +}
> +
> +static int mv88e6xxx_setup_devlink_regions_ports(struct dsa_switch *ds,
> +						 struct mv88e6xxx_chip *chip)
> +{
> +	int port, port_err;
> +	int err;
> +
> +	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
> +		err = mv88e6xxx_setup_devlink_regions_port(ds, chip, port);
> +		if (err)
> +			goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	for (port_err = 0; port_err < port; port_err++)

"while (port-- >= 0)" should do the trick. Also, maybe it would be
overall more aesthetic if you wouldn't use the goto and have 2 exit
points in such a small function, but a simple break here. Like this:

	int err;

	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
		err = mv88e6xxx_setup_devlink_regions_port(ds, chip, port);
		if (err) {
			while (port-- >= 0)
				mv88e6xxx_teardown_devlink_regions_port(chip,
									port);
			break;
		}
	}

	return err;

> +		mv88e6xxx_teardown_devlink_regions_port(chip, port_err);
> +
> +	return err;
>  }
>  
>  static int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds,
> @@ -511,8 +595,25 @@ static int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds,
>  int mv88e6xxx_setup_devlink_regions(struct dsa_switch *ds)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
> +	int err;
> +
> +	err = mv88e6xxx_setup_devlink_regions_global(ds, chip);
> +	if (err)
> +		return err;
> +
> +	err = mv88e6xxx_setup_devlink_regions_ports(ds, chip);
> +	if (err)
> +		mv88e6xxx_teardown_devlink_regions_global(chip);
>  
> -	return mv88e6xxx_setup_devlink_regions_global(ds, chip);
> +	return err;
> +}
> +
> +void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +
> +	mv88e6xxx_teardown_devlink_regions_ports(chip);
> +	mv88e6xxx_teardown_devlink_regions_global(chip);
>  }
>  
>  int mv88e6xxx_devlink_info_get(struct dsa_switch *ds,
> -- 
> 2.28.0
> 

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Andrew Lunn]

On Sun, Sep 27, 2020 at 01:00:34AM +0300, Vladimir Oltean wrote:
> On Sat, Sep 26, 2020 at 11:06:26PM +0200, Andrew Lunn wrote:
> > Not all ports of a switch need to be used, particularly in embedded
> > systems. Add a port flavour for ports which physically exist in the
> > switch, but are not connected to the front panel etc, and so are
> > unused.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> 
> You also have an iproute2 patch prepared, right?

https://github.com/lunn/iproute2 port-regions

	 Andrew
 

Re: [PATCH net-next v2 3/7] net: dsa: Register devlink ports before calling DSA driver setup()

[Andrew Lunn]

> > +static int dsa_port_devlink_setup(struct dsa_port *dp)
> >  {
> >  	struct devlink_port *dlp = &dp->devlink_port;
> > +	struct dsa_switch_tree *dst = dp->ds->dst;
> > +	struct devlink_port_attrs attrs = {};
> > +	struct devlink *dl = dp->ds->devlink;
> > +	const unsigned char *id;
> > +	unsigned char len;
> > +	int err;
> > +
> > +	id = (const unsigned char *)&dst->index;
> > +	len = sizeof(dst->index);
> > +
> > +	attrs.phys.port_number = dp->index;
> > +	memcpy(attrs.switch_id.id, id, len);
> > +	attrs.switch_id.id_len = len;
> > +
> > +	if (dp->setup)
> > +		return 0;
> >  
> 
> I wonder what this is protecting against? I ran on a multi-switch tree
> without these 2 lines and I didn't get anything like multiple
> registration or things like that. What is the call path that would call
> dsa_port_devlink_setup twice?

I made a duplicate copy of dsa_port_setup() and trimmed out what was
not needed to give the new dsa_port_setup() and
dsa_port_devlink_setup(). I did not trim enough...

> 
> > +	switch (dp->type) {
> > +	case DSA_PORT_TYPE_UNUSED:
> > +		memset(dlp, 0, sizeof(*dlp));
> > +		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;
> 
> > +		devlink_port_attrs_set(dlp, &attrs);
> > +		err = devlink_port_register(dl, dlp, dp->index);
> 
> These 2 lines are common everywhere. Could you move them out of the
> switch-case statement?

Yes, that makes sense. Too much blind copy/paste without actually
reviewing the code afterwards.

	  Andrew

Re: [PATCH net-next v2 7/7] net: dsa: mv88e6xxx: Add per port devlink regions

[Andrew Lunn]

> I was meaning to ask this since the global regions patchset, but I
> forgot.
> 
> Do we not expect to see, under the same circumstances, the same region
> snapshot on a big endian and on a little endian system?

We have never had any issues with endinness with MDIO. PHY/DSA drivers
work with host endian. The MDIO bus controller does what it needs to
do when shifting the bits out, as required by class 22 or 45.

netlink in general assume host endian, as far as i know. So a big
endian and a little endian snapshot are going to be different.

Arnd did an interesting presentation for LPC. He basically shows that
big endian is going away, with the exception of IBM big iron. I don't
expect an IBM Z to have a DSA switch!

       Andrew

Re: [PATCH net-next v2 7/7] net: dsa: mv88e6xxx: Add per port devlink regions

[Vladimir Oltean]

On Sun, Sep 27, 2020 at 03:03:00AM +0200, Andrew Lunn wrote:
> > I was meaning to ask this since the global regions patchset, but I
> > forgot.
> >
> > Do we not expect to see, under the same circumstances, the same region
> > snapshot on a big endian and on a little endian system?
>
> We have never had any issues with endinness with MDIO. PHY/DSA drivers
> work with host endian. The MDIO bus controller does what it needs to
> do when shifting the bits out, as required by class 22 or 45.
>
> netlink in general assume host endian, as far as i know. So a big
> endian and a little endian snapshot are going to be different.

If the binary form of the snapshot is supposed to be consumed and done
with immediately after the netlink communication is over, sure. But it
is presented to the user, and in fact this is even the way it is
presented by default, with devlink, there's no pretty-printing.

> Arnd did an interesting presentation for LPC. He basically shows that
> big endian is going away, with the exception of IBM big iron. I don't
> expect an IBM Z to have a DSA switch!

Tell that to my T1040 chip with an 8-port Seville switch driven by the
Ocelot driver.

Also, armv8 can also boot in big-endian mode.

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Jakub Kicinski]

On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
> Not all ports of a switch need to be used, particularly in embedded
> systems. Add a port flavour for ports which physically exist in the
> switch, but are not connected to the front panel etc, and so are
> unused.

This is missing the explanation of why reporting such ports makes sense.

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Vladimir Oltean]

On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:
> On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
> > Not all ports of a switch need to be used, particularly in embedded
> > systems. Add a port flavour for ports which physically exist in the
> > switch, but are not connected to the front panel etc, and so are
> > unused.
>
> This is missing the explanation of why reporting such ports makes sense.

Because this is a core devlink patch, we're talking really generalistic
here. And since devlink regions are a debugging features, it makes sense
to tell a debugging story. Let's say there is a switch which had
configured all its ports to be part of the flooding replication lists,
but also configured other things incorrectly such that attempting to
flood a frame to a disabled port would leak a memory buffer. After
enough time, the system would lock up. So unused ports are not absent
from the system and they might even make a difference if the procedure
to disable a port is buggy (and there would be no debugging without
bugs, right?). I see no reason why we would hide them. Devlink ports are
not net devices, I thought that was the whole point, to have insight
into the hardware and not have to deal with an approximate abstraction.

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Andrew Lunn]

On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:
> On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:
> > On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
> > > Not all ports of a switch need to be used, particularly in embedded
> > > systems. Add a port flavour for ports which physically exist in the
> > > switch, but are not connected to the front panel etc, and so are
> > > unused.
> >
> > This is missing the explanation of why reporting such ports makes sense.
> 
> Because this is a core devlink patch, we're talking really generalistic
> here.

Hi Vladimir

I don't think Jakub is questioning the why. He just wants it in the
commit message.

       Andrew

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Jakub Kicinski]

On Tue, 29 Sep 2020 00:07:30 +0200 Andrew Lunn wrote:
> On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:
> > On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:  
> > > On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:  
> > > > Not all ports of a switch need to be used, particularly in embedded
> > > > systems. Add a port flavour for ports which physically exist in the
> > > > switch, but are not connected to the front panel etc, and so are
> > > > unused.  
> > >
> > > This is missing the explanation of why reporting such ports makes sense.  
> > 
> > Because this is a core devlink patch, we're talking really generalistic
> > here.  
> 
> Hi Vladimir
> 
> I don't think Jakub is questioning the why. He just wants it in the
> commit message.

Ack, I think we need to clearly say when those should be exposed. 
Most ASICs will have disabled ports, and we don't expect NICs to
suddenly start reporting ports for all PCI PFs they may have.

Also I keep thinking that these ports and all their objects should 
be hidden under some switch from user space perspective because they 
are unlikely to be valuable to see for a normal user. Thoughts?

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Florian Fainelli]

On 9/28/2020 3:35 PM, Jakub Kicinski wrote:
> On Tue, 29 Sep 2020 00:07:30 +0200 Andrew Lunn wrote:
>> On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:
>>> On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:
>>>> On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
>>>>> Not all ports of a switch need to be used, particularly in embedded
>>>>> systems. Add a port flavour for ports which physically exist in the
>>>>> switch, but are not connected to the front panel etc, and so are
>>>>> unused.
>>>>
>>>> This is missing the explanation of why reporting such ports makes sense.
>>>
>>> Because this is a core devlink patch, we're talking really generalistic
>>> here.
>>
>> Hi Vladimir
>>
>> I don't think Jakub is questioning the why. He just wants it in the
>> commit message.
> 
> Ack, I think we need to clearly say when those should be exposed.
> Most ASICs will have disabled ports, and we don't expect NICs to
> suddenly start reporting ports for all PCI PFs they may have.
> 
> Also I keep thinking that these ports and all their objects should
> be hidden under some switch from user space perspective because they
> are unlikely to be valuable to see for a normal user. Thoughts?

Hidden in what sense? They are already hidden in that there is no 
net_device object being created for them. Are you asking for adding 
another option to say, devlink show like:

devlink show -a

which would also show the ports that are disabled during a dump?
-- 
Florian

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Jakub Kicinski]

On Mon, 28 Sep 2020 15:36:50 -0700 Florian Fainelli wrote:
> On 9/28/2020 3:35 PM, Jakub Kicinski wrote:
> > On Tue, 29 Sep 2020 00:07:30 +0200 Andrew Lunn wrote:  
> >> On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:  
> >>> On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:  
> >>>> On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:  
> >>>>> Not all ports of a switch need to be used, particularly in embedded
> >>>>> systems. Add a port flavour for ports which physically exist in the
> >>>>> switch, but are not connected to the front panel etc, and so are
> >>>>> unused.  
> >>>>
> >>>> This is missing the explanation of why reporting such ports makes sense.  
> >>>
> >>> Because this is a core devlink patch, we're talking really generalistic
> >>> here.  
> >>
> >> Hi Vladimir
> >>
> >> I don't think Jakub is questioning the why. He just wants it in the
> >> commit message.  
> > 
> > Ack, I think we need to clearly say when those should be exposed.
> > Most ASICs will have disabled ports, and we don't expect NICs to
> > suddenly start reporting ports for all PCI PFs they may have.
> > 
> > Also I keep thinking that these ports and all their objects should
> > be hidden under some switch from user space perspective because they
> > are unlikely to be valuable to see for a normal user. Thoughts?  
> 
> Hidden in what sense? They are already hidden in that there is no 
> net_device object being created for them. Are you asking for adding 
> another option to say, devlink show like:
> 
> devlink show -a
> 
> which would also show the ports that are disabled during a dump?

Yup, exactly. Looks like ip uses -a for something I don't quite
understand - but some switch along those lines. We already have 
-d for hiding less-relevant attributes.

Do you think this is an overkill? I don't feel strongly.

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Florian Fainelli]

On 9/28/2020 4:39 PM, Jakub Kicinski wrote:
> On Mon, 28 Sep 2020 15:36:50 -0700 Florian Fainelli wrote:
>> On 9/28/2020 3:35 PM, Jakub Kicinski wrote:
>>> On Tue, 29 Sep 2020 00:07:30 +0200 Andrew Lunn wrote:
>>>> On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:
>>>>> On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:
>>>>>> On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
>>>>>>> Not all ports of a switch need to be used, particularly in embedded
>>>>>>> systems. Add a port flavour for ports which physically exist in the
>>>>>>> switch, but are not connected to the front panel etc, and so are
>>>>>>> unused.
>>>>>>
>>>>>> This is missing the explanation of why reporting such ports makes sense.
>>>>>
>>>>> Because this is a core devlink patch, we're talking really generalistic
>>>>> here.
>>>>
>>>> Hi Vladimir
>>>>
>>>> I don't think Jakub is questioning the why. He just wants it in the
>>>> commit message.
>>>
>>> Ack, I think we need to clearly say when those should be exposed.
>>> Most ASICs will have disabled ports, and we don't expect NICs to
>>> suddenly start reporting ports for all PCI PFs they may have.
>>>
>>> Also I keep thinking that these ports and all their objects should
>>> be hidden under some switch from user space perspective because they
>>> are unlikely to be valuable to see for a normal user. Thoughts?
>>
>> Hidden in what sense? They are already hidden in that there is no
>> net_device object being created for them. Are you asking for adding
>> another option to say, devlink show like:
>>
>> devlink show -a
>>
>> which would also show the ports that are disabled during a dump?
> 
> Yup, exactly. Looks like ip uses -a for something I don't quite
> understand - but some switch along those lines. We already have
> -d for hiding less-relevant attributes.
> 
> Do you think this is an overkill? I don't feel strongly.

That makes sense to me as it would be confusing to suddenly show unused 
port flavors after this patch series land. Andrew, Vladimir, does that 
work for you as well?
-- 
Florian

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Vladimir Oltean]

On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
> That makes sense to me as it would be confusing to suddenly show unused port
> flavors after this patch series land. Andrew, Vladimir, does that work for
> you as well?

I have nothing to object against somebody adding a '--all' argument to
devlink port commands.

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Jiri Pirko]

Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
>On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
>> That makes sense to me as it would be confusing to suddenly show unused port
>> flavors after this patch series land. Andrew, Vladimir, does that work for
>> you as well?
>
>I have nothing to object against somebody adding a '--all' argument to
>devlink port commands.

How "unused" is a "flavour"? It seems to me more like a separate
attribute as port of any "flavour" may be potentially "unused". I don't
think we should mix these 2.

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Vladimir Oltean]

On Tue, Sep 29, 2020 at 03:07:58PM +0200, Jiri Pirko wrote:
> Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
> >On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
> >> That makes sense to me as it would be confusing to suddenly show unused port
> >> flavors after this patch series land. Andrew, Vladimir, does that work for
> >> you as well?
> >
> >I have nothing to object against somebody adding a '--all' argument to
> >devlink port commands.
> 
> How "unused" is a "flavour"? It seems to me more like a separate
> attribute as port of any "flavour" may be potentially "unused". I don't
> think we should mix these 2.
> 

I guess it's you who suggested it might make sense to add an "unused"
port flavour?

> Okay. That looks fine. I wonder if it would make sense to have another
> flavour for "unused" ports.

https://patchwork.ozlabs.org/project/netdev/cover/20180322105522.8186-1-jiri@resnulli.us/

Thanks,
-Vladimir

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Andrew Lunn]

On Tue, Sep 29, 2020 at 03:07:58PM +0200, Jiri Pirko wrote:
> Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
> >On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
> >> That makes sense to me as it would be confusing to suddenly show unused port
> >> flavors after this patch series land. Andrew, Vladimir, does that work for
> >> you as well?
> >
> >I have nothing to object against somebody adding a '--all' argument to
> >devlink port commands.
> 
> How "unused" is a "flavour"? It seems to me more like a separate
> attribute as port of any "flavour" may be potentially "unused". I don't
> think we should mix these 2.

Hi Jiri

Current flavours are:

enum devlink_port_flavour {
        DEVLINK_PORT_FLAVOUR_PHYSICAL, /* Any kind of a port physically
                                        * facing the user.
                                        */
        DEVLINK_PORT_FLAVOUR_CPU, /* CPU port */
        DEVLINK_PORT_FLAVOUR_DSA, /* Distributed switch architecture
                                   * interconnect port.
                                   */
        DEVLINK_PORT_FLAVOUR_PCI_PF, /* Represents eswitch port for
                                      * the PCI PF. It is an internal
                                      * port that faces the PCI PF.
                                      */
        DEVLINK_PORT_FLAVOUR_PCI_VF, /* Represents eswitch port
                                      * for the PCI VF. It is an internal
                                      * port that faces the PCI VF.
                                      */
        DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */
};

A port in the DSA world is generally just a port on the switch. It is
not limited in nature. It can be used as a PHYSICAL, or CPU, or a DSA
port. We don't consider them as unused PHYISCAL ports, or unused CPU
ports. They are just unused ports. Which is why i added an UNUSED
flavour.

Now, it could be this world view is actually just a DSA world
view. Maybe some ASICs have strict roles for their ports? They are not
configurable? And then i could see it being an attribute? But that
messes up the DSA world view :-(

      Andrew

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Jiri Pirko]

Tue, Sep 29, 2020 at 03:48:56PM CEST, vladimir.oltean@nxp.com wrote:
>On Tue, Sep 29, 2020 at 03:07:58PM +0200, Jiri Pirko wrote:
>> Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
>> >On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
>> >> That makes sense to me as it would be confusing to suddenly show unused port
>> >> flavors after this patch series land. Andrew, Vladimir, does that work for
>> >> you as well?
>> >
>> >I have nothing to object against somebody adding a '--all' argument to
>> >devlink port commands.
>> 
>> How "unused" is a "flavour"? It seems to me more like a separate
>> attribute as port of any "flavour" may be potentially "unused". I don't
>> think we should mix these 2.
>> 
>
>I guess it's you who suggested it might make sense to add an "unused"
>port flavour?

Maybe, but that was just an idea. 2 years later, I don't think it is a
good idea.

>
>> Okay. That looks fine. I wonder if it would make sense to have another
>> flavour for "unused" ports.
>
>https://patchwork.ozlabs.org/project/netdev/cover/20180322105522.8186-1-jiri@resnulli.us/
>
>Thanks,
>-Vladimir

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Jiri Pirko]

Tue, Sep 29, 2020 at 03:57:00PM CEST, andrew@lunn.ch wrote:
>On Tue, Sep 29, 2020 at 03:07:58PM +0200, Jiri Pirko wrote:
>> Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
>> >On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
>> >> That makes sense to me as it would be confusing to suddenly show unused port
>> >> flavors after this patch series land. Andrew, Vladimir, does that work for
>> >> you as well?
>> >
>> >I have nothing to object against somebody adding a '--all' argument to
>> >devlink port commands.
>> 
>> How "unused" is a "flavour"? It seems to me more like a separate
>> attribute as port of any "flavour" may be potentially "unused". I don't
>> think we should mix these 2.
>
>Hi Jiri
>
>Current flavours are:
>
>enum devlink_port_flavour {
>        DEVLINK_PORT_FLAVOUR_PHYSICAL, /* Any kind of a port physically
>                                        * facing the user.
>                                        */
>        DEVLINK_PORT_FLAVOUR_CPU, /* CPU port */
>        DEVLINK_PORT_FLAVOUR_DSA, /* Distributed switch architecture
>                                   * interconnect port.
>                                   */
>        DEVLINK_PORT_FLAVOUR_PCI_PF, /* Represents eswitch port for
>                                      * the PCI PF. It is an internal
>                                      * port that faces the PCI PF.
>                                      */
>        DEVLINK_PORT_FLAVOUR_PCI_VF, /* Represents eswitch port
>                                      * for the PCI VF. It is an internal
>                                      * port that faces the PCI VF.
>                                      */
>        DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */
>};
>
>A port in the DSA world is generally just a port on the switch. It is
>not limited in nature. It can be used as a PHYSICAL, or CPU, or a DSA
>port. We don't consider them as unused PHYISCAL ports, or unused CPU
>ports. They are just unused ports. Which is why i added an UNUSED
>flavour.

I get it. But I as I wrote previously, I wonder if used/unused should
not be another attribute. Then the flavour can be "undefined".

But, why do you want to show "unused" ports? Can the user do something
with them? What is the value in showing them?



>
>Now, it could be this world view is actually just a DSA world
>view. Maybe some ASICs have strict roles for their ports? They are not
>configurable? And then i could see it being an attribute? But that
>messes up the DSA world view :-(
>
>      Andrew

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Andrew Lunn]

> I get it. But I as I wrote previously, I wonder if used/unused should
> not be another attribute. Then the flavour can be "undefined".

In the DSA world, it is not undefined. It is clear defined as
unused. And it cannot be on-the-fly changed. It is a property of the
PCB, in that the pins exist on the chip, but they simply don't go
anywhere on the PCB. This is quite common on appliances, e.g. The
switch has 7 ports, but the installation in the aircraft is a big
ring, so there is a 'left', 'right', 'aux' and the CPU port. That
leaves 3 ports totally unused.

> But, why do you want to show "unused" ports? Can the user do something
> with them? What is the value in showing them?

Because they are just ports, they can have regions. We can look at the
region and be sure they are powered off, the boot loader etc has not
left them in a funny state, bridged to other ports, etc.

Regions are a developers tool, not a 'user' tools. So the idea of
hiding them by default in 'devlink port show' does make some sense,
and have a flag like -d for details, which includes them. In 'devlink
region show' i would probably list all regions, independent of any -d
flag.

      Andrew

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Jiri Pirko]

Wed, Sep 30, 2020 at 03:57:25PM CEST, andrew@lunn.ch wrote:
>> I get it. But I as I wrote previously, I wonder if used/unused should
>> not be another attribute. Then the flavour can be "undefined".
>
>In the DSA world, it is not undefined. It is clear defined as
>unused. And it cannot be on-the-fly changed. It is a property of the
>PCB, in that the pins exist on the chip, but they simply don't go
>anywhere on the PCB. This is quite common on appliances, e.g. The
>switch has 7 ports, but the installation in the aircraft is a big
>ring, so there is a 'left', 'right', 'aux' and the CPU port. That
>leaves 3 ports totally unused.

Understand the DSA usecase.


>
>> But, why do you want to show "unused" ports? Can the user do something
>> with them? What is the value in showing them?
>
>Because they are just ports, they can have regions. We can look at the

What do you mean by "regions"? Devlink regions? They are per-device, not
per-port. I have to be missing something.


>region and be sure they are powered off, the boot loader etc has not
>left them in a funny state, bridged to other ports, etc.

It is driver's responsibility to ensure that. But that does not mean
that the devlink port needs to be visible.


>
>Regions are a developers tool, not a 'user' tools. So the idea of
>hiding them by default in 'devlink port show' does make some sense,
>and have a flag like -d for details, which includes them. In 'devlink
>region show' i would probably list all regions, independent of any -d
>flag.
>
>      Andrew

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Andrew Lunn]

> What do you mean by "regions"? Devlink regions? They are per-device, not
> per-port. I have to be missing something.

The rest of the patch series, which add regions per port! This came
out of the discussion from the first version of this patchset, and
Jakub said it would make sense to add per port regions, rather than
have regions which embedded a port number in there name.

     Andrew
 

Re: [PATCH net-next v2 1/7] net: devlink: Add unused port flavour

[Jiri Pirko]

Wed, Sep 30, 2020 at 04:53:57PM CEST, andrew@lunn.ch wrote:
>> What do you mean by "regions"? Devlink regions? They are per-device, not
>> per-port. I have to be missing something.
>
>The rest of the patch series, which add regions per port! This came

Okay. Sorry about that. netdev ml kicked me out so I didn't receive
emails from it for couple of days :/


>out of the discussion from the first version of this patchset, and
>Jakub said it would make sense to add per port regions, rather than
>have regions which embedded a port number in there name.

For sure. If something is per-port, should be per-port. I agree.

>
>     Andrew
>