linux-kernel - Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are not set in DT

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: <20181030071313.rq2d4cmfrla5boc7@vireshk-i7>
Date:   Tue, 30 Oct 2018 12:43:13 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     rjw@...ysocki.net, vincent.guittot@...aro.org,
        linux-kernel@...r.kernel.org,
        Chris Redpath <chris.redpath@...aro.org>,
        Quentin Perret <quentin.perret@...aro.org>,
        Amit Kucheria <amit.kucheria@...aro.org>,
        Nicolas Dechesne <nicolas.dechesne@...aro.org>,
        Niklas Cassel <niklas.cassel@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH 4/4] base/drivers/topology: Default dmpis-mhz if they are
 not set in DT

On 29-10-18, 17:23, Daniel Lezcano wrote:
> In the case of assymetric SoC with the same micro-architecture, we
> have a group of CPUs with smaller OPPs than the other group. One
> example is the 96boards dragonboard 820c. There is no dmips/MHz
> difference between both groups, so no need to specify the values in
> the DT. Unfortunately, without these defined, there is no scaling
> capacity comutation triggered, so we need to write
> 'capacity-dmips-mhz' for each CPU with the same value in order to
> force the scaled capacity computation.
> 
> Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no
> 'capacity-dmips-mhz' is defined in the DT.
> 
> This was tested on db820c:
>  - specified values in the DT (correct results)
>  - partial values defined in the DT (error + fallback to defaults)
>  - no specified values in the DT (correct results)
> 
> correct results are:
>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>    758
>    758
>   1024
>   1024
> 
>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
> 
> That reflects the capacity for the max frequencies 1593600 and 2150400.
> 
> Cc: Chris Redpath <chris.redpath@...aro.org>
> Cc: Quentin Perret <quentin.perret@...aro.org>
> Cc: Viresh Kumar <viresh.kumar@...aro.org>
> Cc: Amit Kucheria <amit.kucheria@...aro.org>
> Cc: Nicolas Dechesne <nicolas.dechesne@...aro.org>
> Cc: Niklas Cassel <niklas.cassel@...aro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> ---
>  drivers/base/arch_topology.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 7311641..7d594a6 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -205,6 +205,21 @@ static struct notifier_block init_cpu_capacity_notifier = {
>  	.notifier_call = init_cpu_capacity_callback,
>  };
>  
> +static int topology_set_default_capacity(void)
> +{
> +	int cpu;
> +
> +	raw_capacity = kzalloc(num_possible_cpus() * sizeof(*raw_capacity),
> +			       GFP_KERNEL);

kmalloc ?

> +	if (!raw_capacity)
> +		return -ENOMEM;
> +
> +	for_each_possible_cpu(cpu)
> +		raw_capacity[cpu] = SCHED_CAPACITY_SCALE;

It makes it look as if it is important to set default values to
SCHED_CAPACITY_SCALE while that is not the case. Maybe add a comment
that all CPUs are assumed to have same capacity now. Maybe initialize
to 1 instead ? Not sure if that would be better or not.

> +
> +	return 0;
> +}
> +
>  static int __init register_cpufreq_notifier(void)
>  {
>  	int ret;
> @@ -214,9 +229,19 @@ static int __init register_cpufreq_notifier(void)
>  	 * until we have the necessary code to parse the cpu capacity, so
>  	 * skip registering cpufreq notifier.
>  	 */
> -	if (!acpi_disabled || !raw_capacity)
> +	if (!acpi_disabled)
>  		return -EINVAL;
>  
> +	if (!raw_capacity) {
> +
> +		pr_info("cpu_capacity: No capacity defined in DT, set default "
> +		       "values to %ld\n", SCHED_CAPACITY_SCALE);

So we will end up doing this also for the case where the DT is
partially filled with the capacity info and we already printed error
messages for it. Should we really do that ?

-- 
viresh