[QUEUED v20180920 00/16] stm class/intel_th: Queued updates for v4.20

[QUEUED v20180920 00/16] stm class/intel_th: Queued updates for v4.20

[Alexander Shishkin]

Hi,

These are patches I have queued so far, that I'm planning to send to
Greg for the next merge window. This is mainly support for MIPI SyS-T
protocol and all the infrastructure changes to make it possible.

MIPI SyS-T is a public standard [0] for a software-level trace protocol
that in this instance we use on top of STM devices (if we choose to) and
MIPI STP protocol.

The old "protocol" is preserved for compatibility.

[0] http://www.mipi.org/specifications/sys-t

Alexander Shishkin (16):
  stm class: Rework policy node fallback
  stm class: Clarify configfs root type/operations names
  stm class: Clean up stp_configfs_init
  stm class: Introduce framing protocol drivers
  stm class: Add a helper for writing data packets
  stm class: Factor out default framing protocol
  stm class: Switch over to the protocol driver
  stm class: Add MIPI SyS-T protocol support
  stm class: p_sys-t: Add support for CLOCKSYNC packets
  stm class: p_sys-t: Document the configfs interface
  stm class: Document the MIPI SyS-T protocol usage
  intel_th: SPDX-ify the documentation
  stm class: SPDX-ify the documentation
  stm class: heartbeat: Fix whitespace
  lib: Add memcat_p(): paste 2 pointer arrays together
  stm class: Use memcat_p()

 .../ABI/testing/configfs-stp-policy-p_sys-t   |  41 ++
 Documentation/trace/intel_th.rst              |   2 +
 Documentation/trace/stm.rst                   |   2 +
 Documentation/trace/sys-t.rst                 |  62 +++
 drivers/hwtracing/stm/Kconfig                 |  29 ++
 drivers/hwtracing/stm/Makefile                |   6 +
 drivers/hwtracing/stm/core.c                  | 292 +++++++++++---
 drivers/hwtracing/stm/heartbeat.c             |   2 +-
 drivers/hwtracing/stm/p_basic.c               |  47 +++
 drivers/hwtracing/stm/p_sys-t.c               | 381 ++++++++++++++++++
 drivers/hwtracing/stm/policy.c                | 148 +++++--
 drivers/hwtracing/stm/stm.h                   |  57 ++-
 include/linux/string.h                        |   7 +
 lib/Kconfig.debug                             |   8 +
 lib/Makefile                                  |   1 +
 lib/string.c                                  |  31 ++
 lib/test_memcat_p.c                           | 115 ++++++
 17 files changed, 1138 insertions(+), 93 deletions(-)
 create mode 100644 Documentation/ABI/testing/configfs-stp-policy-p_sys-t
 create mode 100644 Documentation/trace/sys-t.rst
 create mode 100644 drivers/hwtracing/stm/p_basic.c
 create mode 100644 drivers/hwtracing/stm/p_sys-t.c
 create mode 100644 lib/test_memcat_p.c

-- 
2.18.0

[QUEUED v20180920 01/16] stm class: Rework policy node fallback

[Alexander Shishkin]

Currently, if no matching policy node can be found for a trace source,
we'll try to use "default" policy node, then, if that doesn't exist,
we'll pick the first node, in order of creation. If that also fails,
we'll allocate M/C range from the beginning of the device's M/C range.

This makes it difficult to know which node (if any) was used in any
particular case.

In order to make things more deterministic, the new order is as follows:
  * if they supply ID string, use that and nothing else,
  * if they are a task, use their task name (comm),
  * use "default", if it exists,
  * return failure, to let them know there is no suitable rule.

