Re: [PATCH RFC v7 5/9] PM / devfreq: Add new track_remote flag for governors

[sashiko-bot@kernel.org]

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

New issues:
- [High] Bypassing `profile->target()` for `TRACK_REMOTE` governors leaves `new_freq` unaligned to exact OPP frequencies, permanently breaking transition statistics and causing a warning flood.

Pre-existing issues:
- [High] Unprotected read of `devfreq->governor` can lead to a NULL pointer dereference or use-after-free due to concurrent modification.
- [High] The resume_freq state variable is incorrectly overwritten with suspend_freq during suspend transitions, preventing the device from properly restoring its pre-suspend frequency on resume.
--

commit fa5d93fe675fe1183dc1fbfcc716761f517b7cf0
Author: Sibi Sankar <sibi.sankar@oss.qualcomm.com>

PM / devfreq: Add new track_remote flag for governors

Some devfreq governors need to track frequency changes performed on remote
devices rather than driving the frequency updates themselves. In such cases,
the device's frequency is already updated by an external entity and devfreq
only needs to keep its transition statistics and notifier subscribers in sync.

> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2f27a239e34a6..71c6ced9e4e1d 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -348,6 +348,26 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>  	unsigned long cur_freq;
>  	int err = 0;
>  
> +	/*
> +	 * When a remote agent (e.g. firmware) owns the frequency, skip the
> +	 * local profile->target() call. Still emit the PRECHANGE/POSTCHANGE
> +	 * pair and the trace event so that transition-notifier subscribers
> +	 * (passive governor, devfreq cooling, etc.) and tracing observe the
> +	 * frequency change exactly as they would on the normal path.
> +	 */
> +	if (devfreq->governor &&
> +	    IS_SUPPORTED_FLAG(devfreq->governor->flags, TRACK_REMOTE)) {

[Severity: High]
This is a pre-existing issue, but does this evaluation of devfreq->governor
risk a NULL pointer dereference in drivers/devfreq/devfreq.c?

If governor_store() concurrently sets devfreq->governor to NULL,
devfreq->governor could be evaluated as true for the check, but then become
NULL when dereferencing devfreq->governor->flags in devfreq_set_target().

> +		freqs.old = devfreq->previous_freq;
> +		freqs.new = new_freq;
> +		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
> +
> +		if (trace_devfreq_frequency_enabled() && new_freq != freqs.old)
> +			trace_devfreq_frequency(devfreq, new_freq, freqs.old);
> +
> +		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
> +		goto update_status;

[Severity: High]
Will bypassing profile->target() cause transition statistics to permanently
break due to unaligned frequencies?

In drivers/devfreq/devfreq.c:devfreq_update_target(), the frequency is
clamped against PM QoS bounds before calling devfreq_set_target():

	if (freq < min_freq) {
		freq = min_freq;
		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
	}
	if (freq > max_freq) {
		freq = max_freq;
		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
	}

Since we skip profile->target() here, unaligned PM QoS values are passed
directly to devfreq_update_status(). Inside devfreq_update_status(),
devfreq_get_freq_level() attempts to find an exact match in the OPP table:

	lev = devfreq_get_freq_level(devfreq, freq);
	if (lev < 0) {
		ret = lev;
		goto out;
	}

This mismatch causes devfreq_update_status() to return an error, triggering a
warning flood. Further, previous_freq will be updated to this invalid
frequency, causing all future status updates to fail the prev_lev check.

> +	}
> +
>  	if (devfreq->profile->get_cur_freq)

[ ... ]

> @@ -375,6 +395,7 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>  	freqs.new = new_freq;
>  	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>  
> +update_status:
>  	if (devfreq_update_status(devfreq, new_freq))
>  		dev_warn(&devfreq->dev,
>  			 "Couldn't update frequency transition information.\n");

[Severity: High]
This isn't a bug introduced by this patch, but does the following code at the
end of devfreq_set_target() incorrectly overwrite the resume frequency during
suspend?

	if (devfreq->suspend_freq)
		devfreq->resume_freq = new_freq;

When devfreq_suspend_device() in drivers/devfreq/devfreq.c is called, it
explicitly initiates a transition to suspend_freq:

	if (devfreq->suspend_freq) {
		mutex_lock(&devfreq->lock);
		ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0);

This overwrites devfreq->resume_freq with the suspend_freq, which prevents the
device from restoring its actual active pre-suspend frequency on resume.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-rfc_v7_scmi_memlat-v7-0-f3f68c608f25@oss.qualcomm.com?part=5