| Message ID | 20240819035147.2239767-1-zhanjie9@hisilicon.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: CPPC: Return desired perf in ->get() if feedback counters are 0 | expand |
Can anyone take a look at this additional error handling of cppc_cpufreq? Thanks! Jie On 21/08/2024 16:27, Jie Zhan wrote: > cc linux-arm-kernel > > On 19/08/2024 11:51, Jie Zhan wrote: >> The CPPC performance feedback counters could return 0 when the target >> cpu >> is in a deep idle state (e.g. powered off) and those counters are not >> powered. cppc_cpufreq_get_rate() returns 0 in this case, triggering two >> problems: >> >> 1. cpufreq_online() gets a false error and doesn't generate a cpufreq >> policy, which happens in cpufreq_add_dev() when a new cpu device is >> added. >> 2. 'cpuinfo_cur_freq' shows '<unknown>' >> >> Don't take it as an error and return the frequency corresponding to the >> desired perf when the feedback counters are 0. >> >> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns >> zero in all error cases.") >> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> >> --- >> drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpufreq/cppc_cpufreq.c >> b/drivers/cpufreq/cppc_cpufreq.c >> index bafa32dd375d..1c5eb12c1a5a 100644 >> --- a/drivers/cpufreq/cppc_cpufreq.c >> +++ b/drivers/cpufreq/cppc_cpufreq.c >> @@ -748,18 +748,25 @@ static unsigned int >> cppc_cpufreq_get_rate(unsigned int cpu) >> ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); >> if (ret) >> - return 0; >> + goto out_err; >> udelay(2); /* 2usec delay between sampling */ >> ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); >> if (ret) >> - return 0; >> + goto out_err; >> delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, >> &fb_ctrs_t1); >> return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); >> + >> +out_err: >> + if (ret == -EFAULT) >> + return cppc_perf_to_khz(&cpu_data->perf_caps, >> + cpu_data->perf_ctrls.desired_perf); >> + >> + return 0; >> } >> static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, >> int state) >
Cc'd few developers. On 19-08-24, 11:51, Jie Zhan wrote: > The CPPC performance feedback counters could return 0 when the target cpu > is in a deep idle state (e.g. powered off) and those counters are not > powered. cppc_cpufreq_get_rate() returns 0 in this case, triggering two > problems: > > 1. cpufreq_online() gets a false error and doesn't generate a cpufreq > policy, which happens in cpufreq_add_dev() when a new cpu device is added. > 2. 'cpuinfo_cur_freq' shows '<unknown>' > > Don't take it as an error and return the frequency corresponding to the > desired perf when the feedback counters are 0. > > Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") > Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> > --- > drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index bafa32dd375d..1c5eb12c1a5a 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -748,18 +748,25 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); > if (ret) > - return 0; > + goto out_err; > > udelay(2); /* 2usec delay between sampling */ > > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); > if (ret) > - return 0; > + goto out_err; > > delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, > &fb_ctrs_t1); > > return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); > + > +out_err: > + if (ret == -EFAULT) > + return cppc_perf_to_khz(&cpu_data->perf_caps, > + cpu_data->perf_ctrls.desired_perf); > + > + return 0; > } > > static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > -- > 2.33.0 >
Hi, On Wednesday 28 Aug 2024 at 12:20:41 (+0530), Viresh Kumar wrote: > Cc'd few developers. > > On 19-08-24, 11:51, Jie Zhan wrote: > > The CPPC performance feedback counters could return 0 when the target cpu > > is in a deep idle state (e.g. powered off) and those counters are not > > powered. cppc_cpufreq_get_rate() returns 0 in this case, triggering two > > problems: > > > > 1. cpufreq_online() gets a false error and doesn't generate a cpufreq > > policy, which happens in cpufreq_add_dev() when a new cpu device is added. > > 2. 'cpuinfo_cur_freq' shows '<unknown>' I suppose 2. is not necessarily a problem as the current (hardware) frequency is indeed unknown. But there's not clean way to fix 1. while keeping 2. as is, or at least not one I could identify. > > > > Don't take it as an error and return the frequency corresponding to the > > desired perf when the feedback counters are 0. > > > > Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") > > Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> > > --- > > drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > > index bafa32dd375d..1c5eb12c1a5a 100644 > > --- a/drivers/cpufreq/cppc_cpufreq.c > > +++ b/drivers/cpufreq/cppc_cpufreq.c > > @@ -748,18 +748,25 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > > > > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); > > if (ret) > > - return 0; > > + goto out_err; > > > > udelay(2); /* 2usec delay between sampling */ > > > > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); > > if (ret) > > - return 0; > > + goto out_err; > > > > delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, > > &fb_ctrs_t1); > > > > return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); > > + > > +out_err: > > + if (ret == -EFAULT) > > + return cppc_perf_to_khz(&cpu_data->perf_caps, > > + cpu_data->perf_ctrls.desired_perf); > > + A better way might be to cppc_get_desired_perf(cpu, &desired_perf) first and return the khz equivalent of the result, as currently done in hisi_cppc_cpufreq_get_rate(). Even a merge of the two functions might be suitable, but I'm not familiar with the specifics of the hisilicon platforms involved. This might be better as some platforms can provide performance feedback through the desired performance register so a read of it would be better than using the cached desired_perf value. Hope it helps, Ionela. > > + return 0; > > } > > > > static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > > -- > > 2.33.0 > > > > -- > viresh
On 28/08/2024 16:17, Ionela Voinescu wrote: > Hi, > > On Wednesday 28 Aug 2024 at 12:20:41 (+0530), Viresh Kumar wrote: >> Cc'd few developers. >> >> On 19-08-24, 11:51, Jie Zhan wrote: >>> The CPPC performance feedback counters could return 0 when the target cpu >>> is in a deep idle state (e.g. powered off) and those counters are not >>> powered. cppc_cpufreq_get_rate() returns 0 in this case, triggering two >>> problems: >>> >>> 1. cpufreq_online() gets a false error and doesn't generate a cpufreq >>> policy, which happens in cpufreq_add_dev() when a new cpu device is added. >>> 2. 'cpuinfo_cur_freq' shows '<unknown>' Hi Ionela, > I suppose 2. is not necessarily a problem as the current (hardware) > frequency is indeed unknown. > > But there's not clean way to fix 1. while keeping 2. as is, or at least > not one I could identify. Yeah. 1 is the main thing to deal with. >>> Don't take it as an error and return the frequency corresponding to the >>> desired perf when the feedback counters are 0. >>> >>> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") >>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> >>> --- >>> drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >>> index bafa32dd375d..1c5eb12c1a5a 100644 >>> --- a/drivers/cpufreq/cppc_cpufreq.c >>> +++ b/drivers/cpufreq/cppc_cpufreq.c >>> @@ -748,18 +748,25 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) >>> >>> ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); >>> if (ret) >>> - return 0; >>> + goto out_err; >>> >>> udelay(2); /* 2usec delay between sampling */ >>> >>> ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); >>> if (ret) >>> - return 0; >>> + goto out_err; >>> >>> delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, >>> &fb_ctrs_t1); >>> >>> return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); >>> + >>> +out_err: >>> + if (ret == -EFAULT) >>> + return cppc_perf_to_khz(&cpu_data->perf_caps, >>> + cpu_data->perf_ctrls.desired_perf); >>> + > A better way might be to cppc_get_desired_perf(cpu, &desired_perf) first > and return the khz equivalent of the result, as currently done in > hisi_cppc_cpufreq_get_rate(). Even a merge of the two functions might be > suitable, but I'm not familiar with the specifics of the hisilicon platforms > involved. This might be better as some platforms can provide performance > feedback through the desired performance register so a read of it would > be better than using the cached desired_perf value. > > Hope it helps, > Ionela. Sure, understood. Getting the latest desired perf would be more compatible across platforms. Merging hisi_cppc_cpufreq_get_rate() can be risky but worth a try. The workaround also disables the FIE. I'll figure out whether it's feasible to do. I'll send a V2 if no objection to the error handling. Thanks, Jie > >>> + return 0; >>> } >>> >>> static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) >>> -- >>> 2.33.0 >>> >> -- >> viresh
On Wednesday 28 Aug 2024 at 17:45:09 (+0800), Jie Zhan wrote: > > > On 28/08/2024 16:17, Ionela Voinescu wrote: > > Hi, > > > > On Wednesday 28 Aug 2024 at 12:20:41 (+0530), Viresh Kumar wrote: > > > Cc'd few developers. > > > > > > On 19-08-24, 11:51, Jie Zhan wrote: > > > > The CPPC performance feedback counters could return 0 when the target cpu > > > > is in a deep idle state (e.g. powered off) and those counters are not > > > > powered. cppc_cpufreq_get_rate() returns 0 in this case, triggering two > > > > problems: > > > > > > > > 1. cpufreq_online() gets a false error and doesn't generate a cpufreq > > > > policy, which happens in cpufreq_add_dev() when a new cpu device is added. > > > > 2. 'cpuinfo_cur_freq' shows '<unknown>' > Hi Ionela, > > I suppose 2. is not necessarily a problem as the current (hardware) > > frequency is indeed unknown. > > > > But there's not clean way to fix 1. while keeping 2. as is, or at least > > not one I could identify. > Yeah. 1 is the main thing to deal with. > > > > Don't take it as an error and return the frequency corresponding to the > > > > desired perf when the feedback counters are 0. > > > > > > > > Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") > > > > Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> > > > > --- > > > > drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++-- > > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > > > > index bafa32dd375d..1c5eb12c1a5a 100644 > > > > --- a/drivers/cpufreq/cppc_cpufreq.c > > > > +++ b/drivers/cpufreq/cppc_cpufreq.c > > > > @@ -748,18 +748,25 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > > > > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); > > > > if (ret) > > > > - return 0; > > > > + goto out_err; > > > > udelay(2); /* 2usec delay between sampling */ > > > > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); > > > > if (ret) > > > > - return 0; > > > > + goto out_err; > > > > delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, > > > > &fb_ctrs_t1); > > > > return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); > > > > + > > > > +out_err: > > > > + if (ret == -EFAULT) > > > > + return cppc_perf_to_khz(&cpu_data->perf_caps, > > > > + cpu_data->perf_ctrls.desired_perf); > > > > + > > A better way might be to cppc_get_desired_perf(cpu, &desired_perf) first > > and return the khz equivalent of the result, as currently done in > > hisi_cppc_cpufreq_get_rate(). Even a merge of the two functions might be > > suitable, but I'm not familiar with the specifics of the hisilicon platforms > > involved. This might be better as some platforms can provide performance > > feedback through the desired performance register so a read of it would > > be better than using the cached desired_perf value. > > > > Hope it helps, > > Ionela. > Sure, understood. > Getting the latest desired perf would be more compatible across platforms. > > Merging hisi_cppc_cpufreq_get_rate() can be risky but worth a try. The > workaround also disables the FIE. I'll figure out whether it's feasible to > do. Thanks! What I was thinking was that possibly after your changes the current cppc_cpufreq_get_rate() would be suitable for what is now the hisilicon workaround, so there wouldn't be a need to overwrite the .get callback with a custom one. In depends on whether on that particular platform the unsupported counter registers read as 0 and result in the same -EFAUT error. As for disabling FIE, the current cppc_check_hisi_workaround() can be called from cppc_freq_invariance_init() as an added check to the existing ones that result in disabling FIE. Thanks, Ionela. > > I'll send a V2 if no objection to the error handling. > > Thanks, > Jie > > > > > > + return 0; > > > > } > > > > static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > > > > -- > > > > 2.33.0 > > > > > > > -- > > > viresh >
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c index bafa32dd375d..1c5eb12c1a5a 100644 --- a/drivers/cpufreq/cppc_cpufreq.c +++ b/drivers/cpufreq/cppc_cpufreq.c @@ -748,18 +748,25 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); if (ret) - return 0; + goto out_err; udelay(2); /* 2usec delay between sampling */ ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); if (ret) - return 0; + goto out_err; delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, &fb_ctrs_t1); return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); + +out_err: + if (ret == -EFAULT) + return cppc_perf_to_khz(&cpu_data->perf_caps, + cpu_data->perf_ctrls.desired_perf); + + return 0; } static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
The CPPC performance feedback counters could return 0 when the target cpu is in a deep idle state (e.g. powered off) and those counters are not powered. cppc_cpufreq_get_rate() returns 0 in this case, triggering two problems: 1. cpufreq_online() gets a false error and doesn't generate a cpufreq policy, which happens in cpufreq_add_dev() when a new cpu device is added. 2. 'cpuinfo_cur_freq' shows '<unknown>' Don't take it as an error and return the frequency corresponding to the desired perf when the feedback counters are 0. Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> --- drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)