| Message ID | 20250121084435.2839280-2-beata.michalska@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for AArch64 AMUv1-based average freq | expand |
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@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 ? > 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
On 21-01-2025 14:14, 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@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) > 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 Looks good to me. Reviewed-by: Prasanna Kumar T S M <ptsm@linux.microsoft.com>
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@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 ? --- 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
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@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
On 21-01-25, 16:14, Beata Michalska wrote: > Theoretically speaking - it should, though what would 0 actually > represent then ? 0 won't be a failure, that's clear, since errors are represented differently now. I am not sure what 0 frequency would mean and it can be left as an architecture specific value, which is a corner case I am not sure will ever occur.
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@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. 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
On Wed, Jan 22, 2025 at 11:42:50AM +0530, Viresh Kumar wrote: > On 21-01-25, 16:14, Beata Michalska wrote: > > Theoretically speaking - it should, though what would 0 actually > > represent then ? > > 0 won't be a failure, that's clear, since errors are represented > differently now. I am not sure what 0 frequency would mean and it can > be left as an architecture specific value, which is a corner case I am > not sure will ever occur. That would mean we are opting for presenting '0' value (whatever that means) instead of trying alternative ways of getting 'current' frequency ? This is still the scaling_cur_freq. --- BR Beata > > -- > viresh
On 23-01-25, 22:45, Beata Michalska wrote: > That would mean we are opting for presenting '0' value (whatever that means) > instead of trying alternative ways of getting 'current' frequency ? > This is still the scaling_cur_freq. A return value of 0 should typically mean something went wrong somewhere and didn't return the right value to us. - For the print message, I think we should just print the value instead of UNKNOWN. Let the user / developer decide what to do with it. - As for trying other mechanism to find the frequency now, maybe you are right and looking for an alternate way is the right way to go. And that would be consistent with existing behavior too.
On 21-01-2025 14:14, 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@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) > 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 Looks good to me. Reviewed-by: Prasanna Kumar T S M <ptsm@linux.microsoft.com>
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) 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
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@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(-)