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

[Geert Uytterhoeven]

On Tue, 18 Nov 2025 at 14:08, Konrad Dybcio
<konrad.dybcio@xxxxxxxxxxxxxxxx> wrote:
> On 11/18/25 7:46 AM, Udit Tiwari wrote:
> > Thanks for the review, please find my response inline.
> >
> > On 11/17/2025 5:55 PM, Konrad Dybcio wrote:
> >> On 11/17/25 7:27 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.
>
> [...]
>
> >>> Signed-off-by: Udit Tiwari <quic_utiwari@xxxxxxxxxxx>
> >>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202511160711.Q6ytYvlG-lkp@xxxxxxxxx/
> >>> ---
> >>> Changes in v4:
> >>> - Annotate runtime PM callbacks with __maybe_unused to silence W=1 warnings.
> >>> - Add Reported-by and Closes tags for kernel test robot warning.
> >>
> >> The tags are now saying
> >>
> >> "The kernel test robot reported that the QCE driver does not have PM
> >> operations and this patch fixes that."
> >>
> >> Which doesn't have a reflection in reality.
> >>
> >> [...]
> >>
> > I may be misunderstanding this comment but the bot flagged W=1 unused-function warnings under !CONFIG_PM. In v4 I added __maybe_unused and Reported-by/Closes for that exact warning; I didn’t mean to imply the driver lacks PM ops.
>
> The case where the tags would apply would be:
>
> A patch is submitted
> The patch gets reviewed and applied to the tree
> Kernel testing robot reports an issue
> You send a fix-up patch (incl. robot's tags)

Exactly. The robot's report even says so:

    If you fix the issue in a separate patch/commit (i.e. not just a
new version of
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    the same patch/commit), kindly add following tags
    ^^^^^^^^^^^^^^^^^^^^^^
    | Reported-by: kernel test robot <lkp@xxxxxxxxx>
    | Closes: https://lore.kernel.org/oe-kbuild-all/202511160711.Q6ytYvlG-lkp@xxxxxxxxx/

>
> [...]
>
> >>> +    ret = pm_clk_add(dev, "bus");
> >>> +    if (ret)
> >>> +        return ret;
> >>
> >> Not all SoC have a pair of clocks. This is going to break those who don't
> >>
> >> Konrad
> > On the concern that not all SoCs have "core/iface/bus" clocks and that this could break those platforms: i believe the PM clock helpers are tolerant of missing clock entries. If a clock is not described in DT, pm_clk_add will not cause the probe to fail, also on such platforms, runtime/system PM will simply not toggle that clock.
> >
> > I’ve tested this on sc7280 where the QCE node has no clock entries, and the driver probes and operates correctly; runtime PM and interconnect behavior are as expected.
> >
> > If you’d like this handled in a specific way, please let me know—I’m happy to implement that approach.
>
> No, you're right. I took a look at the pm_clk_add() call chain and noticed
> that clk_get() (notably not _optional) is in there, but apparently its
> retval is never propagated if things fail
>
> (+RJW/Geert is that intended behavior?)

I think most checking is done before calling pm_clk_add() or
pm_clk_add_clk(). of_pm_clk_add_clks() also calls of_clk_get() first.
So that looks intentional to me.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds