Re: [PATCH v2 2/3] soc: qcom: spm: Add MSM8960 SAW2 CPU support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unconditional division by zero during regulator registration
- [High] Infinite polling loop in hardware register write sequence
--

commit 17a126a384bb63507998b0691e4ff49e0de54dab
Author: Rudraksha Gupta <guptarud@gmail.com>

soc: qcom: spm: Add MSM8960 SAW2 CPU support

This commit adds SPM register data for the MSM8960 CPU. It configures the SPM
sequences, PMIC delays, and regulator ranges to support the SAW2 v1.1 hardware
present in this SoC.

Link: https://github.com/CyanogenMod/android_kernel_samsung_d2/blob/0dbe2b56847b304d30b809dfd08ba3b4a61d9af8/arch/arm/mach-msm/board-express.c#L3265-L3285
Link: https://github.com/CyanogenMod/android_kernel_samsung_d2/blob/0dbe2b56847b304d30b809dfd08ba3b4a61d9af8/arch/arm/mach-msm/board-express.c#L3353-L3381

> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> index f75659fff2874..fb3ec8aa42b07 100644
> --- a/drivers/soc/qcom/spm.c
> +++ b/drivers/soc/qcom/spm.c
> @@ -253,6 +253,22 @@ static const struct spm_reg_data spm_reg_8064_cpu = {
>  	.ramp_delay = 1250,
>  };
>  
> +static const struct spm_reg_data spm_reg_8960_cpu = {
> +	.reg_offset = spm_reg_offset_v1_1,

[Severity: High]
This isn't a regression introduced by this patch, but registering these
offsets enables a code path with an unbounded polling loop.

During probe, setting up the default low power mode eventually calls
spm_register_write_sync():

drivers/soc/qcom/spm.c:spm_register_write_sync() {
	...
	do {
		writel_relaxed(val, drv->reg_base +
				drv->reg_data->reg_offset[reg]);
		ret = readl_relaxed(drv->reg_base +
				drv->reg_data->reg_offset[reg]);
		if (ret == val)
			break;
		cpu_relax();
	} while (1);
}

If the hardware is in an invalid state, or if certain register bits are
read-only and fail to reflect the written toggle, could this loop hang
indefinitely?

Would it be safer to use a proper timeout mechanism like
read_poll_timeout_atomic() here to allow for graceful recovery?

> +	.spm_cfg = 0x1f,
> +	.pmic_dly = 0x03020004,
> +	.pmic_data[0] = 0x0084009c,
> +	.pmic_data[1] = 0x00a4001c,
> +	.seq = { 0x03, 0x0f, 0x00, 0x24, 0x54, 0x10, 0x09, 0x03, 0x01,
> +		0x10, 0x54, 0x30, 0x0c, 0x24, 0x30, 0x0f },
> +	.start_index[PM_SLEEP_MODE_STBY] = 0,
> +	.start_index[PM_SLEEP_MODE_SPC] = 2,
> +	.set_vdd = smp_set_vdd_v1_1,
> +	.range = &spm_v1_1_regulator_range,
> +	.init_uV = 1300000,

[Severity: Critical]
This is a pre-existing issue, but I noticed that setting init_uV here
triggers a division by zero during probe.

When spm_register_regulator() allocates rdesc via devm_kzalloc(),
rdesc->uV_step is zero-initialized and never explicitly set. Later, it
performs this calculation:

drivers/soc/qcom/spm.c:spm_register_regulator() {
	...
	drv->volt_sel = DIV_ROUND_UP(drv->reg_data->init_uV - rdesc->min_uV,
				     rdesc->uV_step);
	...
}

Will this result in an unconditional division by zero when calculating
drv->volt_sel?

> +	.ramp_delay = 1250,
> +};
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-expressatt_cpufreq-v2-0-b9b7726ccb6d@gmail.com?part=2