| 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
| ||
|
Date: Wed, 13 Apr 2022 21:24:25 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Ansuel Smith <ansuelsmth@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Andy Gross <agross@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-clk@...r.kernel.org
Subject: Re: [PATCH v6 10/18] clk: qcom: krait-cc: drop hardcoded safe_sel
On 13/04/2022 20:35, Ansuel Smith wrote:
> On Wed, Apr 13, 2022 at 08:25:31PM +0300, Dmitry Baryshkov wrote:
>> On 22/03/2022 02:15, Ansuel Smith wrote:
>>> Drop hardcoded safe_sel definition and use helper to correctly calculate
>>> it. We assume qsb clk is always present as it should be declared in DTS
>>> per Documentation and in the absence of that, it's declared as a fixed
>>> clk.
>>
>> Why? Can safe_sel (sec_mux index) change?
>>
>
> No it can't but I think it would be better to have stuff that is based
> on real defined data instead of wrong struct that works just because a
> hardcoded value is used.
>
> Example for the reason of using this hardcoded value, nobody notice that
> the mux list for the secondary mux was wrong. Now it didn't cause any
> problem as we use the secondary mux just to source out of qsb and we
> used the hardcoded value so the error was bypassed but as soon as the
> value was actually parsed from the defined table, bam secondary mux was
> sourcing out of pll8.
>
> I honestly think that dropping the hardcoded value would make more clear
> what the safe sel is and what is referring to.
Can you define indices in the parents and use defined indice instead?
I see your point, but in my opinion adding an API to determine a
compile-time constant value is a bit of overkill.
>
>>>
>>> Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
>>> ---
>>> drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------
>>> 1 file changed, 27 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
>>> index e9508e3104ea..5f98ee1c3681 100644
>>> --- a/drivers/clk/qcom/krait-cc.c
>>> +++ b/drivers/clk/qcom/krait-cc.c
>>> @@ -26,6 +26,17 @@ static unsigned int pri_mux_map[] = {
>>> 0,
>>> };
>>> +static u8 krait_get_mux_sel(struct krait_mux_clk *mux, struct clk *safe_clk)
>>> +{
>>> + struct clk_hw *safe_hw = __clk_get_hw(safe_clk);
>>> +
>>> + /*
>>> + * We can ignore errors from clk_hw_get_index_of_parent()
>>> + * as we create these parents in this driver.
>>> + */
>>> + return clk_hw_get_index_of_parent(&mux->hw, safe_hw);
>>> +}
>>> +
>>> /*
>>> * Notifier function for switching the muxes to safe parent
>>> * while the hfpll is getting reprogrammed.
>>> @@ -116,8 +127,8 @@ krait_add_div(struct device *dev, int id, const char *s, unsigned int offset)
>>> }
>>> static struct clk *
>>> -krait_add_sec_mux(struct device *dev, int id, const char *s,
>>> - unsigned int offset, bool unique_aux)
>>> +krait_add_sec_mux(struct device *dev, struct clk *qsb, int id,
>>> + const char *s, unsigned int offset, bool unique_aux)
>>> {
>>> int ret;
>>> struct krait_mux_clk *mux;
>>> @@ -144,7 +155,6 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
>>> mux->shift = 2;
>>> mux->parent_map = sec_mux_map;
>>> mux->hw.init = &init;
>>> - mux->safe_sel = 0;
>>> init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
>>> if (!init.name)
>>> @@ -166,6 +176,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
>>> if (IS_ERR(clk))
>>> goto err_clk;
>>> + mux->safe_sel = krait_get_mux_sel(mux, qsb);
>>> ret = krait_notifier_register(dev, clk, mux);
>>> if (ret)
>>> clk = ERR_PTR(ret);
>>> @@ -204,7 +215,6 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>>> mux->lpl = id >= 0;
>>> mux->parent_map = pri_mux_map;
>>> mux->hw.init = &init;
>>> - mux->safe_sel = 2;
>>> init.name = kasprintf(GFP_KERNEL, "krait%s_pri_mux", s);
>>> if (!init.name)
>>> @@ -226,6 +236,7 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>>> if (IS_ERR(clk))
>>> goto err_clk;
>>> + mux->safe_sel = krait_get_mux_sel(mux, sec_mux);
>>> ret = krait_notifier_register(dev, clk, mux);
>>> if (ret)
>>> clk = ERR_PTR(ret);
>>> @@ -238,7 +249,9 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>>> }
>>> /* id < 0 for L2, otherwise id == physical CPU number */
>>> -static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
>>> +static struct clk *
>>> +krait_add_clks(struct device *dev, struct clk *qsb, int id,
>>> + bool unique_aux)
>>> {
>>> unsigned int offset;
>>> void *p = NULL;
>>> @@ -261,7 +274,7 @@ static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
>>> goto err;
>>> }
>>> - sec_mux = krait_add_sec_mux(dev, id, s, offset, unique_aux);
>>> + sec_mux = krait_add_sec_mux(dev, qsb, id, s, offset, unique_aux);
>>> if (IS_ERR(sec_mux)) {
>>> clk = sec_mux;
>>> goto err;
>>> @@ -301,18 +314,19 @@ static int krait_cc_probe(struct platform_device *pdev)
>>> int cpu;
>>> struct clk *clk;
>>> struct clk **clks;
>>> - struct clk *l2_pri_mux_clk;
>>> + struct clk *l2_pri_mux_clk, *qsb;
>>> id = of_match_device(krait_cc_match_table, dev);
>>> if (!id)
>>> return -ENODEV;
>>> /* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */
>>> - if (IS_ERR(clk_get(dev, "qsb")))
>>> - clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
>>> + qsb = clk_get(dev, "qsb");
>>> + if (IS_ERR(qsb))
>>> + qsb = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
>>> - if (IS_ERR(clk))
>>> - return PTR_ERR(clk);
>>> + if (IS_ERR(qsb))
>>> + return PTR_ERR(qsb);
>>> if (!id->data) {
>>> clk = clk_register_fixed_factor(dev, "acpu_aux",
>>> @@ -327,13 +341,13 @@ static int krait_cc_probe(struct platform_device *pdev)
>>> return -ENOMEM;
>>> for_each_possible_cpu(cpu) {
>>> - clk = krait_add_clks(dev, cpu, id->data);
>>> + clk = krait_add_clks(dev, qsb, cpu, id->data);
>>> if (IS_ERR(clk))
>>> return PTR_ERR(clk);
>>> clks[cpu] = clk;
>>> }
>>> - l2_pri_mux_clk = krait_add_clks(dev, -1, id->data);
>>> + l2_pri_mux_clk = krait_add_clks(dev, qsb, -1, id->data);
>>> if (IS_ERR(l2_pri_mux_clk))
>>> return PTR_ERR(l2_pri_mux_clk);
>>> clks[4] = l2_pri_mux_clk;
>>
>>
>> --
>> With best wishes
>> Dmitry
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists