Re: [PATCH] acpi, cpuidle: Register with cpuidle even if cpu is onlined after boot (beyond maxcpus)

Re: [PATCH] acpi, cpuidle: Register with cpuidle even if cpu is onlined after boot (beyond maxcpus)

[Thomas Renninger]

On Monday, June 25, 2012 01:25:43 PM Srivatsa S. Bhat wrote:
> 
> Daniel Lezcano noticed that after booting with maxcpus=X, if we online the
> remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then
> for the newly onlined cpus, the cpuidle directory is not found under
> /sys/devices/system/cpu/cpuY.
> 
> Partly, the reason for this is that acpi restricts the initialization to cpus
> within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi
> processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is
> used to restrict the number of cpus brought up during boot. That doesn't mean
> that we should hard restrict the bring up of the remaining cpus later on.

Sorry, but IMO it exaclty does mean that (adding more general lists for
further comments).

If you can online more cores than maxcpus= via sysfs, this sounds like a bug.
Not the other way around.

Compare with Documentation/kernel-parameters.txt:
        maxcpus=        [SMP] Maximum number of processors that an SMP kernel
                        should make use of.  maxcpus=n : n >= 0 limits the
                        kernel to using 'n' processors.  n=0 is a special case,
                        it is equivalent to "nosmp", which also disables
                        the IO APIC.

Chances that you run into more problems are high.
It would help if it is explained why at all this would be needed.

If there really is a valid use-case, possibly a bootcpus= param could get
defined or above documentation is adjusted. But this would need thorough
double checking, because I expect maxcpus=X was never intended to bring up
more than X cores later via sysfs and I expect there are more
places where things have to get ajusted.

    Thomas

