[PATCH net-next 0/7] Devlink health updates

[PATCH net-next 0/7] Devlink health updates

[Eran Ben Elisha]

This patchset fixes some comments that were received for the devlink
health series, mostly around the devlink health buffers API.

It offers a new devlink<->driver API for passing health dump and diagnose info.
As part of this patchset, the new API is developed and integrated into the
devlink health and mlx5e TX reporter.
Also, added some helpers together with the new API, which reduce the code
required by the driver to fill dump and diagnose significantly.

Eventually, it also deletes the old API.

In addition, it includes some small fixes in the devlink and mlx5e TX reporter.


Eran Ben Elisha (7):
  devlink: Add devlink msg API
  net/mlx5e: Move driver to use devlink msg API
  devlink: move devlink health reporter to use devlink msg API
  devlink: Delete depracated health buffers API
  devlink: Remove spaces around "=" in the logger print
  devlink: Fix use-after-free at reporter destroy
  net/mlx5e: Add RTNL lock to TX recover flow

 .../mellanox/mlx5/core/en/reporter_tx.c       | 124 +---
 include/net/devlink.h                         |  79 +--
 include/trace/events/devlink.h                |   2 +-
 include/uapi/linux/devlink.h                  |  14 +-
 net/core/devlink.c                            | 633 ++++++++----------
 5 files changed, 342 insertions(+), 510 deletions(-)

-- 
2.17.1

[PATCH net-next 1/7] devlink: Add devlink msg API

[Eran Ben Elisha]

Devlink msg is a mechanism to pass descriptors between drivers and
devlink, in json-like format. The API allows the driver to add objects,
object pair, value array (nested attributes), value and name.

Driver can use this API to fill the msg context in a format which can be
translated by the devlink to the netlink message later.

With this API, driver does not need to declare the total size per message
context, and it dynamically add more messages to the context (bounded by
the system memory limitation only).
There is an implicit request to have the preliminary main objects size
created via this API to be upper bounded by netlink SKB size, as those
objects do not get fragmented between different SKBs when passed to the
netlink layer.

Devlink msg will replace devlink buffers for health reporters. Devlink
buffers which will be deprecated and removed in the downstream patch.

Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
CC: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
---
 include/net/devlink.h        |  70 ++++++
 include/uapi/linux/devlink.h |   8 +
 net/core/devlink.c           | 455 +++++++++++++++++++++++++++++++++++
 3 files changed, 533 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index a81a1b7a67d7..fe323e9b14e1 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -424,6 +424,7 @@ struct devlink_region;
 
 typedef void devlink_snapshot_data_dest_t(const void *data);
 
+struct devlink_msg_ctx;
 struct devlink_health_buffer;
 struct devlink_health_reporter;
 
@@ -615,6 +616,21 @@ int devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
 				   u8 *data, u32 snapshot_id,
 				   devlink_snapshot_data_dest_t *data_destructor);
 
+int devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype);
+int devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx);
+int devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
+			  void *value, u16 value_len, u8 value_nla_type);
+int devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx,
+			 char *name);
+int devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
+				    char *name, void *value, u16 value_len,
+				    u8 value_nla_type);
+int devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
+				     char *name, void **value, u16 *value_len,
+				     u8 *value_nla_type, int array_size);
+int devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name);
+int devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name);
+
 int devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
 				     int attrtype);
 void devlink_health_buffer_nest_end(struct devlink_health_buffer *buffer);
@@ -903,6 +919,60 @@ devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
 	return 0;
 }
 
+static inline int
+devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype)
+{
+	return 0;
+}
+
+static inline int
+devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx)
+{
+	return 0;
+}
+
+static inline int
+devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
+		      void *value, u16 value_len, u8 value_nla_type)
+{
+	return 0;
+}
+
+static inline int
+devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx,
+		     char *name)
+{
+	return 0;
+}
+
+static inline int
+devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
+				char *name, void *value, u16 value_len,
+				u8 value_nla_type)
+{
+	return 0;
+}
+
+static inline int
+devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
+				 char *name, void **value, u16 *value_len,
+				 u8 *value_nla_type, int array_size)
+{
+	return 0;
+}
+
+static inline int
+devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name)
+{
+	return 0;
+}
+
+static inline int
+devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name)
+{
+	return 0;
+}
+
 static inline int
 devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
 				 int attrtype)
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 6b26bb2ce4dc..68eeda1d0455 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -300,6 +300,14 @@ enum devlink_attr {
 	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE,	/* u8 */
 	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA,	/* dynamic */
 
+	DEVLINK_ATTR_MSG_OBJECT,		/* nested */
+	DEVLINK_ATTR_MSG_OBJECT_PAIR,		/* nested */
+	DEVLINK_ATTR_MSG_OBJECT_NAME,		/* string */
+	DEVLINK_ATTR_MSG_OBJECT_VALUE,		/* nested */
+	DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY,	/* nested */
+	DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,	/* u8 */
+	DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,	/* dynamic */
+
 	DEVLINK_ATTR_HEALTH_REPORTER,			/* nested */
 	DEVLINK_ATTR_HEALTH_REPORTER_NAME,		/* string */
 	DEVLINK_ATTR_HEALTH_REPORTER_STATE,		/* u8 */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 60248a53c0ad..57ca096849b3 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4098,6 +4098,461 @@ devlink_health_buffer_snd(struct genl_info *info,
 	return err;
 }
 
+struct devlink_msg {
+	struct list_head list;
+	int attrtype;
+	u8 nla_type;
+	u16 len;
+	int value[0];
+};
+
+struct devlink_msg_ctx {
+	struct list_head msg_list;
+	int max_nested_depth;
+	int curr_nest;
+};
+
+#define DEVLINK_MSG_MAX_SIZE (4096 - GENL_HDRLEN)
+
+static struct devlink_msg_ctx *devlink_msg_ctx_alloc(void)
+{
+	struct devlink_msg_ctx *msg_ctx;
+
+	msg_ctx = kzalloc(sizeof(*msg_ctx), GFP_KERNEL);
+	if (!msg_ctx)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&msg_ctx->msg_list);
+
+	return msg_ctx;
+}
+
+static void devlink_msg_ctx_free(struct devlink_msg_ctx *msg_ctx)
+{
+	struct devlink_msg *msg, *tm;
+
+	list_for_each_entry_safe(msg, tm, &msg_ctx->msg_list, list) {
+		list_del(&msg->list);
+		kfree(msg);
+	}
+	kfree(msg_ctx);
+}
+
+int devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype)
+{
+	struct devlink_msg *nest_msg;
+
+	if (attrtype != DEVLINK_ATTR_MSG_OBJECT &&
+	    attrtype != DEVLINK_ATTR_MSG_OBJECT_PAIR &&
+	    attrtype != DEVLINK_ATTR_MSG_OBJECT_VALUE &&
+	    attrtype != DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY)
+		return -EINVAL;
+
+	nest_msg = kzalloc(sizeof(*nest_msg), GFP_KERNEL);
+	if (!nest_msg)
+		return -ENOMEM;
+
+	nest_msg->attrtype = attrtype;
+	msg_ctx->curr_nest++;
+	msg_ctx->max_nested_depth = max(msg_ctx->max_nested_depth,
+					msg_ctx->curr_nest);
+	list_add_tail(&nest_msg->list, &msg_ctx->msg_list);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devlink_msg_nest_start);
+
+int devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx)
+{
+	struct devlink_msg *nest_msg;
+
+	WARN_ON(!msg_ctx->curr_nest);
+	nest_msg = kzalloc(sizeof(*nest_msg), GFP_KERNEL);
+	if (!nest_msg)
+		return -ENOMEM;
+
+	msg_ctx->curr_nest--;
+	list_add_tail(&nest_msg->list, &msg_ctx->msg_list);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devlink_msg_nest_end);
+
+int devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
+			  void *value, u16 value_len, u8 value_nla_type)
+{
+	struct devlink_msg *value_msg;
+
+	if (value_len > DEVLINK_MSG_MAX_SIZE)
+		return -EMSGSIZE;
+
+	if (value_nla_type != NLA_U8 &&
+	    value_nla_type != NLA_U32 &&
+	    value_nla_type != NLA_U64 &&
+	    value_nla_type != NLA_NUL_STRING &&
+	    value_nla_type != NLA_BINARY)
+		return -EINVAL;
+
+	value_msg = kzalloc(sizeof(*value_msg) + value_len, GFP_KERNEL);
+	if (!value_msg)
+		return -ENOMEM;
+
+	value_msg->nla_type = value_nla_type;
+	value_msg->len = value_len;
+	value_msg->attrtype = DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA;
+	memcpy(&value_msg->value, value, value_len);
+	list_add_tail(&value_msg->list, &msg_ctx->msg_list);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devlink_msg_put_value);
+
+int devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx, char *name)
+{
+	struct devlink_msg *name_msg;
+
+	if (strlen(name) + 1 > DEVLINK_MSG_MAX_SIZE)
+		return -EMSGSIZE;
+
+	name_msg = kzalloc(sizeof(*name_msg) + strlen(name) + 1, GFP_KERNEL);
+	if (!name_msg)
+		return -ENOMEM;
+
+	name_msg->nla_type = NLA_NUL_STRING;
+	name_msg->len = strlen(name) + 1;
+	name_msg->attrtype = DEVLINK_ATTR_MSG_OBJECT_NAME;
+	memcpy(&name_msg->value, name, name_msg->len);
+	list_add_tail(&name_msg->list, &msg_ctx->msg_list);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devlink_msg_put_name);
+
+int devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
+				    char *name, void *value, u16 value_len,
+				    u8 value_nla_type)
+{
+	int err;
+
+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
+	if (err)
+		return err;
+
+	err = devlink_msg_put_name(msg_ctx, name);
+	if (err)
+		return err;
+
+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
+	if (err)
+		return err;
+
+	err = devlink_msg_put_value(msg_ctx, value, value_len, value_nla_type);
+	if (err)
+		return err;
+
+	err = devlink_msg_nest_end(msg_ctx);
+	if (err)
+		return err;
+
+	err = devlink_msg_nest_end(msg_ctx);
+	if (err)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devlink_msg_put_name_value_pair);
+
+int devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
+				     char *name, void **value, u16 *value_len,
+				     u8 *value_nla_type, int array_size)
+{
+	int err, i;
+
+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
+	if (err)
+		return err;
+
+	err = devlink_msg_put_name(msg_ctx, name);
+	if (err)
+		return err;
+
+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
+	if (err)
+		return err;
+
+	err = devlink_msg_nest_start(msg_ctx,
+				     DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY);
+	if (err)
+		return err;
+
+	for (i = 0; i < array_size; i++) {
+		err = devlink_msg_nest_start(msg_ctx,
+					     DEVLINK_ATTR_MSG_OBJECT_VALUE);
+		if (err)
+			return err;
+
+		err = devlink_msg_put_value(msg_ctx, value[i],
+					    value_len[i], value_nla_type[i]);
+		if (err)
+			return err;
+		err = devlink_msg_nest_end(msg_ctx);
+		if (err)
+			return err;
+	}
+
+	err = devlink_msg_nest_end(msg_ctx);
+	if (err)
+		return err;
+
+	err = devlink_msg_nest_end(msg_ctx);
+	if (err)
+		return err;
+
+	err = devlink_msg_nest_end(msg_ctx);
+	if (err)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devlink_msg_put_name_value_array);
+
+int devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name)
+{
+	int err;
+
+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT);
+	if (err)
+		return err;
+
+	if (!name)
+		return 0;
+
+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
+	if (err)
+		return err;
+
+	err = devlink_msg_put_name(msg_ctx, name);
+	if (err)
+		return err;
+
+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
+	if (err)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devlink_msg_object_start);
+
+int devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name)
+{
+	int err;
+
+	if (!name)
+		goto end_object;
+
+	err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT_VALUE */
+	if (err)
+		return err;
+
+	err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT_PAIR */
+	if (err)
+		return err;
+
+end_object:
+	err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT */
+	if (err)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devlink_msg_object_end);
+
+static int
+devlink_msg_fill_data(struct sk_buff *skb, struct devlink_msg *msg)
+{
+	int err;
+
+	switch (msg->nla_type) {
+	case NLA_U8:
+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
+				 *(u8 *)msg->value);
+		break;
+	case NLA_U32:
+		err = nla_put_u32(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
+				  *(u32 *)msg->value);
+		break;
+	case NLA_U64:
+		err = nla_put_u64_64bit(skb,
+					DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
+					*(u64 *)msg->value, DEVLINK_ATTR_PAD);
+		break;
+	case NLA_NUL_STRING:
+		err = nla_put_string(skb,
+				     DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
+				     (char *)&msg->value);
+		break;
+	case NLA_BINARY:
+		err = nla_put(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
+			      msg->len, (void *)&msg->value);
+		break;
+	default:
+		err = -EINVAL;
+		break;
+	}
+
+	return err;
+}
+
+static int
+devlink_msg_fill_type(struct sk_buff *skb, struct devlink_msg *msg)
+{
+	int err;
+
+	switch (msg->nla_type) {
+	case NLA_U8:
+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
+				 NLA_U8);
+		break;
+	case NLA_U32:
+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
+				 NLA_U32);
+		break;
+	case NLA_U64:
+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
+				 NLA_U64);
+		break;
+	case NLA_NUL_STRING:
+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
+				 NLA_NUL_STRING);
+		break;
+	case NLA_BINARY:
+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
+				 NLA_BINARY);
+		break;
+	default:
+		err = -EINVAL;
+		break;
+	}
+
+	return err;
+}
+
+static int
+devlink_msg_prepare_skb(struct sk_buff *skb, struct devlink_msg_ctx *msg_ctx,
+			int *start)
+{
+	struct nlattr **nlattr_arr;
+	struct devlink_msg *msg;
+	int j = 0, i = 0;
+	int err;
+
+	nlattr_arr = kcalloc(msg_ctx->max_nested_depth,
+			     sizeof(*nlattr_arr), GFP_KERNEL);
+	if (!nlattr_arr)
+		return -EINVAL;
+
+	list_for_each_entry(msg, &msg_ctx->msg_list, list) {
+		if (j < *start) {
+			j++;
+			continue;
+		}
+
+		switch (msg->attrtype) {
+		case DEVLINK_ATTR_MSG_OBJECT:
+		case DEVLINK_ATTR_MSG_OBJECT_PAIR:
+		case DEVLINK_ATTR_MSG_OBJECT_VALUE:
+		case DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY:
+			nlattr_arr[i] = nla_nest_start(skb, msg->attrtype);
+			if (!nlattr_arr[i]) {
+				err = -EMSGSIZE;
+				goto nla_put_failure;
+			}
+			i++;
+			break;
+		case DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA:
+			err = devlink_msg_fill_data(skb, msg);
+			if (err)
+				goto nla_put_failure;
+			err = devlink_msg_fill_type(skb, msg);
+			if (err)
+				goto nla_put_failure;
+			break;
+		case DEVLINK_ATTR_MSG_OBJECT_NAME:
+			err = nla_put_string(skb, msg->attrtype,
+					     (char *)&msg->value);
+			if (err)
+				goto nla_put_failure;
+			break;
+		default: /* No attrtype */
+			nla_nest_end(skb, nlattr_arr[--i]);
+			break;
+		}
+		j++;
+		if (!i)
+			*start = j;
+	}
+
+	kfree(nlattr_arr);
+	return 0;
+
+nla_put_failure:
+	for (j = 0; j < i; j++)
+		nla_nest_cancel(skb, nlattr_arr[j]);
+	kfree(nlattr_arr);
+	return err;
+}
+
+static int devlink_msg_snd(struct genl_info *info,
+			   enum devlink_command cmd, int flags,
+			   struct devlink_msg_ctx *msg_ctx)
+{
+	struct sk_buff *skb;
+	struct nlmsghdr *nlh;
+	int err, index = 0;
+	bool last = false;
+	void *hdr;
+
+	while (!last) {
+		int tmp_index = index;
+
+		skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+		if (!skb)
+			return -ENOMEM;
+
+		hdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
+				  &devlink_nl_family, NLM_F_MULTI, cmd);
+		if (!hdr)
+			goto nla_put_failure;
+
+		err = devlink_msg_prepare_skb(skb, msg_ctx, &index);
+		if (!err)
+			last = true;
+		else if (err != -EMSGSIZE || tmp_index == index)
+			goto nla_put_failure;
+
+		genlmsg_end(skb, hdr);
+		err = genlmsg_reply(skb, info);
+		if (err)
+			return err;
+	}
+
+	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+	nlh = nlmsg_put(skb, info->snd_portid, info->snd_seq,
+			NLMSG_DONE, 0, flags | NLM_F_MULTI);
+	err = genlmsg_reply(skb, info);
+	if (err)
+		return err;
+
+	return 0;
+
+nla_put_failure:
+	err = -EIO;
+	nlmsg_free(skb);
+	return err;
+}
+
 struct devlink_health_reporter {
 	struct list_head list;
 	struct devlink_health_buffer **dump_buffers_array;
-- 
2.17.1

Re: [PATCH net-next 1/7] devlink: Add devlink msg API

[Jiri Pirko]

Tue, Jan 22, 2019 at 04:57:18PM CET, eranbe@mellanox.com wrote:
>Devlink msg is a mechanism to pass descriptors between drivers and
>devlink, in json-like format. The API allows the driver to add objects,
>object pair, value array (nested attributes), value and name.
>
>Driver can use this API to fill the msg context in a format which can be
>translated by the devlink to the netlink message later.
>
>With this API, driver does not need to declare the total size per message
>context, and it dynamically add more messages to the context (bounded by
>the system memory limitation only).
>There is an implicit request to have the preliminary main objects size
>created via this API to be upper bounded by netlink SKB size, as those
>objects do not get fragmented between different SKBs when passed to the
>netlink layer.
>
>Devlink msg will replace devlink buffers for health reporters. Devlink
>buffers which will be deprecated and removed in the downstream patch.
>
>Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>CC: Wei Yongjun <weiyongjun1@huawei.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> include/net/devlink.h        |  70 ++++++
> include/uapi/linux/devlink.h |   8 +
> net/core/devlink.c           | 455 +++++++++++++++++++++++++++++++++++
> 3 files changed, 533 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index a81a1b7a67d7..fe323e9b14e1 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -424,6 +424,7 @@ struct devlink_region;
> 
> typedef void devlink_snapshot_data_dest_t(const void *data);
> 
>+struct devlink_msg_ctx;
> struct devlink_health_buffer;
> struct devlink_health_reporter;
> 
>@@ -615,6 +616,21 @@ int devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
> 				   u8 *data, u32 snapshot_id,
> 				   devlink_snapshot_data_dest_t *data_destructor);
> 
>+int devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype);


devlink_msg_*() functions should work with struct devlink_msg.
devlink_msg_ctx*() functions should work with struct devlink_msg_ctx.
Please be consistent with the naming.

I think you can call this just "struct devlink_msg" of maybe "fmsg" as
for "formatted message" - so it is not confused with any other devlink
netlink message.



