Re: [PATCH V3 11/14] coresight: sink: Add TRBE driver

[...]

quoted

quoted

+	if (nr_pages < 2)
+		return NULL;
+
+	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, trbe_alloc_node(event));
+	if (IS_ERR(buf))
+		return ERR_PTR(-ENOMEM);
+
+	pglist = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
+	if (IS_ERR(pglist)) {
+		kfree(buf);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0; i < nr_pages; i++)
+		pglist[i] = virt_to_page(pages[i]);
+
+	buf->trbe_base = (unsigned long) vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
+	if (IS_ERR((void *)buf->trbe_base)) {

Why not simply make buf->trbe_base a void * instead of having to do all this

There are many arithmetic and comparison operations involving trbe_base
element. Hence it might be better to keep it as unsigned long, also to
keeps it consistent with other pointers i.e trbe_write, trbe_limit.

That is a fair point.  Please add a comment to explain your design choice and
make sure the sparse checker is happy with all of it.

Snippet from $cat drivers/hwtracing/coresight/coresight-trbe.c | grep "trbe_base"
There are just two places type casting trbe_base back to (void *).

	memset((void *)buf->trbe_base + head, ETE_IGNORE_PACKET, len);
	return buf->trbe_base + offset;
	WARN_ON(buf->trbe_write < buf->trbe_base);
	set_trbe_base_pointer(buf->trbe_base);
	buf->trbe_base = (unsigned long)vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
	if (IS_ERR((void *)buf->trbe_base)) {
		return ERR_PTR(buf->trbe_base);
	buf->trbe_limit = buf->trbe_base + nr_pages * PAGE_SIZE;
	buf->trbe_write = buf->trbe_base;
	vunmap((void *)buf->trbe_base);
	base = get_trbe_base_pointer();
	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
	if (buf->trbe_limit == buf->trbe_base) {
	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
	if (buf->trbe_limit == buf->trbe_base) {
	offset = get_trbe_limit_pointer() - get_trbe_base_pointer();
	buf->trbe_write = buf->trbe_base + PERF_IDX2OFF(handle->head, buf);
	if (buf->trbe_limit == buf->trbe_base) {
	WARN_ON(buf->trbe_base != get_trbe_base_pointer());
		if (get_trbe_write_pointer() == get_trbe_base_pointer())
  

quoted

casting?  And IS_ERR() doesn't work with vmap().

Sure, will drop IS_ERR() here.

[...]

quoted

quoted

+
+static ssize_t dbm_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct trbe_cpudata *cpudata = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", cpudata->trbe_dbm);
+}
+static DEVICE_ATTR_RO(dbm);

What does "dbm" stand for?  Looking at the documentation for TRBIDR_EL1.F, I
don't see what "dbm" relates to.

I made it up to refer TRBIDR_EL1.F as "Dirty (and Access Flag) Bit Management".
Could change it as "afdbm" to be more specific or if it is preferred.

I don't see "afdbm" being a better solution - why not simply "flag"?


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel