linux-kernel - Re: [PATCH v2 2/2] cpufreq: Wire-up arch-flavored freq info into cpufreq_verify_current

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZW8D5TfSwuJfdYeD@arm.com>
Date:   Tue, 5 Dec 2023 11:05:09 +0000
From:   Ionela Voinescu <ionela.voinescu@....com>
To:     Sumit Gupta <sumitg@...dia.com>
Cc:     Beata Michalska <beata.michalska@....com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-pm@...r.kernel.org, sudeep.holla@....covm, will@...nel.org,
        catalin.marinas@....com, viresh.kumar@...aro.org,
        rafael@...nel.org, yang@...amperecomputing.com,
        linux-tegra@...r.kernel.org
Subject: Re: [PATCH v2 2/2] cpufreq: Wire-up arch-flavored freq info into
 cpufreq_verify_current_freq

Hi Sumit,

On Friday 01 Dec 2023 at 18:32:10 (+0530), Sumit Gupta wrote:
> Hi Ionela,
> 
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -1756,7 +1756,8 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
> > >   {
> > >        unsigned int new_freq;
> > > 
> > > -     new_freq = cpufreq_driver->get(policy->cpu);
> > > +     new_freq = arch_freq_get_on_cpu(policy->cpu);
> > > +     new_freq = new_freq ?: cpufreq_driver->get(policy->cpu);
> > 
> > Given that arch_freq_get_on_cpu() is an average frequency, it does not
> > seem right to me to trigger the sync & update process of
> > cpufreq_verify_current_freq() based on it.
> > 
> > cpufreq_verify_current_freq() will at least modify the internal state of
> > the policy and send PRE and POST notifications, if not do a full frequency
> > update, based on this average frequency, which is likely different from
> > the current frequency, even beyond the 1MHz threshold.
> > 
> > While I believe it's okay to return this average frequency in
> > cpuinfo_cur_freq, I don't think it should be used as an indication of
> > an accurate current frequency, which is what
> > cpufreq_verify_current_freq() expects.
> > 
> > Sumit, can you give more details on the issue at [1] and why this change
> > fixes it?
> > 
> > [1] https://lore.kernel.org/lkml/6a5710f6-bfbb-5dfd-11cd-0cd02220cee7@nvidia.com/
> > 
> > Thank you,
> > Ionela.
> > 
> cpufreq_verify_current_freq() also updates 'policy->cur' in POST
> notification if the frequency from hardware has more delta (out of sync).
> 
> As the value from 'cpufreq_driver->get()' is not reliable due to [1],
> calling the 'get' hook can update the 'policy->cur' with a wrong value when
> governor starts in cpufreq_start_governor().
> And if the frequency is never changed after the governor starts during
> boot e.g. when performance governor is set as default, then
> 'scaling_cur_freq' always returns wrong value.
> 
> Instead, the arch_freq_get_on_cpu() API updates 'policy->cur' with a more
> stable freq value.
> 
> [1] https://lore.kernel.org/lkml/20230418113459.12860-7-sumitg@nvidia.com/

Got it, many thanks! 

As the code is right now in v2, arch_freq_get_on_cpu() is called on
show_scaling_cur_freq(), so the problem you describe would not show up.
policy->cur would still be incorrect, but 'scaling_cur_freq' would
return the value from arch_freq_get_on_cpu().

Would it be enough if arch_freq_get_on_cpu() gets also called from
show_cpuinfo_cur_freq() instead of cpufreq_verify_current_freq()?

Thanks,
Ionela.

> 
> Best regards,
> Sumit Gupta
> 
> > >        if (!new_freq)
> > >                return 0;
> > > 
> > > --
> > > 2.25.1
> > >