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 <[email protected]> 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/[email protected]?part=4
