[PATCH v2 0/3] devlink region trigger support

[PATCH v2 0/3] devlink region trigger support

[Jacob Keller]

This series consists of patches to enable devlink to request a snapshot via
a new DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT.

A reviewer might notice that the devlink health API already has such support
for handling a similar case. However, the health API does not make sense in
cases where the data is not related to an error condition.

In this case, using the health API only for the dumping feels incorrect.
Regions make sense when the addressable content is not captured
automatically on error conditions, but only upon request by the devlink API.

The netdevsim driver is modified to support the new trigger_snapshot
callback as an example of how this can be used.

Jacob Keller (3):
  devlink: add callback to trigger region snapshots
  devlink: introduce command to trigger region snapshot
  netdevsim: support triggering snapshot through devlink

 drivers/net/ethernet/mellanox/mlx4/crdump.c |  4 +-
 drivers/net/netdevsim/dev.c                 | 37 ++++++++++++-----
 include/net/devlink.h                       | 12 ++++--
 include/uapi/linux/devlink.h                |  2 +
 net/core/devlink.c                          | 45 +++++++++++++++++++--
 5 files changed, 80 insertions(+), 20 deletions(-)

-- 
2.25.0.rc1

[PATCH v2 1/3] devlink: add callback to trigger region snapshots

[Jacob Keller]

Add a trigger_snapshot parameter to the devlink_region_create function.
This is a function pointer that will be used to enable devlink API to
request a snapshot of a region.

Passing NULL is acceptable to indicate the region does not support
triggered snapshots.

Future commits will introduce the new devlink command and modify
netdevsim as an example of how the trigger will work.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/mellanox/mlx4/crdump.c |  4 ++--
 drivers/net/netdevsim/dev.c                 |  3 ++-
 include/net/devlink.h                       | 12 ++++++++----
 net/core/devlink.c                          | 11 +++++++----
 4 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/crdump.c b/drivers/net/ethernet/mellanox/mlx4/crdump.c
index 64ed725aec28..4b1373414b0b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/crdump.c
+++ b/drivers/net/ethernet/mellanox/mlx4/crdump.c
@@ -207,7 +207,7 @@ int mlx4_crdump_init(struct mlx4_dev *dev)
 		devlink_region_create(devlink,
 				      region_cr_space_str,
 				      MAX_NUM_OF_DUMPS_TO_STORE,
-				      pci_resource_len(pdev, 0));
+				      pci_resource_len(pdev, 0), NULL);
 	if (IS_ERR(crdump->region_crspace))
 		mlx4_warn(dev, "crdump: create devlink region %s err %ld\n",
 			  region_cr_space_str,
@@ -218,7 +218,7 @@ int mlx4_crdump_init(struct mlx4_dev *dev)
 		devlink_region_create(devlink,
 				      region_fw_health_str,
 				      MAX_NUM_OF_DUMPS_TO_STORE,
-				      HEALTH_BUFFER_SIZE);
+				      HEALTH_BUFFER_SIZE, NULL);
 	if (IS_ERR(crdump->region_fw_health))
 		mlx4_warn(dev, "crdump: create devlink region %s err %ld\n",
 			  region_fw_health_str,
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 4b39aba2e9c4..2af97eeb7ba1 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -248,7 +248,8 @@ static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
 	nsim_dev->dummy_region =
 		devlink_region_create(devlink, "dummy",
 				      NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
-				      NSIM_DEV_DUMMY_REGION_SIZE);
+				      NSIM_DEV_DUMMY_REGION_SIZE,
+				      NULL);
 	return PTR_ERR_OR_ZERO(nsim_dev->dummy_region);
 }
 
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 38b4acb93f74..f93b1a07c9f2 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -491,6 +491,10 @@ struct devlink_info_req;
 
 typedef void devlink_snapshot_data_dest_t(const void *data);
 
+typedef int devlink_trigger_snapshot_t(struct devlink *devlink,
+				       struct devlink_region *region,
+				       struct netlink_ext_ack *extack);
+
 struct devlink_fmsg;
 struct devlink_health_reporter;
 
@@ -933,10 +937,10 @@ void devlink_port_param_value_changed(struct devlink_port *devlink_port,
 				      u32 param_id);
 void devlink_param_value_str_fill(union devlink_param_value *dst_val,
 				  const char *src);
