Re: [PATCH v3] iommu/arm-smmu: avoid calling request_irq in atomic context
On Wed, Jul 30, 2014 at 05:51:48PM +0100, Mitchel Humpherys wrote:
> On Wed, Jul 30 2014 at 08:31:14 AM, Will Deacon wrote:
> > On Tue, Jul 29, 2014 at 07:11:15PM +0100, Mitchel Humpherys wrote:
> >> 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.
[...]
> > @@ -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?
Actually, no, and it's a subtle change introduced by this fix. Before, we
would partially tear-down the domain here by freeing the cbndx but actually,
that's racy as hell. The moment we drop the lock another device can attach
successfully to the domain we've set, so we can't safely tear things down at
that point. The best bet is to continue with the warning -- you end up with
a domain without a context interrupt, but that's not fatal to the driver.
If we returned an error, I can't think of a safe way to reset the domain.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] iommu/arm-smmu: avoid calling request_irq in atomic context
On Wed, Jul 30 2014 at 08:31:14 AM, Will Deacon 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: GW
>> 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?
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)
Re: [PATCH v3] iommu/arm-smmu: avoid calling request_irq in atomic context
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: GW
> 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;
