Re: [PATCH v2 7/9] i2c: qcom-geni: Vote for SE clock rate using OPP

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

New issues:
- [High] OPP performance state vote is leaked in geni_i2c_runtime_resume() if the device fails to power on.

Pre-existing issues:
- [High] Missing pm_runtime_put on error path when set_rate fails in geni_i2c_xfer().
--

commit 4acc892f1fa53a19ee146b34812667642ea2545f
Author: Praveen Talari <[email protected]>

i2c: qcom-geni: Vote for SE clock rate using OPP

This commit adds the source clock index and frequency to the I2C clock map
entries and uses them while configuring the controller. It requests the
required source clock rate through dev_pm_opp_set_rate() and programs
SE_GENI_CLK_SEL with the matching source index.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 658636c1ee0e2..a3b7609e3cf95 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -218,8 +224,13 @@ static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
>  	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>  	u32 val;
> +	int ret;
> +
> +	ret = dev_pm_opp_set_rate(se->dev, itr->src_clk_freq);
> +	if (ret)
> +		return ret;
>  

[Severity: High]
This is a pre-existing issue, but does returning an error here leak the
runtime PM reference in geni_i2c_xfer()?

Looking at geni_i2c_xfer(), it calls pm_runtime_get_sync() and then calls
this configuration function:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_xfer() {
    ...
    pm_runtime_get_sync(gi2c->se.dev);
    ...
    ret = qcom_geni_i2c_conf(&gi2c->se);
    if (ret)
        return ret;
    ...
}

If dev_pm_opp_set_rate() fails here, does geni_i2c_xfer() return directly
without dropping the runtime PM usage counter, permanently preventing the
device from entering runtime suspend?

[ ... ]

> @@ -1236,6 +1249,12 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>  	int ret = 0;
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>  
> +	if (gi2c->clk_fld && gi2c->clk_fld->src_clk_freq) {
> +		ret = dev_pm_opp_set_rate(dev, gi2c->clk_fld->src_clk_freq);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (gi2c->dev_data->power_on) {
>  		ret = gi2c->dev_data->power_on(&gi2c->se);
>  		if (ret)

[Severity: High]
If gi2c->dev_data->power_on() fails here, does this leak the OPP performance
state vote?

Since dev_pm_opp_set_rate() was successfully called just above, if the
power-on fails and returns early, it skips calling dev_pm_opp_set_rate(dev, 0)
to cancel the earlier rate vote.

Will this leave the power domain artificially elevated indefinitely, since the
PM core considers the device suspended due to the resume failure and will not
invoke the runtime_suspend callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-derive_clk_perf_tbl_from_perf_domain_opp_table-v2-0-091697dbeb02@oss.qualcomm.com?part=7