-struct devlink_region *devlink_region_create(struct devlink *devlink,
-					     const char *region_name,
-					     u32 region_max_snapshots,
-					     u64 region_size);
+struct devlink_region *
+devlink_region_create(struct devlink *devlink, const char *region_name,
+		      u32 region_max_snapshots, u64 region_size,
+		      devlink_trigger_snapshot_t *trigger_snapshot);
 void devlink_region_destroy(struct devlink_region *region);
 u32 devlink_region_snapshot_id_get(struct devlink *devlink);
 int devlink_region_snapshot_create(struct devlink_region *region,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index b6fc67dbd612..e54600afdaf0 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -349,6 +349,7 @@ struct devlink_region {
 	u32 max_snapshots;
 	u32 cur_snapshots;
 	u64 size;
+	devlink_trigger_snapshot_t *trigger_snapshot;
 };
 
 struct devlink_snapshot {
@@ -7499,11 +7500,12 @@ EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
  *	@region_name: region name
  *	@region_max_snapshots: Maximum supported number of snapshots for region
  *	@region_size: size of region
+ *	@trigger_snapshot: function to trigger creation of snapshot
  */
-struct devlink_region *devlink_region_create(struct devlink *devlink,
-					     const char *region_name,
-					     u32 region_max_snapshots,
-					     u64 region_size)
+struct devlink_region *
+devlink_region_create(struct devlink *devlink, const char *region_name,
+		      u32 region_max_snapshots, u64 region_size,
+		      devlink_trigger_snapshot_t *trigger_snapshot)
 {
 	struct devlink_region *region;
 	int err = 0;
@@ -7525,6 +7527,7 @@ struct devlink_region *devlink_region_create(struct devlink *devlink,
 	region->max_snapshots = region_max_snapshots;
 	region->name = region_name;
 	region->size = region_size;
+	region->trigger_snapshot = trigger_snapshot;
 	INIT_LIST_HEAD(&region->snapshot_list);
 	list_add_tail(&region->list, &devlink->region_list);
 	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
-- 
2.25.0.rc1

[PATCH v2 1/1] devlink: add support for DEVLINK_CMD_REGION_TRIGGER

[Jacob Keller]

Add support for the devlink command to trigger a snapshot if the region
supports it.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 devlink/devlink.c            | 20 ++++++++++++++++++++
 include/uapi/linux/devlink.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 95f05a0b..3a473531 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -4200,6 +4200,7 @@ static const char *cmd_obj(uint8_t cmd)
 	case DEVLINK_CMD_REGION_SET:
 	case DEVLINK_CMD_REGION_NEW:
 	case DEVLINK_CMD_REGION_DEL:
+	case DEVLINK_CMD_REGION_TRIGGER:
 		return "region";
 	case DEVLINK_CMD_FLASH_UPDATE:
 	case DEVLINK_CMD_FLASH_UPDATE_END:
@@ -6362,12 +6363,28 @@ static int cmd_region_read(struct dl *dl)
 	return err;
 }
 
+static int cmd_region_trigger(struct dl *dl)
+{
+	struct nlmsghdr *nlh;
+	int err;
+
+	nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_REGION_TRIGGER,
+			NLM_F_REQUEST | NLM_F_ACK);
+
+	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE_REGION, 0);
+	if (err)
+		return err;
+
+	return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
+}
+
 static void cmd_region_help(void)
 {
 	pr_err("Usage: devlink region show [ DEV/REGION ]\n");
 	pr_err("       devlink region del DEV/REGION snapshot SNAPSHOT_ID\n");
 	pr_err("       devlink region dump DEV/REGION [ snapshot SNAPSHOT_ID ]\n");
 	pr_err("       devlink region read DEV/REGION [ snapshot SNAPSHOT_ID ] address ADDRESS length LENGTH\n");
+	pr_err("       devlink region trigger DEV/REGION\n");
 }
 
 static int cmd_region(struct dl *dl)
@@ -6389,6 +6406,9 @@ static int cmd_region(struct dl *dl)
 	} else if (dl_argv_match(dl, "read")) {
 		dl_arg_inc(dl);
 		return cmd_region_read(dl);
+	} else if (dl_argv_match(dl, "trigger")) {
+		dl_arg_inc(dl);
+		return cmd_region_trigger(dl);
 	}
 	pr_err("Command \"%s\" not found\n", dl_argv(dl));
 	return -ENOENT;
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 3f82dedd..37348f84 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -117,6 +117,8 @@ enum devlink_command {
 	DEVLINK_CMD_TRAP_GROUP_NEW,
 	DEVLINK_CMD_TRAP_GROUP_DEL,
 
+	DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
-- 
2.21.0

[PATCH v2 2/3] devlink: introduce command to trigger region snapshot

[Jacob Keller]

Devlink regions exist as a mechanism for drivers to export addressable
regions of data to userspace. An astute reviewer might notice that the
devlink-health interface is similar and could be used for such
a purpose. However, the health API is intended for reporting and
recovering from error conditions.

If a driver wants to export a set of data that is not an error
condition, it does not make much sense to use the health APIs. For
example, a driver might expose some portion of the flash memory contents
of the device as a region that could then be dumped. In this case, the
only time it makes sense to capture the region is upon request.

The health API makes more sense for cases where the driver detects error
conditions and triggers dumping and recovery mechanisms automatically.

Add a new command, DEVLINK_CMD_REGION_TRIGGER, which enables userspace
to request a snapshot of a region. A future commit will modify the
netdevsim driver as an example of how this is implemented.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 include/uapi/linux/devlink.h |  2 ++
 net/core/devlink.c           | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index ae37fd4d194a..a5f54953e7b2 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -117,6 +117,8 @@ enum devlink_command {
 	DEVLINK_CMD_TRAP_GROUP_NEW,
 	DEVLINK_CMD_TRAP_GROUP_DEL,
 
+	DEVLINK_CMD_REGION_TRIGGER,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index e54600afdaf0..3b5151a9b7db 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4053,6 +4053,32 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	return err;
 }
 
+static int devlink_nl_cmd_region_trigger_snapshot(struct sk_buff *skb,
+						  struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_region *region;
+	const char *region_name;
+	struct sk_buff *msg;
+	u32 snapshot_id;
+	int err;
+
+	if (!info->attrs[DEVLINK_ATTR_REGION_NAME])
+		return -EINVAL;
+
+	region_name = nla_data(info->attrs[DEVLINK_ATTR_REGION_NAME]);
+	region = devlink_region_get_by_name(devlink, region_name);
+	if (!region)
+		return -EINVAL;
+
+	if (!region->trigger_snapshot) {
+		NL_SET_ERR_MSG_MOD(info->extack, "Triggering snapshots for the requested region is not supported");
+		return -EOPNOTSUPP;
+	}
+
+	return region->trigger_snapshot(devlink, region, info->extack);
+}
+
 struct devlink_info_req {
 	struct sk_buff *msg;
 };
@@ -6159,6 +6185,14 @@ static const struct genl_ops devlink_nl_ops[] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
 	},
