Re: [PATCH v3 6/8] i2c: qcom-geni: Vote for SE clock rate using OPP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: qcom-geni: missing power_off rollback on opp_set_rate failure in resume
- [High] i2c: qcom-geni: PM state desync on opp_set_rate failure in suspend
- [High] i2c: PM desync in suspend causes unclocked access crash
--

commit e5bbe927d2b6dbf7ee2cdbdeb6104c05147c9959
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

i2c: qcom-geni: Vote for SE clock rate using OPP
    
The GENI I2C driver selects a source clock based on the requested bus
frequency and programs the corresponding clock selector. However, it
does not request the selected source clock rate when OPP-based clock
scaling is used.
    
Store the matched source clock frequency and vote for it through
dev_pm_opp_set_rate() before programming the clock selector. Also remove
and restore the vote during runtime suspend and resume.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 5f3d260a3913b..a6fe89d0934f6 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -255,6 +257,11 @@ 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, gi2c->src_freq);
> +	if (ret)
> +		return ret;
>  
>  	writel_relaxed(gi2c->clk_idx, gi2c->se.base + SE_GENI_CLK_SEL);

[Severity: High]
If dev_pm_opp_set_rate(dev, 0) fails during geni_i2c_runtime_suspend(), the
PM core will leave the device state as RPM_ACTIVE even though the physical
clocks were disabled.

On the next transfer, pm_runtime_resume_and_get() would see RPM_ACTIVE and
skip calling geni_i2c_runtime_resume(). This leads to qcom_geni_i2c_conf()
where dev_pm_opp_set_rate(dev, gi2c->src_freq) succeeds, but writel_relaxed()
is executed while the physical hardware is still unclocked.

Could this sequence trigger a fatal bus error?

[ ... ]

> @@ -1286,7 +1293,7 @@ static int geni_i2c_runtime_suspend(struct device *dev)
>  		}
>  	}
>  
> -	return 0;
> +	return dev_pm_opp_set_rate(dev, 0);

[Severity: High]
If dev_pm_opp_set_rate(dev, 0) fails here, the error is returned to the
runtime PM core. Does this leave the PM core thinking the device is RPM_ACTIVE
while the hardware clocks were already physically turned off by the preceding
gi2c->dev_data->power_off() call?

>  }
>  
>  static int geni_i2c_runtime_resume(struct device *dev)
> @@ -1300,6 +1307,12 @@ static int geni_i2c_runtime_resume(struct device *dev)
>  			return ret;
>  	}
>  
> +	if (gi2c->src_freq) {
> +		ret = dev_pm_opp_set_rate(dev, gi2c->src_freq);
> +		if (ret)
> +			return ret;

[Severity: High]
If dev_pm_opp_set_rate() fails, this returns immediately. Does this skip the
gi2c->dev_data->power_off() rollback and enable_irq() calls, leaving the
hardware resources enabled indefinitely while the PM core marks the device as
RPM_SUSPENDED?

> +	}
> +
>  	enable_irq(gi2c->irq);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=6