[1/2] clk: qcom: common: Handle invalid index error

Introduce start_index to handle invalid index error
seen when there are two clock descriptors assigned
to the same clock controller.

[ 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@quicinc.com>
---
 drivers/clk/qcom/common.c | 12 ++++++++----
 drivers/clk/qcom/common.h |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

--
2.17.1

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@quicinc.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, 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.

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.

> 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@quicinc.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

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@quicinc.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@quicinc.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
>

Quoting Taniya Das (2023-04-16 21:40:48)
> 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@quicinc.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, 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.

The start_index should be 0.