+	{
+		.cmd = DEVLINK_CMD_REGION_TRIGGER,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = devlink_nl_cmd_region_trigger_snapshot,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK |
+				  DEVLINK_NL_FLAG_NO_LOCK,
+	},
 	{
 		.cmd = DEVLINK_CMD_INFO_GET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-- 
2.25.0.rc1

[PATCH v2 3/3] netdevsim: support triggering snapshot through devlink

[Jacob Keller]

Implement the trigger_snapshot callback for the dummy devlink region.
This enables the region snapshot to be requested directly through the
devlink API instead of using debugfs as an out-of-band mechanism for
triggering the snapshot.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/netdevsim/dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 2af97eeb7ba1..4ed9c8d8de7c 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -38,30 +38,46 @@ static struct dentry *nsim_dev_ddir;
 
 #define NSIM_DEV_DUMMY_REGION_SIZE (1024 * 32)
 
-static ssize_t nsim_dev_take_snapshot_write(struct file *file,
-					    const char __user *data,
-					    size_t count, loff_t *ppos)
+static int nsim_dev_trigger_snapshot(struct devlink *devlink,
+				     struct devlink_region *region,
+				     struct netlink_ext_ack *extack)
 {
-	struct nsim_dev *nsim_dev = file->private_data;
 	void *dummy_data;
 	int err;
 	u32 id;
 
 	dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
-	if (!dummy_data)
+	if (!dummy_data) {
+		NL_SET_ERR_MSG(extack, "Out of memory");
 		return -ENOMEM;
+	}
 
 	get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
 
-	id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
-	err = devlink_region_snapshot_create(nsim_dev->dummy_region,
-					     dummy_data, id, kfree);
+	id = devlink_region_snapshot_id_get(devlink);
+	err = devlink_region_snapshot_create(region, dummy_data, id, kfree);
 	if (err) {
-		pr_err("Failed to create region snapshot\n");
+		NL_SET_ERR_MSG(extack, "Failed to create region snapshot");
 		kfree(dummy_data);
 		return err;
 	}
 
+	return (0);
+
+}
+
+static ssize_t nsim_dev_take_snapshot_write(struct file *file,
+					    const char __user *data,
+					    size_t count, loff_t *ppos)
+{
+	struct nsim_dev *nsim_dev = file->private_data;
+	int err;
+
+	err = nsim_dev_trigger_snapshot(priv_to_devlink(nsim_dev),
+					nsim_dev->dummy_region, NULL);
+	if (err)
+		return err;
+
 	return count;
 }
 
@@ -249,7 +265,7 @@ static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
 		devlink_region_create(devlink, "dummy",
 				      NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
 				      NSIM_DEV_DUMMY_REGION_SIZE,
-				      NULL);
+				      nsim_dev_trigger_snapshot);
 	return PTR_ERR_OR_ZERO(nsim_dev->dummy_region);
 }
 
-- 
2.25.0.rc1

Re: [PATCH v2 0/3] devlink region trigger support

[Jacob Keller]

On 1/9/2020 11:33 AM, Jacob Keller wrote:
> This series consists of patches to enable devlink to request a snapshot via
> a new DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT.
> 
> A reviewer might notice that the devlink health API already has such support
> for handling a similar case. However, the health API does not make sense in
> cases where the data is not related to an error condition.
> 
> In this case, using the health API only for the dumping feels incorrect.
> Regions make sense when the addressable content is not captured
> automatically on error conditions, but only upon request by the devlink API.
> 
> The netdevsim driver is modified to support the new trigger_snapshot
> callback as an example of how this can be used.
> 

As mentioned by Jakub on an earlier comment, I wanted to clarify: I
implemented this in the netdevsim driver because I wanted to test that
the changes actually worked as expected.

I'm planning on making use of this in an Intel driver soon, but do not
yet have patches ready to send to the list.

Thanks,
Jake

> Jacob Keller (3):
>   devlink: add callback to trigger region snapshots
>   devlink: introduce command to trigger region snapshot
>   netdevsim: support triggering snapshot through devlink
> 
>  drivers/net/ethernet/mellanox/mlx4/crdump.c |  4 +-
>  drivers/net/netdevsim/dev.c                 | 37 ++++++++++++-----
>  include/net/devlink.h                       | 12 ++++--
>  include/uapi/linux/devlink.h                |  2 +
>  net/core/devlink.c                          | 45 +++++++++++++++++++--
>  5 files changed, 80 insertions(+), 20 deletions(-)
> 

Re: [PATCH v2 0/3] devlink region trigger support

[Yunsheng Lin]

On 2020/1/10 3:33, Jacob Keller wrote:
> This series consists of patches to enable devlink to request a snapshot via
> a new DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT.
> 
> A reviewer might notice that the devlink health API already has such support
> for handling a similar case. However, the health API does not make sense in
> cases where the data is not related to an error condition.

Maybe we need to specify the usecases for the region trigger as suggested by
Jacob.

For example, the orginal usecase is to expose some set of flash/NVM contents.
But can it be used to dump the register of the bar space? or some binary
table in the hardware to debug some error that is not detected by hw?

> 
> In this case, using the health API only for the dumping feels incorrect.
> Regions make sense when the addressable content is not captured
> automatically on error conditions, but only upon request by the devlink API.
> 
> The netdevsim driver is modified to support the new trigger_snapshot
> callback as an example of how this can be used.
> 
> Jacob Keller (3):
>   devlink: add callback to trigger region snapshots
>   devlink: introduce command to trigger region snapshot
>   netdevsim: support triggering snapshot through devlink
> 
>  drivers/net/ethernet/mellanox/mlx4/crdump.c |  4 +-
>  drivers/net/netdevsim/dev.c                 | 37 ++++++++++++-----
>  include/net/devlink.h                       | 12 ++++--
>  include/uapi/linux/devlink.h                |  2 +
>  net/core/devlink.c                          | 45 +++++++++++++++++++--
>  5 files changed, 80 insertions(+), 20 deletions(-)
> 

Re: [PATCH v2 0/3] devlink region trigger support

[Jacob Keller]

