Re: [dpdk-dev] [PATCH v1 1/1] power: add support for cppc cpufreq

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 22-Apr-21 10:29 AM, Richael Zhuang wrote:
> 
> 
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Thursday, April 22, 2021 5:06 PM
>> To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
>> Cc: nd <nd@arm.com>; David Hunt <david.hunt@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v1 1/1] power: add support for cppc cpufreq
>>
>> On 22-Apr-21 7:15 AM, Richael Zhuang wrote:
>>> Currently in DPDK only acpi_cpufreq and pstate_cpufreq drivers are
>>> supported, which are both not available on arm64 platforms. Add
>>> support for cppc_cpufreq driver which works on most arm64 platforms.
>>>
>>> Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
>>> ---
>>
>> Just a general note: this looks like a copy-paste of pstate code. Which is
>> perfectly fine, except that we can do better than copying some faults of the
>> pstate code to other drivers. I've submitted a patch [1] attempting to fix
>> some of the pressing issues and code duplication in pstate driver, but i'm
>> sure with a fresh driver, you can do even better :)
>>
>> [1]
>> http://patches.dpdk.org/project/dpdk/patch/20210402092701.258316-1-
>> anatoly.burakov@intel.com/
>>
>> --
>> Thanks,
>> Anatoly
> 
> For CPPC is defined in acpi v5.0+ spec,  I reused most code in acpi_cpufreq  to get a quick workable version on our platform with only cppc driver. I have verified  its basic functions. If you find some problems please help to point out thus I can rework it. Thanks .
> 
> Best Regards,
> Richael
> 

Well, pstate code was copied from ACPI so it does share the same flaws:

- Lots of code duplication (e.g. snprintf for filename, fopen sequences, 
etc.)
- Confusing and bug-prone error handling (e.g. return macros in the 
middle of a function)
- Mixing power management logic and gory details of string handling

Good examples of the above are in your `power_check_turbo()` function - 
lots of string handling code interspersed with file opens, and actual 
logic of power management.

Please see the patch i linked earlier [1] to understand what kind of 
changes i'm suggesting. Perhaps you could do even better :)

[1] 
http://patches.dpdk.org/project/dpdk/patch/20210402092701.258316-1-anatoly.burakov@intel.com/

-- 
Thanks,
Anatoly