>+int devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx);
>+int devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
>+			  void *value, u16 value_len, u8 value_nla_type);
>+int devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx,
>+			 char *name);
>+int devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
>+				    char *name, void *value, u16 value_len,
>+				    u8 value_nla_type);
>+int devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
>+				     char *name, void **value, u16 *value_len,
>+				     u8 *value_nla_type, int array_size);
>+int devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name);
>+int devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name);
>+
> int devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
> 				     int attrtype);
> void devlink_health_buffer_nest_end(struct devlink_health_buffer *buffer);
>@@ -903,6 +919,60 @@ devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
> 	return 0;
> }
> 
>+static inline int
>+devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype)
>+{
>+	return 0;
>+}
>+
>+static inline int
>+devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx)
>+{
>+	return 0;
>+}
>+
>+static inline int
>+devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
>+		      void *value, u16 value_len, u8 value_nla_type)
>+{
>+	return 0;
>+}
>+
>+static inline int
>+devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx,
>+		     char *name)
>+{
>+	return 0;
>+}
>+
>+static inline int
>+devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
>+				char *name, void *value, u16 value_len,
>+				u8 value_nla_type)
>+{
>+	return 0;
>+}
>+
>+static inline int
>+devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
>+				 char *name, void **value, u16 *value_len,
>+				 u8 *value_nla_type, int array_size)
>+{
>+	return 0;
>+}
>+
>+static inline int
>+devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+	return 0;
>+}
>+
>+static inline int
>+devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+	return 0;
>+}
>+
> static inline int
> devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
> 				 int attrtype)
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 6b26bb2ce4dc..68eeda1d0455 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -300,6 +300,14 @@ enum devlink_attr {
> 	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE,	/* u8 */
> 	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA,	/* dynamic */
> 
>+	DEVLINK_ATTR_MSG_OBJECT,		/* nested */
>+	DEVLINK_ATTR_MSG_OBJECT_PAIR,		/* nested */
>+	DEVLINK_ATTR_MSG_OBJECT_NAME,		/* string */
>+	DEVLINK_ATTR_MSG_OBJECT_VALUE,		/* nested */
>+	DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY,	/* nested */
>+	DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,	/* u8 */
>+	DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,	/* dynamic */
>+
> 	DEVLINK_ATTR_HEALTH_REPORTER,			/* nested */
> 	DEVLINK_ATTR_HEALTH_REPORTER_NAME,		/* string */
> 	DEVLINK_ATTR_HEALTH_REPORTER_STATE,		/* u8 */
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 60248a53c0ad..57ca096849b3 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4098,6 +4098,461 @@ devlink_health_buffer_snd(struct genl_info *info,
> 	return err;
> }
> 
>+struct devlink_msg {

Let's call this "struct devlink_msg_item"


>+	struct list_head list;
>+	int attrtype;
>+	u8 nla_type;
>+	u16 len;
>+	int value[0];
>+};
>+
>+struct devlink_msg_ctx {
>+	struct list_head msg_list;
>+	int max_nested_depth;
>+	int curr_nest;
>+};
>+
>+#define DEVLINK_MSG_MAX_SIZE (4096 - GENL_HDRLEN)
>+
>+static struct devlink_msg_ctx *devlink_msg_ctx_alloc(void)
>+{
>+	struct devlink_msg_ctx *msg_ctx;
>+
>+	msg_ctx = kzalloc(sizeof(*msg_ctx), GFP_KERNEL);
>+	if (!msg_ctx)
>+		return ERR_PTR(-ENOMEM);
>+
>+	INIT_LIST_HEAD(&msg_ctx->msg_list);
>+
>+	return msg_ctx;
>+}
>+
>+static void devlink_msg_ctx_free(struct devlink_msg_ctx *msg_ctx)
>+{
>+	struct devlink_msg *msg, *tm;
>+
>+	list_for_each_entry_safe(msg, tm, &msg_ctx->msg_list, list) {
>+		list_del(&msg->list);
>+		kfree(msg);
>+	}
>+	kfree(msg_ctx);
>+}
>+
>+int devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype)
>+{
>+	struct devlink_msg *nest_msg;
>+
>+	if (attrtype != DEVLINK_ATTR_MSG_OBJECT &&
>+	    attrtype != DEVLINK_ATTR_MSG_OBJECT_PAIR &&
>+	    attrtype != DEVLINK_ATTR_MSG_OBJECT_VALUE &&
>+	    attrtype != DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY)
>+		return -EINVAL;
>+
>+	nest_msg = kzalloc(sizeof(*nest_msg), GFP_KERNEL);
>+	if (!nest_msg)
>+		return -ENOMEM;
>+
>+	nest_msg->attrtype = attrtype;
>+	msg_ctx->curr_nest++;
>+	msg_ctx->max_nested_depth = max(msg_ctx->max_nested_depth,
>+					msg_ctx->curr_nest);
>+	list_add_tail(&nest_msg->list, &msg_ctx->msg_list);
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_nest_start);
>+
>+int devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx)
>+{
>+	struct devlink_msg *nest_msg;
>+
>+	WARN_ON(!msg_ctx->curr_nest);
>+	nest_msg = kzalloc(sizeof(*nest_msg), GFP_KERNEL);
>+	if (!nest_msg)
>+		return -ENOMEM;
>+
>+	msg_ctx->curr_nest--;
>+	list_add_tail(&nest_msg->list, &msg_ctx->msg_list);
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_nest_end);
>+
>+int devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
>+			  void *value, u16 value_len, u8 value_nla_type)
>+{
>+	struct devlink_msg *value_msg;
>+
>+	if (value_len > DEVLINK_MSG_MAX_SIZE)
>+		return -EMSGSIZE;
>+
>+	if (value_nla_type != NLA_U8 &&
>+	    value_nla_type != NLA_U32 &&
>+	    value_nla_type != NLA_U64 &&
>+	    value_nla_type != NLA_NUL_STRING &&
>+	    value_nla_type != NLA_BINARY)
>+		return -EINVAL;
>+
>+	value_msg = kzalloc(sizeof(*value_msg) + value_len, GFP_KERNEL);

Looks fine.


>+	if (!value_msg)
>+		return -ENOMEM;
>+
>+	value_msg->nla_type = value_nla_type;
>+	value_msg->len = value_len;
>+	value_msg->attrtype = DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA;
>+	memcpy(&value_msg->value, value, value_len);
>+	list_add_tail(&value_msg->list, &msg_ctx->msg_list);
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_put_value);
>+
>+int devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+	struct devlink_msg *name_msg;
>+
>+	if (strlen(name) + 1 > DEVLINK_MSG_MAX_SIZE)
>+		return -EMSGSIZE;
>+
>+	name_msg = kzalloc(sizeof(*name_msg) + strlen(name) + 1, GFP_KERNEL);
>+	if (!name_msg)
>+		return -ENOMEM;
>+
>+	name_msg->nla_type = NLA_NUL_STRING;
>+	name_msg->len = strlen(name) + 1;
>+	name_msg->attrtype = DEVLINK_ATTR_MSG_OBJECT_NAME;
>+	memcpy(&name_msg->value, name, name_msg->len);
>+	list_add_tail(&name_msg->list, &msg_ctx->msg_list);
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_put_name);
>+
>+int devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
>+				    char *name, void *value, u16 value_len,
>+				    u8 value_nla_type)
>+{
>+	int err;
>+
>+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_put_name(msg_ctx, name);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_put_value(msg_ctx, value, value_len, value_nla_type);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_nest_end(msg_ctx);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_nest_end(msg_ctx);
>+	if (err)
>+		return err;
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_put_name_value_pair);
>+
>+int devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
>+				     char *name, void **value, u16 *value_len,
>+				     u8 *value_nla_type, int array_size)

This is limitting the arrays to plain values. One should be able to nest
objects inside. If fact, that is what you can to do with sqs:

object start
  name sqs
  array start
    object start
      index 0
      xxx yyy
    object end
    object start
      index 1
      xxx yyy
    object end
  array end
object end

So you need to have:
devlink_msg_put_array_start()
devlink_msg_put_array_end()


>+{
>+	int err, i;
>+
>+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_put_name(msg_ctx, name);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_nest_start(msg_ctx,
>+				     DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY);
>+	if (err)
>+		return err;
>+
>+	for (i = 0; i < array_size; i++) {
>+		err = devlink_msg_nest_start(msg_ctx,
>+					     DEVLINK_ATTR_MSG_OBJECT_VALUE);
>+		if (err)
>+			return err;
>+
>+		err = devlink_msg_put_value(msg_ctx, value[i],
>+					    value_len[i], value_nla_type[i]);
>+		if (err)
>+			return err;
>+		err = devlink_msg_nest_end(msg_ctx);
>+		if (err)
>+			return err;
>+	}
>+
>+	err = devlink_msg_nest_end(msg_ctx);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_nest_end(msg_ctx);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_nest_end(msg_ctx);
>+	if (err)
>+		return err;
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_put_name_value_array);
>+
>+int devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+	int err;
>+
>+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT);
>+	if (err)
>+		return err;
>+
>+	if (!name)
>+		return 0;
>+
>+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_put_name(msg_ctx, name);
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
>+	if (err)
>+		return err;
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_object_start);
>+
>+int devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+	int err;
>+
>+	if (!name)
>+		goto end_object;
>+
>+	err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT_VALUE */
>+	if (err)
>+		return err;
>+
>+	err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT_PAIR */
>+	if (err)
>+		return err;
>+
>+end_object:
>+	err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT */
>+	if (err)
>+		return err;
>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_object_end);
>+
>+static int
>+devlink_msg_fill_data(struct sk_buff *skb, struct devlink_msg *msg)
>+{
>+	int err;
>+
>+	switch (msg->nla_type) {
>+	case NLA_U8:
>+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+				 *(u8 *)msg->value);
>+		break;
>+	case NLA_U32:
>+		err = nla_put_u32(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+				  *(u32 *)msg->value);
>+		break;
>+	case NLA_U64:
>+		err = nla_put_u64_64bit(skb,
>+					DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+					*(u64 *)msg->value, DEVLINK_ATTR_PAD);
>+		break;
>+	case NLA_NUL_STRING:
>+		err = nla_put_string(skb,
>+				     DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+				     (char *)&msg->value);
>+		break;
>+	case NLA_BINARY:
>+		err = nla_put(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+			      msg->len, (void *)&msg->value);
>+		break;
>+	default:
>+		err = -EINVAL;
>+		break;
>+	}
>+
>+	return err;
>+}
>+
>+static int
>+devlink_msg_fill_type(struct sk_buff *skb, struct devlink_msg *msg)
>+{
>+	int err;
>+
>+	switch (msg->nla_type) {
>+	case NLA_U8:
>+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+				 NLA_U8);
>+		break;
>+	case NLA_U32:
>+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+				 NLA_U32);
>+		break;
>+	case NLA_U64:
>+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+				 NLA_U64);
>+		break;
>+	case NLA_NUL_STRING:
>+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+				 NLA_NUL_STRING);
>+		break;
>+	case NLA_BINARY:
>+		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+				 NLA_BINARY);
>+		break;
>+	default:
>+		err = -EINVAL;
>+		break;
>+	}
>+
>+	return err;
>+}
>+
>+static int
>+devlink_msg_prepare_skb(struct sk_buff *skb, struct devlink_msg_ctx *msg_ctx,
>+			int *start)
>+{
>+	struct nlattr **nlattr_arr;
>+	struct devlink_msg *msg;
>+	int j = 0, i = 0;
>+	int err;
>+
>+	nlattr_arr = kcalloc(msg_ctx->max_nested_depth,
>+			     sizeof(*nlattr_arr), GFP_KERNEL);
>+	if (!nlattr_arr)
>+		return -EINVAL;
>+
>+	list_for_each_entry(msg, &msg_ctx->msg_list, list) {
>+		if (j < *start) {
>+			j++;
>+			continue;
>+		}
>+
>+		switch (msg->attrtype) {
>+		case DEVLINK_ATTR_MSG_OBJECT:
>+		case DEVLINK_ATTR_MSG_OBJECT_PAIR:
>+		case DEVLINK_ATTR_MSG_OBJECT_VALUE:
>+		case DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY:
>+			nlattr_arr[i] = nla_nest_start(skb, msg->attrtype);
>+			if (!nlattr_arr[i]) {
>+				err = -EMSGSIZE;
>+				goto nla_put_failure;
>+			}
>+			i++;
>+			break;
>+		case DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA:
>+			err = devlink_msg_fill_data(skb, msg);
>+			if (err)
>+				goto nla_put_failure;
>+			err = devlink_msg_fill_type(skb, msg);
>+			if (err)
>+				goto nla_put_failure;
>+			break;
>+		case DEVLINK_ATTR_MSG_OBJECT_NAME:
>+			err = nla_put_string(skb, msg->attrtype,
>+					     (char *)&msg->value);
>+			if (err)
>+				goto nla_put_failure;
>+			break;
>+		default: /* No attrtype */
>+			nla_nest_end(skb, nlattr_arr[--i]);
>+			break;
>+		}
>+		j++;
>+		if (!i)
>+			*start = j;
>+	}
>+
>+	kfree(nlattr_arr);
>+	return 0;
>+
>+nla_put_failure:
>+	for (j = 0; j < i; j++)
>+		nla_nest_cancel(skb, nlattr_arr[j]);
>+	kfree(nlattr_arr);
>+	return err;
>+}
>+
>+static int devlink_msg_snd(struct genl_info *info,
>+			   enum devlink_command cmd, int flags,
>+			   struct devlink_msg_ctx *msg_ctx)
>+{
>+	struct sk_buff *skb;
>+	struct nlmsghdr *nlh;
>+	int err, index = 0;
>+	bool last = false;
>+	void *hdr;
>+
>+	while (!last) {
>+		int tmp_index = index;
>+
>+		skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+		if (!skb)
>+			return -ENOMEM;
>+
>+		hdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
>+				  &devlink_nl_family, NLM_F_MULTI, cmd);
>+		if (!hdr)
>+			goto nla_put_failure;
>+
>+		err = devlink_msg_prepare_skb(skb, msg_ctx, &index);
>+		if (!err)
>+			last = true;
>+		else if (err != -EMSGSIZE || tmp_index == index)
>+			goto nla_put_failure;
>+
>+		genlmsg_end(skb, hdr);
>+		err = genlmsg_reply(skb, info);
>+		if (err)
>+			return err;


Looks fine.


>+	}
>+
>+	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!skb)
>+		return -ENOMEM;
>+	nlh = nlmsg_put(skb, info->snd_portid, info->snd_seq,
>+			NLMSG_DONE, 0, flags | NLM_F_MULTI);
>+	err = genlmsg_reply(skb, info);
>+	if (err)
>+		return err;
>+
>+	return 0;
>+
>+nla_put_failure:
>+	err = -EIO;
>+	nlmsg_free(skb);
>+	return err;
>+}
>+
> struct devlink_health_reporter {
> 	struct list_head list;
> 	struct devlink_health_buffer **dump_buffers_array;
>-- 
>2.17.1
>

Re: [PATCH net-next 1/7] devlink: Add devlink msg API

[Eran Ben Elisha]

On 1/23/2019 4:31 PM, Jiri Pirko wrote:
> Tue, Jan 22, 2019 at 04:57:18PM CET, eranbe@mellanox.com wrote:
>> Devlink msg is a mechanism to pass descriptors between drivers and
>> devlink, in json-like format. The API allows the driver to add objects,
>> object pair, value array (nested attributes), value and name.
>>
>> Driver can use this API to fill the msg context in a format which can be
>> translated by the devlink to the netlink message later.
>>
>> With this API, driver does not need to declare the total size per message
>> context, and it dynamically add more messages to the context (bounded by
>> the system memory limitation only).
>> There is an implicit request to have the preliminary main objects size
>> created via this API to be upper bounded by netlink SKB size, as those
>> objects do not get fragmented between different SKBs when passed to the
>> netlink layer.
>>
>> Devlink msg will replace devlink buffers for health reporters. Devlink
>> buffers which will be deprecated and removed in the downstream patch.
>>
>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>> CC: Wei Yongjun <weiyongjun1@huawei.com>
>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>> ---
>> include/net/devlink.h        |  70 ++++++
>> include/uapi/linux/devlink.h |   8 +
>> net/core/devlink.c           | 455 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 533 insertions(+)
>>
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index a81a1b7a67d7..fe323e9b14e1 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -424,6 +424,7 @@ struct devlink_region;
>>
>> typedef void devlink_snapshot_data_dest_t(const void *data);
>>
>> +struct devlink_msg_ctx;
>> struct devlink_health_buffer;
>> struct devlink_health_reporter;
>>
>> @@ -615,6 +616,21 @@ int devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
>> 				   u8 *data, u32 snapshot_id,
>> 				   devlink_snapshot_data_dest_t *data_destructor);
>>
>> +int devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype);
> 
> 
> devlink_msg_*() functions should work with struct devlink_msg.
> devlink_msg_ctx*() functions should work with struct devlink_msg_ctx.
> Please be consistent with the naming.
> 
> I think you can call this just "struct devlink_msg" of maybe "fmsg" as
> for "formatted message" - so it is not confused with any other devlink
> netlink message.

Ack.

