linux-kernel - Re: [PATCH] ARM: cpuidle: Support asymmetric idle definition

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]

Date:   Mon, 22 May 2017 11:11:37 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     rjw@...ysocki.net, lorenzo.pieralisi@....com,
        Sudeep Holla <sudeep.holla@....com>, leo.yan@...aro.org,
        "open list:CPUIDLE DRIVERS" <linux-pm@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ARM: cpuidle: Support asymmetric idle definition



On 19/05/17 17:45, Daniel Lezcano wrote:
> Some hardware have clusters with different idle states. The current code does
> not support this and fails as it expects all the idle states to be identical.
> 
> Because of this, the Mediatek mtk8173 had to create the same idle state for a
> big.Little system and now the Hisilicon 960 is facing the same situation.
> 

While I agree the we don't support them today, it's better to benchmark
and record/compare the gain we get with the support for cluster based
idle states.

> Solve this by simply assuming the multiple driver will be needed for all the
> platforms using the ARM generic cpuidle driver which makes sense because of the
> different topologies we can support with a single kernel for ARM32 or ARM64.
> 

Unfortunately, it's not true always and for sure will break with the new
ARM DynamIQ [1]

> Tested on:
>  - 96boards: Hikey 620
>  - 96boards: Hikey 960
>  - 96boards: dragonboard410c
>  - Mediatek 8173
> 
> Tested-by: Leo Yan <leo.yan@...aro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> ---
>  drivers/cpuidle/Kconfig.arm   |  1 +
>  drivers/cpuidle/cpuidle-arm.c | 55 +++++++++++++++++++++++++++++--------------
>  2 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 21340e0..f521448 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -4,6 +4,7 @@
>  config ARM_CPUIDLE
>          bool "Generic ARM/ARM64 CPU idle Driver"
>          select DT_IDLE_STATES
> +	select CPU_IDLE_MULTIPLE_DRIVERS
>          help
>            Select this to enable generic cpuidle driver for ARM.
>            It provides a generic idle driver whose idle states are configured
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index f440d38..bec31d5 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -18,6 +18,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/slab.h>
> +#include <linux/topology.h>
>  
>  #include <asm/cpuidle.h>
>  
> @@ -44,7 +45,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
>  	return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, idx);
>  }
>  
> -static struct cpuidle_driver arm_idle_driver = {
> +static struct cpuidle_driver arm_idle_driver __initdata = {
>  	.name = "arm_idle",
>  	.owner = THIS_MODULE,
>  	/*
> @@ -80,23 +81,40 @@ static const struct of_device_id arm_idle_state_match[] __initconst = {
>  static int __init arm_idle_init(void)
>  {
>  	int cpu, ret;
> -	struct cpuidle_driver *drv = &arm_idle_driver;
> +	struct cpuidle_driver *drv = NULL;
>  	struct cpuidle_device *dev;
>  
> -	/*
> -	 * Initialize idle states data, starting at index 1.
> -	 * This driver is DT only, if no DT idle states are detected (ret == 0)
> -	 * let the driver initialization fail accordingly since there is no
> -	 * reason to initialize the idle driver if only wfi is supported.
> -	 */
> -	ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> -	if (ret <= 0)
> -		return ret ? : -ENODEV;
> -
> -	ret = cpuidle_register_driver(drv);
> -	if (ret) {
> -		pr_err("Failed to register cpuidle driver\n");
> -		return ret;
> +	for_each_possible_cpu(cpu) {
> +
> +		if (drv && cpumask_test_cpu(cpu, drv->cpumask))
> +			continue;
> +
> +		ret = -ENOMEM;
> +
> +		drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL);
> +		if (!drv)
> +			goto out_fail;
> +
> +		drv->cpumask = &cpu_topology[cpu].core_sibling;
> +

This is not always true and not architecturally guaranteed. So instead
of introducing this broken dependency, better to extract information
from the device tree.

-- 
Regards,
Sudeep

[1]
https://community.arm.com/processors/b/blog/posts/arm-dynamiq-technology-for-the-next-era-of-compute