[PATCH net-next RFC v1 0/4] Add per port devlink regions

[PATCH net-next RFC v1 0/4] 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/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/8/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

DSA only instantiates devlink ports for switch ports which are used.
For this hardware, only 4 user ports and the CPU port have devlink
ports, which explains the discontinuous port regions.

Andrew Lunn (4):
  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/chip.c    |   8 +
 drivers/net/dsa/mv88e6xxx/devlink.c |  61 +++++++
 drivers/net/dsa/mv88e6xxx/devlink.h |   6 +-
 include/net/devlink.h               |  27 +++
 include/net/dsa.h                   |  19 +++
 net/core/devlink.c                  | 250 +++++++++++++++++++++++++---
 net/dsa/dsa.c                       |  14 ++
 7 files changed, 358 insertions(+), 27 deletions(-)

-- 
2.28.0

[PATCH net-next RFC v1 1/4] 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    | 251 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 252 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 045468390480..66469cdcdc1e 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)
 {
@@ -3905,6 +3922,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;
@@ -3952,6 +3974,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)
@@ -4198,16 +4225,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;
 
@@ -4226,10 +4267,76 @@ 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:
+	mutex_unlock(&devlink_mutex);
+	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_mutex);
+	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;
@@ -4239,25 +4346,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);
@@ -4270,8 +4362,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] ||
@@ -4281,7 +4375,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;
 
@@ -4298,9 +4404,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;
@@ -4311,7 +4419,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;
@@ -4347,7 +4468,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;
 
@@ -4469,10 +4595,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;
 
@@ -4493,8 +4621,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;
@@ -4531,6 +4672,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;
@@ -7622,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);
@@ -7645,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);
@@ -8723,6 +8871,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

Re: [PATCH net-next RFC v1 1/4] net: devlink: Add support for port regions

[Vladimir Oltean]

On Sat, Sep 19, 2020 at 04:43:29PM +0200, Andrew Lunn wrote:
> 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    | 251 +++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 252 insertions(+), 26 deletions(-)
> 
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 045468390480..66469cdcdc1e 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4198,16 +4225,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);
> +

This looks like a simple enough solution, but am I right that old
kernels, which ignore this new DEVLINK_ATTR_PORT_INDEX netlink
attribute, will consequently interpret any devlink command for a port as
being for a global region? Sure, in the end, that kernel will probably
fail anyway, due to the region name mismatch. And at the moment there
isn't any driver that registers a global and a port region with the same
name. But when that will happen, the user space tools of the future will
trigger incorrect behavior into the kernel of today, instead of it
reporting an unsupported operation as it should. Or am I
misunderstanding?

>  	if (!region)
>  		return -EINVAL;
>  

Thanks,
-Vladimir

Re: [PATCH net-next RFC v1 1/4] net: devlink: Add support for port regions

[Vladimir Oltean]

On Mon, Sep 21, 2020 at 02:45:39AM +0300, Vladimir Oltean wrote:
> This looks like a simple enough solution, but am I right that old
> kernels, which ignore this new DEVLINK_ATTR_PORT_INDEX netlink
> attribute, will consequently interpret any devlink command for a port as
> being for a global region? Sure, in the end, that kernel will probably
> fail anyway, due to the region name mismatch. And at the moment there
> isn't any driver that registers a global and a port region with the same
> name. But when that will happen, the user space tools of the future will
> trigger incorrect behavior into the kernel of today, instead of it
> reporting an unsupported operation as it should. Or am I
> misunderstanding?

Thinking about this more, I believe that the only conditions that need
to be avoided are:
- mlx4 should never create a port region called "cr-space" or "fw-health"
- ice should never create a port region called "nvm-flash" or
  "device-caps"
- netdevsim should never create a port region called "dummy"
- mv88e6xxx should never create a port region called "global1",
  "global2" or "atu"

Because these are the only region names supported by kernels that don't
parse DEVLINK_ATTR_PORT_INDEX, I think we don't need to complicate the
solution, and go with DEVLINK_ATTR_PORT_INDEX.

-Vladimir

Re: [PATCH net-next RFC v1 1/4] net: devlink: Add support for port regions

