[resend] cpuidle: ACPI processor idle regression

[resend] cpuidle: ACPI processor idle regression

[Daniel Lezcano]

Sorry, a bad copy/paste address for Rafael.

----

Hi all,

while testing the processor_idle for ACPI, I noticed the states usage is
not used for cpuidle in sysfs.

Reproduced on Lenovo x230, with a i7-3520M CPU @ 2.90GHz without the
intel_idle driver.

After git-bisecting, the commit where this regression occurs is:

ac212b6980d8d5eda705864fc5a8ecddc6d6eacc is the first bad commit
commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Fri May 3 00:26:22 2013 +0200

    ACPI / processor: Use common hotplug infrastructure

    Split the ACPI processor driver into two parts, one that is
    non-modular, resides in the ACPI core and handles the enumeration
    and hotplug of processors and one that implements the rest of the
    existing processor driver functionality.

    The non-modular part uses an ACPI scan handler object to enumerate
    processors on the basis of information provided by the ACPI namespace
    and to hook up with the common ACPI hotplug infrastructure.  It also
    populates the ACPI handle of each processor device having a
    corresponding object in the ACPI namespace, which allows the driver
    proper to bind to those devices, and makes the driver bind to them
    if it is readily available (i.e. loaded) when the scan handler's
    .attach() routine is running.

    There are a few reasons to make this change.

    First, switching the ACPI processor driver to using the common ACPI
    hotplug infrastructure reduces code duplication and size considerably,
    even though a new file is created along with a header comment etc.

    Second, since the common hotplug code attempts to offline devices
    before starting the (non-reversible) removal procedure, it will abort
    (and possibly roll back) hot-remove operations involving processors
    if cpu_down() returns an error code for one of them instead of
    continuing them blindly (if /sys/firmware/acpi/hotplug/force_remove
    is unset).  That is a more desirable behavior than what the current
    code does.

    Finally, the separation of the scan/hotplug part from the driver
    proper makes it possible to simplify the driver's .remove() routine,
    because it doesn't need to worry about the possible cleanup related
    to processor removal any more (the scan/hotplug part is responsible
    for that now) and can handle device removal and driver removal
    symmetricaly (i.e. as appropriate).

    Some user-visible changes in sysfs are made (for example, the
    'sysdev' link from the ACPI device node to the processor device's
    directory is gone and a 'physical_node' link is present instead
    and a corresponding 'firmware_node' is present in the processor
    device's directory, the processor driver is now visible under
    /sys/bus/cpu/drivers/ and bound to the processor device), but
    that shouldn't affect the functionality that users care about
    (frequency scaling, C-states and thermal management).

    Tested on my venerable Toshiba Portege R500.

    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
    Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Reviewed-by: Toshi Kani <toshi.kani@hp.com>


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [resend] cpuidle: ACPI processor idle regression

[Rafael J. Wysocki]

On Thursday, May 30, 2013 03:46:24 PM Daniel Lezcano wrote:
> 
> Sorry, a bad copy/paste address for Rafael.
> 
> ----
> 
> Hi all,
> 
> while testing the processor_idle for ACPI, I noticed the states usage is
> not used for cpuidle in sysfs.
> 
> Reproduced on Lenovo x230, with a i7-3520M CPU @ 2.90GHz without the
> intel_idle driver.
> 
> After git-bisecting, the commit where this regression occurs is:

Reproduced and investigating that.

Thanks,
Rafael


> ac212b6980d8d5eda705864fc5a8ecddc6d6eacc is the first bad commit
> commit ac212b6980d8d5eda705864fc5a8ecddc6d6eacc
> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Date:   Fri May 3 00:26:22 2013 +0200
> 
>     ACPI / processor: Use common hotplug infrastructure
> 
>     Split the ACPI processor driver into two parts, one that is
>     non-modular, resides in the ACPI core and handles the enumeration
>     and hotplug of processors and one that implements the rest of the
>     existing processor driver functionality.
> 
>     The non-modular part uses an ACPI scan handler object to enumerate
>     processors on the basis of information provided by the ACPI namespace
>     and to hook up with the common ACPI hotplug infrastructure.  It also
>     populates the ACPI handle of each processor device having a
>     corresponding object in the ACPI namespace, which allows the driver
>     proper to bind to those devices, and makes the driver bind to them
>     if it is readily available (i.e. loaded) when the scan handler's
>     .attach() routine is running.
> 
>     There are a few reasons to make this change.
> 
>     First, switching the ACPI processor driver to using the common ACPI
>     hotplug infrastructure reduces code duplication and size considerably,
>     even though a new file is created along with a header comment etc.
> 
>     Second, since the common hotplug code attempts to offline devices
>     before starting the (non-reversible) removal procedure, it will abort
>     (and possibly roll back) hot-remove operations involving processors
>     if cpu_down() returns an error code for one of them instead of
>     continuing them blindly (if /sys/firmware/acpi/hotplug/force_remove
>     is unset).  That is a more desirable behavior than what the current
>     code does.
> 
>     Finally, the separation of the scan/hotplug part from the driver
>     proper makes it possible to simplify the driver's .remove() routine,
>     because it doesn't need to worry about the possible cleanup related
>     to processor removal any more (the scan/hotplug part is responsible
>     for that now) and can handle device removal and driver removal
>     symmetricaly (i.e. as appropriate).
> 
>     Some user-visible changes in sysfs are made (for example, the
>     'sysdev' link from the ACPI device node to the processor device's
>     directory is gone and a 'physical_node' link is present instead
>     and a corresponding 'firmware_node' is present in the processor
>     device's directory, the processor driver is now visible under
>     /sys/bus/cpu/drivers/ and bound to the processor device), but
>     that shouldn't affect the functionality that users care about
>     (frequency scaling, C-states and thermal management).
> 
>     Tested on my venerable Toshiba Portege R500.
> 
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>     Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>     Reviewed-by: Toshi Kani <toshi.kani@hp.com>
> 
> 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [resend] cpuidle: ACPI processor idle regression

