On Thu, 14 May 2015 13:53:57 +1000
Alexey Kardashevskiy [off-list ref] wrote:

On 05/14/2015 01:00 AM, Thomas Huth wrote:

quoted

On Tue, 12 May 2015 01:39:12 +1000
Alexey Kardashevskiy [off-list ref] wrote:

...

quoted

quoted

-/*
- * hwaddr is a kernel virtual address here (0xc... bazillion),
- * tce_build converts it to a physical address.
- */
-int iommu_tce_build(struct iommu_table *tbl, unsigned long entry,
-		unsigned long hwaddr, enum dma_data_direction direction)
-{
-	int ret = -EBUSY;
-	unsigned long oldtce;
-	struct iommu_pool *pool = get_pool(tbl, entry);
-
-	spin_lock(&(pool->lock));
-
-	oldtce = tbl->it_ops->get(tbl, entry);
-	/* Add new entry if it is not busy */
-	if (!(oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)))
-		ret = tbl->it_ops->set(tbl, entry, 1, hwaddr, direction, NULL);
-
-	spin_unlock(&(pool->lock));
+	if (!ret && ((*direction == DMA_FROM_DEVICE) ||
+			(*direction == DMA_BIDIRECTIONAL)))

You could drop some of the parentheses:

	if (!ret && (*direction == DMA_FROM_DEVICE ||
			*direction == DMA_BIDIRECTIONAL))

I really (really) like braces. Is there any kernel code design rule against it?

I don't think so ... but for me it's rather the other way round: If I
see too many braces, I always wonder whether there is a reason for it in
the sense that I did not understand the statement right at the first
glance. Additionally, this is something that Pascal programmers like to
do, so IMHO this just looks ugly in C.

quoted

quoted

@@ -405,19 +410,26 @@ static long tce_iommu_ioctl(void *iommu_data,
  			return -EINVAL;

  		/* iova is checked by the IOMMU API */
-		tce = param.vaddr;
  		if (param.flags & VFIO_DMA_MAP_FLAG_READ)
-			tce |= TCE_PCI_READ;
-		if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
-			tce |= TCE_PCI_WRITE;
+			if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
+				direction = DMA_BIDIRECTIONAL;
+			else
+				direction = DMA_TO_DEVICE;
+		else
+			if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
+				direction = DMA_FROM_DEVICE;
+			else
+				return -EINVAL;

IMHO some curly braces for the outer if-statement would be really fine
here.

I believe checkpatch.pl won't like it. There is a check against single 
lines having braces after "if" statements.

If you write your code like this (I was only talking about the outer
braces!):

	if (param.flags & VFIO_DMA_MAP_FLAG_READ) {
		if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
			direction = DMA_BIDIRECTIONAL;
		else
			direction = DMA_TO_DEVICE;
	} else {
		if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
			direction = DMA_FROM_DEVICE;
		else
			return -EINVAL;
	}

... then checkpatch should not complain, as far as I know - in this
case, the braces include three lines, don't they?

 Thomas