[Andrew Lunn]

On Mon, Sep 21, 2020 at 12:23:18AM +0000, Vladimir Oltean wrote:
> On Mon, Sep 21, 2020 at 02:45:39AM +0300, Vladimir Oltean wrote:
> > This looks like a simple enough solution, but am I right that old
> > kernels, which ignore this new DEVLINK_ATTR_PORT_INDEX netlink
> > attribute, will consequently interpret any devlink command for a port as
> > being for a global region? Sure, in the end, that kernel will probably
> > fail anyway, due to the region name mismatch. And at the moment there
> > isn't any driver that registers a global and a port region with the same
> > name. But when that will happen, the user space tools of the future will
> > trigger incorrect behavior into the kernel of today, instead of it
> > reporting an unsupported operation as it should. Or am I
> > misunderstanding?
> 
> Thinking about this more, I believe that the only conditions that need
> to be avoided are:
> - mlx4 should never create a port region called "cr-space" or "fw-health"
> - ice should never create a port region called "nvm-flash" or
>   "device-caps"
> - netdevsim should never create a port region called "dummy"
> - mv88e6xxx should never create a port region called "global1",
>   "global2" or "atu"
> 
> Because these are the only region names supported by kernels that don't
> parse DEVLINK_ATTR_PORT_INDEX, I think we don't need to complicate the
> solution, and go with DEVLINK_ATTR_PORT_INDEX.

It would be easy to check when adding a per port region if a global
region of the same name already exists. Checking when adding a global
region to see if there is a port region with the same name is a bit
more work, but doable.

     Andrew

Re: [PATCH net-next RFC v1 1/4] net: devlink: Add support for port regions

[Vladimir Oltean]

On Mon, Sep 21, 2020 at 05:02:13AM +0200, Andrew Lunn wrote:
> On Mon, Sep 21, 2020 at 12:23:18AM +0000, Vladimir Oltean wrote:
> > On Mon, Sep 21, 2020 at 02:45:39AM +0300, Vladimir Oltean wrote:
> > > This looks like a simple enough solution, but am I right that old
> > > kernels, which ignore this new DEVLINK_ATTR_PORT_INDEX netlink
> > > attribute, will consequently interpret any devlink command for a port as
> > > being for a global region? Sure, in the end, that kernel will probably
> > > fail anyway, due to the region name mismatch. And at the moment there
> > > isn't any driver that registers a global and a port region with the same
> > > name. But when that will happen, the user space tools of the future will
> > > trigger incorrect behavior into the kernel of today, instead of it
> > > reporting an unsupported operation as it should. Or am I
> > > misunderstanding?
> >
> > Thinking about this more, I believe that the only conditions that need
> > to be avoided are:
> > - mlx4 should never create a port region called "cr-space" or "fw-health"
> > - ice should never create a port region called "nvm-flash" or
> >   "device-caps"
> > - netdevsim should never create a port region called "dummy"
> > - mv88e6xxx should never create a port region called "global1",
> >   "global2" or "atu"
> >
> > Because these are the only region names supported by kernels that don't
> > parse DEVLINK_ATTR_PORT_INDEX, I think we don't need to complicate the
> > solution, and go with DEVLINK_ATTR_PORT_INDEX.
>
> It would be easy to check when adding a per port region if a global
> region of the same name already exists. Checking when adding a global
> region to see if there is a port region with the same name is a bit
> more work, but doable.

See, I don't think that the check would be useful. By the time the new
kernel understands DEVLINK_ATTR_PORT_INDEX, the global and port regions
are already properly namespaced. So future drivers can have a port and
a global region of the same name. The problem is only with kernels that
don't understand DEVLINK_ATTR_PORT_INDEX. These only support global
regions. The concern is that future port regions for these specific 4
drivers might confuse them.

-Vladimir

[PATCH net-next RFC v1 2/4] 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/core/devlink.c |  3 +--
 net/dsa/dsa.c      | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index d16057c5987a..01da896b2998 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -665,6 +665,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/core/devlink.c b/net/core/devlink.c
