Re: [PATCH v3 3/4] spi: qcom-geni: Use resources helper APIs in runtime PM functions

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing rollback of activated resources when dev_pm_opp_set_rate() fails during runtime resume.
- [High] Dropping performance state vote before fallible operations leaves the device broken on suspend failure.
- [High] System crash due to unclocked register access when pm_runtime_get_sync() fails in spi_geni_init().
- [High] Silent failure in packing configuration for bits_per_word < 8 leads to inconsistent hardware state and data corruption.
- [High] Asynchronous pm_runtime_put() in probe error path leaves hardware active.
- [Low] Unconditional assignment of dev_pm_ops struct creates dead code when PM is disabled.
--

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

spi: qcom-geni: Use resources helper APIs in runtime PM functions

This commit introduces the geni_se_resources_activate() and
geni_se_resources_deactivate() helper APIs. These functions reduce
code duplication during runtime power management. The drivers now use
a streamlined method for ICC, clock, and SE resource operations.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 333216df922e8..4205349376955 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c

[ ... ]

> @@ -1137,15 +1129,14 @@ static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
>  	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
>  	int ret;
>  
> -	ret = geni_icc_enable(&mas->se);
> +	ret = geni_se_resources_activate(&mas->se);
>  	if (ret)
>  		return ret;
>  
> -	ret = geni_se_resources_on(&mas->se);
> -	if (ret)
> -		return ret;
> +	if (mas->se.has_opp)
> +		return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
>  
> -	return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
> +	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but if dev_pm_opp_set_rate() fails here, does
the function return an error without calling geni_se_resources_deactivate()
to roll back the resources enabled by geni_se_resources_activate()? Could this
cause a persistent leak of clock references and interconnect bandwidth?

There are also a few pre-existing issues in this file and related code that
are not directly introduced by this patch, but might be worth addressing:

drivers/soc/qcom/qcom-geni-se.c:geni_se_resources_deactivate() {
    ...
	if (se->has_opp)
		dev_pm_opp_set_rate(se->dev, 0);

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

[Severity: High]
This isn't a bug introduced by this patch, but dropping the OPP rate to 0
before the fallible pinctrl_pm_select_sleep_state() creates a window for state
inconsistency. If pinctrl fails and an error is returned, PM runtime will leave
the device marked as RPM_ACTIVE. Will subsequent accesses to the device be
unclocked, potentially leading to hardware hangs or asynchronous external aborts?

drivers/spi/spi-geni-qcom.c:spi_geni_driver {
    ...
	.driver = {
		.name = "geni_spi",
		.pm = &spi_geni_pm_ops,
		.of_match_table = spi_geni_dt_match,
	},
    ...
}

[Severity: Low]
This is a pre-existing issue, but does assigning &spi_geni_pm_ops unconditionally
to .pm produce dead code when CONFIG_PM is disabled? Should this be wrapped with
pm_ptr() to ensure the callbacks are properly conditionally compiled out?

drivers/spi/spi-geni-qcom.c:spi_geni_init() {
    ...
	pm_runtime_get_sync(mas->dev);

	proto = geni_se_read_proto(se);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the unchecked
pm_runtime_get_sync() allow execution to proceed to geni_se_read_proto() even
if it fails? Could this trigger an unclocked register access and cause an
asynchronous external abort?

drivers/soc/qcom/qcom-geni-se.c:geni_se_config_packing() {
    ...
	int iter = (ceil_bpw * pack_words) / BITS_PER_BYTE;
	int i;

	if (iter <= 0 || iter > NUM_PACKING_VECTORS)
		return;
    ...
}

[Severity: High]
This is a pre-existing issue, but for valid bits_per_word values less than 8,
doesn't iter exceed NUM_PACKING_VECTORS (4)? If the function silently returns
without writing to the SE_GENI_TX/RX_PACKING_CFGn registers, will the hardware
operate using stale packing configurations and potentially corrupt data?

drivers/spi/spi-geni-qcom.c:spi_geni_init() {
    ...
out_pm:
	pm_runtime_put(mas->dev);
	return ret;
}

[Severity: High]
This is a pre-existing issue, but doesn't the use of the asynchronous
pm_runtime_put() in the error path allow pm_runtime_disable() (called during
device removal or probe failure) to cancel the pending suspend? Does this leave
the hardware active and permanently leak clock and interconnect resources?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-enable-spi-on-sa8255p-v3-0-43984eac4c67@oss.qualcomm.com?part=3