Re: [RFC PATCH 4/9] iommu/rockchip: skip orphaned-fault banks in rk_iommu_is_stall_active

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Skipping orphaned-fault banks in `rk_iommu_is_stall_active` causes `rk_iommu_disable_stall` to unconditionally time out if all banks are skipped (e.g., on a single-bank IOMMU).

Pre-existing issues:
- [High] Multi-bank disable operations (`rk_iommu_disable_stall` and `rk_iommu_disable_paging`) prematurely terminate their waits as soon as the first bank completes its operation.
--

--- Patch [4]: [RFC PATCH 4/9] iommu/rockchip: skip orphaned-fault banks in rk_iommu_is_stall_active ---
commit 0709b1254bd6d532239176eba388c28b0ed8ea3c
Author: Jiaxing Hu <gahing@gahingwoo.com>

iommu/rockchip: skip orphaned-fault banks in rk_iommu_is_stall_active

This commit updates rk_iommu_is_stall_active() to detect and skip
orphaned-fault IOMMU banks that were left in a faulted state by boot
firmware. This ensures the driver doesn't treat them as "not stalled"
which would otherwise cause stall wait loops to fail.

> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 87ae036d64145..ad462cee6bd89 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -380,9 +380,23 @@ static bool rk_iommu_is_stall_active(struct rk_iommu *iommu)
>  	bool active = true;
>  	int i;
>  
> -	for (i = 0; i < iommu->num_mmu; i++)
> -		active &= !!(rk_iommu_read(iommu->bases[i], RK_MMU_STATUS) &
> -					   RK_MMU_STATUS_STALL_ACTIVE);
> +	for (i = 0; i < iommu->num_mmu; i++) {
> +		u32 status = rk_iommu_read(iommu->bases[i], RK_MMU_STATUS);
> +
> +		/*
> +		 * A bank stuck with PAGE_FAULT_ACTIVE but without STALL_ACTIVE
> +		 * and with IDLE set has an orphaned fault left by firmware before
> +		 * paging was configured.  It cannot enter stall mode but has no
> +		 * transaction in flight, so it is already quiescent.  Skip it
> +		 * rather than treating it as "not stalled."
> +		 */
> +		if ((status & RK_MMU_STATUS_PAGE_FAULT_ACTIVE) &&
> +		    !(status & RK_MMU_STATUS_STALL_ACTIVE) &&
> +		    (status & RK_MMU_STATUS_IDLE))
> +			continue;

[Severity: Medium]
Does skipping banks here cause a timeout if all banks are skipped?

If an IOMMU has a single bank (num_mmu == 1) and encounters this orphaned
fault, the loop skips the only bank and the active variable remains true.

When rk_iommu_disable_stall() polls for the stall to complete:

	ret = readx_poll_timeout(rk_iommu_is_stall_active, iommu, val,
				 !val, RK_MMU_POLL_PERIOD_US,
				 RK_MMU_POLL_TIMEOUT_US);

It expects rk_iommu_is_stall_active() to return false, but it will return
true, causing an unconditional timeout and a spurious error log.

> +
> +		active &= !!(status & RK_MMU_STATUS_STALL_ACTIVE);

[Severity: High]
This is a pre-existing issue, but does this bitwise AND reduction cause
multi-bank disable operations to terminate their waits prematurely?

When disabling an IOMMU with multiple banks (num_mmu > 1), returning false
as soon as the first bank drops its STALL_ACTIVE status leaves subsequent
banks still active.

The same logic exists in rk_iommu_is_paging_enabled(), where the bitwise
AND causes readx_poll_timeout() to terminate its wait as soon as one bank
is disabled, rather than waiting for all banks:

	for (i = 0; i < iommu->num_mmu; i++)
		enable &= !!(rk_iommu_read(iommu->bases[i], RK_MMU_STATUS) &
					   RK_MMU_STATUS_PAGING_ENABLED);

Can this lead to modifying register state or disabling hardware clocks while
some MMU banks are still actively processing commands?

> +	}
>  
>  	return active;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717085220.3212274-1-gahing@gahingwoo.com?part=4