linux-kernel - Re: [PATCH v10 5/6] soc: qcom: Add support for Core Power Reduction v3, v4 and Hardened

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 27 Feb 2023 10:13:05 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>,
        Niklas Cassel <nks@...wful.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     Robert Marko <robimarko@...il.com>, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-pm@...r.kernel.org,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
Subject: Re: [PATCH v10 5/6] soc: qcom: Add support for Core Power Reduction
 v3, v4 and Hardened

Il 27/02/23 03:55, Dmitry Baryshkov ha scritto:
> On 17/02/2023 13:08, Konrad Dybcio wrote:
>> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
>>
>> This commit introduces a new driver, based on the one for cpr v1,
>> to enable support for the newer Qualcomm Core Power Reduction
>> hardware, known downstream as CPR3, CPR4 and CPRh, and support
>> for MSM8998 and SDM630 CPU power reduction.
>>
>> In these new versions of the hardware, support for various new
>> features was introduced, including voltage reduction for the GPU,
>> security hardening and a new way of controlling CPU DVFS,
>> consisting in internal communication between microcontrollers,
>> specifically the CPR-Hardened and the Operating State Manager.
>>
>> The CPR v3, v4 and CPRh are present in a broad range of SoCs,
>> from the mid-range to the high end ones including, but not limited
>> to, MSM8953/8996/8998, SDM630/636/660/845.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
>> [Konrad: rebase, apply review comments]
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
>> ---
>>   drivers/soc/qcom/Kconfig      |   22 +
>>   drivers/soc/qcom/Makefile     |    4 +-
>>   drivers/soc/qcom/cpr-common.h |    2 +
>>   drivers/soc/qcom/cpr3.c       | 2923 +++++++++++++++++++++++++++++++++++++++++
>>   include/soc/qcom/cpr.h        |   17 +
>>   5 files changed, 2967 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/soc/qcom/cpr.h b/include/soc/qcom/cpr.h
>> new file mode 100644
>> index 000000000000..2ba4324d18f6
>> --- /dev/null
>> +++ b/include/soc/qcom/cpr.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2013-2020, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2019 Linaro Limited
>> + * Copyright (c) 2021, AngeloGioacchino Del Regno
>> + *                     <angelogioacchino.delregno@...ainline.org>
>> + */
>> +
>> +#ifndef __CPR_H__
>> +#define __CPR_H__
>> +
>> +struct cpr_ext_data {
>> +    int mem_acc_threshold_uV;
>> +    int apm_threshold_uV;
>> +};
> 
> Who is going to use this? Is it the cpufreq driver or some other driver?
> We are adding an API without a clean user, can we drop it for now?
> 

This is mandatory: qcom-cpufreq-hw is supposed to program the OSM before
starting.

 From SDM845 onwards, the OSM is programmed by the bootloader before booting
Linux;
In MSM8996/98, SDM630/636/660, others, the bootloader does not program the OSM
uC, so this has to be done in Linux - specifically, in the CPUFREQ driver
(qcom-cpufreq-hw), otherwise this driver is completely pointless to have.

CPU DVFS requires three uC to be correctly programmed in order to work:
  - SAW (for sleep states)
  - CPR-Hardened (voltage control, mandatory for stability)
  - OSM (for cpufreq-hw frequency steps [1..N])

Failing to *correctly* program either of the three will render CPU DVFS unusable.


That clarified, my opinion is:
No, you can't drop this. It's an essential piece for functionality.

I agree in that this commit introduces a header that has only an internal (as in
cpr3.c) user and no external ones, but I think that Konrad didn't want to include
the qcom-cpufreq-hw.c commits in this series because it's already huge and pretty
difficult to review; adding the cpufreq-hw commits would make the situation worse.

Konrad, perhaps you can send the cpufreq-hw commits in a separate series, in
which cover letter you mention a dependency on this one?
That would *clearly* show the full picture to reviewers.

I remember that when I sent the cpufreq-hw series along with this one (~2 years
ago, I think?) that code had positive reviews from Bjorn, so it should be OK.
It wasn't picked just-only-because of the cpr3 dependency.

Regards,
Angelo

>> +
>> +#endif /* __CPR_H__ */
>>
>