diff mbox series

[10/12] ASoC: SOF: Provide probe debugfs support

Message ID 20200124190413.18154-11-cezary.rojewski@intel.com (mailing list archive)
State New, archived
Headers show
Series ASoC: SOF: Data probing | expand

Commit Message

Cezary Rojewski Jan. 24, 2020, 7:04 p.m. UTC
Define debugfs subdirectory delegated for IPC communitation with DSP.
Input format: uint,uint,(...) which are later translated into DWORDS
sequence and further into instances of struct of interest given the IPC
type.

For Extractor probes, following have been enabled:
- PROBE_POINT_ADD (echo <..> probe_points)
- PROBE_POINT_REMOVE (echo <..> probe_points_remove)
- PROBE_POINT_INFO (cat probe_points)

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/sof/debug.c | 208 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 208 insertions(+)

Comments

Pierre-Louis Bossart Jan. 24, 2020, 8:22 p.m. UTC | #1
You should probably add a higher-level explanation in the commit message 
that to make use of probes, the user needs to specific which buffers of 
the firmware topology they want to extract data from, and that debugfs 
is the configuration interface. The streaming part is handled via the 
compressed interface.

> Define debugfs subdirectory delegated for IPC communitation with DSP.

communication.

> Input format: uint,uint,(...) which are later translated into DWORDS
> sequence and further into instances of struct of interest given the IPC
> type.

we should probably add a documentation part that specifies the values 
expected, as you did some time back.

> For Extractor probes, following have been enabled:
> - PROBE_POINT_ADD (echo <..> probe_points)
> - PROBE_POINT_REMOVE (echo <..> probe_points_remove)
> - PROBE_POINT_INFO (cat probe_points)

Doesn't appear very intuitive to me, is this the same as in previous 
solutions or a new design of your own?

> +static ssize_t ppoints_read(struct file *file,
> +		char __user *to, size_t count, loff_t *ppos)

avoid ppoints acronym, use probe_points_read? same in the rest of the patch.
Cezary Rojewski Jan. 27, 2020, 12:31 p.m. UTC | #2
On 2020-01-24 21:22, Pierre-Louis Bossart wrote:
> You should probably add a higher-level explanation in the commit message 
> that to make use of probes, the user needs to specific which buffers of 
> the firmware topology they want to extract data from, and that debugfs 
> is the configuration interface. The streaming part is handled via the 
> compressed interface.
> 
>> Define debugfs subdirectory delegated for IPC communitation with DSP.
> 
> communication.
> 

Reworded in v2, thanks.

>> Input format: uint,uint,(...) which are later translated into DWORDS
>> sequence and further into instances of struct of interest given the IPC
>> type.
> 
> we should probably add a documentation part that specifies the values 
> expected, as you did some time back.
> 

Documentation will be released on a later date as SOF's probes are still 
a very fresh subject.

>> For Extractor probes, following have been enabled:
>> - PROBE_POINT_ADD (echo <..> probe_points)
>> - PROBE_POINT_REMOVE (echo <..> probe_points_remove)
>> - PROBE_POINT_INFO (cat probe_points)
> 
> Doesn't appear very intuitive to me, is this the same as in previous 
> solutions or a new design of your own?
> 