This should provide enough convenience with the "default" catch-all node,
while not leaving *everything* to chance. As a side effect, this relaxes
the requirement of using ioctl() for identification with the possibility of
using task names as policy nodes.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/core.c   | 89 ++++++++++++++++++++--------------
 drivers/hwtracing/stm/policy.c | 12 ++---
 drivers/hwtracing/stm/stm.h    |  2 -
 3 files changed, 58 insertions(+), 45 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 10bcb5d73f90..7b1c549d6320 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -293,15 +293,15 @@ static int stm_output_assign(struct stm_device *stm, unsigned int width,
 	if (width > stm->data->sw_nchannels)
 		return -EINVAL;
 
-	if (policy_node) {
-		stp_policy_node_get_ranges(policy_node,
-					   &midx, &mend, &cidx, &cend);
-	} else {
-		midx = stm->data->sw_start;
-		cidx = 0;
-		mend = stm->data->sw_end;
-		cend = stm->data->sw_nchannels - 1;
-	}
+	/* We no longer accept policy_node==NULL here */
+	if (WARN_ON_ONCE(!policy_node))
+		return -EINVAL;
+
+	/*
+	 * Also, the caller holds reference to policy_node, so it won't
+	 * disappear on us.
+	 */
+	stp_policy_node_get_ranges(policy_node, &midx, &mend, &cidx, &cend);
 
 	spin_lock(&stm->mc_lock);
 	spin_lock(&output->lock);
@@ -405,19 +405,30 @@ static int stm_char_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static int stm_file_assign(struct stm_file *stmf, char *id, unsigned int width)
+static int
+stm_assign_first_policy(struct stm_device *stm, struct stm_output *output,
+			char **ids, unsigned int width)
 {
-	struct stm_device *stm = stmf->stm;
-	int ret;
+	struct stp_policy_node *pn;
+	int err, n;
 
-	stmf->policy_node = stp_policy_node_lookup(stm, id);
+	/*
+	 * On success, stp_policy_node_lookup() will return holding the
+	 * configfs subsystem mutex, which is then released in
+	 * stp_policy_node_put(). This allows the pdrv->output_open() in
+	 * stm_output_assign() to serialize against the attribute accessors.
+	 */
+	for (n = 0, pn = NULL; ids[n] && !pn; n++)
+		pn = stp_policy_node_lookup(stm, ids[n]);
 
-	ret = stm_output_assign(stm, width, stmf->policy_node, &stmf->output);
+	if (!pn)
+		return -EINVAL;
 
-	if (stmf->policy_node)
-		stp_policy_node_put(stmf->policy_node);
+	err = stm_output_assign(stm, width, pn, output);
 
-	return ret;
+	stp_policy_node_put(pn);
+
+	return err;
 }
 
 static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
@@ -455,16 +466,23 @@ static ssize_t stm_char_write(struct file *file, const char __user *buf,
 		count = PAGE_SIZE - 1;
 
 	/*
-	 * if no m/c have been assigned to this writer up to this
-	 * point, use "default" policy entry
+	 * If no m/c have been assigned to this writer up to this
+	 * point, try to use the task name and "default" policy entries.
 	 */
 	if (!stmf->output.nr_chans) {
-		err = stm_file_assign(stmf, "default", 1);
+		char comm[sizeof(current->comm)];
+		char *ids[] = { comm, "default", NULL };
+
+		get_task_comm(comm, current);
+
+		err = stm_assign_first_policy(stmf->stm, &stmf->output, ids, 1);
 		/*
 		 * EBUSY means that somebody else just assigned this
-		 * output, which is just fine for write()
+		 * output, which is just fine for write(), except for (XXX)
+		 * it doesn't actually return -EBUSY, but -EINVAL and WARNs
+		 * in this case. Hrrr.
 		 */
-		if (err && err != -EBUSY)
+		if (err)
 			return err;
 	}
 
@@ -550,6 +568,7 @@ static int stm_char_policy_set_ioctl(struct stm_file *stmf, void __user *arg)
 {
 	struct stm_device *stm = stmf->stm;
 	struct stp_policy_id *id;
+	char *ids[] = { NULL, NULL };
 	int ret = -EINVAL;
 	u32 size;
 
@@ -582,7 +601,9 @@ static int stm_char_policy_set_ioctl(struct stm_file *stmf, void __user *arg)
 	    id->width > PAGE_SIZE / stm->data->sw_mmiosz)
 		goto err_free;
 
-	ret = stm_file_assign(stmf, id->id, id->width);
+	ids[0] = id->id;
+	ret = stm_assign_first_policy(stmf->stm, &stmf->output, ids,
+				      id->width);
 	if (ret)
 		goto err_free;
 
@@ -818,8 +839,8 @@ EXPORT_SYMBOL_GPL(stm_unregister_device);
 static int stm_source_link_add(struct stm_source_device *src,
 			       struct stm_device *stm)
 {
-	char *id;
-	int err;
+	char *ids[] = { NULL, "default", NULL };
+	int err = -ENOMEM;
 
 	mutex_lock(&stm->link_mutex);
 	spin_lock(&stm->link_lock);
@@ -833,19 +854,13 @@ static int stm_source_link_add(struct stm_source_device *src,
 	spin_unlock(&stm->link_lock);
 	mutex_unlock(&stm->link_mutex);
 
-	id = kstrdup(src->data->name, GFP_KERNEL);
-	if (id) {
-		src->policy_node =
-			stp_policy_node_lookup(stm, id);
-
-		kfree(id);
-	}
-
-	err = stm_output_assign(stm, src->data->nr_chans,
-				src->policy_node, &src->output);
+	ids[0] = kstrdup(src->data->name, GFP_KERNEL);
+	if (!ids[0])
+		goto fail_detach;
 
-	if (src->policy_node)
-		stp_policy_node_put(src->policy_node);
+	err = stm_assign_first_policy(stm, &src->output, ids,
+				      src->data->nr_chans);
+	kfree(ids[0]);
 
 	if (err)
 		goto fail_detach;
diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 3fd07e275b34..15d35d891643 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -392,7 +392,7 @@ static struct configfs_subsystem stp_policy_subsys = {
 static struct stp_policy_node *
 __stp_policy_node_lookup(struct stp_policy *policy, char *s)
 {
-	struct stp_policy_node *policy_node, *ret;
+	struct stp_policy_node *policy_node, *ret = NULL;
 	struct list_head *head = &policy->group.cg_children;
 	struct config_item *item;
 	char *start, *end = s;
@@ -400,10 +400,6 @@ __stp_policy_node_lookup(struct stp_policy *policy, char *s)
 	if (list_empty(head))
 		return NULL;
 
-	/* return the first entry if everything else fails */
-	item = list_entry(head->next, struct config_item, ci_entry);
-	ret = to_stp_policy_node(item);
-
 next:
 	for (;;) {
 		start = strsep(&end, "/");
@@ -449,13 +445,17 @@ stp_policy_node_lookup(struct stm_device *stm, char *s)
 
 	if (policy_node)
 		config_item_get(&policy_node->group.cg_item);
-	mutex_unlock(&stp_policy_subsys.su_mutex);
+	else
+		mutex_unlock(&stp_policy_subsys.su_mutex);
 
 	return policy_node;
 }
 
 void stp_policy_node_put(struct stp_policy_node *policy_node)
 {
+	lockdep_assert_held(&stp_policy_subsys.su_mutex);
+
+	mutex_unlock(&stp_policy_subsys.su_mutex);
 	config_item_put(&policy_node->group.cg_item);
 }
 
diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
index 923571adc6f4..e5df08ae59cf 100644
--- a/drivers/hwtracing/stm/stm.h
+++ b/drivers/hwtracing/stm/stm.h
@@ -57,7 +57,6 @@ struct stm_output {
 
 struct stm_file {
 	struct stm_device	*stm;
-	struct stp_policy_node	*policy_node;
 	struct stm_output	output;
 };
 
@@ -71,7 +70,6 @@ struct stm_source_device {
 	struct stm_device __rcu	*link;
 	struct list_head	link_entry;
 	/* one output per stm_source device */
-	struct stp_policy_node	*policy_node;
 	struct stm_output	output;
 };
 
-- 
2.18.0

[QUEUED v20180920 02/16] stm class: Clarify configfs root type/operations names

[Alexander Shishkin]

The current naming of stp-policy root type and group ops is confusing,
rename them for better readability.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/policy.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 15d35d891643..530448bd5a34 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -311,7 +311,7 @@ static const struct config_item_type stp_policy_type = {
 };
 
 static struct config_group *
-stp_policies_make(struct config_group *group, const char *name)
+stp_policy_make(struct config_group *group, const char *name)
 {
 	struct config_group *ret;
 	struct stm_device *stm;
@@ -368,12 +368,12 @@ stp_policies_make(struct config_group *group, const char *name)
 	return ret;
 }
 
-static struct configfs_group_operations stp_policies_group_ops = {
-	.make_group	= stp_policies_make,
+static struct configfs_group_operations stp_policy_root_group_ops = {
+	.make_group	= stp_policy_make,
 };
 
-static const struct config_item_type stp_policies_type = {
-	.ct_group_ops	= &stp_policies_group_ops,
+static const struct config_item_type stp_policy_root_type = {
+	.ct_group_ops	= &stp_policy_root_group_ops,
 	.ct_owner	= THIS_MODULE,
 };
 
@@ -381,7 +381,7 @@ static struct configfs_subsystem stp_policy_subsys = {
 	.su_group = {
 		.cg_item = {
 			.ci_namebuf	= "stp-policy",
-			.ci_type	= &stp_policies_type,
+			.ci_type	= &stp_policy_root_type,
 		},
 	},
 };
-- 
2.18.0

[QUEUED v20180920 03/16] stm class: Clean up stp_configfs_init

[Alexander Shishkin]

Minor code shortening, no functional changes.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/policy.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 530448bd5a34..a505f055f464 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -461,13 +461,9 @@ void stp_policy_node_put(struct stp_policy_node *policy_node)
 
 int __init stp_configfs_init(void)
 {
-	int err;
-
 	config_group_init(&stp_policy_subsys.su_group);
 	mutex_init(&stp_policy_subsys.su_mutex);
-	err = configfs_register_subsystem(&stp_policy_subsys);
-
-	return err;
+	return configfs_register_subsystem(&stp_policy_subsys);
 }
 
 void __exit stp_configfs_exit(void)
-- 
2.18.0

[QUEUED v20180920 04/16] stm class: Introduce framing protocol drivers

[Alexander Shishkin]

At the moment, the stm class applies a certain STP framing pattern to
the data as it is written to the underlying STM device. In order to
allow different framing patterns (aka protocols), this patch introduces
the concept of STP protocol drivers, defines data structures and APIs
for the protocol drivers to use.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/core.c   | 142 ++++++++++++++++++++++++++++++++
 drivers/hwtracing/stm/policy.c | 145 ++++++++++++++++++++++++++++++---
 drivers/hwtracing/stm/stm.h    |  52 ++++++++++--
 3 files changed, 321 insertions(+), 18 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 7b1c549d6320..c869a30661ac 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -316,11 +316,26 @@ static int stm_output_assign(struct stm_device *stm, unsigned int width,
 	output->master = midx;
 	output->channel = cidx;
 	output->nr_chans = width;
+	if (stm->pdrv->output_open) {
+		void *priv = stp_policy_node_priv(policy_node);
+
+		if (WARN_ON_ONCE(!priv))
+			goto unlock;
+
+		/* configfs subsys mutex is held by the caller */
+		ret = stm->pdrv->output_open(priv, output);
+		if (ret)
+			goto unlock;
+	}
+
 	stm_output_claim(stm, output);
 	dev_dbg(&stm->dev, "assigned %u:%u (+%u)\n", midx, cidx, width);
 
 	ret = 0;
 unlock:
+	if (ret)
+		output->nr_chans = 0;
+
 	spin_unlock(&output->lock);
 	spin_unlock(&stm->mc_lock);
 
@@ -333,6 +348,8 @@ static void stm_output_free(struct stm_device *stm, struct stm_output *output)
 	spin_lock(&output->lock);
 	if (output->nr_chans)
 		stm_output_disclaim(stm, output);
+	if (stm->pdrv->output_close)
+		stm->pdrv->output_close(output);
 	spin_unlock(&output->lock);
 	spin_unlock(&stm->mc_lock);
 }
@@ -349,6 +366,122 @@ static int major_match(struct device *dev, const void *data)
 	return MAJOR(dev->devt) == major;
 }
 
+/*
+ * Framing protocol management
+ * Modules can implement STM protocol drivers and (un-)register them
+ * with the STM class framework.
+ */
+static struct list_head stm_pdrv_head;
+static struct mutex stm_pdrv_mutex;
+
+struct stm_pdrv_entry {
+	struct list_head			entry;
+	const struct stm_protocol_driver	*pdrv;
+	const struct config_item_type		*node_type;
+};
+
+static const struct stm_pdrv_entry *
+__stm_lookup_protocol(const char *name)
+{
+	struct stm_pdrv_entry *pe;
+
+	list_for_each_entry(pe, &stm_pdrv_head, entry) {
+		if (!name || !*name || !strcmp(name, pe->pdrv->name))
+			return pe;
+	}
+
+	return NULL;
+}
+
+int stm_register_protocol(const struct stm_protocol_driver *pdrv)
+{
+	struct stm_pdrv_entry *pe = NULL;
+	int ret = -ENOMEM;
+
+	mutex_lock(&stm_pdrv_mutex);
+
+	if (__stm_lookup_protocol(pdrv->name)) {
+		ret = -EEXIST;
+		goto unlock;
+	}
+
+	pe = kzalloc(sizeof(*pe), GFP_KERNEL);
+	if (!pe)
+		goto unlock;
+
+	if (pdrv->policy_attr) {
+		pe->node_type = get_policy_node_type(pdrv->policy_attr);
+		if (!pe->node_type)
+			goto unlock;
+	}
+
+	list_add_tail(&pe->entry, &stm_pdrv_head);
+	pe->pdrv = pdrv;
+
+	ret = 0;
+unlock:
+	mutex_unlock(&stm_pdrv_mutex);
+
+	if (ret)
+		kfree(pe);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(stm_register_protocol);
+
+void stm_unregister_protocol(const struct stm_protocol_driver *pdrv)
+{
+	struct stm_pdrv_entry *pe, *iter;
+
+	mutex_lock(&stm_pdrv_mutex);
+
+	list_for_each_entry_safe(pe, iter, &stm_pdrv_head, entry) {
+		if (pe->pdrv == pdrv) {
+			list_del(&pe->entry);
+
+			/* XXX: factor out */
+			if (pe->node_type) {
+				kfree(pe->node_type->ct_attrs);
+				kfree(pe->node_type);
+			}
+			kfree(pe);
+			break;
+		}
+	}
+
+	mutex_unlock(&stm_pdrv_mutex);
+}
+EXPORT_SYMBOL_GPL(stm_unregister_protocol);
+
+static bool stm_get_protocol(const struct stm_protocol_driver *pdrv)
+{
+	return try_module_get(pdrv->owner);
+}
+
+void stm_put_protocol(const struct stm_protocol_driver *pdrv)
+{
+	module_put(pdrv->owner);
+}
+
+int stm_lookup_protocol(const char *name,
+			const struct stm_protocol_driver **pdrv,
+			const struct config_item_type **node_type)
+{
+	const struct stm_pdrv_entry *pe;
+
+	mutex_lock(&stm_pdrv_mutex);
+
+	pe = __stm_lookup_protocol(name);
+	if (pe && pe->pdrv && stm_get_protocol(pe->pdrv)) {
+		*pdrv = pe->pdrv;
+		*node_type = pe->node_type;
+	}
+
+	mutex_unlock(&stm_pdrv_mutex);
+
+	return pe ? 0 : -ENOENT;
+}
+
 static int stm_char_open(struct inode *inode, struct file *file)
 {
 	struct stm_file *stmf;
@@ -1178,6 +1311,15 @@ static int __init stm_core_init(void)
 		goto err_src;
 
 	init_srcu_struct(&stm_source_srcu);
+	INIT_LIST_HEAD(&stm_pdrv_head);
+	mutex_init(&stm_pdrv_mutex);
+
+	/*
+	 * So as to not confuse existing users with a requirement
+	 * to load yet another module, do it here.
+	 */
+	if (IS_ENABLED(CONFIG_STM_PROTO_BASIC))
+		(void)request_module_nowait("stm_p_basic");
 
 	stm_core_up++;
 
diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index a505f055f464..894598cfe2b7 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -33,8 +33,18 @@ struct stp_policy_node {
 	unsigned int		last_master;
 	unsigned int		first_channel;
 	unsigned int		last_channel;
+	/* this is the one that's exposed to the attributes */
+	unsigned char		priv[0];
 };
 
+void *stp_policy_node_priv(struct stp_policy_node *pn)
+{
+	if (!pn)
+		return NULL;
+
+	return pn->priv;
+}
+
 static struct configfs_subsystem stp_policy_subsys;
 
 void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
@@ -68,6 +78,14 @@ to_stp_policy_node(struct config_item *item)
 		NULL;
 }
 
+void *to_pdrv_policy_node(struct config_item *item)
+{
+	struct stp_policy_node *node = to_stp_policy_node(item);
+
+	return stp_policy_node_priv(node);
+}
+EXPORT_SYMBOL_GPL(to_pdrv_policy_node);
+
 static ssize_t
 stp_policy_node_masters_show(struct config_item *item, char *page)
 {
@@ -163,7 +181,9 @@ stp_policy_node_channels_store(struct config_item *item, const char *page,
 
 static void stp_policy_node_release(struct config_item *item)
 {
-	kfree(to_stp_policy_node(item));
+	struct stp_policy_node *node = to_stp_policy_node(item);
+
+	kfree(node);
 }
 
 static struct configfs_item_operations stp_policy_node_item_ops = {
@@ -182,10 +202,61 @@ static struct configfs_attribute *stp_policy_node_attrs[] = {
 static const struct config_item_type stp_policy_type;
 static const struct config_item_type stp_policy_node_type;
 
+/* lifted from arch/x86/events/core.c */
+static struct configfs_attribute **merge_attr(struct configfs_attribute **a, struct configfs_attribute **b)
+{
+	struct configfs_attribute **new;
+	int j, i;
+
+	for (j = 0; a[j]; j++)
+		;
+	for (i = 0; b[i]; i++)
+		j++;
+	j++;
+
+	new = kmalloc_array(j, sizeof(struct configfs_attribute *),
+			    GFP_KERNEL);
+	if (!new)
+		return NULL;
+
+	j = 0;
+	for (i = 0; a[i]; i++)
+		new[j++] = a[i];
+	for (i = 0; b[i]; i++)
+		new[j++] = b[i];
+	new[j] = NULL;
+
+	return new;
+}
+
+const struct config_item_type *
+get_policy_node_type(struct configfs_attribute **attrs)
+{
+	struct config_item_type *type;
+	struct configfs_attribute **merged;
+
+	type = kmemdup(&stp_policy_node_type, sizeof(stp_policy_node_type),
+		       GFP_KERNEL);
+	if (!type)
+		return NULL;
+
+	merged = merge_attr(stp_policy_node_attrs, attrs);
+	if (!merged) {
+		kfree(type);
+		return NULL;
+	}
+
+	type->ct_attrs = merged;
+
+	return type;
+}
+
 static struct config_group *
 stp_policy_node_make(struct config_group *group, const char *name)
 {
+	const struct config_item_type *type = &stp_policy_node_type;
 	struct stp_policy_node *policy_node, *parent_node;
+	const struct stm_protocol_driver *pdrv;
 	struct stp_policy *policy;
 
 	if (group->cg_item.ci_type == &stp_policy_type) {
@@ -199,12 +270,20 @@ stp_policy_node_make(struct config_group *group, const char *name)
 	if (!policy->stm)
 		return ERR_PTR(-ENODEV);
 
-	policy_node = kzalloc(sizeof(struct stp_policy_node), GFP_KERNEL);
+	pdrv = policy->stm->pdrv;
+	policy_node =
+		kzalloc(offsetof(struct stp_policy_node, priv[pdrv->priv_sz]),
+			GFP_KERNEL);
 	if (!policy_node)
 		return ERR_PTR(-ENOMEM);
 
-	config_group_init_type_name(&policy_node->group, name,
-				    &stp_policy_node_type);
+	if (pdrv->policy_node_init)
+		pdrv->policy_node_init((void *)policy_node->priv);
+
+	if (policy->stm->pdrv_node_type)
+		type = policy->stm->pdrv_node_type;
+
+	config_group_init_type_name(&policy_node->group, name, type);
 
 	policy_node->policy = policy;
 
@@ -254,8 +333,26 @@ static ssize_t stp_policy_device_show(struct config_item *item,
 
 CONFIGFS_ATTR_RO(stp_policy_, device);
 
+static ssize_t stp_policy_protocol_show(struct config_item *item,
+					char *page)
+{
+	struct stp_policy *policy = to_stp_policy(item);
+	ssize_t count;
+
+	/* XXX: there shouldn't be 'none', ever */
+	count = sprintf(page, "%s\n",
+			(policy && policy->stm) ?
+			policy->stm->pdrv->name :
+			"<none>");
+
+	return count;
+}
+
+CONFIGFS_ATTR_RO(stp_policy_, protocol);
+
 static struct configfs_attribute *stp_policy_attrs[] = {
 	&stp_policy_attr_device,
+	&stp_policy_attr_protocol,
 	NULL,
 };
 
@@ -276,6 +373,7 @@ void stp_policy_unbind(struct stp_policy *policy)
 	stm->policy = NULL;
 	policy->stm = NULL;
 
+	stm_put_protocol(stm->pdrv);
 	stm_put_device(stm);
 }
 
@@ -313,9 +411,12 @@ static const struct config_item_type stp_policy_type = {
 static struct config_group *
 stp_policy_make(struct config_group *group, const char *name)
 {
+	const struct config_item_type *pdrv_node_type;
+	const struct stm_protocol_driver *pdrv;
+	char *devname, *proto, *p;
 	struct config_group *ret;
 	struct stm_device *stm;
-	char *devname, *p;
+	int err;
 
 	devname = kasprintf(GFP_KERNEL, "%s", name);
 	if (!devname)
@@ -326,6 +427,7 @@ stp_policy_make(struct config_group *group, const char *name)
 	 * <device_name> is the name of an existing stm device; may
 	 *               contain dots;
 	 * <policy_name> is an arbitrary string; may not contain dots
+	 * <device_name>:<protocol_name>.<policy_name>
 	 */
 	p = strrchr(devname, '.');
 	if (!p) {
@@ -335,11 +437,28 @@ stp_policy_make(struct config_group *group, const char *name)
 
 	*p = '\0';
 
+	/*
+	 * look for ":<protocol_name>":
+	 *  + no protocol suffix: fall back to whatever is available;
+	 *  + unknown protocol: fail the whole thing
+	 */
+	proto = strrchr(devname, ':');
+	if (proto)
+		*proto++ = '\0';
+
 	stm = stm_find_device(devname);
+	if (!stm) {
+		kfree(devname);
+		return ERR_PTR(-ENODEV);
+	}
+
+	err = stm_lookup_protocol(proto, &pdrv, &pdrv_node_type);
 	kfree(devname);
 
-	if (!stm)
+	if (err) {
+		stm_put_device(stm);
 		return ERR_PTR(-ENODEV);
+	}
 
 	mutex_lock(&stm->policy_mutex);
 	if (stm->policy) {
@@ -349,21 +468,27 @@ stp_policy_make(struct config_group *group, const char *name)
 
 	stm->policy = kzalloc(sizeof(*stm->policy), GFP_KERNEL);
 	if (!stm->policy) {
-		ret = ERR_PTR(-ENOMEM);
-		goto unlock_policy;
+		mutex_unlock(&stm->policy_mutex);
+		stm_put_protocol(pdrv);
+		stm_put_device(stm);
+		return ERR_PTR(-ENOMEM);
 	}
 
 	config_group_init_type_name(&stm->policy->group, name,
 				    &stp_policy_type);
-	stm->policy->stm = stm;
 
+	stm->pdrv = pdrv;
+	stm->pdrv_node_type = pdrv_node_type;
+	stm->policy->stm = stm;
 	ret = &stm->policy->group;
 
 unlock_policy:
 	mutex_unlock(&stm->policy_mutex);
 
-	if (IS_ERR(ret))
+	if (IS_ERR(ret)) {
+		stm_put_protocol(stm->pdrv);
 		stm_put_device(stm);
+	}
 
 	return ret;
 }
diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
index e5df08ae59cf..921ebd9fd3bd 100644
--- a/drivers/hwtracing/stm/stm.h
+++ b/drivers/hwtracing/stm/stm.h
@@ -10,20 +10,17 @@
 #ifndef _STM_STM_H_
 #define _STM_STM_H_
 
+#include <linux/configfs.h>
+
 struct stp_policy;
 struct stp_policy_node;
+struct stm_protocol_driver;
 
-struct stp_policy_node *
-stp_policy_node_lookup(struct stm_device *stm, char *s);
-void stp_policy_node_put(struct stp_policy_node *policy_node);
-void stp_policy_unbind(struct stp_policy *policy);
-
-void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
-				unsigned int *mstart, unsigned int *mend,
-				unsigned int *cstart, unsigned int *cend);
 int stp_configfs_init(void);
 void stp_configfs_exit(void);
 
+void *stp_policy_node_priv(struct stp_policy_node *pn);
+
 struct stp_master {
 	unsigned int	nr_free;
 	unsigned long	chan_map[0];
@@ -40,6 +37,9 @@ struct stm_device {
 	struct mutex		link_mutex;
 	spinlock_t		link_lock;
 	struct list_head	link_list;
+	/* framing protocol in use */
+	const struct stm_protocol_driver	*pdrv;
+	const struct config_item_type		*pdrv_node_type;
 	/* master allocation */
 	spinlock_t		mc_lock;
 	struct stp_master	*masters[0];
@@ -48,11 +48,25 @@ struct stm_device {
 #define to_stm_device(_d)				\
 	container_of((_d), struct stm_device, dev)
 
+struct stp_policy_node *
+stp_policy_node_lookup(struct stm_device *stm, char *s);
+void stp_policy_node_put(struct stp_policy_node *policy_node);
+void stp_policy_unbind(struct stp_policy *policy);
+
+void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
+				unsigned int *mstart, unsigned int *mend,
+				unsigned int *cstart, unsigned int *cend);
+
+/* XXX: unXXX this! */
+const struct config_item_type *
+get_policy_node_type(struct configfs_attribute **attrs);
+
 struct stm_output {
 	spinlock_t		lock;
 	unsigned int		master;
 	unsigned int		channel;
 	unsigned int		nr_chans;
+	void			*pdrv_private;
 };
 
 struct stm_file {
@@ -76,4 +90,26 @@ struct stm_source_device {
 #define to_stm_source_device(_d)				\
 	container_of((_d), struct stm_source_device, dev)
 
+void *to_pdrv_policy_node(struct config_item *item);
+
+struct stm_protocol_driver {
+	struct module	*owner;
+	const char	*name;
+	ssize_t		(*write)(struct stm_data *data,
+				 struct stm_output *output, unsigned int chan,
+				 const char *buf, size_t count);
+	void		(*policy_node_init)(void *arg);
+	int		(*output_open)(void *priv, struct stm_output *output);
+	void		(*output_close)(struct stm_output *output);
+	ssize_t		priv_sz;
+	struct configfs_attribute	**policy_attr;
+};
+
+int stm_register_protocol(const struct stm_protocol_driver *pdrv);
+void stm_unregister_protocol(const struct stm_protocol_driver *pdrv);
+int stm_lookup_protocol(const char *name,
+			const struct stm_protocol_driver **pdrv,
+			const struct config_item_type **type);
+void stm_put_protocol(const struct stm_protocol_driver *pdrv);
+
 #endif /* _STM_STM_H_ */
-- 
2.18.0

[QUEUED v20180920 05/16] stm class: Add a helper for writing data packets

[Alexander Shishkin]

Add a helper to write a sequence of bytes as STP data packets. This
is used by protocol drivers to output their metadata, as well as the
actual data payload.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/core.c | 50 ++++++++++++++++++++++++++----------
 drivers/hwtracing/stm/stm.h  |  3 +++
 2 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index c869a30661ac..9ed2f06deb47 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -564,27 +564,51 @@ stm_assign_first_policy(struct stm_device *stm, struct stm_output *output,
 	return err;
 }
 
-static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
-			  unsigned int channel, const char *buf, size_t count)
+/**
+ * stm_data_write() - send the given payload as data packets
+ * @data:	stm driver's data
+ * @m:		STP master
+ * @c:		STP channel
+ * @ts_first:	timestamp the first packet
+ * @buf:	data payload buffer
+ * @count:	data payload size
+ */
+ssize_t notrace stm_data_write(struct stm_data *data, unsigned int m,
+			       unsigned int c, bool ts_first, const void *buf,
+			       size_t count)
 {
-	unsigned int flags = STP_PACKET_TIMESTAMPED;
-	const unsigned char *p = buf, nil = 0;
-	size_t pos;
+	unsigned int flags = ts_first ? STP_PACKET_TIMESTAMPED : 0;
 	ssize_t sz;
+	size_t pos;
 
-	for (pos = 0, p = buf; count > pos; pos += sz, p += sz) {
+	for (pos = 0, sz = 0; pos < count; pos += sz) {
 		sz = min_t(unsigned int, count - pos, 8);
-		sz = data->packet(data, master, channel, STP_PACKET_DATA, flags,
-				  sz, p);
-		flags = 0;
-
-		if (sz < 0)
+		sz = data->packet(data, m, c, STP_PACKET_DATA, flags, sz,
+				  &((u8 *)buf)[pos]);
+		if (sz <= 0)
 			break;
+
+		if (ts_first) {
+			flags = 0;
+			ts_first = false;
+		}
 	}
 
-	data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0, &nil);
+	return sz < 0 ? sz : pos;
+}
+EXPORT_SYMBOL_GPL(stm_data_write);
+
+static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
+			  unsigned int channel, const char *buf, size_t count)
+{
+	ssize_t sz;
+
+	sz = stm_data_write(data, master, channel, true, buf, count);
+	if (sz > 0)
+		data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
+			     buf);
 
-	return pos;
+	return sz;
 }
 
 static ssize_t stm_char_write(struct file *file, const char __user *buf,
diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
index 921ebd9fd3bd..753d83acb7ae 100644
--- a/drivers/hwtracing/stm/stm.h
+++ b/drivers/hwtracing/stm/stm.h
@@ -111,5 +111,8 @@ int stm_lookup_protocol(const char *name,
 			const struct stm_protocol_driver **pdrv,
 			const struct config_item_type **type);
 void stm_put_protocol(const struct stm_protocol_driver *pdrv);
+ssize_t stm_data_write(struct stm_data *data, unsigned int m,
+		       unsigned int c, bool ts_first, const void *buf,
+		       size_t count);
 
 #endif /* _STM_STM_H_ */
-- 
2.18.0

[QUEUED v20180920 06/16] stm class: Factor out default framing protocol

[Alexander Shishkin]

The STP framing pattern that the stm class implicitly applies to the
data payload is, in fact, a protocol. This patch moves the relevant code
out of the stm core into its own driver module.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/Kconfig   | 15 +++++++++++
 drivers/hwtracing/stm/Makefile  |  4 +++
 drivers/hwtracing/stm/p_basic.c | 47 +++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+)
 create mode 100644 drivers/hwtracing/stm/p_basic.c

diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
index 723e2d90083d..262e7891fb97 100644
--- a/drivers/hwtracing/stm/Kconfig
+++ b/drivers/hwtracing/stm/Kconfig
@@ -11,6 +11,21 @@ config STM
 
 if STM
 
+config STM_PROTO_BASIC
+	tristate "Basic STM framing protocol driver"
+	default CONFIG_STM
+	help
+	  This is a simple framing protocol for sending data over STM
+	  devices. This was the protocol that the STM framework used
+	  exclusively until the MIPI SyS-T support was added. Use this
+	  driver for compatibility with your existing STM setup.
+
+	  The receiving side only needs to be able to decode the MIPI
+	  STP protocol in order to extract the data.
+
+	  If you want to be able to use the basic protocol or want the
+	  backwards compatibility for your existing setup, say Y.
+
 config STM_DUMMY
 	tristate "Dummy STM driver"
 	help
diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile
index effc19e5190f..1571de66eca4 100644
--- a/drivers/hwtracing/stm/Makefile
+++ b/drivers/hwtracing/stm/Makefile
@@ -3,6 +3,10 @@ obj-$(CONFIG_STM)	+= stm_core.o
 
 stm_core-y		:= core.o policy.o
 
+obj-$(CONFIG_STM_PROTO_BASIC) += stm_p_basic.o
+
+stm_p_basic-y		:= p_basic.o
+
 obj-$(CONFIG_STM_DUMMY)	+= dummy_stm.o
 
 obj-$(CONFIG_STM_SOURCE_CONSOLE)	+= stm_console.o
diff --git a/drivers/hwtracing/stm/p_basic.c b/drivers/hwtracing/stm/p_basic.c
new file mode 100644
index 000000000000..64e3325a4a8a
--- /dev/null
+++ b/drivers/hwtracing/stm/p_basic.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Basic framing protocol for STM devices.
+ * Copyright (c) 2018, Intel Corporation.
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/stm.h>
+#include "stm.h"
+
+static ssize_t basic_write(struct stm_data *data, struct stm_output *output,
+			   unsigned int chan, const char *buf, size_t count)
+{
+	unsigned int c = output->channel + chan;
+	unsigned int m = output->master;
+	ssize_t sz;
+
+	sz = stm_data_write(data, m, c, true, buf, count);
+	if (sz > 0)
+		data->packet(data, m, c, STP_PACKET_FLAG, 0, 0, buf);
+
+	return sz;
+}
+
+static const struct stm_protocol_driver basic_pdrv = {
+	.owner	= THIS_MODULE,
+	.name	= "p_basic",
+	.write	= basic_write,
+};
+
+static int basic_stm_init(void)
+{
+	return stm_register_protocol(&basic_pdrv);
+}
+
+static void basic_stm_exit(void)
+{
+	stm_unregister_protocol(&basic_pdrv);
+}
+
+module_init(basic_stm_init);
+module_exit(basic_stm_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Basic STM framing protocol driver");
+MODULE_AUTHOR("Alexander Shishkin <alexander.shishkin@linux.intel.com>");
-- 
2.18.0

[QUEUED v20180920 07/16] stm class: Switch over to the protocol driver

[Alexander Shishkin]

Now that the default framing protocol is factored out into its own driver,
switch over to using the driver for writing data.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/core.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 9ed2f06deb47..fb224574b3db 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -598,17 +598,21 @@ ssize_t notrace stm_data_write(struct stm_data *data, unsigned int m,
 }
 EXPORT_SYMBOL_GPL(stm_data_write);
 
-static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
-			  unsigned int channel, const char *buf, size_t count)
+static ssize_t notrace
+stm_write(struct stm_device *stm, struct stm_output *output,
+	  unsigned int chan, const char *buf, size_t count)
 {
-	ssize_t sz;
+	int err;
+
+	/* stm->pdrv is serialized against policy_mutex */
+	if (!stm->pdrv)
+		return -ENODEV;
 
-	sz = stm_data_write(data, master, channel, true, buf, count);
-	if (sz > 0)
-		data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
-			     buf);
+	err = stm->pdrv->write(stm->data, output, chan, buf, count);
+	if (err < 0)
+		return err;
 
-	return sz;
+	return err;
 }
 
 static ssize_t stm_char_write(struct file *file, const char __user *buf,
@@ -655,8 +659,7 @@ static ssize_t stm_char_write(struct file *file, const char __user *buf,
 
 	pm_runtime_get_sync(&stm->dev);
 
-	count = stm_write(stm->data, stmf->output.master, stmf->output.channel,
-			  kbuf, count);
+	count = stm_write(stm, &stmf->output, 0, kbuf, count);
 
 	pm_runtime_mark_last_busy(&stm->dev);
 	pm_runtime_put_autosuspend(&stm->dev);
@@ -1306,9 +1309,7 @@ int notrace stm_source_write(struct stm_source_data *data,
 
 	stm = srcu_dereference(src->link, &stm_source_srcu);
 	if (stm)
-		count = stm_write(stm->data, src->output.master,
-				  src->output.channel + chan,
-				  buf, count);
+		count = stm_write(stm, &src->output, chan, buf, count);
 	else
 		count = -ENODEV;
 
-- 
2.18.0

[QUEUED v20180920 08/16] stm class: Add MIPI SyS-T protocol support

[Alexander Shishkin]

This adds support for MIPI SyS-T protocol as specified in an open
standard [1]. In additional to marking message boundaries, it also
supports tagging messages with the source UUID, to provide better
distinction between trace sources, including payload length and
timestamp in the message's metadata.

This driver adds attributes to STP policy nodes to control/configure
these metadata features.

[1] https://www.mipi.org/specifications/sys-t

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/Kconfig   |  14 ++
 drivers/hwtracing/stm/Makefile  |   2 +
 drivers/hwtracing/stm/p_sys-t.c | 301 ++++++++++++++++++++++++++++++++
 3 files changed, 317 insertions(+)
 create mode 100644 drivers/hwtracing/stm/p_sys-t.c

diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
index 262e7891fb97..752dd66742bf 100644
--- a/drivers/hwtracing/stm/Kconfig
+++ b/drivers/hwtracing/stm/Kconfig
@@ -26,6 +26,20 @@ config STM_PROTO_BASIC
 	  If you want to be able to use the basic protocol or want the
 	  backwards compatibility for your existing setup, say Y.
 
+config STM_PROTO_SYS_T
+	tristate "MIPI SyS-T STM framing protocol driver"
+	default CONFIG_STM
+	help
+	  This is an implementation of MIPI SyS-T protocol to be used
+	  over the STP transport. In addition to the data payload, it
+	  also carries additional metadata for time correlation, better
+	  means of trace source identification, etc.
+
+	  The receiving side must be able to decode this protocol in
+	  addition to the MIPI STP, in order to extract the data.
+
+	  If you don't know what this is, say N.
+
 config STM_DUMMY
 	tristate "Dummy STM driver"
 	help
diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile
index 1571de66eca4..1692fcd29277 100644
--- a/drivers/hwtracing/stm/Makefile
+++ b/drivers/hwtracing/stm/Makefile
@@ -4,8 +4,10 @@ obj-$(CONFIG_STM)	+= stm_core.o
 stm_core-y		:= core.o policy.o
 
 obj-$(CONFIG_STM_PROTO_BASIC) += stm_p_basic.o
+obj-$(CONFIG_STM_PROTO_SYS_T) += stm_p_sys-t.o
 
 stm_p_basic-y		:= p_basic.o
+stm_p_sys-t-y		:= p_sys-t.o
 
 obj-$(CONFIG_STM_DUMMY)	+= dummy_stm.o
 
diff --git a/drivers/hwtracing/stm/p_sys-t.c b/drivers/hwtracing/stm/p_sys-t.c
new file mode 100644
index 000000000000..8f3066ff1b7b
--- /dev/null
+++ b/drivers/hwtracing/stm/p_sys-t.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MIPI SyS-T framing protocol for STM devices.
+ * Copyright (c) 2018, Intel Corporation.
+ */
+
+#include <linux/configfs.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/uuid.h>
+#include <linux/stm.h>
+#include "stm.h"
+
+enum sys_t_message_type {
+	MIPI_SYST_TYPE_BUILD	= 0,
+	MIPI_SYST_TYPE_SHORT32,
+	MIPI_SYST_TYPE_STRING,
+	MIPI_SYST_TYPE_CATALOG,
+	MIPI_SYST_TYPE_RAW	= 6,
+	MIPI_SYST_TYPE_SHORT64,
+	MIPI_SYST_TYPE_CLOCK,
+};
+
+enum sys_t_message_severity {
+	MIPI_SYST_SEVERITY_MAX	= 0,
+	MIPI_SYST_SEVERITY_FATAL,
+	MIPI_SYST_SEVERITY_ERROR,
+	MIPI_SYST_SEVERITY_WARNING,
+	MIPI_SYST_SEVERITY_INFO,
+	MIPI_SYST_SEVERITY_USER1,
+	MIPI_SYST_SEVERITY_USER2,
+	MIPI_SYST_SEVERITY_DEBUG,
+};
+
+enum sys_t_message_build_subtype {
+	MIPI_SYST_BUILD_ID_COMPACT32 = 0,
+	MIPI_SYST_BUILD_ID_COMPACT64,
+	MIPI_SYST_BUILD_ID_LONG,
+};
+
+enum sys_t_message_clock_subtype {
+	MIPI_SYST_CLOCK_TRANSPORT_SYNC = 1,
+};
+
+enum sys_t_message_string_subtype {
+	MIPI_SYST_STRING_GENERIC	= 1,
+	MIPI_SYST_STRING_FUNCTIONENTER,
+	MIPI_SYST_STRING_FUNCTIONEXIT,
+	MIPI_SYST_STRING_INVALIDPARAM	= 5,
+	MIPI_SYST_STRING_ASSERT		= 7,
+	MIPI_SYST_STRING_PRINTF_32	= 11,
+	MIPI_SYST_STRING_PRINTF_64	= 12,
+};
+
+#define MIPI_SYST_TYPE(t)		((u32)(MIPI_SYST_TYPE_ ## t))
+#define MIPI_SYST_SEVERITY(s)		((u32)(MIPI_SYST_SEVERITY_ ## s) << 4)
+#define MIPI_SYST_OPT_LOC		BIT(8)
+#define MIPI_SYST_OPT_LEN		BIT(9)
+#define MIPI_SYST_OPT_CHK		BIT(10)
+#define MIPI_SYST_OPT_TS		BIT(11)
+#define MIPI_SYST_UNIT(u)		((u32)(u) << 12)
+#define MIPI_SYST_ORIGIN(o)		((u32)(o) << 16)
+#define MIPI_SYST_OPT_GUID		BIT(23)
+#define MIPI_SYST_SUBTYPE(s)		((u32)(MIPI_SYST_ ## s) << 24)
+#define MIPI_SYST_UNITLARGE(u)		(MIPI_SYST_UNIT(u & 0xf) | \
+					 MIPI_SYST_ORIGIN(u >> 4))
+#define MIPI_SYST_TYPES(t, s)		(MIPI_SYST_TYPE(t) | \
+					 MIPI_SYST_SUBTYPE(t ## _ ## s))
+
+#define DATA_HEADER	(MIPI_SYST_TYPES(STRING, GENERIC)	| \
+			 MIPI_SYST_SEVERITY(INFO)		| \
+			 MIPI_SYST_OPT_GUID)
+
+struct sys_t_policy_node {
+	uuid_t		uuid;
+	bool		do_len;
+	unsigned long	ts_interval;
+};
+
+struct sys_t_output {
+	struct sys_t_policy_node	node;
+	unsigned long	ts_jiffies;
+};
+
+static void sys_t_policy_node_init(void *priv)
+{
+	struct sys_t_policy_node *pn = priv;
+
+	generate_random_uuid(pn->uuid.b);
+}
+
+static int sys_t_output_open(void *priv, struct stm_output *output)
+{
+	struct sys_t_policy_node *pn = priv;
+	struct sys_t_output *opriv;
+
+	opriv = kzalloc(sizeof(*opriv), GFP_ATOMIC);
+	if (!opriv)
+		return -ENOMEM;
+
+	memcpy(&opriv->node, pn, sizeof(opriv->node));
+	output->pdrv_private = opriv;
+
+	return 0;
+}
+
+static void sys_t_output_close(struct stm_output *output)
+{
+	kfree(output->pdrv_private);
+}
+
+static ssize_t sys_t_policy_uuid_show(struct config_item *item,
+				      char *page)
+{
+	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
+
+	return sprintf(page, "%pU\n", &pn->uuid);
+}
+
+static ssize_t
+sys_t_policy_uuid_store(struct config_item *item, const char *page,
+			size_t count)
+{
+	struct mutex *mutexp = &item->ci_group->cg_subsys->su_mutex;
+	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
+	int ret;
+
+	mutex_lock(mutexp);
+	ret = uuid_parse(page, &pn->uuid);
+	mutex_unlock(mutexp);
+
+	return ret < 0 ? ret : count;
+}
+
+CONFIGFS_ATTR(sys_t_policy_, uuid);
+
+static ssize_t sys_t_policy_do_len_show(struct config_item *item,
+				      char *page)
+{
+	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
+
+	return sprintf(page, "%d\n", pn->do_len);
+}
+
+static ssize_t
+sys_t_policy_do_len_store(struct config_item *item, const char *page,
+			size_t count)
+{
+	struct mutex *mutexp = &item->ci_group->cg_subsys->su_mutex;
+	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
+	int ret;
+
+	mutex_lock(mutexp);
+	ret = kstrtobool(page, &pn->do_len);
+	mutex_unlock(mutexp);
+
+	return ret ? ret : count;
+}
+
+CONFIGFS_ATTR(sys_t_policy_, do_len);
+
+static ssize_t sys_t_policy_ts_interval_show(struct config_item *item,
+					     char *page)
+{
+	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
+
+	return sprintf(page, "%u\n", jiffies_to_msecs(pn->ts_interval));
+}
+
+static ssize_t
+sys_t_policy_ts_interval_store(struct config_item *item, const char *page,
+			       size_t count)
+{
+	struct mutex *mutexp = &item->ci_group->cg_subsys->su_mutex;
+	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
+	unsigned int ms;
+	int ret;
+
+	mutex_lock(mutexp);
+	ret = kstrtouint(page, 10, &ms);
+	mutex_unlock(mutexp);
+
+	if (!ret) {
+		pn->ts_interval = msecs_to_jiffies(ms);
+		return count;
+	}
+
+	return ret;
+}
+
+CONFIGFS_ATTR(sys_t_policy_, ts_interval);
+
+static struct configfs_attribute *sys_t_policy_attrs[] = {
+	&sys_t_policy_attr_uuid,
+	&sys_t_policy_attr_do_len,
+	&sys_t_policy_attr_ts_interval,
+	NULL,
+};
+
+static inline bool sys_t_need_ts(struct sys_t_output *op)
+{
+	if (op->node.ts_interval &&
+	    time_after(op->ts_jiffies + op->node.ts_interval, jiffies)) {
+		op->ts_jiffies = jiffies;
+
+		return true;
+	}
+
+	return false;
+}
+
+static ssize_t sys_t_write(struct stm_data *data, struct stm_output *output,
+			   unsigned int chan, const char *buf, size_t count)
+{
+	struct sys_t_output *op = output->pdrv_private;
+	unsigned int c = output->channel + chan;
+	unsigned int m = output->master;
+	u32 header = DATA_HEADER;
+	ssize_t sz;
+
+	/* We require an existing policy node to proceed */
+	if (!op)
+		return -EINVAL;
+
+	if (op->node.do_len)
+		header |= MIPI_SYST_OPT_LEN;
+	if (sys_t_need_ts(op))
+		header |= MIPI_SYST_OPT_TS;
+
+	/*
+	 * STP framing rules for SyS-T frames:
+	 *   * the first packet of the SyS-T frame is timestamped;
+	 *   * the last packet is a FLAG.
+	 */
+	/* Message layout: HEADER / GUID / [LENGTH /][TIMESTAMP /] DATA */
+	/* HEADER */
+	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_TIMESTAMPED,
+			  4, (u8 *)&header);
+	if (sz <= 0)
+		return sz;
+
+	/* GUID */
+	sz = stm_data_write(data, m, c, false, op->node.uuid.b, UUID_SIZE);
+	if (sz <= 0)
+		return sz;
+
+	/* [LENGTH] */
+	if (op->node.do_len) {
+		u16 length = count;
+
+		sz = data->packet(data, m, c, STP_PACKET_DATA, 0, 2,
+				  (u8 *)&length);
+		if (sz <= 0)
+			return sz;
+	}
+
+	/* [TIMESTAMP] */
+	if (header & MIPI_SYST_OPT_TS) {
+		u64 ts = ktime_get_real_ns();
+
+		sz = stm_data_write(data, m, c, false, &ts, sizeof(ts));
+		if (sz <= 0)
+			return sz;
+	}
+
+	/* DATA */
+	sz = stm_data_write(data, m, c, false, buf, count);
+	if (sz > 0)
+		data->packet(data, m, c, STP_PACKET_FLAG, 0, 0, buf);
+
+	return sz;
+}
+
+static const struct stm_protocol_driver sys_t_pdrv = {
+	.owner			= THIS_MODULE,
+	.name			= "p_sys-t",
+	.priv_sz		= sizeof(struct sys_t_policy_node),
+	.write			= sys_t_write,
+	.policy_attr		= sys_t_policy_attrs,
+	.policy_node_init	= sys_t_policy_node_init,
+	.output_open		= sys_t_output_open,
+	.output_close		= sys_t_output_close,
+};
+
+static int sys_t_stm_init(void)
+{
+	return stm_register_protocol(&sys_t_pdrv);
+}
+
+static void sys_t_stm_exit(void)
+{
+	stm_unregister_protocol(&sys_t_pdrv);
+}
+
+module_init(sys_t_stm_init);
+module_exit(sys_t_stm_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MIPI SyS-T STM framing protocol driver");
+MODULE_AUTHOR("Alexander Shishkin <alexander.shishkin@linux.intel.com>");
-- 
2.18.0

[QUEUED v20180920 09/16] stm class: p_sys-t: Add support for CLOCKSYNC packets

[Alexander Shishkin]

This adds support for CLOCKSYNC SyS-T packets, that establish correlation
between the transport clock (STP timestamps) and SyS-T timestamps. These
packets are sent periodically to allow the decoder to keep both time
sources in sync.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/p_sys-t.c | 80 +++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/hwtracing/stm/p_sys-t.c b/drivers/hwtracing/stm/p_sys-t.c
index 8f3066ff1b7b..79d40515a84c 100644
--- a/drivers/hwtracing/stm/p_sys-t.c
+++ b/drivers/hwtracing/stm/p_sys-t.c
@@ -72,15 +72,20 @@ enum sys_t_message_string_subtype {
 			 MIPI_SYST_SEVERITY(INFO)		| \
 			 MIPI_SYST_OPT_GUID)
 
+#define CLOCK_SYNC_HEADER	(MIPI_SYST_TYPES(CLOCK, TRANSPORT_SYNC)	| \
+				 MIPI_SYST_SEVERITY(MAX))
+
 struct sys_t_policy_node {
 	uuid_t		uuid;
 	bool		do_len;
 	unsigned long	ts_interval;
+	unsigned long	clocksync_interval;
 };
 
 struct sys_t_output {
 	struct sys_t_policy_node	node;
 	unsigned long	ts_jiffies;
+	unsigned long	clocksync_jiffies;
 };
 
 static void sys_t_policy_node_init(void *priv)
@@ -191,10 +196,42 @@ sys_t_policy_ts_interval_store(struct config_item *item, const char *page,
 
 CONFIGFS_ATTR(sys_t_policy_, ts_interval);
 
+static ssize_t sys_t_policy_clocksync_interval_show(struct config_item *item,
+						    char *page)
+{
+	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
+
+	return sprintf(page, "%u\n", jiffies_to_msecs(pn->clocksync_interval));
+}
+
+static ssize_t
+sys_t_policy_clocksync_interval_store(struct config_item *item,
+				      const char *page, size_t count)
+{
+	struct mutex *mutexp = &item->ci_group->cg_subsys->su_mutex;
+	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
+	unsigned int ms;
+	int ret;
+
+	mutex_lock(mutexp);
+	ret = kstrtouint(page, 10, &ms);
+	mutex_unlock(mutexp);
+
+	if (!ret) {
+		pn->clocksync_interval = msecs_to_jiffies(ms);
+		return count;
+	}
+
+	return ret;
+}
+
+CONFIGFS_ATTR(sys_t_policy_, clocksync_interval);
+
 static struct configfs_attribute *sys_t_policy_attrs[] = {
 	&sys_t_policy_attr_uuid,
 	&sys_t_policy_attr_do_len,
 	&sys_t_policy_attr_ts_interval,
+	&sys_t_policy_attr_clocksync_interval,
 	NULL,
 };
 
@@ -210,6 +247,43 @@ static inline bool sys_t_need_ts(struct sys_t_output *op)
 	return false;
 }
 
+static bool sys_t_need_clock_sync(struct sys_t_output *op)
+{
+	if (op->node.clocksync_interval &&
+	    time_after(op->clocksync_jiffies + op->node.clocksync_interval,
+		       jiffies)) {
+		op->clocksync_jiffies = jiffies;
+
+		return true;
+	}
+
+	return false;
+}
+
+static ssize_t
+sys_t_clock_sync(struct stm_data *data, unsigned int m, unsigned int c)
+{
+	u32 header = CLOCK_SYNC_HEADER;
+	unsigned char nil = 0;
+	u64 payload[2]; /* Clock value and frequency */
+	ssize_t sz;
+
+	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_TIMESTAMPED,
+			  4, (u8 *)&header);
+	if (sz <= 0)
+		return sz;
+
+	payload[0] = ktime_get_real_ns();
+	payload[1] = NSEC_PER_SEC;
+	sz = stm_data_write(data, m, c, false, &payload, sizeof(payload));
+	if (sz <= 0)
+		return sz;
+
+	data->packet(data, m, c, STP_PACKET_FLAG, 0, 0, &nil);
+
+	return sizeof(header) + sizeof(payload);
+}
+
 static ssize_t sys_t_write(struct stm_data *data, struct stm_output *output,
 			   unsigned int chan, const char *buf, size_t count)
 {
@@ -223,6 +297,12 @@ static ssize_t sys_t_write(struct stm_data *data, struct stm_output *output,
 	if (!op)
 		return -EINVAL;
 
+	if (sys_t_need_clock_sync(op)) {
+		sz = sys_t_clock_sync(data, m, c);
+		if (sz <= 0)
+			return sz;
+	}
+
 	if (op->node.do_len)
 		header |= MIPI_SYST_OPT_LEN;
 	if (sys_t_need_ts(op))
-- 
2.18.0

[QUEUED v20180920 10/16] stm class: p_sys-t: Document the configfs interface

[Alexander Shishkin]

This adds ABI documentation for the new configfs attributes that come
with the MIPI SyS-T protocol driver.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 .../ABI/testing/configfs-stp-policy-p_sys-t   | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/ABI/testing/configfs-stp-policy-p_sys-t

diff --git a/Documentation/ABI/testing/configfs-stp-policy-p_sys-t b/Documentation/ABI/testing/configfs-stp-policy-p_sys-t
new file mode 100644
index 000000000000..b290d1c00dcf
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-stp-policy-p_sys-t
@@ -0,0 +1,41 @@
+What:		/config/stp-policy/<device>:p_sys-t.<policy>/<node>/uuid
+Date:		June 2018
+KernelVersion:	4.19
+Description:
+		UUID source identifier string, RW.
+		Default value is randomly generated at the mkdir <node> time.
+		Data coming from trace sources that use this <node> will be
+		tagged with this UUID in the MIPI SyS-T packet stream, to
+		allow the decoder to discern between different sources
+		within the same master/channel range, and identify the
+		higher level decoders that may be needed for each source.
+
+What:		/config/stp-policy/<device>:p_sys-t.<policy>/<node>/do_len
+Date:		June 2018
+KernelVersion:	4.19
+Description:
+		Include payload length in the MIPI SyS-T header, boolean.
+		If enabled, the SyS-T protocol encoder will include payload
+		length in each packet's metadata. This is normally redundant
+		if the underlying transport protocol supports marking message
+		boundaries (which STP does), so this is off by default.
+
+What:		/config/stp-policy/<device>:p_sys-t.<policy>/<node>/ts_interval
+Date:		June 2018
+KernelVersion:	4.19
+Description:
+		Time interval in milliseconds. Include a timestamp in the
+		MIPI SyS-T packet metadata, if this many milliseconds have
+		passed since the previous packet from this source. Zero is
+		the default and stands for "never send the timestamp".
+
+What:		/config/stp-policy/<device>:p_sys-t.<policy>/<node>/clocksync_interval
+Date:		June 2018
+KernelVersion:	4.19
+Description:
+		Time interval in milliseconds. Send a CLOCKSYNC packet if
+		this many milliseconds have passed since the previous
+		CLOCKSYNC packet from this source. Zero is the default and
+		stands for "never send the CLOCKSYNC". It makes sense to
+		use this option with sources that generate constant and/or
+		periodic data, like stm_heartbeat.
-- 
2.18.0

[QUEUED v20180920 11/16] stm class: Document the MIPI SyS-T protocol usage

[Alexander Shishkin]

Add a document describing MIPI SyS-T protocol driver usage.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 Documentation/trace/sys-t.rst | 62 +++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/trace/sys-t.rst

diff --git a/Documentation/trace/sys-t.rst b/Documentation/trace/sys-t.rst
new file mode 100644
index 000000000000..3d8eb92735e9
--- /dev/null
+++ b/Documentation/trace/sys-t.rst
@@ -0,0 +1,62 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===================
+MIPI SyS-T over STP
+===================
+
+The MIPI SyS-T protocol driver can be used with STM class devices to
+generate standardized trace stream. Aside from being a standard, it
+provides better trace source identification and timestamp correlation.
+
+In order to use the MIPI SyS-T protocol driver with your STM device,
+first, you'll need CONFIG_STM_PROTO_SYS_T.
+
+Now, you can select which protocol driver you want to use when you create
+a policy for your STM device, by specifying it in the policy name:
+
+# mkdir /config/stp-policy/dummy_stm.0:p_sys-t.my-policy/
+
+In other words, the policy name format is extended like this:
+
+  <device_name>:<protocol_name>.<policy_name>
+
+With Intel TH, therefore it can look like "0-sth:p_sys-t.my-policy".
+
+If the protocol name is omitted, the STM class will chose whichever
+protocol driver was loaded first.
+
+You can also double check that everything is working as expected by
+
+# cat /config/stp-policy/dummy_stm.0:p_sys-t.my-policy/protocol
+p_sys-t
+
+Now, with the MIPI SyS-T protocol driver, each policy node in the
+configfs gets a few additional attributes, which determine per-source
+parameters specific to the protocol:
+
+# mkdir /config/stp-policy/dummy_stm.0:p_sys-t.my-policy/default
+# ls /config/stp-policy/dummy_stm.0:p_sys-t.my-policy/default
+channels
+clocksync_interval
+do_len
+masters
+ts_interval
+uuid
+
+The most important one here is the "uuid", which determines the UUID
+that will be used to tag all data coming from this source. It is
+automatically generated when a new node is created, but it is likely
+that you would want to change it.
+
+do_len switches on/off the additional "payload length" field in the
+MIPI SyS-T message header. It is off by default as the STP already
+marks message boundaries.
+
+ts_interval and clocksync_interval determine how much time in milliseconds
+can pass before we need to include a protocol (not transport, aka STP)
+timestamp in a message header or send a CLOCKSYNC packet, respectively.
+
+See Documentation/ABI/testing/configfs-stp-policy-p_sys-t for more
+details.
+
+* [1] https://www.mipi.org/specifications/sys-t
-- 
2.18.0

[QUEUED v20180920 12/16] intel_th: SPDX-ify the documentation

[Alexander Shishkin]

Add the SPDX header to the Intel TH documentation.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 Documentation/trace/intel_th.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/trace/intel_th.rst b/Documentation/trace/intel_th.rst
index 19e2d633f3c7..baa12eb09ef4 100644
--- a/Documentation/trace/intel_th.rst
+++ b/Documentation/trace/intel_th.rst
@@ -1,3 +1,5 @@
+.. SPDX-License-Identifier: GPL-2.0
+
 =======================
 Intel(R) Trace Hub (TH)
 =======================
-- 
2.18.0

[QUEUED v20180920 13/16] stm class: SPDX-ify the documentation

[Alexander Shishkin]

Add the SPDX header to the STM class documentation.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 Documentation/trace/stm.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/trace/stm.rst b/Documentation/trace/stm.rst
index 2c22ddb7fd3e..56b959a39814 100644
--- a/Documentation/trace/stm.rst
+++ b/Documentation/trace/stm.rst
@@ -1,3 +1,5 @@
+.. SPDX-License-Identifier: GPL-2.0
+
 ===================
 System Trace Module
 ===================
-- 
2.18.0

[QUEUED v20180920 14/16] stm class: heartbeat: Fix whitespace

[Alexander Shishkin]

Fix whitespace in the code for better readability, no functional changes.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/heartbeat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwtracing/stm/heartbeat.c b/drivers/hwtracing/stm/heartbeat.c
index 7db42395e131..3e7df1c0477f 100644
--- a/drivers/hwtracing/stm/heartbeat.c
+++ b/drivers/hwtracing/stm/heartbeat.c
@@ -76,7 +76,7 @@ static int stm_heartbeat_init(void)
 			goto fail_unregister;
 
 		stm_heartbeat[i].data.nr_chans	= 1;
-		stm_heartbeat[i].data.link		= stm_heartbeat_link;
+		stm_heartbeat[i].data.link	= stm_heartbeat_link;
 		stm_heartbeat[i].data.unlink	= stm_heartbeat_unlink;
 		hrtimer_init(&stm_heartbeat[i].hrtimer, CLOCK_MONOTONIC,
 			     HRTIMER_MODE_ABS);
-- 
2.18.0

[QUEUED v20180920 15/16] lib: Add memcat_p(): paste 2 pointer arrays together

[Alexander Shishkin]

This adds a helper to paste 2 pointer arrays together, useful for merging
various types of attribute arrays. There are a few places in the kernel
tree where this is open coded, and I just added one more in the STM class.

The naming is inspired by memset_p() and memcat(), and partial credit for
it goes to Andy Shevchenko.

This patch adds the function wrapped in a type-enforcing macro and a test
module.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/string.h |   7 +++
 lib/Kconfig.debug      |   8 +++
 lib/Makefile           |   1 +
 lib/string.c           |  31 +++++++++++
 lib/test_memcat_p.c    | 115 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 162 insertions(+)
 create mode 100644 lib/test_memcat_p.c

diff --git a/include/linux/string.h b/include/linux/string.h
index 4a5a0eb7df51..27d0482e5e05 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -131,6 +131,13 @@ static inline void *memset_p(void **p, void *v, __kernel_size_t n)
 		return memset64((uint64_t *)p, (uintptr_t)v, n);
 }
 
+extern void **__memcat_p(void **a, void **b);
+#define memcat_p(a, b) ({					\
+	BUILD_BUG_ON_MSG(!__same_type(*(a), *(b)),		\
+			 "type mismatch in memcat_p()");	\
+	(typeof(*a) *)__memcat_p((void **)(a), (void **)(b));	\
+})
+
 #ifndef __HAVE_ARCH_MEMCPY
 extern void * memcpy(void *,const void *,__kernel_size_t);
 #endif
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 613316724c6a..177e21006359 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1965,6 +1965,14 @@ config TEST_DEBUG_VIRTUAL
 
 	  If unsure, say N.
 
+config TEST_MEMCAT_P
+	tristate "Test memcat_p() helper function"
+	help
+	  Test the memcat_p() helper for correctly merging two
+	  pointer arrays together.
+
+	  If unsure, say N.
+
 endif # RUNTIME_TESTING_MENU
 
 config MEMTEST
diff --git a/lib/Makefile b/lib/Makefile
index ca3f7ebb900d..c2588a2d7b1e 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_TEST_UUID) += test_uuid.o
 obj-$(CONFIG_TEST_PARMAN) += test_parman.o
 obj-$(CONFIG_TEST_KMOD) += test_kmod.o
 obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
+obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/string.c b/lib/string.c
index 2c0900a5d51a..453f35994eb6 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -27,6 +27,7 @@
 #include <linux/export.h>
 #include <linux/bug.h>
 #include <linux/errno.h>
+#include <linux/slab.h>
 
 #include <asm/byteorder.h>
 #include <asm/word-at-a-time.h>
@@ -890,6 +891,36 @@ void *memscan(void *addr, int c, size_t size)
 EXPORT_SYMBOL(memscan);
 #endif
 
+/*
+ * Merge two NULL-terminated pointer arrays into a newly allocated
+ * array, which is also NULL-terminated. Nomenclature is inspired by
+ * memset_p() and memcat() found elsewhere in the kernel source tree.
+ */
+void **__memcat_p(void **a, void **b)
+{
+	void **p = a, **new;
+	int nr;
+
+	/* count the elements in both arrays */
+	for (nr = 0, p = a; *p; nr++, p++)
+		;
+	for (p = b; *p; nr++, p++)
+		;
+	/* one for the NULL-terminator */
+	nr++;
+
+	new = kmalloc_array(nr, sizeof(void *), GFP_KERNEL);
+	if (!new)
+		return NULL;
+
+	/* nr -> last index; p points to NULL in b[] */
+	for (nr--; nr >= 0; nr--, p = p == b ? &a[nr] : p - 1)
+		new[nr] = *p;
+
+	return new;
+}
+EXPORT_SYMBOL_GPL(__memcat_p);
+
 #ifndef __HAVE_ARCH_STRSTR
 /**
  * strstr - Find the first substring in a %NUL terminated string
diff --git a/lib/test_memcat_p.c b/lib/test_memcat_p.c
new file mode 100644
index 000000000000..2b163a749ecb
--- /dev/null
+++ b/lib/test_memcat_p.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for memcat_p() in lib/string.c
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+
+struct test_struct {
+	int		num;
+	unsigned int	magic;
+};
+
+#define MAGIC		0xf00ff00f
+/* Size of each of the NULL-terminated input arrays */
+#define INPUT_MAX	128
+/* Expected number of non-NULL elements in the output array */
+#define EXPECT		(INPUT_MAX * 2 - 2)
+
+static int __init test_memcat_p_init(void)
+{
+	struct test_struct **in0, **in1, **out, **p;
+	int err = -ENOMEM, i, r, total = 0;
+
+	in0 = kcalloc(INPUT_MAX, sizeof(*in0), GFP_KERNEL);
+	if (!in0)
+		return err;
+
+	in1 = kcalloc(INPUT_MAX, sizeof(*in1), GFP_KERNEL);
+	if (!in1)
+		goto err_free_in0;
+
+	for (i = 0, r = 1; i < INPUT_MAX - 1; i++) {
+		in0[i] = kmalloc(sizeof(**in0), GFP_KERNEL);
+		if (!in0[i])
+			goto err_free_elements;
+
+		in1[i] = kmalloc(sizeof(**in1), GFP_KERNEL);
+		if (!in1[i]) {
+			kfree(in0[i]);
+			goto err_free_elements;
+		}
+
+		/* lifted from test_sort.c */
+		r = (r * 725861) % 6599;
+		in0[i]->num = r;
+		in1[i]->num = -r;
+		in0[i]->magic = MAGIC;
+		in1[i]->magic = MAGIC;
+	}
+
+	in0[i] = in1[i] = NULL;
+
+	out = memcat_p(in0, in1);
+	if (!out)
+		goto err_free_all_elements;
+
+	err = -EINVAL;
+	for (i = 0, p = out; *p && (i < INPUT_MAX * 2 - 1); p++, i++) {
+		total += (*p)->num;
+
+		if ((*p)->magic != MAGIC) {
+			pr_err("test failed: wrong magic at %d: %u\n", i,
+			       (*p)->magic);
+			goto err_free_out;
+		}
+	}
+
+	if (total) {
+		pr_err("test failed: expected zero total, got %d\n", total);
+		goto err_free_out;
+	}
+
+	if (i != EXPECT) {
+		pr_err("test failed: expected output size %d, got %d\n",
+		       EXPECT, i);
+		goto err_free_out;
+	}
+
+	for (i = 0; i < INPUT_MAX - 1; i++)
+		if (out[i] != in0[i] || out[i + INPUT_MAX - 1] != in1[i]) {
+			pr_err("test failed: wrong element order at %d\n", i);
+			goto err_free_out;
+		}
+
+	err = 0;
+	pr_info("test passed\n");
+
+err_free_out:
+	kfree(out);
+err_free_all_elements:
+	i = INPUT_MAX;
+err_free_elements:
+	for (i--; i >= 0; i--) {
+		kfree(in1[i]);
+		kfree(in0[i]);
+	}
+
+	kfree(in1);
+err_free_in0:
+	kfree(in0);
+
+	return err;
+}
+
+static void __exit test_memcat_p_exit(void)
+{
+}
+
+module_init(test_memcat_p_init);
+module_exit(test_memcat_p_exit);
+
+MODULE_LICENSE("GPL");
-- 
2.18.0

[QUEUED v20180920 16/16] stm class: Use memcat_p()

[Alexander Shishkin]

Instead of a local copy, use the memcat_p() helper to merge policy
node attributes.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/policy.c | 29 +----------------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 894598cfe2b7..2e3841ee7030 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -202,33 +202,6 @@ static struct configfs_attribute *stp_policy_node_attrs[] = {
 static const struct config_item_type stp_policy_type;
 static const struct config_item_type stp_policy_node_type;
 
-/* lifted from arch/x86/events/core.c */
-static struct configfs_attribute **merge_attr(struct configfs_attribute **a, struct configfs_attribute **b)
-{
-	struct configfs_attribute **new;
-	int j, i;
-
-	for (j = 0; a[j]; j++)
-		;
-	for (i = 0; b[i]; i++)
-		j++;
-	j++;
-
-	new = kmalloc_array(j, sizeof(struct configfs_attribute *),
-			    GFP_KERNEL);
-	if (!new)
-		return NULL;
-
-	j = 0;
-	for (i = 0; a[i]; i++)
-		new[j++] = a[i];
-	for (i = 0; b[i]; i++)
-		new[j++] = b[i];
-	new[j] = NULL;
-
-	return new;
-}
-
 const struct config_item_type *
 get_policy_node_type(struct configfs_attribute **attrs)
 {
@@ -240,7 +213,7 @@ get_policy_node_type(struct configfs_attribute **attrs)
 	if (!type)
 		return NULL;
 
-	merged = merge_attr(stp_policy_node_attrs, attrs);
+	merged = memcat_p(stp_policy_node_attrs, attrs);
 	if (!merged) {
 		kfree(type);
 		return NULL;
-- 
2.18.0

Re: [QUEUED v20180920 15/16] lib: Add memcat_p(): paste 2 pointer arrays together

[Greg Kroah-Hartman]

On Thu, Sep 20, 2018 at 03:45:52PM +0300, Alexander Shishkin wrote:
> This adds a helper to paste 2 pointer arrays together, useful for merging
> various types of attribute arrays. There are a few places in the kernel
> tree where this is open coded, and I just added one more in the STM class.
> 
> The naming is inspired by memset_p() and memcat(), and partial credit for
> it goes to Andy Shevchenko.
> 
> This patch adds the function wrapped in a type-enforcing macro and a test
> module.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

It would be good to get someone else to review/ack a core function like
this, to verify nothing is incorrect.  Especially given the fun of
off-by-one issues in C we have had over the years...

thanks,

greg k-h

Re: [QUEUED v20180920 15/16] lib: Add memcat_p(): paste 2 pointer arrays together

[Andy Shevchenko]

On Thu, Sep 20, 2018 at 03:45:52PM +0300, Alexander Shishkin wrote:
> This adds a helper to paste 2 pointer arrays together, useful for merging
> various types of attribute arrays. There are a few places in the kernel
> tree where this is open coded, and I just added one more in the STM class.
> 
> The naming is inspired by memset_p() and memcat(), and partial credit for
> it goes to Andy Shevchenko.
> 
> This patch adds the function wrapped in a type-enforcing macro and a test
> module.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/string.h |   7 +++
>  lib/Kconfig.debug      |   8 +++
>  lib/Makefile           |   1 +
>  lib/string.c           |  31 +++++++++++
>  lib/test_memcat_p.c    | 115 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 162 insertions(+)
>  create mode 100644 lib/test_memcat_p.c
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 4a5a0eb7df51..27d0482e5e05 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -131,6 +131,13 @@ static inline void *memset_p(void **p, void *v, __kernel_size_t n)
>  		return memset64((uint64_t *)p, (uintptr_t)v, n);
>  }
>  
> +extern void **__memcat_p(void **a, void **b);
> +#define memcat_p(a, b) ({					\
> +	BUILD_BUG_ON_MSG(!__same_type(*(a), *(b)),		\
> +			 "type mismatch in memcat_p()");	\
> +	(typeof(*a) *)__memcat_p((void **)(a), (void **)(b));	\
> +})
> +
>  #ifndef __HAVE_ARCH_MEMCPY
>  extern void * memcpy(void *,const void *,__kernel_size_t);
>  #endif
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 613316724c6a..177e21006359 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1965,6 +1965,14 @@ config TEST_DEBUG_VIRTUAL
>  
>  	  If unsure, say N.
>  
> +config TEST_MEMCAT_P
> +	tristate "Test memcat_p() helper function"
> +	help
> +	  Test the memcat_p() helper for correctly merging two
> +	  pointer arrays together.
> +
> +	  If unsure, say N.
> +
>  endif # RUNTIME_TESTING_MENU
>  
>  config MEMTEST
> diff --git a/lib/Makefile b/lib/Makefile
> index ca3f7ebb900d..c2588a2d7b1e 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_TEST_UUID) += test_uuid.o
>  obj-$(CONFIG_TEST_PARMAN) += test_parman.o
>  obj-$(CONFIG_TEST_KMOD) += test_kmod.o
>  obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
> +obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o
>  
>  ifeq ($(CONFIG_DEBUG_KOBJECT),y)
>  CFLAGS_kobject.o += -DDEBUG
> diff --git a/lib/string.c b/lib/string.c
> index 2c0900a5d51a..453f35994eb6 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -27,6 +27,7 @@
>  #include <linux/export.h>
>  #include <linux/bug.h>
>  #include <linux/errno.h>
> +#include <linux/slab.h>
>  
>  #include <asm/byteorder.h>
>  #include <asm/word-at-a-time.h>
> @@ -890,6 +891,36 @@ void *memscan(void *addr, int c, size_t size)
>  EXPORT_SYMBOL(memscan);
>  #endif
>  
> +/*
> + * Merge two NULL-terminated pointer arrays into a newly allocated
> + * array, which is also NULL-terminated. Nomenclature is inspired by
> + * memset_p() and memcat() found elsewhere in the kernel source tree.
> + */
> +void **__memcat_p(void **a, void **b)
> +{
> +	void **p = a, **new;
> +	int nr;
> +
> +	/* count the elements in both arrays */
> +	for (nr = 0, p = a; *p; nr++, p++)
> +		;
> +	for (p = b; *p; nr++, p++)
> +		;
> +	/* one for the NULL-terminator */
> +	nr++;
> +
> +	new = kmalloc_array(nr, sizeof(void *), GFP_KERNEL);
> +	if (!new)
> +		return NULL;
> +
> +	/* nr -> last index; p points to NULL in b[] */
> +	for (nr--; nr >= 0; nr--, p = p == b ? &a[nr] : p - 1)
> +		new[nr] = *p;

As far as I understand the logic behind this initially we have:
	n elements + NULL + m elements + NULL,
	nr equals n + m + NULL,
nr - 1 points to the last element in the new array at the beginning of the
loop. Since we are operating on arrays, p at some point becomes 'b' at which
time we switch to the 'a' array. It might be rewritten slightly more
straightforward (like in two loops, but on the other hand it would require to
track number of elements in each of the arrays separately).

So, I don't see any off-by-one issues here.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> +
> +	return new;
> +}
> +EXPORT_SYMBOL_GPL(__memcat_p);
> +
>  #ifndef __HAVE_ARCH_STRSTR
>  /**
>   * strstr - Find the first substring in a %NUL terminated string
> diff --git a/lib/test_memcat_p.c b/lib/test_memcat_p.c
> new file mode 100644
> index 000000000000..2b163a749ecb
> --- /dev/null
> +++ b/lib/test_memcat_p.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test cases for memcat_p() in lib/string.c
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +
> +struct test_struct {
> +	int		num;
> +	unsigned int	magic;
> +};
> +
> +#define MAGIC		0xf00ff00f
> +/* Size of each of the NULL-terminated input arrays */
> +#define INPUT_MAX	128
> +/* Expected number of non-NULL elements in the output array */
> +#define EXPECT		(INPUT_MAX * 2 - 2)
> +
> +static int __init test_memcat_p_init(void)
> +{
> +	struct test_struct **in0, **in1, **out, **p;
> +	int err = -ENOMEM, i, r, total = 0;
> +
> +	in0 = kcalloc(INPUT_MAX, sizeof(*in0), GFP_KERNEL);
> +	if (!in0)
> +		return err;
> +
> +	in1 = kcalloc(INPUT_MAX, sizeof(*in1), GFP_KERNEL);
> +	if (!in1)
> +		goto err_free_in0;
> +
> +	for (i = 0, r = 1; i < INPUT_MAX - 1; i++) {
> +		in0[i] = kmalloc(sizeof(**in0), GFP_KERNEL);
> +		if (!in0[i])
> +			goto err_free_elements;
> +
> +		in1[i] = kmalloc(sizeof(**in1), GFP_KERNEL);
> +		if (!in1[i]) {
> +			kfree(in0[i]);
> +			goto err_free_elements;
> +		}
> +
> +		/* lifted from test_sort.c */
> +		r = (r * 725861) % 6599;
> +		in0[i]->num = r;
> +		in1[i]->num = -r;
> +		in0[i]->magic = MAGIC;
> +		in1[i]->magic = MAGIC;
> +	}
> +
> +	in0[i] = in1[i] = NULL;
> +
> +	out = memcat_p(in0, in1);
> +	if (!out)
> +		goto err_free_all_elements;
> +
> +	err = -EINVAL;
> +	for (i = 0, p = out; *p && (i < INPUT_MAX * 2 - 1); p++, i++) {
> +		total += (*p)->num;
> +
> +		if ((*p)->magic != MAGIC) {
> +			pr_err("test failed: wrong magic at %d: %u\n", i,
> +			       (*p)->magic);
> +			goto err_free_out;
> +		}
> +	}
> +
> +	if (total) {
> +		pr_err("test failed: expected zero total, got %d\n", total);
> +		goto err_free_out;
> +	}
> +
> +	if (i != EXPECT) {
> +		pr_err("test failed: expected output size %d, got %d\n",
> +		       EXPECT, i);
> +		goto err_free_out;
> +	}
> +
> +	for (i = 0; i < INPUT_MAX - 1; i++)
> +		if (out[i] != in0[i] || out[i + INPUT_MAX - 1] != in1[i]) {
> +			pr_err("test failed: wrong element order at %d\n", i);
> +			goto err_free_out;
> +		}
> +
> +	err = 0;
> +	pr_info("test passed\n");
> +
> +err_free_out:
> +	kfree(out);
> +err_free_all_elements:
> +	i = INPUT_MAX;
> +err_free_elements:
> +	for (i--; i >= 0; i--) {
> +		kfree(in1[i]);
> +		kfree(in0[i]);
> +	}
> +
> +	kfree(in1);
> +err_free_in0:
> +	kfree(in0);
> +
> +	return err;
> +}
> +
> +static void __exit test_memcat_p_exit(void)
> +{
> +}
> +
> +module_init(test_memcat_p_init);
> +module_exit(test_memcat_p_exit);
> +
> +MODULE_LICENSE("GPL");
> -- 
> 2.18.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [QUEUED v20180920 01/16] stm class: Rework policy node fallback

[Mathieu Poirier]

Hi Alex,

On Thu, Sep 20, 2018 at 03:45:38PM +0300, Alexander Shishkin wrote:
> Currently, if no matching policy node can be found for a trace source,
> we'll try to use "default" policy node, then, if that doesn't exist,
> we'll pick the first node, in order of creation. If that also fails,
> we'll allocate M/C range from the beginning of the device's M/C range.
> 
> This makes it difficult to know which node (if any) was used in any
> particular case.
> 
> In order to make things more deterministic, the new order is as follows:
>   * if they supply ID string, use that and nothing else,
>   * if they are a task, use their task name (comm),
>   * use "default", if it exists,
>   * return failure, to let them know there is no suitable rule.

I am good with the "default" catch all clause but the change in behaviour has to
be documented.  Otherwise people can be mislead in thinking that a regression
has happened.

Before this patch:

root@nano:~# mkdir /sys/kernel/config/stp-policy/20100000.stm.test
root@nano:~# echo 20100000.stm > /sys/class/stm_source/ftrace/stm_source_link
root@linaro-nano:~# cat /sys/class/stm_source/ftrace/stm_source_link
20100000.stm

After this patch:
root@nano:~# mkdir /sys/kernel/config/stp-policy/20100000.stm.test
root@nano:~# echo 20100000.stm > /sys/class/stm_source/ftrace/stm_source_link
bash: echo: write error: Invalid argument
root@nano:~# mkdir /sys/kernel/config/stp-policy/20100000.stm.test/default
root@nano:~# echo 20100000.stm > /sys/class/stm_source/ftrace/stm_source_link
root@linaro-nano:~# cat /sys/class/stm_source/ftrace/stm_source_link
20100000.stm

> 
> This should provide enough convenience with the "default" catch-all node,
> while not leaving *everything* to chance. As a side effect, this relaxes
> the requirement of using ioctl() for identification with the possibility of
> using task names as policy nodes.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/stm/core.c   | 89 ++++++++++++++++++++--------------
>  drivers/hwtracing/stm/policy.c | 12 ++---
>  drivers/hwtracing/stm/stm.h    |  2 -
>  3 files changed, 58 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index 10bcb5d73f90..7b1c549d6320 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -293,15 +293,15 @@ static int stm_output_assign(struct stm_device *stm, unsigned int width,
>  	if (width > stm->data->sw_nchannels)
>  		return -EINVAL;
>  
> -	if (policy_node) {
> -		stp_policy_node_get_ranges(policy_node,
> -					   &midx, &mend, &cidx, &cend);
> -	} else {
> -		midx = stm->data->sw_start;
> -		cidx = 0;
> -		mend = stm->data->sw_end;
> -		cend = stm->data->sw_nchannels - 1;
> -	}
> +	/* We no longer accept policy_node==NULL here */
> +	if (WARN_ON_ONCE(!policy_node))
> +		return -EINVAL;
> +
> +	/*
> +	 * Also, the caller holds reference to policy_node, so it won't
> +	 * disappear on us.
> +	 */
> +	stp_policy_node_get_ranges(policy_node, &midx, &mend, &cidx, &cend);
>  
>  	spin_lock(&stm->mc_lock);
>  	spin_lock(&output->lock);
> @@ -405,19 +405,30 @@ static int stm_char_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> -static int stm_file_assign(struct stm_file *stmf, char *id, unsigned int width)
> +static int
> +stm_assign_first_policy(struct stm_device *stm, struct stm_output *output,
> +			char **ids, unsigned int width)
>  {
> -	struct stm_device *stm = stmf->stm;
> -	int ret;
> +	struct stp_policy_node *pn;
> +	int err, n;
>  
> -	stmf->policy_node = stp_policy_node_lookup(stm, id);
> +	/*
> +	 * On success, stp_policy_node_lookup() will return holding the
> +	 * configfs subsystem mutex, which is then released in
> +	 * stp_policy_node_put(). This allows the pdrv->output_open() in
> +	 * stm_output_assign() to serialize against the attribute accessors.
> +	 */
> +	for (n = 0, pn = NULL; ids[n] && !pn; n++)
> +		pn = stp_policy_node_lookup(stm, ids[n]);
>  
> -	ret = stm_output_assign(stm, width, stmf->policy_node, &stmf->output);
> +	if (!pn)
> +		return -EINVAL;
>  
> -	if (stmf->policy_node)
> -		stp_policy_node_put(stmf->policy_node);
> +	err = stm_output_assign(stm, width, pn, output);
>  
> -	return ret;
> +	stp_policy_node_put(pn);
> +
> +	return err;
>  }
>  
>  static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
> @@ -455,16 +466,23 @@ static ssize_t stm_char_write(struct file *file, const char __user *buf,
>  		count = PAGE_SIZE - 1;
>  
>  	/*
> -	 * if no m/c have been assigned to this writer up to this
> -	 * point, use "default" policy entry
> +	 * If no m/c have been assigned to this writer up to this
> +	 * point, try to use the task name and "default" policy entries.
>  	 */
>  	if (!stmf->output.nr_chans) {
> -		err = stm_file_assign(stmf, "default", 1);
> +		char comm[sizeof(current->comm)];
> +		char *ids[] = { comm, "default", NULL };
> +
> +		get_task_comm(comm, current);
> +
> +		err = stm_assign_first_policy(stmf->stm, &stmf->output, ids, 1);
>  		/*
>  		 * EBUSY means that somebody else just assigned this
> -		 * output, which is just fine for write()
> +		 * output, which is just fine for write(), except for (XXX)
> +		 * it doesn't actually return -EBUSY, but -EINVAL and WARNs
> +		 * in this case. Hrrr.
>  		 */
> -		if (err && err != -EBUSY)
> +		if (err)
>  			return err;
>  	}
>  
> @@ -550,6 +568,7 @@ static int stm_char_policy_set_ioctl(struct stm_file *stmf, void __user *arg)
>  {
>  	struct stm_device *stm = stmf->stm;
>  	struct stp_policy_id *id;
> +	char *ids[] = { NULL, NULL };
>  	int ret = -EINVAL;
>  	u32 size;
>  
> @@ -582,7 +601,9 @@ static int stm_char_policy_set_ioctl(struct stm_file *stmf, void __user *arg)
>  	    id->width > PAGE_SIZE / stm->data->sw_mmiosz)
>  		goto err_free;
>  
> -	ret = stm_file_assign(stmf, id->id, id->width);
> +	ids[0] = id->id;
> +	ret = stm_assign_first_policy(stmf->stm, &stmf->output, ids,
> +				      id->width);
>  	if (ret)
>  		goto err_free;
>  
> @@ -818,8 +839,8 @@ EXPORT_SYMBOL_GPL(stm_unregister_device);
>  static int stm_source_link_add(struct stm_source_device *src,
>  			       struct stm_device *stm)
>  {
> -	char *id;
> -	int err;
> +	char *ids[] = { NULL, "default", NULL };
> +	int err = -ENOMEM;
>  
>  	mutex_lock(&stm->link_mutex);
>  	spin_lock(&stm->link_lock);
> @@ -833,19 +854,13 @@ static int stm_source_link_add(struct stm_source_device *src,
>  	spin_unlock(&stm->link_lock);
>  	mutex_unlock(&stm->link_mutex);
>  
> -	id = kstrdup(src->data->name, GFP_KERNEL);
> -	if (id) {
> -		src->policy_node =
> -			stp_policy_node_lookup(stm, id);
> -
> -		kfree(id);
> -	}
> -
> -	err = stm_output_assign(stm, src->data->nr_chans,
> -				src->policy_node, &src->output);
> +	ids[0] = kstrdup(src->data->name, GFP_KERNEL);
> +	if (!ids[0])
> +		goto fail_detach;
>  
> -	if (src->policy_node)
> -		stp_policy_node_put(src->policy_node);
> +	err = stm_assign_first_policy(stm, &src->output, ids,
> +				      src->data->nr_chans);
> +	kfree(ids[0]);
>  
>  	if (err)
>  		goto fail_detach;
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index 3fd07e275b34..15d35d891643 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -392,7 +392,7 @@ static struct configfs_subsystem stp_policy_subsys = {
>  static struct stp_policy_node *
>  __stp_policy_node_lookup(struct stp_policy *policy, char *s)
>  {
> -	struct stp_policy_node *policy_node, *ret;
> +	struct stp_policy_node *policy_node, *ret = NULL;
>  	struct list_head *head = &policy->group.cg_children;
>  	struct config_item *item;
>  	char *start, *end = s;
> @@ -400,10 +400,6 @@ __stp_policy_node_lookup(struct stp_policy *policy, char *s)
>  	if (list_empty(head))
>  		return NULL;
>  
> -	/* return the first entry if everything else fails */
> -	item = list_entry(head->next, struct config_item, ci_entry);
> -	ret = to_stp_policy_node(item);
> -
>  next:
>  	for (;;) {
>  		start = strsep(&end, "/");
> @@ -449,13 +445,17 @@ stp_policy_node_lookup(struct stm_device *stm, char *s)
>  
>  	if (policy_node)
>  		config_item_get(&policy_node->group.cg_item);
> -	mutex_unlock(&stp_policy_subsys.su_mutex);
> +	else
> +		mutex_unlock(&stp_policy_subsys.su_mutex);
>  
>  	return policy_node;
>  }
>  
>  void stp_policy_node_put(struct stp_policy_node *policy_node)
>  {
> +	lockdep_assert_held(&stp_policy_subsys.su_mutex);
> +
> +	mutex_unlock(&stp_policy_subsys.su_mutex);
>  	config_item_put(&policy_node->group.cg_item);
>  }
>  
> diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
> index 923571adc6f4..e5df08ae59cf 100644
> --- a/drivers/hwtracing/stm/stm.h
> +++ b/drivers/hwtracing/stm/stm.h
> @@ -57,7 +57,6 @@ struct stm_output {
>  
>  struct stm_file {
>  	struct stm_device	*stm;
> -	struct stp_policy_node	*policy_node;
>  	struct stm_output	output;
>  };
>  
> @@ -71,7 +70,6 @@ struct stm_source_device {
>  	struct stm_device __rcu	*link;
>  	struct list_head	link_entry;
>  	/* one output per stm_source device */
> -	struct stp_policy_node	*policy_node;
>  	struct stm_output	output;
>  };
>  
> -- 
> 2.18.0
> 

Re: [QUEUED v20180920 04/16] stm class: Introduce framing protocol drivers

[Mathieu Poirier]

On Thu, Sep 20, 2018 at 03:45:41PM +0300, Alexander Shishkin wrote:
> At the moment, the stm class applies a certain STP framing pattern to
> the data as it is written to the underlying STM device. In order to
> allow different framing patterns (aka protocols), this patch introduces
> the concept of STP protocol drivers, defines data structures and APIs
> for the protocol drivers to use.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/stm/core.c   | 142 ++++++++++++++++++++++++++++++++
>  drivers/hwtracing/stm/policy.c | 145 ++++++++++++++++++++++++++++++---
>  drivers/hwtracing/stm/stm.h    |  52 ++++++++++--
>  3 files changed, 321 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index 7b1c549d6320..c869a30661ac 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -316,11 +316,26 @@ static int stm_output_assign(struct stm_device *stm, unsigned int width,
>  	output->master = midx;
>  	output->channel = cidx;
>  	output->nr_chans = width;
> +	if (stm->pdrv->output_open) {
> +		void *priv = stp_policy_node_priv(policy_node);
> +
> +		if (WARN_ON_ONCE(!priv))
> +			goto unlock;
> +
> +		/* configfs subsys mutex is held by the caller */
> +		ret = stm->pdrv->output_open(priv, output);
> +		if (ret)
> +			goto unlock;
> +	}
> +
>  	stm_output_claim(stm, output);
>  	dev_dbg(&stm->dev, "assigned %u:%u (+%u)\n", midx, cidx, width);
>  
>  	ret = 0;
>  unlock:
> +	if (ret)
> +		output->nr_chans = 0;
> +
>  	spin_unlock(&output->lock);
>  	spin_unlock(&stm->mc_lock);
>  
> @@ -333,6 +348,8 @@ static void stm_output_free(struct stm_device *stm, struct stm_output *output)
>  	spin_lock(&output->lock);
>  	if (output->nr_chans)
>  		stm_output_disclaim(stm, output);
> +	if (stm->pdrv->output_close)
> +		stm->pdrv->output_close(output);
>  	spin_unlock(&output->lock);
>  	spin_unlock(&stm->mc_lock);
>  }
> @@ -349,6 +366,122 @@ static int major_match(struct device *dev, const void *data)
>  	return MAJOR(dev->devt) == major;
>  }
>  
> +/*
> + * Framing protocol management
> + * Modules can implement STM protocol drivers and (un-)register them
> + * with the STM class framework.
> + */
> +static struct list_head stm_pdrv_head;
> +static struct mutex stm_pdrv_mutex;
> +
> +struct stm_pdrv_entry {
> +	struct list_head			entry;
> +	const struct stm_protocol_driver	*pdrv;
> +	const struct config_item_type		*node_type;
> +};
> +
> +static const struct stm_pdrv_entry *
> +__stm_lookup_protocol(const char *name)
> +{
> +	struct stm_pdrv_entry *pe;
> +
> +	list_for_each_entry(pe, &stm_pdrv_head, entry) {
> +		if (!name || !*name || !strcmp(name, pe->pdrv->name))
> +			return pe;
> +	}

Please add a comment asserting your intentions with this loop.  I had to look at
it for quite a while before understanding all the implications it conveys.

> +
> +	return NULL;
> +}
> +
> +int stm_register_protocol(const struct stm_protocol_driver *pdrv)
> +{
> +	struct stm_pdrv_entry *pe = NULL;
> +	int ret = -ENOMEM;
> +
> +	mutex_lock(&stm_pdrv_mutex);
> +
> +	if (__stm_lookup_protocol(pdrv->name)) {
> +		ret = -EEXIST;
> +		goto unlock;
> +	}
> +
> +	pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> +	if (!pe)
> +		goto unlock;
> +
> +	if (pdrv->policy_attr) {
> +		pe->node_type = get_policy_node_type(pdrv->policy_attr);
> +		if (!pe->node_type)
> +			goto unlock;
> +	}
> +
> +	list_add_tail(&pe->entry, &stm_pdrv_head);
> +	pe->pdrv = pdrv;
> +
> +	ret = 0;
> +unlock:
> +	mutex_unlock(&stm_pdrv_mutex);
> +
> +	if (ret)
> +		kfree(pe);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(stm_register_protocol);
> +
> +void stm_unregister_protocol(const struct stm_protocol_driver *pdrv)
> +{
> +	struct stm_pdrv_entry *pe, *iter;
> +
> +	mutex_lock(&stm_pdrv_mutex);
> +
> +	list_for_each_entry_safe(pe, iter, &stm_pdrv_head, entry) {
> +		if (pe->pdrv == pdrv) {
> +			list_del(&pe->entry);
> +
> +			/* XXX: factor out */
> +			if (pe->node_type) {
> +				kfree(pe->node_type->ct_attrs);
> +				kfree(pe->node_type);
> +			}
> +			kfree(pe);
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&stm_pdrv_mutex);
> +}
> +EXPORT_SYMBOL_GPL(stm_unregister_protocol);
> +
> +static bool stm_get_protocol(const struct stm_protocol_driver *pdrv)
> +{
> +	return try_module_get(pdrv->owner);
> +}
> +
> +void stm_put_protocol(const struct stm_protocol_driver *pdrv)
> +{
> +	module_put(pdrv->owner);
> +}
> +
> +int stm_lookup_protocol(const char *name,
> +			const struct stm_protocol_driver **pdrv,
> +			const struct config_item_type **node_type)
> +{
> +	const struct stm_pdrv_entry *pe;
> +
> +	mutex_lock(&stm_pdrv_mutex);
> +
> +	pe = __stm_lookup_protocol(name);
> +	if (pe && pe->pdrv && stm_get_protocol(pe->pdrv)) {
> +		*pdrv = pe->pdrv;
> +		*node_type = pe->node_type;
> +	}
> +
> +	mutex_unlock(&stm_pdrv_mutex);
> +
> +	return pe ? 0 : -ENOENT;
> +}
> +
>  static int stm_char_open(struct inode *inode, struct file *file)
>  {
>  	struct stm_file *stmf;
> @@ -1178,6 +1311,15 @@ static int __init stm_core_init(void)
>  		goto err_src;
>  
>  	init_srcu_struct(&stm_source_srcu);
> +	INIT_LIST_HEAD(&stm_pdrv_head);
> +	mutex_init(&stm_pdrv_mutex);
> +
> +	/*
> +	 * So as to not confuse existing users with a requirement
> +	 * to load yet another module, do it here.
> +	 */
> +	if (IS_ENABLED(CONFIG_STM_PROTO_BASIC))
> +		(void)request_module_nowait("stm_p_basic");
>  
>  	stm_core_up++;
>  
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index a505f055f464..894598cfe2b7 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -33,8 +33,18 @@ struct stp_policy_node {
>  	unsigned int		last_master;
>  	unsigned int		first_channel;
>  	unsigned int		last_channel;
> +	/* this is the one that's exposed to the attributes */
> +	unsigned char		priv[0];
>  };
>  
> +void *stp_policy_node_priv(struct stp_policy_node *pn)
> +{
> +	if (!pn)
> +		return NULL;
> +
> +	return pn->priv;
> +}
> +
>  static struct configfs_subsystem stp_policy_subsys;
>  
>  void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
> @@ -68,6 +78,14 @@ to_stp_policy_node(struct config_item *item)
>  		NULL;
>  }
>  
> +void *to_pdrv_policy_node(struct config_item *item)
> +{
> +	struct stp_policy_node *node = to_stp_policy_node(item);
> +
> +	return stp_policy_node_priv(node);
> +}
> +EXPORT_SYMBOL_GPL(to_pdrv_policy_node);
> +
>  static ssize_t
>  stp_policy_node_masters_show(struct config_item *item, char *page)
>  {
> @@ -163,7 +181,9 @@ stp_policy_node_channels_store(struct config_item *item, const char *page,
>  
>  static void stp_policy_node_release(struct config_item *item)
>  {
> -	kfree(to_stp_policy_node(item));
> +	struct stp_policy_node *node = to_stp_policy_node(item);
> +
> +	kfree(node);
>  }
>  
>  static struct configfs_item_operations stp_policy_node_item_ops = {
> @@ -182,10 +202,61 @@ static struct configfs_attribute *stp_policy_node_attrs[] = {
>  static const struct config_item_type stp_policy_type;
>  static const struct config_item_type stp_policy_node_type;
>  
> +/* lifted from arch/x86/events/core.c */
> +static struct configfs_attribute **merge_attr(struct configfs_attribute **a, struct configfs_attribute **b)
> +{
> +	struct configfs_attribute **new;
> +	int j, i;
> +
> +	for (j = 0; a[j]; j++)
> +		;
> +	for (i = 0; b[i]; i++)
> +		j++;
> +	j++;
> +
> +	new = kmalloc_array(j, sizeof(struct configfs_attribute *),
> +			    GFP_KERNEL);
> +	if (!new)
> +		return NULL;
> +
> +	j = 0;
> +	for (i = 0; a[i]; i++)
> +		new[j++] = a[i];
> +	for (i = 0; b[i]; i++)
> +		new[j++] = b[i];
> +	new[j] = NULL;
> +
> +	return new;
> +}
> +
> +const struct config_item_type *
> +get_policy_node_type(struct configfs_attribute **attrs)
> +{
> +	struct config_item_type *type;
> +	struct configfs_attribute **merged;
> +
> +	type = kmemdup(&stp_policy_node_type, sizeof(stp_policy_node_type),
> +		       GFP_KERNEL);
> +	if (!type)
> +		return NULL;
> +
> +	merged = merge_attr(stp_policy_node_attrs, attrs);
> +	if (!merged) {
> +		kfree(type);
> +		return NULL;
> +	}
> +
> +	type->ct_attrs = merged;
> +
> +	return type;
> +}
> +
>  static struct config_group *
>  stp_policy_node_make(struct config_group *group, const char *name)
>  {
> +	const struct config_item_type *type = &stp_policy_node_type;
>  	struct stp_policy_node *policy_node, *parent_node;
> +	const struct stm_protocol_driver *pdrv;
>  	struct stp_policy *policy;
>  
>  	if (group->cg_item.ci_type == &stp_policy_type) {
> @@ -199,12 +270,20 @@ stp_policy_node_make(struct config_group *group, const char *name)
>  	if (!policy->stm)
>  		return ERR_PTR(-ENODEV);
>  
> -	policy_node = kzalloc(sizeof(struct stp_policy_node), GFP_KERNEL);
> +	pdrv = policy->stm->pdrv;
> +	policy_node =
> +		kzalloc(offsetof(struct stp_policy_node, priv[pdrv->priv_sz]),
> +			GFP_KERNEL);
>  	if (!policy_node)
>  		return ERR_PTR(-ENOMEM);
>  
> -	config_group_init_type_name(&policy_node->group, name,
> -				    &stp_policy_node_type);
> +	if (pdrv->policy_node_init)
> +		pdrv->policy_node_init((void *)policy_node->priv);
> +
> +	if (policy->stm->pdrv_node_type)
> +		type = policy->stm->pdrv_node_type;
> +
> +	config_group_init_type_name(&policy_node->group, name, type);
>  
>  	policy_node->policy = policy;
>  
> @@ -254,8 +333,26 @@ static ssize_t stp_policy_device_show(struct config_item *item,
>  
>  CONFIGFS_ATTR_RO(stp_policy_, device);
>  
> +static ssize_t stp_policy_protocol_show(struct config_item *item,
> +					char *page)
> +{
> +	struct stp_policy *policy = to_stp_policy(item);
> +	ssize_t count;
> +
> +	/* XXX: there shouldn't be 'none', ever */
> +	count = sprintf(page, "%s\n",
> +			(policy && policy->stm) ?
> +			policy->stm->pdrv->name :
> +			"<none>");
> +
> +	return count;
> +}
> +
> +CONFIGFS_ATTR_RO(stp_policy_, protocol);
> +
>  static struct configfs_attribute *stp_policy_attrs[] = {
>  	&stp_policy_attr_device,
> +	&stp_policy_attr_protocol,
>  	NULL,
>  };
>  
> @@ -276,6 +373,7 @@ void stp_policy_unbind(struct stp_policy *policy)
>  	stm->policy = NULL;
>  	policy->stm = NULL;
>  
> +	stm_put_protocol(stm->pdrv);
>  	stm_put_device(stm);
>  }
>  
> @@ -313,9 +411,12 @@ static const struct config_item_type stp_policy_type = {
>  static struct config_group *
>  stp_policy_make(struct config_group *group, const char *name)
>  {
> +	const struct config_item_type *pdrv_node_type;
> +	const struct stm_protocol_driver *pdrv;
> +	char *devname, *proto, *p;
>  	struct config_group *ret;
>  	struct stm_device *stm;
> -	char *devname, *p;
> +	int err;
>  
>  	devname = kasprintf(GFP_KERNEL, "%s", name);
>  	if (!devname)
> @@ -326,6 +427,7 @@ stp_policy_make(struct config_group *group, const char *name)
>  	 * <device_name> is the name of an existing stm device; may
>  	 *               contain dots;
>  	 * <policy_name> is an arbitrary string; may not contain dots
> +	 * <device_name>:<protocol_name>.<policy_name>
>  	 */
>  	p = strrchr(devname, '.');
>  	if (!p) {
> @@ -335,11 +437,28 @@ stp_policy_make(struct config_group *group, const char *name)
>  
>  	*p = '\0';
>  
> +	/*
> +	 * look for ":<protocol_name>":
> +	 *  + no protocol suffix: fall back to whatever is available;
> +	 *  + unknown protocol: fail the whole thing
> +	 */
> +	proto = strrchr(devname, ':');
> +	if (proto)
> +		*proto++ = '\0';
> +
>  	stm = stm_find_device(devname);
> +	if (!stm) {
> +		kfree(devname);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	err = stm_lookup_protocol(proto, &pdrv, &pdrv_node_type);
>  	kfree(devname);
>  
> -	if (!stm)
> +	if (err) {
> +		stm_put_device(stm);
>  		return ERR_PTR(-ENODEV);
> +	}

This condition prevent the subsystem from being used until patch 06/16 is added.
I would also suggest automatically compiling in the functionality provided by
p_basic if p_sys-t is not selected.  That way we preserve the original
behaviour.  I would also use p_basic if no protocol driver is selected rather
than leaving it to the insertion order to make things more deterministic.  

>  
>  	mutex_lock(&stm->policy_mutex);
>  	if (stm->policy) {
> @@ -349,21 +468,27 @@ stp_policy_make(struct config_group *group, const char *name)
>  
>  	stm->policy = kzalloc(sizeof(*stm->policy), GFP_KERNEL);
>  	if (!stm->policy) {
> -		ret = ERR_PTR(-ENOMEM);
> -		goto unlock_policy;
> +		mutex_unlock(&stm->policy_mutex);
> +		stm_put_protocol(pdrv);
> +		stm_put_device(stm);
> +		return ERR_PTR(-ENOMEM);
>  	}
>  
>  	config_group_init_type_name(&stm->policy->group, name,
>  				    &stp_policy_type);
> -	stm->policy->stm = stm;
>  
> +	stm->pdrv = pdrv;
> +	stm->pdrv_node_type = pdrv_node_type;
> +	stm->policy->stm = stm;
>  	ret = &stm->policy->group;
>  
>  unlock_policy:
>  	mutex_unlock(&stm->policy_mutex);
>  
> -	if (IS_ERR(ret))
> +	if (IS_ERR(ret)) {
> +		stm_put_protocol(stm->pdrv);
>  		stm_put_device(stm);
> +	}
>  
>  	return ret;
>  }
> diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
> index e5df08ae59cf..921ebd9fd3bd 100644
> --- a/drivers/hwtracing/stm/stm.h
> +++ b/drivers/hwtracing/stm/stm.h
> @@ -10,20 +10,17 @@
>  #ifndef _STM_STM_H_
>  #define _STM_STM_H_
>  
> +#include <linux/configfs.h>
> +
>  struct stp_policy;
>  struct stp_policy_node;
> +struct stm_protocol_driver;
>  
> -struct stp_policy_node *
> -stp_policy_node_lookup(struct stm_device *stm, char *s);
> -void stp_policy_node_put(struct stp_policy_node *policy_node);
> -void stp_policy_unbind(struct stp_policy *policy);
> -
> -void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
> -				unsigned int *mstart, unsigned int *mend,
> -				unsigned int *cstart, unsigned int *cend);
>  int stp_configfs_init(void);
>  void stp_configfs_exit(void);
>  
> +void *stp_policy_node_priv(struct stp_policy_node *pn);
> +
>  struct stp_master {
>  	unsigned int	nr_free;
>  	unsigned long	chan_map[0];
> @@ -40,6 +37,9 @@ struct stm_device {
>  	struct mutex		link_mutex;
>  	spinlock_t		link_lock;
>  	struct list_head	link_list;
> +	/* framing protocol in use */
> +	const struct stm_protocol_driver	*pdrv;
> +	const struct config_item_type		*pdrv_node_type;
>  	/* master allocation */
>  	spinlock_t		mc_lock;
>  	struct stp_master	*masters[0];
> @@ -48,11 +48,25 @@ struct stm_device {
>  #define to_stm_device(_d)				\
>  	container_of((_d), struct stm_device, dev)
>  
> +struct stp_policy_node *
> +stp_policy_node_lookup(struct stm_device *stm, char *s);
> +void stp_policy_node_put(struct stp_policy_node *policy_node);
> +void stp_policy_unbind(struct stp_policy *policy);
> +
> +void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
> +				unsigned int *mstart, unsigned int *mend,
> +				unsigned int *cstart, unsigned int *cend);
> +
> +/* XXX: unXXX this! */
> +const struct config_item_type *
> +get_policy_node_type(struct configfs_attribute **attrs);
> +
>  struct stm_output {
>  	spinlock_t		lock;
>  	unsigned int		master;
>  	unsigned int		channel;
>  	unsigned int		nr_chans;
> +	void			*pdrv_private;
>  };
>  
>  struct stm_file {
> @@ -76,4 +90,26 @@ struct stm_source_device {
>  #define to_stm_source_device(_d)				\
>  	container_of((_d), struct stm_source_device, dev)
>  
> +void *to_pdrv_policy_node(struct config_item *item);
> +
> +struct stm_protocol_driver {
> +	struct module	*owner;
> +	const char	*name;
> +	ssize_t		(*write)(struct stm_data *data,
> +				 struct stm_output *output, unsigned int chan,
> +				 const char *buf, size_t count);
> +	void		(*policy_node_init)(void *arg);
> +	int		(*output_open)(void *priv, struct stm_output *output);
> +	void		(*output_close)(struct stm_output *output);
> +	ssize_t		priv_sz;
> +	struct configfs_attribute	**policy_attr;
> +};
> +
> +int stm_register_protocol(const struct stm_protocol_driver *pdrv);
> +void stm_unregister_protocol(const struct stm_protocol_driver *pdrv);
> +int stm_lookup_protocol(const char *name,
> +			const struct stm_protocol_driver **pdrv,
> +			const struct config_item_type **type);
> +void stm_put_protocol(const struct stm_protocol_driver *pdrv);
> +
>  #endif /* _STM_STM_H_ */
> -- 
> 2.18.0
> 

Re: [QUEUED v20180920 05/16] stm class: Add a helper for writing data packets

[Mathieu Poirier]

On Thu, Sep 20, 2018 at 03:45:42PM +0300, Alexander Shishkin wrote:
> Add a helper to write a sequence of bytes as STP data packets. This
> is used by protocol drivers to output their metadata, as well as the
> actual data payload.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/stm/core.c | 50 ++++++++++++++++++++++++++----------
>  drivers/hwtracing/stm/stm.h  |  3 +++
>  2 files changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index c869a30661ac..9ed2f06deb47 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -564,27 +564,51 @@ stm_assign_first_policy(struct stm_device *stm, struct stm_output *output,
>  	return err;
>  }
>  
> -static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
> -			  unsigned int channel, const char *buf, size_t count)
> +/**
> + * stm_data_write() - send the given payload as data packets
> + * @data:	stm driver's data
> + * @m:		STP master
> + * @c:		STP channel
> + * @ts_first:	timestamp the first packet
> + * @buf:	data payload buffer
> + * @count:	data payload size
> + */
> +ssize_t notrace stm_data_write(struct stm_data *data, unsigned int m,
> +			       unsigned int c, bool ts_first, const void *buf,
> +			       size_t count)
>  {
> -	unsigned int flags = STP_PACKET_TIMESTAMPED;
> -	const unsigned char *p = buf, nil = 0;
> -	size_t pos;
> +	unsigned int flags = ts_first ? STP_PACKET_TIMESTAMPED : 0;
>  	ssize_t sz;
> +	size_t pos;
>  
> -	for (pos = 0, p = buf; count > pos; pos += sz, p += sz) {
> +	for (pos = 0, sz = 0; pos < count; pos += sz) {
>  		sz = min_t(unsigned int, count - pos, 8);
> -		sz = data->packet(data, master, channel, STP_PACKET_DATA, flags,
> -				  sz, p);
> -		flags = 0;
> -
> -		if (sz < 0)
> +		sz = data->packet(data, m, c, STP_PACKET_DATA, flags, sz,
> +				  &((u8 *)buf)[pos]);
> +		if (sz <= 0)
>  			break;
> +
> +		if (ts_first) {
> +			flags = 0;
> +			ts_first = false;
> +		}
>  	}
>  
> -	data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0, &nil);
> +	return sz < 0 ? sz : pos;
> +}
> +EXPORT_SYMBOL_GPL(stm_data_write);
> +
> +static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
> +			  unsigned int channel, const char *buf, size_t count)
> +{
> +	ssize_t sz;
> +
> +	sz = stm_data_write(data, master, channel, true, buf, count);
> +	if (sz > 0)
> +		data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
> +			     buf);

The original code the payload of a flag packet was '0' while in this patch
changes it to be anything.  Some external tooling could be very confused.

>  
> -	return pos;
> +	return sz;
>  }
>  
>  static ssize_t stm_char_write(struct file *file, const char __user *buf,
> diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
> index 921ebd9fd3bd..753d83acb7ae 100644
> --- a/drivers/hwtracing/stm/stm.h
> +++ b/drivers/hwtracing/stm/stm.h
> @@ -111,5 +111,8 @@ int stm_lookup_protocol(const char *name,
>  			const struct stm_protocol_driver **pdrv,
>  			const struct config_item_type **type);
>  void stm_put_protocol(const struct stm_protocol_driver *pdrv);
> +ssize_t stm_data_write(struct stm_data *data, unsigned int m,
> +		       unsigned int c, bool ts_first, const void *buf,
> +		       size_t count);
>  
>  #endif /* _STM_STM_H_ */
> -- 
> 2.18.0
> 

Re: [QUEUED v20180920 08/16] stm class: Add MIPI SyS-T protocol support

[Mathieu Poirier]

On Thu, Sep 20, 2018 at 03:45:45PM +0300, Alexander Shishkin wrote:
> This adds support for MIPI SyS-T protocol as specified in an open
> standard [1]. In additional to marking message boundaries, it also

s/additional/addition

> supports tagging messages with the source UUID, to provide better
> distinction between trace sources, including payload length and
> timestamp in the message's metadata.
> 
> This driver adds attributes to STP policy nodes to control/configure
> these metadata features.
> 
> [1] https://www.mipi.org/specifications/sys-t
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/stm/Kconfig   |  14 ++
>  drivers/hwtracing/stm/Makefile  |   2 +
>  drivers/hwtracing/stm/p_sys-t.c | 301 ++++++++++++++++++++++++++++++++
>  3 files changed, 317 insertions(+)
>  create mode 100644 drivers/hwtracing/stm/p_sys-t.c
> 
> diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
> index 262e7891fb97..752dd66742bf 100644
> --- a/drivers/hwtracing/stm/Kconfig
> +++ b/drivers/hwtracing/stm/Kconfig
> @@ -26,6 +26,20 @@ config STM_PROTO_BASIC
>  	  If you want to be able to use the basic protocol or want the
>  	  backwards compatibility for your existing setup, say Y.
>  
> +config STM_PROTO_SYS_T
> +	tristate "MIPI SyS-T STM framing protocol driver"
> +	default CONFIG_STM
> +	help
> +	  This is an implementation of MIPI SyS-T protocol to be used
> +	  over the STP transport. In addition to the data payload, it
> +	  also carries additional metadata for time correlation, better
> +	  means of trace source identification, etc.
> +
> +	  The receiving side must be able to decode this protocol in
> +	  addition to the MIPI STP, in order to extract the data.
> +
> +	  If you don't know what this is, say N.
> +
>  config STM_DUMMY
>  	tristate "Dummy STM driver"
>  	help
> diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile
> index 1571de66eca4..1692fcd29277 100644
> --- a/drivers/hwtracing/stm/Makefile
> +++ b/drivers/hwtracing/stm/Makefile
> @@ -4,8 +4,10 @@ obj-$(CONFIG_STM)	+= stm_core.o
>  stm_core-y		:= core.o policy.o
>  
>  obj-$(CONFIG_STM_PROTO_BASIC) += stm_p_basic.o
> +obj-$(CONFIG_STM_PROTO_SYS_T) += stm_p_sys-t.o
>  
>  stm_p_basic-y		:= p_basic.o
> +stm_p_sys-t-y		:= p_sys-t.o
>  
>  obj-$(CONFIG_STM_DUMMY)	+= dummy_stm.o
>  
> diff --git a/drivers/hwtracing/stm/p_sys-t.c b/drivers/hwtracing/stm/p_sys-t.c
> new file mode 100644
> index 000000000000..8f3066ff1b7b
> --- /dev/null
> +++ b/drivers/hwtracing/stm/p_sys-t.c
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MIPI SyS-T framing protocol for STM devices.
> + * Copyright (c) 2018, Intel Corporation.
> + */
> +
> +#include <linux/configfs.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/uuid.h>
> +#include <linux/stm.h>
> +#include "stm.h"
> +
> +enum sys_t_message_type {
> +	MIPI_SYST_TYPE_BUILD	= 0,
> +	MIPI_SYST_TYPE_SHORT32,
> +	MIPI_SYST_TYPE_STRING,
> +	MIPI_SYST_TYPE_CATALOG,
> +	MIPI_SYST_TYPE_RAW	= 6,
> +	MIPI_SYST_TYPE_SHORT64,
> +	MIPI_SYST_TYPE_CLOCK,
> +};
> +
> +enum sys_t_message_severity {
> +	MIPI_SYST_SEVERITY_MAX	= 0,
> +	MIPI_SYST_SEVERITY_FATAL,
> +	MIPI_SYST_SEVERITY_ERROR,
> +	MIPI_SYST_SEVERITY_WARNING,
> +	MIPI_SYST_SEVERITY_INFO,
> +	MIPI_SYST_SEVERITY_USER1,
> +	MIPI_SYST_SEVERITY_USER2,
> +	MIPI_SYST_SEVERITY_DEBUG,
> +};
> +
> +enum sys_t_message_build_subtype {
> +	MIPI_SYST_BUILD_ID_COMPACT32 = 0,
> +	MIPI_SYST_BUILD_ID_COMPACT64,
> +	MIPI_SYST_BUILD_ID_LONG,
> +};
> +
> +enum sys_t_message_clock_subtype {
> +	MIPI_SYST_CLOCK_TRANSPORT_SYNC = 1,
> +};
> +
> +enum sys_t_message_string_subtype {
> +	MIPI_SYST_STRING_GENERIC	= 1,
> +	MIPI_SYST_STRING_FUNCTIONENTER,
> +	MIPI_SYST_STRING_FUNCTIONEXIT,
> +	MIPI_SYST_STRING_INVALIDPARAM	= 5,
> +	MIPI_SYST_STRING_ASSERT		= 7,
> +	MIPI_SYST_STRING_PRINTF_32	= 11,
> +	MIPI_SYST_STRING_PRINTF_64	= 12,
> +};
> +
> +#define MIPI_SYST_TYPE(t)		((u32)(MIPI_SYST_TYPE_ ## t))
> +#define MIPI_SYST_SEVERITY(s)		((u32)(MIPI_SYST_SEVERITY_ ## s) << 4)
> +#define MIPI_SYST_OPT_LOC		BIT(8)
> +#define MIPI_SYST_OPT_LEN		BIT(9)
> +#define MIPI_SYST_OPT_CHK		BIT(10)
> +#define MIPI_SYST_OPT_TS		BIT(11)
> +#define MIPI_SYST_UNIT(u)		((u32)(u) << 12)
> +#define MIPI_SYST_ORIGIN(o)		((u32)(o) << 16)
> +#define MIPI_SYST_OPT_GUID		BIT(23)
> +#define MIPI_SYST_SUBTYPE(s)		((u32)(MIPI_SYST_ ## s) << 24)
> +#define MIPI_SYST_UNITLARGE(u)		(MIPI_SYST_UNIT(u & 0xf) | \
> +					 MIPI_SYST_ORIGIN(u >> 4))
> +#define MIPI_SYST_TYPES(t, s)		(MIPI_SYST_TYPE(t) | \
> +					 MIPI_SYST_SUBTYPE(t ## _ ## s))
> +
> +#define DATA_HEADER	(MIPI_SYST_TYPES(STRING, GENERIC)	| \
> +			 MIPI_SYST_SEVERITY(INFO)		| \
> +			 MIPI_SYST_OPT_GUID)
> +
> +struct sys_t_policy_node {
> +	uuid_t		uuid;
> +	bool		do_len;
> +	unsigned long	ts_interval;
> +};
> +
> +struct sys_t_output {
> +	struct sys_t_policy_node	node;
> +	unsigned long	ts_jiffies;
> +};
> +
> +static void sys_t_policy_node_init(void *priv)
> +{
> +	struct sys_t_policy_node *pn = priv;
> +
> +	generate_random_uuid(pn->uuid.b);
> +}
> +
> +static int sys_t_output_open(void *priv, struct stm_output *output)
> +{
> +	struct sys_t_policy_node *pn = priv;
> +	struct sys_t_output *opriv;
> +
> +	opriv = kzalloc(sizeof(*opriv), GFP_ATOMIC);
> +	if (!opriv)
> +		return -ENOMEM;
> +
> +	memcpy(&opriv->node, pn, sizeof(opriv->node));
> +	output->pdrv_private = opriv;
> +
> +	return 0;
> +}
> +
> +static void sys_t_output_close(struct stm_output *output)
> +{
> +	kfree(output->pdrv_private);
> +}
> +
> +static ssize_t sys_t_policy_uuid_show(struct config_item *item,
> +				      char *page)
> +{
> +	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
> +
> +	return sprintf(page, "%pU\n", &pn->uuid);
> +}
> +
> +static ssize_t
> +sys_t_policy_uuid_store(struct config_item *item, const char *page,
> +			size_t count)
> +{
> +	struct mutex *mutexp = &item->ci_group->cg_subsys->su_mutex;
> +	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
> +	int ret;
> +
> +	mutex_lock(mutexp);
> +	ret = uuid_parse(page, &pn->uuid);
> +	mutex_unlock(mutexp);
> +
> +	return ret < 0 ? ret : count;
> +}
> +
> +CONFIGFS_ATTR(sys_t_policy_, uuid);
> +
> +static ssize_t sys_t_policy_do_len_show(struct config_item *item,
> +				      char *page)
> +{
> +	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
> +
> +	return sprintf(page, "%d\n", pn->do_len);
> +}
> +
> +static ssize_t
> +sys_t_policy_do_len_store(struct config_item *item, const char *page,
> +			size_t count)
> +{
> +	struct mutex *mutexp = &item->ci_group->cg_subsys->su_mutex;
> +	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
> +	int ret;
> +
> +	mutex_lock(mutexp);
> +	ret = kstrtobool(page, &pn->do_len);
> +	mutex_unlock(mutexp);
> +
> +	return ret ? ret : count;
> +}
> +
> +CONFIGFS_ATTR(sys_t_policy_, do_len);
> +
> +static ssize_t sys_t_policy_ts_interval_show(struct config_item *item,
> +					     char *page)
> +{
> +	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
> +
> +	return sprintf(page, "%u\n", jiffies_to_msecs(pn->ts_interval));
> +}
> +
> +static ssize_t
> +sys_t_policy_ts_interval_store(struct config_item *item, const char *page,
> +			       size_t count)
> +{
> +	struct mutex *mutexp = &item->ci_group->cg_subsys->su_mutex;
> +	struct sys_t_policy_node *pn = to_pdrv_policy_node(item);
> +	unsigned int ms;
> +	int ret;
> +
> +	mutex_lock(mutexp);
> +	ret = kstrtouint(page, 10, &ms);
> +	mutex_unlock(mutexp);
> +
> +	if (!ret) {
> +		pn->ts_interval = msecs_to_jiffies(ms);
> +		return count;
> +	}
> +
> +	return ret;
> +}
> +
> +CONFIGFS_ATTR(sys_t_policy_, ts_interval);
> +
> +static struct configfs_attribute *sys_t_policy_attrs[] = {
> +	&sys_t_policy_attr_uuid,
> +	&sys_t_policy_attr_do_len,
> +	&sys_t_policy_attr_ts_interval,
> +	NULL,
> +};
> +
> +static inline bool sys_t_need_ts(struct sys_t_output *op)
> +{
> +	if (op->node.ts_interval &&
> +	    time_after(op->ts_jiffies + op->node.ts_interval, jiffies)) {
> +		op->ts_jiffies = jiffies;
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static ssize_t sys_t_write(struct stm_data *data, struct stm_output *output,
> +			   unsigned int chan, const char *buf, size_t count)
> +{
> +	struct sys_t_output *op = output->pdrv_private;
> +	unsigned int c = output->channel + chan;
> +	unsigned int m = output->master;
> +	u32 header = DATA_HEADER;
> +	ssize_t sz;
> +
> +	/* We require an existing policy node to proceed */
> +	if (!op)
> +		return -EINVAL;
> +
> +	if (op->node.do_len)
> +		header |= MIPI_SYST_OPT_LEN;
> +	if (sys_t_need_ts(op))
> +		header |= MIPI_SYST_OPT_TS;
> +
> +	/*
> +	 * STP framing rules for SyS-T frames:
> +	 *   * the first packet of the SyS-T frame is timestamped;
> +	 *   * the last packet is a FLAG.
> +	 */
> +	/* Message layout: HEADER / GUID / [LENGTH /][TIMESTAMP /] DATA */
> +	/* HEADER */
> +	sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_TIMESTAMPED,
> +			  4, (u8 *)&header);
> +	if (sz <= 0)
> +		return sz;
> +
> +	/* GUID */
> +	sz = stm_data_write(data, m, c, false, op->node.uuid.b, UUID_SIZE);
> +	if (sz <= 0)
> +		return sz;
> +
> +	/* [LENGTH] */
> +	if (op->node.do_len) {
> +		u16 length = count;
> +
> +		sz = data->packet(data, m, c, STP_PACKET_DATA, 0, 2,
> +				  (u8 *)&length);
> +		if (sz <= 0)
> +			return sz;
> +	}
> +
> +	/* [TIMESTAMP] */
> +	if (header & MIPI_SYST_OPT_TS) {
> +		u64 ts = ktime_get_real_ns();
> +
> +		sz = stm_data_write(data, m, c, false, &ts, sizeof(ts));
> +		if (sz <= 0)
> +			return sz;
> +	}
> +
> +	/* DATA */
> +	sz = stm_data_write(data, m, c, false, buf, count);
> +	if (sz > 0)
> +		data->packet(data, m, c, STP_PACKET_FLAG, 0, 0, buf);
> +
> +	return sz;
> +}
> +
> +static const struct stm_protocol_driver sys_t_pdrv = {
> +	.owner			= THIS_MODULE,
> +	.name			= "p_sys-t",
> +	.priv_sz		= sizeof(struct sys_t_policy_node),
> +	.write			= sys_t_write,
> +	.policy_attr		= sys_t_policy_attrs,
> +	.policy_node_init	= sys_t_policy_node_init,
> +	.output_open		= sys_t_output_open,
> +	.output_close		= sys_t_output_close,
> +};
> +
> +static int sys_t_stm_init(void)
> +{
> +	return stm_register_protocol(&sys_t_pdrv);
> +}
> +
> +static void sys_t_stm_exit(void)
> +{
> +	stm_unregister_protocol(&sys_t_pdrv);
> +}
> +
> +module_init(sys_t_stm_init);
> +module_exit(sys_t_stm_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("MIPI SyS-T STM framing protocol driver");
> +MODULE_AUTHOR("Alexander Shishkin <alexander.shishkin@linux.intel.com>");
> -- 
> 2.18.0
> 

Re: [QUEUED v20180920 00/16] stm class/intel_th: Queued updates for v4.20

[Mathieu Poirier]

On Thu, Sep 20, 2018 at 03:45:37PM +0300, Alexander Shishkin wrote:
> Hi,
> 
> These are patches I have queued so far, that I'm planning to send to
> Greg for the next merge window. This is mainly support for MIPI SyS-T
> protocol and all the infrastructure changes to make it possible.
> 
> MIPI SyS-T is a public standard [0] for a software-level trace protocol
> that in this instance we use on top of STM devices (if we choose to) and
> MIPI STP protocol.
> 
> The old "protocol" is preserved for compatibility.

I have tested this set on my side and things work as advertised.  Since I do not
have a MIPI SyS-T decoder I can't validate the output, but the mechanic behind
the functionality is simple and trivial to use.  I think you have succeeded in
introducing something that could have otherwise been messy. 

Tested-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> 
> [0] http://www.mipi.org/specifications/sys-t
> 
> Alexander Shishkin (16):
>   stm class: Rework policy node fallback
>   stm class: Clarify configfs root type/operations names
>   stm class: Clean up stp_configfs_init
>   stm class: Introduce framing protocol drivers
>   stm class: Add a helper for writing data packets
>   stm class: Factor out default framing protocol
>   stm class: Switch over to the protocol driver
>   stm class: Add MIPI SyS-T protocol support
>   stm class: p_sys-t: Add support for CLOCKSYNC packets
>   stm class: p_sys-t: Document the configfs interface
>   stm class: Document the MIPI SyS-T protocol usage
>   intel_th: SPDX-ify the documentation
>   stm class: SPDX-ify the documentation
>   stm class: heartbeat: Fix whitespace
>   lib: Add memcat_p(): paste 2 pointer arrays together
>   stm class: Use memcat_p()
> 
>  .../ABI/testing/configfs-stp-policy-p_sys-t   |  41 ++
>  Documentation/trace/intel_th.rst              |   2 +
>  Documentation/trace/stm.rst                   |   2 +
>  Documentation/trace/sys-t.rst                 |  62 +++
>  drivers/hwtracing/stm/Kconfig                 |  29 ++
>  drivers/hwtracing/stm/Makefile                |   6 +
>  drivers/hwtracing/stm/core.c                  | 292 +++++++++++---
>  drivers/hwtracing/stm/heartbeat.c             |   2 +-
>  drivers/hwtracing/stm/p_basic.c               |  47 +++
>  drivers/hwtracing/stm/p_sys-t.c               | 381 ++++++++++++++++++
>  drivers/hwtracing/stm/policy.c                | 148 +++++--
>  drivers/hwtracing/stm/stm.h                   |  57 ++-
>  include/linux/string.h                        |   7 +
>  lib/Kconfig.debug                             |   8 +
>  lib/Makefile                                  |   1 +
>  lib/string.c                                  |  31 ++
>  lib/test_memcat_p.c                           | 115 ++++++
>  17 files changed, 1138 insertions(+), 93 deletions(-)
>  create mode 100644 Documentation/ABI/testing/configfs-stp-policy-p_sys-t
>  create mode 100644 Documentation/trace/sys-t.rst
>  create mode 100644 drivers/hwtracing/stm/p_basic.c
>  create mode 100644 drivers/hwtracing/stm/p_sys-t.c
>  create mode 100644 lib/test_memcat_p.c
> 
> -- 
> 2.18.0
> 

Re: [QUEUED v20180920 00/16] stm class/intel_th: Queued updates for v4.20

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On Thu, Sep 20, 2018 at 03:45:37PM +0300, Alexander Shishkin wrote:
>> Hi,
>> 
>> These are patches I have queued so far, that I'm planning to send to
>> Greg for the next merge window. This is mainly support for MIPI SyS-T
>> protocol and all the infrastructure changes to make it possible.
>> 
>> MIPI SyS-T is a public standard [0] for a software-level trace protocol
>> that in this instance we use on top of STM devices (if we choose to) and
>> MIPI STP protocol.
>> 
>> The old "protocol" is preserved for compatibility.
>
> I have tested this set on my side and things work as advertised.  Since I do not
> have a MIPI SyS-T decoder I can't validate the output, but the mechanic behind
> the functionality is simple and trivial to use.  I think you have succeeded in
> introducing something that could have otherwise been messy. 
>
> Tested-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Thanks a bunch! An update is coming up.

Cheers,
--
Alex

Re: [QUEUED v20180920 05/16] stm class: Add a helper for writing data packets

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

>> +static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
>> +			  unsigned int channel, const char *buf, size_t count)
>> +{
>> +	ssize_t sz;
>> +
>> +	sz = stm_data_write(data, master, channel, true, buf, count);
>> +	if (sz > 0)
>> +		data->packet(data, master, channel, STP_PACKET_FLAG, 0, 0,
>> +			     buf);
>
> The original code the payload of a flag packet was '0' while in this patch
> changes it to be anything.  Some external tooling could be very confused.

I think the original intention was so: the 'size' field of ->packet()
refers to how many bytes from the given 'payload' the callback should
use. IOW, with size 0, buf may point to anything (still a valid pointer
though). But that should have been documented better from the beginning,
so you're completely right. I'll make a note to myself to go over the
API bits and sort out stuff like that.

Thanks,
--
Alex

Re: [QUEUED v20180920 04/16] stm class: Introduce framing protocol drivers

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

>> +	err = stm_lookup_protocol(proto, &pdrv, &pdrv_node_type);
>>  	kfree(devname);
>>  
>> -	if (!stm)
>> +	if (err) {
>> +		stm_put_device(stm);
>>  		return ERR_PTR(-ENODEV);
>> +	}
>
> This condition prevent the subsystem from being used until patch 06/16 is added.

Good catch, I completely missed that.

> I would also suggest automatically compiling in the functionality provided by
> p_basic if p_sys-t is not selected.

That's kind of the intention; in Kconfig the PROTO_BASIC should default
to the same thing as CONFIG_STM, so it should be present unless the user
specifically interfered and disabled it.

> That way we preserve the original
> behaviour.  I would also use p_basic if no protocol driver is selected rather
> than leaving it to the insertion order to make things more
> deterministic.

Agreed, more deterministic is good.

Thanks,
--
Alex