Re: [PATCH v6 2/7] iommu/arm-smmu: Move handing of RESUME to the context fault handler

[Will Deacon <will@kernel.org>]

On Thu, May 15, 2025 at 03:58:44PM -0400, Connor Abbott wrote:
> The upper layer fault handler is now expected to handle everything
> required to retry the transaction or dump state related to it, since we
> enable threaded IRQs. This means that we can take charge of writing
> RESUME, making sure that we always write it after writing FSR as
> recommended by the specification.
> 
> The iommu handler should write -EAGAIN if a transaction needs to be
> retried. This avoids tricky cross-tree changes in drm/msm, since it
> never wants to retry the transaction and it already returns 0 from its
> fault handler. Therefore it will continue to correctly terminate the
> transaction without any changes required.
> 
> devcoredumps from drm/msm will temporarily be broken until it is fixed
> to collect devcoredumps inside its fault handler, but fixing that first
> would actually be worse because MMU-500 ignores writes to RESUME unless
> all fields of FSR (except SS of course) are clear and raises an
> interrupt when only SS is asserted. Right now, things happen to work
> most of the time if we collect a devcoredump, because RESUME is written
> asynchronously in the fault worker after the fault handler clears FSR
> and finishes, although there will be some spurious faults, but if this
> is changed before this commit fixes the FSR/RESUME write order then SS
> will never be cleared, the interrupt will never be cleared, and the
> whole system will hang every time a fault happens. It will therefore
> help bisectability if this commit goes first.
> 
> I've changed the TBU path to also accept -EAGAIN and do the same thing,
> while keeping the old -EBUSY behavior. Although the old path was broken
> because you'd get a storm of interrupts due to returning IRQ_NONE that
> would eventually result in the interrupt being disabled, and I think it
> was dead code anyway, so it should eventually be deleted. Note that
> drm/msm never uses TBU so this is untested.
> 
> Signed-off-by: Connor Abbott <cwabbott0@gmail.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c |  9 +++++++++
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c       | 14 --------------
>  drivers/iommu/arm/arm-smmu/arm-smmu.c            |  6 ++++++
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> index 548783f3f8e89fd978367afa65c473002f66e2e7..3e0c2c7c639b0c09243578ebb95129398c630ef2 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> @@ -406,6 +406,12 @@ irqreturn_t qcom_smmu_context_fault(int irq, void *dev)
>  			arm_smmu_print_context_fault_info(smmu, idx, &cfi);
>  
>  		arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_FSR, cfi.fsr);
> +
> +		if (cfi.fsr & ARM_SMMU_CB_FSR_SS) {
> +			arm_smmu_cb_write(smmu, idx, ARM_SMMU_CB_RESUME,
> +					  ret == -EAGAIN ? 0 : ARM_SMMU_RESUME_TERMINATE);
> +		}
> +
>  		return IRQ_HANDLED;
>  	}
>  
> @@ -416,6 +422,9 @@ irqreturn_t qcom_smmu_context_fault(int irq, void *dev)
>  	if (!tmp || tmp == -EBUSY) {
>  		ret = IRQ_HANDLED;
>  		resume = ARM_SMMU_RESUME_TERMINATE;
> +	} else if (tmp == -EAGAIN) {
> +		ret = IRQ_HANDLED;
> +		resume = 0;
>  	} else {
>  		phys_addr_t phys_atos = qcom_smmu_verify_fault(smmu_domain, cfi.iova, cfi.fsr);

Hrm, this debug stuff looks like it could use some clean-up. Not for
this series, but I may have a quick look on top...

Will