On 1/9/2020 8:10 PM, Yunsheng Lin wrote:
> On 2020/1/10 3:33, Jacob Keller wrote:
>> This series consists of patches to enable devlink to request a snapshot via
>> a new DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT.
>>
>> A reviewer might notice that the devlink health API already has such support
>> for handling a similar case. However, the health API does not make sense in
>> cases where the data is not related to an error condition.
> 
> Maybe we need to specify the usecases for the region trigger as suggested by
> Jacob.
> 
> For example, the orginal usecase is to expose some set of flash/NVM contents.
> But can it be used to dump the register of the bar space? or some binary
> table in the hardware to debug some error that is not detected by hw?
> 


regions can essentially be used to dump arbitrary addressable content. I
think all of the above are great examples.

I have a series of patches to update and convert the devlink
documentation, and I do provide some further detail in the new
devlink-region.rst file.

Perhaps you could review that and provide suggestions on what would make
sense to add there?

https://lore.kernel.org/netdev/20200109224625.1470433-13-jacob.e.keller@intel.com/

Thanks,
Jake

Re: [PATCH v2 0/3] devlink region trigger support

[Yunsheng Lin]

On 2020/1/11 1:52, Jacob Keller wrote:
> On 1/9/2020 8:10 PM, Yunsheng Lin wrote:
>> On 2020/1/10 3:33, Jacob Keller wrote:
>>> This series consists of patches to enable devlink to request a snapshot via
>>> a new DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT.
>>>
>>> A reviewer might notice that the devlink health API already has such support
>>> for handling a similar case. However, the health API does not make sense in
>>> cases where the data is not related to an error condition.
>>
>> Maybe we need to specify the usecases for the region trigger as suggested by
>> Jacob.
>>
>> For example, the orginal usecase is to expose some set of flash/NVM contents.
>> But can it be used to dump the register of the bar space? or some binary
>> table in the hardware to debug some error that is not detected by hw?
>>
> 
> 
> regions can essentially be used to dump arbitrary addressable content. I
> think all of the above are great examples.
> 
> I have a series of patches to update and convert the devlink
> documentation, and I do provide some further detail in the new
> devlink-region.rst file.
> 
> Perhaps you could review that and provide suggestions on what would make
> sense to add there?

For the case of region for mlx4, I am not sure it worths the effort to
document it, because Jiri has mention that there was plan to convert mlx4 to
use "devlink health" api for the above case.

Also, there is dpipe, health and region api:
For health and region, they seems similar to me, and the non-essential
difference is:
1. health can be used used to dump content of tlv style, and can be triggered
   by driver automatically or by user manually.

2. region can be used to dump binary content and can be triggered by driver
   automatically only.

It would be good to merged the above to the same api(perhaps merge the binary
content dumping of region api to health api), then we can resue the same dump
ops for both driver and user triggering case.

For dpipe, it does not seems flexible enough to dump a table, yes, it provides
better visibility, but I am not sure it worth the effort, also, it would be better
to share the same table dump ops for driver and user triggering case.
For hns3 driver, we may have mac, vlan and flow director table that may need to dump
in both driver and user triggering case to debug some complex issues.

So It would be better to be able to dump table(maybe including binary table), binary
content and tlv content for a sinle api, I suppose the health api is the one to do
that? because health api has already supported driver and user triggering case, and
only need to add the table and binary content dumpping.


> 
> https://lore.kernel.org/netdev/20200109224625.1470433-13-jacob.e.keller@intel.com/
> 
> Thanks,
> Jake
> 
> .
> 

Re: [PATCH v2 0/3] devlink region trigger support

[Jakub Kicinski]

On Sat, 11 Jan 2020 09:51:00 +0800, Yunsheng Lin wrote:
> > regions can essentially be used to dump arbitrary addressable content. I
> > think all of the above are great examples.
> > 
> > I have a series of patches to update and convert the devlink
> > documentation, and I do provide some further detail in the new
> > devlink-region.rst file.
> > 
> > Perhaps you could review that and provide suggestions on what would make
> > sense to add there?  
> 
> For the case of region for mlx4, I am not sure it worths the effort to
> document it, because Jiri has mention that there was plan to convert mlx4 to
> use "devlink health" api for the above case.
> 
> Also, there is dpipe, health and region api:
> For health and region, they seems similar to me, and the non-essential
> difference is:
> 1. health can be used used to dump content of tlv style, and can be triggered
>    by driver automatically or by user manually.
> 
> 2. region can be used to dump binary content and can be triggered by driver
>    automatically only.
> 
> It would be good to merged the above to the same api(perhaps merge the binary
> content dumping of region api to health api), then we can resue the same dump
> ops for both driver and user triggering case.

I think there is a fundamental difference between health API and
regions in the fact that health reporters allow for returning
structured data from different sources which are associated with 
an event/error condition. That includes information read from the
hardware or driver/software state. Region API (as Jake said) is good
for dumping arbitrary addressable content, e.g. registers. I don't see
much use for merging the two right now, FWIW...

Re: [PATCH v2 0/3] devlink region trigger support

[Alex Vesker]

On 1/12/2020 10:45 PM, Jakub Kicinski wrote:
> On Sat, 11 Jan 2020 09:51:00 +0800, Yunsheng Lin wrote:
>>> regions can essentially be used to dump arbitrary addressable content. I
>>> think all of the above are great examples.
>>>
>>> I have a series of patches to update and convert the devlink
>>> documentation, and I do provide some further detail in the new
>>> devlink-region.rst file.
>>>
>>> Perhaps you could review that and provide suggestions on what would make
>>> sense to add there?  
>> For the case of region for mlx4, I am not sure it worths the effort to
>> document it, because Jiri has mention that there was plan to convert mlx4 to
>> use "devlink health" api for the above case.
>>
>> Also, there is dpipe, health and region api:
>> For health and region, they seems similar to me, and the non-essential
>> difference is:
>> 1. health can be used used to dump content of tlv style, and can be triggered
>>    by driver automatically or by user manually.
>>
>> 2. region can be used to dump binary content and can be triggered by driver
>>    automatically only.
>>
>> It would be good to merged the above to the same api(perhaps merge the binary
>> content dumping of region api to health api), then we can resue the same dump
>> ops for both driver and user triggering case.
> I think there is a fundamental difference between health API and
> regions in the fact that health reporters allow for returning
> structured data from different sources which are associated with 
> an event/error condition. That includes information read from the
> hardware or driver/software state. Region API (as Jake said) is good
> for dumping arbitrary addressable content, e.g. registers. I don't see
> much use for merging the two right now, FWIW...
>
Totally agree with Jakub, I think health and region are different and
each has its
usages as mentioned above. Using words such as recovery and health for
exposing
registers doesn't sound correct. There are future usages I can think of
for region if we
will add the trigger support as well as the live region read.

