Messages in this thread | | | Subject | Re: [PATCH v3 4/4] powercap/drivers/dtpm: Add CPU energy model based support | From | Daniel Lezcano <> | Date | Thu, 26 Nov 2020 16:06:54 +0100 |
| |
Hi Lukasz,
On 26/11/2020 11:06, Lukasz Luba wrote: > Hi Daniel, > > On 11/23/20 9:42 PM, Daniel Lezcano wrote: >> With the powercap dtpm controller, we are able to plug devices with >> power limitation features in the tree. >> > > [snip] > >> + >> +static void pd_release(struct dtpm *dtpm) >> +{ >> + struct dtpm_cpu *dtpm_cpu = dtpm->private; >> + > > Maybe it's worth to add: > ------------------->8---------------- > if (freq_qos_request_active(&dtpm_cpu->qos_req)) > freq_qos_remove_request(&dtpm_cpu->qos_req); > -------------------8<--------------- > > If we are trying to unregister dtpm in error path due to freq_qos > registration failure, a warning would be emitted from freq_qos.
Ah yes, good point.
>> + freq_qos_remove_request(&dtpm_cpu->qos_req); >> + kfree(dtpm_cpu); >> +} > > [snip] > >> + >> +static int cpuhp_dtpm_cpu_online(unsigned int cpu) >> +{ >> + struct dtpm *dtpm; >> + struct dtpm_cpu *dtpm_cpu; >> + struct cpufreq_policy *policy; >> + struct em_perf_domain *pd; >> + char name[CPUFREQ_NAME_LEN]; >> + int ret; >> + >> + policy = cpufreq_cpu_get(cpu); >> + >> + if (!policy) >> + return 0; >> + >> + pd = em_cpu_get(cpu); >> + if (!pd) >> + return -EINVAL; >> + >> + dtpm = per_cpu(dtpm_per_cpu, cpu); >> + if (dtpm) >> + return power_add(dtpm, pd); >> + >> + dtpm = dtpm_alloc(&dtpm_ops); >> + if (!dtpm) >> + return -EINVAL; >> + >> + dtpm_cpu = kzalloc(sizeof(dtpm_cpu), GFP_KERNEL); >> + if (!dtpm_cpu) { >> + kfree(dtpm); >> + return -ENOMEM; >> + } >> + >> + dtpm->private = dtpm_cpu; >> + dtpm_cpu->cpu = cpu; >> + >> + for_each_cpu(cpu, policy->related_cpus) >> + per_cpu(dtpm_per_cpu, cpu) = dtpm; >> + >> + sprintf(name, "cpu%d", dtpm_cpu->cpu); >> + >> + ret = dtpm_register(name, dtpm, __parent); >> + if (ret) >> + goto out_kfree_dtpm_cpu; >> + >> + ret = power_add(dtpm, pd); >> + if (ret) >> + goto out_power_sub; > > Shouldn't we call dtpm_unregister() instead? > The dtpm_unregister() would remove the zone, which IIUC we > are currently missing. > >> + >> + ret = freq_qos_add_request(&policy->constraints, >> + &dtpm_cpu->qos_req, FREQ_QOS_MAX, >> + pd->table[pd->nr_perf_states - 1].frequency); >> + if (ret) >> + goto out_dtpm_unregister; > > Could this trigger different steps, starting from out_power_sub_v2 > below? > >> + >> + return 0; >> + >> +out_dtpm_unregister: >> + dtpm_unregister(dtpm); >> + dtpm_cpu = NULL; /* Already freed by the release ops */ >> +out_power_sub: >> + power_sub(dtpm, pd); > > I would change the order of these two above into something like:
Ok, I'll revisit the rollback routine.
> out_power_sub_v2: > power_sub(dtpm, pd); > out_dtpm_unregister_v2: > dtpm_unregister(dtpm); > dtpm_cpu = NULL; > >> +out_kfree_dtpm_cpu: >> + for_each_cpu(cpu, policy->related_cpus) >> + per_cpu(dtpm_per_cpu, cpu) = NULL; >> + kfree(dtpm_cpu); >> + >> + return ret; >> +} > > IIUC power_sub() would decrement the power and set it to 0 for that > dtmp, then the dtpm_unregister() would also try to decrement the power, > but by the value of 0. So it should be safe.
Right.
Thanks for the review
-- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
| |