From patchwork Wed Jul 30 15:31:14 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Will Deacon X-Patchwork-Id: 34546 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-vc0-f198.google.com (mail-vc0-f198.google.com [209.85.220.198]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id EB8B520792 for ; Wed, 30 Jul 2014 15:32:54 +0000 (UTC) Received: by mail-vc0-f198.google.com with SMTP id le20sf3906445vcb.5 for ; Wed, 30 Jul 2014 08:32:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:date:from:to:subject:message-id :references:mime-version:in-reply-to:user-agent:cc:precedence :list-id:list-unsubscribe:list-archive:list-post:list-help :list-subscribe:sender:errors-to:x-original-sender :x-original-authentication-results:mailing-list:content-disposition :content-type:content-transfer-encoding; bh=rG8SMwHgivBe3Fq7ZhTRP4KDAH4zpXI9YI+8QBRmq/g=; b=cZubfMtfXzZi/YsU6gqyd52wU4mQmVLABSTK/66sQAcydjCBACKhIFit7TV86mcwKV s++9DuV5fgnBLY0CV52N/f/PKMbuGwC+6Vc1AcfK6bdX6FEBJbaVmVPeYT1uGXRSXJob dP+67mazPC3dqU8ylyP+S5i1U8JpYFy3twDsvYNvXjlZDIxIUukN7NZYvAg2OK1xepyb P7jXUhvKkRFtazr6UzIeIO/IBx9Q7AFcBCgBkw7TNdypfZEAKbDdyuJXNwdxpE2+aSRy lOJQnYVIHBCvUHTHK5KXqnNOv+GgTTmpPrXw4HEm4nrjUXXl0ww+ZYpHvnTGsdG67v1g xE+w== X-Gm-Message-State: ALoCoQnsg5K/8gS3+GFgwiDD4hftSPgowdpw2liMMKUYODc5np/E3eScXsbJ+G/yjgW1Vd6iMqlf X-Received: by 10.236.98.33 with SMTP id u21mr1649479yhf.39.1406734370101; Wed, 30 Jul 2014 08:32:50 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.37.6 with SMTP id q6ls584008qgq.49.gmail; Wed, 30 Jul 2014 08:32:50 -0700 (PDT) X-Received: by 10.52.248.232 with SMTP id yp8mr3091116vdc.83.1406734369987; Wed, 30 Jul 2014 08:32:49 -0700 (PDT) Received: from mail-vc0-f179.google.com (mail-vc0-f179.google.com [209.85.220.179]) by mx.google.com with ESMTPS id th5si2027876vdb.39.2014.07.30.08.32.49 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 30 Jul 2014 08:32:49 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.179 as permitted sender) client-ip=209.85.220.179; Received: by mail-vc0-f179.google.com with SMTP id hq11so2046616vcb.24 for ; Wed, 30 Jul 2014 08:32:49 -0700 (PDT) X-Received: by 10.52.120.38 with SMTP id kz6mr2054750vdb.86.1406734368729; Wed, 30 Jul 2014 08:32:48 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.221.37.5 with SMTP id tc5csp36657vcb; Wed, 30 Jul 2014 08:32:48 -0700 (PDT) X-Received: by 10.70.94.42 with SMTP id cz10mr5606125pdb.107.1406734367471; Wed, 30 Jul 2014 08:32:47 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id aw13si2817928pac.24.2014.07.30.08.32.46 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 30 Jul 2014 08:32:47 -0700 (PDT) Received-SPF: none (google.com: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org does not designate permitted sender hosts) client-ip=2001:1868:205::9; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1XCVqu-0008J3-Oc; Wed, 30 Jul 2014 15:31:40 +0000 Received: from cam-admin0.cambridge.arm.com ([217.140.96.50]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1XCVqr-00083W-05 for linux-arm-kernel@lists.infradead.org; Wed, 30 Jul 2014 15:31:37 +0000 Received: from arm.com (edgewater-inn.cambridge.arm.com [10.1.203.34]) by cam-admin0.cambridge.arm.com (8.12.6/8.12.6) with ESMTP id s6UFV7wo000111; Wed, 30 Jul 2014 16:31:07 +0100 (BST) Date: Wed, 30 Jul 2014 16:31:14 +0100 From: Will Deacon To: Mitchel Humpherys Subject: Re: [PATCH v3] iommu/arm-smmu: avoid calling request_irq in atomic context Message-ID: <20140730153114.GG8989@arm.com> References: <1406657475-1232-1-git-send-email-mitchelh@codeaurora.org> MIME-Version: 1.0 In-Reply-To: <1406657475-1232-1-git-send-email-mitchelh@codeaurora.org> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140730_083137_399447_9BF0C23D X-CRM114-Status: GOOD ( 24.00 ) X-Spam-Score: -5.7 (-----) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-5.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [217.140.96.50 listed in list.dnswl.org] -0.7 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -0.0 SPF_PASS SPF: sender matches SPF record Cc: "iommu@lists.linux-foundation.org" , Russell King - ARM Linux , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: will.deacon@arm.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.179 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 Content-Disposition: inline Hey Mitch, On Tue, Jul 29, 2014 at 07:11:15PM +0100, Mitchel Humpherys wrote: > request_irq shouldn't be called from atomic context since it might > sleep, but we're calling it with a spinlock held, resulting in: > > [ 9.172202] BUG: sleeping function called from invalid context at kernel/mm/slub.c:926 > [ 9.182989] in_atomic(): 1, irqs_disabled(): 128, pid: 1, name: swapper/0 > [ 9.189762] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W 3.10.40-gbc1b510b-38437-g55831d3bd9-dirty #97 > [ 9.199757] [] (unwind_backtrace+0x0/0x11c) from [] (show_stack+0x10/0x14) > [ 9.208346] [] (show_stack+0x10/0x14) from [] (kmem_cache_alloc_trace+0x3c/0x210) > [ 9.217543] [] (kmem_cache_alloc_trace+0x3c/0x210) from [] (request_threaded_irq+0x88/0x11c) > [ 9.227702] [] (request_threaded_irq+0x88/0x11c) from [] (arm_smmu_attach_dev+0x188/0x858) > [ 9.237686] [] (arm_smmu_attach_dev+0x188/0x858) from [] (arm_iommu_attach_device+0x18/0xd0) > [ 9.247837] [] (arm_iommu_attach_device+0x18/0xd0) from [] (arm_smmu_test_probe+0x68/0xd4) > [ 9.257823] [] (arm_smmu_test_probe+0x68/0xd4) from [] (driver_probe_device+0x12c/0x330) > [ 9.267629] [] (driver_probe_device+0x12c/0x330) from [] (__driver_attach+0x68/0x8c) > [ 9.277090] [] (__driver_attach+0x68/0x8c) from [] (bus_for_each_dev+0x70/0x84) > [ 9.286118] [] (bus_for_each_dev+0x70/0x84) from [] (bus_add_driver+0x100/0x244) > [ 9.295233] [] (bus_add_driver+0x100/0x244) from [] (driver_register+0x9c/0x124) > [ 9.304347] [] (driver_register+0x9c/0x124) from [] (arm_smmu_test_init+0x14/0x38) > [ 9.313635] [] (arm_smmu_test_init+0x14/0x38) from [] (do_one_initcall+0xb8/0x160) > [ 9.322926] [] (do_one_initcall+0xb8/0x160) from [] (kernel_init_freeable+0x108/0x1cc) > [ 9.332564] [] (kernel_init_freeable+0x108/0x1cc) from [] (kernel_init+0xc/0xe4) > [ 9.341675] [] (kernel_init+0xc/0xe4) from [] (ret_from_fork+0x14/0x3c) > > Fix this by moving the request_irq out of the critical section. This > should be okay since smmu_domain->smmu is still being protected by the > critical section. Also, we still don't program the Stream Match Register > until after registering our interrupt handler so we shouldn't be missing > any interrupts. > > Signed-off-by: Mitchel Humpherys > --- > Changelog: > > - v3: rework irq request code to avoid requesting the irq every > time a master is added to the domain > - v2: return error code from request_irq on failure > --- > drivers/iommu/arm-smmu.c | 73 +++++++++++++++++++++++++++--------------------- > 1 file changed, 41 insertions(+), 32 deletions(-) I think this is correct, but we can do some cleanup now that you've moved all the locking into the conditional. Messy diff below, which would be much nicer sqaushed into your patch. What do you reckon? Will --->8 diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 572f5579d38b..e33df1a676ec 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -868,10 +868,15 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain) static int arm_smmu_init_domain_context(struct iommu_domain *domain, struct arm_smmu_device *smmu) { - int ret, start; + int irq, start, ret = 0; + unsigned long flags; struct arm_smmu_domain *smmu_domain = domain->priv; struct arm_smmu_cfg *cfg = &smmu_domain->cfg; + spin_lock_irqsave(&smmu_domain->lock, flags); + if (smmu_domain->smmu) + goto out_unlock; + if (smmu->features & ARM_SMMU_FEAT_TRANS_NESTED) { /* * We will likely want to change this if/when KVM gets @@ -890,7 +895,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, ret = __arm_smmu_alloc_bitmap(smmu->context_map, start, smmu->num_context_banks); if (IS_ERR_VALUE(ret)) - return ret; + goto out_unlock; cfg->cbndx = ret; if (smmu->version == 1) { @@ -902,7 +907,22 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, ACCESS_ONCE(smmu_domain->smmu) = smmu; arm_smmu_init_context_bank(smmu_domain); + spin_unlock_irqrestore(&smmu_domain->lock, flags); + + irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx]; + ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED, + "arm-smmu-context-fault", smmu_domain); + if (IS_ERR_VALUE(ret)) { + dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n", + cfg->irptndx, irq); + cfg->irptndx = INVALID_IRPTNDX; + } + return 0; + +out_unlock: + spin_unlock_irqrestore(&smmu_domain->lock, flags); + return ret; } static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) @@ -1158,12 +1178,10 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain, static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) { - int irq, ret; + int ret; struct arm_smmu_domain *smmu_domain = domain->priv; struct arm_smmu_device *smmu, *dom_smmu; - struct arm_smmu_master_cfg *master_cfg; - struct arm_smmu_cfg *cfg = &smmu_domain->cfg; - unsigned long flags; + struct arm_smmu_master_cfg *cfg; smmu = dev_get_master_dev(dev)->archdata.iommu; if (!smmu) { @@ -1171,57 +1189,33 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) return -ENXIO; } - dom_smmu = ACCESS_ONCE(smmu_domain->smmu); - /* * Sanity check the domain. We don't support domains across * different SMMUs. */ + dom_smmu = ACCESS_ONCE(smmu_domain->smmu); if (!dom_smmu) { - spin_lock_irqsave(&smmu_domain->lock, flags); - /* - * make sure we're not racing with someone who might - * have just attached and init'd the domain context - */ - dom_smmu = smmu_domain->smmu; - if (dom_smmu) { - spin_unlock_irqrestore(&smmu_domain->lock, flags); - goto have_dom_smmu; - } /* Now that we have a master, we can finalise the domain */ ret = arm_smmu_init_domain_context(domain, smmu); - spin_unlock_irqrestore(&smmu_domain->lock, flags); if (IS_ERR_VALUE(ret)) return ret; - dom_smmu = smmu_domain->smmu; - irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx]; - ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED, - "arm-smmu-context-fault", domain); - if (IS_ERR_VALUE(ret)) { - dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n", - cfg->irptndx, irq); - cfg->irptndx = INVALID_IRPTNDX; - return ret; - } + dom_smmu = smmu_domain->smmu; } -have_dom_smmu: - if (dom_smmu != smmu) { dev_err(dev, "cannot attach to SMMU %s whilst already attached to domain on SMMU %s\n", - dev_name(smmu_domain->smmu->dev), - dev_name(smmu->dev)); + dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev)); return -EINVAL; } /* Looks ok, so add the device to the domain */ - master_cfg = find_smmu_master_cfg(smmu_domain->smmu, dev); - if (!master_cfg) + cfg = find_smmu_master_cfg(smmu_domain->smmu, dev); + if (!cfg) return -ENODEV; - return arm_smmu_domain_add_master(smmu_domain, master_cfg); + return arm_smmu_domain_add_master(smmu_domain, cfg); } static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)