index 66469cdcdc1e..4701ec17f3da 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4292,7 +4292,6 @@ static int devlink_nl_cmd_region_get_port_dumpit(struct sk_buff *msg,
 	}
 
 out:
-	mutex_unlock(&devlink_mutex);
 	return err;
 }
 
@@ -4330,7 +4329,7 @@ static int devlink_nl_cmd_region_get_devlink_dumpit(struct sk_buff *msg,
 	}
 
 out:
-	mutex_unlock(&devlink_mutex);
+	mutex_unlock(&devlink->lock);
 	return err;
 }
 
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

Re: [PATCH net-next RFC v1 2/4] net: dsa: Add devlink port regions support to DSA

[Vladimir Oltean]

On Sat, Sep 19, 2020 at 04:43: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>
> ---
>  include/net/dsa.h  |  5 +++++
>  net/core/devlink.c |  3 +--
>  net/dsa/dsa.c      | 14 ++++++++++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index d16057c5987a..01da896b2998 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -665,6 +665,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/core/devlink.c b/net/core/devlink.c
> index 66469cdcdc1e..4701ec17f3da 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4292,7 +4292,6 @@ static int devlink_nl_cmd_region_get_port_dumpit(struct sk_buff *msg,
>  	}
>  
>  out:
> -	mutex_unlock(&devlink_mutex);

This diff is probably not intended?

>  	return err;
>  }
>  
> @@ -4330,7 +4329,7 @@ static int devlink_nl_cmd_region_get_devlink_dumpit(struct sk_buff *msg,
>  	}
>  
>  out:
> -	mutex_unlock(&devlink_mutex);
> +	mutex_unlock(&devlink->lock);

Similar here.

>  	return err;
>  }
>  
> 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
> 

Thanks,
-Vladimir

Re: [PATCH net-next RFC v1 2/4] net: dsa: Add devlink port regions support to DSA

[Andrew Lunn]

On Sun, Sep 20, 2020 at 11:23:29PM +0000, Vladimir Oltean wrote:
> On Sat, Sep 19, 2020 at 04:43: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>
> > ---
> >  include/net/dsa.h  |  5 +++++
> >  net/core/devlink.c |  3 +--
> >  net/dsa/dsa.c      | 14 ++++++++++++++
> >  3 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index d16057c5987a..01da896b2998 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -665,6 +665,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/core/devlink.c b/net/core/devlink.c
> > index 66469cdcdc1e..4701ec17f3da 100644
> > --- a/net/core/devlink.c
> > +++ b/net/core/devlink.c
> > @@ -4292,7 +4292,6 @@ static int devlink_nl_cmd_region_get_port_dumpit(struct sk_buff *msg,
> >  	}
> >  
> >  out:
> > -	mutex_unlock(&devlink_mutex);
> 
> This diff is probably not intended?

Correct. Looks like i squashed a mutex fix into the wrong patch :-(

	 Andrew

[PATCH net-next RFC v1 3/4] 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 01da896b2998..a24d5158ee0c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -685,6 +685,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

Re: [PATCH net-next RFC v1 3/4] net: dsa: Add helper for converting devlink port to ds and port

[Vladimir Oltean]

On Sat, Sep 19, 2020 at 04:43:31PM +0200, Andrew Lunn wrote:
> 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 01da896b2998..a24d5158ee0c 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -685,6 +685,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)

How about dsa_devlink_port_to_index?
It avoids the repetition and it also indicates more clearly that it
returns an index rather than a struct dsa_port, without needing to fire
up ctags.

> +{
> +	return port->index;
> +}
> +
>  struct dsa_switch_driver {
>  	struct list_head	list;
>  	const struct dsa_switch_ops *ops;
> -- 
> 2.28.0
> 

Thanks,
-Vladimir

Re: [PATCH net-next RFC v1 3/4] net: dsa: Add helper for converting devlink port to ds and port

[Andrew Lunn]

On Sun, Sep 20, 2020 at 11:52:03PM +0000, Vladimir Oltean wrote:
> On Sat, Sep 19, 2020 at 04:43:31PM +0200, Andrew Lunn wrote:
> > 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 01da896b2998..a24d5158ee0c 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -685,6 +685,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)
> 
> How about dsa_devlink_port_to_index?
> It avoids the repetition and it also indicates more clearly that it
> returns an index rather than a struct dsa_port, without needing to fire
> up ctags.

