Re: [PATCH v2 1/4] PM / devfreq: Fall back to previous_freq when get_cur_freq() fails

[Choi Chanwoo <cwchoi00@gmail.com>]

On 9/8/2026 4:21 AM, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> devfreq_set_target() calls the optional ->get_cur_freq() callback to get
> the frequency that is passed as freqs.old to the DEVFREQ_PRECHANGE and
> DEVFREQ_POSTCHANGE notifiers, but it does not check the return value. If
> the callback fails without setting @freq, cur_freq is never assigned, and
> an uninitialized stack value is passed to the notifiers.
> 
> hisi_uncore_get_cur_freq() can hit this. It returns -ENODEV without
> setting @freq when its PCC channel is missing. On the mailbox error path
> it sets @freq to 0 instead, so that the core does not read a random
> value.
> 
> The other two callers, cur_freq_show() and devfreq_monitor_resume(),
> already check the return value and use devfreq->previous_freq when the
> callback fails. Do the same in devfreq_set_target().
> 
> This does not seem to cause a visible problem today. The passive governor
> is the only DEVFREQ_TRANSITION_NOTIFIER user in the tree, and it only
> reads freqs.new. So this patch is not marked for stable.
> 
> Fixes: 0fe3a66410a3 ("PM / devfreq: Add new DEVFREQ_TRANSITION_NOTIFIER notifier")
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Avri Altman <avri.altman@sandisk.com>
> ---
>  drivers/devfreq/devfreq.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index f08fc6966eae..f20d9a660779 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -348,9 +348,8 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>  	unsigned long cur_freq;
>  	int err = 0;
>  
> -	if (devfreq->profile->get_cur_freq)
> -		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
> -	else
> +	if (!devfreq->profile->get_cur_freq ||
> +	    devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq))
>  		cur_freq = devfreq->previous_freq;
>  
>  	freqs.old = cur_freq;

I agree this patch.
But, I don't recommend to execute to function on if statement.
In order to improve the readability, could you please update it as following:

        unsigned long cur_freq = devfreq->previous_freq;
	unsigned long freq;
	...
	if (devfreq->profile->get_cur_freq) {
		err = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
		if (!err)
			cur_freq = freq;
	}

-- 
Best Regards,
Chanwoo Choi
Principal Engineer, Samsung Electronics