> So
> teach acpi to play nice with maxcpus= parameter, and perform the initialization
> for all present cpus, but defer their startup to the point when they are
> actually onlined later.
> 
> But that alone won't suffice as a fix, because there is one more thing that
> the present but !online cpus lack - the need_hotplug_init flag.
> The need_hotplug_init flag is set only for physically hotplugged cpus and not
> for cpus which were already present but not brought online during boot (due to
> the maxcpus= parameter). For cpus with this flag set, during the online
> operation, acpi_cpu_soft_notify() takes care of the registration with cpuidle.
> So, in order to allow the present but !online cpus to be onlined later with
> full features (by making use of acpi_cpu_soft_notify()), set their
> need_hotplug_init flag.
> 
> Reported-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> 
> ---
> 
> Daniel, this patch works for me. Does it work for you as well?
> 
>  drivers/acpi/processor_driver.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 0734086..f29d30f 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -551,11 +551,6 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>  		return 0;
>  	}
>  
> -#ifdef CONFIG_SMP
> -	if (pr->id >= setup_max_cpus && pr->id != 0)
> -		return 0;
> -#endif
> -
>  	BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
>  
>  	/*
> @@ -580,6 +575,17 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>  		goto err_clear_processor;
>  	}
>  
> +#ifdef CONFIG_SMP
> +	if (pr->id >= setup_max_cpus && pr->id != 0) {
> +		/*
> +		 * Don't start cpus beyond maxcpus now. But allow them to
> +		 * to be brought online later.
> +		 */
> +		pr->flags.need_hotplug_init = 1;
> +		return 0;
> +	}
> +#endif
> +
>  	/*
>  	 * Do not start hotplugged CPUs now, but when they
>  	 * are onlined the first time
> 
> 
> 

Re: [PATCH] acpi, cpuidle: Register with cpuidle even if cpu is onlined after boot (beyond maxcpus)

[Srivatsa S. Bhat]

On 06/25/2012 07:23 PM, Thomas Renninger wrote:

> On Monday, June 25, 2012 01:25:43 PM Srivatsa S. Bhat wrote:
>>
>> Daniel Lezcano noticed that after booting with maxcpus=X, if we online the
>> remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then
>> for the newly onlined cpus, the cpuidle directory is not found under
>> /sys/devices/system/cpu/cpuY.
>>
>> Partly, the reason for this is that acpi restricts the initialization to cpus
>> within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi
>> processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is
>> used to restrict the number of cpus brought up during boot. That doesn't mean
>> that we should hard restrict the bring up of the remaining cpus later on.
> 
> Sorry, but IMO it exaclty does mean that (adding more general lists for
> further comments).
> 
> If you can online more cores than maxcpus= via sysfs, this sounds like a bug.
> Not the other way around.
> 
> Compare with Documentation/kernel-parameters.txt:
>         maxcpus=        [SMP] Maximum number of processors that an SMP kernel
>                         should make use of.  maxcpus=n : n >= 0 limits the
>                         kernel to using 'n' processors.  n=0 is a special case,
>                         it is equivalent to "nosmp", which also disables
>                         the IO APIC.
> 
> Chances that you run into more problems are high.


Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and
should forbid any new cpus from coming online, but in the interest of avoiding
problems/complications in some obscure paths, I guess it makes sense to avoid
onlining new cpus beyond maxcpus.

In any case, I was just trying to see why the simple removal of the setup_max_cpus
check in acpi_processor_add() wasn't enough to expose the cpuidle directories under
the new cpus.. and while debugging that, I came up with this patch. I don't mind
if this doesn't get picked up.

> It would help if it is explained why at all this would be needed.
> 


Right, the usecase of why somebody would like to online new cpus beyond maxcpus
doesn't look all that solid anyway. So I am OK with leaving the code as it is now.

> If there really is a valid use-case, possibly a bootcpus= param could get
> defined or above documentation is adjusted. But this would need thorough
> double checking, because I expect maxcpus=X was never intended to bring up
> more than X cores later via sysfs and I expect there are more
> places where things have to get ajusted.
> 


Yep, that sounds reasonable.

Regards,
Srivatsa S. Bhat

> 
>> So
>> teach acpi to play nice with maxcpus= parameter, and perform the initialization
>> for all present cpus, but defer their startup to the point when they are
>> actually onlined later.
>>
>> But that alone won't suffice as a fix, because there is one more thing that
>> the present but !online cpus lack - the need_hotplug_init flag.
>> The need_hotplug_init flag is set only for physically hotplugged cpus and not
>> for cpus which were already present but not brought online during boot (due to
>> the maxcpus= parameter). For cpus with this flag set, during the online
>> operation, acpi_cpu_soft_notify() takes care of the registration with cpuidle.
>> So, in order to allow the present but !online cpus to be onlined later with
>> full features (by making use of acpi_cpu_soft_notify()), set their
>> need_hotplug_init flag.
>>
>> Reported-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>>
>> ---
>>
>> Daniel, this patch works for me. Does it work for you as well?
>>
>>  drivers/acpi/processor_driver.c |   16 +++++++++++-----
>>  1 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 0734086..f29d30f 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -551,11 +551,6 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>>  		return 0;
>>  	}
>>  
>> -#ifdef CONFIG_SMP
>> -	if (pr->id >= setup_max_cpus && pr->id != 0)
>> -		return 0;
>> -#endif
>> -
>>  	BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
>>  
>>  	/*
>> @@ -580,6 +575,17 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>>  		goto err_clear_processor;
>>  	}
>>  
>> +#ifdef CONFIG_SMP
>> +	if (pr->id >= setup_max_cpus && pr->id != 0) {
>> +		/*
>> +		 * Don't start cpus beyond maxcpus now. But allow them to
>> +		 * to be brought online later.
>> +		 */
>> +		pr->flags.need_hotplug_init = 1;
>> +		return 0;
>> +	}
>> +#endif
>> +
>>  	/*
>>  	 * Do not start hotplugged CPUs now, but when they
>>  	 * are onlined the first time
>>
>>
>>

Re: [PATCH] acpi, cpuidle: Register with cpuidle even if cpu is onlined after boot (beyond maxcpus)

[Thomas Renninger]

On Monday, June 25, 2012 06:03:42 PM Srivatsa S. Bhat wrote:
> On 06/25/2012 07:23 PM, Thomas Renninger wrote:
> 
> > On Monday, June 25, 2012 01:25:43 PM Srivatsa S. Bhat wrote:
> >>
> >> Daniel Lezcano noticed that after booting with maxcpus=X, if we online the
> >> remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then
> >> for the newly onlined cpus, the cpuidle directory is not found under
> >> /sys/devices/system/cpu/cpuY.
> >>
> >> Partly, the reason for this is that acpi restricts the initialization to cpus
> >> within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi
> >> processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is
> >> used to restrict the number of cpus brought up during boot. That doesn't mean
> >> that we should hard restrict the bring up of the remaining cpus later on.
> > 
> > Sorry, but IMO it exaclty does mean that (adding more general lists for
> > further comments).
> > 
> > If you can online more cores than maxcpus= via sysfs, this sounds like a bug.
> > Not the other way around.
> > 
> > Compare with Documentation/kernel-parameters.txt:
> >         maxcpus=        [SMP] Maximum number of processors that an SMP kernel
> >                         should make use of.  maxcpus=n : n >= 0 limits the
> >                         kernel to using 'n' processors.  n=0 is a special case,
> >                         it is equivalent to "nosmp", which also disables
> >                         the IO APIC.
> > 
> > Chances that you run into more problems are high.
> 
> 
> Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and
> should forbid any new cpus from coming online, but in the interest of avoiding
> problems/complications in some obscure paths, I guess it makes sense to avoid
> onlining new cpus beyond maxcpus.

Yep, for such reasons:
   - That nobody realizes this to be useful and makes use of it in a productive
     environment
   - If I see maxcpus=X in a bugreport's dmesg command line,
     I want to be sure that's true.
   - To enforce that things work as documented


Wow, after looking a bit into this I found (Documentation/cpu-hotplug.txt):

maxcpus=n    Restrict boot time cpus to n. Say if you have 4 cpus, using
             maxcpus=2 will only boot 2. You can choose to bring the
             other cpus later online, read FAQ's for more info.

Looks like someone already documented this (IMO broken) behavior.
I didn't find further info in the FAQs.

> In any case, I was just trying to see why the simple removal of the setup_max_cpus
> check in acpi_processor_add() wasn't enough to expose the cpuidle directories under
> the new cpus.. and while debugging that, I came up with this patch. I don't mind
> if this doesn't get picked up.

> Right, the usecase of why somebody would like to online new cpus beyond maxcpus
> doesn't look all that solid anyway. So I am OK with leaving the code as it is now.

In the end this is a debug option, I expect everybody is aware of that.
Yep, let's just leave it...

   Thomas

Re: [PATCH] acpi, cpuidle: Register with cpuidle even if cpu is onlined after boot (beyond maxcpus)

[Daniel Lezcano]

On 06/26/2012 11:29 AM, Thomas Renninger wrote:
> On Monday, June 25, 2012 06:03:42 PM Srivatsa S. Bhat wrote:
>> On 06/25/2012 07:23 PM, Thomas Renninger wrote:
>>
>>> On Monday, June 25, 2012 01:25:43 PM Srivatsa S. Bhat wrote:
>>>>
>>>> Daniel Lezcano noticed that after booting with maxcpus=X, if we online the
>>>> remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then
>>>> for the newly onlined cpus, the cpuidle directory is not found under
>>>> /sys/devices/system/cpu/cpuY.
>>>>
>>>> Partly, the reason for this is that acpi restricts the initialization to cpus
>>>> within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi
>>>> processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is
>>>> used to restrict the number of cpus brought up during boot. That doesn't mean
>>>> that we should hard restrict the bring up of the remaining cpus later on.
>>>
>>> Sorry, but IMO it exaclty does mean that (adding more general lists for
>>> further comments).
>>>
>>> If you can online more cores than maxcpus= via sysfs, this sounds like a bug.
>>> Not the other way around.
>>>
>>> Compare with Documentation/kernel-parameters.txt:
>>>         maxcpus=        [SMP] Maximum number of processors that an SMP kernel
>>>                         should make use of.  maxcpus=n : n >= 0 limits the
>>>                         kernel to using 'n' processors.  n=0 is a special case,
>>>                         it is equivalent to "nosmp", which also disables
>>>                         the IO APIC.
>>>
>>> Chances that you run into more problems are high.
>>
>>
>> Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and
>> should forbid any new cpus from coming online, but in the interest of avoiding
>> problems/complications in some obscure paths, I guess it makes sense to avoid
>> onlining new cpus beyond maxcpus.
> 
> Yep, for such reasons:
>    - That nobody realizes this to be useful and makes use of it in a productive
>      environment
>    - If I see maxcpus=X in a bugreport's dmesg command line,
>      I want to be sure that's true.
>    - To enforce that things work as documented
> 
> 
> Wow, after looking a bit into this I found (Documentation/cpu-hotplug.txt):
> 
> maxcpus=n    Restrict boot time cpus to n. Say if you have 4 cpus, using
>              maxcpus=2 will only boot 2. You can choose to bring the
>              other cpus later online, read FAQ's for more info.
> 
> Looks like someone already documented this (IMO broken) behavior.
> I didn't find further info in the FAQs.
> 
>> In any case, I was just trying to see why the simple removal of the setup_max_cpus
>> check in acpi_processor_add() wasn't enough to expose the cpuidle directories under
>> the new cpus.. and while debugging that, I came up with this patch. I don't mind
>> if this doesn't get picked up.
> 
>> Right, the usecase of why somebody would like to online new cpus beyond maxcpus
>> doesn't look all that solid anyway. So I am OK with leaving the code as it is now.
> 
> In the end this is a debug option, I expect everybody is aware of that.
> Yep, let's just leave it...

In this case, let's remove the intel_idle_cpu_init stuff in
acpi_cpu_soft_notify, no ?


-- 
 <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: [PATCH] acpi, cpuidle: Register with cpuidle even if cpu is onlined after boot (beyond maxcpus)

[Srivatsa S. Bhat]

On 06/26/2012 03:11 PM, Daniel Lezcano wrote:
> On 06/26/2012 11:29 AM, Thomas Renninger wrote:
>> On Monday, June 25, 2012 06:03:42 PM Srivatsa S. Bhat wrote:
>>> On 06/25/2012 07:23 PM, Thomas Renninger wrote:
>>>
>>>> On Monday, June 25, 2012 01:25:43 PM Srivatsa S. Bhat wrote:
>>>>>
>>>>> Daniel Lezcano noticed that after booting with maxcpus=X, if we online the
>>>>> remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then
>>>>> for the newly onlined cpus, the cpuidle directory is not found under
>>>>> /sys/devices/system/cpu/cpuY.
>>>>>
>>>>> Partly, the reason for this is that acpi restricts the initialization to cpus
>>>>> within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi
>>>>> processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is
>>>>> used to restrict the number of cpus brought up during boot. That doesn't mean
>>>>> that we should hard restrict the bring up of the remaining cpus later on.
>>>>
>>>> Sorry, but IMO it exaclty does mean that (adding more general lists for
>>>> further comments).
>>>>
>>>> If you can online more cores than maxcpus= via sysfs, this sounds like a bug.
>>>> Not the other way around.
>>>>
>>>> Compare with Documentation/kernel-parameters.txt:
>>>>         maxcpus=        [SMP] Maximum number of processors that an SMP kernel
>>>>                         should make use of.  maxcpus=n : n >= 0 limits the
>>>>                         kernel to using 'n' processors.  n=0 is a special case,
>>>>                         it is equivalent to "nosmp", which also disables
>>>>                         the IO APIC.
>>>>
>>>> Chances that you run into more problems are high.
>>>
>>>
>>> Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and
>>> should forbid any new cpus from coming online, but in the interest of avoiding
>>> problems/complications in some obscure paths, I guess it makes sense to avoid
>>> onlining new cpus beyond maxcpus.
>>
>> Yep, for such reasons:
>>    - That nobody realizes this to be useful and makes use of it in a productive
>>      environment
>>    - If I see maxcpus=X in a bugreport's dmesg command line,
>>      I want to be sure that's true.
>>    - To enforce that things work as documented
>>
>>
>> Wow, after looking a bit into this I found (Documentation/cpu-hotplug.txt):
>>
>> maxcpus=n    Restrict boot time cpus to n. Say if you have 4 cpus, using
>>              maxcpus=2 will only boot 2. You can choose to bring the
>>              other cpus later online, read FAQ's for more info.
>>
>> Looks like someone already documented this (IMO broken) behavior.
>> I didn't find further info in the FAQs.
>>
>>> In any case, I was just trying to see why the simple removal of the setup_max_cpus
>>> check in acpi_processor_add() wasn't enough to expose the cpuidle directories under
>>> the new cpus.. and while debugging that, I came up with this patch. I don't mind
>>> if this doesn't get picked up.
>>
>>> Right, the usecase of why somebody would like to online new cpus beyond maxcpus
>>> doesn't look all that solid anyway. So I am OK with leaving the code as it is now.
>>
>> In the end this is a debug option, I expect everybody is aware of that.
>> Yep, let's just leave it...
> 
> In this case, let's remove the intel_idle_cpu_init stuff in
> acpi_cpu_soft_notify, no ?
> 

Why? And how would that help? The intel_idle_cpu_init() call is essential if intel_idle
driver is being used instead of acpi idle.

Regards,
Srivatsa S. Bhat

Re: [PATCH] acpi, cpuidle: Register with cpuidle even if cpu is onlined after boot (beyond maxcpus)

[Daniel Lezcano]

On 06/26/2012 11:58 AM, Srivatsa S. Bhat wrote:
> On 06/26/2012 03:11 PM, Daniel Lezcano wrote:
>> On 06/26/2012 11:29 AM, Thomas Renninger wrote:
>>> On Monday, June 25, 2012 06:03:42 PM Srivatsa S. Bhat wrote:
>>>> On 06/25/2012 07:23 PM, Thomas Renninger wrote:
>>>>
>>>>> On Monday, June 25, 2012 01:25:43 PM Srivatsa S. Bhat wrote:
>>>>>>
>>>>>> Daniel Lezcano noticed that after booting with maxcpus=X, if we online the
>>>>>> remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then
>>>>>> for the newly onlined cpus, the cpuidle directory is not found under
>>>>>> /sys/devices/system/cpu/cpuY.
>>>>>>
>>>>>> Partly, the reason for this is that acpi restricts the initialization to cpus
>>>>>> within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi
>>>>>> processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is
>>>>>> used to restrict the number of cpus brought up during boot. That doesn't mean
>>>>>> that we should hard restrict the bring up of the remaining cpus later on.
>>>>>
>>>>> Sorry, but IMO it exaclty does mean that (adding more general lists for
>>>>> further comments).
>>>>>
>>>>> If you can online more cores than maxcpus= via sysfs, this sounds like a bug.
>>>>> Not the other way around.
>>>>>
>>>>> Compare with Documentation/kernel-parameters.txt:
>>>>>         maxcpus=        [SMP] Maximum number of processors that an SMP kernel
>>>>>                         should make use of.  maxcpus=n : n >= 0 limits the
>>>>>                         kernel to using 'n' processors.  n=0 is a special case,
>>>>>                         it is equivalent to "nosmp", which also disables
>>>>>                         the IO APIC.
>>>>>
>>>>> Chances that you run into more problems are high.
>>>>
>>>>
>>>> Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and
>>>> should forbid any new cpus from coming online, but in the interest of avoiding
>>>> problems/complications in some obscure paths, I guess it makes sense to avoid
>>>> onlining new cpus beyond maxcpus.
>>>
>>> Yep, for such reasons:
>>>    - That nobody realizes this to be useful and makes use of it in a productive
>>>      environment
>>>    - If I see maxcpus=X in a bugreport's dmesg command line,
>>>      I want to be sure that's true.
>>>    - To enforce that things work as documented
>>>
>>>
>>> Wow, after looking a bit into this I found (Documentation/cpu-hotplug.txt):
>>>
>>> maxcpus=n    Restrict boot time cpus to n. Say if you have 4 cpus, using
>>>              maxcpus=2 will only boot 2. You can choose to bring the
>>>              other cpus later online, read FAQ's for more info.
>>>
>>> Looks like someone already documented this (IMO broken) behavior.
>>> I didn't find further info in the FAQs.
>>>
>>>> In any case, I was just trying to see why the simple removal of the setup_max_cpus
>>>> check in acpi_processor_add() wasn't enough to expose the cpuidle directories under
>>>> the new cpus.. and while debugging that, I came up with this patch. I don't mind
>>>> if this doesn't get picked up.
>>>
>>>> Right, the usecase of why somebody would like to online new cpus beyond maxcpus
>>>> doesn't look all that solid anyway. So I am OK with leaving the code as it is now.
>>>
>>> In the end this is a debug option, I expect everybody is aware of that.
>>> Yep, let's just leave it...
>>
>> In this case, let's remove the intel_idle_cpu_init stuff in
>> acpi_cpu_soft_notify, no ?
>>
> 
> Why? And how would that help? The intel_idle_cpu_init() call is essential if intel_idle
> driver is being used instead of acpi idle.

AFAIU, this code is not called after onlining a cpu greater than maxcpus
and Thomas thinks that system with cpu hotplug at runtime are not sold.

The problem I see with this code is acpi and intel-idle are tied
together now. I would like to break this dependency and use the notifier
to handle the cpu hotplug directly in intel-idle.

It is hard to test my patch as there is not such system and maxcpus is
not correctly handled here. I can use your patch to test my patch but
anyway ... I am just asking if that would make sense to remove this
portion of code instead :)

If we want to keep this code untouched, I can try my patch and maybe
Thomas will agreed to test it also on a cpu-online-runtime-system if he
has one.

Thanks
  -- Daniel

-- 
 <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: [PATCH] acpi, cpuidle: Register with cpuidle even if cpu is onlined after boot (beyond maxcpus)

[Thomas Renninger]

On Tuesday, June 26, 2012 12:42:14 PM Daniel Lezcano wrote:
> On 06/26/2012 11:58 AM, Srivatsa S. Bhat wrote:
> > On 06/26/2012 03:11 PM, Daniel Lezcano wrote:
> >> On 06/26/2012 11:29 AM, Thomas Renninger wrote:
...
> >>
> >> In this case, let's remove the intel_idle_cpu_init stuff in
> >> acpi_cpu_soft_notify, no ?
> >>
> > 
> > Why? And how would that help? The intel_idle_cpu_init() call is essential if intel_idle
> > driver is being used instead of acpi idle.
> 
> AFAIU, this code is not called after onlining a cpu greater than maxcpus
> and Thomas thinks that system with cpu hotplug at runtime are not sold.
Not 100% sure. Also the code paths to handle real CPU hotplug existed
already (via ACPI notify on the processor object) and did work.
I only fixed to correctly initialize idle states.

> The problem I see with this code is acpi and intel-idle are tied
> together now. I would like to break this dependency and use the notifier
> to handle the cpu hotplug directly in intel-idle.
> 
> It is hard to test my patch as there is not such system and maxcpus is
> not correctly handled here. I can use your patch to test my patch but
> anyway ... I am just asking if that would make sense to remove this
> portion of code instead :)
> 
> If we want to keep this code untouched, I can try my patch and maybe
> Thomas will agreed to test it also on a cpu-online-runtime-system if he
> has one.
But not this patch, we agreed it's not worth to look at:
"System exceeding maxcpus=x via cpu soft onlining does not initialize
power management on exceeding cores", right?

If you have a patch touching this, please point me to it.
I can have a look at it and if really necessary give it a test.

   Thomas

[PATCH] acpi: intel_idle : break dependency between modules

[Daniel Lezcano]

When the system is booted with some cpus offline, the idle
driver is not initialized. When a cpu is set online, the
acpi code call the intel idle init function. Unfortunately
this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the
notifier of intel_idle. In order to make it work, the notifier
must be initialized in the right order, acpi then intel_idle.
This is done in the Makefile. This patch has the benefit of
encapsulating the intel_idle driver and remove some exported
functions.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/Makefile                |    3 ++-
 drivers/acpi/processor_driver.c |    7 -------
 drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
 include/linux/cpuidle.h         |    7 -------
 4 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 2ba29ff..a2454b8 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -12,8 +12,9 @@ obj-$(CONFIG_PCI)		+= pci/
 obj-$(CONFIG_PARISC)		+= parisc/
 obj-$(CONFIG_RAPIDIO)		+= rapidio/
 obj-y				+= video/
-obj-y				+= idle/
+# acpi must come before idle for initialization
 obj-$(CONFIG_ACPI)		+= acpi/
+obj-y				+= idle/
 obj-$(CONFIG_SFI)		+= sfi/
 # PnP must come after ACPI since it will eventually need to check if acpi
 # was used and do nothing if so
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 0734086..8648b29 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		 * Initialize missing things
 		 */
 		if (pr->flags.need_hotplug_init) {
-			struct cpuidle_driver *idle_driver =
-				cpuidle_get_driver();
-
 			printk(KERN_INFO "Will online and init hotplugged "
 			       "CPU: %d\n", pr->id);
 			WARN(acpi_processor_start(pr), "Failed to start CPU:"
 				" %d\n", pr->id);
 			pr->flags.need_hotplug_init = 0;
-			if (idle_driver && !strcmp(idle_driver->name,
-						   "intel_idle")) {
-				intel_idle_cpu_init(pr->id);
-			}
 		/* Normal CPU soft online event */
 		} else {
 			acpi_processor_ppc_has_changed(pr, 0);
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index d0f59c3..4c36039 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 static int intel_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv, int index);
+static int intel_idle_cpu_init(int cpu);
 
 static struct cpuidle_state *cpuidle_state_table;
 
