On Wed, Jul 30 2014 at 08:31:14 AM, Will Deacon <will.dea...@arm.com> wrote:
> 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] [<c020c448>] (unwind_backtrace+0x0/0x11c) from
>> [<c02097d0>] (show_stack+0x10/0x14)
>> [ 9.208346] [<c02097d0>] (show_stack+0x10/0x14) from [<c0301d74>]
>> (kmem_cache_alloc_trace+0x3c/0x210)
>> [ 9.217543] [<c0301d74>] (kmem_cache_alloc_trace+0x3c/0x210) from
>> [<c0276a48>] (request_threaded_irq+0x88/0x11c)
>> [ 9.227702] [<c0276a48>] (request_threaded_irq+0x88/0x11c) from
>> [<c0931ca4>] (arm_smmu_attach_dev+0x188/0x858)
>> [ 9.237686] [<c0931ca4>] (arm_smmu_attach_dev+0x188/0x858) from
>> [<c0212cd8>] (arm_iommu_attach_device+0x18/0xd0)
>> [ 9.247837] [<c0212cd8>] (arm_iommu_attach_device+0x18/0xd0) from
>> [<c093314c>] (arm_smmu_test_probe+0x68/0xd4)
>> [ 9.257823] [<c093314c>] (arm_smmu_test_probe+0x68/0xd4) from
>> [<c05aadd0>] (driver_probe_device+0x12c/0x330)
>> [ 9.267629] [<c05aadd0>] (driver_probe_device+0x12c/0x330) from
>> [<c05ab080>] (__driver_attach+0x68/0x8c)
>> [ 9.277090] [<c05ab080>] (__driver_attach+0x68/0x8c) from
>> [<c05a92d4>] (bus_for_each_dev+0x70/0x84)
>> [ 9.286118] [<c05a92d4>] (bus_for_each_dev+0x70/0x84) from
>> [<c05aa3b0>] (bus_add_driver+0x100/0x244)
>> [ 9.295233] [<c05aa3b0>] (bus_add_driver+0x100/0x244) from
>> [<c05ab5d0>] (driver_register+0x9c/0x124)
>> [ 9.304347] [<c05ab5d0>] (driver_register+0x9c/0x124) from
>> [<c0933088>] (arm_smmu_test_init+0x14/0x38)
>> [ 9.313635] [<c0933088>] (arm_smmu_test_init+0x14/0x38) from
>> [<c0200618>] (do_one_initcall+0xb8/0x160)
>> [ 9.322926] [<c0200618>] (do_one_initcall+0xb8/0x160) from
>> [<c1200b7c>] (kernel_init_freeable+0x108/0x1cc)
>> [ 9.332564] [<c1200b7c>] (kernel_init_freeable+0x108/0x1cc) from
>> [<c0b924b0>] (kernel_init+0xc/0xe4)
>> [ 9.341675] [<c0b924b0>] (kernel_init+0xc/0xe4) from [<c0205e38>]
>> (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 <mitch...@codeaurora.org>
>> ---
>> 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?
Much cleaner, thanks. Just one question below.
>
> 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;
We want to return ret here due to the request_irq failure, right?
> + }
> +
> return 0;
> +
> +out_unlock:
> + spin_unlock_irqrestore(&smmu_domain->lock, flags);
> + return ret;
> }
-Mitch
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
