Re: [PATCH v7 06/12] media: iris: Replace enum-indexed clock and power domain tables with per-block structures

[sashiko-bot@kernel.org]

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

New issues:
- [High] Uninitialized stack pointer dereference when a hardware block has 0 power domains.
- [High] Secondary hardware cores are left unpowered due to missing iteration over `num_cores`.
- [High] NULL pointer dereference when optional hardware blocks are enabled in hardware but missing from platform data.
- [High] Premature OPP voltage drop causes DVFS sequence violation.
- [High] Uninitialized fields in `struct dev_pm_domain_attach_data` result in arbitrary behavior.
- [Medium] API mismatch silently discards additional power domains if `pd_cnt > 1`.

Pre-existing issues:
- [High] Resource leak of Runtime PM usage counter and OPP rate on error path.
--

commit 569dbace99d0eb806398e44f1f3dd0f46e77e155
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Replace enum-indexed clock and power domain tables with per-block structures

This patch replaces flat, enum-indexed tables for clocks and power domains with 
per-hardware block structures in the iris driver. It groups devices, power 
domains, and clocks together logically in the iris_power_domain struct, and 
removes index lookups at runtime.

> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
> index c2dcb50a27824..681db1da9559a 100644
> --- a/drivers/media/platform/qcom/iris/iris_probe.c
> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
> @@ -39,15 +39,32 @@ static int iris_init_icc(struct iris_core *core)
>  	return devm_of_icc_bulk_get(core->dev, core->icc_count, core->icc_tbl);
>  }
>  
> -static int iris_init_power_domains(struct iris_core *core)
> +static int iris_init_power_domains_per_block(struct iris_core *core,
> +					     struct iris_power_domain *pd,
> +					     const struct iris_power_domain_data *pd_data)
>  {
> +	struct dev_pm_domain_attach_data iris_pd_data;

[Severity: High]
Does this leave fields in iris_pd_data uninitialized? Since it's allocated on the
stack without zero-initialization, any fields added to struct
dev_pm_domain_attach_data (like link_flags) could contain stack garbage when
passed to devm_pm_domain_attach_list().

> +	struct dev_pm_domain_list *pmdomain_tbl;
>  	int ret;
>  
> +	iris_pd_data.pd_names = pd_data->pd_names;
> +	iris_pd_data.num_pd_names = pd_data->pd_cnt;
> +	iris_pd_data.pd_flags = PD_FLAG_NO_DEV_LINK;
> +
> +	ret = devm_pm_domain_attach_list(core->dev, &iris_pd_data, &pmdomain_tbl);
> +	if (ret < 0)
> +		return ret;
> +
> +	pd->dev = pmdomain_tbl->pd_devs[0];

[Severity: High]
If pd_data->pd_cnt is 0, devm_pm_domain_attach_list() returns 0 but does not
modify pmdomain_tbl. Does this result in an uninitialized pointer dereference
when accessing pmdomain_tbl->pd_devs[0]?

[Severity: Medium]
Also, if the platform data specifies multiple power domains (pd_cnt > 1), does
this logic silently discard all but the first power domain? Since
PD_FLAG_NO_DEV_LINK is used, the driver has to power them on manually, which
means secondary domains might never be powered on.

> +
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c
> index 773f6548370a2..fdc6eb68e5c78 100644
> --- a/drivers/media/platform/qcom/iris/iris_resources.c
> +++ b/drivers/media/platform/qcom/iris/iris_resources.c
[ ... ]
> @@ -70,7 +69,7 @@ int iris_opp_set_rate(struct device *dev, unsigned long freq)
>  	return dev_pm_opp_set_opp(dev, opp);
>  }
>  
> -int iris_enable_power_domains(struct iris_core *core, struct device *pd_dev)
> +int iris_enable_power_domain_and_clocks(struct iris_core *core, struct iris_power_domain *pd)
>  {
>  	int ret;
>  
> @@ -78,66 +77,20 @@ int iris_enable_power_domains(struct iris_core *core, struct device *pd_dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = pm_runtime_get_sync(pd_dev);
> +	ret = pm_runtime_get_sync(pd->dev);
>  	if (ret < 0)
>  		return ret;
>  

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak the
runtime PM usage counter and leave the global core device forced to its highest
performance state? If pm_runtime_get_sync() fails, there is no
pm_runtime_put_noidle() or iris_opp_set_rate(core->dev, 0) called before
returning.

> -	return ret;
> -}
> -
> -int iris_disable_power_domains(struct iris_core *core, struct device *pd_dev)
> -{
> -	int ret;
> -
> -	ret = iris_opp_set_rate(core->dev, 0);
> +	ret = clk_bulk_prepare_enable(pd->clk_cnt, pd->clocks);
>  	if (ret)
> -		return ret;
> -
> -	pm_runtime_put_sync(pd_dev);
> -
> -	return 0;
> -}
> +		pm_runtime_put_sync(pd->dev);
>  
> -static struct clk *iris_get_clk_by_type(struct iris_core *core, enum platform_clk_type clk_type)
> -{
[ ... ]
> +	return ret;
>  }
>  
> -int iris_disable_unprepare_clock(struct iris_core *core, enum platform_clk_type clk_type)
> +void iris_disable_power_domain_and_clocks(struct iris_core *core, struct iris_power_domain *pd)
>  {
> -	struct clk *clock;
> -
> -	clock = iris_get_clk_by_type(core, clk_type);
> -	if (!clock)
> -		return -EINVAL;
> -
> -	clk_disable_unprepare(clock);
> -
> -	return 0;
> +	clk_bulk_disable_unprepare(pd->clk_cnt, pd->clocks);
> +	iris_opp_set_rate(core->dev, 0);
> +	pm_runtime_put_sync(pd->dev);
>  }

[Severity: High]
Could dropping the OPP rate to 0 inside a block-specific helper cause issues?
If multiple blocks are being torn down (for example, in
iris_vpu4x_power_off_hardware() which shuts down VPP1, VPP0, then VCODEC), this
would instantly drop the global controller OPP to 0 when the first block is
disabled, while other blocks' clocks are still running. Does this violate DVFS
constraints?

[ ... ]
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu4x.c b/drivers/media/platform/qcom/iris/iris_vpu4x.c
> index 02e100a4045fc..0feb5eaa544f9 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu4x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu4x.c
[ ... ]
> @@ -158,116 +136,43 @@ static void iris_vpu4x_ahb_sync_reset_hardware(struct iris_core *core)
>  	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>  }
>  
> -static int iris_vpu4x_enable_hardware_clocks(struct iris_core *core, u32 efuse_value)
> -{
[ ... ]
> -}
> -
>  static int iris_vpu4x_power_on_hardware(struct iris_core *core)
>  {
>  	u32 efuse_value = readl(core->reg_base + WRAPPER_EFUSE_MONITOR);
>  	int ret;
>  
> -	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> +	ret = iris_enable_power_domain_and_clocks(core, core->vcodec);
>  	if (ret)
>  		return ret;
>  
>  	if (!(efuse_value & DISABLE_VIDEO_VPP0_BIT)) {
> -		ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs
> -						[IRIS_VPP0_HW_POWER_DOMAIN]);
> +		ret = iris_enable_power_domain_and_clocks(core, core->vcodec_vpp0);

[Severity: High]
If the hardware efuse indicates VPP0 is present, but the platform data omits
vcodec_vpp0_data, core->vcodec_vpp0 will not be allocated during probe. Does
this result in a NULL pointer dereference here since
iris_enable_power_domain_and_clocks() accesses pd->dev?

>  		if (ret)
>  			goto disable_hw_power_domain;
>  	}
>  
[ ... ]
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index 23a7ce3ecb2d0..f35754ef53ed1 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
[ ... ]
>  int iris_vpu_power_on_hw(struct iris_core *core)
>  {
> -	int ret;
> -
> -	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> -	if (ret)
> -		return ret;
> -
> -	ret = iris_prepare_enable_clock(core, IRIS_HW_CLK);
> -	if (ret)
> -		goto err_disable_power;
> -
> -	ret = iris_prepare_enable_clock(core, IRIS_HW_AHB_CLK);
> -	if (ret && ret != -ENOENT)
> -		goto err_disable_hw_clock;
> -
> -	ret = iris_prepare_enable_clock(core, IRIS_BSE_HW_CLK);
> -	if (ret && ret != -ENOENT)
> -		goto err_disable_hw_ahb_clock;
> -
> -	return 0;
> -
> -err_disable_hw_ahb_clock:
> -	iris_disable_unprepare_clock(core, IRIS_HW_AHB_CLK);
> -err_disable_hw_clock:
> -	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
> -err_disable_power:
> -	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> -
> -	return ret;
> +	return iris_enable_power_domain_and_clocks(core, core->vcodec);
>  }

[Severity: High]
Does this fail to power on any secondary hardware cores? The probe function
dynamically allocates core->vcodec as an array based on num_cores, but this only
passes the pointer to the first element to
iris_enable_power_domain_and_clocks(), without iterating over the remaining
cores.

>  
>  int iris_vpu_set_hwmode(struct iris_core *core)
>  {
> -	return dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], true);
> +	return dev_pm_genpd_set_hwmode(core->vcodec->dev, true);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-glymur-v7-0-afaa55d11fe0@oss.qualcomm.com?part=6