@@ -302,22 +303,28 @@ static void __setup_broadcast_timer(void *arg)
 	clockevents_notify(reason, &cpu);
 }
 
-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;
 
 	switch (action & 0xf) {
 	case CPU_ONLINE:
 		smp_call_function_single(hotcpu, __setup_broadcast_timer,
 			(void *)true, 1);
+
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
+		if (!dev->registered)
+			intel_idle_cpu_init(hotcpu);
+
 		break;
 	}
 	return NOTIFY_OK;
 }
 
-static struct notifier_block setup_broadcast_notifier = {
-	.notifier_call = setup_broadcast_cpuhp_notify,
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
 };
 
 static void auto_demotion_disable(void *dummy)
@@ -407,7 +414,7 @@ static int intel_idle_probe(void)
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
 	else {
 		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-		register_cpu_notifier(&setup_broadcast_notifier);
+		register_cpu_notifier(&cpu_hotplug_notifier);
 	}
 
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
@@ -494,7 +501,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;
@@ -539,7 +546,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {
@@ -583,7 +589,7 @@ static void __exit intel_idle_exit(void)
 
 	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-		unregister_cpu_notifier(&setup_broadcast_notifier);
+		unregister_cpu_notifier(&cpu_hotplug_notifier);
 	}
 
 	return;
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 5ab7183..66d7e0d 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -213,14 +213,7 @@ struct cpuidle_governor {
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 
-#ifdef CONFIG_INTEL_IDLE
-extern int intel_idle_cpu_init(int cpu);
 #else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
-#endif
-
-#else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
 
 static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}
-- 
1.7.5.4

Re: [PATCH] acpi: intel_idle : break dependency between modules

[Thomas Renninger]

Hi,

I agree that such a dependency between 2 modules is not
nice. But your patch will have bad side-effects (see comments
embedded below).

On Wednesday, June 27, 2012 11:07:48 AM Daniel Lezcano wrote:
> When the system is booted with some cpus offline, the idle
> driver is not initialized. When a cpu is set online, the
> acpi code call the intel idle init function. Unfortunately
> this code introduce a dependency between intel_idle and acpi.
> 
> This patch is intended to remove this dependency by using the
> notifier of intel_idle. In order to make it work, the notifier
> must be initialized in the right order, acpi then intel_idle.
> This is done in the Makefile. This patch has the benefit of
> encapsulating the intel_idle driver and remove some exported
> functions.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/Makefile                |    3 ++-
>  drivers/acpi/processor_driver.c |    7 -------
>  drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
>  include/linux/cpuidle.h         |    7 -------
>  4 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 2ba29ff..a2454b8 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -12,8 +12,9 @@ obj-$(CONFIG_PCI)		+= pci/
>  obj-$(CONFIG_PARISC)		+= parisc/
>  obj-$(CONFIG_RAPIDIO)		+= rapidio/
>  obj-y				+= video/
> -obj-y				+= idle/
> +# acpi must come before idle for initialization
>  obj-$(CONFIG_ACPI)		+= acpi/
> +obj-y				+= idle/
This breaks intel_idle.
Loading order defines which one comes first and is used: intel_idle
or ACPI processor cpuidle driver.
With above, one would get acpi_idle cpuidle driver if both are
compiled in, instead of the intel_idle one.

Why exactly is this necessary, couldn't it just work?