Hi Vladimir

Just finishing off the next version of these patches, and i looped
back to check i addressed all the comments.

This one i tend to disagree with. If you look at DSA drivers, a port
variable is always an integer index. dp is used to refer to a
dsa_port.

If anything, i would suggest we rename dsa_to_port() to dsa_to_dp(),
and dsa_port_from_netdev to dsa_dp_from_netdev() or maybe
dsa_netdev_to_dp().

    Andrew

Re: [PATCH net-next RFC v1 3/4] net: dsa: Add helper for converting devlink port to ds and port

[Vladimir Oltean]

On Sat, Sep 26, 2020 at 07:28:26PM +0200, Andrew Lunn wrote:
> Hi Vladimir
>
> Just finishing off the next version of these patches, and i looped
> back to check i addressed all the comments.
>
> This one i tend to disagree with. If you look at DSA drivers, a port
> variable is always an integer index. dp is used to refer to a
> dsa_port.
>
> If anything, i would suggest we rename dsa_to_port() to dsa_to_dp(),
> and dsa_port_from_netdev to dsa_dp_from_netdev() or maybe
> dsa_netdev_to_dp().

Maybe it's just me, but I don't like the "dp" name too much, and I think
all conversions of those functions sound worse with the new names.
But you do have a point with the inconsistency, so I suppose you could
leave the name as it is? Or maybe dsa_dlp_to_port() if you fancy that
abbreviation for a devlink port?

[PATCH net-next RFC v1 4/4] 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/chip.c    |  8 ++++
 drivers/net/dsa/mv88e6xxx/devlink.c | 61 +++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/devlink.h |  6 ++-
 3 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 9417412e5fce..3d2d813f7a79 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2741,10 +2741,16 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
+	err = mv88e6xxx_setup_devlink_regions_port(ds, chip, port);
+	if (err)
+		return err;
+
 	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_serdes_power(chip, port, true);
 	mv88e6xxx_reg_unlock(chip);
 
+	if (err)
+		mv88e6xxx_teardown_devlink_regions_port(chip, port);
 	return err;
 }
 
@@ -2752,6 +2758,8 @@ static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
+	mv88e6xxx_teardown_devlink_regions_port(chip, port);
+
 	mv88e6xxx_reg_lock(chip);
 	if (mv88e6xxx_serdes_power(chip, port, false))
 		dev_err(chip->dev, "failed to power off SERDES\n");
diff --git a/drivers/net/dsa/mv88e6xxx/devlink.c b/drivers/net/dsa/mv88e6xxx/devlink.c
index 81e1560db206..dc8a244aa154 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;
@@ -478,6 +514,31 @@ void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds)
 	mv88e6xxx_teardown_devlink_regions_global(chip);
 }
 
+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;
+}
+
+void mv88e6xxx_teardown_devlink_regions_port(struct mv88e6xxx_chip *chip,
+					     int port)
+{
+	if (chip->ports[port].region)
+		dsa_devlink_region_destroy(chip->ports[port].region);
+}
+
 static int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds,
 						  struct mv88e6xxx_chip *chip)
 {
diff --git a/drivers/net/dsa/mv88e6xxx/devlink.h b/drivers/net/dsa/mv88e6xxx/devlink.h
index 3d72db3dcf95..9302f0ee550d 100644
--- a/drivers/net/dsa/mv88e6xxx/devlink.h
+++ b/drivers/net/dsa/mv88e6xxx/devlink.h
@@ -14,7 +14,11 @@ int mv88e6xxx_devlink_param_set(struct dsa_switch *ds, u32 id,
 				struct devlink_param_gset_ctx *ctx);
 int mv88e6xxx_setup_devlink_regions(struct dsa_switch *ds);
 void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds);