> 
> 
> 
>> +int devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx);
>> +int devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
>> +			  void *value, u16 value_len, u8 value_nla_type);
>> +int devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx,
>> +			 char *name);
>> +int devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
>> +				    char *name, void *value, u16 value_len,
>> +				    u8 value_nla_type);
>> +int devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
>> +				     char *name, void **value, u16 *value_len,
>> +				     u8 *value_nla_type, int array_size);
>> +int devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name);
>> +int devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name);
>> +
>> int devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
>> 				     int attrtype);
>> void devlink_health_buffer_nest_end(struct devlink_health_buffer *buffer);
>> @@ -903,6 +919,60 @@ devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
>> 	return 0;
>> }
>>
>> +static inline int
>> +devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int
>> +devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int
>> +devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
>> +		      void *value, u16 value_len, u8 value_nla_type)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int
>> +devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx,
>> +		     char *name)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int
>> +devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
>> +				char *name, void *value, u16 value_len,
>> +				u8 value_nla_type)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int
>> +devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
>> +				 char *name, void **value, u16 *value_len,
>> +				 u8 *value_nla_type, int array_size)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int
>> +devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int
>> +devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name)
>> +{
>> +	return 0;
>> +}
>> +
>> static inline int
>> devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
>> 				 int attrtype)
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index 6b26bb2ce4dc..68eeda1d0455 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -300,6 +300,14 @@ enum devlink_attr {
>> 	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE,	/* u8 */
>> 	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA,	/* dynamic */
>>
>> +	DEVLINK_ATTR_MSG_OBJECT,		/* nested */
>> +	DEVLINK_ATTR_MSG_OBJECT_PAIR,		/* nested */
>> +	DEVLINK_ATTR_MSG_OBJECT_NAME,		/* string */
>> +	DEVLINK_ATTR_MSG_OBJECT_VALUE,		/* nested */
>> +	DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY,	/* nested */
>> +	DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,	/* u8 */
>> +	DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,	/* dynamic */
>> +
>> 	DEVLINK_ATTR_HEALTH_REPORTER,			/* nested */
>> 	DEVLINK_ATTR_HEALTH_REPORTER_NAME,		/* string */
>> 	DEVLINK_ATTR_HEALTH_REPORTER_STATE,		/* u8 */
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 60248a53c0ad..57ca096849b3 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -4098,6 +4098,461 @@ devlink_health_buffer_snd(struct genl_info *info,
>> 	return err;
>> }
>>
>> +struct devlink_msg {
> 
> Let's call this "struct devlink_msg_item"

Ack.

> 
> 
>> +	struct list_head list;
>> +	int attrtype;
>> +	u8 nla_type;
>> +	u16 len;
>> +	int value[0];
>> +};
>> +
>> +struct devlink_msg_ctx {
>> +	struct list_head msg_list;
>> +	int max_nested_depth;
>> +	int curr_nest;
>> +};
>> +
>> +#define DEVLINK_MSG_MAX_SIZE (4096 - GENL_HDRLEN)
>> +
>> +static struct devlink_msg_ctx *devlink_msg_ctx_alloc(void)
>> +{
>> +	struct devlink_msg_ctx *msg_ctx;
>> +
>> +	msg_ctx = kzalloc(sizeof(*msg_ctx), GFP_KERNEL);
>> +	if (!msg_ctx)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	INIT_LIST_HEAD(&msg_ctx->msg_list);
>> +
>> +	return msg_ctx;
>> +}
>> +
>> +static void devlink_msg_ctx_free(struct devlink_msg_ctx *msg_ctx)
>> +{
>> +	struct devlink_msg *msg, *tm;
>> +
>> +	list_for_each_entry_safe(msg, tm, &msg_ctx->msg_list, list) {
>> +		list_del(&msg->list);
>> +		kfree(msg);
>> +	}
>> +	kfree(msg_ctx);
>> +}
>> +
>> +int devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype)
>> +{
>> +	struct devlink_msg *nest_msg;
>> +
>> +	if (attrtype != DEVLINK_ATTR_MSG_OBJECT &&
>> +	    attrtype != DEVLINK_ATTR_MSG_OBJECT_PAIR &&
>> +	    attrtype != DEVLINK_ATTR_MSG_OBJECT_VALUE &&
>> +	    attrtype != DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY)
>> +		return -EINVAL;
>> +
>> +	nest_msg = kzalloc(sizeof(*nest_msg), GFP_KERNEL);
>> +	if (!nest_msg)
>> +		return -ENOMEM;
>> +
>> +	nest_msg->attrtype = attrtype;
>> +	msg_ctx->curr_nest++;
>> +	msg_ctx->max_nested_depth = max(msg_ctx->max_nested_depth,
>> +					msg_ctx->curr_nest);
>> +	list_add_tail(&nest_msg->list, &msg_ctx->msg_list);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_msg_nest_start);
>> +
>> +int devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx)
>> +{
>> +	struct devlink_msg *nest_msg;
>> +
>> +	WARN_ON(!msg_ctx->curr_nest);
>> +	nest_msg = kzalloc(sizeof(*nest_msg), GFP_KERNEL);
>> +	if (!nest_msg)
>> +		return -ENOMEM;
>> +
>> +	msg_ctx->curr_nest--;
>> +	list_add_tail(&nest_msg->list, &msg_ctx->msg_list);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_msg_nest_end);
>> +
>> +int devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
>> +			  void *value, u16 value_len, u8 value_nla_type)
>> +{
>> +	struct devlink_msg *value_msg;
>> +
>> +	if (value_len > DEVLINK_MSG_MAX_SIZE)
>> +		return -EMSGSIZE;
>> +
>> +	if (value_nla_type != NLA_U8 &&
>> +	    value_nla_type != NLA_U32 &&
>> +	    value_nla_type != NLA_U64 &&
>> +	    value_nla_type != NLA_NUL_STRING &&
>> +	    value_nla_type != NLA_BINARY)
>> +		return -EINVAL;
>> +
>> +	value_msg = kzalloc(sizeof(*value_msg) + value_len, GFP_KERNEL);
> 
> Looks fine.
> 
> 
>> +	if (!value_msg)
>> +		return -ENOMEM;
>> +
>> +	value_msg->nla_type = value_nla_type;
>> +	value_msg->len = value_len;
>> +	value_msg->attrtype = DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA;
>> +	memcpy(&value_msg->value, value, value_len);
>> +	list_add_tail(&value_msg->list, &msg_ctx->msg_list);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_msg_put_value);
>> +
>> +int devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx, char *name)
>> +{
>> +	struct devlink_msg *name_msg;
>> +
>> +	if (strlen(name) + 1 > DEVLINK_MSG_MAX_SIZE)
>> +		return -EMSGSIZE;
>> +
>> +	name_msg = kzalloc(sizeof(*name_msg) + strlen(name) + 1, GFP_KERNEL);
>> +	if (!name_msg)
>> +		return -ENOMEM;
>> +
>> +	name_msg->nla_type = NLA_NUL_STRING;
>> +	name_msg->len = strlen(name) + 1;
>> +	name_msg->attrtype = DEVLINK_ATTR_MSG_OBJECT_NAME;
>> +	memcpy(&name_msg->value, name, name_msg->len);
>> +	list_add_tail(&name_msg->list, &msg_ctx->msg_list);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_msg_put_name);
>> +
>> +int devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
>> +				    char *name, void *value, u16 value_len,
>> +				    u8 value_nla_type)
>> +{
>> +	int err;
>> +
>> +	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_put_name(msg_ctx, name);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_put_value(msg_ctx, value, value_len, value_nla_type);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_nest_end(msg_ctx);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_nest_end(msg_ctx);
>> +	if (err)
>> +		return err;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_msg_put_name_value_pair);
>> +
>> +int devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
>> +				     char *name, void **value, u16 *value_len,
>> +				     u8 *value_nla_type, int array_size)
> 
> This is limitting the arrays to plain values. One should be able to nest
> objects inside. If fact, that is what you can to do with sqs:
> 
> object start
>    name sqs
>    array start
>      object start
>        index 0
>        xxx yyy
>      object end
>      object start
>        index 1
>        xxx yyy
>      object end
>    array end
> object end
> 
> So you need to have:
> devlink_msg_put_array_start()
> devlink_msg_put_array_end()

Sounds good, will do.

> 
> 
>> +{
>> +	int err, i;
>> +
>> +	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_put_name(msg_ctx, name);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_nest_start(msg_ctx,
>> +				     DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY);
>> +	if (err)
>> +		return err;
>> +
>> +	for (i = 0; i < array_size; i++) {
>> +		err = devlink_msg_nest_start(msg_ctx,
>> +					     DEVLINK_ATTR_MSG_OBJECT_VALUE);
>> +		if (err)
>> +			return err;
>> +
>> +		err = devlink_msg_put_value(msg_ctx, value[i],
>> +					    value_len[i], value_nla_type[i]);
>> +		if (err)
>> +			return err;
>> +		err = devlink_msg_nest_end(msg_ctx);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	err = devlink_msg_nest_end(msg_ctx);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_nest_end(msg_ctx);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_nest_end(msg_ctx);
>> +	if (err)
>> +		return err;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_msg_put_name_value_array);
>> +
>> +int devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name)
>> +{
>> +	int err;
>> +
>> +	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT);
>> +	if (err)
>> +		return err;
>> +
>> +	if (!name)
>> +		return 0;
>> +
>> +	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_put_name(msg_ctx, name);
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
>> +	if (err)
>> +		return err;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_msg_object_start);
>> +
>> +int devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name)
>> +{
>> +	int err;
>> +
>> +	if (!name)
>> +		goto end_object;
>> +
>> +	err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT_VALUE */
>> +	if (err)
>> +		return err;
>> +
>> +	err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT_PAIR */
>> +	if (err)
>> +		return err;
>> +
>> +end_object:
>> +	err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT */
>> +	if (err)
>> +		return err;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_msg_object_end);
>> +
>> +static int
>> +devlink_msg_fill_data(struct sk_buff *skb, struct devlink_msg *msg)
>> +{
>> +	int err;
>> +
>> +	switch (msg->nla_type) {
>> +	case NLA_U8:
>> +		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>> +				 *(u8 *)msg->value);
>> +		break;
>> +	case NLA_U32:
>> +		err = nla_put_u32(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>> +				  *(u32 *)msg->value);
>> +		break;
>> +	case NLA_U64:
>> +		err = nla_put_u64_64bit(skb,
>> +					DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>> +					*(u64 *)msg->value, DEVLINK_ATTR_PAD);
>> +		break;
>> +	case NLA_NUL_STRING:
>> +		err = nla_put_string(skb,
>> +				     DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>> +				     (char *)&msg->value);
>> +		break;
>> +	case NLA_BINARY:
>> +		err = nla_put(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>> +			      msg->len, (void *)&msg->value);
>> +		break;
>> +	default:
>> +		err = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static int
>> +devlink_msg_fill_type(struct sk_buff *skb, struct devlink_msg *msg)
>> +{
>> +	int err;
>> +
>> +	switch (msg->nla_type) {
>> +	case NLA_U8:
>> +		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>> +				 NLA_U8);
>> +		break;
>> +	case NLA_U32:
>> +		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>> +				 NLA_U32);
>> +		break;
>> +	case NLA_U64:
>> +		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>> +				 NLA_U64);
>> +		break;
>> +	case NLA_NUL_STRING:
>> +		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>> +				 NLA_NUL_STRING);
>> +		break;
>> +	case NLA_BINARY:
>> +		err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>> +				 NLA_BINARY);
>> +		break;
>> +	default:
>> +		err = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static int
>> +devlink_msg_prepare_skb(struct sk_buff *skb, struct devlink_msg_ctx *msg_ctx,
>> +			int *start)
>> +{
>> +	struct nlattr **nlattr_arr;
>> +	struct devlink_msg *msg;
>> +	int j = 0, i = 0;
>> +	int err;
>> +
>> +	nlattr_arr = kcalloc(msg_ctx->max_nested_depth,
>> +			     sizeof(*nlattr_arr), GFP_KERNEL);
>> +	if (!nlattr_arr)
>> +		return -EINVAL;
>> +
>> +	list_for_each_entry(msg, &msg_ctx->msg_list, list) {
>> +		if (j < *start) {
>> +			j++;
>> +			continue;
>> +		}
>> +
>> +		switch (msg->attrtype) {
>> +		case DEVLINK_ATTR_MSG_OBJECT:
>> +		case DEVLINK_ATTR_MSG_OBJECT_PAIR:
>> +		case DEVLINK_ATTR_MSG_OBJECT_VALUE:
>> +		case DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY:
>> +			nlattr_arr[i] = nla_nest_start(skb, msg->attrtype);
>> +			if (!nlattr_arr[i]) {
>> +				err = -EMSGSIZE;
>> +				goto nla_put_failure;
>> +			}
>> +			i++;
>> +			break;
>> +		case DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA:
>> +			err = devlink_msg_fill_data(skb, msg);
>> +			if (err)
>> +				goto nla_put_failure;
>> +			err = devlink_msg_fill_type(skb, msg);
>> +			if (err)
>> +				goto nla_put_failure;
>> +			break;
>> +		case DEVLINK_ATTR_MSG_OBJECT_NAME:
>> +			err = nla_put_string(skb, msg->attrtype,
>> +					     (char *)&msg->value);
>> +			if (err)
>> +				goto nla_put_failure;
>> +			break;
>> +		default: /* No attrtype */
>> +			nla_nest_end(skb, nlattr_arr[--i]);
>> +			break;
>> +		}
>> +		j++;
>> +		if (!i)
>> +			*start = j;
>> +	}
>> +
>> +	kfree(nlattr_arr);
>> +	return 0;
>> +
>> +nla_put_failure:
>> +	for (j = 0; j < i; j++)
>> +		nla_nest_cancel(skb, nlattr_arr[j]);
>> +	kfree(nlattr_arr);
>> +	return err;
>> +}
>> +
>> +static int devlink_msg_snd(struct genl_info *info,
>> +			   enum devlink_command cmd, int flags,
>> +			   struct devlink_msg_ctx *msg_ctx)
>> +{
>> +	struct sk_buff *skb;
>> +	struct nlmsghdr *nlh;
>> +	int err, index = 0;
>> +	bool last = false;
>> +	void *hdr;
>> +
>> +	while (!last) {
>> +		int tmp_index = index;
>> +
>> +		skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> +		if (!skb)
>> +			return -ENOMEM;
>> +
>> +		hdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
>> +				  &devlink_nl_family, NLM_F_MULTI, cmd);
>> +		if (!hdr)
>> +			goto nla_put_failure;
>> +
>> +		err = devlink_msg_prepare_skb(skb, msg_ctx, &index);
>> +		if (!err)
>> +			last = true;
>> +		else if (err != -EMSGSIZE || tmp_index == index)
>> +			goto nla_put_failure;
>> +
>> +		genlmsg_end(skb, hdr);
>> +		err = genlmsg_reply(skb, info);
>> +		if (err)
>> +			return err;
> 
> 
> Looks fine.
> 
> 
>> +	}
>> +
>> +	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> +	if (!skb)
>> +		return -ENOMEM;
>> +	nlh = nlmsg_put(skb, info->snd_portid, info->snd_seq,
>> +			NLMSG_DONE, 0, flags | NLM_F_MULTI);
>> +	err = genlmsg_reply(skb, info);
>> +	if (err)
>> +		return err;
>> +
>> +	return 0;
>> +
>> +nla_put_failure:
>> +	err = -EIO;
>> +	nlmsg_free(skb);
>> +	return err;
>> +}
>> +
>> struct devlink_health_reporter {
>> 	struct list_head list;
>> 	struct devlink_health_buffer **dump_buffers_array;
>> -- 
>> 2.17.1
>>

[PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg API

[Eran Ben Elisha]

As part of the devlink health reporter diagnose ops callback, the mlx5e TX
reporter used devlink health buffers API. Which will soon be depracated.
Modify the reporter to use the new devlink msg API.

The actual set of the new diagnose function will be done later in the
downstream patch, only when devlink will actually expose a new diagnose
operation that uses the new API.

Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
---
 .../mellanox/mlx5/core/en/reporter_tx.c       | 123 ++++--------------
 1 file changed, 26 insertions(+), 97 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
index d9675afbb924..fc92850c214a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
@@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter,
 }
 
 static int
-mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
+mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx,
 					u32 sqn, u8 state, u8 stopped)
 {
-	int err, i;
-	int nest = 0;
-	char name[20];
-
-	err = devlink_health_buffer_nest_start(buffer,
-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
-	if (err)
-		goto buffer_error;
-	nest++;
-
-	err = devlink_health_buffer_nest_start(buffer,
-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
-	if (err)
-		goto buffer_error;
-	nest++;
-
-	sprintf(name, "SQ 0x%x", sqn);
-	err = devlink_health_buffer_put_object_name(buffer, name);
-	if (err)
-		goto buffer_error;
-
-	err = devlink_health_buffer_nest_start(buffer,
-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
-	if (err)
-		goto buffer_error;
-	nest++;
-
-	err = devlink_health_buffer_nest_start(buffer,
-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
-	if (err)
-		goto buffer_error;
-	nest++;
-
-	err = devlink_health_buffer_nest_start(buffer,
-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
-	if (err)
-		goto buffer_error;
-	nest++;
-
-	err = devlink_health_buffer_put_object_name(buffer, "HW state");
-	if (err)
-		goto buffer_error;
+	int err;
 
-	err = devlink_health_buffer_nest_start(buffer,
-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
+	err = devlink_msg_object_start(msg_ctx, "SQ");
 	if (err)
-		goto buffer_error;
-	nest++;
+		return err;
 
-	err = devlink_health_buffer_put_value_u8(buffer, state);
+	err = devlink_msg_object_start(msg_ctx, NULL);
 	if (err)
-		goto buffer_error;
-
-	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */
-	nest--;
-
-	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */
-	nest--;
+		return err;
 
-	err = devlink_health_buffer_nest_start(buffer,
-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
+	err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn,
+					      sizeof(sqn), NLA_U32);
 	if (err)
-		goto buffer_error;
-	nest++;
+		return err;
 
-	err = devlink_health_buffer_put_object_name(buffer, "stopped");
+	err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state,
+					      sizeof(state), NLA_U8);
 	if (err)
-		goto buffer_error;
+		return err;
 
-	err = devlink_health_buffer_nest_start(buffer,
-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
+	err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped,
+					      sizeof(stopped), NLA_U8);
 	if (err)
-		goto buffer_error;
-	nest++;
+		return err;
 
-	err = devlink_health_buffer_put_value_u8(buffer, stopped);
+	err = devlink_msg_object_end(msg_ctx, NULL);
 	if (err)
-		goto buffer_error;
-
-	for (i = 0; i < nest; i++)
-		devlink_health_buffer_nest_end(buffer);
-
-	return 0;
+		return err;
 
-buffer_error:
-	for (i = 0; i < nest; i++)
-		devlink_health_buffer_nest_cancel(buffer);
+	err = devlink_msg_object_end(msg_ctx, "SQ");
 	return err;
 }
 
 static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
-				      struct devlink_health_buffer **buffers_array,
-				      unsigned int buffer_size,
-				      unsigned int num_buffers)
+				      struct devlink_msg_ctx *msg_ctx)
 {
 	struct mlx5e_priv *priv = devlink_health_reporter_priv(reporter);
-	unsigned int buff = 0;
-	int i = 0, err = 0;
-
-	if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN)
-		return -ENOMEM;
+	int i, err = 0;
 
 	mutex_lock(&priv->state_lock);
 
@@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
 		return 0;
 	}
 
-	while (i < priv->channels.num * priv->channels.params.num_tc) {
+	for (i = 0; i < priv->channels.num * priv->channels.params.num_tc;
+	     i++) {
 		struct mlx5e_txqsq *sq = priv->txq2sq[i];
 		u8 state;
 
@@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
 		if (err)
 			break;
 
-		err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff],
-							      sq->sqn, state,
+		err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn,
+							      state,
 							      netif_xmit_stopped(sq->txq));
-		if (err) {
-			if (++buff == num_buffers)
-				break;
-		} else {
-			i++;
-		}
+		if (err)
+			break;
 	}
 
 	mutex_unlock(&priv->state_lock);
@@ -330,11 +264,6 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
 static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
 		.name = "TX",
 		.recover = mlx5e_tx_reporter_recover,
-		.diagnose_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC *
-				 MLX5E_TX_REPORTER_PER_SQ_MAX_LEN,
-		.diagnose = mlx5e_tx_reporter_diagnose,
-		.dump_size = 0,
-		.dump = NULL,
 };
 
 #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
-- 
2.17.1

Re: [PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg API

[Jiri Pirko]

Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@mellanox.com wrote:
>As part of the devlink health reporter diagnose ops callback, the mlx5e TX
>reporter used devlink health buffers API. Which will soon be depracated.
>Modify the reporter to use the new devlink msg API.
>
>The actual set of the new diagnose function will be done later in the
>downstream patch, only when devlink will actually expose a new diagnose
>operation that uses the new API.
>
>Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> .../mellanox/mlx5/core/en/reporter_tx.c       | 123 ++++--------------
> 1 file changed, 26 insertions(+), 97 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>index d9675afbb924..fc92850c214a 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>@@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter,
> }
> 
> static int
>-mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
>+mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx,
> 					u32 sqn, u8 state, u8 stopped)

What is "state" and "stopped"? Is "stopped" a bool? Is "state" an enum.


> {
>-	int err, i;
>-	int nest = 0;
>-	char name[20];
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	sprintf(name, "SQ 0x%x", sqn);
>-	err = devlink_health_buffer_put_object_name(buffer, name);
>-	if (err)
>-		goto buffer_error;
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>-	if (err)
>-		goto buffer_error;
>-	nest++;
>-
>-	err = devlink_health_buffer_put_object_name(buffer, "HW state");
>-	if (err)
>-		goto buffer_error;
>+	int err;
> 
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>+	err = devlink_msg_object_start(msg_ctx, "SQ");
> 	if (err)
>-		goto buffer_error;
>-	nest++;
>+		return err;
> 
>-	err = devlink_health_buffer_put_value_u8(buffer, state);
>+	err = devlink_msg_object_start(msg_ctx, NULL);
> 	if (err)
>-		goto buffer_error;
>-
>-	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */
>-	nest--;
>-
>-	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */
>-	nest--;
>+		return err;
> 
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>+	err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn,
>+					      sizeof(sqn), NLA_U32);
> 	if (err)
>-		goto buffer_error;
>-	nest++;
>+		return err;
> 
>-	err = devlink_health_buffer_put_object_name(buffer, "stopped");
>+	err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state,
>+					      sizeof(state), NLA_U8);
> 	if (err)
>-		goto buffer_error;
>+		return err;
> 
>-	err = devlink_health_buffer_nest_start(buffer,
>-					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>+	err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped,
>+					      sizeof(stopped), NLA_U8);
> 	if (err)
>-		goto buffer_error;
>-	nest++;
>+		return err;
> 
>-	err = devlink_health_buffer_put_value_u8(buffer, stopped);
>+	err = devlink_msg_object_end(msg_ctx, NULL);
> 	if (err)
>-		goto buffer_error;
>-
>-	for (i = 0; i < nest; i++)
>-		devlink_health_buffer_nest_end(buffer);
>-
>-	return 0;
>+		return err;
> 
>-buffer_error:
>-	for (i = 0; i < nest; i++)
>-		devlink_health_buffer_nest_cancel(buffer);
>+	err = devlink_msg_object_end(msg_ctx, "SQ");
> 	return err;
> }
> 
> static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>-				      struct devlink_health_buffer **buffers_array,
>-				      unsigned int buffer_size,
>-				      unsigned int num_buffers)
>+				      struct devlink_msg_ctx *msg_ctx)
> {
> 	struct mlx5e_priv *priv = devlink_health_reporter_priv(reporter);
>-	unsigned int buff = 0;
>-	int i = 0, err = 0;
>-
>-	if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN)
>-		return -ENOMEM;
>+	int i, err = 0;
> 
> 	mutex_lock(&priv->state_lock);
> 
>@@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> 		return 0;
> 	}
> 
>-	while (i < priv->channels.num * priv->channels.params.num_tc) {
>+	for (i = 0; i < priv->channels.num * priv->channels.params.num_tc;
>+	     i++) {
> 		struct mlx5e_txqsq *sq = priv->txq2sq[i];
> 		u8 state;
> 
>@@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> 		if (err)
> 			break;
> 
>-		err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff],
>-							      sq->sqn, state,
>+		err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn,
>+							      state,
> 							      netif_xmit_stopped(sq->txq));

This should be an array. On SQ entry : one member of array.

So if you want to change it, you need to do this in 2 patches. One API,
one the actual layout. :/



>-		if (err) {
>-			if (++buff == num_buffers)
>-				break;
>-		} else {
>-			i++;
>-		}
>+		if (err)
>+			break;
> 	}
> 
> 	mutex_unlock(&priv->state_lock);
>@@ -330,11 +264,6 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
> 		.name = "TX",
> 		.recover = mlx5e_tx_reporter_recover,
>-		.diagnose_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC *
>-				 MLX5E_TX_REPORTER_PER_SQ_MAX_LEN,
>-		.diagnose = mlx5e_tx_reporter_diagnose,

So you change the callback, remove it so the dissection is broken.



>-		.dump_size = 0,
>-		.dump = NULL,


This has 0 relation to the patch. Should be separate.


> };
> 
> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>-- 
>2.17.1
>

Re: [PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg API

[Eran Ben Elisha]

On 1/23/2019 4:39 PM, Jiri Pirko wrote:
> Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@mellanox.com wrote:
>> As part of the devlink health reporter diagnose ops callback, the mlx5e TX
>> reporter used devlink health buffers API. Which will soon be depracated.
>> Modify the reporter to use the new devlink msg API.
>>
>> The actual set of the new diagnose function will be done later in the
>> downstream patch, only when devlink will actually expose a new diagnose
>> operation that uses the new API.
>>
>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>> ---
>> .../mellanox/mlx5/core/en/reporter_tx.c       | 123 ++++--------------
>> 1 file changed, 26 insertions(+), 97 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>> index d9675afbb924..fc92850c214a 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>> @@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter,
>> }
>>
>> static int
>> -mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
>> +mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx,
>> 					u32 sqn, u8 state, u8 stopped)
> 
> What is "state" and "stopped"? Is "stopped" a bool? Is "state" an enum.
> 
stopped is the reply from netif_xmit_stopped, and it is a bool.
state is the HW state of the SQ.
it is defined in the ifc file:
enum {
         MLX5_SQC_STATE_RST  = 0x0,
         MLX5_SQC_STATE_RDY  = 0x1,
         MLX5_SQC_STATE_ERR  = 0x3,
};
I could have translated it into strings, but I though it would be fine 
to leave it as is.

> 
>> {
>> -	int err, i;
>> -	int nest = 0;
>> -	char name[20];
>> -
>> -	err = devlink_health_buffer_nest_start(buffer,
>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>> -	if (err)
>> -		goto buffer_error;
>> -	nest++;
>> -
>> -	err = devlink_health_buffer_nest_start(buffer,
>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>> -	if (err)
>> -		goto buffer_error;
>> -	nest++;
>> -
>> -	sprintf(name, "SQ 0x%x", sqn);
>> -	err = devlink_health_buffer_put_object_name(buffer, name);
>> -	if (err)
>> -		goto buffer_error;
>> -
>> -	err = devlink_health_buffer_nest_start(buffer,
>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>> -	if (err)
>> -		goto buffer_error;
>> -	nest++;
>> -
>> -	err = devlink_health_buffer_nest_start(buffer,
>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>> -	if (err)
>> -		goto buffer_error;
>> -	nest++;
>> -
>> -	err = devlink_health_buffer_nest_start(buffer,
>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>> -	if (err)
>> -		goto buffer_error;
>> -	nest++;
>> -
>> -	err = devlink_health_buffer_put_object_name(buffer, "HW state");
>> -	if (err)
>> -		goto buffer_error;
>> +	int err;
>>
>> -	err = devlink_health_buffer_nest_start(buffer,
>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>> +	err = devlink_msg_object_start(msg_ctx, "SQ");
>> 	if (err)
>> -		goto buffer_error;
>> -	nest++;
>> +		return err;
>>
>> -	err = devlink_health_buffer_put_value_u8(buffer, state);
>> +	err = devlink_msg_object_start(msg_ctx, NULL);
>> 	if (err)
>> -		goto buffer_error;
>> -
>> -	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */
>> -	nest--;
>> -
>> -	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */
>> -	nest--;
>> +		return err;
>>
>> -	err = devlink_health_buffer_nest_start(buffer,
>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn,
>> +					      sizeof(sqn), NLA_U32);
>> 	if (err)
>> -		goto buffer_error;
>> -	nest++;
>> +		return err;
>>
>> -	err = devlink_health_buffer_put_object_name(buffer, "stopped");
>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state,
>> +					      sizeof(state), NLA_U8);
>> 	if (err)
>> -		goto buffer_error;
>> +		return err;
>>
>> -	err = devlink_health_buffer_nest_start(buffer,
>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped,
>> +					      sizeof(stopped), NLA_U8);
>> 	if (err)
>> -		goto buffer_error;
>> -	nest++;
>> +		return err;
>>
>> -	err = devlink_health_buffer_put_value_u8(buffer, stopped);
>> +	err = devlink_msg_object_end(msg_ctx, NULL);
>> 	if (err)
>> -		goto buffer_error;
>> -
>> -	for (i = 0; i < nest; i++)
>> -		devlink_health_buffer_nest_end(buffer);
>> -
>> -	return 0;
>> +		return err;
>>
>> -buffer_error:
>> -	for (i = 0; i < nest; i++)
>> -		devlink_health_buffer_nest_cancel(buffer);
>> +	err = devlink_msg_object_end(msg_ctx, "SQ");
>> 	return err;
>> }
>>
>> static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>> -				      struct devlink_health_buffer **buffers_array,
>> -				      unsigned int buffer_size,
>> -				      unsigned int num_buffers)
>> +				      struct devlink_msg_ctx *msg_ctx)
>> {
>> 	struct mlx5e_priv *priv = devlink_health_reporter_priv(reporter);
>> -	unsigned int buff = 0;
>> -	int i = 0, err = 0;
>> -
>> -	if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN)
>> -		return -ENOMEM;
>> +	int i, err = 0;
>>
>> 	mutex_lock(&priv->state_lock);
>>
>> @@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>> 		return 0;
>> 	}
>>
>> -	while (i < priv->channels.num * priv->channels.params.num_tc) {
>> +	for (i = 0; i < priv->channels.num * priv->channels.params.num_tc;
>> +	     i++) {
>> 		struct mlx5e_txqsq *sq = priv->txq2sq[i];
>> 		u8 state;
>>
>> @@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>> 		if (err)
>> 			break;
>>
>> -		err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff],
>> -							      sq->sqn, state,
>> +		err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn,
>> +							      state,
>> 							      netif_xmit_stopped(sq->txq));
> 
> This should be an array. On SQ entry : one member of array.
> 
> So if you want to change it, you need to do this in 2 patches. One API,
> one the actual layout. :/
> 
> 
> 
>> -		if (err) {
>> -			if (++buff == num_buffers)
>> -				break;
>> -		} else {
>> -			i++;
>> -		}
>> +		if (err)
>> +			break;
>> 	}
>>
>> 	mutex_unlock(&priv->state_lock);
>> @@ -330,11 +264,6 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>> 		.name = "TX",
>> 		.recover = mlx5e_tx_reporter_recover,
>> -		.diagnose_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC *
>> -				 MLX5E_TX_REPORTER_PER_SQ_MAX_LEN,
>> -		.diagnose = mlx5e_tx_reporter_diagnose,
> 
> So you change the callback, remove it so the dissection is broken.

This is needed in order to have this patch compiled.

> 
> 
> 
>> -		.dump_size = 0,
>> -		.dump = NULL,
> 
> 
> This has 0 relation to the patch. Should be separate.

ack.

> 
> 
>> };
>>
>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>> -- 
>> 2.17.1
>>

Re: [PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg API

[Jiri Pirko]

Thu, Jan 24, 2019 at 08:39:12AM CET, eranbe@mellanox.com wrote:
>
>
>On 1/23/2019 4:39 PM, Jiri Pirko wrote:
>> Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@mellanox.com wrote:
>>> As part of the devlink health reporter diagnose ops callback, the mlx5e TX
>>> reporter used devlink health buffers API. Which will soon be depracated.
>>> Modify the reporter to use the new devlink msg API.
>>>
>>> The actual set of the new diagnose function will be done later in the
>>> downstream patch, only when devlink will actually expose a new diagnose
>>> operation that uses the new API.
>>>
>>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>>> ---
>>> .../mellanox/mlx5/core/en/reporter_tx.c       | 123 ++++--------------
>>> 1 file changed, 26 insertions(+), 97 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> index d9675afbb924..fc92850c214a 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> @@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter,
>>> }
>>>
>>> static int
>>> -mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
>>> +mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx,
>>> 					u32 sqn, u8 state, u8 stopped)
>> 
>> What is "state" and "stopped"? Is "stopped" a bool? Is "state" an enum.
>> 
>stopped is the reply from netif_xmit_stopped, and it is a bool.

If it is bool, you should add it into message as NLA_FLAG, not NLA_U8


>state is the HW state of the SQ.
>it is defined in the ifc file:
>enum {
>         MLX5_SQC_STATE_RST  = 0x0,
>         MLX5_SQC_STATE_RDY  = 0x1,
>         MLX5_SQC_STATE_ERR  = 0x3,
>};
>I could have translated it into strings, but I though it would be fine 
>to leave it as is.

It's fine as u8

>
>> 
>>> {
>>> -	int err, i;
>>> -	int nest = 0;
>>> -	char name[20];
>>> -
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>>> -	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> -
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>> -	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> -
>>> -	sprintf(name, "SQ 0x%x", sqn);
>>> -	err = devlink_health_buffer_put_object_name(buffer, name);
>>> -	if (err)
>>> -		goto buffer_error;
>>> -
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>> -	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> -
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>>> -	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> -
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>> -	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> -
>>> -	err = devlink_health_buffer_put_object_name(buffer, "HW state");
>>> -	if (err)
>>> -		goto buffer_error;
>>> +	int err;
>>>
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>> +	err = devlink_msg_object_start(msg_ctx, "SQ");
>>> 	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_put_value_u8(buffer, state);
>>> +	err = devlink_msg_object_start(msg_ctx, NULL);
>>> 	if (err)
>>> -		goto buffer_error;
>>> -
>>> -	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */
>>> -	nest--;
>>> -
>>> -	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */
>>> -	nest--;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn,
>>> +					      sizeof(sqn), NLA_U32);
>>> 	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_put_object_name(buffer, "stopped");
>>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state,
>>> +					      sizeof(state), NLA_U8);
>>> 	if (err)
>>> -		goto buffer_error;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped,
>>> +					      sizeof(stopped), NLA_U8);
>>> 	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_put_value_u8(buffer, stopped);
>>> +	err = devlink_msg_object_end(msg_ctx, NULL);
>>> 	if (err)
>>> -		goto buffer_error;
>>> -
>>> -	for (i = 0; i < nest; i++)
>>> -		devlink_health_buffer_nest_end(buffer);
>>> -
>>> -	return 0;
>>> +		return err;
>>>
>>> -buffer_error:
>>> -	for (i = 0; i < nest; i++)
>>> -		devlink_health_buffer_nest_cancel(buffer);
>>> +	err = devlink_msg_object_end(msg_ctx, "SQ");
>>> 	return err;
>>> }
>>>
>>> static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> -				      struct devlink_health_buffer **buffers_array,
>>> -				      unsigned int buffer_size,
>>> -				      unsigned int num_buffers)
>>> +				      struct devlink_msg_ctx *msg_ctx)
>>> {
>>> 	struct mlx5e_priv *priv = devlink_health_reporter_priv(reporter);
>>> -	unsigned int buff = 0;
>>> -	int i = 0, err = 0;
>>> -
>>> -	if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN)
>>> -		return -ENOMEM;
>>> +	int i, err = 0;
>>>
>>> 	mutex_lock(&priv->state_lock);
>>>
>>> @@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> 		return 0;
>>> 	}
>>>
>>> -	while (i < priv->channels.num * priv->channels.params.num_tc) {
>>> +	for (i = 0; i < priv->channels.num * priv->channels.params.num_tc;
>>> +	     i++) {
>>> 		struct mlx5e_txqsq *sq = priv->txq2sq[i];
>>> 		u8 state;
>>>
>>> @@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> 		if (err)
>>> 			break;
>>>
>>> -		err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff],
>>> -							      sq->sqn, state,
>>> +		err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn,
>>> +							      state,
>>> 							      netif_xmit_stopped(sq->txq));
>> 
>> This should be an array. On SQ entry : one member of array.
>> 
>> So if you want to change it, you need to do this in 2 patches. One API,
>> one the actual layout. :/
>> 
>> 
>> 
>>> -		if (err) {
>>> -			if (++buff == num_buffers)
>>> -				break;
>>> -		} else {
>>> -			i++;
>>> -		}
>>> +		if (err)
>>> +			break;
>>> 	}
>>>
>>> 	mutex_unlock(&priv->state_lock);
>>> @@ -330,11 +264,6 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>>> 		.name = "TX",
>>> 		.recover = mlx5e_tx_reporter_recover,
>>> -		.diagnose_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC *
>>> -				 MLX5E_TX_REPORTER_PER_SQ_MAX_LEN,
>>> -		.diagnose = mlx5e_tx_reporter_diagnose,
>> 
>> So you change the callback, remove it so the dissection is broken.
>
>This is needed in order to have this patch compiled.

You have to figure it out without breakage. This is not the way to
do this.

>
>> 
>> 
>> 
>>> -		.dump_size = 0,
>>> -		.dump = NULL,
>> 
>> 
>> This has 0 relation to the patch. Should be separate.
>
>ack.
>
>> 
>> 
>>> };
>>>
>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>> -- 
>>> 2.17.1
>>>

Re: [PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg API

[Eran Ben Elisha]

On 1/24/2019 11:08 AM, Jiri Pirko wrote:
> Thu, Jan 24, 2019 at 08:39:12AM CET, eranbe@mellanox.com wrote:
>>
>>
>> On 1/23/2019 4:39 PM, Jiri Pirko wrote:
>>> Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@mellanox.com wrote:
>>>> As part of the devlink health reporter diagnose ops callback, the mlx5e TX
>>>> reporter used devlink health buffers API. Which will soon be depracated.
>>>> Modify the reporter to use the new devlink msg API.
>>>>
>>>> The actual set of the new diagnose function will be done later in the
>>>> downstream patch, only when devlink will actually expose a new diagnose
>>>> operation that uses the new API.
>>>>
>>>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>>>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>>>> ---
>>>> .../mellanox/mlx5/core/en/reporter_tx.c       | 123 ++++--------------
>>>> 1 file changed, 26 insertions(+), 97 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> index d9675afbb924..fc92850c214a 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> @@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter,
>>>> }
>>>>
>>>> static int
>>>> -mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
>>>> +mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx,
>>>> 					u32 sqn, u8 state, u8 stopped)
>>>
>>> What is "state" and "stopped"? Is "stopped" a bool? Is "state" an enum.
>>>
>> stopped is the reply from netif_xmit_stopped, and it is a bool.
> 
> If it is bool, you should add it into message as NLA_FLAG, not NLA_U8
> 
> 
>> state is the HW state of the SQ.
>> it is defined in the ifc file:
>> enum {
>>          MLX5_SQC_STATE_RST  = 0x0,
>>          MLX5_SQC_STATE_RDY  = 0x1,
>>          MLX5_SQC_STATE_ERR  = 0x3,
>> };
>> I could have translated it into strings, but I though it would be fine
>> to leave it as is.
> 
> It's fine as u8
> 
>>
>>>
>>>> {
>>>> -	int err, i;
>>>> -	int nest = 0;
>>>> -	char name[20];
>>>> -
>>>> -	err = devlink_health_buffer_nest_start(buffer,
>>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>>>> -	if (err)
>>>> -		goto buffer_error;
>>>> -	nest++;
>>>> -
>>>> -	err = devlink_health_buffer_nest_start(buffer,
>>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>>> -	if (err)
>>>> -		goto buffer_error;
>>>> -	nest++;
>>>> -
>>>> -	sprintf(name, "SQ 0x%x", sqn);
>>>> -	err = devlink_health_buffer_put_object_name(buffer, name);
>>>> -	if (err)
>>>> -		goto buffer_error;
>>>> -
>>>> -	err = devlink_health_buffer_nest_start(buffer,
>>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>>> -	if (err)
>>>> -		goto buffer_error;
>>>> -	nest++;
>>>> -
>>>> -	err = devlink_health_buffer_nest_start(buffer,
>>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>>>> -	if (err)
>>>> -		goto buffer_error;
>>>> -	nest++;
>>>> -
>>>> -	err = devlink_health_buffer_nest_start(buffer,
>>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>>> -	if (err)
>>>> -		goto buffer_error;
>>>> -	nest++;
>>>> -
>>>> -	err = devlink_health_buffer_put_object_name(buffer, "HW state");
>>>> -	if (err)
>>>> -		goto buffer_error;
>>>> +	int err;
>>>>
>>>> -	err = devlink_health_buffer_nest_start(buffer,
>>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>>> +	err = devlink_msg_object_start(msg_ctx, "SQ");
>>>> 	if (err)
>>>> -		goto buffer_error;
>>>> -	nest++;
>>>> +		return err;
>>>>
>>>> -	err = devlink_health_buffer_put_value_u8(buffer, state);
>>>> +	err = devlink_msg_object_start(msg_ctx, NULL);
>>>> 	if (err)
>>>> -		goto buffer_error;
>>>> -
>>>> -	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */
>>>> -	nest--;
>>>> -
>>>> -	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */
>>>> -	nest--;
>>>> +		return err;
>>>>
>>>> -	err = devlink_health_buffer_nest_start(buffer,
>>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn,
>>>> +					      sizeof(sqn), NLA_U32);
>>>> 	if (err)
>>>> -		goto buffer_error;
>>>> -	nest++;
>>>> +		return err;
>>>>
>>>> -	err = devlink_health_buffer_put_object_name(buffer, "stopped");
>>>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state,
>>>> +					      sizeof(state), NLA_U8);
>>>> 	if (err)
>>>> -		goto buffer_error;
>>>> +		return err;
>>>>
>>>> -	err = devlink_health_buffer_nest_start(buffer,
>>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped,
>>>> +					      sizeof(stopped), NLA_U8);
>>>> 	if (err)
>>>> -		goto buffer_error;
>>>> -	nest++;
>>>> +		return err;
>>>>
>>>> -	err = devlink_health_buffer_put_value_u8(buffer, stopped);
>>>> +	err = devlink_msg_object_end(msg_ctx, NULL);
>>>> 	if (err)
>>>> -		goto buffer_error;
>>>> -
>>>> -	for (i = 0; i < nest; i++)
>>>> -		devlink_health_buffer_nest_end(buffer);
>>>> -
>>>> -	return 0;
>>>> +		return err;
>>>>
>>>> -buffer_error:
>>>> -	for (i = 0; i < nest; i++)
>>>> -		devlink_health_buffer_nest_cancel(buffer);
>>>> +	err = devlink_msg_object_end(msg_ctx, "SQ");
>>>> 	return err;
>>>> }
>>>>
>>>> static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> -				      struct devlink_health_buffer **buffers_array,
>>>> -				      unsigned int buffer_size,
>>>> -				      unsigned int num_buffers)
>>>> +				      struct devlink_msg_ctx *msg_ctx)
>>>> {
>>>> 	struct mlx5e_priv *priv = devlink_health_reporter_priv(reporter);
>>>> -	unsigned int buff = 0;
>>>> -	int i = 0, err = 0;
>>>> -
>>>> -	if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN)
>>>> -		return -ENOMEM;
>>>> +	int i, err = 0;
>>>>
>>>> 	mutex_lock(&priv->state_lock);
>>>>
>>>> @@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> 		return 0;
>>>> 	}
>>>>
>>>> -	while (i < priv->channels.num * priv->channels.params.num_tc) {
>>>> +	for (i = 0; i < priv->channels.num * priv->channels.params.num_tc;
>>>> +	     i++) {
>>>> 		struct mlx5e_txqsq *sq = priv->txq2sq[i];
>>>> 		u8 state;
>>>>
>>>> @@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> 		if (err)
>>>> 			break;
>>>>
>>>> -		err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff],
>>>> -							      sq->sqn, state,
>>>> +		err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn,
>>>> +							      state,
>>>> 							      netif_xmit_stopped(sq->txq));
>>>
>>> This should be an array. On SQ entry : one member of array.
>>>
>>> So if you want to change it, you need to do this in 2 patches. One API,
>>> one the actual layout. :/
>>>
>>>
>>>
>>>> -		if (err) {
>>>> -			if (++buff == num_buffers)
>>>> -				break;
>>>> -		} else {
>>>> -			i++;
>>>> -		}
>>>> +		if (err)
>>>> +			break;
>>>> 	}
>>>>
>>>> 	mutex_unlock(&priv->state_lock);
>>>> @@ -330,11 +264,6 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>>>> 		.name = "TX",
>>>> 		.recover = mlx5e_tx_reporter_recover,
>>>> -		.diagnose_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC *
>>>> -				 MLX5E_TX_REPORTER_PER_SQ_MAX_LEN,
>>>> -		.diagnose = mlx5e_tx_reporter_diagnose,
>>>
>>> So you change the callback, remove it so the dissection is broken.
>>
>> This is needed in order to have this patch compiled.
> 
> You have to figure it out without breakage. This is not the way to
> do this.

This would cause all devlink and mlx5e patches to be altogether when 
moving to the new API, which I don't like at all.

(#2) I think we shall take your approach and do a revert and put it all 
again on top.

> 
>>
>>>
>>>
>>>
>>>> -		.dump_size = 0,
>>>> -		.dump = NULL,
>>>
>>>
>>> This has 0 relation to the patch. Should be separate.
>>
>> ack.
>>
>>>
>>>
>>>> };
>>>>
>>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>>> -- 
>>>> 2.17.1
>>>>

[PATCH net-next 3/7] devlink: move devlink health reporter to use devlink msg API

[Eran Ben Elisha]

Move devlink reporter diagnose and dump operations to use the new msg API.
Redefine the signature of diagnose and dump operations and move the mlx5e
reporter to use it with the new format.

Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
---
 .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
 include/net/devlink.h                         |  9 +-
 net/core/devlink.c                            | 95 +++++--------------
 3 files changed, 28 insertions(+), 77 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
index fc92850c214a..7238cda670ba 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
@@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
 static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
 		.name = "TX",
 		.recover = mlx5e_tx_reporter_recover,
+		.diagnose = mlx5e_tx_reporter_diagnose,
 };
 
 #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
diff --git a/include/net/devlink.h b/include/net/devlink.h
index fe323e9b14e1..d66de8b80cc2 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -442,17 +442,12 @@ struct devlink_health_reporter;
 
 struct devlink_health_reporter_ops {
 	char *name;
-	unsigned int dump_size;
-	unsigned int diagnose_size;
 	int (*recover)(struct devlink_health_reporter *reporter,
 		       void *priv_ctx);
 	int (*dump)(struct devlink_health_reporter *reporter,
-		    struct devlink_health_buffer **buffers_array,
-		    unsigned int buffer_size, unsigned int num_buffers,
-		    void *priv_ctx);
+		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
 	int (*diagnose)(struct devlink_health_reporter *reporter,
-			struct devlink_health_buffer **buffers_array,
-			unsigned int buffer_size, unsigned int num_buffers);
+			struct devlink_msg_ctx *msg_ctx);
 };
 
 struct devlink_ops {
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 57ca096849b3..347b638e6f32 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
 
 struct devlink_health_reporter {
 	struct list_head list;
-	struct devlink_health_buffer **dump_buffers_array;
+	struct devlink_msg_ctx *dump_msg_ctx;
 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
-	struct devlink_health_buffer **diagnose_buffers_array;
-	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
 	void *priv;
 	const struct devlink_health_reporter_ops *ops;
 	struct devlink *devlink;
@@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
 		goto unlock;
 	}
 
-	if (WARN_ON(ops->dump && !ops->dump_size) ||
-	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
-	    WARN_ON(auto_recover && !ops->recover) ||
+	if (WARN_ON(auto_recover && !ops->recover) ||
 	    WARN_ON(graceful_period && !ops->recover)) {
 		reporter = ERR_PTR(-EINVAL);
 		goto unlock;
@@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
 		goto unlock;
 	}
 
-	if (ops->dump) {
-		reporter->dump_buffers_array =
-			devlink_health_buffers_create(ops->dump_size);
-		if (!reporter->dump_buffers_array) {
-			kfree(reporter);
-			reporter = ERR_PTR(-ENOMEM);
-			goto unlock;
-		}
-	}
-
-	if (ops->diagnose) {
-		reporter->diagnose_buffers_array =
-			devlink_health_buffers_create(ops->diagnose_size);
-		if (!reporter->diagnose_buffers_array) {
-			devlink_health_buffers_destroy(reporter->dump_buffers_array,
-						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
-			kfree(reporter);
-			reporter = ERR_PTR(-ENOMEM);
-			goto unlock;
-		}
-	}
-
 	list_add_tail(&reporter->list, &devlink->reporter_list);
 	mutex_init(&reporter->dump_lock);
-	mutex_init(&reporter->diagnose_lock);
 
 	reporter->priv = priv;
 	reporter->ops = ops;
@@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
 {
 	mutex_lock(&reporter->devlink->lock);
 	list_del(&reporter->list);
-	devlink_health_buffers_destroy(reporter->dump_buffers_array,
-				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
-	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
-				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
+	if (reporter->dump_msg_ctx)
+		devlink_msg_ctx_free(reporter->dump_msg_ctx);
 	kfree(reporter);
 	mutex_unlock(&reporter->devlink->lock);
 }
@@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
 	if (reporter->dump_avail)
 		return 0;
 
-	devlink_health_buffers_reset(reporter->dump_buffers_array,
-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
-	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
-				     DEVLINK_HEALTH_BUFFER_SIZE,
-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
-				     priv_ctx);
+	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
+	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
+		err = PTR_ERR(reporter->dump_msg_ctx);
+		reporter->dump_msg_ctx = NULL;
+		return err;
+	}
+
+	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
+				  priv_ctx);
 	if (!err) {
 		reporter->dump_avail = true;
 		reporter->dump_ts = jiffies;
@@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_health_reporter *reporter;
-	u64 num_of_buffers;
+	struct devlink_msg_ctx *msg_ctx;
 	int err;
 
 	reporter = devlink_health_reporter_get_from_info(devlink, info);
@@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
 	if (!reporter->ops->diagnose)
 		return -EOPNOTSUPP;
 
-	num_of_buffers =
-		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
+	msg_ctx = devlink_msg_ctx_alloc();
+	if (IS_ERR_OR_NULL(msg_ctx))
+		return PTR_ERR(msg_ctx);
 
-	mutex_lock(&reporter->diagnose_lock);
-	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
-				     num_of_buffers);
-
-	err = reporter->ops->diagnose(reporter,
-				      reporter->diagnose_buffers_array,
-				      DEVLINK_HEALTH_BUFFER_SIZE,
-				      num_of_buffers);
+	err = reporter->ops->diagnose(reporter, msg_ctx);
 	if (err)
 		goto out;
 
-	err = devlink_health_buffer_snd(info,
-					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
-					0, reporter->diagnose_buffers_array,
-					num_of_buffers);
-	if (err)
-		goto out;
-
-	mutex_unlock(&reporter->diagnose_lock);
-	return 0;
+	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
+			      0, msg_ctx);
 
 out:
-	mutex_unlock(&reporter->diagnose_lock);
+	devlink_msg_ctx_free(msg_ctx);
 	return err;
 }
 
@@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
 {
 	reporter->dump_avail = false;
 	reporter->dump_ts = 0;
-	devlink_health_buffers_reset(reporter->dump_buffers_array,
-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
+	devlink_msg_ctx_free(reporter->dump_msg_ctx);
+	reporter->dump_msg_ctx = NULL;
 }
 
 static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
@@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_health_reporter *reporter;
-	u64 num_of_buffers;
 	int err;
 
 	reporter = devlink_health_reporter_get_from_info(devlink, info);
@@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
 	if (!reporter->ops->dump)
 		return -EOPNOTSUPP;
 
-	num_of_buffers =
-		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
-
 	mutex_lock(&reporter->dump_lock);
 	err = devlink_health_do_dump(reporter, NULL);
 	if (err)
 		goto out;
 
-	err = devlink_health_buffer_snd(info,
-					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
-					0, reporter->dump_buffers_array,
-					num_of_buffers);
+	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
+			      0, reporter->dump_msg_ctx);
 
 out:
 	mutex_unlock(&reporter->dump_lock);
-- 
2.17.1

Re: [PATCH net-next 3/7] devlink: move devlink health reporter to use devlink msg API

[Jiri Pirko]

Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>Move devlink reporter diagnose and dump operations to use the new msg API.
>Redefine the signature of diagnose and dump operations and move the mlx5e
>reporter to use it with the new format.
>
>Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
> include/net/devlink.h                         |  9 +-
> net/core/devlink.c                            | 95 +++++--------------
> 3 files changed, 28 insertions(+), 77 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>index fc92850c214a..7238cda670ba 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>@@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
> 		.name = "TX",
> 		.recover = mlx5e_tx_reporter_recover,
>+		.diagnose = mlx5e_tx_reporter_diagnose,

Unrelated to this patch.


> };
> 
> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index fe323e9b14e1..d66de8b80cc2 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -442,17 +442,12 @@ struct devlink_health_reporter;
> 
> struct devlink_health_reporter_ops {
> 	char *name;
>-	unsigned int dump_size;
>-	unsigned int diagnose_size;
> 	int (*recover)(struct devlink_health_reporter *reporter,
> 		       void *priv_ctx);
> 	int (*dump)(struct devlink_health_reporter *reporter,
>-		    struct devlink_health_buffer **buffers_array,
>-		    unsigned int buffer_size, unsigned int num_buffers,
>-		    void *priv_ctx);
>+		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
> 	int (*diagnose)(struct devlink_health_reporter *reporter,
>-			struct devlink_health_buffer **buffers_array,
>-			unsigned int buffer_size, unsigned int num_buffers);
>+			struct devlink_msg_ctx *msg_ctx);
> };
> 
> struct devlink_ops {
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 57ca096849b3..347b638e6f32 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
> 
> struct devlink_health_reporter {
> 	struct list_head list;
>-	struct devlink_health_buffer **dump_buffers_array;
>+	struct devlink_msg_ctx *dump_msg_ctx;
> 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>-	struct devlink_health_buffer **diagnose_buffers_array;
>-	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */

Howcome you don't need the mutex anymore?


> 	void *priv;
> 	const struct devlink_health_reporter_ops *ops;
> 	struct devlink *devlink;
>@@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
> 		goto unlock;
> 	}
> 
>-	if (WARN_ON(ops->dump && !ops->dump_size) ||
>-	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>-	    WARN_ON(auto_recover && !ops->recover) ||
>+	if (WARN_ON(auto_recover && !ops->recover) ||
> 	    WARN_ON(graceful_period && !ops->recover)) {
> 		reporter = ERR_PTR(-EINVAL);
> 		goto unlock;
>@@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
> 		goto unlock;
> 	}
> 
>-	if (ops->dump) {
>-		reporter->dump_buffers_array =
>-			devlink_health_buffers_create(ops->dump_size);
>-		if (!reporter->dump_buffers_array) {
>-			kfree(reporter);
>-			reporter = ERR_PTR(-ENOMEM);
>-			goto unlock;
>-		}
>-	}
>-
>-	if (ops->diagnose) {
>-		reporter->diagnose_buffers_array =
>-			devlink_health_buffers_create(ops->diagnose_size);
>-		if (!reporter->diagnose_buffers_array) {
>-			devlink_health_buffers_destroy(reporter->dump_buffers_array,
>-						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>-			kfree(reporter);
>-			reporter = ERR_PTR(-ENOMEM);
>-			goto unlock;
>-		}
>-	}
>-
> 	list_add_tail(&reporter->list, &devlink->reporter_list);
> 	mutex_init(&reporter->dump_lock);
>-	mutex_init(&reporter->diagnose_lock);
> 
> 	reporter->priv = priv;
> 	reporter->ops = ops;
>@@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
> {
> 	mutex_lock(&reporter->devlink->lock);
> 	list_del(&reporter->list);
>-	devlink_health_buffers_destroy(reporter->dump_buffers_array,
>-				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>-	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>-				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>+	if (reporter->dump_msg_ctx)
>+		devlink_msg_ctx_free(reporter->dump_msg_ctx);
> 	kfree(reporter);
> 	mutex_unlock(&reporter->devlink->lock);
> }
>@@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
> 	if (reporter->dump_avail)
> 		return 0;
> 
>-	devlink_health_buffers_reset(reporter->dump_buffers_array,
>-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>-	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>-				     DEVLINK_HEALTH_BUFFER_SIZE,
>-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>-				     priv_ctx);
>+	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>+	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>+		err = PTR_ERR(reporter->dump_msg_ctx);
>+		reporter->dump_msg_ctx = NULL;
>+		return err;
>+	}
>+
>+	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>+				  priv_ctx);
> 	if (!err) {
> 		reporter->dump_avail = true;
> 		reporter->dump_ts = jiffies;
>@@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
> {
> 	struct devlink *devlink = info->user_ptr[0];
> 	struct devlink_health_reporter *reporter;
>-	u64 num_of_buffers;
>+	struct devlink_msg_ctx *msg_ctx;
> 	int err;
> 
> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>@@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
> 	if (!reporter->ops->diagnose)
> 		return -EOPNOTSUPP;
> 
>-	num_of_buffers =
>-		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>+	msg_ctx = devlink_msg_ctx_alloc();
>+	if (IS_ERR_OR_NULL(msg_ctx))
>+		return PTR_ERR(msg_ctx);
> 
>-	mutex_lock(&reporter->diagnose_lock);
>-	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>-				     num_of_buffers);
>-
>-	err = reporter->ops->diagnose(reporter,
>-				      reporter->diagnose_buffers_array,
>-				      DEVLINK_HEALTH_BUFFER_SIZE,
>-				      num_of_buffers);
>+	err = reporter->ops->diagnose(reporter, msg_ctx);

So this is not needed to be in reporter now? Why it was needed before?



> 	if (err)
> 		goto out;
> 
>-	err = devlink_health_buffer_snd(info,
>-					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>-					0, reporter->diagnose_buffers_array,
>-					num_of_buffers);
>-	if (err)
>-		goto out;
>-
>-	mutex_unlock(&reporter->diagnose_lock);
>-	return 0;
>+	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>+			      0, msg_ctx);
> 
> out:
>-	mutex_unlock(&reporter->diagnose_lock);
>+	devlink_msg_ctx_free(msg_ctx);
> 	return err;
> }
> 
>@@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
> {
> 	reporter->dump_avail = false;
> 	reporter->dump_ts = 0;
>-	devlink_health_buffers_reset(reporter->dump_buffers_array,
>-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>+	devlink_msg_ctx_free(reporter->dump_msg_ctx);
>+	reporter->dump_msg_ctx = NULL;
> }
> 
> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>@@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
> {
> 	struct devlink *devlink = info->user_ptr[0];
> 	struct devlink_health_reporter *reporter;
>-	u64 num_of_buffers;
> 	int err;
> 
> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>@@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
> 	if (!reporter->ops->dump)
> 		return -EOPNOTSUPP;
> 
>-	num_of_buffers =
>-		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>-
> 	mutex_lock(&reporter->dump_lock);
> 	err = devlink_health_do_dump(reporter, NULL);
> 	if (err)
> 		goto out;
> 
>-	err = devlink_health_buffer_snd(info,
>-					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>-					0, reporter->dump_buffers_array,
>-					num_of_buffers);
>+	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>+			      0, reporter->dump_msg_ctx);
> 
> out:
> 	mutex_unlock(&reporter->dump_lock);
>-- 
>2.17.1
>

Re: [PATCH net-next 3/7] devlink: move devlink health reporter to use devlink msg API

[Eran Ben Elisha]

On 1/23/2019 4:42 PM, Jiri Pirko wrote:
> Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>> Move devlink reporter diagnose and dump operations to use the new msg API.
>> Redefine the signature of diagnose and dump operations and move the mlx5e
>> reporter to use it with the new format.
>>
>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>> ---
>> .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
>> include/net/devlink.h                         |  9 +-
>> net/core/devlink.c                            | 95 +++++--------------
>> 3 files changed, 28 insertions(+), 77 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>> index fc92850c214a..7238cda670ba 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>> @@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>> 		.name = "TX",
>> 		.recover = mlx5e_tx_reporter_recover,
>> +		.diagnose = mlx5e_tx_reporter_diagnose,
> 
> Unrelated to this patch.

ack.

> 
> 
>> };
>>
>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index fe323e9b14e1..d66de8b80cc2 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -442,17 +442,12 @@ struct devlink_health_reporter;
>>
>> struct devlink_health_reporter_ops {
>> 	char *name;
>> -	unsigned int dump_size;
>> -	unsigned int diagnose_size;
>> 	int (*recover)(struct devlink_health_reporter *reporter,
>> 		       void *priv_ctx);
>> 	int (*dump)(struct devlink_health_reporter *reporter,
>> -		    struct devlink_health_buffer **buffers_array,
>> -		    unsigned int buffer_size, unsigned int num_buffers,
>> -		    void *priv_ctx);
>> +		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
>> 	int (*diagnose)(struct devlink_health_reporter *reporter,
>> -			struct devlink_health_buffer **buffers_array,
>> -			unsigned int buffer_size, unsigned int num_buffers);
>> +			struct devlink_msg_ctx *msg_ctx);
>> };
>>
>> struct devlink_ops {
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 57ca096849b3..347b638e6f32 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
>>
>> struct devlink_health_reporter {
>> 	struct list_head list;
>> -	struct devlink_health_buffer **dump_buffers_array;
>> +	struct devlink_msg_ctx *dump_msg_ctx;
>> 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>> -	struct devlink_health_buffer **diagnose_buffers_array;
>> -	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
> 
> Howcome you don't need the mutex anymore?

Now, as data is allocated on the fly while diagnose_doit(), no need to 
store the diagnose  over the reporter anymore. So no need for any mutex 
locking in order to prepare and send it.

> 
> 
>> 	void *priv;
>> 	const struct devlink_health_reporter_ops *ops;
>> 	struct devlink *devlink;
>> @@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
>> 		goto unlock;
>> 	}
>>
>> -	if (WARN_ON(ops->dump && !ops->dump_size) ||
>> -	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>> -	    WARN_ON(auto_recover && !ops->recover) ||
>> +	if (WARN_ON(auto_recover && !ops->recover) ||
>> 	    WARN_ON(graceful_period && !ops->recover)) {
>> 		reporter = ERR_PTR(-EINVAL);
>> 		goto unlock;
>> @@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
>> 		goto unlock;
>> 	}
>>
>> -	if (ops->dump) {
>> -		reporter->dump_buffers_array =
>> -			devlink_health_buffers_create(ops->dump_size);
>> -		if (!reporter->dump_buffers_array) {
>> -			kfree(reporter);
>> -			reporter = ERR_PTR(-ENOMEM);
>> -			goto unlock;
>> -		}
>> -	}
>> -
>> -	if (ops->diagnose) {
>> -		reporter->diagnose_buffers_array =
>> -			devlink_health_buffers_create(ops->diagnose_size);
>> -		if (!reporter->diagnose_buffers_array) {
>> -			devlink_health_buffers_destroy(reporter->dump_buffers_array,
>> -						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>> -			kfree(reporter);
>> -			reporter = ERR_PTR(-ENOMEM);
>> -			goto unlock;
>> -		}
>> -	}
>> -
>> 	list_add_tail(&reporter->list, &devlink->reporter_list);
>> 	mutex_init(&reporter->dump_lock);
>> -	mutex_init(&reporter->diagnose_lock);
>>
>> 	reporter->priv = priv;
>> 	reporter->ops = ops;
>> @@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
>> {
>> 	mutex_lock(&reporter->devlink->lock);
>> 	list_del(&reporter->list);
>> -	devlink_health_buffers_destroy(reporter->dump_buffers_array,
>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>> -	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>> +	if (reporter->dump_msg_ctx)
>> +		devlink_msg_ctx_free(reporter->dump_msg_ctx);
>> 	kfree(reporter);
>> 	mutex_unlock(&reporter->devlink->lock);
>> }
>> @@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
>> 	if (reporter->dump_avail)
>> 		return 0;
>>
>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>> -	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>> -				     DEVLINK_HEALTH_BUFFER_SIZE,
>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>> -				     priv_ctx);
>> +	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>> +	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>> +		err = PTR_ERR(reporter->dump_msg_ctx);
>> +		reporter->dump_msg_ctx = NULL;
>> +		return err;
>> +	}
>> +
>> +	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>> +				  priv_ctx);
>> 	if (!err) {
>> 		reporter->dump_avail = true;
>> 		reporter->dump_ts = jiffies;
>> @@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>> {
>> 	struct devlink *devlink = info->user_ptr[0];
>> 	struct devlink_health_reporter *reporter;
>> -	u64 num_of_buffers;
>> +	struct devlink_msg_ctx *msg_ctx;
>> 	int err;
>>
>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>> @@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>> 	if (!reporter->ops->diagnose)
>> 		return -EOPNOTSUPP;
>>
>> -	num_of_buffers =
>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>> +	msg_ctx = devlink_msg_ctx_alloc();
>> +	if (IS_ERR_OR_NULL(msg_ctx))
>> +		return PTR_ERR(msg_ctx);
>>
>> -	mutex_lock(&reporter->diagnose_lock);
>> -	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>> -				     num_of_buffers);
>> -
>> -	err = reporter->ops->diagnose(reporter,
>> -				      reporter->diagnose_buffers_array,
>> -				      DEVLINK_HEALTH_BUFFER_SIZE,
>> -				      num_of_buffers);
>> +	err = reporter->ops->diagnose(reporter, msg_ctx);
> 
> So this is not needed to be in reporter now? Why it was needed before?

see reply above.

> 
> 
> 
>> 	if (err)
>> 		goto out;
>>
>> -	err = devlink_health_buffer_snd(info,
>> -					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>> -					0, reporter->diagnose_buffers_array,
>> -					num_of_buffers);
>> -	if (err)
>> -		goto out;
>> -
>> -	mutex_unlock(&reporter->diagnose_lock);
>> -	return 0;
>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>> +			      0, msg_ctx);
>>
>> out:
>> -	mutex_unlock(&reporter->diagnose_lock);
>> +	devlink_msg_ctx_free(msg_ctx);
>> 	return err;
>> }
>>
>> @@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
>> {
>> 	reporter->dump_avail = false;
>> 	reporter->dump_ts = 0;
>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>> +	devlink_msg_ctx_free(reporter->dump_msg_ctx);
>> +	reporter->dump_msg_ctx = NULL;
>> }
>>
>> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>> @@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>> {
>> 	struct devlink *devlink = info->user_ptr[0];
>> 	struct devlink_health_reporter *reporter;
>> -	u64 num_of_buffers;
>> 	int err;
>>
>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>> @@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>> 	if (!reporter->ops->dump)
>> 		return -EOPNOTSUPP;
>>
>> -	num_of_buffers =
>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>> -
>> 	mutex_lock(&reporter->dump_lock);
>> 	err = devlink_health_do_dump(reporter, NULL);
>> 	if (err)
>> 		goto out;
>>
>> -	err = devlink_health_buffer_snd(info,
>> -					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>> -					0, reporter->dump_buffers_array,
>> -					num_of_buffers);
>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>> +			      0, reporter->dump_msg_ctx);
>>
>> out:
>> 	mutex_unlock(&reporter->dump_lock);
>> -- 
>> 2.17.1
>>