>  obj-$(CONFIG_SFI)		+= sfi/
>  # PnP must come after ACPI since it will eventually need to check if acpi
>  # was used and do nothing if so
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 0734086..8648b29 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>  		 * Initialize missing things
>  		 */
>  		if (pr->flags.need_hotplug_init) {
> -			struct cpuidle_driver *idle_driver =
> -				cpuidle_get_driver();
> -
>  			printk(KERN_INFO "Will online and init hotplugged "
>  			       "CPU: %d\n", pr->id);
>  			WARN(acpi_processor_start(pr), "Failed to start CPU:"
>  				" %d\n", pr->id);
>  			pr->flags.need_hotplug_init = 0;
> -			if (idle_driver && !strcmp(idle_driver->name,
> -						   "intel_idle")) {
> -				intel_idle_cpu_init(pr->id);
> -			}
>  		/* Normal CPU soft online event */
>  		} else {
>  			acpi_processor_ppc_has_changed(pr, 0);
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index d0f59c3..4c36039 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
>  static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
>  static int intel_idle(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv, int index);
> +static int intel_idle_cpu_init(int cpu);
>  
>  static struct cpuidle_state *cpuidle_state_table;
>  
> @@ -302,22 +303,28 @@ static void __setup_broadcast_timer(void *arg)
>  	clockevents_notify(reason, &cpu);
>  }
>  
> -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
> -		unsigned long action, void *hcpu)
> +static int cpu_hotplug_notify(struct notifier_block *n,
> +			      unsigned long action, void *hcpu)
>  {
>  	int hotcpu = (unsigned long)hcpu;
> +	struct cpuidle_device *dev;
>  
>  	switch (action & 0xf) {
>  	case CPU_ONLINE:
>  		smp_call_function_single(hotcpu, __setup_broadcast_timer,
>  			(void *)true, 1);
> +
> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
> +		if (!dev->registered)
> +			intel_idle_cpu_init(hotcpu);
> +
A small comment why this can happen and needs to be done
(real hotplugged cpu case) might help here later.

>  		break;
>  	}
>  	return NOTIFY_OK;
>  }
>  
> -static struct notifier_block setup_broadcast_notifier = {
> -	.notifier_call = setup_broadcast_cpuhp_notify,
> +static struct notifier_block cpu_hotplug_notifier = {
> +	.notifier_call = cpu_hotplug_notify,
>  };
>  
>  static void auto_demotion_disable(void *dummy)
> @@ -407,7 +414,7 @@ static int intel_idle_probe(void)
>  		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
>  	else {
>  		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
> -		register_cpu_notifier(&setup_broadcast_notifier);
> +		register_cpu_notifier(&cpu_hotplug_notifier);

The notifier always has to be registered now, not only in:
if (boot_cpu_has(X86_FEATURE_ARAT))     /* Always Reliable APIC Timer */
case.

>  	}
>  
>  	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
> @@ -494,7 +501,7 @@ static int intel_idle_cpuidle_driver_init(void)
>   * allocate, initialize, register cpuidle_devices
>   * @cpu: cpu/core to initialize
>   */
> -int intel_idle_cpu_init(int cpu)
> +static int intel_idle_cpu_init(int cpu)
>  {
>  	int cstate;
>  	struct cpuidle_device *dev;
> @@ -539,7 +546,6 @@ int intel_idle_cpu_init(int cpu)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
>  
>  static int __init intel_idle_init(void)
>  {
> @@ -583,7 +589,7 @@ static void __exit intel_idle_exit(void)
>  
>  	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
>  		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
> -		unregister_cpu_notifier(&setup_broadcast_notifier);
> +		unregister_cpu_notifier(&cpu_hotplug_notifier);
Same.

Re: [PATCH] acpi: intel_idle : break dependency between modules

[Daniel Lezcano]

On 06/27/2012 03:06 PM, Thomas Renninger wrote:
> Hi,
> 
> I agree that such a dependency between 2 modules is not
> nice. But your patch will have bad side-effects (see comments
> embedded below).
> 
> On Wednesday, June 27, 2012 11:07:48 AM Daniel Lezcano wrote:
>> When the system is booted with some cpus offline, the idle
>> driver is not initialized. When a cpu is set online, the
>> acpi code call the intel idle init function. Unfortunately
>> this code introduce a dependency between intel_idle and acpi.
>>
>> This patch is intended to remove this dependency by using the
>> notifier of intel_idle. In order to make it work, the notifier
>> must be initialized in the right order, acpi then intel_idle.
>> This is done in the Makefile. This patch has the benefit of
>> encapsulating the intel_idle driver and remove some exported
>> functions.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/Makefile                |    3 ++-
>>  drivers/acpi/processor_driver.c |    7 -------
>>  drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
>>  include/linux/cpuidle.h         |    7 -------
>>  4 files changed, 16 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 2ba29ff..a2454b8 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -12,8 +12,9 @@ obj-$(CONFIG_PCI)		+= pci/
>>  obj-$(CONFIG_PARISC)		+= parisc/
>>  obj-$(CONFIG_RAPIDIO)		+= rapidio/
>>  obj-y				+= video/
>> -obj-y				+= idle/
>> +# acpi must come before idle for initialization
>>  obj-$(CONFIG_ACPI)		+= acpi/
>> +obj-y				+= idle/
> This breaks intel_idle.
> Loading order defines which one comes first and is used: intel_idle
> or ACPI processor cpuidle driver.
> With above, one would get acpi_idle cpuidle driver if both are
> compiled in, instead of the intel_idle one.
> 
> Why exactly is this necessary, couldn't it just work?

[...]

I just wanted to keep same order. If it is not necessary, I won't invert
the compilation order in the next patch.

>> -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
>> -		unsigned long action, void *hcpu)
>> +static int cpu_hotplug_notify(struct notifier_block *n,
>> +			      unsigned long action, void *hcpu)
>>  {
>>  	int hotcpu = (unsigned long)hcpu;
>> +	struct cpuidle_device *dev;
>>  
>>  	switch (action & 0xf) {
>>  	case CPU_ONLINE:
>>  		smp_call_function_single(hotcpu, __setup_broadcast_timer,
>>  			(void *)true, 1);
>> +
>> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
>> +		if (!dev->registered)
>> +			intel_idle_cpu_init(hotcpu);
>> +
> A small comment why this can happen and needs to be done
> (real hotplugged cpu case) might help here later.

Yes, that makes sense.

[...]

>>  static void auto_demotion_disable(void *dummy)
>> @@ -407,7 +414,7 @@ static int intel_idle_probe(void)
>>  		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
>>  	else {
>>  		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
>> -		register_cpu_notifier(&setup_broadcast_notifier);
>> +		register_cpu_notifier(&cpu_hotplug_notifier);
> 
> The notifier always has to be registered now, not only in:
> if (boot_cpu_has(X86_FEATURE_ARAT))     /* Always Reliable APIC Timer */
> case.

Oops, right.


>>  	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
>> @@ -494,7 +501,7 @@ static int intel_idle_cpuidle_driver_init(void)
>>   * allocate, initialize, register cpuidle_devices
>>   * @cpu: cpu/core to initialize
>>   */
>> -int intel_idle_cpu_init(int cpu)
>> +static int intel_idle_cpu_init(int cpu)
>>  {
>>  	int cstate;
>>  	struct cpuidle_device *dev;
>> @@ -539,7 +546,6 @@ int intel_idle_cpu_init(int cpu)
>>  
>>  	return 0;
>>  }
>> -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
>>  
>>  static int __init intel_idle_init(void)
>>  {
>> @@ -583,7 +589,7 @@ static void __exit intel_idle_exit(void)
>>  
>>  	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
>>  		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
>> -		unregister_cpu_notifier(&setup_broadcast_notifier);
>> +		unregister_cpu_notifier(&cpu_hotplug_notifier);
> Same.


Thanks for the review !

  -- Daniel

-- 
 <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

[PATCH v2] acpi: intel_idle : break dependency between modules

[Daniel Lezcano]

When the system is booted with some cpus offline, the idle
driver is not initialized. When a cpu is set online, the
acpi code call the intel idle init function. Unfortunately
this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the
notifier of intel_idle. This patch has the benefit of
encapsulating the intel_idle driver and remove some exported
functions.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/acpi/processor_driver.c |    7 ------
 drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
 include/linux/cpuidle.h         |    7 ------
 3 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 0734086..8648b29 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		 * Initialize missing things
 		 */
 		if (pr->flags.need_hotplug_init) {
-			struct cpuidle_driver *idle_driver =
-				cpuidle_get_driver();
-
 			printk(KERN_INFO "Will online and init hotplugged "
 			       "CPU: %d\n", pr->id);
 			WARN(acpi_processor_start(pr), "Failed to start CPU:"
 				" %d\n", pr->id);
 			pr->flags.need_hotplug_init = 0;
-			if (idle_driver && !strcmp(idle_driver->name,
-						   "intel_idle")) {
-				intel_idle_cpu_init(pr->id);
-			}
 		/* Normal CPU soft online event */
 		} else {
 			acpi_processor_ppc_has_changed(pr, 0);
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index d0f59c3..fe95d54 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 static int intel_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv, int index);
+static int intel_idle_cpu_init(int cpu);
 
 static struct cpuidle_state *cpuidle_state_table;
 
@@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
 	clockevents_notify(reason, &cpu);
 }
 
-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;
 
 	switch (action & 0xf) {
 	case CPU_ONLINE:
-		smp_call_function_single(hotcpu, __setup_broadcast_timer,
-			(void *)true, 1);
+
+		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
+			smp_call_function_single(hotcpu, __setup_broadcast_timer,
+						 (void *)true, 1);
+
+		/*
+		 * Some systems can hotplug a cpu at runtime after
+		 * the kernel has booted, we have to initialize the
+		 * driver in this case
+		 */
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
+		if (!dev->registered)
+			intel_idle_cpu_init(hotcpu);
+
 		break;
 	}
 	return NOTIFY_OK;
 }
 
-static struct notifier_block setup_broadcast_notifier = {
-	.notifier_call = setup_broadcast_cpuhp_notify,
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
 };
 
 static void auto_demotion_disable(void *dummy)
@@ -405,10 +419,10 @@ static int intel_idle_probe(void)
 
 	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
-	else {
+	else
 		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-		register_cpu_notifier(&setup_broadcast_notifier);
-	}
+
+	register_cpu_notifier(&cpu_hotplug_notifier);
 
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
 		" model 0x%X\n", boot_cpu_data.x86_model);
@@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;
@@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {
@@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
 	intel_idle_cpuidle_devices_uninit();
 	cpuidle_unregister_driver(&intel_idle_driver);
 
-	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
+
+	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-		unregister_cpu_notifier(&setup_broadcast_notifier);
-	}
+	unregister_cpu_notifier(&cpu_hotplug_notifier);
 
 	return;
 }
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 5ab7183..66d7e0d 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -213,14 +213,7 @@ struct cpuidle_governor {
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 
-#ifdef CONFIG_INTEL_IDLE
-extern int intel_idle_cpu_init(int cpu);
 #else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
-#endif
-
-#else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
 
 static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}
-- 
1.7.5.4

Re: [PATCH v2] acpi: intel_idle : break dependency between modules

[Srivatsa S. Bhat]

On 06/28/2012 02:16 PM, Daniel Lezcano wrote:
> When the system is booted with some cpus offline, the idle
> driver is not initialized. When a cpu is set online, the
> acpi code call the intel idle init function. Unfortunately
> this code introduce a dependency between intel_idle and acpi.
> 
> This patch is intended to remove this dependency by using the
> notifier of intel_idle. This patch has the benefit of
> encapsulating the intel_idle driver and remove some exported
> functions.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>


Looks good to me.

Regards,
Srivatsa S. Bhat

> ---
>  drivers/acpi/processor_driver.c |    7 ------
>  drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
>  include/linux/cpuidle.h         |    7 ------
>  3 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 0734086..8648b29 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>  		 * Initialize missing things
>  		 */
>  		if (pr->flags.need_hotplug_init) {
> -			struct cpuidle_driver *idle_driver =
> -				cpuidle_get_driver();
> -
>  			printk(KERN_INFO "Will online and init hotplugged "
>  			       "CPU: %d\n", pr->id);
>  			WARN(acpi_processor_start(pr), "Failed to start CPU:"
>  				" %d\n", pr->id);
>  			pr->flags.need_hotplug_init = 0;
> -			if (idle_driver && !strcmp(idle_driver->name,
> -						   "intel_idle")) {
> -				intel_idle_cpu_init(pr->id);
> -			}
>  		/* Normal CPU soft online event */
>  		} else {
>  			acpi_processor_ppc_has_changed(pr, 0);
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index d0f59c3..fe95d54 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
>  static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
>  static int intel_idle(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv, int index);
> +static int intel_idle_cpu_init(int cpu);
> 
>  static struct cpuidle_state *cpuidle_state_table;
> 
> @@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
>  	clockevents_notify(reason, &cpu);
>  }
> 
> -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
> -		unsigned long action, void *hcpu)
> +static int cpu_hotplug_notify(struct notifier_block *n,
> +			      unsigned long action, void *hcpu)
>  {
>  	int hotcpu = (unsigned long)hcpu;
> +	struct cpuidle_device *dev;
> 
>  	switch (action & 0xf) {
>  	case CPU_ONLINE:
> -		smp_call_function_single(hotcpu, __setup_broadcast_timer,
> -			(void *)true, 1);
> +
> +		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
> +			smp_call_function_single(hotcpu, __setup_broadcast_timer,
> +						 (void *)true, 1);
> +
> +		/*
> +		 * Some systems can hotplug a cpu at runtime after
> +		 * the kernel has booted, we have to initialize the
> +		 * driver in this case
> +		 */
> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
> +		if (!dev->registered)
> +			intel_idle_cpu_init(hotcpu);
> +
>  		break;
>  	}
>  	return NOTIFY_OK;
>  }
> 
> -static struct notifier_block setup_broadcast_notifier = {
> -	.notifier_call = setup_broadcast_cpuhp_notify,
> +static struct notifier_block cpu_hotplug_notifier = {
> +	.notifier_call = cpu_hotplug_notify,
>  };
> 
>  static void auto_demotion_disable(void *dummy)
> @@ -405,10 +419,10 @@ static int intel_idle_probe(void)
> 
>  	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
>  		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
> -	else {
> +	else
>  		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
> -		register_cpu_notifier(&setup_broadcast_notifier);
> -	}
> +
> +	register_cpu_notifier(&cpu_hotplug_notifier);
> 
>  	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
>  		" model 0x%X\n", boot_cpu_data.x86_model);
> @@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
>   * allocate, initialize, register cpuidle_devices
>   * @cpu: cpu/core to initialize
>   */
> -int intel_idle_cpu_init(int cpu)
> +static int intel_idle_cpu_init(int cpu)
>  {
>  	int cstate;
>  	struct cpuidle_device *dev;
> @@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
> 
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
> 
>  static int __init intel_idle_init(void)
>  {
> @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
>  	intel_idle_cpuidle_devices_uninit();
>  	cpuidle_unregister_driver(&intel_idle_driver);
> 
> -	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
> +
> +	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
>  		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
> -		unregister_cpu_notifier(&setup_broadcast_notifier);
> -	}
> +	unregister_cpu_notifier(&cpu_hotplug_notifier);
> 
>  	return;
>  }
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 5ab7183..66d7e0d 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -213,14 +213,7 @@ struct cpuidle_governor {
>  extern int cpuidle_register_governor(struct cpuidle_governor *gov);
>  extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
> 
> -#ifdef CONFIG_INTEL_IDLE
> -extern int intel_idle_cpu_init(int cpu);
>  #else
> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
> -#endif
> -
> -#else
> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
> 
>  static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
>  {return 0;}
> 

Re: [PATCH v2] acpi: intel_idle : break dependency between modules

[Daniel Lezcano]

On 06/28/2012 01:24 PM, Srivatsa S. Bhat wrote:
> On 06/28/2012 02:16 PM, Daniel Lezcano wrote:
>> When the system is booted with some cpus offline, the idle
>> driver is not initialized. When a cpu is set online, the
>> acpi code call the intel idle init function. Unfortunately
>> this code introduce a dependency between intel_idle and acpi.
>>
>> This patch is intended to remove this dependency by using the
>> notifier of intel_idle. This patch has the benefit of
>> encapsulating the intel_idle driver and remove some exported
>> functions.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> 
> Looks good to me.

Thanks for the review Srivatsa.

Shall I consider it as an acked-by ?

  -- Daniel

> 
> Regards,
> Srivatsa S. Bhat
> 
>> ---
>>  drivers/acpi/processor_driver.c |    7 ------
>>  drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
>>  include/linux/cpuidle.h         |    7 ------
>>  3 files changed, 27 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 0734086..8648b29 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>>  		 * Initialize missing things
>>  		 */
>>  		if (pr->flags.need_hotplug_init) {
>> -			struct cpuidle_driver *idle_driver =
>> -				cpuidle_get_driver();
>> -
>>  			printk(KERN_INFO "Will online and init hotplugged "
>>  			       "CPU: %d\n", pr->id);
>>  			WARN(acpi_processor_start(pr), "Failed to start CPU:"
>>  				" %d\n", pr->id);
>>  			pr->flags.need_hotplug_init = 0;
>> -			if (idle_driver && !strcmp(idle_driver->name,
>> -						   "intel_idle")) {
>> -				intel_idle_cpu_init(pr->id);
>> -			}
>>  		/* Normal CPU soft online event */
>>  		} else {
>>  			acpi_processor_ppc_has_changed(pr, 0);
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index d0f59c3..fe95d54 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
>>  static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
>>  static int intel_idle(struct cpuidle_device *dev,
>>  			struct cpuidle_driver *drv, int index);
>> +static int intel_idle_cpu_init(int cpu);
>>
>>  static struct cpuidle_state *cpuidle_state_table;
>>
>> @@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
>>  	clockevents_notify(reason, &cpu);
>>  }
>>
>> -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
>> -		unsigned long action, void *hcpu)
>> +static int cpu_hotplug_notify(struct notifier_block *n,
>> +			      unsigned long action, void *hcpu)
>>  {
>>  	int hotcpu = (unsigned long)hcpu;
>> +	struct cpuidle_device *dev;
>>
>>  	switch (action & 0xf) {
>>  	case CPU_ONLINE:
>> -		smp_call_function_single(hotcpu, __setup_broadcast_timer,
>> -			(void *)true, 1);
>> +
>> +		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
>> +			smp_call_function_single(hotcpu, __setup_broadcast_timer,
>> +						 (void *)true, 1);
>> +
>> +		/*
>> +		 * Some systems can hotplug a cpu at runtime after
>> +		 * the kernel has booted, we have to initialize the
>> +		 * driver in this case
>> +		 */
>> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
>> +		if (!dev->registered)
>> +			intel_idle_cpu_init(hotcpu);
>> +
>>  		break;
>>  	}
>>  	return NOTIFY_OK;
>>  }
>>
>> -static struct notifier_block setup_broadcast_notifier = {
>> -	.notifier_call = setup_broadcast_cpuhp_notify,
>> +static struct notifier_block cpu_hotplug_notifier = {
>> +	.notifier_call = cpu_hotplug_notify,
>>  };
>>
>>  static void auto_demotion_disable(void *dummy)
>> @@ -405,10 +419,10 @@ static int intel_idle_probe(void)
>>
>>  	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
>>  		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
>> -	else {
>> +	else
>>  		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
>> -		register_cpu_notifier(&setup_broadcast_notifier);
>> -	}
>> +
>> +	register_cpu_notifier(&cpu_hotplug_notifier);
>>
>>  	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
>>  		" model 0x%X\n", boot_cpu_data.x86_model);
>> @@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
>>   * allocate, initialize, register cpuidle_devices
>>   * @cpu: cpu/core to initialize
>>   */
>> -int intel_idle_cpu_init(int cpu)
>> +static int intel_idle_cpu_init(int cpu)
>>  {
>>  	int cstate;
>>  	struct cpuidle_device *dev;
>> @@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
>>
>>  	return 0;
>>  }
>> -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
>>
>>  static int __init intel_idle_init(void)
>>  {
>> @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
>>  	intel_idle_cpuidle_devices_uninit();
>>  	cpuidle_unregister_driver(&intel_idle_driver);
>>
>> -	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
>> +
>> +	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
>>  		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
>> -		unregister_cpu_notifier(&setup_broadcast_notifier);
>> -	}
>> +	unregister_cpu_notifier(&cpu_hotplug_notifier);
>>
>>  	return;
>>  }
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 5ab7183..66d7e0d 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -213,14 +213,7 @@ struct cpuidle_governor {
>>  extern int cpuidle_register_governor(struct cpuidle_governor *gov);
>>  extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
>>
>> -#ifdef CONFIG_INTEL_IDLE
>> -extern int intel_idle_cpu_init(int cpu);
>>  #else
>> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
>> -#endif
>> -
>> -#else
>> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
>>
>>  static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
>>  {return 0;}
>>
> 


-- 
 <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: [PATCH v2] acpi: intel_idle : break dependency between modules

[Srivatsa S. Bhat]

On 06/28/2012 04:57 PM, Daniel Lezcano wrote:
> On 06/28/2012 01:24 PM, Srivatsa S. Bhat wrote:
>> On 06/28/2012 02:16 PM, Daniel Lezcano wrote:
>>> When the system is booted with some cpus offline, the idle
>>> driver is not initialized. When a cpu is set online, the
>>> acpi code call the intel idle init function. Unfortunately
>>> this code introduce a dependency between intel_idle and acpi.
>>>
>>> This patch is intended to remove this dependency by using the
>>> notifier of intel_idle. This patch has the benefit of
>>> encapsulating the intel_idle driver and remove some exported
>>> functions.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>>
>> Looks good to me.
> 
> Thanks for the review Srivatsa.
> 
> Shall I consider it as an acked-by ?
> 

Sure, go ahead! :-)

