| 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
| ||
|
Message-ID: <Z5K2mdin7hyPXIIP@arm.com>
Date: Thu, 23 Jan 2025 22:37:29 +0100
From: Beata Michalska <beata.michalska@....com>
To: Vanshidhar Konda <vanshikonda@...amperecomputing.com>
Cc: Viresh Kumar <viresh.kumar@...aro.org>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org,
ionela.voinescu@....com, sudeep.holla@....com, will@...nel.org,
catalin.marinas@....com, rafael@...nel.org, sumitg@...dia.com,
yang@...amperecomputing.com, lihuisong@...wei.com,
zhanjie9@...ilicon.com
Subject: Re: [PATCH v9 1/5] cpufreq: Allow arch_freq_get_on_cpu to return an
error
On Tue, Jan 21, 2025 at 10:40:55AM -0800, Vanshidhar Konda wrote:
> On Tue, Jan 21, 2025 at 04:14:32PM +0100, Beata Michalska wrote:
> > On Tue, Jan 21, 2025 at 04:17:06PM +0530, Viresh Kumar wrote:
> > > On 21-01-25, 08:44, Beata Michalska wrote:
> > > > Allow arch_freq_get_on_cpu to return an error for cases when retrieving
> > > > current CPU frequency is not possible, whether that being due to lack of
> > > > required arch support or due to other circumstances when the current
> > > > frequency cannot be determined at given point of time.
> > > >
> > > > Signed-off-by: Beata Michalska <beata.michalska@....com>
> > > > ---
> > > > arch/x86/kernel/cpu/aperfmperf.c | 2 +-
> > > > arch/x86/kernel/cpu/proc.c | 7 +++++--
> > > > drivers/cpufreq/cpufreq.c | 8 ++++----
> > > > include/linux/cpufreq.h | 2 +-
> > > > 4 files changed, 11 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kernel/cpu/aperfmperf.c b/arch/x86/kernel/cpu/aperfmperf.c
> > > > index f642de2ebdac..6cf31a1649c4 100644
> > > > --- a/arch/x86/kernel/cpu/aperfmperf.c
> > > > +++ b/arch/x86/kernel/cpu/aperfmperf.c
> > > > @@ -498,7 +498,7 @@ void arch_scale_freq_tick(void)
> > > > */
> > > > #define MAX_SAMPLE_AGE ((unsigned long)HZ / 50)
> > > >
> > > > -unsigned int arch_freq_get_on_cpu(int cpu)
> > > > +int arch_freq_get_on_cpu(int cpu)
> > > > {
> > > > struct aperfmperf *s = per_cpu_ptr(&cpu_samples, cpu);
> > > > unsigned int seq, freq;
> > > > diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> > > > index 41ed01f46bd9..d79f5845a463 100644
> > > > --- a/arch/x86/kernel/cpu/proc.c
> > > > +++ b/arch/x86/kernel/cpu/proc.c
> > > > @@ -86,9 +86,12 @@ static int show_cpuinfo(struct seq_file *m, void *v)
> > > > seq_printf(m, "microcode\t: 0x%x\n", c->microcode);
> > > >
> > > > if (cpu_has(c, X86_FEATURE_TSC)) {
> > > > - unsigned int freq = arch_freq_get_on_cpu(cpu);
> > > > + int freq = arch_freq_get_on_cpu(cpu);
> > > >
> > > > - seq_printf(m, "cpu MHz\t\t: %u.%03u\n", freq / 1000, (freq % 1000));
> > > > + if (freq <= 0)
> > > > + seq_puts(m, "cpu MHz\t\t: Unknown\n");
> > > > + else
> > > > + seq_printf(m, "cpu MHz\t\t: %u.%03u\n", freq / 1000, (freq % 1000));
> > > > }
> > > >
> > > > /* Cache size */
> > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > index 418236fef172..6f45684483c4 100644
> > > > --- a/drivers/cpufreq/cpufreq.c
> > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > @@ -728,18 +728,18 @@ show_one(cpuinfo_transition_latency, cpuinfo.transition_latency);
> > > > show_one(scaling_min_freq, min);
> > > > show_one(scaling_max_freq, max);
> > > >
> > > > -__weak unsigned int arch_freq_get_on_cpu(int cpu)
> > > > +__weak int arch_freq_get_on_cpu(int cpu)
> > > > {
> > > > - return 0;
> > > > + return -EOPNOTSUPP;
> > > > }
> > > >
> > > > static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
> > > > {
> > > > ssize_t ret;
> > > > - unsigned int freq;
> > > > + int freq;
> > > >
> > > > freq = arch_freq_get_on_cpu(policy->cpu);
> > > > - if (freq)
> > > > + if (freq > 0)
> > >
> > > >= ?
> > >
> > > Since we can return error now, 0 should be considered a valid
> > > frequency value ?
> > Theoretically speaking - it should, though what would 0 actually
> > represent then ?
> I would think the value of 0 would be valid and should be interpreted in a
> product/architecture specific manner? From silicon behavior, to me only negative
> frequency values wouldn't make any sense.
Still not convinced as of when '0' could represent a valid frequency and what
that one would actually imply but I do not have that strong of an opinion here.
>
> In Patch 1 of this series we interpret 0 as "Unknown" on a x86 system though. So
> for the sake of consistency should we consider 0 a valid value everywhere?
Yes, but on the other hand, showing 'Unknown' could be skipped entirely here as
the arch_freq_get_on_cpu for x86 is always providing a 'valid' frequency.
The change here was just to make things somewhat sane.
Note that the new attribute introduced in the following patch will utilise
potential error values from arch_freq_get_on_cpu though.
---
BR
Beata
>
> Regards,
> Vanshi
>
> >
> > ---
> > BR
> > Beata
> > >
> > > > ret = sysfs_emit(buf, "%u\n", freq);
> > > > else if (cpufreq_driver->setpolicy && cpufreq_driver->get)
> > > > ret = sysfs_emit(buf, "%u\n", cpufreq_driver->get(policy->cpu));
> > > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> > > > index 7fe0981a7e46..02fd4746231d 100644
> > > > --- a/include/linux/cpufreq.h
> > > > +++ b/include/linux/cpufreq.h
> > > > @@ -1184,7 +1184,7 @@ static inline int of_perf_domain_get_sharing_cpumask(int pcpu, const char *list_
> > > > }
> > > > #endif
> > > >
> > > > -extern unsigned int arch_freq_get_on_cpu(int cpu);
> > > > +extern int arch_freq_get_on_cpu(int cpu);
> > > >
> > > > #ifndef arch_set_freq_scale
> > > > static __always_inline
> > > > --
> > > > 2.25.1
> > >
> > > --
> > > viresh
Powered by blists - more mailing lists