Message-ID: <d850071e-162d-9303-6e28-1fe675f69ce4@linaro.org>
Date:   Mon, 17 Apr 2023 14:49:27 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Taniya Das <quic_tdas@...cinc.com>
Cc:     Andy Gross <agross@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] clk: qcom: common: Handle invalid index error

On 17/04/2023 07:40, Taniya Das wrote:
> Hi Dmitry,
> 
> Thanks for the comments.
> 
> 
> On 3/3/2023 4:14 PM, Dmitry Baryshkov wrote:
>> On Fri, 3 Mar 2023 at 11:30, Taniya Das <quic_tdas@...cinc.com> wrote:
>>>
>>> Introduce start_index to handle invalid index error
>>> seen when there are two clock descriptors assigned
>>> to the same clock controller.
>>
>> Please provide details of the exact case that you are trying to solve
>> (this might go to the cover letter). I think the commit message is
>> slightly misleading here. Are you trying to add error messages or to
>> prevent them from showing up?
>>
> 
> We are trying to avoid error messages from showing up.
> 
>> I'm asking because error messages do not seem to correspond to patch
>> 2. You add start_index to make the kernel warn for the clock indices
>> less than LPASS_AUDIO_CC_CDIV_RX_MCLK_DIV_CLK_SRC = 4, while quoted
>> messages show indices 5,6,7.
>>
> 
> Right, we want the kernel to warn if the clock index is less than 
> start_index,

This is arguable but logical. Usually we do not warn for absent clocks.

> along with that we also want to handle the case where 
> num_rclks is uninitialized because of same clock descriptor being 
> assigned to two clock controllers.

Hmm, but num_rclks is always initialized, isn't it? In the worst case it 
will default to 0 meaning that

> Earlier Invalid index error was showing up for valid indices 5,6,7 
> because of the simple if check(idx >= num_rclks), hence we enhanced the 
> checks to handle the above case and compare the index to the start_index 
> + num_rclks, instead of simply comparing it with num_clks.

This is not a part of the patch and it will be incorrect anyway, since 
num_rclks = desc->num_clks = ARRAY_SIZE(some_cc_clocks).

Checking idx against `start_index + num_rclks` will allow one to get 
clocks after the end of rclks array.

For lpass_audio_cc_sc7280_desc num_rclks should get the value of 16, as 
the last entry in llpass_audio_cc_sc7280_clocks has index 
LPASS_AUDIO_CC_RX_MCLK_CLK_SRC = 15.

My analysis might be completely wrong, but I can only assume that 
somehow wrong clock controller got used. Could you please give it a try 
with the 
https://lore.kernel.org/linux-clk/20230417114659.137535-1-dmitry.baryshkov@linaro.org/ 
being applied?

> 
>> Nit: please don't overwrap the commit message, the recommended line
>> width is about 72-77 chars.
>>
> 
> Done.
> 
>>>
>>> [ 3.600604] qcom_cc_clk_hw_get: invalid index 5
>>> [ 3.625251] qcom_cc_clk_hw_get: invalid index 6
>>> [ 3.648190] qcom_cc_clk_hw_get: invalid index 7
>>
>>>
>>> Fixes: 120c15528390 ("clk: qcom: Migrate to clk_hw based registration 
>>> and OF APIs")
>>> Signed-off-by: Taniya Das <quic_tdas@...cinc.com>
>>> ---
>>>   drivers/clk/qcom/common.c | 12 ++++++++----
>>>   drivers/clk/qcom/common.h |  1 +
>>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>> index 75f09e6e057e..0e80535b61f2 100644
>>> --- a/drivers/clk/qcom/common.c
>>> +++ b/drivers/clk/qcom/common.c
>>> @@ -21,6 +21,7 @@ struct qcom_cc {
>>>          struct qcom_reset_controller reset;
>>>          struct clk_regmap **rclks;
>>>          size_t num_rclks;
>>> +       u32 rclks_start_index;
>>>   };
>>>
>>>   const
>>> @@ -226,12 +227,13 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct 
>>> of_phandle_args *clkspec,
>>>          struct qcom_cc *cc = data;
>>>          unsigned int idx = clkspec->args[0];
>>>
>>> -       if (idx >= cc->num_rclks) {
>>> +       if (idx >= cc->rclks_start_index && idx < cc->num_rclks)
>>> +               return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
>>> +       else if (idx < cc->rclks_start_index && idx >= cc->num_rclks)
>>>                  pr_err("%s: invalid index %u\n", __func__, idx);
>>> -               return ERR_PTR(-EINVAL);
>>> -       }
>>>
>>> -       return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
>>> +       return ERR_PTR(-EINVAL);
>>> +
>>>   }
>>>
>>>   int qcom_cc_really_probe(struct platform_device *pdev,
>>> @@ -281,6 +283,8 @@ int qcom_cc_really_probe(struct platform_device 
>>> *pdev,
>>>
>>>          cc->rclks = rclks;
>>>          cc->num_rclks = num_clks;
>>> +       if (desc->start_index)
>>> +               cc->rclks_start_index = desc->start_index;
>>>
>>>          qcom_cc_drop_protected(dev, cc);
>>>
>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>> index 9c8f7b798d9f..924f36af55b3 100644
>>> --- a/drivers/clk/qcom/common.h
>>> +++ b/drivers/clk/qcom/common.h
>>> @@ -23,6 +23,7 @@ struct qcom_cc_desc {
>>>          const struct regmap_config *config;
>>>          struct clk_regmap **clks;
>>>          size_t num_clks;
>>> +       u32 start_index;
>>>          const struct qcom_reset_map *resets;
>>>          size_t num_resets;
>>>          struct gdsc **gdscs;
>>> -- 
>>> 2.17.1
>>>
>>
>>
>> -- 
>> With best wishes
>>
>>
>>
>> Dmitry
> 

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists