Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754850AbbENABy (ORCPT ); Wed, 13 May 2015 20:01:54 -0400 Received: from e23smtp02.au.ibm.com ([202.81.31.144]:49464 "EHLO e23smtp02.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754092AbbENABv (ORCPT ); Wed, 13 May 2015 20:01:51 -0400 Date: Thu, 14 May 2015 10:00:48 +1000 From: Gavin Shan To: Alexey Kardashevskiy Cc: linuxppc-dev@lists.ozlabs.org, David Gibson , Benjamin Herrenschmidt , Paul Mackerras , Alex Williamson , Gavin Shan , Wei Yang , linux-kernel@vger.kernel.org Subject: Re: [PATCH kernel v10 13/34] powerpc/powernv: Do not set "read" flag if direction==DMA_NONE Message-ID: <20150514000048.GA13216@gwshan> Reply-To: Gavin Shan References: <1431358763-24371-1-git-send-email-aik@ozlabs.ru> <1431358763-24371-14-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1431358763-24371-14-git-send-email-aik@ozlabs.ru> User-Agent: Mutt/1.5.23 (2014-03-12) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15051400-0005-0000-0000-000001C8ABCC Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4059 Lines: 111 On Tue, May 12, 2015 at 01:39:02AM +1000, Alexey Kardashevskiy wrote: >Normally a bitmap from the iommu_table is used to track what TCE entry >is in use. Since we are going to use iommu_table without its locks and >do xchg() instead, it becomes essential not to put bits which are not >implied in the direction flag as the old TCE value (more precisely - >the permission bits) will be used to decide whether to put the page or not. > >This adds iommu_direction_to_tce_perm() (its counterpart is there already) >and uses it for powernv's pnv_tce_build(). > >Signed-off-by: Alexey Kardashevskiy >Reviewed-by: David Gibson Reviewed-by: Gavin Shan >--- >Changes: >v9: >* added comment why we must put only valid permission bits >--- > arch/powerpc/include/asm/iommu.h | 1 + > arch/powerpc/kernel/iommu.c | 15 +++++++++++++++ > arch/powerpc/platforms/powernv/pci.c | 7 +------ > 3 files changed, 17 insertions(+), 6 deletions(-) > >diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h >index e94a5e3..d91bd69 100644 >--- a/arch/powerpc/include/asm/iommu.h >+++ b/arch/powerpc/include/asm/iommu.h >@@ -200,6 +200,7 @@ extern int iommu_take_ownership(struct iommu_table *tbl); > extern void iommu_release_ownership(struct iommu_table *tbl); > > extern enum dma_data_direction iommu_tce_direction(unsigned long tce); >+extern unsigned long iommu_direction_to_tce_perm(enum dma_data_direction dir); > > #endif /* __KERNEL__ */ > #endif /* _ASM_IOMMU_H */ >diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c >index 8673c94..31319f8 100644 >--- a/arch/powerpc/kernel/iommu.c >+++ b/arch/powerpc/kernel/iommu.c >@@ -863,6 +863,21 @@ void iommu_free_coherent(struct iommu_table *tbl, size_t size, > } > } > >+unsigned long iommu_direction_to_tce_perm(enum dma_data_direction dir) >+{ >+ switch (dir) { >+ case DMA_BIDIRECTIONAL: >+ return TCE_PCI_READ | TCE_PCI_WRITE; >+ case DMA_FROM_DEVICE: >+ return TCE_PCI_WRITE; >+ case DMA_TO_DEVICE: >+ return TCE_PCI_READ; >+ default: >+ return 0; It might be nice to have a WARN_ON() or log for the default case. If the TCE entry is going to be updated without permission bits by ppc_md.tce_build(). The DMA operation covered by this TCE entry will cause EEH error. More logs would be helpful to locate the root cause of the EEH error :-) >+ } >+} >+EXPORT_SYMBOL_GPL(iommu_direction_to_tce_perm); >+ The function converts generic permission flags to PCI specific flags as the names (TCE_PCI_{READ,WRITE}) indicates. I'm not sure if it's reasonable to have function name iommu_direction_to_pci_tce_perm(). Platform devices who have DMA capability might have different flags other than TCE_PCI_{READ,WRITE} and possibly use iommu.c to manage platform specific TCE table. We might not have the use case for now, so I'm not sure it makes sense to have a more specific function name. Thanks, Gavin > #ifdef CONFIG_IOMMU_API > /* > * SPAPR TCE API >diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c >index bca2aeb..b7ea245 100644 >--- a/arch/powerpc/platforms/powernv/pci.c >+++ b/arch/powerpc/platforms/powernv/pci.c >@@ -576,15 +576,10 @@ static int pnv_tce_build(struct iommu_table *tbl, long index, long npages, > unsigned long uaddr, enum dma_data_direction direction, > struct dma_attrs *attrs, bool rm) > { >- u64 proto_tce; >+ u64 proto_tce = iommu_direction_to_tce_perm(direction); > __be64 *tcep, *tces; > u64 rpn; > >- proto_tce = TCE_PCI_READ; // Read allowed >- >- if (direction != DMA_TO_DEVICE) >- proto_tce |= TCE_PCI_WRITE; >- > tces = tcep = ((__be64 *)tbl->it_base) + index - tbl->it_offset; > rpn = __pa(uaddr) >> tbl->it_page_shift; > >-- >2.4.0.rc3.8.gfb3e7d5 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/