-
+int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds,
+					 struct mv88e6xxx_chip *chip,
+					 int port);
+void mv88e6xxx_teardown_devlink_regions_port(struct mv88e6xxx_chip *chip,
+					     int port);
 int mv88e6xxx_devlink_info_get(struct dsa_switch *ds,
 			       struct devlink_info_req *req,
 			       struct netlink_ext_ack *extack);
-- 
2.28.0

Re: [PATCH net-next RFC v1 4/4] net: dsa: mv88e6xxx: Add per port devlink regions

[Vladimir Oltean]

On Sat, Sep 19, 2020 at 04:43: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/chip.c    |  8 ++++
>  drivers/net/dsa/mv88e6xxx/devlink.c | 61 +++++++++++++++++++++++++++++
>  drivers/net/dsa/mv88e6xxx/devlink.h |  6 ++-
>  3 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 9417412e5fce..3d2d813f7a79 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2741,10 +2741,16 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	int err;
>  
> +	err = mv88e6xxx_setup_devlink_regions_port(ds, chip, port);
> +	if (err)
> +		return err;
> +
>  	mv88e6xxx_reg_lock(chip);
>  	err = mv88e6xxx_serdes_power(chip, port, true);
>  	mv88e6xxx_reg_unlock(chip);
>  
> +	if (err)
> +		mv88e6xxx_teardown_devlink_regions_port(chip, port);
>  	return err;
>  }
>  
> @@ -2752,6 +2758,8 @@ static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  
> +	mv88e6xxx_teardown_devlink_regions_port(chip, port);
> +
>  	mv88e6xxx_reg_lock(chip);
>  	if (mv88e6xxx_serdes_power(chip, port, false))
>  		dev_err(chip->dev, "failed to power off SERDES\n");
> diff --git a/drivers/net/dsa/mv88e6xxx/devlink.c b/drivers/net/dsa/mv88e6xxx/devlink.c
> index 81e1560db206..dc8a244aa154 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;
> @@ -478,6 +514,31 @@ void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds)
>  	mv88e6xxx_teardown_devlink_regions_global(chip);
>  }
>  
> +int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds,
> +					 struct mv88e6xxx_chip *chip,
> +					 int port)

You probably don't need to pass both *ds and *chip here, since you can
derive one from the other.

> +{
> +	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;
> +}
> +
> +void mv88e6xxx_teardown_devlink_regions_port(struct mv88e6xxx_chip *chip,
> +					     int port)
> +{
> +	if (chip->ports[port].region)
> +		dsa_devlink_region_destroy(chip->ports[port].region);
> +}
> +
>  static int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds,
>  						  struct mv88e6xxx_chip *chip)
>  {
> diff --git a/drivers/net/dsa/mv88e6xxx/devlink.h b/drivers/net/dsa/mv88e6xxx/devlink.h
> index 3d72db3dcf95..9302f0ee550d 100644
> --- a/drivers/net/dsa/mv88e6xxx/devlink.h
> +++ b/drivers/net/dsa/mv88e6xxx/devlink.h
> @@ -14,7 +14,11 @@ int mv88e6xxx_devlink_param_set(struct dsa_switch *ds, u32 id,
>  				struct devlink_param_gset_ctx *ctx);
>  int mv88e6xxx_setup_devlink_regions(struct dsa_switch *ds);
>  void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds);
> -
> +int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds,
> +					 struct mv88e6xxx_chip *chip,
> +					 int port);
> +void mv88e6xxx_teardown_devlink_regions_port(struct mv88e6xxx_chip *chip,
> +					     int port);
>  int mv88e6xxx_devlink_info_get(struct dsa_switch *ds,
>  			       struct devlink_info_req *req,
>  			       struct netlink_ext_ack *extack);
> -- 
> 2.28.0
> 

Thanks,
-Vladimir

Re: [PATCH net-next RFC v1 0/4] Add per port devlink regions

[Vladimir Oltean]

On Sat, Sep 19, 2020 at 04:43:28PM +0200, Andrew Lunn wrote:
>
> DSA only instantiates devlink ports for switch ports which are used.
> For this hardware, only 4 user ports and the CPU port have devlink
> ports, which explains the discontinuous port regions.

