Re: [PATCH v7] crypto: qce - Add runtime PM and interconnect bandwidth scaling support

[Konrad Dybcio]

On 2/21/26 5:00 AM, Bjorn Andersson wrote:
> On Fri, Feb 20, 2026 at 10:52:04AM +0100, Konrad Dybcio wrote:
>> On 2/20/26 8:28 AM, quic_utiwari@xxxxxxxxxxx wrote:
>>> From: Udit Tiwari <quic_utiwari@xxxxxxxxxxx>
>>>
>>> The Qualcomm Crypto Engine (QCE) driver currently lacks support for
>>> runtime power management (PM) and interconnect bandwidth control.
>>> As a result, the hardware remains fully powered and clocks stay
>>> enabled even when the device is idle. Additionally, static
>>> interconnect bandwidth votes are held indefinitely, preventing the
>>> system from reclaiming unused bandwidth.
>>>
>>> Address this by enabling runtime PM and dynamic interconnect
>>> bandwidth scaling to allow the system to suspend the device when idle
>>> and scale interconnect usage based on actual demand. Improve overall
>>> system efficiency by reducing power usage and optimizing interconnect
>>> resource allocation.
>>
>> [...]

[...]

>>
>>
>>> +static int __maybe_unused qce_runtime_suspend(struct device *dev)
>>
>> I think you should be able to drop __maybe_unused if you drop the
>> SET_ prefix in pm_ops
> 
> I believe that's correct.
> 
>> and add a pm_ptr() around &qce_crypto_pm_ops in
>> the assignment at the end
>>
> 
> Doesn't that turn into NULL if CONFIG_PM=n and then you get a warning
> about unused struct?

If I'm reading

1a3c7bb08826 ("PM: core: Add new *_PM_OPS macros, deprecate old ones")

correctly, it should be fine.. I'm seeing e.g. 

161266c754e7 ("can: rcar_canfd: Convert to DEFINE_SIMPLE_DEV_PM_OPS()")

do that, but I don't fully understand why this works. Perhaps __maybe_unused
does not resolve recursively?

> 
>>> +{
>>> +	struct qce_device *qce = dev_get_drvdata(dev);
>>> +
>>> +	icc_disable(qce->mem_path);
>>
>> icc_disable() can also fail, since under the hood it's an icc_set(path, 0, 0),
>> please check its retval
>>
> 
> Given that the outcome of returning an error from the runtime_suspend
> callback is to leave the domain in ACTIVE state I presume that also
> means we need to turn icc_enable() again if pm_clk_suspend() where to
> fail?

Seems that way

> Two things I noted while looking at icc_disable():
> 
> 1) icc_bulk_disable() return type is void. Which perhaps relates to the
> fact that qcom_icc_set() can't fail?

I think both of these are problems.


> 2) Error handling in icc_disable() is broken.
> 
> icc_disable() sets enabled = false on the path, then calls icc_set_bw()
> with the current bandwith again. This stores the old votes, then calls
> aggregate_requests() (which ignored enabled == false votes) and then
> attempts to apply_contraints().
> 
> If the apply_contraints() fails, it reinstate the old vote (which in
> this case is the same as the new vote) and then does the
> aggregate_requests() and apply_contraints() dance again.
> 
> I'm assuming the idea here is to give the provider->set() method a
> chance to reject the new votes.

For obscure cases where e.g. going under a certain total bandwidth
across a bus would be strictly forbidden and only enforced in .set()?

> But during the re-application of the old votes (which are same as the
> new ones) enabled is still false across the path, so we're not
> reinstating anything and while we're exiting icc_disabled() with an
> error, the path is now disabled - in software, because we have no idea
> what the hardware state is.

What you described also seems like a real issue. The latter part, we
probably just have to hope that the "enable back" vote goes through.
Else, the system would likely fall apart within seconds anyway

Konrad