[PATCH] iommu/arm-smmu: Only return IRQ_NONE if FSR is not set

[PATCH] iommu/arm-smmu: Only return IRQ_NONE if FSR is not set

[Mitchel Humpherys]

Currently we return IRQ_NONE from the context fault handler if the FSR
doesn't actually have the fault bit set (some sort of miswired
interrupt?) or if the client doesn't register an IOMMU fault handler.
However, registering a client fault handler is optional, so telling the
interrupt framework that the interrupt wasn't for this device if the
client doesn't register a handler isn't exactly accurate.  Fix this by
returning IRQ_HANDLED even if the client doesn't register a handler.

Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 48a39dfa9777..95560d447a54 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -653,7 +653,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 		dev_err_ratelimited(smmu->dev,
 		    "Unhandled context fault: iova=0x%08lx, fsynr=0x%x, cb=%d\n",
 		    iova, fsynr, cfg->cbndx);
-		ret = IRQ_NONE;
+		ret = IRQ_HANDLED;
 		resume = RESUME_TERMINATE;
 	}
 
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] iommu/arm-smmu: Only return IRQ_NONE if FSR is not set

[Will Deacon]

Hi Mitch,

On Sat, Sep 26, 2015 at 01:12:05AM +0100, Mitchel Humpherys wrote:
> Currently we return IRQ_NONE from the context fault handler if the FSR
> doesn't actually have the fault bit set (some sort of miswired
> interrupt?) or if the client doesn't register an IOMMU fault handler.
> However, registering a client fault handler is optional, so telling the
> interrupt framework that the interrupt wasn't for this device if the
> client doesn't register a handler isn't exactly accurate.  Fix this by
> returning IRQ_HANDLED even if the client doesn't register a handler.
> 
> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
> ---
>  drivers/iommu/arm-smmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 48a39dfa9777..95560d447a54 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -653,7 +653,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>  		dev_err_ratelimited(smmu->dev,
>  		    "Unhandled context fault: iova=0x%08lx, fsynr=0x%x, cb=%d\n",
>  		    iova, fsynr, cfg->cbndx);
> -		ret = IRQ_NONE;
> +		ret = IRQ_HANDLED;
>  		resume = RESUME_TERMINATE;

Hmm, but if we haven't actually done anything to rectify the cause of the
fault, what means that we won't take it again immediately? I guess I'm not
understanding the use-case that triggered you to write this patch...

Will

[PATCH] iommu/arm-smmu: Only return IRQ_NONE if FSR is not set

[Mitchel Humpherys]

On Mon, Oct 05 2015 at 03:24:03 PM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Mitch,
>
> On Sat, Sep 26, 2015 at 01:12:05AM +0100, Mitchel Humpherys wrote:
>> Currently we return IRQ_NONE from the context fault handler if the FSR
>> doesn't actually have the fault bit set (some sort of miswired
>> interrupt?) or if the client doesn't register an IOMMU fault handler.
>> However, registering a client fault handler is optional, so telling the
>> interrupt framework that the interrupt wasn't for this device if the
>> client doesn't register a handler isn't exactly accurate.  Fix this by
>> returning IRQ_HANDLED even if the client doesn't register a handler.
>> 
>> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
>> ---
>>  drivers/iommu/arm-smmu.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 48a39dfa9777..95560d447a54 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -653,7 +653,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>>  		dev_err_ratelimited(smmu->dev,
>>  		    "Unhandled context fault: iova=0x%08lx, fsynr=0x%x, cb=%d\n",
>>  		    iova, fsynr, cfg->cbndx);
>> -		ret = IRQ_NONE;
>> +		ret = IRQ_HANDLED;
>>  		resume = RESUME_TERMINATE;
>
> Hmm, but if we haven't actually done anything to rectify the cause of the
> fault, what means that we won't take it again immediately? I guess I'm not
> understanding the use-case that triggered you to write this patch...

Does returning IRQ_NONE actually prevent us from taking another
interrupt (despite clearing the FSR below)?  We definitely take more
interrupts on our platform despite returning IRQ_NONE, but maybe we have
something misconfigured...

I thought that returning IRQ_NONE would simply affect spurious interrupt
accounting (only disabling the interrupt if we took enough of them in
close enough succession to flag a misbehaving device).

As far as a valid use case, I can't think of one.  I just thought we
were messing up the spurious interrupt accounting.


-Mitch

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] iommu/arm-smmu: Only return IRQ_NONE if FSR is not set

[Will Deacon]

On Tue, Oct 06, 2015 at 01:40:33PM -0700, Mitchel Humpherys wrote:
> On Mon, Oct 05 2015 at 03:24:03 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Sat, Sep 26, 2015 at 01:12:05AM +0100, Mitchel Humpherys wrote:
> >> Currently we return IRQ_NONE from the context fault handler if the FSR
> >> doesn't actually have the fault bit set (some sort of miswired
> >> interrupt?) or if the client doesn't register an IOMMU fault handler.
> >> However, registering a client fault handler is optional, so telling the
> >> interrupt framework that the interrupt wasn't for this device if the
> >> client doesn't register a handler isn't exactly accurate.  Fix this by
> >> returning IRQ_HANDLED even if the client doesn't register a handler.
> >> 
> >> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
> >> ---
> >>  drivers/iommu/arm-smmu.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 48a39dfa9777..95560d447a54 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -653,7 +653,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> >>  		dev_err_ratelimited(smmu->dev,
> >>  		    "Unhandled context fault: iova=0x%08lx, fsynr=0x%x, cb=%d\n",
> >>  		    iova, fsynr, cfg->cbndx);
> >> -		ret = IRQ_NONE;
> >> +		ret = IRQ_HANDLED;
> >>  		resume = RESUME_TERMINATE;
> >
> > Hmm, but if we haven't actually done anything to rectify the cause of the
> > fault, what means that we won't take it again immediately? I guess I'm not
> > understanding the use-case that triggered you to write this patch...
> 
> Does returning IRQ_NONE actually prevent us from taking another
> interrupt (despite clearing the FSR below)?  We definitely take more
> interrupts on our platform despite returning IRQ_NONE, but maybe we have
> something misconfigured...

No, IRQ_NONE doesn't prevent anything unless we trigger the spurious IRQ
detector (1000 IRQs / second iirc). In that case, the handler ends up being
invoked off the back of a timer tick last time I looked.

My concern is that the source of the interrupt isn't handled at all in
the case above. Sure, we clear the FSR, but we haven't actually done
anything to stop the fault occuring again. It would be like dealing with
a page fault from userspace by simply returning back to the application
without actually updating the page tables.

Will