Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966262AbcKKAq3 (ORCPT ); Thu, 10 Nov 2016 19:46:29 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:55244 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935685AbcKKAq0 (ORCPT ); Thu, 10 Nov 2016 19:46:26 -0500 From: Laurent Pinchart To: Magnus Damm Cc: iommu@lists.linux-foundation.org, laurent.pinchart+renesas@ideasonboard.com, geert+renesas@glider.be, joro@8bytes.org, linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, horms+renesas@verge.net.au, robin.murphy@arm.com, m.szyprowski@samsung.com Subject: Re: [PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context Date: Fri, 11 Nov 2016 02:46:14 +0200 Message-ID: <2771674.NN4Eo9tErC@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: <20161019233553.10506.72700.sendpatchset@little-apple> References: <20161019233533.10506.16810.sendpatchset@little-apple> <20161019233553.10506.72700.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: 6290 Lines: 224 Hi Magnus, Thank you for the patch. On Thursday 20 Oct 2016 08:35:53 Magnus Damm wrote: > From: Magnus Damm > > Introduce a bitmap for context handing and convert the > interrupt routine to handle all registered contexts. > > At this point the number of contexts are still limited. > > Also remove the use of the ARM specific mapping variable > from ipmmu_irq() to allow compile on ARM64. > > Signed-off-by: Magnus Damm > Reviewed-by: Joerg Roedel > --- > > Changes since V5: > - None > > Changes since V4: > - None > > Changes since V3: > - None > > Changes since V2: (Thanks again to Laurent!) > - Introduce a spinlock together with the bitmap and domain array. > - Break out code into separate functions for alloc and free. > - Perform free after (instead of before) configuring hardware registers. > - Use the spinlock to protect the domain array in the interrupt handler. > > Changes since V1: (Thanks to Laurent for feedback!) > - Use simple find_first_zero()/set_bit()/clear_bit() for context > management. - For allocation rely on spinlock held when calling > ipmmu_domain_init_context() - For test/free use atomic bitops > - Return IRQ_HANDLED if any of the contexts generated interrupts > > drivers/iommu/ipmmu-vmsa.c | 76 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 66 insertions(+), 10 deletions(-) > > --- 0004/drivers/iommu/ipmmu-vmsa.c > +++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:48:23.770607110 +0900 > @@ -8,6 +8,7 @@ > * the Free Software Foundation; version 2 of the License. > */ > > +#include > #include > #include > #include > @@ -26,12 +27,17 @@ > > #include "io-pgtable.h" > > +#define IPMMU_CTX_MAX 1 > + > struct ipmmu_vmsa_device { > struct device *dev; > void __iomem *base; > struct list_head list; > > unsigned int num_utlbs; > + spinlock_t lock; /* Protects ctx and domains[] */ > + DECLARE_BITMAP(ctx, IPMMU_CTX_MAX); > + struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX]; > > struct dma_iommu_mapping *mapping; > }; > @@ -293,9 +299,29 @@ static struct iommu_gather_ops ipmmu_gat > * Domain/Context Management > */ > > +static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu, > + struct ipmmu_vmsa_domain *domain) Nitpicking, I'd name this ipmmu_domain_alloc_context() as the driver uses the alloc abbreviation already. > +{ > + unsigned long flags; > + int ret; > + > + spin_lock_irqsave(&mmu->lock, flags); > + > + ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX); > + if (ret != IPMMU_CTX_MAX) { > + mmu->domains[ret] = domain; > + set_bit(ret, mmu->ctx); > + } How about returning a negative error code on error instead of IPMMU_CTX_MAX ? I think it would make the API clearer, avoiding the need to think about special error handling for this function. Having said that, I find that the init/alloc and destroy/free function names don't carry a very clear semantic. Given the size of the alloc and free functions, how about inlining them in their single caller ? > + > + spin_unlock_irqrestore(&mmu->lock, flags); > + > + return ret; > +} > + > static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain) > { > u64 ttbr; > + int ret; > > /* > * Allocate the page table operations. > @@ -325,10 +351,15 @@ static int ipmmu_domain_init_context(str > return -EINVAL; > > /* > - * TODO: When adding support for multiple contexts, find an unused > - * context. > + * Find an unused context. > */ The comment now holds on one line. > - domain->context_id = 0; > + ret = ipmmu_domain_allocate_context(domain->mmu, domain); > + if (ret == IPMMU_CTX_MAX) { > + free_io_pgtable_ops(domain->iop); > + return -EBUSY; > + } > + > + domain->context_id = ret; > > /* TTBR0 */ > ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0]; > @@ -370,6 +401,19 @@ static int ipmmu_domain_init_context(str > return 0; > } > > +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu, > + unsigned int context_id) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&mmu->lock, flags); > + > + clear_bit(context_id, mmu->ctx); > + mmu->domains[context_id] = NULL; > + > + spin_unlock_irqrestore(&mmu->lock, flags); > +} > + > static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain) > { > /* > @@ -380,6 +424,7 @@ static void ipmmu_domain_destroy_context > */ > ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH); > ipmmu_tlb_sync(domain); > + ipmmu_domain_free_context(domain->mmu, domain->context_id); > } > > /* > --------------------------------------------------------------------------- > -- @@ -437,16 +482,25 @@ static irqreturn_t ipmmu_domain_irq(stru > static irqreturn_t ipmmu_irq(int irq, void *dev) > { > struct ipmmu_vmsa_device *mmu = dev; > - struct iommu_domain *io_domain; > - struct ipmmu_vmsa_domain *domain; > + irqreturn_t status = IRQ_NONE; > + unsigned int i; > + unsigned long flags; Nitpicking again I like to arrange variable declarations by decreasing line length when there's no reason not to :-) > - if (!mmu->mapping) > - return IRQ_NONE; > + spin_lock_irqsave(&mmu->lock, flags); > + > + /* > + * Check interrupts for all active contexts. > + */ This comment holds on a single line too. With all these small comments addressed, Reviewed-by: Laurent Pinchart > + for (i = 0; i < IPMMU_CTX_MAX; i++) { > + if (!mmu->domains[i]) > + continue; > + if (ipmmu_domain_irq(mmu->domains[i]) == IRQ_HANDLED) > + status = IRQ_HANDLED; > + } > > - io_domain = mmu->mapping->domain; > - domain = to_vmsa_domain(io_domain); > + spin_unlock_irqrestore(&mmu->lock, flags); > > - return ipmmu_domain_irq(domain); > + return status; > } > > /* > --------------------------------------------------------------------------- > -- @@ -774,6 +828,8 @@ static int ipmmu_probe(struct platform_d > > mmu->dev = &pdev->dev; > mmu->num_utlbs = 32; > + spin_lock_init(&mmu->lock); > + bitmap_zero(mmu->ctx, IPMMU_CTX_MAX); > > /* Map I/O memory and request IRQ. */ > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); -- Regards, Laurent Pinchart