Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ce6c952b-2e2b-67d9-5023-e740ed798758@quicinc.com>
Date:   Mon, 17 Apr 2023 10:10:48 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 1/2] clk: qcom: common: Handle invalid index error
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC:     Andy Gross <agross@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20230303092859.22094-1-quic_tdas@quicinc.com>
 <20230303092859.22094-2-quic_tdas@quicinc.com>
 <CAA8EJpq5xBF=Wt-1_hGR-7qZHREcALurmR4ucmMmZaC-R_7Ttg@mail.gmail.com>
From:   Taniya Das <quic_tdas@quicinc.com>
In-Reply-To: <CAA8EJpq5xBF=Wt-1_hGR-7qZHREcALurmR4ucmMmZaC-R_7Ttg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

-- 
Thanks & Regards,
Taniya Das.