Message-ID: <11fce8ad-46a4-305c-cc8f-c7530c13ecbd@linaro.org>
Date:   Fri, 3 Mar 2023 13:35:08 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [RFC PATCH] interconnect: qcom: icc-rpm: Don't call
 __qcom_icc_set twice on the same node
Content-Language: en-US
To:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        linux-arm-msm@vger.kernel.org, andersson@kernel.org,
        agross@kernel.org
Cc:     marijn.suijten@somainline.org, Georgi Djakov <djakov@kernel.org>,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230303023500.2173137-1-konrad.dybcio@linaro.org>
 <59b28fb4-1367-9872-ed63-90847e380bb6@linaro.org>
 <29513b9e-8561-7f7e-370e-7515116c7ee6@linaro.org>
 <3116a08a-30a8-c9b9-f079-26739c9e6d49@linaro.org>
 <08020872-6316-8f81-ac6a-c6eef408818f@linaro.org>
 <db4f4e53-e8b9-0807-7490-2c6b76194ad5@linaro.org>
 <2cfba291-656f-9a43-fdd5-7601a6fe6ef5@linaro.org>
 <0b1fafe6-0814-f1a6-ae19-7bd273751102@linaro.org>
 <133be267-1aab-350f-f3ba-142cf4dee4d4@linaro.org>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <133be267-1aab-350f-f3ba-142cf4dee4d4@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 3.03.2023 12:50, Bryan O'Donoghue wrote:
> On 03/03/2023 11:42, Konrad Dybcio wrote:
>>
>>
>> On 3.03.2023 12:40, Bryan O'Donoghue wrote:
>>> On 03/03/2023 11:39, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 3.03.2023 12:36, Bryan O'Donoghue wrote:
>>>>> On 03/03/2023 11:35, Bryan O'Donoghue wrote:
>>>>>> On 03/03/2023 11:33, Konrad Dybcio wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 3.03.2023 12:32, Bryan O'Donoghue wrote:
>>>>>>>> On 03/03/2023 02:35, Konrad Dybcio wrote:
>>>>>>>>> Currently, when sync_state calls set(n, n) all the paths for setting
>>>>>>>>> parameters on an icc node are called twice. Avoid that.
>>>>>>>>>
>>>>>>>>> Fixes: 751f4d14cdb4 ("interconnect: icc-rpm: Set destination bandwidth as well as source bandwidth")
>>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>> ---
>>>>>>>>> RFC comes from the fact that I *believe* this should be correct, but I'm
>>>>>>>>> not entirely sure about it..
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>      drivers/interconnect/qcom/icc-rpm.c | 2 +-
>>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>>>>>>>>> index a6e0de03f46b..d35db1af9b08 100644
>>>>>>>>> --- a/drivers/interconnect/qcom/icc-rpm.c
>>>>>>>>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>>>>>>>>> @@ -387,7 +387,7 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>>>>>>>>>          ret = __qcom_icc_set(src, src_qn, sum_bw);
>>>>>>>>>          if (ret)
>>>>>>>>>              return ret;
>>>>>>>>> -    if (dst_qn) {
>>>>>>>>> +    if (dst_qn && src_qn != dst_qn) {
>>>>>>>>>              ret = __qcom_icc_set(dst, dst_qn, sum_bw);
>>>>>>>>>              if (ret)
>>>>>>>>>                  return ret;
>>>>>>>>
>>>>>>>> Is it possible for src_qn == dst_qn ?
>>>>>>> As the commit message says, sync_state calls set(n, n) in
>>>>>>> drivers/interconnect/core.c : icc_sync_state(struct device *dev)
>>>>>>
>>>>>> So you've _seen_ that happen ?
>>>>>>
>>>>>
>>>>> Assuming you have, then why isn't the fix in sync_state i.e. that's an error for everybody right ?
>>>> I believe that there's simply no other way of updating every single node
>>>> on its own with the icc api, without taking any links into play. But I
>>>> see exynos and i.mx also effectively calling it twice on each node.
>>>>
>>>> Konrad
>>>
>>> I mean. I'm fine for you to retain my RB on this qcom specific patch since this seems like a real bug to me but... it seems like a generic bug across arches that should probably be resolved @ the higher level.
>>>
>>> ?
>> I suppose we could change the set(n, n) in sync_state to be set(n, NULL)
>> and enforce parameter null-checking on all provider->set functions. Do
>> I understand this correctly?
>>
>> Konrad
>>>
>>> ---
>>> bod
> 
> void icc_sync_state(struct device *dev)
> {
>         struct icc_provider *p;
>         struct icc_node *n;
>         static int count;
> 
>         count++;
> 
>         if (count < providers_count)
>                 return;
> 
>         mutex_lock(&icc_lock);
>         synced_state = true;
>         list_for_each_entry(p, &icc_providers, provider_list) {
>                 dev_dbg(p->dev, "interconnect provider is in synced state\n");
>                 list_for_each_entry(n, &p->nodes, node_list) {
>                         if (n->init_avg || n->init_peak) {
>                                 n->init_avg = 0;
>                                 n->init_peak = 0;
>                                 aggregate_requests(n);
>                                 p->set(n, n);
>                         }
>                 }
>         }
>         mutex_unlock(&icc_lock);
> }
> EXPORT_SYMBOL_GPL(icc_sync_state);
> 
> I mean p->set(n,n); is done like this since forever. Now that you draw attention to it, it doesn't make much sense to me..
Yeah, but we're doing the same thing twice.. So maybe this is not
so much a bug fix as it's an optimization..

Thinking about it again, this could use a likely() too, as this
seems to be the only occurence of set(n, n)

Konrad