Re: [PATCH net-next 3/7] devlink: move devlink health reporter to use devlink msg API

[Jiri Pirko]

Thu, Jan 24, 2019 at 08:45:05AM CET, eranbe@mellanox.com wrote:
>
>
>On 1/23/2019 4:42 PM, Jiri Pirko wrote:
>> Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>>> Move devlink reporter diagnose and dump operations to use the new msg API.
>>> Redefine the signature of diagnose and dump operations and move the mlx5e
>>> reporter to use it with the new format.
>>>
>>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>>> ---
>>> .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
>>> include/net/devlink.h                         |  9 +-
>>> net/core/devlink.c                            | 95 +++++--------------
>>> 3 files changed, 28 insertions(+), 77 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> index fc92850c214a..7238cda670ba 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> @@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>>> 		.name = "TX",
>>> 		.recover = mlx5e_tx_reporter_recover,
>>> +		.diagnose = mlx5e_tx_reporter_diagnose,
>> 
>> Unrelated to this patch.
>
>ack.
>
>> 
>> 
>>> };
>>>
>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>> index fe323e9b14e1..d66de8b80cc2 100644
>>> --- a/include/net/devlink.h
>>> +++ b/include/net/devlink.h
>>> @@ -442,17 +442,12 @@ struct devlink_health_reporter;
>>>
>>> struct devlink_health_reporter_ops {
>>> 	char *name;
>>> -	unsigned int dump_size;
>>> -	unsigned int diagnose_size;
>>> 	int (*recover)(struct devlink_health_reporter *reporter,
>>> 		       void *priv_ctx);
>>> 	int (*dump)(struct devlink_health_reporter *reporter,
>>> -		    struct devlink_health_buffer **buffers_array,
>>> -		    unsigned int buffer_size, unsigned int num_buffers,
>>> -		    void *priv_ctx);
>>> +		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
>>> 	int (*diagnose)(struct devlink_health_reporter *reporter,
>>> -			struct devlink_health_buffer **buffers_array,
>>> -			unsigned int buffer_size, unsigned int num_buffers);
>>> +			struct devlink_msg_ctx *msg_ctx);
>>> };
>>>
>>> struct devlink_ops {
>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>> index 57ca096849b3..347b638e6f32 100644
>>> --- a/net/core/devlink.c
>>> +++ b/net/core/devlink.c
>>> @@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
>>>
>>> struct devlink_health_reporter {
>>> 	struct list_head list;
>>> -	struct devlink_health_buffer **dump_buffers_array;
>>> +	struct devlink_msg_ctx *dump_msg_ctx;
>>> 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>>> -	struct devlink_health_buffer **diagnose_buffers_array;
>>> -	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
>> 
>> Howcome you don't need the mutex anymore?
>
>Now, as data is allocated on the fly while diagnose_doit(), no need to 
>store the diagnose  over the reporter anymore. So no need for any mutex 
>locking in order to prepare and send it.

Why the data is allocated on the fly? Shouldn't it be a separate patch
as it looks like a separate change?


>
>> 
>> 
>>> 	void *priv;
>>> 	const struct devlink_health_reporter_ops *ops;
>>> 	struct devlink *devlink;
>>> @@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
>>> 		goto unlock;
>>> 	}
>>>
>>> -	if (WARN_ON(ops->dump && !ops->dump_size) ||
>>> -	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>>> -	    WARN_ON(auto_recover && !ops->recover) ||
>>> +	if (WARN_ON(auto_recover && !ops->recover) ||
>>> 	    WARN_ON(graceful_period && !ops->recover)) {
>>> 		reporter = ERR_PTR(-EINVAL);
>>> 		goto unlock;
>>> @@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
>>> 		goto unlock;
>>> 	}
>>>
>>> -	if (ops->dump) {
>>> -		reporter->dump_buffers_array =
>>> -			devlink_health_buffers_create(ops->dump_size);
>>> -		if (!reporter->dump_buffers_array) {
>>> -			kfree(reporter);
>>> -			reporter = ERR_PTR(-ENOMEM);
>>> -			goto unlock;
>>> -		}
>>> -	}
>>> -
>>> -	if (ops->diagnose) {
>>> -		reporter->diagnose_buffers_array =
>>> -			devlink_health_buffers_create(ops->diagnose_size);
>>> -		if (!reporter->diagnose_buffers_array) {
>>> -			devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>> -						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>>> -			kfree(reporter);
>>> -			reporter = ERR_PTR(-ENOMEM);
>>> -			goto unlock;
>>> -		}
>>> -	}
>>> -
>>> 	list_add_tail(&reporter->list, &devlink->reporter_list);
>>> 	mutex_init(&reporter->dump_lock);
>>> -	mutex_init(&reporter->diagnose_lock);
>>>
>>> 	reporter->priv = priv;
>>> 	reporter->ops = ops;
>>> @@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
>>> {
>>> 	mutex_lock(&reporter->devlink->lock);
>>> 	list_del(&reporter->list);
>>> -	devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>> -	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>>> +	if (reporter->dump_msg_ctx)
>>> +		devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>> 	kfree(reporter);
>>> 	mutex_unlock(&reporter->devlink->lock);
>>> }
>>> @@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
>>> 	if (reporter->dump_avail)
>>> 		return 0;
>>>
>>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>> -	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>>> -				     DEVLINK_HEALTH_BUFFER_SIZE,
>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>>> -				     priv_ctx);
>>> +	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>>> +	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>>> +		err = PTR_ERR(reporter->dump_msg_ctx);
>>> +		reporter->dump_msg_ctx = NULL;
>>> +		return err;
>>> +	}
>>> +
>>> +	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>>> +				  priv_ctx);
>>> 	if (!err) {
>>> 		reporter->dump_avail = true;
>>> 		reporter->dump_ts = jiffies;
>>> @@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>> {
>>> 	struct devlink *devlink = info->user_ptr[0];
>>> 	struct devlink_health_reporter *reporter;
>>> -	u64 num_of_buffers;
>>> +	struct devlink_msg_ctx *msg_ctx;
>>> 	int err;
>>>
>>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>>> @@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>> 	if (!reporter->ops->diagnose)
>>> 		return -EOPNOTSUPP;
>>>
>>> -	num_of_buffers =
>>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>>> +	msg_ctx = devlink_msg_ctx_alloc();
>>> +	if (IS_ERR_OR_NULL(msg_ctx))
>>> +		return PTR_ERR(msg_ctx);
>>>
>>> -	mutex_lock(&reporter->diagnose_lock);
>>> -	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>>> -				     num_of_buffers);
>>> -
>>> -	err = reporter->ops->diagnose(reporter,
>>> -				      reporter->diagnose_buffers_array,
>>> -				      DEVLINK_HEALTH_BUFFER_SIZE,
>>> -				      num_of_buffers);
>>> +	err = reporter->ops->diagnose(reporter, msg_ctx);
>> 
>> So this is not needed to be in reporter now? Why it was needed before?
>
>see reply above.
>
>> 
>> 
>> 
>>> 	if (err)
>>> 		goto out;
>>>
>>> -	err = devlink_health_buffer_snd(info,
>>> -					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>> -					0, reporter->diagnose_buffers_array,
>>> -					num_of_buffers);
>>> -	if (err)
>>> -		goto out;
>>> -
>>> -	mutex_unlock(&reporter->diagnose_lock);
>>> -	return 0;
>>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>> +			      0, msg_ctx);
>>>
>>> out:
>>> -	mutex_unlock(&reporter->diagnose_lock);
>>> +	devlink_msg_ctx_free(msg_ctx);
>>> 	return err;
>>> }
>>>
>>> @@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
>>> {
>>> 	reporter->dump_avail = false;
>>> 	reporter->dump_ts = 0;
>>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>> +	devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>> +	reporter->dump_msg_ctx = NULL;
>>> }
>>>
>>> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>> @@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>> {
>>> 	struct devlink *devlink = info->user_ptr[0];
>>> 	struct devlink_health_reporter *reporter;
>>> -	u64 num_of_buffers;
>>> 	int err;
>>>
>>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>>> @@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>> 	if (!reporter->ops->dump)
>>> 		return -EOPNOTSUPP;
>>>
>>> -	num_of_buffers =
>>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>>> -
>>> 	mutex_lock(&reporter->dump_lock);
>>> 	err = devlink_health_do_dump(reporter, NULL);
>>> 	if (err)
>>> 		goto out;
>>>
>>> -	err = devlink_health_buffer_snd(info,
>>> -					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>> -					0, reporter->dump_buffers_array,
>>> -					num_of_buffers);
>>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>> +			      0, reporter->dump_msg_ctx);
>>>
>>> out:
>>> 	mutex_unlock(&reporter->dump_lock);
>>> -- 
>>> 2.17.1
>>>

Re: [PATCH net-next 3/7] devlink: move devlink health reporter to use devlink msg API

[Eran Ben Elisha]

