| 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: <CAJZ5v0gVNa3ARV58pvJmDxE7VUNZe6Bxh=FoPwJ-4=a136XUNA@mail.gmail.com>
Date: Fri, 26 Sep 2025 12:22:57 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Jie Zhan <zhanjie9@...ilicon.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux PM <linux-pm@...r.kernel.org>,
Mario Limonciello <mario.limonciello@....com>, Shawn Guo <shawnguo@...nel.org>,
Qais Yousef <qyousef@...alina.io>, LKML <linux-kernel@...r.kernel.org>,
Viresh Kumar <viresh.kumar@...aro.org>, Pierre Gondois <pierre.gondois@....com>,
Linux ACPI <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] ACPI: CPPC: Do not use CPUFREQ_ETERNAL as an error value
On Fri, Sep 26, 2025 at 11:30 AM Jie Zhan <zhanjie9@...ilicon.com> wrote:
>
>
>
> On 9/26/2025 1:23 AM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > Instead of using CPUFREQ_ETERNAL for signaling an error condition
> > in cppc_get_transition_latency(), change the return value type of
> > that function to int and make it return a proper negative error
> > code on failures.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> A minor comment inline. Other than that,
> Reviewed-by: Jie Zhan <zhanjie9@...ilicon.com>
Thanks!
> > ---
> >
> > v1 -> v2:
> > * Change cppc_get_transition_latency() return value data type to int
> > * Make it return -ENODATA on errors (Mario)
> > * Update its callers accordingly
> > * Adjust the subject and changelog
> > * Add a missing empty code line to cppc_get_transition_latency()
> >
> > The modifications of this patch don't affect any other patches in the series:
> >
> > https://lore.kernel.org/linux-pm/8605612.T7Z3S40VBb@rafael.j.wysocki/
> >
> > ---
> > drivers/acpi/cppc_acpi.c | 15 ++++++++-------
> > drivers/cpufreq/amd-pstate.c | 8 ++++----
> > drivers/cpufreq/cppc_cpufreq.c | 4 ++--
> > include/acpi/cppc_acpi.h | 6 +++---
> > 4 files changed, 17 insertions(+), 16 deletions(-)
> >
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -1876,7 +1876,7 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);
> > * If desired_reg is in the SystemMemory or SystemIo ACPI address space,
> > * then assume there is no latency.
> > */
> > -unsigned int cppc_get_transition_latency(int cpu_num)
> > +int cppc_get_transition_latency(int cpu_num)
> > {
> > /*
> > * Expected transition latency is based on the PCCT timing values
> > @@ -1889,31 +1889,32 @@ unsigned int cppc_get_transition_latency
> > * completion of a command before issuing the next command,
> > * in microseconds.
> > */
> > - unsigned int latency_ns = 0;
> > struct cpc_desc *cpc_desc;
> > struct cpc_register_resource *desired_reg;
> > int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu_num);
> > struct cppc_pcc_data *pcc_ss_data;
> > + int latency_ns = 0;
> >
> > cpc_desc = per_cpu(cpc_desc_ptr, cpu_num);
> > if (!cpc_desc)
> > - return CPUFREQ_ETERNAL;
> > + return -ENODATA;
> >
> > desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
> > if (CPC_IN_SYSTEM_MEMORY(desired_reg) || CPC_IN_SYSTEM_IO(desired_reg))
> > return 0;
> > +
> An extra line here.
Yes, and it is intentional (see the changes between v1 and v2).
> Alternatively, can we cleanup a bit:
That can be done.
> if (!CPC_IN_PCC(desired_reg) || pcc_ss_id < 0)
> return -ENODATA;
>
> > else if (!CPC_IN_PCC(desired_reg))
> > - return CPUFREQ_ETERNAL;
> > + return -ENODATA;
> >
> > if (pcc_ss_id < 0)
> > - return CPUFREQ_ETERNAL;
> > + return -ENODATA;
> >
> > pcc_ss_data = pcc_data[pcc_ss_id];
> > if (pcc_ss_data->pcc_mpar)
> > latency_ns = 60 * (1000 * 1000 * 1000 / pcc_ss_data->pcc_mpar);
> >
> > - latency_ns = max(latency_ns, pcc_ss_data->pcc_nominal * 1000);
> > - latency_ns = max(latency_ns, pcc_ss_data->pcc_mrtt * 1000);
> > + latency_ns = max_t(int, latency_ns, pcc_ss_data->pcc_nominal * 1000);
> > + latency_ns = max_t(int, latency_ns, pcc_ss_data->pcc_mrtt * 1000);
> >
> > return latency_ns;
> > }
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -872,10 +872,10 @@ static void amd_pstate_update_limits(str
> > */
> > static u32 amd_pstate_get_transition_delay_us(unsigned int cpu)
> > {
> > - u32 transition_delay_ns;
> > + int transition_delay_ns;
> >
> > transition_delay_ns = cppc_get_transition_latency(cpu);
> > - if (transition_delay_ns == CPUFREQ_ETERNAL) {
> > + if (transition_delay_ns < 0) {
> > if (cpu_feature_enabled(X86_FEATURE_AMD_FAST_CPPC))
> > return AMD_PSTATE_FAST_CPPC_TRANSITION_DELAY;
> > else
> > @@ -891,10 +891,10 @@ static u32 amd_pstate_get_transition_del
> > */
> > static u32 amd_pstate_get_transition_latency(unsigned int cpu)
> > {
> > - u32 transition_latency;
> > + int transition_latency;
> >
> > transition_latency = cppc_get_transition_latency(cpu);
> > - if (transition_latency == CPUFREQ_ETERNAL)
> > + if (transition_latency < 0)
> > return AMD_PSTATE_TRANSITION_LATENCY;
> >
> > return transition_latency;
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -310,9 +310,9 @@ static int cppc_verify_policy(struct cpu
> >
> > static unsigned int get_transition_latency_from_cppc(unsigned int cpu)
> > {
> > - unsigned int transition_latency_ns = cppc_get_transition_latency(cpu);
> > + int transition_latency_ns = cppc_get_transition_latency(cpu);
> >
> > - if (transition_latency_ns == CPUFREQ_ETERNAL)
> > + if (transition_latency_ns < 0)
> > return CPUFREQ_DEFAULT_TRANSITION_LATENCY_NS / NSEC_PER_USEC;
> >
> > return transition_latency_ns / NSEC_PER_USEC;
> > --- a/include/acpi/cppc_acpi.h
> > +++ b/include/acpi/cppc_acpi.h
> > @@ -160,7 +160,7 @@ extern unsigned int cppc_khz_to_perf(str
> > extern bool acpi_cpc_valid(void);
> > extern bool cppc_allow_fast_switch(void);
> > extern int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data);
> > -extern unsigned int cppc_get_transition_latency(int cpu);
> > +extern int cppc_get_transition_latency(int cpu);
> > extern bool cpc_ffh_supported(void);
> > extern bool cpc_supported_by_cpu(void);
> > extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
> > @@ -216,9 +216,9 @@ static inline bool cppc_allow_fast_switc
> > {
> > return false;
> > }
> > -static inline unsigned int cppc_get_transition_latency(int cpu)
> > +static inline int cppc_get_transition_latency(int cpu)
> > {
> > - return CPUFREQ_ETERNAL;
> > + return -ENODATA;
> > }
> > static inline bool cpc_ffh_supported(void)
> > {
> >
> >
> >
> >
>
Powered by blists - more mailing lists