Re: [RFC PATCH v2 4/8] iommu/rockchip: take all DT clocks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Resource mapping loop incorrectly indexes `iommu->bases` array, leading to gaps and invalid pointer dereferences
--

--- Patch [4]: [RFC PATCH v2 4/8] iommu/rockchip: take all DT clocks ---
commit 77bcbb4eca287b3632f08a8528235d32ff400f72
Author: Jiaxing Hu <gahing@gahingwoo.com>

iommu/rockchip: take all DT clocks
    
rk_iommu only enabled a fixed {aclk,iface} pair.  On the RK3576 NPU the
MMU sits behind the CBUF/DSU gates, so writes to DTE_ADDR are silently
dropped until those clocks run too (reads work, writes need more clocks).
Use devm_clk_bulk_get_all() to take every clock the devicetree provides
instead of a fixed pair, so a platform can list the full set it needs.
Clocks stay optional for the older clock-less devicetrees.

> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 87ae036d64145..2ac65c0c5c0b4 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c

[ ... ]

> @@ -1254,25 +1249,20 @@ static int rk_iommu_probe(struct platform_device *pdev)

[Severity: High]
This is a pre-existing issue, but does the resource mapping loop in
rk_iommu_probe() incorrectly index the iommu->bases array?

If multiple memory resources are provided but one fails to map (for example,
if num_res = 2 and mapping for index 0 fails), iommu->bases[0] holds an
ERR_PTR while iommu->bases[1] holds the valid base. However, iommu->num_mmu
becomes 1.

Since subsequent driver loops iterate from 0 to num_mmu - 1 and
unconditionally dereference iommu->bases, this would lead to a crash on
iommu->bases[0].

Also, if num_res == 0, devm_kcalloc() returns ZERO_SIZE_PTR and reading
iommu->bases[0] on the error path would panic:

	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]);

Could we assign to iommu->bases[iommu->num_mmu] instead of iommu->bases[i]
to prevent gaps in the array, and handle the num_res == 0 error path
differently?

> 	iommu->reset_disabled = device_property_read_bool(dev,
> 					"rockchip,disable-mmu-reset");
>  
> -	iommu->num_clocks = ARRAY_SIZE(rk_iommu_clocks);
> -	iommu->clocks = devm_kcalloc(iommu->dev, iommu->num_clocks,
> -				     sizeof(*iommu->clocks), GFP_KERNEL);
> -	if (!iommu->clocks)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < iommu->num_clocks; ++i)
> -		iommu->clocks[i].id = rk_iommu_clocks[i];
> -
> 	/*
> -	 * iommu clocks should be present for all new devices and devicetrees
> -	 * but there are older devicetrees without clocks out in the wild.
> -	 * So clocks as optional for the time being.
> +	 * Take every clock the devicetree provides.  Most IOMMU instances
> +	 * need exactly "aclk" + "iface", but e.g. the RK3576 NPU IOMMUs sit
> +	 * behind additional gates (CBUF/DSU) whose clocks must be running
> +	 * for register writes to land.  Clocks stay optional because there
> +	 * are older devicetrees without clocks out in the wild.
> 	 */
> -	err = devm_clk_bulk_get(iommu->dev, iommu->num_clocks, iommu->clocks);
> +	err = devm_clk_bulk_get_all(iommu->dev, &iommu->clocks);
> 	if (err == -ENOENT)
> 		iommu->num_clocks = 0;
> -	else if (err)
> +	else if (err < 0)
> 		return err;
> +	else
> +		iommu->num_clocks = err;
>  
> 	err = clk_bulk_prepare(iommu->num_clocks, iommu->clocks);
> 	if (err)

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