Re: [PATCH v3 4/4] powercap/drivers/dtpm: Add CPU energy model based support

[Daniel Lezcano]

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