On 1/24/2019 11:09 AM, Jiri Pirko wrote:
> Thu, Jan 24, 2019 at 08:45:05AM CET, eranbe@mellanox.com wrote:
>>
>>
>> On 1/23/2019 4:42 PM, Jiri Pirko wrote:
>>> Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>>>> Move devlink reporter diagnose and dump operations to use the new msg API.
>>>> Redefine the signature of diagnose and dump operations and move the mlx5e
>>>> reporter to use it with the new format.
>>>>
>>>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>>>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>>>> ---
>>>> .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
>>>> include/net/devlink.h                         |  9 +-
>>>> net/core/devlink.c                            | 95 +++++--------------
>>>> 3 files changed, 28 insertions(+), 77 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> index fc92850c214a..7238cda670ba 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> @@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>>>> 		.name = "TX",
>>>> 		.recover = mlx5e_tx_reporter_recover,
>>>> +		.diagnose = mlx5e_tx_reporter_diagnose,
>>>
>>> Unrelated to this patch.
>>
>> ack.
>>
>>>
>>>
>>>> };
>>>>
>>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>>> index fe323e9b14e1..d66de8b80cc2 100644
>>>> --- a/include/net/devlink.h
>>>> +++ b/include/net/devlink.h
>>>> @@ -442,17 +442,12 @@ struct devlink_health_reporter;
>>>>
>>>> struct devlink_health_reporter_ops {
>>>> 	char *name;
>>>> -	unsigned int dump_size;
>>>> -	unsigned int diagnose_size;
>>>> 	int (*recover)(struct devlink_health_reporter *reporter,
>>>> 		       void *priv_ctx);
>>>> 	int (*dump)(struct devlink_health_reporter *reporter,
>>>> -		    struct devlink_health_buffer **buffers_array,
>>>> -		    unsigned int buffer_size, unsigned int num_buffers,
>>>> -		    void *priv_ctx);
>>>> +		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
>>>> 	int (*diagnose)(struct devlink_health_reporter *reporter,
>>>> -			struct devlink_health_buffer **buffers_array,
>>>> -			unsigned int buffer_size, unsigned int num_buffers);
>>>> +			struct devlink_msg_ctx *msg_ctx);
>>>> };
>>>>
>>>> struct devlink_ops {
>>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>>> index 57ca096849b3..347b638e6f32 100644
>>>> --- a/net/core/devlink.c
>>>> +++ b/net/core/devlink.c
>>>> @@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
>>>>
>>>> struct devlink_health_reporter {
>>>> 	struct list_head list;
>>>> -	struct devlink_health_buffer **dump_buffers_array;
>>>> +	struct devlink_msg_ctx *dump_msg_ctx;
>>>> 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>>>> -	struct devlink_health_buffer **diagnose_buffers_array;
>>>> -	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
>>>
>>> Howcome you don't need the mutex anymore?
>>
>> Now, as data is allocated on the fly while diagnose_doit(), no need to
>> store the diagnose  over the reporter anymore. So no need for any mutex
>> locking in order to prepare and send it.
> 
> Why the data is allocated on the fly? Shouldn't it be a separate patch
> as it looks like a separate change?
> 
This start to be really complicated (but possible).

(#1) I think we shall take your approach and do a revert and put it all 
again on top.

> 
>>
>>>
>>>
>>>> 	void *priv;
>>>> 	const struct devlink_health_reporter_ops *ops;
>>>> 	struct devlink *devlink;
>>>> @@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
>>>> 		goto unlock;
>>>> 	}
>>>>
>>>> -	if (WARN_ON(ops->dump && !ops->dump_size) ||
>>>> -	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>>>> -	    WARN_ON(auto_recover && !ops->recover) ||
>>>> +	if (WARN_ON(auto_recover && !ops->recover) ||
>>>> 	    WARN_ON(graceful_period && !ops->recover)) {
>>>> 		reporter = ERR_PTR(-EINVAL);
>>>> 		goto unlock;
>>>> @@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
>>>> 		goto unlock;
>>>> 	}
>>>>
>>>> -	if (ops->dump) {
>>>> -		reporter->dump_buffers_array =
>>>> -			devlink_health_buffers_create(ops->dump_size);
>>>> -		if (!reporter->dump_buffers_array) {
>>>> -			kfree(reporter);
>>>> -			reporter = ERR_PTR(-ENOMEM);
>>>> -			goto unlock;
>>>> -		}
>>>> -	}
>>>> -
>>>> -	if (ops->diagnose) {
>>>> -		reporter->diagnose_buffers_array =
>>>> -			devlink_health_buffers_create(ops->diagnose_size);
>>>> -		if (!reporter->diagnose_buffers_array) {
>>>> -			devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>>> -						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>>>> -			kfree(reporter);
>>>> -			reporter = ERR_PTR(-ENOMEM);
>>>> -			goto unlock;
>>>> -		}
>>>> -	}
>>>> -
>>>> 	list_add_tail(&reporter->list, &devlink->reporter_list);
>>>> 	mutex_init(&reporter->dump_lock);
>>>> -	mutex_init(&reporter->diagnose_lock);
>>>>
>>>> 	reporter->priv = priv;
>>>> 	reporter->ops = ops;
>>>> @@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
>>>> {
>>>> 	mutex_lock(&reporter->devlink->lock);
>>>> 	list_del(&reporter->list);
>>>> -	devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>>> -	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>>>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>>>> +	if (reporter->dump_msg_ctx)
>>>> +		devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>>> 	kfree(reporter);
>>>> 	mutex_unlock(&reporter->devlink->lock);
>>>> }
>>>> @@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
>>>> 	if (reporter->dump_avail)
>>>> 		return 0;
>>>>
>>>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>>> -	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>>>> -				     DEVLINK_HEALTH_BUFFER_SIZE,
>>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>>>> -				     priv_ctx);
>>>> +	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>>>> +	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>>>> +		err = PTR_ERR(reporter->dump_msg_ctx);
>>>> +		reporter->dump_msg_ctx = NULL;
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>>>> +				  priv_ctx);
>>>> 	if (!err) {
>>>> 		reporter->dump_avail = true;
>>>> 		reporter->dump_ts = jiffies;
>>>> @@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>>> {
>>>> 	struct devlink *devlink = info->user_ptr[0];
>>>> 	struct devlink_health_reporter *reporter;
>>>> -	u64 num_of_buffers;
>>>> +	struct devlink_msg_ctx *msg_ctx;
>>>> 	int err;
>>>>
>>>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>>>> @@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>>> 	if (!reporter->ops->diagnose)
>>>> 		return -EOPNOTSUPP;
>>>>
>>>> -	num_of_buffers =
>>>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>>>> +	msg_ctx = devlink_msg_ctx_alloc();
>>>> +	if (IS_ERR_OR_NULL(msg_ctx))
>>>> +		return PTR_ERR(msg_ctx);
>>>>
>>>> -	mutex_lock(&reporter->diagnose_lock);
>>>> -	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>>>> -				     num_of_buffers);
>>>> -
>>>> -	err = reporter->ops->diagnose(reporter,
>>>> -				      reporter->diagnose_buffers_array,
>>>> -				      DEVLINK_HEALTH_BUFFER_SIZE,
>>>> -				      num_of_buffers);
>>>> +	err = reporter->ops->diagnose(reporter, msg_ctx);
>>>
>>> So this is not needed to be in reporter now? Why it was needed before?
>>
>> see reply above.
>>
>>>
>>>
>>>
>>>> 	if (err)
>>>> 		goto out;
>>>>
>>>> -	err = devlink_health_buffer_snd(info,
>>>> -					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>>> -					0, reporter->diagnose_buffers_array,
>>>> -					num_of_buffers);
>>>> -	if (err)
>>>> -		goto out;
>>>> -
>>>> -	mutex_unlock(&reporter->diagnose_lock);
>>>> -	return 0;
>>>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>>> +			      0, msg_ctx);
>>>>
>>>> out:
>>>> -	mutex_unlock(&reporter->diagnose_lock);
>>>> +	devlink_msg_ctx_free(msg_ctx);
>>>> 	return err;
>>>> }
>>>>
>>>> @@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
>>>> {
>>>> 	reporter->dump_avail = false;
>>>> 	reporter->dump_ts = 0;
>>>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>>> +	devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>>> +	reporter->dump_msg_ctx = NULL;
>>>> }
>>>>
>>>> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>>> @@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>>> {
>>>> 	struct devlink *devlink = info->user_ptr[0];
>>>> 	struct devlink_health_reporter *reporter;
>>>> -	u64 num_of_buffers;
>>>> 	int err;
>>>>
>>>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>>>> @@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>>> 	if (!reporter->ops->dump)
>>>> 		return -EOPNOTSUPP;
>>>>
>>>> -	num_of_buffers =
>>>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>>>> -
>>>> 	mutex_lock(&reporter->dump_lock);
>>>> 	err = devlink_health_do_dump(reporter, NULL);
>>>> 	if (err)
>>>> 		goto out;
>>>>
>>>> -	err = devlink_health_buffer_snd(info,
>>>> -					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>>> -					0, reporter->dump_buffers_array,
>>>> -					num_of_buffers);
>>>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>>> +			      0, reporter->dump_msg_ctx);
>>>>
>>>> out:
>>>> 	mutex_unlock(&reporter->dump_lock);
>>>> -- 
>>>> 2.17.1
>>>>

[PATCH net-next 4/7] devlink: Delete depracated health buffers API

[Eran Ben Elisha]

It is not in use anymore and can be safely removed from the kernel code.

Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
---
 include/net/devlink.h        |  76 ------
 include/uapi/linux/devlink.h |   8 -
 net/core/devlink.c           | 501 -----------------------------------
 3 files changed, 585 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index d66de8b80cc2..0aaf51289628 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -425,7 +425,6 @@ struct devlink_region;
 typedef void devlink_snapshot_data_dest_t(const void *data);
 
 struct devlink_msg_ctx;
-struct devlink_health_buffer;
 struct devlink_health_reporter;
 
 /**
@@ -626,22 +625,6 @@ int devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
 int devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name);
 int devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name);
 
-int devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
-				     int attrtype);
-void devlink_health_buffer_nest_end(struct devlink_health_buffer *buffer);
-void devlink_health_buffer_nest_cancel(struct devlink_health_buffer *buffer);
-int devlink_health_buffer_put_object_name(struct devlink_health_buffer *buffer,
-					  char *name);
-int devlink_health_buffer_put_value_u8(struct devlink_health_buffer *buffer,
-				       u8 value);
-int devlink_health_buffer_put_value_u32(struct devlink_health_buffer *buffer,
-					u32 value);
-int devlink_health_buffer_put_value_u64(struct devlink_health_buffer *buffer,
-					u64 value);
-int devlink_health_buffer_put_value_string(struct devlink_health_buffer *buffer,
-					   char *name);
-int devlink_health_buffer_put_value_data(struct devlink_health_buffer *buffer,
-					 void *data, int len);
 struct devlink_health_reporter *
 devlink_health_reporter_create(struct devlink *devlink,
 			       const struct devlink_health_reporter_ops *ops,
@@ -968,65 +951,6 @@ devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name)
 	return 0;
 }
 
-static inline int
-devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
-				 int attrtype)
-{
-	return 0;
-}
-
-static inline void
-devlink_health_buffer_nest_end(struct devlink_health_buffer *buffer)
-{
-}
-
-static inline void
-devlink_health_buffer_nest_cancel(struct devlink_health_buffer *buffer)
-{
-}
-
-static inline int
-devlink_health_buffer_put_object_name(struct devlink_health_buffer *buffer,
-				      char *name)
-{
-	return 0;
-}
-
-static inline int
-devlink_health_buffer_put_value_u8(struct devlink_health_buffer *buffer,
-				   u8 value)
-{
-	return 0;
-}
-
-static inline int
-devlink_health_buffer_put_value_u32(struct devlink_health_buffer *buffer,
-				    u32 value)
-{
-	return 0;
-}
-
-static inline int
-devlink_health_buffer_put_value_u64(struct devlink_health_buffer *buffer,
-				    u64 value)
-{
-	return 0;
-}
-
-static inline int
-devlink_health_buffer_put_value_string(struct devlink_health_buffer *buffer,
-				       char *name)
-{
-	return 0;
-}
-
-static inline int
-devlink_health_buffer_put_value_data(struct devlink_health_buffer *buffer,
-				     void *data, int len)
-{
-	return 0;
-}
-
 static inline struct devlink_health_reporter *
 devlink_health_reporter_create(struct devlink *devlink,
 			       const struct devlink_health_reporter_ops *ops,
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 68eeda1d0455..e4c62e80ab5e 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -292,14 +292,6 @@ enum devlink_attr {
 	DEVLINK_ATTR_REGION_CHUNK_ADDR,         /* u64 */
 	DEVLINK_ATTR_REGION_CHUNK_LEN,          /* u64 */
 
-	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT,		/* nested */
-	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR,		/* nested */
-	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_NAME,		/* string */
-	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE,	/* nested */
-	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_ARRAY,	/* nested */
-	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE,	/* u8 */
-	DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA,	/* dynamic */
-
 	DEVLINK_ATTR_MSG_OBJECT,		/* nested */
 	DEVLINK_ATTR_MSG_OBJECT_PAIR,		/* nested */
 	DEVLINK_ATTR_MSG_OBJECT_NAME,		/* string */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 347b638e6f32..379ebd3f9a59 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3597,507 +3597,6 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	return 0;
 }
 
