Re: [RFC PATCH 5/9] iommu/rockchip: skip orphaned-fault banks in CMD_ENABLE_STALL dispatch

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Missing reset_control_put() causes a resource leak on PM domain removal.
- [Medium] rk_iommu_disable_stall() unconditionally times out if all MMU banks are in the orphaned-fault state.
- [Low] Relaxing the global 'required' list breaks validation for RK3588 NPU.

Pre-existing issues:
- [High] On multi-bank IOMMUs (num_mmu > 1), rk_iommu_disable_stall and rk_iommu_disable_paging return success prematurely when disabling, potentially leaving banks active.
- [High] The `rk_iommu_probe` function contains a critical flaw in how it populates and handles errors in the `iommu->bases` array, leading to a guaranteed kernel panic on malformed device trees and a sparse array bug resulting in `ERR_PTR` dereferences.
--

--- Patch [5]: [RFC PATCH 5/9] iommu/rockchip: skip orphaned-fault banks in CMD_ENABLE_STALL dispatch ---
commit e1850eec170c4af87cf79a1f7f05257d02a8ccf8
Author: Jiaxing Hu <gahing@gahingwoo.com>

iommu/rockchip: skip orphaned-fault banks in CMD_ENABLE_STALL dispatch

Sending CMD_ENABLE_STALL to a bank that carries an orphaned firmware
fault (PAGE_FAULT_ACTIVE & !STALL_ACTIVE & IDLE) causes the command to
be silently dropped by that bank.  The dropped command also interferes
with the other banks' stall transition, delaying them past the poll
timeout even though they would otherwise stall within a few
microseconds.

> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index ad462cee6bd89..7dbcc978ede79 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -436,7 +436,21 @@ static int rk_iommu_enable_stall(struct rk_iommu *iommu)
>  	if (!rk_iommu_is_paging_enabled(iommu))
>  		return 0;
>  
> -	rk_iommu_command(iommu, RK_MMU_CMD_ENABLE_STALL);
> +	/*
> +	 * Do not send CMD_ENABLE_STALL to orphaned-fault banks
> +	 * (PAGE_FAULT_ACTIVE & !STALL_ACTIVE & IDLE): the command is ignored
> +	 * by such a bank but its presence on the shared bus delays the other
> +	 * banks from reaching STALL_ACTIVE within the poll timeout.
> +	 */
> +	for (i = 0; i < iommu->num_mmu; i++) {
> +		u32 status = rk_iommu_read(iommu->bases[i], RK_MMU_STATUS);
> +
> +		if ((status & RK_MMU_STATUS_PAGE_FAULT_ACTIVE) &&
> +		    !(status & RK_MMU_STATUS_STALL_ACTIVE) &&
> +		    (status & RK_MMU_STATUS_IDLE))
> +			continue;
> +		writel(RK_MMU_CMD_ENABLE_STALL, iommu->bases[i] + RK_MMU_COMMAND);
> +	}
>  
>  	ret = readx_poll_timeout(rk_iommu_is_stall_active, iommu, val,
>  				 val, RK_MMU_POLL_PERIOD_US,

[Severity: Medium]
Does rk_iommu_disable_stall() unconditionally timeout if all MMU banks are in
the orphaned-fault state?

Looking at rk_iommu_is_stall_active(), if all banks are orphaned, the loop
skips evaluating them:

drivers/iommu/rockchip-iommu.c:rk_iommu_is_stall_active() {
...
	for (i = 0; i < iommu->num_mmu; i++) {
...
		if ((status & RK_MMU_STATUS_PAGE_FAULT_ACTIVE) &&
		    !(status & RK_MMU_STATUS_STALL_ACTIVE) &&
		    (status & RK_MMU_STATUS_IDLE))
			continue;

		active &= !!(status & RK_MMU_STATUS_STALL_ACTIVE);
	}

	return active;
}

Since 'active' starts as true, it remains true and does not reflect their
bypassed state, meaning a fully-skipped IOMMU reports as stalled.

Then in rk_iommu_disable_stall():

drivers/iommu/rockchip-iommu.c:rk_iommu_disable_stall() {
...
	ret = readx_poll_timeout(rk_iommu_is_stall_active, iommu, val,
				 !val, RK_MMU_POLL_PERIOD_US,
				 RK_MMU_POLL_TIMEOUT_US);
...
}

The polling loop waits for !rk_iommu_is_stall_active(), which will
unconditionally exhaust its timeout if all banks are orphaned and continually
report as stalled.

[Severity: High]
This is a pre-existing issue, but do rk_iommu_disable_stall() and
rk_iommu_disable_paging() return success prematurely when disabling on
multi-bank IOMMUs?

Because of the bitwise AND reduction in rk_iommu_is_stall_active():

drivers/iommu/rockchip-iommu.c:rk_iommu_is_stall_active() {
...
	for (i = 0; i < iommu->num_mmu; i++) {
...
		active &= !!(status & RK_MMU_STATUS_STALL_ACTIVE);
	}

	return active;
}

The function returns false (indicating no stall) if ANY single bank has
finished its stall transition, ignoring banks that are still active.

Then in rk_iommu_disable_stall():

drivers/iommu/rockchip-iommu.c:rk_iommu_disable_stall() {
...
	ret = readx_poll_timeout(rk_iommu_is_stall_active, iommu, val,
				 !val, RK_MMU_POLL_PERIOD_US,
...
}

The poll condition !val incorrectly succeeds as soon as any single bank
unstalls, potentially leaving other hardware banks active.

[Severity: High]
This is a pre-existing issue, but is there a risk of a kernel panic in
rk_iommu_probe() due to how the iommu->bases array is handled?

If the device tree has 0 resources, or if devm_ioremap_resource() fails for an
early resource but succeeds for a later one in a multi-MMU configuration, the
array is populated sparsely:

drivers/iommu/rockchip-iommu.c:rk_iommu_probe() {
...
	for (i = 0; i < num_res; i++) {
		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
		if (!res)
			continue;
		iommu->bases[i] = devm_ioremap_resource(&pdev->dev, res);
		if (IS_ERR(iommu->bases[i]))
			continue;
		iommu->num_mmu++;
	}
	if (iommu->num_mmu == 0)
		return PTR_ERR(iommu->bases[0]);
...
}

If zero resources are provided, devm_kcalloc() returns ZERO_SIZE_PTR, which is
then invalidly dereferenced at index 0. Also, if an early resource fails
mapping, the array is populated sparsely using loop index 'i' instead of
'num_mmu', but runtime logic expects contiguous valid pointers, leading to a
dereference of ERR_PTR during register reads.

[Severity: Medium]
Does rockchip_pm_remove_one_domain() miss freeing reset controls on
PM domain removal?

When the PM domain is removed (e.g., during driver unbind) or if probing
fails for a later power domain:

drivers/pmdomain/rockchip/pm-domains.c:rockchip_pm_remove_one_domain() {
...
	clk_bulk_unprepare(pd->num_clks, pd->clks);
	clk_bulk_put(pd->num_clks, pd->clks);
...
}

This teardown function misses the reset_control_put() call to free the
reset controls retrieved during addition, causing a resource leak.

[Severity: Low]
Does relaxing the global 'required' list break validation for RK3588 NPU?

Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml:
...
  - reset-names
  - npu-supply

additionalProperties: false
...

The 'sram-supply' property was removed from the global 'required' list here.
Will DT schema validation fail to flag missing 'sram-supply' for RK3588
devices?

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