Regards,
Srivatsa S. Bhat

Re: [PATCH v2] acpi: intel_idle : break dependency between modules

[Rafael J. Wysocki]

On Thursday, June 28, 2012, Daniel Lezcano wrote:
> When the system is booted with some cpus offline, the idle
> driver is not initialized. When a cpu is set online, the
> acpi code call the intel idle init function. Unfortunately
> this code introduce a dependency between intel_idle and acpi.
> 
> This patch is intended to remove this dependency by using the
> notifier of intel_idle. This patch has the benefit of
> encapsulating the intel_idle driver and remove some exported
> functions.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

This one looks good to me too.

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

Len, are you going to take it?

Rafael


> ---
>  drivers/acpi/processor_driver.c |    7 ------
>  drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
>  include/linux/cpuidle.h         |    7 ------
>  3 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 0734086..8648b29 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>  		 * Initialize missing things
>  		 */
>  		if (pr->flags.need_hotplug_init) {
> -			struct cpuidle_driver *idle_driver =
> -				cpuidle_get_driver();
> -
>  			printk(KERN_INFO "Will online and init hotplugged "
>  			       "CPU: %d\n", pr->id);
>  			WARN(acpi_processor_start(pr), "Failed to start CPU:"
>  				" %d\n", pr->id);
>  			pr->flags.need_hotplug_init = 0;
> -			if (idle_driver && !strcmp(idle_driver->name,
> -						   "intel_idle")) {
> -				intel_idle_cpu_init(pr->id);
> -			}
>  		/* Normal CPU soft online event */
>  		} else {
>  			acpi_processor_ppc_has_changed(pr, 0);
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index d0f59c3..fe95d54 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
>  static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
>  static int intel_idle(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv, int index);
> +static int intel_idle_cpu_init(int cpu);
>  
>  static struct cpuidle_state *cpuidle_state_table;
>  
> @@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
>  	clockevents_notify(reason, &cpu);
>  }
>  
> -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
> -		unsigned long action, void *hcpu)
> +static int cpu_hotplug_notify(struct notifier_block *n,
> +			      unsigned long action, void *hcpu)
>  {
>  	int hotcpu = (unsigned long)hcpu;
> +	struct cpuidle_device *dev;
>  
>  	switch (action & 0xf) {
>  	case CPU_ONLINE:
> -		smp_call_function_single(hotcpu, __setup_broadcast_timer,
> -			(void *)true, 1);
> +
> +		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
> +			smp_call_function_single(hotcpu, __setup_broadcast_timer,
> +						 (void *)true, 1);
> +
> +		/*
> +		 * Some systems can hotplug a cpu at runtime after
> +		 * the kernel has booted, we have to initialize the
> +		 * driver in this case
> +		 */
> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
> +		if (!dev->registered)
> +			intel_idle_cpu_init(hotcpu);
> +
>  		break;
>  	}
>  	return NOTIFY_OK;
>  }
>  
> -static struct notifier_block setup_broadcast_notifier = {
> -	.notifier_call = setup_broadcast_cpuhp_notify,
> +static struct notifier_block cpu_hotplug_notifier = {
> +	.notifier_call = cpu_hotplug_notify,
>  };
>  
>  static void auto_demotion_disable(void *dummy)
> @@ -405,10 +419,10 @@ static int intel_idle_probe(void)
>  
>  	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
>  		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
> -	else {
> +	else
>  		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
> -		register_cpu_notifier(&setup_broadcast_notifier);
> -	}
> +
> +	register_cpu_notifier(&cpu_hotplug_notifier);
>  
>  	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
>  		" model 0x%X\n", boot_cpu_data.x86_model);
> @@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
>   * allocate, initialize, register cpuidle_devices
>   * @cpu: cpu/core to initialize
>   */
> -int intel_idle_cpu_init(int cpu)
> +static int intel_idle_cpu_init(int cpu)
>  {
>  	int cstate;
>  	struct cpuidle_device *dev;
> @@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
>  
>  static int __init intel_idle_init(void)
>  {
> @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
>  	intel_idle_cpuidle_devices_uninit();
>  	cpuidle_unregister_driver(&intel_idle_driver);
>  
> -	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
> +
> +	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
>  		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
> -		unregister_cpu_notifier(&setup_broadcast_notifier);
> -	}
> +	unregister_cpu_notifier(&cpu_hotplug_notifier);
>  
>  	return;
>  }
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 5ab7183..66d7e0d 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -213,14 +213,7 @@ struct cpuidle_governor {
>  extern int cpuidle_register_governor(struct cpuidle_governor *gov);
>  extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
>  
> -#ifdef CONFIG_INTEL_IDLE
> -extern int intel_idle_cpu_init(int cpu);
>  #else
> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
> -#endif
> -
> -#else
> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
>  
>  static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
>  {return 0;}
> 

Re: [PATCH v2] acpi: intel_idle : break dependency between modules

[Daniel Lezcano]

On 06/28/2012 09:24 PM, Rafael J. Wysocki wrote:
> On Thursday, June 28, 2012, Daniel Lezcano wrote:
>> When the system is booted with some cpus offline, the idle
>> driver is not initialized. When a cpu is set online, the
>> acpi code call the intel idle init function. Unfortunately
>> this code introduce a dependency between intel_idle and acpi.
>>
>> This patch is intended to remove this dependency by using the
>> notifier of intel_idle. This patch has the benefit of
>> encapsulating the intel_idle driver and remove some exported
>> functions.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> This one looks good to me too.
> 
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

Thanks for the review Rafael.

> Len, are you going to take it?

Rafael, Len,

After the discussion [1], I put in place a tree at:

ssh://git.linaro.org/srv/git.linaro.org/git/people/dlezcano/cpuidle-next.git
#cpuidle-next

I proposed this tree to group the cpuidle modifications and prevent the
last minutes conflict when Len will apply them.

I also included the tree into linux-next for wider testing.

If you want I can take this patch even if it is related to acpi also and
in the future if you agree, Len or you could pull from there.

Thanks
  -- Daniel

[1] https://lkml.org/lkml/2012/6/18/113

> 
> Rafael
> 
> 
>> ---
>>  drivers/acpi/processor_driver.c |    7 ------
>>  drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
>>  include/linux/cpuidle.h         |    7 ------
>>  3 files changed, 27 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 0734086..8648b29 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>>  		 * Initialize missing things
>>  		 */
>>  		if (pr->flags.need_hotplug_init) {
>> -			struct cpuidle_driver *idle_driver =
>> -				cpuidle_get_driver();
>> -
>>  			printk(KERN_INFO "Will online and init hotplugged "
>>  			       "CPU: %d\n", pr->id);
>>  			WARN(acpi_processor_start(pr), "Failed to start CPU:"
>>  				" %d\n", pr->id);
>>  			pr->flags.need_hotplug_init = 0;
>> -			if (idle_driver && !strcmp(idle_driver->name,
>> -						   "intel_idle")) {
>> -				intel_idle_cpu_init(pr->id);
>> -			}
>>  		/* Normal CPU soft online event */
>>  		} else {
>>  			acpi_processor_ppc_has_changed(pr, 0);
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index d0f59c3..fe95d54 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
>>  static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
>>  static int intel_idle(struct cpuidle_device *dev,
>>  			struct cpuidle_driver *drv, int index);
>> +static int intel_idle_cpu_init(int cpu);
>>  
>>  static struct cpuidle_state *cpuidle_state_table;
>>  
>> @@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
>>  	clockevents_notify(reason, &cpu);
>>  }
>>  
>> -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
>> -		unsigned long action, void *hcpu)
>> +static int cpu_hotplug_notify(struct notifier_block *n,
>> +			      unsigned long action, void *hcpu)
>>  {
>>  	int hotcpu = (unsigned long)hcpu;
>> +	struct cpuidle_device *dev;
>>  
>>  	switch (action & 0xf) {
>>  	case CPU_ONLINE:
>> -		smp_call_function_single(hotcpu, __setup_broadcast_timer,
>> -			(void *)true, 1);
>> +
>> +		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
>> +			smp_call_function_single(hotcpu, __setup_broadcast_timer,
>> +						 (void *)true, 1);
>> +
>> +		/*
>> +		 * Some systems can hotplug a cpu at runtime after
>> +		 * the kernel has booted, we have to initialize the
>> +		 * driver in this case
>> +		 */
>> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
>> +		if (!dev->registered)
>> +			intel_idle_cpu_init(hotcpu);
>> +
>>  		break;
>>  	}
>>  	return NOTIFY_OK;
>>  }
>>  
>> -static struct notifier_block setup_broadcast_notifier = {
>> -	.notifier_call = setup_broadcast_cpuhp_notify,
>> +static struct notifier_block cpu_hotplug_notifier = {
>> +	.notifier_call = cpu_hotplug_notify,
>>  };
>>  
>>  static void auto_demotion_disable(void *dummy)
>> @@ -405,10 +419,10 @@ static int intel_idle_probe(void)
>>  
>>  	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
>>  		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
>> -	else {
>> +	else
>>  		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
>> -		register_cpu_notifier(&setup_broadcast_notifier);
>> -	}
>> +
>> +	register_cpu_notifier(&cpu_hotplug_notifier);
>>  
>>  	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
>>  		" model 0x%X\n", boot_cpu_data.x86_model);
>> @@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
>>   * allocate, initialize, register cpuidle_devices
>>   * @cpu: cpu/core to initialize
>>   */
>> -int intel_idle_cpu_init(int cpu)
>> +static int intel_idle_cpu_init(int cpu)
>>  {
>>  	int cstate;
>>  	struct cpuidle_device *dev;
>> @@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
>>  
>>  	return 0;
>>  }
>> -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
>>  
>>  static int __init intel_idle_init(void)
>>  {
>> @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
>>  	intel_idle_cpuidle_devices_uninit();
>>  	cpuidle_unregister_driver(&intel_idle_driver);
>>  
>> -	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
>> +
>> +	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
>>  		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
>> -		unregister_cpu_notifier(&setup_broadcast_notifier);
>> -	}
>> +	unregister_cpu_notifier(&cpu_hotplug_notifier);
>>  
>>  	return;
>>  }
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 5ab7183..66d7e0d 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -213,14 +213,7 @@ struct cpuidle_governor {
>>  extern int cpuidle_register_governor(struct cpuidle_governor *gov);
>>  extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
>>  
>> -#ifdef CONFIG_INTEL_IDLE
>> -extern int intel_idle_cpu_init(int cpu);
>>  #else
>> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
>> -#endif
>> -
>> -#else
>> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
>>  
>>  static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
>>  {return 0;}
>>
> 


-- 
 <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: [PATCH v2] acpi: intel_idle : break dependency between modules

[Rafael J. Wysocki]

On Friday, June 29, 2012, Daniel Lezcano wrote:
> On 06/28/2012 09:24 PM, Rafael J. Wysocki wrote:
> > On Thursday, June 28, 2012, Daniel Lezcano wrote:
> >> When the system is booted with some cpus offline, the idle
> >> driver is not initialized. When a cpu is set online, the
> >> acpi code call the intel idle init function. Unfortunately
> >> this code introduce a dependency between intel_idle and acpi.
> >>
> >> This patch is intended to remove this dependency by using the
> >> notifier of intel_idle. This patch has the benefit of
> >> encapsulating the intel_idle driver and remove some exported
> >> functions.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > 
> > This one looks good to me too.
> > 
> > Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Thanks for the review Rafael.
> 
> > Len, are you going to take it?
> 
> Rafael, Len,
> 
> After the discussion [1], I put in place a tree at:
> 
> ssh://git.linaro.org/srv/git.linaro.org/git/people/dlezcano/cpuidle-next.git
> #cpuidle-next
> 
> I proposed this tree to group the cpuidle modifications and prevent the
> last minutes conflict when Len will apply them.
> 
> I also included the tree into linux-next for wider testing.

I can't speak for Len, but I'm not sure he'll like this.

Anyway, you seem to have the same material as Len in that tree, won't there
be any conflicts in linux-next?

Rafael

Re: [PATCH v2] acpi: intel_idle : break dependency between modules

[Daniel Lezcano]

On 06/30/2012 12:27 AM, Rafael J. Wysocki wrote:
> On Friday, June 29, 2012, Daniel Lezcano wrote:
>> On 06/28/2012 09:24 PM, Rafael J. Wysocki wrote:
>>> On Thursday, June 28, 2012, Daniel Lezcano wrote:
>>>> When the system is booted with some cpus offline, the idle
>>>> driver is not initialized. When a cpu is set online, the
>>>> acpi code call the intel idle init function. Unfortunately
>>>> this code introduce a dependency between intel_idle and acpi.
>>>>
>>>> This patch is intended to remove this dependency by using the
>>>> notifier of intel_idle. This patch has the benefit of
>>>> encapsulating the intel_idle driver and remove some exported
>>>> functions.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>> This one looks good to me too.
>>>
>>> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
>>
>> Thanks for the review Rafael.
>>
>>> Len, are you going to take it?
>>
>> Rafael, Len,
>>
>> After the discussion [1], I put in place a tree at:
>>
>> ssh://git.linaro.org/srv/git.linaro.org/git/people/dlezcano/cpuidle-next.git
>> #cpuidle-next
>>
>> I proposed this tree to group the cpuidle modifications and prevent the
>> last minutes conflict when Len will apply them.
>>
>> I also included the tree into linux-next for wider testing.
> 
> I can't speak for Len, but I'm not sure he'll like this.

I sent the proposition a week ago and Len was Cc'ed. I guess he is very
busy but the problem we are facing is there are a lot of pending
modifications for cpuidle because of some new architecture (like the
big.LITTLE and tegra3). Colin's patchset is one of them.

> Anyway, you seem to have the same material as Len in that tree, won't there
> be any conflicts in linux-next?

At the first glance no, until he merge the patches we sent. As I already
said the purpose is to help to consolidate the patches sent for cpuidle
by acting as a proxy.

I hope that helps.

Thanks
  -- Daniel



-- 
 <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: [linux-pm] [PATCH] acpi: intel_idle : break dependency between modules

[Srivatsa S. Bhat]

On 06/27/2012 02:37 PM, Daniel Lezcano wrote:
> When the system is booted with some cpus offline, the idle
> driver is not initialized. When a cpu is set online, the
> acpi code call the intel idle init function. Unfortunately
> this code introduce a dependency between intel_idle and acpi.
> 
> This patch is intended to remove this dependency by using the
> notifier of intel_idle. In order to make it work, the notifier
> must be initialized in the right order, acpi then intel_idle.
> This is done in the Makefile.

There is a much better way of doing this. See below.

> This patch has the benefit of
> encapsulating the intel_idle driver and remove some exported
> functions.
> 

Nice :)

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/Makefile                |    3 ++-
>  drivers/acpi/processor_driver.c |    7 -------
>  drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
>  include/linux/cpuidle.h         |    7 -------
>  4 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 2ba29ff..a2454b8 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -12,8 +12,9 @@ obj-$(CONFIG_PCI)		+= pci/
>  obj-$(CONFIG_PARISC)		+= parisc/
>  obj-$(CONFIG_RAPIDIO)		+= rapidio/
>  obj-y				+= video/
> -obj-y				+= idle/
> +# acpi must come before idle for initialization
>  obj-$(CONFIG_ACPI)		+= acpi/
> +obj-y				+= idle/
>  obj-$(CONFIG_SFI)		+= sfi/
>  # PnP must come after ACPI since it will eventually need to check if acpi
>  # was used and do nothing if so

OK, so all you are trying to do here is ensure that the intel idle related
notifier runs _after_ the acpi related one. To do that, you don't have to touch
the Makefiles at all! Just use the appropriate priorities for the notifiers
(default is 0), to handle the dependency. You can find some examples in kernel/
sched/core.c.  And while doing that you might want to add a separate notifier
in intel idle rather than piggy back on the existing timer related one (because
you are handling a dependency here, which might not apply to timers, or even
worse, cause unwanted side-effects, if any).

I'll take a look at the rest of the patch tomorrow. I think Thomas has already
pointed out the rest of the issues.

Regards,
Srivatsa S. Bhat

> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 0734086..8648b29 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>  		 * Initialize missing things
>  		 */
>  		if (pr->flags.need_hotplug_init) {
> -			struct cpuidle_driver *idle_driver =
> -				cpuidle_get_driver();
> -
>  			printk(KERN_INFO "Will online and init hotplugged "
>  			       "CPU: %d\n", pr->id);
>  			WARN(acpi_processor_start(pr), "Failed to start CPU:"
>  				" %d\n", pr->id);
>  			pr->flags.need_hotplug_init = 0;
> -			if (idle_driver && !strcmp(idle_driver->name,
> -						   "intel_idle")) {
> -				intel_idle_cpu_init(pr->id);
> -			}
>  		/* Normal CPU soft online event */
>  		} else {
>  			acpi_processor_ppc_has_changed(pr, 0);
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index d0f59c3..4c36039 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
>  static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
>  static int intel_idle(struct cpuidle_device *dev,
>  			struct cpuidle_driver *drv, int index);
> +static int intel_idle_cpu_init(int cpu);
> 
>  static struct cpuidle_state *cpuidle_state_table;
> 
> @@ -302,22 +303,28 @@ static void __setup_broadcast_timer(void *arg)
>  	clockevents_notify(reason, &cpu);
>  }
> 
> -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
> -		unsigned long action, void *hcpu)
> +static int cpu_hotplug_notify(struct notifier_block *n,
> +			      unsigned long action, void *hcpu)
>  {
>  	int hotcpu = (unsigned long)hcpu;
> +	struct cpuidle_device *dev;
> 
>  	switch (action & 0xf) {
>  	case CPU_ONLINE:
>  		smp_call_function_single(hotcpu, __setup_broadcast_timer,
>  			(void *)true, 1);
> +
> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
> +		if (!dev->registered)
> +			intel_idle_cpu_init(hotcpu);
> +
>  		break;
>  	}
>  	return NOTIFY_OK;
>  }
> 
> -static struct notifier_block setup_broadcast_notifier = {
> -	.notifier_call = setup_broadcast_cpuhp_notify,
> +static struct notifier_block cpu_hotplug_notifier = {
> +	.notifier_call = cpu_hotplug_notify,
>  };
> 
>  static void auto_demotion_disable(void *dummy)
> @@ -407,7 +414,7 @@ static int intel_idle_probe(void)
>  		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
>  	else {
>  		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
> -		register_cpu_notifier(&setup_broadcast_notifier);
> +		register_cpu_notifier(&cpu_hotplug_notifier);
>  	}
> 
>  	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
> @@ -494,7 +501,7 @@ static int intel_idle_cpuidle_driver_init(void)
>   * allocate, initialize, register cpuidle_devices
>   * @cpu: cpu/core to initialize
>   */
> -int intel_idle_cpu_init(int cpu)
> +static int intel_idle_cpu_init(int cpu)
>  {
>  	int cstate;
>  	struct cpuidle_device *dev;
> @@ -539,7 +546,6 @@ int intel_idle_cpu_init(int cpu)
> 
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
> 
>  static int __init intel_idle_init(void)
>  {
> @@ -583,7 +589,7 @@ static void __exit intel_idle_exit(void)
> 
>  	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
>  		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
> -		unregister_cpu_notifier(&setup_broadcast_notifier);
> +		unregister_cpu_notifier(&cpu_hotplug_notifier);
>  	}
> 
>  	return;
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 5ab7183..66d7e0d 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -213,14 +213,7 @@ struct cpuidle_governor {
>  extern int cpuidle_register_governor(struct cpuidle_governor *gov);
>  extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
> 
> -#ifdef CONFIG_INTEL_IDLE
> -extern int intel_idle_cpu_init(int cpu);
>  #else
> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
> -#endif
> -
> -#else
> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
> 
>  static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
>  {return 0;}
> 

Re: [linux-pm] [PATCH] acpi: intel_idle : break dependency between modules

[Thomas Renninger]

On Wednesday, June 27, 2012 06:16:33 PM Srivatsa S. Bhat wrote:
> On 06/27/2012 02:37 PM, Daniel Lezcano wrote:
> > When the system is booted with some cpus offline, the idle
> > driver is not initialized. When a cpu is set online, the
> > acpi code call the intel idle init function. Unfortunately
> > this code introduce a dependency between intel_idle and acpi.
> > 
> > This patch is intended to remove this dependency by using the
> > notifier of intel_idle. In order to make it work, the notifier
> > must be initialized in the right order, acpi then intel_idle.
> > This is done in the Makefile.
> 
> There is a much better way of doing this. See below.
> 
> > This patch has the benefit of
> > encapsulating the intel_idle driver and remove some exported
> > functions.
> > 
> 
> Nice :)
> 
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >  drivers/Makefile                |    3 ++-
> >  drivers/acpi/processor_driver.c |    7 -------
> >  drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
> >  include/linux/cpuidle.h         |    7 -------
> >  4 files changed, 16 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 2ba29ff..a2454b8 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -12,8 +12,9 @@ obj-$(CONFIG_PCI)		+= pci/
> >  obj-$(CONFIG_PARISC)		+= parisc/
> >  obj-$(CONFIG_RAPIDIO)		+= rapidio/
> >  obj-y				+= video/
> > -obj-y				+= idle/
> > +# acpi must come before idle for initialization
> >  obj-$(CONFIG_ACPI)		+= acpi/
> > +obj-y				+= idle/
> >  obj-$(CONFIG_SFI)		+= sfi/
> >  # PnP must come after ACPI since it will eventually need to check if acpi
> >  # was used and do nothing if so
> 
> OK, so all you are trying to do here is ensure that the intel idle related
> notifier runs _after_ the acpi related one.
I might oversee something, if you have concerns, please point me to it.
If it's all about keeping the order of excuting these functions:
   acpi_processor_start(pr)
and
   intel_idle_cpu_init()
There should be no need for it. Intel idle is pretty separated.

  Thomas

Re: [linux-pm] [PATCH] acpi: intel_idle : break dependency between modules

[Srivatsa S. Bhat]

On 06/28/2012 01:04 PM, Thomas Renninger wrote:
> On Wednesday, June 27, 2012 06:16:33 PM Srivatsa S. Bhat wrote:
>> On 06/27/2012 02:37 PM, Daniel Lezcano wrote:
>>> When the system is booted with some cpus offline, the idle
>>> driver is not initialized. When a cpu is set online, the
>>> acpi code call the intel idle init function. Unfortunately
>>> this code introduce a dependency between intel_idle and acpi.
>>>
>>> This patch is intended to remove this dependency by using the
>>> notifier of intel_idle. In order to make it work, the notifier
>>> must be initialized in the right order, acpi then intel_idle.
>>> This is done in the Makefile.
>>
>> There is a much better way of doing this. See below.
>>
>>> This patch has the benefit of
>>> encapsulating the intel_idle driver and remove some exported
>>> functions.
>>>
>>
>> Nice :)
>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>>  drivers/Makefile                |    3 ++-
>>>  drivers/acpi/processor_driver.c |    7 -------
>>>  drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
>>>  include/linux/cpuidle.h         |    7 -------
>>>  4 files changed, 16 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/Makefile b/drivers/Makefile
>>> index 2ba29ff..a2454b8 100644
>>> --- a/drivers/Makefile
>>> +++ b/drivers/Makefile
>>> @@ -12,8 +12,9 @@ obj-$(CONFIG_PCI)		+= pci/
>>>  obj-$(CONFIG_PARISC)		+= parisc/
>>>  obj-$(CONFIG_RAPIDIO)		+= rapidio/
>>>  obj-y				+= video/
>>> -obj-y				+= idle/
>>> +# acpi must come before idle for initialization
>>>  obj-$(CONFIG_ACPI)		+= acpi/
>>> +obj-y				+= idle/
>>>  obj-$(CONFIG_SFI)		+= sfi/
>>>  # PnP must come after ACPI since it will eventually need to check if acpi
>>>  # was used and do nothing if so
>>
>> OK, so all you are trying to do here is ensure that the intel idle related
>> notifier runs _after_ the acpi related one.
> I might oversee something, if you have concerns, please point me to it.
> If it's all about keeping the order of excuting these functions:
>    acpi_processor_start(pr)
> and
>    intel_idle_cpu_init()
> There should be no need for it. Intel idle is pretty separated.
> 

I digged through the code a bit, and even I couldn't find any reason why there
should be a dependency between the two events.

Regards,
Srivatsa S. Bhat

Re: [linux-pm] [PATCH] acpi, cpuidle: Register with cpuidle even if cpu is onlined after boot (beyond maxcpus)

[Srivatsa S. Bhat]

On 06/26/2012 04:12 PM, Daniel Lezcano wrote:
> On 06/26/2012 11:58 AM, Srivatsa S. Bhat wrote:
>> On 06/26/2012 03:11 PM, Daniel Lezcano wrote:
>>> On 06/26/2012 11:29 AM, Thomas Renninger wrote:
>>>> On Monday, June 25, 2012 06:03:42 PM Srivatsa S. Bhat wrote:
>>>>> On 06/25/2012 07:23 PM, Thomas Renninger wrote:
>>>>>
>>>>>> On Monday, June 25, 2012 01:25:43 PM Srivatsa S. Bhat wrote:
>>>>>>>
>>>>>>> Daniel Lezcano noticed that after booting with maxcpus=X, if we online the
>>>>>>> remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then
>>>>>>> for the newly onlined cpus, the cpuidle directory is not found under
>>>>>>> /sys/devices/system/cpu/cpuY.
>>>>>>>
>>>>>>> Partly, the reason for this is that acpi restricts the initialization to cpus
>>>>>>> within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi
>>>>>>> processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is
>>>>>>> used to restrict the number of cpus brought up during boot. That doesn't mean
>>>>>>> that we should hard restrict the bring up of the remaining cpus later on.
>>>>>>
>>>>>> Sorry, but IMO it exaclty does mean that (adding more general lists for
>>>>>> further comments).
>>>>>>
>>>>>> If you can online more cores than maxcpus= via sysfs, this sounds like a bug.
>>>>>> Not the other way around.
>>>>>>
>>>>>> Compare with Documentation/kernel-parameters.txt:
>>>>>>         maxcpus=        [SMP] Maximum number of processors that an SMP kernel
>>>>>>                         should make use of.  maxcpus=n : n >= 0 limits the
>>>>>>                         kernel to using 'n' processors.  n=0 is a special case,
>>>>>>                         it is equivalent to "nosmp", which also disables
>>>>>>                         the IO APIC.
>>>>>>
>>>>>> Chances that you run into more problems are high.
>>>>>
>>>>>
>>>>> Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and
>>>>> should forbid any new cpus from coming online, but in the interest of avoiding
>>>>> problems/complications in some obscure paths, I guess it makes sense to avoid
>>>>> onlining new cpus beyond maxcpus.
>>>>
>>>> Yep, for such reasons:
>>>>    - That nobody realizes this to be useful and makes use of it in a productive
>>>>      environment
>>>>    - If I see maxcpus=X in a bugreport's dmesg command line,
>>>>      I want to be sure that's true.
>>>>    - To enforce that things work as documented
>>>>
>>>>
>>>> Wow, after looking a bit into this I found (Documentation/cpu-hotplug.txt):
>>>>
>>>> maxcpus=n    Restrict boot time cpus to n. Say if you have 4 cpus, using
>>>>              maxcpus=2 will only boot 2. You can choose to bring the
>>>>              other cpus later online, read FAQ's for more info.
>>>>
>>>> Looks like someone already documented this (IMO broken) behavior.
>>>> I didn't find further info in the FAQs.
>>>>
>>>>> In any case, I was just trying to see why the simple removal of the setup_max_cpus
>>>>> check in acpi_processor_add() wasn't enough to expose the cpuidle directories under
>>>>> the new cpus.. and while debugging that, I came up with this patch. I don't mind
>>>>> if this doesn't get picked up.
>>>>
>>>>> Right, the usecase of why somebody would like to online new cpus beyond maxcpus
>>>>> doesn't look all that solid anyway. So I am OK with leaving the code as it is now.
>>>>
>>>> In the end this is a debug option, I expect everybody is aware of that.
>>>> Yep, let's just leave it...
>>>
>>> In this case, let's remove the intel_idle_cpu_init stuff in
>>> acpi_cpu_soft_notify, no ?
>>>
>>
>> Why? And how would that help? The intel_idle_cpu_init() call is essential if intel_idle
>> driver is being used instead of acpi idle.
> 
> AFAIU, this code is not called after onlining a cpu greater than maxcpus
> and Thomas thinks that system with cpu hotplug at runtime are not sold.
> 

No, the point that Thomas is making is, if you boot with maxcpus=X, it looks
odd if you want to online more cpus later on. And allowing that is scary
because those code paths may not be well-tested or even designed to do that.

But one thing is crystal clear about the maxcpus semantics: if you say maxcpus=X
while booting, the kernel must not even *attempt* to initialize *anything* for
the remaining cpus, as far as possible. For all you know, the user might have
discovered a problem (which will cause a crash during init) and hence is setting
maxcpus to a smaller value than available, to just be able to still have a usable
system.

> The problem I see with this code is acpi and intel-idle are tied
> together now. I would like to break this dependency and use the notifier
> to handle the cpu hotplug directly in intel-idle.
> 

Ok, that's a different problem, unrelated to maxcpus. And in that context, what
you are proposing (breaking that dependency) looks good to me.

> It is hard to test my patch as there is not such system and maxcpus is
> not correctly handled here. I can use your patch to test my patch but
> anyway ... I am just asking if that would make sense to remove this
> portion of code instead :)
> 

> If we want to keep this code untouched, I can try my patch and maybe
> Thomas will agreed to test it also on a cpu-online-runtime-system if he
> has one.
> 

Again, to reiterate, we all agree that we can offline/online existing cpus
on a running system. We can also (perhaps) do physical cpu hotplug, and
we want to support it in Linux, if such hardware exists. What doesn't really
make much sense is a "usecase" where you boot the kernel with maxcpus=X and
then try to online more cpus than that. Saying no to that looks safe and is
preferred, than trying to "handle" it, because we cannot guarantee that it
will work in all cases anyway.

But in a separate context (unrelated to maxcpus), moving the intel idle stuff
into intel idle code (from acpi idle) looks like a sensible thing to do. But
then, dependency between the two must be handled properly (ie., low-level acpi
init must happen first, followed by intel idle init, for a hotplugged cpu).

Regards,
Srivatsa S. Bhat