Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936365AbcCQQnn (ORCPT ); Thu, 17 Mar 2016 12:43:43 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:33825 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936348AbcCQQng (ORCPT ); Thu, 17 Mar 2016 12:43:36 -0400 From: Laurent Pinchart To: Magnus Damm Cc: iommu@lists.linux-foundation.org, laurent.pinchart+renesas@ideasonboard.com, geert+renesas@glider.be, linux-sh@vger.kernel.org, joro@8bytes.org, linux-kernel@vger.kernel.org, horms+renesas@verge.net.au Subject: Re: [PATCH v2 03/04] iommu/ipmmu-vmsa: Break out 32-bit ARM mapping code Date: Thu, 17 Mar 2016 12:10:22 +0200 Message-ID: <2432608.qKZAd0hYOh@avalon> User-Agent: KMail/4.14.8 (Linux/4.1.15-gentoo-r1; KDE/4.14.8; x86_64; ; ) In-Reply-To: <20160315042204.22103.88238.sendpatchset@little-apple> References: <20160315042136.22103.26570.sendpatchset@little-apple> <20160315042204.22103.88238.sendpatchset@little-apple> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5995 Lines: 207 Hi Magnus, Thank you for the patch. On Tuesday 15 March 2016 13:22:04 Magnus Damm wrote: > From: Magnus Damm > > Make the driver compile on more than just 32-bit ARM > by breaking out and wrapping ARM specific functions > in #ifdefs. Not pretty, but needed to be able to use > the driver on other architectures like ARM64. Given that the callers of the new ARM-specific functions will, after patch "[PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops", be used only on !CONFIG_IOMMU_DMA, and that ARM64 selects CONFIG_IOMMU_DMA, wouldn't it make more sense not to extract the ARM-specific code in separate functions but compile-out the whole ipmmu_ops structure and all the related operation handlers when !CONFIG_IOMMU_DMA ? I would order the patches as follows. [PATCH v2 01/04] iommu/ipmmu-vmsa: Remove platform data handling [PATCH v2 02/04] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context [PATCH v2 04/04] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency [PATCH 02/04] iommu/ipmmu-vmsa: Break out utlb parsing code [PATCH 03/04] iommu/ipmmu-vmsa: Break out domain allocation code and then squash the following two patches, with conditional compilation for ipmmu_ops. [PATCH 01/04] iommu/ipmmu-vmsa: 32-bit ARM may have CONFIG_IOMMU_DMA=y [PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops I think the result will be cleaner. Please see below for a couple of other comments. > Signed-off-by: Magnus Damm > --- > > Changes since V1: > - Rebased to work without patch 2 and 3 from V1 series > > drivers/iommu/ipmmu-vmsa.c | 94 +++++++++++++++++++++++++++------------- > 1 file changed, 62 insertions(+), 32 deletions(-) > > --- 0004/drivers/iommu/ipmmu-vmsa.c > +++ work/drivers/iommu/ipmmu-vmsa.c 2016-03-15 12:25:45.040513000 +0900 > @@ -22,8 +22,10 @@ > #include > #include > > +#ifdef CONFIG_ARM > #include > #include > +#endif > > #include "io-pgtable.h" > > @@ -38,7 +40,9 @@ struct ipmmu_vmsa_device { > DECLARE_BITMAP(ctx, IPMMU_CTX_MAX); > struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX]; > > +#ifdef CONFIG_ARM > struct dma_iommu_mapping *mapping; > +#endif > }; > > struct ipmmu_vmsa_domain { > @@ -615,6 +619,60 @@ static int ipmmu_find_utlbs(struct ipmmu > return 0; > } > > +#ifdef CONFIG_ARM > +static int ipmmu_map_attach(struct device *dev, struct ipmmu_vmsa_device > *mmu) > +{ > + int ret; > + > + /* > + * Create the ARM mapping, used by the ARM DMA mapping core to allocate > + * VAs. This will allocate a corresponding IOMMU domain. > + * > + * TODO: > + * - Create one mapping per context (TLB). > + * - Make the mapping size configurable ? We currently use a 2GB mapping > + * at a 1GB offset to ensure that NULL VAs will fault. > + */ > + if (!mmu->mapping) { > + struct dma_iommu_mapping *mapping; > + > + mapping = arm_iommu_create_mapping(&platform_bus_type, > + SZ_1G, SZ_2G); > + if (IS_ERR(mapping)) { > + dev_err(mmu->dev, "failed to create ARM IOMMU mapping\n"); > + return PTR_ERR(mapping); > + } > + > + mmu->mapping = mapping; > + } > + > + /* Attach the ARM VA mapping to the device. */ > + ret = arm_iommu_attach_device(dev, mmu->mapping); > + if (ret < 0) { > + dev_err(dev, "Failed to attach device to VA mapping\n"); > + arm_iommu_release_mapping(mmu->mapping); > + } > + > + return ret; > +} How about adding a blank line here ? > +static inline void ipmmu_detach(struct device *dev) > +{ > + arm_iommu_detach_device(dev); > +} And another one here ? > +static inline void ipmmu_release_mapping(struct ipmmu_vmsa_device *mmu) > +{ > + arm_iommu_release_mapping(mmu->mapping); > +} > +#else > +static inline int ipmmu_map_attach(struct device *dev, > + struct ipmmu_vmsa_device *mmu) > +{ > + return 0; > +} > +static inline void ipmmu_detach(struct device *dev) {} > +static inline void ipmmu_release_mapping(struct ipmmu_vmsa_device *mmu) {} The compiler should be smart enough to inline all these functions if needed. > +#endif > + > static int ipmmu_add_device(struct device *dev) > { > struct ipmmu_vmsa_archdata *archdata; > @@ -695,41 +753,13 @@ static int ipmmu_add_device(struct devic > archdata->num_utlbs = num_utlbs; > dev->archdata.iommu = archdata; > > - /* > - * Create the ARM mapping, used by the ARM DMA mapping core to allocate > - * VAs. This will allocate a corresponding IOMMU domain. > - * > - * TODO: > - * - Create one mapping per context (TLB). > - * - Make the mapping size configurable ? We currently use a 2GB mapping > - * at a 1GB offset to ensure that NULL VAs will fault. > - */ > - if (!mmu->mapping) { > - struct dma_iommu_mapping *mapping; > - > - mapping = arm_iommu_create_mapping(&platform_bus_type, > - SZ_1G, SZ_2G); > - if (IS_ERR(mapping)) { > - dev_err(mmu->dev, "failed to create ARM IOMMU mapping\n"); > - ret = PTR_ERR(mapping); > - goto error; > - } > - > - mmu->mapping = mapping; > - } > - > - /* Attach the ARM VA mapping to the device. */ > - ret = arm_iommu_attach_device(dev, mmu->mapping); > - if (ret < 0) { > - dev_err(dev, "Failed to attach device to VA mapping\n"); > + ret = ipmmu_map_attach(dev, mmu); > + if (ret < 0) > goto error; > - } > > return 0; > > error: > - arm_iommu_release_mapping(mmu->mapping); > - > kfree(dev->archdata.iommu); > kfree(utlbs); > > @@ -745,7 +775,7 @@ static void ipmmu_remove_device(struct d > { > struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu; > > - arm_iommu_detach_device(dev); > + ipmmu_detach(dev); > iommu_group_remove_device(dev); > > kfree(archdata->utlbs); > @@ -856,7 +886,7 @@ static int ipmmu_remove(struct platform_ > list_del(&mmu->list); > spin_unlock(&ipmmu_devices_lock); > > - arm_iommu_release_mapping(mmu->mapping); > + ipmmu_release_mapping(mmu); > > ipmmu_device_reset(mmu); -- Regards, Laurent Pinchart