On 27/06/14 19:04, mathieu.poirier at linaro.org wrote:

I don't understand this sentence. It makes is sound like user input is
needed somehow.

Isn't this what GENMASK() already does?

+#define BMVAL(val, lsb, msb)	((val & BM(lsb, msb)) >> lsb)
+#define BVAL(val, n)		((val & BIT(n)) >> n)

BVAL() is obfuscation and should be removed.

As an example (taken from one of the patches that consumes this macro):

+	for (i = TIMEOUT_US;
+	     BVAL(cs_readl(drvdata->base, ETB_FFCR), ETB_FFCR_BIT) != 0
+	     && i > 0; i--)
+		udelay(1);

Is not really as readable as:

+	for (i = TIMEOUT_US;
+	     cs_readl(drvdata->base, ETB_FFCR) & ETB_FFCR_BIT && i > 0;
+	     i--)
+		udelay(1);

Within the whole patchset it is only every usedIt is only ever used call
site looks more or less like this:

+#define cs_writel(addr, val, off)	__raw_writel((val), addr + off)
+#define cs_readl(addr, off)		__raw_readl(addr + off)

Out of interest, would readl/writel_relaxed() more appropriate?

Why is coresight_mutex a semaphore?

+static int coresight_find_link_inport(struct coresight_device *csdev)
+{
+	int i;
+	struct coresight_device *parent;
+	struct coresight_connection *conn;
+
+	parent = container_of(csdev->path_link.next, struct coresight_device,
+			     path_link);
+	for (i = 0; i < parent->nr_conns; i++) {
+		conn = &parent->conns[i];
+		if (conn->child_dev == csdev)
+			return conn->child_port;
+	}
+
+	pr_err("coresight: couldn't find inport, parent: %d, child: %d\n",
+	       parent->id, csdev->id);
+	return 0;
+}
+
+static int coresight_find_link_outport(struct coresight_device *csdev)
+{
+	int i;
+	struct coresight_device *child;
+	struct coresight_connection *conn;
+
+	child = container_of(csdev->path_link.prev, struct coresight_device,
+			      path_link);
+	for (i = 0; i < csdev->nr_conns; i++) {
+		conn = &csdev->conns[i];
+		if (conn->child_dev == child)
+			return conn->outport;
+	}
+
+	pr_err("coresight: couldn't find outport, parent: %d, child: %d\n",
+	       csdev->id, child->id);
+	return 0;
+}
+
+static int coresight_enable_sink(struct coresight_device *csdev)
+{
+	int ret;
+
+	if (csdev->refcnt.sink_refcnt == 0) {
+		if (csdev->ops->sink_ops->enable) {
+			ret = csdev->ops->sink_ops->enable(csdev);
+			if (ret)
+				goto err;
+			csdev->enable = true;
+		}
+	}
+	csdev->refcnt.sink_refcnt++;
+
+	return 0;
+err:
+	return ret;
+}
+
+static void coresight_disable_sink(struct coresight_device *csdev)
+{
+	if (csdev->refcnt.sink_refcnt == 1) {
+		if (csdev->ops->sink_ops->disable) {
+			csdev->ops->sink_ops->disable(csdev);
+			csdev->enable = false;
+		}
+	}
+	csdev->refcnt.sink_refcnt--;
+}
+
+static int coresight_enable_link(struct coresight_device *csdev)
+{
+	int ret;
+	int link_subtype;
+	int refport, inport, outport;
+
+	inport = coresight_find_link_inport(csdev);
+	outport = coresight_find_link_outport(csdev);
+
+	link_subtype = csdev->subtype.link_subtype;
+	if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG)
+		refport = inport;
+	else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT)
+		refport = outport;
+	else
+		refport = 0;
+
+	if (csdev->refcnt.link_refcnts[refport] == 0) {
+		if (csdev->ops->link_ops->enable) {
+			ret = csdev->ops->link_ops->enable(csdev, inport,
+							   outport);
+			if (ret)
+				goto err;
+			csdev->enable = true;
+		}
+	}
+	csdev->refcnt.link_refcnts[refport]++;
+
+	return 0;
+err:
+	return ret;
+}
+
+static void coresight_disable_link(struct coresight_device *csdev)
+{
+	int link_subtype;
+	int refport, inport, outport;
+
+	inport = coresight_find_link_inport(csdev);
+	outport = coresight_find_link_outport(csdev);
+
+	link_subtype = csdev->subtype.link_subtype;
+	if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG)
+		refport = inport;
+	else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT)
+		refport = outport;
+	else
+		refport = 0;

