linux-kernel - Re: [PATCH v14 8/9] 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] [day] [month] [year] [list]

Message-ID: <f40a24da-d201-4d57-b472-6c98c0e2deaa@linaro.org>
Date: Wed, 19 Jun 2024 22:07:07 +0200
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 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>,
 Conor Dooley <conor+dt@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>, 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,
 Jeffrey Hugo <quic_jhugo@...cinc.com>,
 Marijn Suijten <marijn.suijten@...ainline.org>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
Subject: Re: [PATCH v14 8/9] soc: qcom: Add support for Core Power Reduction
 v3, v4 and Hardened

On 9/6/23 12:23, Ulf Hansson wrote:
> On Mon, 28 Aug 2023 at 13:42, Konrad Dybcio <konrad.dybcio@...aro.org> 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]
>> Tested-by: Jeffrey Hugo <quic_jhugo@...cinc.com>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
>> ---

[...]

>> +
>> +static void cpr_pd_detach_dev(struct generic_pm_domain *domain,
>> +                             struct device *dev)
>> +{
>> +       struct cpr_thread *thread = container_of(domain, struct cpr_thread, pd);
>> +       struct cpr_drv *drv = thread->drv;
>> +
>> +       mutex_lock(&drv->lock);
>> +
>> +       dev_dbg(drv->dev, "detach callback for: %s\n", dev_name(dev));
>> +       thread->attached_cpu_dev = NULL;
>> +
>> +       mutex_unlock(&drv->lock);
> 
> Don't you need to do some additional cleanup here? Like calling
> dev_pm_opp_of_remove_table() for example?
> 

Ouch, right..

[...]

>> +
>> +       /* CPR-Hardened performance states are managed in firmware */
>> +       if (desc->cpr_type == CTRL_TYPE_CPRH)
>> +               thread->pd.set_performance_state = cprh_dummy_set_performance_state;
> 
> The dummy function above always returns 0, without actually doing
> anything. I am trying to understand the purpose of this.
> 
> Would you mind elaborating on this a bit?

It looks like this was put in place to overcome the

.. && genpd->set_performance_state)

check in of_genpd_add_provider_onecell() that gatekeeps calling
functions that parse OPP tables from DT

[...]

> Note that, this was mostly a drive-by-review, looking at the genpd
> provider specific parts. In general this looks good to me, other than
> the minor comments I had above.

No worries, every time I open this file, I end up fixing more and
more things..

Konrad