[PATCH v9 1/5] cpufreq: Allow arch_freq_get_on_cpu to return an error

Vanshidhar Konda vanshikonda at os.amperecomputing.com
Tue Jan 21 10:40:55 PST 2025


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 at arm.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.

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?

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



More information about the linux-arm-kernel mailing list