I already read these 7 lines once...

+
+	if (csdev->refcnt.link_refcnts[refport] == 1) {
+		if (csdev->ops->link_ops->disable) {
+			csdev->ops->link_ops->disable(csdev, inport, outport);
+			csdev->enable = false;
+		}
+	}
+	csdev->refcnt.link_refcnts[refport]--;
+}
+
+static int coresight_enable_source(struct coresight_device *csdev)
+{
+	int ret;
+
+	if (csdev->refcnt.source_refcnt == 0) {
+		if (csdev->ops->source_ops->enable) {
+			ret = csdev->ops->source_ops->enable(csdev);
+			if (ret)
+				goto err;
+			csdev->enable = true;
+		}
+	}
+	csdev->refcnt.source_refcnt++;
+
+	return 0;
+err:
+	return ret;
+}
+
+static void coresight_disable_source(struct coresight_device *csdev)
+{
+	if (csdev->refcnt.source_refcnt == 1) {
+		if (csdev->ops->source_ops->disable) {
+			csdev->ops->source_ops->disable(csdev);
+			csdev->enable = false;
+		}
+	}
+	csdev->refcnt.source_refcnt--;
+}
+
+static struct list_head *coresight_build_path(struct coresight_device *csdev,
+					      struct list_head *path)
+{
+	int i;
+	struct list_head *p;
+	struct coresight_connection *conn;
+
+	if (csdev->id == curr_sink) {
+		list_add_tail(&csdev->path_link, path);
+		return path;
+	}
+
+	for (i = 0; i < csdev->nr_conns; i++) {
+		conn = &csdev->conns[i];
+		p = coresight_build_path(conn->child_dev, path);
+		if (p) {
+			list_add_tail(&csdev->path_link, p);
+			return p;
+		}
+	}
+	return NULL;
+}
+
+static void coresight_release_path(struct list_head *path)
+{
+	struct coresight_device *cd, *temp;
+
+	list_for_each_entry_safe(cd, temp, path, path_link)
+		list_del(&cd->path_link);
+}
+
+static int coresight_enable_path(struct list_head *path, bool incl_source)
+{
+	int ret = 0;
+	struct coresight_device *cd;
+
+	list_for_each_entry(cd, path, path_link) {
+		if (cd == list_first_entry(path, struct coresight_device,
+					   path_link)) {
+			ret = coresight_enable_sink(cd);
+		} else if (list_is_last(&cd->path_link, path)) {
+			if (incl_source)
+				ret = coresight_enable_source(cd);
+		} else {
+			ret = coresight_enable_link(cd);
+		}
+		if (ret)
+			goto err;
+	}
+	return 0;
+err:
+	list_for_each_entry_continue_reverse(cd, path, path_link) {
+		if (cd == list_first_entry(path, struct coresight_device,
+					   path_link)) {
+			coresight_disable_sink(cd);
+		} else if (list_is_last(&cd->path_link, path)) {
+			if (incl_source)
+				coresight_disable_source(cd);
+		} else {
+			coresight_disable_link(cd);
+		}
+	}
+	return ret;
+}
+
+static void coresight_disable_path(struct list_head *path, bool incl_source)
+{
+	struct coresight_device *cd;
+
+	list_for_each_entry(cd, path, path_link) {
+		if (cd == list_first_entry(path, struct coresight_device,
+					   path_link)) {
+			coresight_disable_sink(cd);
+		} else if (list_is_last(&cd->path_link, path)) {
+			if (incl_source)
+				coresight_disable_source(cd);
+		} else {
+			coresight_disable_link(cd);
+		}
+	}
+}
+
+static int coresight_switch_sink(struct coresight_device *csdev)
+{
+	int ret = 0;
+	LIST_HEAD(path);
+	struct coresight_device *cd;
+
+	if (IS_ERR_OR_NULL(csdev))
+		return -EINVAL;

If we really believe the caller is likely to do something this stupid we
should probably WARN_ON() for their own good.

+
+	down(&coresight_mutex);
+	if (csdev->id == curr_sink)
+		goto out;
+
+	list_for_each_entry(cd, &coresight_devs, dev_link) {
+		if (cd->type == CORESIGHT_DEV_TYPE_SOURCE && cd->enable) {
+			coresight_build_path(cd, &path);
+			coresight_disable_path(&path, false);
+			coresight_release_path(&path);
+		}
+	}
+	curr_sink = csdev->id;
+	list_for_each_entry(cd, &coresight_devs, dev_link) {
+		if (cd->type == CORESIGHT_DEV_TYPE_SOURCE && cd->enable) {
+			coresight_build_path(cd, &path);
+			ret = coresight_enable_path(&path, false);
+			coresight_release_path(&path);
+			if (ret)
+				goto err;
+		}
+	}
+out:
+	up(&coresight_mutex);
+	return 0;
+err:
+	list_for_each_entry(cd, &coresight_devs, dev_link) {
+		if (cd->type == CORESIGHT_DEV_TYPE_SOURCE && cd->enable)
+			coresight_disable_source(cd);
+	}
+	pr_err("coresight: sink switch failed, sources disabled; try again\n");

coresight_mutex is still locked at this point (so trying again won't
help ;-).

+	return ret;
+}
+
+int coresight_enable(struct coresight_device *csdev)
+{
+	int ret = 0;
+	LIST_HEAD(path);
+
+	if (IS_ERR_OR_NULL(csdev))
+		return -EINVAL;

WARN_ON() or remove.

+
+	down(&coresight_mutex);
+	if (csdev->type != CORESIGHT_DEV_TYPE_SOURCE) {
+		ret = -EINVAL;
+		pr_err("coresight: wrong device type in %s\n", __func__);
+		goto out;
+	}
+	if (csdev->enable)
+		goto out;
+
+	coresight_build_path(csdev, &path);
+	ret = coresight_enable_path(&path, true);
+	coresight_release_path(&path);
+	if (ret)
+		pr_err("coresight: enable failed\n");
+out:
+	up(&coresight_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(coresight_enable);
+
+void coresight_disable(struct coresight_device *csdev)
+{
+	LIST_HEAD(path);
+
+	if (IS_ERR_OR_NULL(csdev))
+		return;
+
+	down(&coresight_mutex);
+	if (csdev->type != CORESIGHT_DEV_TYPE_SOURCE) {
+		pr_err("coresight: wrong device type in %s\n", __func__);
+		goto out;
+	}
+	if (!csdev->enable)
+		goto out;
+
+	coresight_build_path(csdev, &path);
+	coresight_disable_path(&path, true);
+	coresight_release_path(&path);
+out:
+	up(&coresight_mutex);
+}
+EXPORT_SYMBOL_GPL(coresight_disable);
+
+void coresight_abort(void)
+{
+	struct coresight_device *cd;
+
+	if (down_trylock(&coresight_mutex)) {
+		pr_err("coresight: abort could not be processed\n");
+		return;
+	}
+	if (curr_sink == NO_SINK)
+		goto out;
+
+	list_for_each_entry(cd, &coresight_devs, dev_link) {
+		if (cd->id == curr_sink) {
+			if (cd->enable && cd->ops->sink_ops->abort) {
+				cd->ops->sink_ops->abort(cd);
+				cd->enable = false;
+			}
+		}
+	}
+out:
+	up(&coresight_mutex);
+}
+EXPORT_SYMBOL_GPL(coresight_abort);
+
+static ssize_t debugfs_curr_sink_get(void *data, u64 *val)
+{
+	struct coresight_device *csdev = data;
+
+	*val = (csdev->id == curr_sink) ? 1 : 0;
+	return 0;
+}
+
+static ssize_t debugfs_curr_sink_set(void *data, u64 val)
+{
+	struct coresight_device *csdev = data;
+
+	if (val)
+		return coresight_switch_sink(csdev);
+	else
+		return -EINVAL;
+}
+CORESIGHT_DEBUGFS_ENTRY(debugfs_curr_sink, "curr_sink",
+			S_IRUGO | S_IWUSR, debugfs_curr_sink_get,
+			debugfs_curr_sink_set, "%llu\n");
+
+static ssize_t debugfs_enable_get(void *data, u64 *val)
+{
+	struct coresight_device *csdev = data;
+
+	*val = csdev->enable;
+	return 0;
+}
+
+static ssize_t debugfs_enable_set(void *data, u64 val)
+{
+	struct coresight_device *csdev = data;
+
+	if (val)
+		return coresight_enable(csdev);
+	else
+		coresight_disable(csdev);
+
+	return 0;
+}
+CORESIGHT_DEBUGFS_ENTRY(debugfs_enable, "enable",
+			S_IRUGO | S_IWUSR, debugfs_enable_get,
+			debugfs_enable_set, "%llu\n");
+
+
+static const struct coresight_ops_entry *coresight_grps_sink[] = {
+	&debugfs_curr_sink_entry,
+	NULL,
+};
+
+static const struct coresight_ops_entry *coresight_grps_source[] = {
+	&debugfs_enable_entry,
+	NULL,
+};
+
+struct coresight_group_entries {
+	const char *name;
+	const struct coresight_ops_entry **entries;
+};
+
+struct coresight_group_entries coresight_debugfs_entries[] = {
+	{
+		.name = "none",
+	},
+	{
+		.name = "sink",
+		.entries = coresight_grps_sink,
+	},
+	{
+		.name = "link",
+	},
+	{
+		.name = "linksink",
+	},
+	{
+		.name = "source",
+		.entries = coresight_grps_source,
+	},
+};
+
+static void coresight_device_release(struct device *dev)
+{
+	struct coresight_device *csdev = to_coresight_device(dev);
+	kfree(csdev);
+}
+
+static void coresight_fixup_orphan_conns(struct coresight_device *csdev)
+{
+	struct coresight_connection *conn, *temp;
+
+	list_for_each_entry_safe(conn, temp, &coresight_orph_conns, link) {
+		if (conn->child_id == csdev->id) {
+			conn->child_dev = csdev;
+			list_del(&conn->link);
+		}
+	}
+}
+
+static void coresight_fixup_device_conns(struct coresight_device *csdev)
+{
+	int i;
+	struct coresight_device *cd;
+	bool found;
+
+	for (i = 0; i < csdev->nr_conns; i++) {
+		found = false;
+		list_for_each_entry(cd, &coresight_devs, dev_link) {
+			if (csdev->conns[i].child_id == cd->id) {
+				csdev->conns[i].child_dev = cd;
+				found = true;
+				break;
+			}
+		}
+		if (!found)
+			list_add_tail(&csdev->conns[i].link,
+				      &coresight_orph_conns);
+	}
+}
+
+static int debugfs_coresight_init(void)
+{
+	if (!cs_debugfs_parent) {
+		cs_debugfs_parent = debugfs_create_dir("coresight", 0);
+		if (IS_ERR(cs_debugfs_parent))
+			return -1;
+	}
+
+	return 0;
+}
+
+static struct dentry *coresight_debugfs_desc_init(
+				struct coresight_device *csdev,
+				const struct coresight_ops_entry **debugfs_ops)
+{
+	int i = 0;
+	struct dentry *parent;
+	struct device *dev = &csdev->dev;
+	const struct coresight_ops_entry *ops_entry, **ops_entries;
+
+	parent = debugfs_create_dir(dev_name(dev), cs_debugfs_parent);
+	if (IS_ERR(parent))
+		return NULL;
+
+	/* device-specific ops */
+	while (debugfs_ops && debugfs_ops[i]) {
+		ops_entry = debugfs_ops[i];
+		if (!debugfs_create_file(ops_entry->name, ops_entry->mode,
+					 parent, dev_get_drvdata(dev->parent),
+					 ops_entry->ops)) {
+			debugfs_remove_recursive(parent);
+			return NULL;
+		}
+		i++;
+	}
+
+	/* group-specific ops */
+	i = 0;
+	ops_entries = coresight_debugfs_entries[csdev->type].entries;
+
+	while (ops_entries && ops_entries[i]) {
+		if (!debugfs_create_file(ops_entries[i]->name,
+					 ops_entries[i]->mode,
+					 parent, csdev, ops_entries[i]->ops)) {
+			debugfs_remove_recursive(parent);
+			return NULL;
+		}
+		i++;
+	}
+
+	return parent;
+}
+
+struct coresight_device *coresight_register(struct coresight_desc *desc)
+{
+	int i;
+	int ret;
+	int link_subtype;
+	int nr_refcnts;
+	int *refcnts = NULL;
+	struct coresight_device *csdev;
+	struct coresight_connection *conns;
+
+	if (IS_ERR_OR_NULL(desc))
+		return ERR_PTR(-EINVAL);
+
+	csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
+	if (!csdev) {
+		ret = -ENOMEM;
+		goto err_kzalloc_csdev;
+	}
+
+	csdev->id = desc->pdata->id;
+
+	if (desc->type == CORESIGHT_DEV_TYPE_LINK ||
+	    desc->type == CORESIGHT_DEV_TYPE_LINKSINK) {
+		link_subtype = desc->subtype.link_subtype;
+		if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG)
+			nr_refcnts = desc->pdata->nr_inports;
+		else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT)
+			nr_refcnts = desc->pdata->nr_outports;
+		else
+			nr_refcnts = 1;
+
+		refcnts = kzalloc(sizeof(*refcnts) * nr_refcnts, GFP_KERNEL);
+		if (!refcnts) {
+			ret = -ENOMEM;
+			goto err_kzalloc_refcnts;
+		}
+		csdev->refcnt.link_refcnts = refcnts;
+	}
+
+	csdev->nr_conns = desc->pdata->nr_outports;
+	conns = kzalloc(sizeof(*conns) * csdev->nr_conns, GFP_KERNEL);
+	if (!conns) {
+		ret = -ENOMEM;
+		goto err_kzalloc_conns;
+	}
+
+	for (i = 0; i < csdev->nr_conns; i++) {
+		conns[i].outport = desc->pdata->outports[i];
+		conns[i].child_id = desc->pdata->child_ids[i];
+		conns[i].child_port = desc->pdata->child_ports[i];
+	}
+	csdev->conns = conns;
+
+	csdev->type = desc->type;
+	csdev->subtype = desc->subtype;
+	csdev->ops = desc->ops;
+	csdev->owner = desc->owner;
+
+	csdev->dev.parent = desc->dev;
+	csdev->dev.release = coresight_device_release;
+	dev_set_name(&csdev->dev, "%s", desc->pdata->name);
+
+	down(&coresight_mutex);
+	if (desc->pdata->default_sink) {
+		if (curr_sink == NO_SINK) {
+			curr_sink = csdev->id;
+		} else {
+			ret = -EINVAL;
+			goto err_default_sink;
+		}
+	}
+
+	coresight_fixup_device_conns(csdev);
+
+	debugfs_coresight_init();

Return value ignored here.

+	csdev->de = coresight_debugfs_desc_init(csdev, desc->debugfs_ops);
+
+	coresight_fixup_orphan_conns(csdev);
+
+	list_add_tail(&csdev->dev_link, &coresight_devs);
+	up(&coresight_mutex);
+
+	return csdev;
...

   ^^^^^^                                     ^^

Not static and no return value.

+#endif
+
+#endif