| Message ID | 20240919084552.3591400-1-zhanjie9@hisilicon.com |
|---|---|
| Headers | show |
| Series | cppc_cpufreq: Rework ->get() error handling when cores are idle | expand |
Hi Jie, LGTM except for some trivial, Reviewed-by: Huisong Li <lihuisong@huawei.com> 在 2024/9/19 16:45, Jie Zhan 写道: > The CPPC performance feedback counters could be 0 or unchanged when the > target cpu is in a low-power idle state, e.g. power-gated or clock-gated. > > When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes > cpufreq_online() get a false error and fail to generate a cpufreq policy. > > When the counters are unchanged, the existing cppc_perf_from_fbctrs() > returns a cached desired perf, but some platforms may update the real > frequency back to the desired perf reg. > > For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf > to reflect the frequency; if failed, return the cached desired perf. > > Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") > Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> > Reviewed-by: Zeng Heng <zengheng4@huawei.com> > Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com> > --- > drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++------- > 1 file changed, 39 insertions(+), 10 deletions(-) > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index bafa32dd375d..e55192303a9f 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work) > > perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs, > &fb_ctrs); > + if (!perf) > + return; > + > cppc_fi->prev_perf_fb_ctrs = fb_ctrs; > > perf <<= SCHED_CAPACITY_SHIFT; > @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data, > > /* Check to avoid divide-by zero and invalid delivered_perf */ Now this comment can be removed, right? > if (!delta_reference || !delta_delivered) > - return cpu_data->perf_ctrls.desired_perf; > + return 0; > > return (reference_perf * delta_delivered) / delta_reference; > } > > +static int cppc_get_perf_ctrs_sample(int cpu, > + struct cppc_perf_fb_ctrs *fb_ctrs_t0, > + struct cppc_perf_fb_ctrs *fb_ctrs_t1) > +{ > + int ret; > + > + ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0); > + if (ret) > + return ret; > + > + udelay(2); /* 2usec delay between sampling */ > + > + return cppc_get_perf_ctrs(cpu, fb_ctrs_t1); > +} > + > static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > { > struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0}; > @@ -746,18 +764,29 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > > cpufreq_cpu_put(policy); > > - ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); > - if (ret) > - return 0; > - > - udelay(2); /* 2usec delay between sampling */ > - > - ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); > - if (ret) > - return 0; > + ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1); > + if (ret) { > + if (ret == -EFAULT) > + goto out_invalid_counters; suggest that add some comments for ret == -EFAULT case. Because this error code depands on the implementation of cppc_get_perf_ctrs. If add a new exception case which also return -EFAULT, then this switch is unreasonable. > + else > + return 0; > + } > > delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0, > &fb_ctrs_t1); > + if (!delivered_perf) > + goto out_invalid_counters; > + > + return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); > + > +out_invalid_counters: > + /* > + * Feedback counters could be unchanged or 0 when a cpu enters a > + * low-power idle state, e.g. clock-gated or power-gated. > + * Get the lastest or cached desired perf for reflecting frequency. > + */ > + if (cppc_get_desired_perf(cpu, &delivered_perf)) > + delivered_perf = cpu_data->perf_ctrls.desired_perf; > > return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf); > }
On 25/09/2024 17:28, lihuisong (C) wrote: > Hi Jie, > > LGTM except for some trivial, > Reviewed-by: Huisong Li <lihuisong@huawei.com> Thanks. > > > 在 2024/9/19 16:45, Jie Zhan 写道: >> The CPPC performance feedback counters could be 0 or unchanged when the >> target cpu is in a low-power idle state, e.g. power-gated or clock-gated. >> >> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes >> cpufreq_online() get a false error and fail to generate a cpufreq policy. >> >> When the counters are unchanged, the existing cppc_perf_from_fbctrs() >> returns a cached desired perf, but some platforms may update the real >> frequency back to the desired perf reg. >> >> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf >> to reflect the frequency; if failed, return the cached desired perf. >> >> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") >> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> >> Reviewed-by: Zeng Heng <zengheng4@huawei.com> >> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com> >> --- >> drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++------- >> 1 file changed, 39 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >> index bafa32dd375d..e55192303a9f 100644 >> --- a/drivers/cpufreq/cppc_cpufreq.c >> +++ b/drivers/cpufreq/cppc_cpufreq.c >> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work) >> perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs, >> &fb_ctrs); >> + if (!perf) >> + return; >> + >> cppc_fi->prev_perf_fb_ctrs = fb_ctrs; >> perf <<= SCHED_CAPACITY_SHIFT; >> @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data, >> /* Check to avoid divide-by zero and invalid delivered_perf */ > Now this comment can be removed, right? Didn't notice this comment, but, having a check, I think it still fits. '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks invalid delivered_perf. So I think we just leave it unchanged. >> if (!delta_reference || !delta_delivered) >> - return cpu_data->perf_ctrls.desired_perf; >> + return 0; >> return (reference_perf * delta_delivered) / delta_reference; >> } >> +static int cppc_get_perf_ctrs_sample(int cpu, >> + struct cppc_perf_fb_ctrs *fb_ctrs_t0, >> + struct cppc_perf_fb_ctrs *fb_ctrs_t1) >> +{ >> + int ret; >> + >> + ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0); >> + if (ret) >> + return ret; >> + >> + udelay(2); /* 2usec delay between sampling */ >> + >> + return cppc_get_perf_ctrs(cpu, fb_ctrs_t1); >> +} >> + >> static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) >> { >> struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0}; >> @@ -746,18 +764,29 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) >> cpufreq_cpu_put(policy); >> - ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); >> - if (ret) >> - return 0; >> - >> - udelay(2); /* 2usec delay between sampling */ >> - >> - ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); >> - if (ret) >> - return 0; >> + ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1); >> + if (ret) { >> + if (ret == -EFAULT) >> + goto out_invalid_counters; > suggest that add some comments for ret == -EFAULT case. > Because this error code depands on the implementation of cppc_get_perf_ctrs. > If add a new exception case which also return -EFAULT, then this switch is unreasonable. Sure. What about adding the following comment: /* -EFAULT indicates that any of the associated CPPC regs is 0. */ Thanks, Jie
在 2024/9/26 10:57, Jie Zhan 写道: > > On 25/09/2024 17:28, lihuisong (C) wrote: >> Hi Jie, >> >> LGTM except for some trivial, >> Reviewed-by: Huisong Li <lihuisong@huawei.com> > Thanks. > >> >> 在 2024/9/19 16:45, Jie Zhan 写道: >>> The CPPC performance feedback counters could be 0 or unchanged when the >>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated. >>> >>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes >>> cpufreq_online() get a false error and fail to generate a cpufreq policy. >>> >>> When the counters are unchanged, the existing cppc_perf_from_fbctrs() >>> returns a cached desired perf, but some platforms may update the real >>> frequency back to the desired perf reg. >>> >>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf >>> to reflect the frequency; if failed, return the cached desired perf. >>> >>> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") >>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> >>> Reviewed-by: Zeng Heng <zengheng4@huawei.com> >>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com> >>> --- >>> drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++------- >>> 1 file changed, 39 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >>> index bafa32dd375d..e55192303a9f 100644 >>> --- a/drivers/cpufreq/cppc_cpufreq.c >>> +++ b/drivers/cpufreq/cppc_cpufreq.c >>> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work) >>> perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs, >>> &fb_ctrs); >>> + if (!perf) >>> + return; >>> + >>> cppc_fi->prev_perf_fb_ctrs = fb_ctrs; >>> perf <<= SCHED_CAPACITY_SHIFT; >>> @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data, >>> /* Check to avoid divide-by zero and invalid delivered_perf */ >> Now this comment can be removed, right? > Didn't notice this comment, but, having a check, I think it still fits. > '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks > invalid delivered_perf. The comment "avoid divide-by zero" is just for the below code: "(reference_perf * delta_delivered) / delta_reference". So It is also useful, but I think It's obvious and it doesn't make much sense. The comment "avoid invalid delivered_perf" is for the return value. Now this func return zero which can't count as a valid delivered_perf, right? > > So I think we just leave it unchanged. > >>> if (!delta_reference || !delta_delivered) >>> - return cpu_data->perf_ctrls.desired_perf; >>> + return 0; >>> return (reference_perf * delta_delivered) / delta_reference; >>> } >>> +static int cppc_get_perf_ctrs_sample(int cpu, >>> + struct cppc_perf_fb_ctrs *fb_ctrs_t0, >>> + struct cppc_perf_fb_ctrs *fb_ctrs_t1) >>> +{ >>> + int ret; >>> + >>> + ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0); >>> + if (ret) >>> + return ret; >>> + >>> + udelay(2); /* 2usec delay between sampling */ >>> + >>> + return cppc_get_perf_ctrs(cpu, fb_ctrs_t1); >>> +} >>> + >>> static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) >>> { >>> struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0}; >>> @@ -746,18 +764,29 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) >>> cpufreq_cpu_put(policy); >>> - ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); >>> - if (ret) >>> - return 0; >>> - >>> - udelay(2); /* 2usec delay between sampling */ >>> - >>> - ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1); >>> - if (ret) >>> - return 0; >>> + ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1); >>> + if (ret) { >>> + if (ret == -EFAULT) >>> + goto out_invalid_counters; >> suggest that add some comments for ret == -EFAULT case. >> Because this error code depands on the implementation of cppc_get_perf_ctrs. >> If add a new exception case which also return -EFAULT, then this switch is unreasonable. > Sure. What about adding the following comment: > > /* -EFAULT indicates that any of the associated CPPC regs is 0. */ Ack > .
On 26/09/2024 14:07, lihuisong (C) wrote: > > 在 2024/9/26 10:57, Jie Zhan 写道: >> >> On 25/09/2024 17:28, lihuisong (C) wrote: >>> Hi Jie, >>> >>> LGTM except for some trivial, >>> Reviewed-by: Huisong Li <lihuisong@huawei.com> >> Thanks. >> >>> >>> 在 2024/9/19 16:45, Jie Zhan 写道: >>>> The CPPC performance feedback counters could be 0 or unchanged when the >>>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated. >>>> >>>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes >>>> cpufreq_online() get a false error and fail to generate a cpufreq policy. >>>> >>>> When the counters are unchanged, the existing cppc_perf_from_fbctrs() >>>> returns a cached desired perf, but some platforms may update the real >>>> frequency back to the desired perf reg. >>>> >>>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf >>>> to reflect the frequency; if failed, return the cached desired perf. >>>> >>>> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") >>>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> >>>> Reviewed-by: Zeng Heng <zengheng4@huawei.com> >>>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com> >>>> --- >>>> drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++------- >>>> 1 file changed, 39 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >>>> index bafa32dd375d..e55192303a9f 100644 >>>> --- a/drivers/cpufreq/cppc_cpufreq.c >>>> +++ b/drivers/cpufreq/cppc_cpufreq.c >>>> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work) >>>> perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs, >>>> &fb_ctrs); >>>> + if (!perf) >>>> + return; >>>> + >>>> cppc_fi->prev_perf_fb_ctrs = fb_ctrs; >>>> perf <<= SCHED_CAPACITY_SHIFT; >>>> @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data, >>>> /* Check to avoid divide-by zero and invalid delivered_perf */ >>> Now this comment can be removed, right? >> Didn't notice this comment, but, having a check, I think it still fits. >> '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks >> invalid delivered_perf. > The comment "avoid divide-by zero" is just for the below code: "(reference_perf * delta_delivered) / delta_reference". > So It is also useful, but I think It's obvious and it doesn't make much sense. > > The comment "avoid invalid delivered_perf" is for the return value. > Now this func return zero which can't count as a valid delivered_perf, right? so, what about this? /* * Avoid divide-by zero and unchanged feedback counters. * Leave it for callers to handle. */ >> >> So I think we just leave it unchanged. >> ...
在 2024/9/26 16:44, Jie Zhan 写道: > > On 26/09/2024 14:07, lihuisong (C) wrote: >> 在 2024/9/26 10:57, Jie Zhan 写道: >>> On 25/09/2024 17:28, lihuisong (C) wrote: >>>> Hi Jie, >>>> >>>> LGTM except for some trivial, >>>> Reviewed-by: Huisong Li <lihuisong@huawei.com> >>> Thanks. >>> >>>> 在 2024/9/19 16:45, Jie Zhan 写道: >>>>> The CPPC performance feedback counters could be 0 or unchanged when the >>>>> target cpu is in a low-power idle state, e.g. power-gated or clock-gated. >>>>> >>>>> When the counters are 0, cppc_cpufreq_get_rate() returns 0 KHz, which makes >>>>> cpufreq_online() get a false error and fail to generate a cpufreq policy. >>>>> >>>>> When the counters are unchanged, the existing cppc_perf_from_fbctrs() >>>>> returns a cached desired perf, but some platforms may update the real >>>>> frequency back to the desired perf reg. >>>>> >>>>> For the above cases in cppc_cpufreq_get_rate(), get the latest desired perf >>>>> to reflect the frequency; if failed, return the cached desired perf. >>>>> >>>>> Fixes: 6a4fec4f6d30 ("cpufreq: cppc: cppc_cpufreq_get_rate() returns zero in all error cases.") >>>>> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com> >>>>> Reviewed-by: Zeng Heng <zengheng4@huawei.com> >>>>> Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com> >>>>> --- >>>>> drivers/cpufreq/cppc_cpufreq.c | 49 +++++++++++++++++++++++++++------- >>>>> 1 file changed, 39 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c >>>>> index bafa32dd375d..e55192303a9f 100644 >>>>> --- a/drivers/cpufreq/cppc_cpufreq.c >>>>> +++ b/drivers/cpufreq/cppc_cpufreq.c >>>>> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work) >>>>> perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs, >>>>> &fb_ctrs); >>>>> + if (!perf) >>>>> + return; >>>>> + >>>>> cppc_fi->prev_perf_fb_ctrs = fb_ctrs; >>>>> perf <<= SCHED_CAPACITY_SHIFT; >>>>> @@ -726,11 +729,26 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data, >>>>> /* Check to avoid divide-by zero and invalid delivered_perf */ >>>> Now this comment can be removed, right? >>> Didn't notice this comment, but, having a check, I think it still fits. >>> '!delta_reference' avoids divide-by zero, and '!delta_delivered' checks >>> invalid delivered_perf. >> The comment "avoid divide-by zero" is just for the below code: "(reference_perf * delta_delivered) / delta_reference". >> So It is also useful, but I think It's obvious and it doesn't make much sense. >> >> The comment "avoid invalid delivered_perf" is for the return value. >> Now this func return zero which can't count as a valid delivered_perf, right? > so, what about this? > > /* > * Avoid divide-by zero and unchanged feedback counters. > * Leave it for callers to handle. > */ good. >>> So I think we just leave it unchanged. >>> > ... > .