linux-kernel - Re: [PATCH RFT 03/20] clk: qcom: smd-rpm: Add support for keepalive votes

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]

Date:   Mon, 6 Mar 2023 15:04:16 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, linux-arm-msm@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        Shawn Guo <shawn.guo@...aro.org>,
        Taniya Das <quic_tdas@...cinc.com>
Subject: Re: [PATCH RFT 03/20] clk: qcom: smd-rpm: Add support for keepalive
 votes



On 6.03.2023 12:28, Konrad Dybcio wrote:
> 
> 
> On 6.03.2023 02:21, Dmitry Baryshkov wrote:
>> On Sat, 4 Mar 2023 at 15:27, Konrad Dybcio <konrad.dybcio@...aro.org> wrote:
>>>
>>> Some bus clock should always have a minimum (19.2 MHz) vote cast on
>>> them, otherwise the platform will fall apart, hang and reboot.
>>>
>>> Add support for specifying which clocks should be kept alive and
>>> always keep a vote on XO_A to make sure the clock tree doesn't
>>> collapse. This removes the need to keep a maximum vote that was
>>> previously guaranteed by clk_smd_rpm_handoff.
>>>
>>> This commit is a combination of existing (not-exactly-upstream) work
>>> by Taniya Das, Shawn Guo and myself.
>>>
>>> Co-developed-by: Shawn Guo <shawn.guo@...aro.org>
>>> Co-developed-by: Taniya Das <quic_tdas@...cinc.com>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
>>> ---
[...]

>>
>>> +
>>> +               ret = clk_set_rate(keepalive_clks[i]->clk, 19200000);
>>
>> Don't we also need to provide a determine_rate() that will not allow
>> one to set clock frequency below 19.2 MHz?
> Hm, sounds like a good idea..
Thinking about it again, I'd have to do it before the clocks are registered
and we'd either end up with 2 loops, one assigning the CLK_IS_CRITICAL flag
and the other one setting the rate.. Will that not be too hacky?

Konrad

> 
>>
>>> +               if (ret)
>>> +                       return ret;
>>> +       }
>>> +
>>> +       /* Keep an active vote on CXO in case no other driver votes for it. */
>>> +       if (rpm_smd_clks[RPM_SMD_XO_A_CLK_SRC])
>>> +               return clk_prepare_enable(rpm_smd_clks[RPM_SMD_XO_A_CLK_SRC]->hw.clk);
>>> +
>>>         return 0;
>>>  err:
>>>         dev_err(&pdev->dev, "Error registering SMD clock driver (%d)\n", ret);
>>
>>
>> I have mixed feelings towards this patch (and the rest of the
>> patchset). It looks to me like we are trying to patch an issue of the
>> interconnect drivers (or in kernel configuration).
> Well, as you noticed, this patch tries to address a situation where a
> critical clock could be disabled. The interconnect driver (as per my
> other recent patchset) also has a concept of "keepalive", but:
> 
> 1. not very many SoCs already have a functional icc driver
> 2. devices with an existing interconnect driver should also be
>    testable without one (through painful ripping out everything-icc
>    from the dts) for regression tracking
> 
> Konrad
> 
>>
>>
>> --
>> With best wishes
>> Dmitry