linux-kernel - Re: [PATCH 2/2] cpufreq: arm_big_little: use generic OPP functions for {init,free}_opp

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: <57220686.9050507@arm.com>
Date:	Thu, 28 Apr 2016 13:48:06 +0100
From:	Sudeep Holla <sudeep.holla@....com>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	Sudeep Holla <sudeep.holla@....com>, linux-kernel@...r.kernel.org,
	Viresh Kumar <vireshk@...nel.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>, linux-pm@...r.kernel.org
Subject: Re: [PATCH 2/2] cpufreq: arm_big_little: use generic OPP functions
 for {init,free}_opp_table



On 28/04/16 12:26, Viresh Kumar wrote:
> On 28-04-16, 11:25, Sudeep Holla wrote:
>> Currently when performing random hotplugs and suspend-to-ram(S2R) on
>> systems using arm_big_little cpufreq driver, we get warnings similar to:
>>
>> cpu cpu1: _opp_add: duplicate OPPs detected. Existing: freq: 600000000,
>> 	volt: 800000, enabled: 1. New: freq: 600000000, volt: 800000, enabled: 1
>>
>> This is mainly because the OPPs for the shared cpus are not set. We can
>> just use dev_pm_opp_of_cpumask_add_table in case the OPPs are obtained
>> from DT(arm_big_little_dt.c) or use dev_pm_opp_set_sharing_cpus if the
>> OPPs are obtained by other means like firmware(e.g. scpi-cpufreq.c)
>>
>> Also now that the generic dev_pm_opp_cpumask_remove_table can handle
>> removal of opp table and entries for all associated CPUs, we can reuse
>> dev_pm_opp_cpumask_remove_table as free_opp_table in cpufreq_arm_bL_ops.
>>
>> This patch makes necessary changes to reuse the generic OPP functions for
>> {init,free}_opp_table and thereby eliminating the warnings.
>>
>> Cc: Viresh Kumar <vireshk@...nel.org>
>> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
>> Cc: linux-pm@...r.kernel.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
>> ---
>>   drivers/cpufreq/arm_big_little.c    | 54 ++++++++++++++++++++-----------------
>>   drivers/cpufreq/arm_big_little.h    |  4 +--
>>   drivers/cpufreq/arm_big_little_dt.c | 21 ++-------------
>>   drivers/cpufreq/scpi-cpufreq.c      | 24 ++++++++++++++---
>>   4 files changed, 54 insertions(+), 49 deletions(-)
>>
>> Hi Viresh,
>>
>> Why is dynamic OPPs not deleted in dev_pm_opp_{,cpumask_}remove_table ?
>> It would remove some code in scpi-cpufreq.c but I would like to understand.
>> Is there any use in not deleting them there ?
>
> That was intentional, consider this case:
> - OPPs are added dynamically from platform code
> - insmod cpufreq-dt.ko (add static (dt) OPPs as well, mostly fail)
> - rmmod  cpufreq-dt.ko (remove all OPPs)
>
> - insmod cpufreq-dt.ko (no more OPPs available, as we removed them both).
>
> That's bad ?
>
> Now, how to fix platforms which don't add dynamic OPPs from platform code? But
> something like the scpi driver, which can get inserted/removed again.
>
> This is what I would suggest:
> - Even if dev_pm_opp_of_cpumask_remove_table() isn't doing any OF specific
>    stuff, lets not update its name and move it out of the ifdef, as it will be
>    used only if the user has created OPPs using DT earlier.
> - But rather make those two routines call two new routines in the core (which
>    don't depend on OF) and implement what these of-remove routines do.
> - Add two more routines for removing OPPs created dynamically and call the two
>    routines created in previous step.
>
> I hope you followed that :)
>

Yes I got it. I had thought of this initially, but somehow got convinced 
that we can delete dynamic OPPs too.

>> -static void scpi_free_opp_table(struct device *cpu_dev)
>> +static void scpi_free_opp_table(cpumask_var_t cpumask)
>>   {
>> +	struct device *cpu_dev = get_cpu_device(cpumask_first(cpumask));
>> +
>> +	cpumask_clear_cpu(cpu_dev->id, cpumask);
>> +	dev_pm_opp_cpumask_remove_table(cpumask);
>> +	cpumask_set_cpu(cpu_dev->id, cpumask);
>> +
>>   	scpi_opp_table_ops(cpu_dev, true);
>>   }
>
> This was a *really* crazy idea :)
>

I knew that :)

> And btw, I think you better move to cpufreq-dt, instead of struggling with this
> one :)
>

Yes that's next on my TODO, but since this is duplicate messages are
getting reported now, it's better to fix that rather than wait for move
to cpufreq-dt. BTW cpufreq-dt will be misnomer after I make it work with
firmware driver DVFS.

Anyways, what ever change to fix these warning now will help to move to
cpufreq-dt IMO.

-- 
Regards,
Sudeep