From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934456AbbEOIKB (ORCPT ); Fri, 15 May 2015 04:10:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55433 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933715AbbEOIJz (ORCPT ); Fri, 15 May 2015 04:09:55 -0400 Date: Fri, 15 May 2015 10:09:49 +0200 From: Thomas Huth To: Alexey Kardashevskiy Cc: linuxppc-dev@lists.ozlabs.org, Wei Yang , Gavin Shan , linux-kernel@vger.kernel.org, Alex Williamson , Paul Mackerras , David Gibson Subject: Re: [PATCH kernel v10 23/34] powerpc/iommu/powernv: Release replaced TCE Message-ID: <20150515100949.0433bfbf@thh440s> In-Reply-To: <55541C55.4060807@ozlabs.ru> References: <1431358763-24371-1-git-send-email-aik@ozlabs.ru> <1431358763-24371-24-git-send-email-aik@ozlabs.ru> <20150513170048.2c6115c6@thh440s> <55541C55.4060807@ozlabs.ru> Organization: Red Hat MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 14 May 2015 13:53:57 +1000 Alexey Kardashevskiy wrote: > On 05/14/2015 01:00 AM, Thomas Huth wrote: > > On Tue, 12 May 2015 01:39:12 +1000 > > Alexey Kardashevskiy 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id B6F741A0015 for ; Fri, 15 May 2015 18:09:57 +1000 (AEST) Date: Fri, 15 May 2015 10:09:49 +0200 From: Thomas Huth To: Alexey Kardashevskiy Subject: Re: [PATCH kernel v10 23/34] powerpc/iommu/powernv: Release replaced TCE Message-ID: <20150515100949.0433bfbf@thh440s> In-Reply-To: <55541C55.4060807@ozlabs.ru> References: <1431358763-24371-1-git-send-email-aik@ozlabs.ru> <1431358763-24371-24-git-send-email-aik@ozlabs.ru> <20150513170048.2c6115c6@thh440s> <55541C55.4060807@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Cc: Wei Yang , Gavin Shan , linux-kernel@vger.kernel.org, Alex Williamson , Paul Mackerras , linuxppc-dev@lists.ozlabs.org, David Gibson List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Thu, 14 May 2015 13:53:57 +1000 Alexey Kardashevskiy wrote: > On 05/14/2015 01:00 AM, Thomas Huth wrote: > > On Tue, 12 May 2015 01:39:12 +1000 > > Alexey Kardashevskiy 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