Re: [PATCH v2 0/3] devlink region trigger support

[Yunsheng Lin]

On 2020/1/13 5:18, Alex Vesker wrote:
> On 1/12/2020 10:45 PM, Jakub Kicinski wrote:
>> On Sat, 11 Jan 2020 09:51:00 +0800, Yunsheng Lin wrote:
>>>> regions can essentially be used to dump arbitrary addressable content. I
>>>> think all of the above are great examples.
>>>>
>>>> I have a series of patches to update and convert the devlink
>>>> documentation, and I do provide some further detail in the new
>>>> devlink-region.rst file.
>>>>
>>>> Perhaps you could review that and provide suggestions on what would make
>>>> sense to add there?  
>>> For the case of region for mlx4, I am not sure it worths the effort to
>>> document it, because Jiri has mention that there was plan to convert mlx4 to
>>> use "devlink health" api for the above case.
>>>
>>> Also, there is dpipe, health and region api:
>>> For health and region, they seems similar to me, and the non-essential
>>> difference is:
>>> 1. health can be used used to dump content of tlv style, and can be triggered
>>>    by driver automatically or by user manually.
>>>
>>> 2. region can be used to dump binary content and can be triggered by driver
>>>    automatically only.
>>>
>>> It would be good to merged the above to the same api(perhaps merge the binary
>>> content dumping of region api to health api), then we can resue the same dump
>>> ops for both driver and user triggering case.
>> I think there is a fundamental difference between health API and
>> regions in the fact that health reporters allow for returning
>> structured data from different sources which are associated with 
>> an event/error condition. That includes information read from the
>> hardware or driver/software state. Region API (as Jake said) is good
>> for dumping arbitrary addressable content, e.g. registers. I don't see
>> much use for merging the two right now, FWIW...

The point is that we are beginning to use health API for event/error
condition, right? Do we use health API or regions API to dump a arbitrary
addressable content when there is an event/error detected?

Also we may need to dump both a arbitrary addressable content and the
structured data when there is an event/error detected, the health API or
regions API can not do both, right?

It still seems it is a little confusing when deciding to use health or
regions API.

>>
> Totally agree with Jakub, I think health and region are different and
> each has its
> usages as mentioned above. Using words such as recovery and health for
> exposing
> registers doesn't sound correct. There are future usages I can think of
> for region if we
> will add the trigger support as well as the live region read.

health API already has "trigger support" now if region API is merged to
health API.

I am not sure I understand "live region" here, what is the usecase of live
region?

> 
> 
> 

Re: [PATCH v2 0/3] devlink region trigger support

[Jakub Kicinski]

On Mon, 13 Jan 2020 09:39:50 +0800, Yunsheng Lin wrote:
> On 2020/1/13 5:18, Alex Vesker wrote:
> > On 1/12/2020 10:45 PM, Jakub Kicinski wrote:  
> >> I think there is a fundamental difference between health API and
> >> regions in the fact that health reporters allow for returning
> >> structured data from different sources which are associated with 
> >> an event/error condition. That includes information read from the
> >> hardware or driver/software state. Region API (as Jake said) is good
> >> for dumping arbitrary addressable content, e.g. registers. I don't see
> >> much use for merging the two right now, FWIW...  
> 
> The point is that we are beginning to use health API for event/error
> condition, right? Do we use health API or regions API to dump a arbitrary
> addressable content when there is an event/error detected?
> 
> Also we may need to dump both a arbitrary addressable content and the

If the information dumped is pertinent in the context of a health event
it's not arbitrary.

> structured data when there is an event/error detected, the health API or
> regions API can not do both, right?

I think for errors and events health API will be more suitable 99% of
the time.

> It still seems it is a little confusing when deciding to use health or
> regions API.
>
> > Totally agree with Jakub, I think health and region are different and
> > each has its
> > usages as mentioned above. Using words such as recovery and health for
> > exposing
> > registers doesn't sound correct. There are future usages I can think of
> > for region if we
> > will add the trigger support as well as the live region read.  
> 
> health API already has "trigger support" now if region API is merged to
> health API.
> 
> I am not sure I understand "live region" here, what is the usecase of live
> region?

Reading registers of a live system without copying them to a snapshot
first. Some chips have so many registers it's impractical to group them
beyond "registers of IP block X", if that. IMHO that fits nicely with
regions, health is grouped by event, so we'd likely want to dump for
example one or two registers from the MAC there, while the entire set
of MAC registers can be exposed as a region.

Re: [PATCH v2 0/3] devlink region trigger support

[Jacob Keller]

On 1/13/2020 3:34 AM, Jakub Kicinski wrote:
> On Mon, 13 Jan 2020 09:39:50 +0800, Yunsheng Lin wrote:
>> I am not sure I understand "live region" here, what is the usecase of live
>> region?
> 
> Reading registers of a live system without copying them to a snapshot
> first. Some chips have so many registers it's impractical to group them
> beyond "registers of IP block X", if that. IMHO that fits nicely with
> regions, health is grouped by event, so we'd likely want to dump for
> example one or two registers from the MAC there, while the entire set
> of MAC registers can be exposed as a region.
> 

