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 <[email protected]>
---
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;
--
2.39.2
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
>> ---
>> 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 <[email protected]>
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 <[email protected]>
>>> ---
>>> 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 ?
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 <[email protected]>
>>>> ---
>>>> 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
>
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 <[email protected]>
>>>> ---
>>>> 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 ?
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 <[email protected]>
>>>>> ---
>>>>> 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
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 <[email protected]>
>>>>>> ---
>>>>>> 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
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 <[email protected]>
>>>>>>> ---
>>>>>>> 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
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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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..
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 <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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
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 <[email protected]>
> ---
> 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