linux-kernel - Re: [PATCH v2 1/3] soc: qcom: ice: Add OPP-based clock scaling support for ICE

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: <7a0db353-02f7-4188-b2d7-9098548f1920@oss.qualcomm.com>
Date: Wed, 17 Dec 2025 14:47:05 +0100
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Abhinaba Rakshit <abhinaba.rakshit@....qualcomm.com>
Cc: Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        "James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Neeraj Soni <neeraj.soni@....qualcomm.com>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-scsi@...r.kernel.org
Subject: Re: [PATCH v2 1/3] soc: qcom: ice: Add OPP-based clock scaling
 support for ICE

On 12/8/25 7:41 AM, Abhinaba Rakshit wrote:
> On Fri, Nov 21, 2025 at 02:46:52PM +0100, Konrad Dybcio wrote:
>> On 11/21/25 11:36 AM, Abhinaba Rakshit wrote:
>>> Register optional operation-points-v2 table for ICE device
>>> and aquire its minimum and maximum frequency during ICE
>>> device probe.
>>>
>>> Introduce clock scaling API qcom_ice_scale_clk which scale ICE
>>> core clock if valid (non-zero) frequencies are obtained from
>>> OPP-table. Zero min and max (default values) frequencies depicts
>>> clock scaling is disabled.
>>>
>>> When an ICE-device specific OPP table is available, use the PM OPP
>>> framework to manage frequency scaling and maintain proper power-domain
>>> constraints. For legacy targets without an ICE-device specific OPP table,
>>> fall back to the standard clock framework APIs to set the frequency.
>>
>> You can still set a frequency through OPP APIs if the table is empty
>> (and one is always created even if devm_pm_opp_of_add_table() fails)
>>
> 
> We observed that when devm_pm_opp_of_add_table() returns -ENODEV (indicating
> that no OPP table is defined in the devicetree), subsequent calls to APIs
> like dev_pm_opp_set_rate() fail because the device does not have an OPP table
> registered. As a result, the clock rate cannot be set using OPP-based helpers.
> 
> Here is an dmesg ice driver logs for lemans device without opp-table defined in its devicetree.
> sh-5.2# dmesg | grep qcom-ice
> [    7.316366] qcom-ice 87c8000.crypto: dev_pm_opp_set_rate: device's opp table doesn't exist
> [    7.325596] qcom-ice 87c8000.crypto: Failed boosting the ICE clk to TURBO
> [    7.333288] qcom-ice 87c8000.crypto: _find_key: OPP table not found (-19)
> [    7.340968] qcom-ice 87c8000.crypto: Unable to find ICE core clock min freq
> [    7.348832] qcom-ice 87c8000.crypto: _find_key: OPP table not found (-19)
> [    7.356510] qcom-ice 87c8000.crypto: Unable to find ICE core clock max freq
> [    7.364377] qcom-ice 87c8000.crypto: Found QC Inline Crypto Engine (ICE) v3.2.0
> [    7.372594] qcom-ice 87c8000.crypto: QC ICE HWKM (Hardware Key Manager) version = 1

Hm, perhaps I missed something..

> Additionally, on legacy targets where ICE does not exist as a separate device,
> the OPP table is managed through the storage subsystem. In such cases, using
> OPP APIs directly for ICE would not be appropriate because the OPP table may
> also control other clocks, leading to unintended side effects.

This is a more convincing argument. But it also pushes me towards
the opinion that it may not be worth supporting the older one on the grounds
of the description being bogus..

> 
>> [...]
>>
>>>  	/*
>>>  	 * Legacy DT binding uses different clk names for each consumer,
>>> -	 * so lets try those first. If none of those are a match, it means
>>> -	 * the we only have one clock and it is part of the dedicated DT node.
>>> -	 * Also, enable the clock before we check what HW version the driver
>>> -	 * supports.
>>> +	 * so lets try those first. Also get its corresponding clock index.
>>> +	 */
>>
>> I would argue *not* setting the rate on targets utilizing a binding without
>> an OPP table for the ICE is probably a smart thing to do, because we may
>> brownout the SoC this way
> 
> Understand the concern here.
> However, our approach is to scale the ICE clock only when the storage subsystem scales
> its own clocks. Since the storage driver already manages the associated power domain
> and voltage adjustments (even for targets without opp-table for ICE) —which are shared
> with ICE—this ensures that any frequency changes occur in a safe context. As a result,
> the risk of a SoC brownout condition should be effectively mitigated.

Can you guarantee that this can be taken for granted for every SoC we've
ever released?

Konrad