Precisely, the usage is exactly the same. Documentation covers the 
usage, and it is actually very intuitive (as it's very simple too) once 
you get chance to put your hands on it.

>> +static ssize_t ppoints_read(struct file *file,
>> +        char __user *to, size_t count, loff_t *ppos)
> 
> avoid ppoints acronym, use probe_points_read? same in the rest of the 
> patch.
> 

Reworded in v2, thanks.
diff mbox series

Patch

diff --git a/sound/soc/sof/debug.c b/sound/soc/sof/debug.c
index d2b3b99d3a20..e0fc0735b0d5 100644
--- a/sound/soc/sof/debug.c
+++ b/sound/soc/sof/debug.c
@@ -17,6 +17,203 @@ 
 #include "sof-priv.h"
 #include "ops.h"
 
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
+#include "probe.h"
+
+/**
+ * strsplit_u32 - Split string into sequence of u32 tokens
+ * @buf:	String to split into tokens.
+ * @delim:	String containing delimiter characters.
+ * @tkns:	Returned u32 sequence pointer.
+ * @num_tkns:	Returned number of tokens obtained.
+ */
+static int
+strsplit_u32(char **buf, const char *delim, u32 **tkns, size_t *num_tkns)
+{
+	char *s;
+	u32 *data, *tmp;
+	size_t count = 0;
+	size_t cap = 32;
+	int ret = 0;
+
+	*tkns = NULL;
+	*num_tkns = 0;
+	data = kcalloc(cap, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	while ((s = strsep(buf, delim)) != NULL) {
+		ret = kstrtouint(s, 0, data + count);
+		if (ret)
+			goto exit;
+		if (++count >= cap) {
+			cap *= 2;
+			tmp = krealloc(data, cap * sizeof(*data), GFP_KERNEL);
+			if (!tmp) {
+				ret = -ENOMEM;
+				goto exit;
+			}
+			data = tmp;
+		}
+	}
+
+	if (!count)
+		goto exit;
+	*tkns = kmemdup(data, count * sizeof(*data), GFP_KERNEL);
+	if (*tkns == NULL) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+	*num_tkns = count;
+
+exit:
+	kfree(data);
+	return ret;
+}
+
+static int tokenize_input(const char __user *from, size_t count,
+		loff_t *ppos, u32 **tkns, size_t *num_tkns)
+{
+	char *buf;
+	int ret;
+
+	buf = kmalloc(count + 1, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = simple_write_to_buffer(buf, count, ppos, from, count);
+	if (ret != count) {
+		ret = ret >= 0 ? -EIO : ret;
+		goto exit;
+	}
+
+	buf[count] = '\0';
+	ret = strsplit_u32((char **)&buf, ",", tkns, num_tkns);
+exit:
+	kfree(buf);
+	return ret;
+}
+
+static ssize_t ppoints_read(struct file *file,
+		char __user *to, size_t count, loff_t *ppos)
+{
+	struct snd_sof_dfsentry *dfse = file->private_data;
+	struct sof_probe_point_desc *desc;
+	size_t num_desc, len = 0;
+	char *buf;
+	int i, ret;
+
+	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = sof_ipc_probe_points_info(dfse->sdev, &desc, &num_desc);
+	if (ret < 0)
+		goto exit;
+
+	for (i = 0; i < num_desc; i++) {
+		ret = snprintf(buf + len, PAGE_SIZE - len,
+			"Id: %#010x  Purpose: %d  Node id: %#x\n",
+			desc[i].buffer_id, desc[i].purpose, desc[i].stream_tag);
+		if (ret < 0)
+			goto free_desc;
+		len += ret;
+	}
+
+	ret = simple_read_from_buffer(to, count, ppos, buf, len);
+free_desc:
+	kfree(desc);
+exit:
+	kfree(buf);
+	return ret;
+}
+
+static ssize_t ppoints_write(struct file *file,
+		const char __user *from, size_t count, loff_t *ppos)
+{
+	struct snd_sof_dfsentry *dfse = file->private_data;
+	struct sof_probe_point_desc *desc;
+	size_t num_tkns, bytes;
+	u32 *tkns;
+	int ret;
+
+	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
+	if (ret < 0)
+		return ret;
+	bytes = sizeof(*tkns) * num_tkns;
+	if (!num_tkns || (bytes % sizeof(*desc))) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	desc = (struct sof_probe_point_desc *)tkns;
+	ret = sof_ipc_probe_points_add(dfse->sdev,
+			desc, bytes / sizeof(*desc));
+	if (!ret)
+		ret = count;
+exit:
+	kfree(tkns);
+	return ret;
+}
+
+static const struct file_operations ppoints_fops = {
+	.open = simple_open,
+	.read = ppoints_read,
+	.write = ppoints_write,
+	.llseek = default_llseek,
+};
+
+static ssize_t ppoints_remove_write(struct file *file,
+		const char __user *from, size_t count, loff_t *ppos)
+{
+	struct snd_sof_dfsentry *dfse = file->private_data;
+	size_t num_tkns;
+	u32 *tkns;
+	int ret;
+
+	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
+	if (ret < 0)
+		return ret;
+	if (!num_tkns) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	ret = sof_ipc_probe_points_remove(dfse->sdev, tkns, num_tkns);
+	if (!ret)
+		ret = count;
+exit:
+	kfree(tkns);
+	return ret;
+}
+
+static const struct file_operations ppoints_remove_fops = {
+	.open = simple_open,
+	.write = ppoints_remove_write,
+	.llseek = default_llseek,
+};
+
+static int snd_sof_debugfs_probe_item(struct snd_sof_dev *sdev,
+				 const char *name, mode_t mode,
+				 const struct file_operations *fops)
+{
+	struct snd_sof_dfsentry *dfse;
+
+	dfse = devm_kzalloc(sdev->dev, sizeof(*dfse), GFP_KERNEL);
+	if (!dfse)
+		return -ENOMEM;
+
+	dfse->type = SOF_DFSENTRY_TYPE_BUF;
+	dfse->sdev = sdev;
+
+	debugfs_create_file(name, mode, sdev->debugfs_root, dfse, fops);
+	/* add to dfsentry list */
+	list_add(&dfse->list, &sdev->dfsentry_list);
+
+	return 0;
+}
+#endif
+
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)
 #define MAX_IPC_FLOOD_DURATION_MS 1000
 #define MAX_IPC_FLOOD_COUNT 10000
@@ -436,6 +633,17 @@  int snd_sof_dbg_init(struct snd_sof_dev *sdev)
 			return err;
 	}
 
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
+	err = snd_sof_debugfs_probe_item(sdev, "probe_points",
+			0644, &ppoints_fops);
+	if (err < 0)
+		return err;
+	err = snd_sof_debugfs_probe_item(sdev, "probe_points_remove",
+			0200, &ppoints_remove_fops);
+	if (err < 0)
+		return err;
+#endif
+
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST)
 	/* create read-write ipc_flood_count debugfs entry */
 	err = snd_sof_debugfs_buf_item(sdev, NULL, 0,