iommu/arm-smmu: Only return IRQ_NONE if FSR is not set
PATCH 1/1 iommu/arm-smmu: Only return IRQ_NONE if FSR is not set
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.
--- drivers/iommu/arm-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/arm-smmu.c +1 -1
@@ -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 ; }
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 1/1 iommu/arm-smmu: Only return IRQ_NONE if FSR is not set
Hi Mitch,
Quoted 18 lines · Mitchel Humpherys
Currently we return
IRQ_NONEfrom 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 returningIRQ_HANDLEDeven if the client doesn't register a handler.Signed-off-by Mitchel Humpherys <[email protected]>--- drivers/iommu/arm-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iommu/arm-smmu.c +0
-0
drivers/iommu/arm-smmu.c static irqreturn_t arm_smmu_context_fault(int irq, void *dev) · line 657
Quoted 6 lines
drivers/iommu/arm-smmu.c +1
-1@@ -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 1/1 iommu/arm-smmu: Only return IRQ_NONE if FSR is not set
Quoted 16 lines · Will Deacon
Hi Mitch,
Quoted 13 lines · Mitchel Humpherys
Currently we return
IRQ_NONEfrom 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 returningIRQ_HANDLEDeven if the client doesn't register a handler.Signed-off-by Mitchel Humpherys <[email protected]>--- drivers/iommu/arm-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines
drivers/iommu/arm-smmu.c static irqreturn_t arm_smmu_context_fault(int irq, void *dev) · line 657
Quoted 11 lines
drivers/iommu/arm-smmu.c +1
-1@@ -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
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 1/1 iommu/arm-smmu: Only return IRQ_NONE if FSR is not set
Quoted 31 lines · Mitchel Humpherys
Quoted 14 lines · Will Deacon
Quoted 13 lines · Mitchel Humpherys
Currently we return
IRQ_NONEfrom 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 returningIRQ_HANDLEDeven if the client doesn't register a handler.Signed-off-by Mitchel Humpherys <[email protected]>--- drivers/iommu/arm-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Quoted 15 lines
drivers/iommu/arm-smmu.c static irqreturn_t arm_smmu_context_fault(int irq, void *dev) · line 657
Quoted 11 lines
drivers/iommu/arm-smmu.c +1
-1@@ -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...
Quoted 4 lines
Does returning
IRQ_NONEactually prevent us from taking another interrupt (despite clearing the FSR below)? We definitely take more interrupts on our platform despite returningIRQ_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