-#define DEVLINK_HEALTH_BUFFER_SIZE (4096 - GENL_HDRLEN)
-#define DEVLINK_HEALTH_BUFFER_DATA_SIZE (DEVLINK_HEALTH_BUFFER_SIZE / 2)
-#define DEVLINK_HEALTH_SIZE_TO_BUFFERS(size) DIV_ROUND_UP(size, DEVLINK_HEALTH_BUFFER_DATA_SIZE)
-#define DEVLINK_HEALTH_BUFFER_MAX_CHUNK 1024
-
-struct devlink_health_buffer {
-	void *data;
-	u64 offset;
-	u64 bytes_left;
-	u64 bytes_left_metadata;
-	u64 max_nested_depth;
-	u64 curr_nest;
-};
-
-struct devlink_health_buffer_desc {
-	int attrtype;
-	u16 len;
-	u8 nla_type;
-	u8 nest_end;
-	int value[0];
-};
-
-static void
-devlink_health_buffers_reset(struct devlink_health_buffer **buffers_list,
-			     u64 num_of_buffers)
-{
-	u64 i;
-
-	for (i = 0; i < num_of_buffers; i++) {
-		memset(buffers_list[i]->data, 0, DEVLINK_HEALTH_BUFFER_SIZE);
-		buffers_list[i]->offset = 0;
-		buffers_list[i]->bytes_left = DEVLINK_HEALTH_BUFFER_DATA_SIZE;
-		buffers_list[i]->bytes_left_metadata =
-			DEVLINK_HEALTH_BUFFER_DATA_SIZE;
-		buffers_list[i]->max_nested_depth = 0;
-		buffers_list[i]->curr_nest = 0;
-	}
-}
-
-static void
-devlink_health_buffers_destroy(struct devlink_health_buffer **buffers_list,
-			       u64 size);
-
-static struct devlink_health_buffer **
-devlink_health_buffers_create(u64 size)
-{
-	struct devlink_health_buffer **buffers_list;
-	u64 num_of_buffers = DEVLINK_HEALTH_SIZE_TO_BUFFERS(size);
-	u64 i;
-
-	buffers_list = kcalloc(num_of_buffers,
-			       sizeof(struct devlink_health_buffer *),
-			       GFP_KERNEL);
-	if (!buffers_list)
-		return NULL;
-
-	for (i = 0; i < num_of_buffers; i++) {
-		struct devlink_health_buffer *buffer;
-		void *data;
-
-		buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
-		data = kzalloc(DEVLINK_HEALTH_BUFFER_SIZE, GFP_KERNEL);
-		if (!buffer || !data) {
-			kfree(buffer);
-			kfree(data);
-			goto buffers_cleanup;
-		}
-		buffers_list[i] = buffer;
-		buffer->data = data;
-	}
-	devlink_health_buffers_reset(buffers_list, num_of_buffers);
-
-	return buffers_list;
-
-buffers_cleanup:
-	devlink_health_buffers_destroy(buffers_list, --i);
-	kfree(buffers_list);
-	return NULL;
-}
-
-static void
-devlink_health_buffers_destroy(struct devlink_health_buffer **buffers_list,
-			       u64 num_of_buffers)
-{
-	u64 i;
-
-	for (i = 0; i < num_of_buffers; i++) {
-		kfree(buffers_list[i]->data);
-		kfree(buffers_list[i]);
-	}
-}
-
-void
-devlink_health_buffer_offset_inc(struct devlink_health_buffer *buffer,
-				 int len)
-{
-	buffer->offset += len;
-}
-
-/* In order to store a nest, need two descriptors, for start and end */
-#define DEVLINK_HEALTH_BUFFER_NEST_SIZE (sizeof(struct devlink_health_buffer_desc) * 2)
-
-int devlink_health_buffer_verify_len(struct devlink_health_buffer *buffer,
-				     int len, int metadata_len)
-{
-	if (len > DEVLINK_HEALTH_BUFFER_DATA_SIZE)
-		return -EINVAL;
-
-	if (buffer->bytes_left < len ||
-	    buffer->bytes_left_metadata < metadata_len)
-		return -ENOMEM;
-
-	return 0;
-}
-
-static struct devlink_health_buffer_desc *
-devlink_health_buffer_get_desc_from_offset(struct devlink_health_buffer *buffer)
-{
-	return buffer->data + buffer->offset;
-}
-
-int
-devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
-				 int attrtype)
-{
-	struct devlink_health_buffer_desc *desc;
-	int err;
-
-	err = devlink_health_buffer_verify_len(buffer, 0,
-					       DEVLINK_HEALTH_BUFFER_NEST_SIZE);
-	if (err)
-		return err;
-
-	if (attrtype != DEVLINK_ATTR_HEALTH_BUFFER_OBJECT &&
-	    attrtype != DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR &&
-	    attrtype != DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE &&
-	    attrtype != DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_ARRAY)
-		return -EINVAL;
-
-	desc = devlink_health_buffer_get_desc_from_offset(buffer);
-
-	desc->attrtype = attrtype;
-	buffer->bytes_left_metadata -= DEVLINK_HEALTH_BUFFER_NEST_SIZE;
-	devlink_health_buffer_offset_inc(buffer, sizeof(*desc));
-
-	buffer->curr_nest++;
-	buffer->max_nested_depth = max(buffer->max_nested_depth,
-				       buffer->curr_nest);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(devlink_health_buffer_nest_start);
-
-enum devlink_health_buffer_nest_end_cancel {
-	DEVLINK_HEALTH_BUFFER_NEST_END = 1,
-	DEVLINK_HEALTH_BUFFER_NEST_CANCEL,
-};
-
-static void
-devlink_health_buffer_nest_end_cancel(struct devlink_health_buffer *buffer,
-				      enum devlink_health_buffer_nest_end_cancel nest)
-{
-	struct devlink_health_buffer_desc *desc;
-
-	WARN_ON(!buffer->curr_nest);
-	buffer->curr_nest--;
-
-	desc = devlink_health_buffer_get_desc_from_offset(buffer);
-	desc->nest_end = nest;
-	devlink_health_buffer_offset_inc(buffer, sizeof(*desc));
-}
-
-void devlink_health_buffer_nest_end(struct devlink_health_buffer *buffer)
-{
-	devlink_health_buffer_nest_end_cancel(buffer,
-					      DEVLINK_HEALTH_BUFFER_NEST_END);
-}
-EXPORT_SYMBOL_GPL(devlink_health_buffer_nest_end);
-
-void devlink_health_buffer_nest_cancel(struct devlink_health_buffer *buffer)
-{
-	devlink_health_buffer_nest_end_cancel(buffer,
-					      DEVLINK_HEALTH_BUFFER_NEST_CANCEL);
-}
-EXPORT_SYMBOL_GPL(devlink_health_buffer_nest_cancel);
-
-int
-devlink_health_buffer_put_object_name(struct devlink_health_buffer *buffer,
-				      char *name)
-{
-	struct devlink_health_buffer_desc *desc;
-	int err;
-
-	err = devlink_health_buffer_verify_len(buffer, strlen(name) + 1,
-					       sizeof(*desc));
-	if (err)
-		return err;
-
-	desc = devlink_health_buffer_get_desc_from_offset(buffer);
-	desc->attrtype = DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_NAME;
-	desc->nla_type = NLA_NUL_STRING;
-	desc->len = strlen(name) + 1;
-	memcpy(&desc->value, name, desc->len);
-	devlink_health_buffer_offset_inc(buffer, sizeof(*desc) + desc->len);
-
-	buffer->bytes_left_metadata -= sizeof(*desc);
-	buffer->bytes_left -= (strlen(name) + 1);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(devlink_health_buffer_put_object_name);
-
-static int
-devlink_health_buffer_put_value(struct devlink_health_buffer *buffer,
-				u8 nla_type, void *value, int len)
-{
-	struct devlink_health_buffer_desc *desc;
-	int err;
-
-	err = devlink_health_buffer_verify_len(buffer, len, sizeof(*desc));
-	if (err)
-		return err;
-
-	desc = devlink_health_buffer_get_desc_from_offset(buffer);
-	desc->attrtype = DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA;
-	desc->nla_type = nla_type;
-	desc->len = len;
-	memcpy(&desc->value, value, len);
-	devlink_health_buffer_offset_inc(buffer, sizeof(*desc) + desc->len);
-
-	buffer->bytes_left_metadata -= sizeof(*desc);
-	buffer->bytes_left -= len;
-
-	return 0;
-}
-
-int
-devlink_health_buffer_put_value_u8(struct devlink_health_buffer *buffer,
-				   u8 value)
-{
-	int err;
-
-	err = devlink_health_buffer_put_value(buffer, NLA_U8, &value,
-					      sizeof(value));
-	if (err)
-		return err;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(devlink_health_buffer_put_value_u8);
-
-int
-devlink_health_buffer_put_value_u32(struct devlink_health_buffer *buffer,
-				    u32 value)
-{
-	int err;
-
-	err = devlink_health_buffer_put_value(buffer, NLA_U32, &value,
-					      sizeof(value));
-	if (err)
-		return err;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(devlink_health_buffer_put_value_u32);
-
-int
-devlink_health_buffer_put_value_u64(struct devlink_health_buffer *buffer,
-				    u64 value)
-{
-	int err;
-
-	err = devlink_health_buffer_put_value(buffer, NLA_U64, &value,
-					      sizeof(value));
-	if (err)
-		return err;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(devlink_health_buffer_put_value_u64);
-
-int
-devlink_health_buffer_put_value_string(struct devlink_health_buffer *buffer,
-				       char *name)
-{
-	int err;
-
-	if (strlen(name) + 1 > DEVLINK_HEALTH_BUFFER_MAX_CHUNK)
-		return -EINVAL;
-
-	err = devlink_health_buffer_put_value(buffer, NLA_NUL_STRING, name,
-					      strlen(name) + 1);
-	if (err)
-		return err;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(devlink_health_buffer_put_value_string);
-
-int
-devlink_health_buffer_put_value_data(struct devlink_health_buffer *buffer,
-				     void *data, int len)
-{
-	int err;
-
-	if (len > DEVLINK_HEALTH_BUFFER_MAX_CHUNK)
-		return -EINVAL;
-
-	err = devlink_health_buffer_put_value(buffer, NLA_BINARY, data, len);
-	if (err)
-		return err;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(devlink_health_buffer_put_value_data);
-
-static int
-devlink_health_buffer_fill_data(struct sk_buff *skb,
-				struct devlink_health_buffer_desc *desc)
-{
-	int err = -EINVAL;
-
-	switch (desc->nla_type) {
-	case NLA_U8:
-		err = nla_put_u8(skb, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA,
-				 *(u8 *)desc->value);
-		break;
-	case NLA_U32:
-		err = nla_put_u32(skb, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA,
-				  *(u32 *)desc->value);
-		break;
-	case NLA_U64:
-		err = nla_put_u64_64bit(skb,
-					DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA,
-					*(u64 *)desc->value, DEVLINK_ATTR_PAD);
-		break;
-	case NLA_NUL_STRING:
-		err = nla_put_string(skb,
-				     DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA,
-				     (char *)&desc->value);
-		break;
-	case NLA_BINARY:
-		err = nla_put(skb, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA,
-			      desc->len, (void *)&desc->value);
-		break;
-	}
-
-	return err;
-}
-
-static int
-devlink_health_buffer_fill_type(struct sk_buff *skb,
-				struct devlink_health_buffer_desc *desc)
-{
-	int err = -EINVAL;
-
-	switch (desc->nla_type) {
-	case NLA_U8:
-		err = nla_put_u8(skb, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE,
-				 NLA_U8);
-		break;
-	case NLA_U32:
-		err = nla_put_u8(skb, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE,
-				 NLA_U32);
-		break;
-	case NLA_U64:
-		err = nla_put_u8(skb, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE,
-				 NLA_U64);
-		break;
-	case NLA_NUL_STRING:
-		err = nla_put_u8(skb, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE,
-				 NLA_NUL_STRING);
-		break;
-	case NLA_BINARY:
-		err = nla_put_u8(skb, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE,
-				 NLA_BINARY);
-		break;
-	}
-
-	return err;
-}
-
-static inline struct devlink_health_buffer_desc *
-devlink_health_buffer_get_next_desc(struct devlink_health_buffer_desc *desc)
-{
-	return (void *)&desc->value + desc->len;
-}
-
-static int
-devlink_health_buffer_prepare_skb(struct sk_buff *skb,
-				  struct devlink_health_buffer *buffer)
-{
-	struct devlink_health_buffer_desc *last_desc, *desc;
-	struct nlattr **buffer_nlattr;
-	int err;
-	int i = 0;
-
-	buffer_nlattr = kcalloc(buffer->max_nested_depth,
-				sizeof(*buffer_nlattr), GFP_KERNEL);
-	if (!buffer_nlattr)
-		return -EINVAL;
-
-	last_desc = devlink_health_buffer_get_desc_from_offset(buffer);
-	desc = buffer->data;
-	while (desc != last_desc) {
-		switch (desc->attrtype) {
-		case DEVLINK_ATTR_HEALTH_BUFFER_OBJECT:
-		case DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR:
-		case DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE:
-		case DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_ARRAY:
-			buffer_nlattr[i] = nla_nest_start(skb, desc->attrtype);
-			if (!buffer_nlattr[i])
-				goto nla_put_failure;
-			i++;
-			break;
-		case DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA:
-			err = devlink_health_buffer_fill_data(skb, desc);
-			if (err)
-				goto nla_put_failure;
-			err = devlink_health_buffer_fill_type(skb, desc);
-			if (err)
-				goto nla_put_failure;
-			break;
-		case DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_NAME:
-			err = nla_put_string(skb, desc->attrtype,
-					     (char *)&desc->value);
-			if (err)
-				goto nla_put_failure;
-			break;
-		default:
-			WARN_ON(!desc->nest_end);
-			WARN_ON(i <= 0);
-			if (desc->nest_end == DEVLINK_HEALTH_BUFFER_NEST_END)
-				nla_nest_end(skb, buffer_nlattr[--i]);
-			else
-				nla_nest_cancel(skb, buffer_nlattr[--i]);
-			break;
-		}
-		desc = devlink_health_buffer_get_next_desc(desc);
-	}
-
-	return 0;
-
-nla_put_failure:
-	kfree(buffer_nlattr);
-	return err;
-}
-
-static int
-devlink_health_buffer_snd(struct genl_info *info,
-			  enum devlink_command cmd, int flags,
-			  struct devlink_health_buffer **buffers_array,
-			  u64 num_of_buffers)
-{
-	struct sk_buff *skb;
-	struct nlmsghdr *nlh;
-	void *hdr;
-	int err;
-	u64 i;
-
-	for (i = 0; i < num_of_buffers; i++) {
-		/* Skip buffer if driver did not fill it up with any data */
-		if (!buffers_array[i]->offset)
-			continue;
-
-		skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
-		if (!skb)
-			return -ENOMEM;
-
-		hdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
-				  &devlink_nl_family, NLM_F_MULTI, cmd);
-		if (!hdr)
-			goto nla_put_failure;
-
-		err = devlink_health_buffer_prepare_skb(skb, buffers_array[i]);
-		if (err)
-			goto nla_put_failure;
-
-		genlmsg_end(skb, hdr);
-		err = genlmsg_reply(skb, info);
-		if (err)
-			return err;
-	}
-
-	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
-	if (!skb)
-		return -ENOMEM;
-	nlh = nlmsg_put(skb, info->snd_portid, info->snd_seq,
-			NLMSG_DONE, 0, flags | NLM_F_MULTI);
-	err = genlmsg_reply(skb, info);
-	if (err)
-		return err;
-
-	return 0;
-
-nla_put_failure:
-	err = -EIO;
-	nlmsg_free(skb);
-	return err;
-}
-
 struct devlink_msg {
 	struct list_head list;
 	int attrtype;
-- 
2.17.1

[PATCH net-next 5/7] devlink: Remove spaces around "=" in the logger print

[Eran Ben Elisha]

No need for spaces around "=" in the logger print.

Fixes: c7af343b4e33 ("devlink: Add health report functionality")
Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
Reported-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
---
 include/trace/events/devlink.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/events/devlink.h b/include/trace/events/devlink.h
index 7e39d2fc7c75..bc09c3408912 100644
--- a/include/trace/events/devlink.h
+++ b/include/trace/events/devlink.h
@@ -98,7 +98,7 @@ TRACE_EVENT(devlink_health_recover_aborted,
 		__entry->time_since_last_recover = time_since_last_recover;
 	),
 
-	TP_printk("bus_name=%s dev_name=%s driver_name=%s reporter_name=%s: health_state=%d time_since_last_recover = %llu recover aborted",
+	TP_printk("bus_name=%s dev_name=%s driver_name=%s reporter_name=%s: health_state=%d time_since_last_recover=%llu recover aborted",
 		  __get_str(bus_name), __get_str(dev_name),
 		  __get_str(driver_name), __get_str(reporter_name),
 		  __entry->health_state,
-- 
2.17.1

[PATCH net-next 6/7] devlink: Fix use-after-free at reporter destroy

[Eran Ben Elisha]

Fix a bug where reporter->devlink attribute was used after the reporter
was freed.

Fixes: 880ee82f0313 ("devlink: Add health reporter create/destroy functionality")
Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
---
 net/core/devlink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 379ebd3f9a59..3cb74694de86 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4150,12 +4150,14 @@ EXPORT_SYMBOL_GPL(devlink_health_reporter_create);
 void
 devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
 {
-	mutex_lock(&reporter->devlink->lock);
+	struct devlink *devlink = reporter->devlink;
+
+	mutex_lock(&devlink->lock);
 	list_del(&reporter->list);
 	if (reporter->dump_msg_ctx)
 		devlink_msg_ctx_free(reporter->dump_msg_ctx);
 	kfree(reporter);
-	mutex_unlock(&reporter->devlink->lock);
+	mutex_unlock(&devlink->lock);
 }
 EXPORT_SYMBOL_GPL(devlink_health_reporter_destroy);
 
-- 
2.17.1

[PATCH net-next 7/7] net/mlx5e: Add RTNL lock to TX recover flow

[Eran Ben Elisha]

As part of the recover flow, driver calls mlx5e_open_locked, which
eventually calls netif_set_real_num_tx_queues. RTNL lock must be held as
part of this flow in advance, like in other flows in the driver.

Fixes: aba25279c100 ("net/mlx5e: Add TX reporter support")
Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
Reported-by: Maria Pasechnik <mariap@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
index 7238cda670ba..5c29de7bd1d6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
@@ -173,10 +173,12 @@ static int mlx5e_tx_reporter_recover_all(struct mlx5e_priv *priv)
 {
 	int err;
 
+	rtnl_lock();
 	mutex_lock(&priv->state_lock);
 	mlx5e_close_locked(priv->netdev);
 	err = mlx5e_open_locked(priv->netdev);
 	mutex_unlock(&priv->state_lock);
+	rtnl_unlock();
 
 	return err;
 }
-- 
2.17.1

Re: [PATCH net-next 0/7] Devlink health updates

[Jiri Pirko]

Tue, Jan 22, 2019 at 04:57:17PM CET, eranbe@mellanox.com wrote:
>This patchset fixes some comments that were received for the devlink
>health series, mostly around the devlink health buffers API.
>
>It offers a new devlink<->driver API for passing health dump and diagnose info.
>As part of this patchset, the new API is developed and integrated into the
>devlink health and mlx5e TX reporter.
>Also, added some helpers together with the new API, which reduce the code
>required by the driver to fill dump and diagnose significantly.
>
>Eventually, it also deletes the old API.
>
>In addition, it includes some small fixes in the devlink and mlx5e TX reporter.

Okay, just leaving, going to review this tomorrow. I would much rather
review the patchset from the beginning, not this incremental patchset.
It changes a lot of things, deprecating api what was just introduced.
Review nightmare :/

Could we do revert, repost? For my health sakes :)

Thanks!


>
>
>Eran Ben Elisha (7):
>  devlink: Add devlink msg API
>  net/mlx5e: Move driver to use devlink msg API
>  devlink: move devlink health reporter to use devlink msg API
>  devlink: Delete depracated health buffers API
>  devlink: Remove spaces around "=" in the logger print
>  devlink: Fix use-after-free at reporter destroy
>  net/mlx5e: Add RTNL lock to TX recover flow
>
> .../mellanox/mlx5/core/en/reporter_tx.c       | 124 +---
> include/net/devlink.h                         |  79 +--
> include/trace/events/devlink.h                |   2 +-
> include/uapi/linux/devlink.h                  |  14 +-
> net/core/devlink.c                            | 633 ++++++++----------
> 5 files changed, 342 insertions(+), 510 deletions(-)
>
>-- 
>2.17.1
>

Re: [PATCH net-next 0/7] Devlink health updates

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 22 Jan 2019 17:58:21 +0100

> Tue, Jan 22, 2019 at 04:57:17PM CET, eranbe@mellanox.com wrote:
>>This patchset fixes some comments that were received for the devlink
>>health series, mostly around the devlink health buffers API.
>>
>>It offers a new devlink<->driver API for passing health dump and diagnose info.
>>As part of this patchset, the new API is developed and integrated into the
>>devlink health and mlx5e TX reporter.
>>Also, added some helpers together with the new API, which reduce the code
>>required by the driver to fill dump and diagnose significantly.
>>
>>Eventually, it also deletes the old API.
>>
>>In addition, it includes some small fixes in the devlink and mlx5e TX reporter.
> 
> Okay, just leaving, going to review this tomorrow. I would much rather
> review the patchset from the beginning, not this incremental patchset.
> It changes a lot of things, deprecating api what was just introduced.
> Review nightmare :/
> 
> Could we do revert, repost? For my health sakes :)

Eran are you ok with the revert?

I'll do it once I have Eran's confirmation.

Re: [PATCH net-next 0/7] Devlink health updates

[Eran Ben Elisha]

On 1/25/2019 8:08 AM, David Miller wrote:
> From: Jiri Pirko <jiri@resnulli.us>
> Date: Tue, 22 Jan 2019 17:58:21 +0100
> 
>> Tue, Jan 22, 2019 at 04:57:17PM CET, eranbe@mellanox.com wrote:
>>> This patchset fixes some comments that were received for the devlink
>>> health series, mostly around the devlink health buffers API.
>>>
>>> It offers a new devlink<->driver API for passing health dump and diagnose info.
>>> As part of this patchset, the new API is developed and integrated into the
>>> devlink health and mlx5e TX reporter.
>>> Also, added some helpers together with the new API, which reduce the code
>>> required by the driver to fill dump and diagnose significantly.
>>>
>>> Eventually, it also deletes the old API.
>>>
>>> In addition, it includes some small fixes in the devlink and mlx5e TX reporter.
>>
>> Okay, just leaving, going to review this tomorrow. I would much rather
>> review the patchset from the beginning, not this incremental patchset.
>> It changes a lot of things, deprecating api what was just introduced.
>> Review nightmare :/
>>
>> Could we do revert, repost? For my health sakes :)
> 
> Eran are you ok with the revert?

Dave, thanks for your consideration.

During the review of this fixes series with Jiri yesterday, we reached 
to a conclusion that it would be cleaner to revert and re-post it again.
I thought I shall submit a revert patchset, but if just remove it, it 
would be better, I guess.

Jiri,
I will probably be able to provide a new version with fixed comments 
from here soon next week.

> 
> I'll do it once I have Eran's confirmation.

Note that you will also have to revert ARM compilation fix which was 
accepted on top.
https://patchwork.ozlabs.org/patch/1029047/

Thanks.

Re: [PATCH net-next 0/7] Devlink health updates

[Jiri Pirko]

Fri, Jan 25, 2019 at 10:16:16AM CET, eranbe@mellanox.com wrote:
>
>
>On 1/25/2019 8:08 AM, David Miller wrote:
>> From: Jiri Pirko <jiri@resnulli.us>
>> Date: Tue, 22 Jan 2019 17:58:21 +0100
>> 
>>> Tue, Jan 22, 2019 at 04:57:17PM CET, eranbe@mellanox.com wrote:
>>>> This patchset fixes some comments that were received for the devlink
>>>> health series, mostly around the devlink health buffers API.
>>>>
>>>> It offers a new devlink<->driver API for passing health dump and diagnose info.
>>>> As part of this patchset, the new API is developed and integrated into the
>>>> devlink health and mlx5e TX reporter.
>>>> Also, added some helpers together with the new API, which reduce the code
>>>> required by the driver to fill dump and diagnose significantly.
>>>>
>>>> Eventually, it also deletes the old API.
>>>>
>>>> In addition, it includes some small fixes in the devlink and mlx5e TX reporter.
>>>
>>> Okay, just leaving, going to review this tomorrow. I would much rather
>>> review the patchset from the beginning, not this incremental patchset.
>>> It changes a lot of things, deprecating api what was just introduced.
>>> Review nightmare :/
>>>
>>> Could we do revert, repost? For my health sakes :)
>> 
>> Eran are you ok with the revert?
>
>Dave, thanks for your consideration.
>
>During the review of this fixes series with Jiri yesterday, we reached 
>to a conclusion that it would be cleaner to revert and re-post it again.
>I thought I shall submit a revert patchset, but if just remove it, it 
>would be better, I guess.
>
>Jiri,
>I will probably be able to provide a new version with fixed comments 
>from here soon next week.

Good. Thanks!


>
>> 
>> I'll do it once I have Eran's confirmation.
>
>Note that you will also have to revert ARM compilation fix which was 
>accepted on top.
>https://patchwork.ozlabs.org/patch/1029047/
>
>Thanks.

Re: [PATCH net-next 0/7] Devlink health updates

[David Miller]

From: Eran Ben Elisha <eranbe@mellanox.com>
Date: Fri, 25 Jan 2019 09:16:16 +0000

> On 1/25/2019 8:08 AM, David Miller wrote:
>> I'll do it once I have Eran's confirmation.
> 
> Note that you will also have to revert ARM compilation fix which was 
> accepted on top.
> https://patchwork.ozlabs.org/patch/1029047/

Thanks for this heads up.

I just pushed out the revert.

If I made any mistakes, please send me a fixup.  But I am pretty sure
I got it right.

Thanks!

Re: [PATCH net-next 0/7] Devlink health updates

[Jiri Pirko]

Tue, Jan 22, 2019 at 04:57:17PM CET, eranbe@mellanox.com wrote:
>This patchset fixes some comments that were received for the devlink
>health series, mostly around the devlink health buffers API.
>
>It offers a new devlink<->driver API for passing health dump and diagnose info.
>As part of this patchset, the new API is developed and integrated into the
>devlink health and mlx5e TX reporter.
>Also, added some helpers together with the new API, which reduce the code
>required by the driver to fill dump and diagnose significantly.
>
>Eventually, it also deletes the old API.
>
>In addition, it includes some small fixes in the devlink and mlx5e TX reporter.

We are (re-)defining UAPI here. You need to present some examples
of devlink tool output, both in json and stdout.
Again, much more convenient to be done for the whole patchset, not this
"fixup-one" :/


>
>
>Eran Ben Elisha (7):
>  devlink: Add devlink msg API
>  net/mlx5e: Move driver to use devlink msg API
>  devlink: move devlink health reporter to use devlink msg API
>  devlink: Delete depracated health buffers API
>  devlink: Remove spaces around "=" in the logger print
>  devlink: Fix use-after-free at reporter destroy
>  net/mlx5e: Add RTNL lock to TX recover flow
>
> .../mellanox/mlx5/core/en/reporter_tx.c       | 124 +---
> include/net/devlink.h                         |  79 +--
> include/trace/events/devlink.h                |   2 +-
> include/uapi/linux/devlink.h                  |  14 +-
> net/core/devlink.c                            | 633 ++++++++----------
> 5 files changed, 342 insertions(+), 510 deletions(-)
>
>-- 
>2.17.1
>

Re: [PATCH net-next 0/7] Devlink health updates

[Eran Ben Elisha]

On 1/23/2019 1:44 PM, Jiri Pirko wrote:
> Tue, Jan 22, 2019 at 04:57:17PM CET, eranbe@mellanox.com wrote:
>> This patchset fixes some comments that were received for the devlink
>> health series, mostly around the devlink health buffers API.
>>
>> It offers a new devlink<->driver API for passing health dump and diagnose info.
>> As part of this patchset, the new API is developed and integrated into the
>> devlink health and mlx5e TX reporter.
>> Also, added some helpers together with the new API, which reduce the code
>> required by the driver to fill dump and diagnose significantly.
>>
>> Eventually, it also deletes the old API.
>>
>> In addition, it includes some small fixes in the devlink and mlx5e TX reporter.
> 
> We are (re-)defining UAPI here. You need to present some examples
> of devlink tool output, both in json and stdout.

Actually we don't really redefining the section, but only having naming 
change, DEVLINK_ATTR_HEALTH_BUFFER_OBJECT* to DEVLINK_ATTR_MSG_OBJECT*. 
It is pretty much the same in UAPI perspective. All examples from 
original patchset are still 100% true.

> Again, much more convenient to be done for the whole patchset, not this
> "fixup-one" :/

The core of the change you have asked for it fully implemented in patch 
0001 (and will be kept as is if I would do the re-done procedure). Me 
and Moshe had an internal review yesterday and it doesn't look like a 
nightmare at all.

Please try to give it a real try as I worked a lot in order to meet the 
DD set here, and if you still find it really hard, let me know.

> 
> 
>>
>>
>> Eran Ben Elisha (7):
>>   devlink: Add devlink msg API
>>   net/mlx5e: Move driver to use devlink msg API
>>   devlink: move devlink health reporter to use devlink msg API
>>   devlink: Delete depracated health buffers API
>>   devlink: Remove spaces around "=" in the logger print
>>   devlink: Fix use-after-free at reporter destroy
>>   net/mlx5e: Add RTNL lock to TX recover flow
>>
>> .../mellanox/mlx5/core/en/reporter_tx.c       | 124 +---
>> include/net/devlink.h                         |  79 +--
>> include/trace/events/devlink.h                |   2 +-
>> include/uapi/linux/devlink.h                  |  14 +-
>> net/core/devlink.c                            | 633 ++++++++----------
>> 5 files changed, 342 insertions(+), 510 deletions(-)
>>
>> -- 
>> 2.17.1
>>