Right. I'm actually wondering about this as well. Region snapshots are
captured in whole and stored and then returned through the devlink
region commands.

This could be problematic if you wanted to expose a larger chunk of
registers or addressable sections of flash contents, as the size of the
contents goes beyond a single page.

If we instead focus regions onto the live-read aspect, the API can
simply be a request to read a segment of the region. Then, the driver
could perform the read of that chunk and report it back.

Re: [PATCH v2 0/3] devlink region trigger support

[Jiri Pirko]

Mon, Jan 13, 2020 at 07:16:51PM CET, jacob.e.keller@intel.com wrote:
>On 1/13/2020 3:34 AM, Jakub Kicinski wrote:
>> On Mon, 13 Jan 2020 09:39:50 +0800, Yunsheng Lin wrote:
>>> I am not sure I understand "live region" here, what is the usecase of live
>>> region?
>> 
>> Reading registers of a live system without copying them to a snapshot
>> first. Some chips have so many registers it's impractical to group them
>> beyond "registers of IP block X", if that. IMHO that fits nicely with
>> regions, health is grouped by event, so we'd likely want to dump for
>> example one or two registers from the MAC there, while the entire set
>> of MAC registers can be exposed as a region.
>> 
>
>Right. I'm actually wondering about this as well. Region snapshots are
>captured in whole and stored and then returned through the devlink
>region commands.

Well, driver does not have to support snapshots for particular region,
only live reading. Yes, this needs to be implemented.

>
>This could be problematic if you wanted to expose a larger chunk of
>registers or addressable sections of flash contents, as the size of the
>contents goes beyond a single page.
>
>If we instead focus regions onto the live-read aspect, the API can
>simply be a request to read a segment of the region. Then, the driver
>could perform the read of that chunk and report it back.

Yep.

Re: [PATCH v2 0/3] devlink region trigger support

[Jiri Pirko]

Sat, Jan 11, 2020 at 02:51:00AM CET, linyunsheng@huawei.com wrote:
>On 2020/1/11 1:52, Jacob Keller wrote:
>> On 1/9/2020 8:10 PM, Yunsheng Lin wrote:
>>> On 2020/1/10 3:33, Jacob Keller wrote:
>>>> This series consists of patches to enable devlink to request a snapshot via
>>>> a new DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT.
>>>>
>>>> A reviewer might notice that the devlink health API already has such support
>>>> for handling a similar case. However, the health API does not make sense in
>>>> cases where the data is not related to an error condition.
>>>
>>> Maybe we need to specify the usecases for the region trigger as suggested by
>>> Jacob.
>>>
>>> For example, the orginal usecase is to expose some set of flash/NVM contents.
>>> But can it be used to dump the register of the bar space? or some binary
>>> table in the hardware to debug some error that is not detected by hw?
>>>
>> 
>> 
>> regions can essentially be used to dump arbitrary addressable content. I
>> think all of the above are great examples.
>> 
>> I have a series of patches to update and convert the devlink
>> documentation, and I do provide some further detail in the new
>> devlink-region.rst file.
>> 
>> Perhaps you could review that and provide suggestions on what would make
>> sense to add there?
>
>For the case of region for mlx4, I am not sure it worths the effort to
>document it, because Jiri has mention that there was plan to convert mlx4 to
>use "devlink health" api for the above case.

It is on the TODO list, yes. For mlx4 usecase the healh reporters are
more suitable.


>
>Also, there is dpipe, health and region api:
>For health and region, they seems similar to me, and the non-essential
>difference is:
>1. health can be used used to dump content of tlv style, and can be triggered
>   by driver automatically or by user manually.
>
>2. region can be used to dump binary content and can be triggered by driver
>   automatically only.
>
>It would be good to merged the above to the same api(perhaps merge the binary
>content dumping of region api to health api), then we can resue the same dump
>ops for both driver and user triggering case.

I was thinking about that as well. Will check it out.

>
>For dpipe, it does not seems flexible enough to dump a table, yes, it provides

Why? That is the purpose of the dpipe, but make the hw
pipeline visible and show you the content of individual nodes.


>better visibility, but I am not sure it worth the effort, also, it would be better
>to share the same table dump ops for driver and user triggering case.
>For hns3 driver, we may have mac, vlan and flow director table that may need to dump
>in both driver and user triggering case to debug some complex issues.
>
>So It would be better to be able to dump table(maybe including binary table), binary
>content and tlv content for a sinle api, I suppose the health api is the one to do
>that? because health api has already supported driver and user triggering case, and
>only need to add the table and binary content dumpping.
>
>
>> 
>> https://lore.kernel.org/netdev/20200109224625.1470433-13-jacob.e.keller@intel.com/
>> 
>> Thanks,
>> Jake
>> 
>> .
>> 
>

Re: [PATCH v2 0/3] devlink region trigger support

[Jacob Keller]

On 1/13/2020 8:58 AM, Jiri Pirko wrote:
> Why? That is the purpose of the dpipe, but make the hw
> pipeline visible and show you the content of individual nodes.
> 

I agree. dpipe seems to be focused specifically on dumping nodes of the
tables that represent the hardware's pipeline. I think it's unrelated to
this discussion about regions vs health API.

Re: [PATCH v2 0/3] devlink region trigger support

[Jiri Pirko]

Mon, Jan 13, 2020 at 07:22:57PM CET, jacob.e.keller@intel.com wrote:
>
>
>On 1/13/2020 8:58 AM, Jiri Pirko wrote:
>> Why? That is the purpose of the dpipe, but make the hw
>> pipeline visible and show you the content of individual nodes.
>> 
>
>I agree. dpipe seems to be focused specifically on dumping nodes of the
>tables that represent the hardware's pipeline. I think it's unrelated to
>this discussion about regions vs health API.

Nod.

