diff mbox series

[RFC] interconnect: qcom: icc-rpm: Don't call __qcom_icc_set twice on the same node

Message ID 20230303023500.2173137-1-konrad.dybcio@linaro.org
State New
Headers
Series [RFC] interconnect: qcom: icc-rpm: Don't call __qcom_icc_set twice on the same node |

Commit Message

Konrad Dybcio March 3, 2023, 2:35 a.m. UTC 
  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(-)

Comments

Bryan O'Donoghue March 3, 2023, 11:32 a.m. UTC | #1 
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 ?

Iff you confirm that experimentally - add my

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Konrad Dybcio March 3, 2023, 11:33 a.m. UTC | #2 
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)

Konrad
> 
> Iff you confirm that experimentally - add my
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Bryan O'Donoghue March 3, 2023, 11:35 a.m. UTC | #3 
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 ?
Konrad Dybcio March 3, 2023, 11:36 a.m. UTC | #4 
On 3.03.2023 12: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 ?
Yes, I did, on every boot previous to this patch, I believe.

Konrad
>
Bryan O'Donoghue March 3, 2023, 11:36 a.m. UTC | #5 
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 ?
Konrad Dybcio March 3, 2023, 11:39 a.m. UTC | #6 
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
Bryan O'Donoghue March 3, 2023, 11:40 a.m. UTC | #7 
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.

?

---
bod
Konrad Dybcio March 3, 2023, 11:42 a.m. UTC | #8 
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
Bryan O'Donoghue March 3, 2023, 11:50 a.m. UTC | #9 
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..
Konrad Dybcio March 3, 2023, 12:35 p.m. UTC | #10 
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
Georgi Djakov March 21, 2023, 7:30 p.m. UTC | #11 
On 3.03.23 4: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.

This could be optimized indeed.

> 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;

Today we also call provider->set(node, node) in icc_node_add() to set
the initial bandwidth when nodes are being added to the topology. The
above change will affect that as well.

BR,
Georgi
diff mbox series

Patch

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;