Re: [PATCH v4 2/2] cpufreq: qcom: Add cpufreq scaling support for Qualcomm Shikra SoC

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Calling kfree() on an interior pointer of a devm-managed array causes severe memory corruption.
- [High] The turbo/boost frequency is improperly ignored and left invalid when the hardware LUT is fully populated.
- [High] The LMh throttle delayed work can outlive the `qcom_cpufreq_data` structure, causing a Use-After-Free when the policy is destroyed.
--

commit 04a4ff128bbeeee8119bf80c19ea931913c35395
Author: Taniya Das <taniya.das@oss.qualcomm.com>

cpufreq: qcom: Add cpufreq scaling support for Qualcomm Shikra SoC

This commit adds cpufreq scaling support for the Qualcomm Shikra SoC. It
introduces the shikra_epss_soc_data structure which reuses the EPSS
configuration but limits the maximum lookup table entries to 12.

> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index ea9a20d27b8fd..3d5a865fb8a35 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c

[ ... ]

> @@ -236,7 +238,7 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
>  		icc_scaling_enabled = false;
>  	}
>  
> -	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +	for (i = 0; i < soc_data->lut_max_entries; i++) {
>  		data = readl_relaxed(drv_data->base + soc_data->reg_freq_lut +
>  				      i * soc_data->lut_row_size);
>  		src = FIELD_GET(LUT_SRC, data);

[Severity: High]
This is a pre-existing issue, but does this loop handle a fully populated LUT
where the last entry is a turbo frequency?

When a frequency has a core_count of LUT_TURBO_IND, it is initially marked as
CPUFREQ_ENTRY_INVALID. The code relies on encountering a duplicate frequency
on the next iteration to convert that invalid entry into CPUFREQ_BOOST_FREQ.

If the hardware LUT is completely filled with unique entries up to its
capacity limit, the loop terminates naturally. Does this leave the final
turbo entry ignored and invalid since the duplicate check is never reached?

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but could the teardown path free a devm-managed
interior pointer?

During policy initialization, the driver assigns policy->driver_data to point
to an interior element within a collectively allocated devm-managed array:

qcom_cpufreq_hw_cpu_init()
    policy->driver_data = &qcom_cpufreq.data[index];

During teardown, the driver appears to manually free this pointer:

qcom_cpufreq_hw_cpu_exit()
    kfree(data);

Can this manual kfree() on an interior pointer bypass devres management and
cause memory issues during device unbind or CPU hotplug?

[Severity: High]
This is a pre-existing issue, but is there a race between the LMh throttle
delayed work and the CPU offline sequence?

In qcom_cpufreq_hw_cpu_offline(), the driver cancels the work before
disabling the interrupt:

qcom_cpufreq_hw_cpu_offline()
    cancel_delayed_work_sync(&data->throttle_work);
    disable_irq_nosync();

If the hardware interrupt fires during this window, the interrupt handler
qcom_lmh_dcvs_handle_irq() unconditionally re-arms the delayed work. 
Later, when qcom_cpufreq_hw_cpu_exit() frees the data structure, the re-armed
work could still execute and attempt to access the freed memory:

qcom_lmh_dcvs_poll()
    mutex_lock(&data->throttle_lock);

Could the driver disable the IRQ before canceling the work to ensure the
work is not re-armed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-shikra-cpufreq-scaling-v4-0-f042a25896c5@oss.qualcomm.com?part=2