Re: [PATCH v2 4/9] soc: qcom: geni-se: Remove OPP rate reset from resource deactivation

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

New issues:
- [Low] Stale docstring in geni_se_resources_activate() referencing removed behavior of geni_se_resources_deactivate()

Pre-existing issues:
- [High] Teardown asymmetry and broken error path in geni_se_resources_deactivate() leading to unclocked access crashes.
--

commit b2f71df99b253ff18b7c7e30383a3f897e2d8b04
Author: Praveen Talari <[email protected]>

soc: qcom: geni-se: Remove OPP rate reset from resource deactivation

Remove the dev_pm_opp_set_rate() call from geni_se_resources_deactivate().
OPP resource management should be controlled by the consumer driver, and
the clocks are already disabled making the rate reset unnecessary.

> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 04f58a6f26fff..d4f0544d73c49 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -1039,8 +1039,8 @@ EXPORT_SYMBOL_GPL(geni_icc_disable);
>   * geni_se_resources_deactivate() - Deactivate GENI SE device resources
>   * @se: Pointer to the geni_se structure
>   *
> - * Deactivates device resources for power saving: OPP rate to 0, pin control
> - * to sleep state, turns off clocks, and disables interconnect. Skips ACPI devices.
> + * Deactivates device resources for power saving: pin control to sleep
> + * state, turns off clocks, and disables interconnect. Skips ACPI devices.
>   *
>   * Return: 0 on success, negative error code on failure
>   */
> @@ -1051,9 +1051,6 @@ int geni_se_resources_deactivate(struct geni_se *se)
>  	if (has_acpi_companion(se->dev))
>  		return 0;
>  
> -	if (se->has_opp)
> -		dev_pm_opp_set_rate(se->dev, 0);
> -

[Severity: Low]
Does this make the docstring in geni_se_resources_activate() stale?

The docstring for geni_se_resources_activate() says:

 * Unlike geni_se_resources_deactivate(), this function doesn't alter the
 * connected genpds' performance states, which must be additionally handled.

Since the dev_pm_opp_set_rate() call is removed here, is there still an
asymmetry between these two functions?

[Severity: High]
This is a pre-existing issue, but can the error path in
geni_se_resources_deactivate() lead to system crashes?

Looking at the rest of geni_se_resources_deactivate() below this change:

	geni_se_clks_off(se);

	return geni_icc_disable(se);

If geni_icc_disable() fails (for instance, a timeout in icc_set_bw()), it
returns an error to the caller after geni_se_clks_off() has already turned
off the clocks.

Because the PM core sees an error returned from the deactivation, it aborts
suspend and keeps the device in an active state. Later register accesses
bypass runtime resume, and since the clocks are actually disabled, this
triggers an immediate synchronous external abort.

Also, within geni_icc_disable():

	ret = icc_disable(se->icc_paths[i].path);
	if (ret) {
		dev_err_ratelimited(se->dev, "ICC disable failed on path '%s': %d\n",
				icc_path_names[i], ret);
		return ret;
	}

If one path fails to disable, it returns immediately without re-enabling
previously disabled interconnect paths, compounding the partial teardown
state.

>  	ret = pinctrl_pm_select_sleep_state(se->dev);
>  	if (ret)
>  		return ret;

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