This is not so much a choice, as it is a workaround of the fact that
dsa_port_setup(), which registers devlink ports with devlink, is called
after ds->ops->setup(), so you can't register your port regions from
the same place as the global regions now.

So you're doing it from ds->ops->port_enable(), which is the DSA wrapper
for .ndo_open(). So, consequently, your port regions will only be
registered when the port is up, and will be unregistered when it goes
down. Is that what you want? I understand that users probably think they
want to debug only the ports that they actively use, but I've heard of
at least one problem in the past which was caused by invalid settings
(flooding in that case) on a port that was down. Sure, this is probably
a rare situation, but as I said, somebody trying to use port regions to
debug something like that is probably going to have a hard time, because
it isn't an easy surgery to figure the probe ordering out.

Thanks,
-Vladimir

Re: [PATCH net-next RFC v1 0/4] Add per port devlink regions

[Florian Fainelli]

On 9/20/2020 4:33 PM, Vladimir Oltean wrote:
> On Sat, Sep 19, 2020 at 04:43:28PM +0200, Andrew Lunn wrote:
>>
>> DSA only instantiates devlink ports for switch ports which are used.
>> For this hardware, only 4 user ports and the CPU port have devlink
>> ports, which explains the discontinuous port regions.
> 
> This is not so much a choice, as it is a workaround of the fact that
> dsa_port_setup(), which registers devlink ports with devlink, is called
> after ds->ops->setup(), so you can't register your port regions from
> the same place as the global regions now.
> 
> So you're doing it from ds->ops->port_enable(), which is the DSA wrapper
> for .ndo_open(). So, consequently, your port regions will only be
> registered when the port is up, and will be unregistered when it goes
> down. Is that what you want? I understand that users probably think they
> want to debug only the ports that they actively use, but I've heard of
> at least one problem in the past which was caused by invalid settings
> (flooding in that case) on a port that was down. Sure, this is probably
> a rare situation, but as I said, somebody trying to use port regions to
> debug something like that is probably going to have a hard time, because
> it isn't an easy surgery to figure the probe ordering out.

Being able to debug the switch configuration as soon as it gets 
registered with DSA all the way through enabling an user port has 
definitively a lot of value so we should aim to support that use case.
-- 
Florian

Re: [PATCH net-next RFC v1 0/4] Add per port devlink regions

[Andrew Lunn]

On Sun, Sep 20, 2020 at 11:33:41PM +0000, Vladimir Oltean wrote:
> On Sat, Sep 19, 2020 at 04:43:28PM +0200, Andrew Lunn wrote:
> >
> > DSA only instantiates devlink ports for switch ports which are used.
> > For this hardware, only 4 user ports and the CPU port have devlink
> > ports, which explains the discontinuous port regions.
> 
> This is not so much a choice, as it is a workaround of the fact that
> dsa_port_setup(), which registers devlink ports with devlink, is called
> after ds->ops->setup(), so you can't register your port regions from
> the same place as the global regions now.

Correct.

> So you're doing it from ds->ops->port_enable(), which is the DSA wrapper
> for .ndo_open(). So, consequently, your port regions will only be
> registered when the port is up, and will be unregistered when it goes
> down. Is that what you want? I understand that users probably think they
> want to debug only the ports that they actively use, but I've heard of
> at least one problem in the past which was caused by invalid settings
> (flooding in that case) on a port that was down. Sure, this is probably
> a rare situation, but as I said, somebody trying to use port regions to
> debug something like that is probably going to have a hard time, because
> it isn't an easy surgery to figure the probe ordering out.

I did intially create the port instances at the same time as the
global ones, and it died a horrible death. And i was aiming to
register a region for each port, not just those which are used.

This splits into two problems.

1) Devlink has no concept of a port which is unused. We simply don't
register unused ports. So we need to add a new devlink_port_flavour:
DEVLINK_PORT_FLAVOUR_UNUSED. That seems easy enough.

2) We need to rearrange the order the core sets stuff up, such that it
registers devlink ports before calling the DSA driver setup()
method. I think that is possible after a quick look at the code.

	Andrew