Re: [PATCH v2 0/3] devlink region trigger support

[Yunsheng Lin]

On 2020/1/14 2:22, Jacob Keller wrote:
> 
> 
> On 1/13/2020 8:58 AM, Jiri Pirko wrote:
>> Why? That is the purpose of the dpipe, but make the hw
>> pipeline visible and show you the content of individual nodes.
>>
> 
> I agree. dpipe seems to be focused specifically on dumping nodes of the
> tables that represent the hardware's pipeline. I think it's unrelated to
> this discussion about regions vs health API.

Sorry for bringing up a not really unrelated question in the thread,

For the hns3 hw mac table, it seems the hns3 hw is pretty simple, it mainly
contain the port bitmaps of a mac address, then the hw can forward the packet
based on the dst mac' port bitamp.

It seems a litte hard to match to the dpipe API the last time I tried to
use dpipe API to dump that.

So maybe it would be good to have the support of table dumping (both structured
and binary table) for health API natively, so that we use it to dump some hw
table for both driver and user triggering cases.

I am not sure if other driver has the above requirement, and if the requirement
makes any sense?

> 
> 

Re: [PATCH v2 0/3] devlink region trigger support

[Jacob Keller]

On 1/14/2020 12:33 AM, Yunsheng Lin wrote:
> On 2020/1/14 2:22, Jacob Keller wrote:
>>
>>
>> On 1/13/2020 8:58 AM, Jiri Pirko wrote:
>>> Why? That is the purpose of the dpipe, but make the hw
>>> pipeline visible and show you the content of individual nodes.
>>>
>>
>> I agree. dpipe seems to be focused specifically on dumping nodes of the
>> tables that represent the hardware's pipeline. I think it's unrelated to
>> this discussion about regions vs health API.
> 
> Sorry for bringing up a not really unrelated question in the thread,
> 

No problem here :) I've been investigating devlink for our products, and
am working towards implementations for several features. Further
discussion is definitely welcome!

> For the hns3 hw mac table, it seems the hns3 hw is pretty simple, it mainly
> contain the port bitmaps of a mac address, then the hw can forward the packet
> based on the dst mac' port bitamp.
> 

Right.

> It seems a litte hard to match to the dpipe API the last time I tried to
> use dpipe API to dump that.

dpipe is primarily targeted towards dumping complex hardware pipelines.

> 
> So maybe it would be good to have the support of table dumping (both structured
> and binary table) for health API natively, so that we use it to dump some hw
> table for both driver and user triggering cases.
> 

Maybe dpipe needs additional mechanism for presenting tables?

> I am not sure if other driver has the above requirement, and if the requirement
> makes any sense?
> 

I think there's value in the ability to express this kind of contents.
Regions could work.

In regards to using devlink-health, I do believe the health API should
remain focused as the area you use for dumping data gathered
specifically in relation to an event such as an error condition.

Regions make sense when you want to allow access to a section of
addressable contents on demand.

A binary table could be dumped via a region, but I'm not 100% sure it
would make sense for structured tables.

Thanks,
Jake

Re: [PATCH v2 0/3] devlink region trigger support

[Yunsheng Lin]

On 2020/1/15 4:04, Jacob Keller wrote:
> On 1/14/2020 12:33 AM, Yunsheng Lin wrote:
>> On 2020/1/14 2:22, Jacob Keller wrote:
>>>
>>>
>>> On 1/13/2020 8:58 AM, Jiri Pirko wrote:
>>>> Why? That is the purpose of the dpipe, but make the hw
>>>> pipeline visible and show you the content of individual nodes.
>>>>
>>>
>>> I agree. dpipe seems to be focused specifically on dumping nodes of the
>>> tables that represent the hardware's pipeline. I think it's unrelated to
>>> this discussion about regions vs health API.
>>
>> Sorry for bringing up a not really unrelated question in the thread,
>>
> 
> No problem here :) I've been investigating devlink for our products, and
> am working towards implementations for several features. Further
> discussion is definitely welcome!
> 
>> For the hns3 hw mac table, it seems the hns3 hw is pretty simple, it mainly
>> contain the port bitmaps of a mac address, then the hw can forward the packet
>> based on the dst mac' port bitamp.
>>
> 
> Right.
> 
>> It seems a litte hard to match to the dpipe API the last time I tried to
>> use dpipe API to dump that.
> 
> dpipe is primarily targeted towards dumping complex hardware pipelines.
> 
>>
>> So maybe it would be good to have the support of table dumping (both structured
>> and binary table) for health API natively, so that we use it to dump some hw
>> table for both driver and user triggering cases.
>>
> 
> Maybe dpipe needs additional mechanism for presenting tables?
> 
>> I am not sure if other driver has the above requirement, and if the requirement
>> makes any sense?
>>
> 
> I think there's value in the ability to express this kind of contents.
> Regions could work.
> 
> In regards to using devlink-health, I do believe the health API should
> remain focused as the area you use for dumping data gathered
> specifically in relation to an event such as an error condition.

what if there are requirements to dump the same data for both error/event
triggering and user cmd triggering case? If we implement a dump ops of
region API for user cmd triggering case, and then implement a similar
dump ops of health API for error/event case, does this not seems unnecessary
duplication of effort?

For user cmd triggering case, it is used to inspect the device' health state
mostly, so maybe it makes sense to use health API to dump that too.

> 
> Regions make sense when you want to allow access to a section of
> addressable contents on demand.
> 
> A binary table could be dumped via a region, but I'm not 100% sure it
> would make sense for structured tables.
> 
> Thanks,
> Jake
> 
> .
> 

Re: [PATCH v2 0/3] devlink region trigger support

[Jiri Pirko]

