lkml.org 
[lkml]   [2015]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH kernel v10 23/34] powerpc/iommu/powernv: Release replaced TCE
On Thu, 14 May 2015 13:53:57 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 05/14/2015 01:00 AM, Thomas Huth wrote:
> > On Tue, 12 May 2015 01:39:12 +1000
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
...
> >> -/*
> >> - * 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.

> >> @@ -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


\
 
 \ /
  Last update: 2015-05-15 10:21    [W:0.073 / U:8.536 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site