Re: [PATCH 3/5] iommu/arm-smmu-v3: enable optional clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] iommu/arm-smmu-v3: device refcount leak in
  arm_smmu_device_probe()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] iommu/arm-smmu-v3: device reference leak in
  arm_smmu_device_dt_probe() [1]
--

commit 30c1e41e25870c67f127c773eba01cc753f1ae3f
Author: Arun Iyengar <aiyengar1990@gmail.com>

iommu/arm-smmu-v3: enable optional clocks

This commit acquires and enables all optional clocks described in the
firmware node during arm_smmu_device_probe() using
devm_clk_bulk_get_all_enabled().

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 6d4520c473086..cce66db932197 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c

[ ... ]

> @@ -5800,6 +5802,11 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;

[Severity: Medium]
This problem wasn't introduced by this patch, but can
arm_smmu_device_dt_probe() leak a reference on smmu->impl_dev [1] when
"#iommu-cells" is missing or invalid?

If "#iommu-cells" validation fails, arm_smmu_device_dt_probe() leaves ret as
-EINVAL and continues instead of returning early.  On "nvidia,tegra264-smmu"
it still calls tegra_cmdqv_dt_probe(), which acquires smmu->impl_dev via
of_find_device_by_node(), and then returns -EINVAL here before
arm_smmu_impl_probe() can take ownership of that reference:

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:arm_smmu_device_dt_probe() {
	if (of_property_read_u32(dev->of_node, "#iommu-cells", &cells))
		dev_err(dev, "missing #iommu-cells property\n");
	else if (cells != 1)
		dev_err(dev, "invalid #iommu-cells value (%d)\n", cells);
	else
		ret = 0;
	...
	if (of_device_is_compatible(dev->of_node, "nvidia,tegra264-smmu")) {
		tegra_cmdqv_dt_probe(dev->of_node, smmu);
		static_branch_enable(&arm_smmu_erratum_repeat_tlbi_cfgi_key);
	}

	return ret;
}

>  
> +	/* Optional clocks: keep them running for as long as we are bound */
> +	ret = devm_clk_bulk_get_all_enabled(dev, &clks);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to enable clocks\n");
> +
>  	smmu = arm_smmu_impl_probe(smmu);

[Severity: Medium]
Does returning here when devm_clk_bulk_get_all_enabled() fails leak the
device reference on smmu->impl_dev?

Earlier in arm_smmu_device_probe(), arm_smmu_device_dt_probe() (via
tegra_cmdqv_dt_probe()) and arm_smmu_device_acpi_probe() (via
acpi_smmu_dsdt_probe_tegra241_cmdqv()) acquire a reference on smmu->impl_dev
and rely on arm_smmu_impl_probe() -> tegra241_cmdqv_probe() to release it.
If devm_clk_bulk_get_all_enabled() fails, such as with -EPROBE_DEFER,
returning before arm_smmu_impl_probe() leaks that reference on each probe
attempt.

[1] https://sashiko.dev/#/bug/linux-084125c9-12e1-45a7-ad09-6882ddb42c3c

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-smmu-v3-pcie-pd-v1-0-628819ca44b9@gmail.com?part=3