[Rafael J. Wysocki]

On Thursday, May 30, 2013 04:56:18 PM Rafael J. Wysocki wrote:
> On Thursday, May 30, 2013 03:46:24 PM Daniel Lezcano wrote:
> > 
> > Sorry, a bad copy/paste address for Rafael.
> > 
> > ----
> > 
> > Hi all,
> > 
> > while testing the processor_idle for ACPI, I noticed the states usage is
> > not used for cpuidle in sysfs.
> > 
> > Reproduced on Lenovo x230, with a i7-3520M CPU @ 2.90GHz without the
> > intel_idle driver.
> > 
> > After git-bisecting, the commit where this regression occurs is:
> 
> Reproduced and investigating that.

The appended patch fixes this for me.  Can you please verify?

Rafael


---
 drivers/acpi/acpi_processor.c   |    5 +++++
 drivers/acpi/processor_driver.c |    3 ---
 2 files changed, 5 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/acpi/processor_driver.c
===================================================================
--- linux-pm.orig/drivers/acpi/processor_driver.c
+++ linux-pm/drivers/acpi/processor_driver.c
@@ -78,9 +78,6 @@ static struct device_driver acpi_process
 	.remove = acpi_processor_stop,
 };
 
-DEFINE_PER_CPU(struct acpi_processor *, processors);
-EXPORT_PER_CPU_SYMBOL(processors);
-
 static void acpi_processor_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct acpi_device *device = data;
Index: linux-pm/drivers/acpi/acpi_processor.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_processor.c
+++ linux-pm/drivers/acpi/acpi_processor.c
@@ -29,6 +29,9 @@
 
 ACPI_MODULE_NAME("processor");
 
+DEFINE_PER_CPU(struct acpi_processor *, processors);
+EXPORT_PER_CPU_SYMBOL(processors);
+
 /* --------------------------------------------------------------------------
                                 Errata Handling
    -------------------------------------------------------------------------- */
@@ -387,6 +390,7 @@ static int __cpuinit acpi_processor_add(
 	 * checks.
 	 */
 	per_cpu(processor_device_array, pr->id) = device;
+	per_cpu(processors, pr->id) = pr;
 
 	dev = get_cpu_device(pr->id);
 	result = acpi_bind_one(dev, pr->handle);
@@ -406,6 +410,7 @@ static int __cpuinit acpi_processor_add(
  err:
 	free_cpumask_var(pr->throttling.shared_cpu_map);
 	device->driver_data = NULL;
+	per_cpu(processors, pr->id) = NULL;
  err_free_pr:
 	kfree(pr);
 	return result;

Re: [resend] cpuidle: ACPI processor idle regression

[Daniel Lezcano]

On 05/30/2013 06:35 PM, Rafael J. Wysocki wrote:
> On Thursday, May 30, 2013 04:56:18 PM Rafael J. Wysocki wrote:
>> On Thursday, May 30, 2013 03:46:24 PM Daniel Lezcano wrote:
>>>
>>> Sorry, a bad copy/paste address for Rafael.
>>>
>>> ----
>>>
>>> Hi all,
>>>
>>> while testing the processor_idle for ACPI, I noticed the states usage is
>>> not used for cpuidle in sysfs.
>>>
>>> Reproduced on Lenovo x230, with a i7-3520M CPU @ 2.90GHz without the
>>> intel_idle driver.
>>>
>>> After git-bisecting, the commit where this regression occurs is:
>>
>> Reproduced and investigating that.
> 
> The appended patch fixes this for me.  Can you please verify?

Yes, verified. It fixes the issue for me too.

Thanks !

> ---
>  drivers/acpi/acpi_processor.c   |    5 +++++
>  drivers/acpi/processor_driver.c |    3 ---
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/acpi/processor_driver.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/processor_driver.c
> +++ linux-pm/drivers/acpi/processor_driver.c
> @@ -78,9 +78,6 @@ static struct device_driver acpi_process
>  	.remove = acpi_processor_stop,
>  };
>  
> -DEFINE_PER_CPU(struct acpi_processor *, processors);
> -EXPORT_PER_CPU_SYMBOL(processors);
> -
>  static void acpi_processor_notify(acpi_handle handle, u32 event, void *data)
>  {
>  	struct acpi_device *device = data;
> Index: linux-pm/drivers/acpi/acpi_processor.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_processor.c
> +++ linux-pm/drivers/acpi/acpi_processor.c
> @@ -29,6 +29,9 @@
>  
>  ACPI_MODULE_NAME("processor");
>  
> +DEFINE_PER_CPU(struct acpi_processor *, processors);
> +EXPORT_PER_CPU_SYMBOL(processors);
> +
>  /* --------------------------------------------------------------------------
>                                  Errata Handling
>     -------------------------------------------------------------------------- */
> @@ -387,6 +390,7 @@ static int __cpuinit acpi_processor_add(
>  	 * checks.
>  	 */
>  	per_cpu(processor_device_array, pr->id) = device;
> +	per_cpu(processors, pr->id) = pr;
>  
>  	dev = get_cpu_device(pr->id);
>  	result = acpi_bind_one(dev, pr->handle);
> @@ -406,6 +410,7 @@ static int __cpuinit acpi_processor_add(
>   err:
>  	free_cpumask_var(pr->throttling.shared_cpu_map);
>  	device->driver_data = NULL;
> +	per_cpu(processors, pr->id) = NULL;
>   err_free_pr:
>  	kfree(pr);
>  	return result;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog