linux-kernel - Re: [PATCH 2/2] 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]
Message-ID: <2056969.dMlCPX6XND@aspire.rjw.lan>
Date:   Mon, 12 Jun 2017 20:49:39 +0200
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     linux-pm@...r.kernel.org, Sudeep Holla <sudeep.holla@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Leo Yan <leo.yan@...aro.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] ARM: cpuidle: Support asymmetric idle definition

On Monday, June 12, 2017 05:55:10 PM 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.
> 
> 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.
> 
> Every CPU has its own driver, so every single CPU can specify in the DT the
> idle states.
> 
> This simple approach allows to support the future dynamIQ system, current SMP
> and HMP.
> 
> Tested on:
>  - 96boards: Hikey 620
>  - 96boards: Hikey 960
>  - 96boards: dragonboard410c
>  - Mediatek 8173
> 
> Cc: Sudeep Holla <sudeep.holla@....com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
> Tested-by: Leo Yan <leo.yan@...aro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>

There seems to have been quite some discussion regarding this one and I'm not
sure about the resolution of it.

I'd feel more comfortable with an ACK or Reviewed-by from Sudeep or Lorenzo here.

> ---
>  drivers/cpuidle/Kconfig.arm   |  1 +
>  drivers/cpuidle/cpuidle-arm.c | 62 ++++++++++++++++++++++++++-----------------
>  2 files changed, 39 insertions(+), 24 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..7080c38 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,30 +81,42 @@ 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;
>  	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;
> -	}
> -
> -	/*
> -	 * Call arch CPU operations in order to initialize
> -	 * idle states suspend back-end specific data
> -	 */
>  	for_each_possible_cpu(cpu) {
> +
> +		drv = kmemdup(&arm_idle_driver, sizeof(*drv), GFP_KERNEL);
> +		if (!drv) {
> +			ret = -ENOMEM;
> +			goto out_fail;
> +		}
> +
> +		drv->cpumask = (struct cpumask *)cpumask_of(cpu);
> +
> +		/*
> +		 * 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) {
> +			ret = ret ? : -ENODEV;
> +			goto out_fail;
> +		}
> +
> +		ret = cpuidle_register_driver(drv);
> +		if (ret) {
> +			pr_err("Failed to register cpuidle driver\n");
> +			goto out_fail;
> +		}
> +
> +		/*
> +		 * Call arch CPU operations in order to initialize
> +		 * idle states suspend back-end specific data
> +		 */
>  		ret = arm_cpuidle_init(cpu);
>  
>  		/*
> @@ -141,10 +154,11 @@ static int __init arm_idle_init(void)
>  		dev = per_cpu(cpuidle_devices, cpu);
>  		cpuidle_unregister_device(dev);
>  		kfree(dev);
> +		drv = cpuidle_get_driver();
> +		cpuidle_unregister_driver(drv);
> +		kfree(drv);
>  	}
>  
> -	cpuidle_unregister_driver(drv);
> -
>  	return ret;
>  }
>  device_initcall(arm_idle_init);
> 

Thanks,
Rafael