linux-kernel - Re: [PATCH] interconnect: Skip call into provider if initial bw is zero

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5e1f37ba-494a-19d2-e412-7631508ab142@linaro.org>
Date:   Sat, 14 Jan 2023 01:40:19 +0000
From:   Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To:     Vivek Aknurwar <quic_viveka@...cinc.com>, djakov@...nel.org
Cc:     quic_mdtipton@...cinc.com, quic_okukatla@...cinc.com,
        linux-pm@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] interconnect: Skip call into provider if initial bw is
 zero

On 14/01/2023 01:24, Bryan O'Donoghue wrote:
> On 13/01/2023 22:07, Vivek Aknurwar wrote:
>> Currently framework sets bw even when init bw requirements are zero 
>> during
>> provider registration, thus resulting bulk of set bw to hw.
>> Avoid this behaviour by skipping provider set bw calls if init bw is 
>> zero.
>>
>> Signed-off-by: Vivek Aknurwar <quic_viveka@...cinc.com>
>> ---
>>   drivers/interconnect/core.c | 17 ++++++++++-------
>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 25debde..43ed595 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -977,14 +977,17 @@ void icc_node_add(struct icc_node *node, struct 
>> icc_provider *provider)
>>       node->avg_bw = node->init_avg;
>>       node->peak_bw = node->init_peak;
>> -    if (provider->pre_aggregate)
>> -        provider->pre_aggregate(node);
>> -
>> -    if (provider->aggregate)
>> -        provider->aggregate(node, 0, node->init_avg, node->init_peak,
>> -                    &node->avg_bw, &node->peak_bw);
>> +    if (node->avg_bw || node->peak_bw) {
>> +        if (provider->pre_aggregate)
>> +            provider->pre_aggregate(node);
>> +
>> +        if (provider->aggregate)
>> +            provider->aggregate(node, 0, node->init_avg, 
>> node->init_peak,
>> +                        &node->avg_bw, &node->peak_bw);
>> +        if (provider->set)
>> +            provider->set(node, node);
>> +    }
>> -    provider->set(node, node);
>>       node->avg_bw = 0;
>>       node->peak_bw = 0;
> 
> I have the same comment/question for this patch that I had for the qcom 
> arch specific version of it. This patch seems to be doing at a higher 
> level what the patch below was doing at a lower level.
> 
> https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@linaro.org/T/#m0c90588d0d1e2ab88c39be8f5f3a8f0b61396349
> 
> what happens to earlier silicon - qcom silicon which previously made 
> explicit zero requests ?
> 
> https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@linaro.org/T/#m589e8280de470e038249bb362634221771d845dd
> 
> https://lkml.org/lkml/2023/1/3/1232
> 
> Isn't it a better idea to let lower layer drivers differentiate what 
> they do ?
> 
> For example on pre 5.4 qcom kernel silicon we might choose to set the 
> value to zero "because that's what the reference code did" but on newer 
> silicon we might opt to skip the zero configuration ?
> 
> I'm happy to be shown the error of my ways but, absent testing to *show* 
> it doesn't impact existing legacy silicon, I think we should be wary of 
> this change.
> 
> ---
> bod

Oh, and what is the effect on Samsung and i.MX silicon interconnect 
providers of skipping the zero set ?

---
bod