Thu, Jan 09, 2020 at 08:33:07PM CET, jacob.e.keller@intel.com wrote:
>This series consists of patches to enable devlink to request a snapshot via
>a new DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT.
>
>A reviewer might notice that the devlink health API already has such support
>for handling a similar case. However, the health API does not make sense in
>cases where the data is not related to an error condition.
>
>In this case, using the health API only for the dumping feels incorrect.
>Regions make sense when the addressable content is not captured
>automatically on error conditions, but only upon request by the devlink API.
>
>The netdevsim driver is modified to support the new trigger_snapshot
>callback as an example of how this can be used.

I don't think that the netdevsim usecase is enough to merge this in. You
need a real-driver user as well.

Of course, netdevsim implementation is good to have to, but you have to
bundle selftests along with that.


>
>Jacob Keller (3):
>  devlink: add callback to trigger region snapshots
>  devlink: introduce command to trigger region snapshot
>  netdevsim: support triggering snapshot through devlink
>
> drivers/net/ethernet/mellanox/mlx4/crdump.c |  4 +-
> drivers/net/netdevsim/dev.c                 | 37 ++++++++++++-----
> include/net/devlink.h                       | 12 ++++--
> include/uapi/linux/devlink.h                |  2 +
> net/core/devlink.c                          | 45 +++++++++++++++++++--
> 5 files changed, 80 insertions(+), 20 deletions(-)
>
>-- 
>2.25.0.rc1
>

Re: [PATCH v2 0/3] devlink region trigger support

[Jacob Keller]

On 1/10/2020 1:40 AM, Jiri Pirko wrote:
> Thu, Jan 09, 2020 at 08:33:07PM CET, jacob.e.keller@intel.com wrote:
>> This series consists of patches to enable devlink to request a snapshot via
>> a new DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT.
>>
>> A reviewer might notice that the devlink health API already has such support
>> for handling a similar case. However, the health API does not make sense in
>> cases where the data is not related to an error condition.
>>
>> In this case, using the health API only for the dumping feels incorrect.
>> Regions make sense when the addressable content is not captured
>> automatically on error conditions, but only upon request by the devlink API.
>>
>> The netdevsim driver is modified to support the new trigger_snapshot
>> callback as an example of how this can be used.
> 
> I don't think that the netdevsim usecase is enough to merge this in. You
> need a real-driver user as well.
> 
Sure. But this direction and implementation seems reasonable?

I am making progress on a driver implementation for this, and I am fine
holding onto these patches until I am ready to submit the full series to
the list with the driver..

Mostly I wanted to make sure the direction was looking good earlier than
the time frame for completing that work.

Thanks,
Jake

Re: [PATCH v2 0/3] devlink region trigger support

[Jakub Kicinski]

On Fri, 10 Jan 2020 09:54:20 -0800, Jacob Keller wrote:
> On 1/10/2020 1:40 AM, Jiri Pirko wrote:
> > Thu, Jan 09, 2020 at 08:33:07PM CET, jacob.e.keller@intel.com wrote:  
> >> This series consists of patches to enable devlink to request a snapshot via
> >> a new DEVLINK_CMD_REGION_TRIGGER_SNAPSHOT.
> >>
> >> A reviewer might notice that the devlink health API already has such support
> >> for handling a similar case. However, the health API does not make sense in
> >> cases where the data is not related to an error condition.
> >>
> >> In this case, using the health API only for the dumping feels incorrect.
> >> Regions make sense when the addressable content is not captured
> >> automatically on error conditions, but only upon request by the devlink API.
> >>
> >> The netdevsim driver is modified to support the new trigger_snapshot
> >> callback as an example of how this can be used.  
> > 
> > I don't think that the netdevsim usecase is enough to merge this in. You
> > need a real-driver user as well.
> >   
> Sure. But this direction and implementation seems reasonable?
> 
> I am making progress on a driver implementation for this, and I am fine
> holding onto these patches until I am ready to submit the full series to
> the list with the driver..
> 
> Mostly I wanted to make sure the direction was looking good earlier than
> the time frame for completing that work.

As Jiri said, quite hard to tell without seeing the real user.

I presume you take some lock to ensure the contents of the snapshot are
atomic?  Otherwise I wonder if you wouldn't be better served by just
allowing region read to operate directly on the data rather then going
through the snapshot create -> read -> snapshot remove cycle. Not sure
Jiri would agree, it require more code.

For the patches themselves we may want to move the callbacks into some
ops structure in the region.  And I don't think you need to make the
trigger command NO_LOCK.

Re: [PATCH v2 0/3] devlink region trigger support

[Jacob Keller]

On 1/10/2020 10:47 AM, Jakub Kicinski wrote:
> On Fri, 10 Jan 2020 09:54:20 -0800, Jacob Keller wrote:
>> Mostly I wanted to make sure the direction was looking good earlier than
>> the time frame for completing that work.
> 
> As Jiri said, quite hard to tell without seeing the real user.
> 

Fair.

> I presume you take some lock to ensure the contents of the snapshot are
> atomic?  Otherwise I wonder if you wouldn't be better served by just
> allowing region read to operate directly on the data rather then going
> through the snapshot create -> read -> snapshot remove cycle. Not sure
> Jiri would agree, it require more code.
> 

Right. I saw the original devlink region commit messages indicated
possible plans to support writing to regions. There is also the question
of handling data that might want to be read sparsely, rather than
reading an entire snapshot at once. Hmm.

> For the patches themselves we may want to move the callbacks into some
> ops structure in the region.  And I don't think you need to make the
> trigger command NO_LOCK.
> 

So the reason it was made NO_LOCK right now is because the trigger ends
up calling devlink_region_snapshot_id_get and
devlink_region_snapshot_create which both take the devlink lock.

I suppose this could be simplified by having the snapshot callback take
similar parameters as the devlink_region_snapshot_create, and assume the
devlink core code does such things as generating an id and adding the
snapshot to the list all while holding the lock...

I will sort out these questions into what I think makes sense for the
use case I envision, and will submit the core devlink patches at the
same time as the driver changes.

Thanks,
Jake