* Re: use of pm_runtime_disable() from driver probe?
From: Rafael J. Wysocki @ 2012-07-08 14:59 UTC (permalink / raw)
To: Alan Stern; +Cc: Kevin Hilman, linux-pm
In-Reply-To: <Pine.LNX.4.44L0.1207072155400.508-100000@netrider.rowland.org>
On Sunday, July 08, 2012, Alan Stern wrote:
> On Sat, 7 Jul 2012, Rafael J. Wysocki wrote:
>
> > > What is the right way to handle the runtime PM enable/disable in a
> > > failed probe attempt?
> >
> > I would recommend using pm_runtime_disable() and then either
> > pm_runtime_set_active() or pm_runtime_set_suspended() depending on what the
> > real power state of the device is at this point.
> >
> > Anyway, you can't force the device into a low-power state using runtime PM
> > after a failing probe, at least in general.
>
> Right. About the best you could do would be to call the appropriate
> runtime_suspend method directly, instead of going through the runtime
> PM core. However, if the probe is failing then there's no reason to
> think the runtime_suspend method will work.
>
> Basically, on the platform bus, if you don't have a driver for a device
> (which you don't, if the probe routine fails) then you can't do runtime
> PM on it.
>
> But what if the device belongs to a PM domain? Putting the entire
> domain back into a low-power state might have the desired effect -- but
> the probe routine wouldn't be able to do this. The platform subsystem
> would have to take care of it somehow.
Yes.
For example, on sh platforms we call pm_gnepd_poweroff_unused() from a
late initcall (drivers/sh/pm_runtime.c), which causes all PM domains that
only contain suspended devices and devices without drivers to be turned off.
Thanks,
Rafael
^ permalink raw reply
* Re: use of pm_runtime_disable() from driver probe?
From: Alan Stern @ 2012-07-08 2:01 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Kevin Hilman, linux-pm
In-Reply-To: <201207072125.26189.rjw@sisk.pl>
On Sat, 7 Jul 2012, Rafael J. Wysocki wrote:
> > What is the right way to handle the runtime PM enable/disable in a
> > failed probe attempt?
>
> I would recommend using pm_runtime_disable() and then either
> pm_runtime_set_active() or pm_runtime_set_suspended() depending on what the
> real power state of the device is at this point.
>
> Anyway, you can't force the device into a low-power state using runtime PM
> after a failing probe, at least in general.
Right. About the best you could do would be to call the appropriate
runtime_suspend method directly, instead of going through the runtime
PM core. However, if the probe is failing then there's no reason to
think the runtime_suspend method will work.
Basically, on the platform bus, if you don't have a driver for a device
(which you don't, if the probe routine fails) then you can't do runtime
PM on it.
But what if the device belongs to a PM domain? Putting the entire
domain back into a low-power state might have the desired effect -- but
the probe routine wouldn't be able to do this. The platform subsystem
would have to take care of it somehow.
Alan Stern
^ permalink raw reply
* Re: use of pm_runtime_disable() from driver probe?
From: Rafael J. Wysocki @ 2012-07-07 19:25 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-pm
In-Reply-To: <873954a5ho.fsf@ti.com>
Hi,
On Saturday, July 07, 2012, Kevin Hilman wrote:
> Hello,
>
> I've got a question around the proper use of pm_runtime_disable() in a
> driver's ->probe method. Basically, using pm_runtime_disable() in
> ->probe() (e.g because of a error/failure) can result in the device
> remaning runtime enabled forever.
Er, no.
> This is because the driver core wraps
> the probe in _get_noresume()/_put_sync() so any usage of _put_sync() in
> ->probe itself does not trigger an actual device runtime suspend. If
> the driver also calls pm_runtime_disable() in probe, that results in the
> device remaining runtime enabled.
I'm not quite sure what way. pm_runtime_disable() increments
dev->power.disable_count and if that is not decremented later, the
device remains runtime disabled.
You probably wanted to say that the device will end up in the full-power
state in that case, which isn't the same and is intentional.
> Consider this example (based on drivers/mmc/host/omap_hsmmc.c). In the
> normal/success path, everything works fine. But in the failure case,
> the after the failed probe, the driver calls pm_runtime_disable() and
> the device is left runtime enabled:
>
> driver_probe()
> {
> /* usage count == 1; due to _get_noresume() in driver core */
> pm_runtime_enable();
> pm_runtime_get_sync()
> /* usage_count == 2 */
>
> /* probe HW */
> if (failure)
> goto error;
>
> /* otherwise, finish HW init */
>
> pm_runtime_put_sync();
> /* usage_count == 1 */
> /* _put_sync() in driver core will make count = 0
> * and device will be runtime suspended */
> return 0;
>
> error:
> pm_runtime_put_sync();
> /* usage_count = 1 */
> pm_runtime_disable();
>
> /* Now, the _put_sync() in driver core will not actually suspend
> since runtime PM has been disabled. */
> }
>
>
> So, the question is: is it right to use pm_runtime_disable() this way?
If your intention is to put the device into a low-power state in case of
a failing .probe(), this isn't the way to do it. pm_runtime_disable()
means "don't do any runtime power management on this device any more"
regardless of the current state of the device (it may resume the device
in some situations, but it won't suspend it, although it may wait for
a suspend in progress to complete).
That said, I'm not aware of any other generally reliable way to do that
either. The problem is, if .probe() is failing we don't have callbacks
allowing us to power manage the device (there may be bus type callbacks
knowing how to do that, but not for the platform bus type).
> Removing it means the device is left in runtime suspended state even in
> the error case. However, without the pm_runtime_disable(), if this
> driver was a module, repeated attempts to load would result in
> unbalalced calls to pm_runtime_enable.
>
> What is the right way to handle the runtime PM enable/disable in a
> failed probe attempt?
I would recommend using pm_runtime_disable() and then either
pm_runtime_set_active() or pm_runtime_set_suspended() depending on what the
real power state of the device is at this point.
Anyway, you can't force the device into a low-power state using runtime PM
after a failing probe, at least in general.
Thanks,
Rafael
^ permalink raw reply
* use of pm_runtime_disable() from driver probe?
From: Kevin Hilman @ 2012-07-06 22:29 UTC (permalink / raw)
To: linux-pm
Hello,
I've got a question around the proper use of pm_runtime_disable() in a
driver's ->probe method. Basically, using pm_runtime_disable() in
->probe() (e.g because of a error/failure) can result in the device
remaning runtime enabled forever. This is because the driver core wraps
the probe in _get_noresume()/_put_sync() so any usage of _put_sync() in
->probe itself does not trigger an actual device runtime suspend. If
the driver also calls pm_runtime_disable() in probe, that results in the
device remaining runtime enabled.
Consider this example (based on drivers/mmc/host/omap_hsmmc.c). In the
normal/success path, everything works fine. But in the failure case,
the after the failed probe, the driver calls pm_runtime_disable() and
the device is left runtime enabled:
driver_probe()
{
/* usage count == 1; due to _get_noresume() in driver core */
pm_runtime_enable();
pm_runtime_get_sync()
/* usage_count == 2 */
/* probe HW */
if (failure)
goto error;
/* otherwise, finish HW init */
pm_runtime_put_sync();
/* usage_count == 1 */
/* _put_sync() in driver core will make count = 0
* and device will be runtime suspended */
return 0;
error:
pm_runtime_put_sync();
/* usage_count = 1 */
pm_runtime_disable();
/* Now, the _put_sync() in driver core will not actually suspend
since runtime PM has been disabled. */
}
So, the question is: is it right to use pm_runtime_disable() this way?
Removing it means the device is left in runtime suspended state even in
the error case. However, without the pm_runtime_disable(), if this
driver was a module, repeated attempts to load would result in
unbalalced calls to pm_runtime_enable.
What is the right way to handle the runtime PM enable/disable in a
failed probe attempt?
Thanks,
Kevin
^ permalink raw reply
* Unusual power consumption of ethernet device r8169 and backlight
From: Jerome Glisse @ 2012-07-06 21:52 UTC (permalink / raw)
To: linux-kernel, netdev, linux-pm
I am seeing somewhat very high power consumption usage of the network
device and backlight
Here is what report powertop, network can go as high as 9W
8.03 W 100.0% Device Display backlight
5.19 W 23.1 pkts/s Device Network
interface: p20p1 (r8169)
When turning off backlight , it effectively vanish and reflect in
power usage, i can't turn off the network device.
So question is this backlight consumption expected ? And am i right
assuming that the network driver is doing something horribly wrong
that the consumption is so high (i must stress that it doesn't matter
if there is network activities or not, plug or unplugged it doesn't
change).
Cheers,
Jerome Glisse
r8169 0000:01:00.0: eth0: RTL8168evl/8111evl at 0xffffc90000052000,
e8:03:9a:b4:a3:90, XID 0c900800 IRQ 45
lspci -vv -s 01:00.0
01:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
RTL8111/8168B PCI Express Gigabit Ethernet controller (rev 06)
Subsystem: Samsung Electronics Co Ltd Device c660
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0, Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 42
Region 0: I/O ports at e000 [size=256]
Region 2: Memory at d0004000 (64-bit, prefetchable) [size=4K]
Region 4: Memory at d0000000 (64-bit, prefetchable) [size=16K]
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [50] MSI: Enable+ Count=1/1 Maskable- 64bit+
Address: 00000000fee0300c Data: 4169
Capabilities: [70] Express (v2) Endpoint, MSI 01
DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
MaxPayload 128 bytes, MaxReadReq 4096 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0
unlimited, L1 <64us
ClockPM+ Surprise- LLActRep- BwNot-
LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk+
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive-
BWMgmt- ABWMgmt-
DevCap2: Completion Timeout: Range ABCD, TimeoutDis+
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-
LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis-,
Selectable De-emphasis: -6dB
Transmit Margin: Normal Operating Range, EnterModifiedCompliance-
ComplianceSOS-
Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-,
EqualizationPhase1-
EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
Capabilities: [b0] MSI-X: Enable- Count=4 Masked-
Vector table: BAR=4 offset=00000000
PBA: BAR=4 offset=00000800
Capabilities: [d0] Vital Product Data
No end tag found
Capabilities: [100 v1] Advanced Error Reporting
UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+
MalfTLP+ ECRC- UnsupReq- ACSViol-
CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
AERCap: First Error Pointer: 00, GenCap+ CGenEn- ChkCap+ ChkEn-
Capabilities: [140 v1] Virtual Channel
Caps: LPEVC=0 RefClk=100ns PATEntryBits=1
Arb: Fixed- WRR32- WRR64- WRR128-
Ctrl: ArbSelect=Fixed
Status: InProgress-
VC0: Caps: PATOffset=00 MaxTimeSlots=1 RejSnoopTrans-
Arb: Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256-
Ctrl: Enable+ ID=0 ArbSelect=Fixed TC/VC=01
Status: NegoPending- InProgress-
Capabilities: [160 v1] Device Serial Number 01-00-00-00-68-4c-e0-00
Kernel driver in use: r8169
^ permalink raw reply
* Re: [PATCH 4/4] cpuidle : move tlb flag to the cpuidle header
From: Rafael J. Wysocki @ 2012-07-06 21:29 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-acpi, linux-pm, linaro-dev
In-Reply-To: <4FF6C6E0.8020609@linaro.org>
On Friday, July 06, 2012, Daniel Lezcano wrote:
> On 07/05/2012 10:43 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 05, 2012, Daniel Lezcano wrote:
> >> Move this specific flag to the header file.
> >
> > The patch evidently does more than that.
> >
> > Is it just a cleanup, or is there a functional reason for doing it?
>
> It is just a cleanup.
In that case, can you please say that in the changelog? I have no objections
to the patch itself, but the changelog is clearly substandard.
> Thanks for reviewing the patches.
No problem.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 3/4] cpuidle: move enter_dead to the driver structure
From: Rafael J. Wysocki @ 2012-07-06 21:27 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-acpi, linux-pm, linaro-dev
In-Reply-To: <4FF6C673.7000003@linaro.org>
On Friday, July 06, 2012, Daniel Lezcano wrote:
> On 07/05/2012 10:40 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 05, 2012, Daniel Lezcano wrote:
> >> The 'enter_dead' function is only used for processor_idle.c
> >> and the same function is used several times. We fall into the
> >> same abuse with the multiple callbacks for the same function.
> >
> > This isn't abuse, mind you. This is a normal practice.
>
> Well, that depends :)
>
> I agree adding a callback per state is nice and flexible
Yes, it is.
> but if it is not used, it is a waste of memory, even if it is 32 bytes.
32 bits, perhaps? And how many of those are there in the whole system,
actually? Is this a number that actually matters?
> >> This patch fixes that by moving the 'enter_dead' function to the
> >> driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
> >> to handle the callback conditional invokation.
> >
> > And how does that improve things?
>
> In order to check if the play_dead is enabled for a specific state, we
> check if the pointer is set. As it has been moved to a single function,
> we need to add a flag to replace this check. That is not an improvement,
Well, exactly.
> it replace a check by another check.
Sorry, but I don't really see the point.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure
From: Rafael J. Wysocki @ 2012-07-06 21:23 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Kevin Hilman, linaro-dev, linux-acpi, linux-pm, Arjan van de Ven
In-Reply-To: <4FF6C4C1.7030004@linaro.org>
On Friday, July 06, 2012, Daniel Lezcano wrote:
> On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 05, 2012, Daniel Lezcano wrote:
> >> We have the state index passed as parameter to the 'enter' function.
> >> Most of the drivers assign their 'enter' functions several times in
> >> the cpuidle_state structure, as we have the index, we can delegate
> >> to the driver to handle their own callback array.
> >>
> >> That will have the benefit of removing multiple lines of code in the
> >> different drivers.
> >
> > Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason.
> > Among other things, this was to avoid recurrence in callbacks - please see
> > acpi_idle_enter_bm() for example.
> >
> > Now, if .enter() is moved to the driver structure, it will have to be an
> > all-purpose complicated routine calling itself recursively at least in
> > some cases. I'm not quite convinced that would be an improvement.
> >
> > On the other hand, I don't see anything wrong with setting several callback
> > pointers to the same routine.
>
> Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state
> to a single structure stored in the driver. The drivers were modified
> and cleanup to take into account this modification.
>
> We saw some regressions like for example the 'disable' which were not
> per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a
> workaround more than a real fix). And now we have new architectures
> (tegra3, big.LITTLE) with different latencies per cpu. Logically we
> should revert Deepthi's patches but from my POV, going back and forth is
> not a good solution (also we have to undo all modifications done in the
> different drivers).
>
> The main purpose of all these cleanup patches are to move out all
> non-data information from the cpuidle_state structure in order to add a
> new api which could be 'cpuidle_register_cpu_latency(int cpu, struct
> cpuidle_latencies latencies)'.
Well, my question is: what specifically is wrong with having callback
pointers in struct cpuidle_state in case when that structure is known
to be common for all the CPUs in the system?
Sure, there are systems in which that is not the case, but then why
should we complicate the simplest case in order to address them?
> For this specific patch, the 'enter' function for all the drivers is not
> used [1] and one of the driver is about to use a single function [2]. So
> we have only one driver is a couple of functions for this which can be
> replaced by an array of callbacks in the driver itself as we have the index.
>
> [1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html
I agree with Deepthi.
> [2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html
And that is fine and dandy as long as the OMAP people want that. Evidently,
that is not wanted by everybody and if it had been, the cpuidle subsystem
would have been designed differently.
I really would like that thing to be reconsidered before we proceed with
these changes.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 2/8] ACPI, x86: fix Dell M6600 ACPI reboot regression via DMI
From: Ingo Molnar @ 2012-07-06 11:09 UTC (permalink / raw)
To: Len Brown; +Cc: linux-acpi, linux-pm, linux-kernel, Zhang Rui, x86, Len Brown
In-Reply-To: <76eb9a30db4bc8fd172f9155247264b5f2686d7b.1341032550.git.len.brown@intel.com>
* Len Brown <lenb@kernel.org> wrote:
> From: Zhang Rui <rui.zhang@intel.com>
>
> Dell Precision M6600 is known to require PCI reboot, so add it to
> the reboot blacklist in pci_reboot_dmi_table[].
>
> https://bugzilla.kernel.org/show_bug.cgi?id=42749
>
> cc: x86@kernel.org
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
> arch/x86/kernel/reboot.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
Acked-by: Ingo Molnar <mingo@kernel.org>
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH 4/4] cpuidle : move tlb flag to the cpuidle header
From: Daniel Lezcano @ 2012-07-06 11:07 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linux-pm, linaro-dev
In-Reply-To: <201207052243.15048.rjw@sisk.pl>
On 07/05/2012 10:43 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Daniel Lezcano wrote:
>> Move this specific flag to the header file.
>
> The patch evidently does more than that.
>
> Is it just a cleanup, or is there a functional reason for doing it?
It is just a cleanup.
Thanks for reviewing the patches.
-- Daniel
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>> drivers/idle/intel_idle.c | 8 --------
>> include/linux/cpuidle.h | 16 +++++++++++++---
>> 2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index fe95d54..3f0eb07 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -101,14 +101,6 @@ static int intel_idle_cpu_init(int cpu);
>> static struct cpuidle_state *cpuidle_state_table;
>>
>> /*
>> - * Set this flag for states where the HW flushes the TLB for us
>> - * and so we don't need cross-calls to keep it consistent.
>> - * If this flag is set, SW flushes the TLB, so even if the
>> - * HW doesn't do the flushing, this flag is safe to use.
>> - */
>> -#define CPUIDLE_FLAG_TLB_FLUSHED 0x10000
>> -
>> -/*
>> * States are indexed by the cstate number,
>> * which is also the index into the MWAIT hint array.
>> * Thus C0 is a dummy.
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 730e12e..be150c9 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -67,10 +67,20 @@ struct cpuidle_state {
>> * the cpuidle core the specified state can use the *
>> * enter_dead function. *
>> * *
>> + * CPUIDLE_FLAG_TLB_FLUSHED : Set this flag for states where the HW flushes the *
>> + * TLB for us and so we don't need cross-calls to *
>> + * keep it consistent. If this flag is set, SW *
>> + * flushes the TLB, so even if the HW doesn't do the *
>> + * flushing, this flag is safe to use. *
>> + * *
>> *******************************************************************************/
>> -#define CPUIDLE_FLAG_TIME_VALID (0x01)
>> -#define CPUIDLE_FLAG_COUPLED (0x02)
>> -#define CPUIDLE_FLAG_DEAD_VALID (0x04)
>> +#define CPUIDLE_FLAG_TIME_VALID (0x01)
>> +#define CPUIDLE_FLAG_COUPLED (0x02)
>> +#define CPUIDLE_FLAG_DEAD_VALID (0x04)
>> +#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
>> +
>> +
>> +
>>
>> /**
>> * cpuidle_get_statedata - retrieves private driver state data
>>
>
--
<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
^ permalink raw reply
* Re: [PATCH 3/4] cpuidle: move enter_dead to the driver structure
From: Daniel Lezcano @ 2012-07-06 11:05 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linux-pm, linaro-dev
In-Reply-To: <201207052240.57686.rjw@sisk.pl>
On 07/05/2012 10:40 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Daniel Lezcano wrote:
>> The 'enter_dead' function is only used for processor_idle.c
>> and the same function is used several times. We fall into the
>> same abuse with the multiple callbacks for the same function.
>
> This isn't abuse, mind you. This is a normal practice.
Well, that depends :)
I agree adding a callback per state is nice and flexible but if it is
not used, it is a waste of memory, even if it is 32 bytes.
>> This patch fixes that by moving the 'enter_dead' function to the
>> driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
>> to handle the callback conditional invokation.
>
> And how does that improve things?
In order to check if the play_dead is enabled for a specific state, we
check if the pointer is set. As it has been moved to a single function,
we need to add a flag to replace this check. That is not an improvement,
it replace a check by another check.
--
<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
^ permalink raw reply
* Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure
From: Daniel Lezcano @ 2012-07-06 10:58 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: lenb, linux-acpi, linux-pm, linaro-dev
In-Reply-To: <201207052238.44330.rjw@sisk.pl>
On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Daniel Lezcano wrote:
>> We have the state index passed as parameter to the 'enter' function.
>> Most of the drivers assign their 'enter' functions several times in
>> the cpuidle_state structure, as we have the index, we can delegate
>> to the driver to handle their own callback array.
>>
>> That will have the benefit of removing multiple lines of code in the
>> different drivers.
>
> Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason.
> Among other things, this was to avoid recurrence in callbacks - please see
> acpi_idle_enter_bm() for example.
>
> Now, if .enter() is moved to the driver structure, it will have to be an
> all-purpose complicated routine calling itself recursively at least in
> some cases. I'm not quite convinced that would be an improvement.
>
> On the other hand, I don't see anything wrong with setting several callback
> pointers to the same routine.
Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state
to a single structure stored in the driver. The drivers were modified
and cleanup to take into account this modification.
We saw some regressions like for example the 'disable' which were not
per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a
workaround more than a real fix). And now we have new architectures
(tegra3, big.LITTLE) with different latencies per cpu. Logically we
should revert Deepthi's patches but from my POV, going back and forth is
not a good solution (also we have to undo all modifications done in the
different drivers).
The main purpose of all these cleanup patches are to move out all
non-data information from the cpuidle_state structure in order to add a
new api which could be 'cpuidle_register_cpu_latency(int cpu, struct
cpuidle_latencies latencies)'.
For this specific patch, the 'enter' function for all the drivers is not
used [1] and one of the driver is about to use a single function [2]. So
we have only one driver is a couple of functions for this which can be
replaced by an array of callbacks in the driver itself as we have the index.
[1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html
[2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/4] cpuidle : move tlb flag to the cpuidle header
From: Rafael J. Wysocki @ 2012-07-05 20:43 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-acpi, linux-pm, linaro-dev
In-Reply-To: <1341494608-16591-4-git-send-email-daniel.lezcano@linaro.org>
On Thursday, July 05, 2012, Daniel Lezcano wrote:
> Move this specific flag to the header file.
The patch evidently does more than that.
Is it just a cleanup, or is there a functional reason for doing it?
Rafael
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/idle/intel_idle.c | 8 --------
> include/linux/cpuidle.h | 16 +++++++++++++---
> 2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index fe95d54..3f0eb07 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -101,14 +101,6 @@ static int intel_idle_cpu_init(int cpu);
> static struct cpuidle_state *cpuidle_state_table;
>
> /*
> - * Set this flag for states where the HW flushes the TLB for us
> - * and so we don't need cross-calls to keep it consistent.
> - * If this flag is set, SW flushes the TLB, so even if the
> - * HW doesn't do the flushing, this flag is safe to use.
> - */
> -#define CPUIDLE_FLAG_TLB_FLUSHED 0x10000
> -
> -/*
> * States are indexed by the cstate number,
> * which is also the index into the MWAIT hint array.
> * Thus C0 is a dummy.
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 730e12e..be150c9 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -67,10 +67,20 @@ struct cpuidle_state {
> * the cpuidle core the specified state can use the *
> * enter_dead function. *
> * *
> + * CPUIDLE_FLAG_TLB_FLUSHED : Set this flag for states where the HW flushes the *
> + * TLB for us and so we don't need cross-calls to *
> + * keep it consistent. If this flag is set, SW *
> + * flushes the TLB, so even if the HW doesn't do the *
> + * flushing, this flag is safe to use. *
> + * *
> *******************************************************************************/
> -#define CPUIDLE_FLAG_TIME_VALID (0x01)
> -#define CPUIDLE_FLAG_COUPLED (0x02)
> -#define CPUIDLE_FLAG_DEAD_VALID (0x04)
> +#define CPUIDLE_FLAG_TIME_VALID (0x01)
> +#define CPUIDLE_FLAG_COUPLED (0x02)
> +#define CPUIDLE_FLAG_DEAD_VALID (0x04)
> +#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
> +
> +
> +
>
> /**
> * cpuidle_get_statedata - retrieves private driver state data
>
^ permalink raw reply
* Re: [PATCH 3/4] cpuidle: move enter_dead to the driver structure
From: Rafael J. Wysocki @ 2012-07-05 20:40 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: linux-acpi, linux-pm, linaro-dev
In-Reply-To: <1341494608-16591-3-git-send-email-daniel.lezcano@linaro.org>
On Thursday, July 05, 2012, Daniel Lezcano wrote:
> The 'enter_dead' function is only used for processor_idle.c
> and the same function is used several times. We fall into the
> same abuse with the multiple callbacks for the same function.
This isn't abuse, mind you. This is a normal practice.
> This patch fixes that by moving the 'enter_dead' function to the
> driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
> to handle the callback conditional invokation.
And how does that improve things?
Rafael
^ permalink raw reply
* Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure
From: Rafael J. Wysocki @ 2012-07-05 20:38 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: lenb, linux-acpi, linux-pm, linaro-dev
In-Reply-To: <1341494608-16591-2-git-send-email-daniel.lezcano@linaro.org>
On Thursday, July 05, 2012, Daniel Lezcano wrote:
> We have the state index passed as parameter to the 'enter' function.
> Most of the drivers assign their 'enter' functions several times in
> the cpuidle_state structure, as we have the index, we can delegate
> to the driver to handle their own callback array.
>
> That will have the benefit of removing multiple lines of code in the
> different drivers.
Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason.
Among other things, this was to avoid recurrence in callbacks - please see
acpi_idle_enter_bm() for example.
Now, if .enter() is moved to the driver structure, it will have to be an
all-purpose complicated routine calling itself recursively at least in
some cases. I'm not quite convinced that would be an improvement.
On the other hand, I don't see anything wrong with setting several callback
pointers to the same routine.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH 1/4] acpi: intel_idle : break dependency between modules
From: Rafael J. Wysocki @ 2012-07-05 20:26 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: lenb, linux-acpi, linux-pm, linaro-dev
In-Reply-To: <1341494608-16591-1-git-send-email-daniel.lezcano@linaro.org>
On Thursday, July 05, 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>
> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
OK, I'm taking this one.
Thanks,
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 13103aeb..7048b97 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -429,18 +429,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 8684a0d..a6b3f2e 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -207,14 +207,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;}
>
^ permalink raw reply
* Re: linux-next : cpuidle - could you add my tree please
From: Daniel Lezcano @ 2012-07-05 13:33 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Stephen Rothwell, Linus Walleij, linux-acpi, linux-pm,
Lists Linaro-dev, Linux Kernel Mailing List, Kevin Hilman,
Peter De Schrijver, Amit Kucheria, linux-next, Colin Cross,
Andrew Morton, Linus Torvalds, Rob Lee, lenb@kernel.org
In-Reply-To: <201207031854.51163.rjw@sisk.pl>
On 07/03/2012 06:54 PM, Rafael J. Wysocki wrote:
> On Tuesday, July 03, 2012, Daniel Lezcano wrote:
>> On 07/03/2012 03:19 PM, Stephen Rothwell wrote:
>>> Hi Daniel,
>>>
>>> On Tue, 03 Jul 2012 14:56:58 +0200 Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>>
>>>>> So do you have a branch in the cpuidle-next.git tree that isn't going to
>>>>> be rebased?
>>>>
>>>> No. I am following Linus tree and adding the patches on top of it.
>>>
>>> Please don't rebase your tree more than necessary - it just makes thing
>>> hard for anyone using your tree as a base for further development and
>>> throws away any testing you may have done.
>>
>> Ok, let me sync with Len and Rafael about the best way to do that.
>
> Please create a branch in your tree for me to pull from and let me know
> which one it is. Please note that this branch must not be rebased after I've
> pulled from it and it's going to be included into my linux-next branch
> automatically.
Ok that sounds good.
Let me put in place the branch and rework my patches because they
conflict with the 'disable' flag moved to the per cpu structure.
In the meantime, I will send you the other patches which do not conflict.
> I'll include it into my v3.6 push, because I have a couple of cpuidle patches
> queued up already. We'll need to discuss the future of it after 3.6, though.
Ok, cool.
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
^ permalink raw reply
* [PATCH 4/4] cpuidle : move tlb flag to the cpuidle header
From: Daniel Lezcano @ 2012-07-05 13:23 UTC (permalink / raw)
To: rjw, lenb; +Cc: linux-acpi, linux-pm, linaro-dev
In-Reply-To: <1341494608-16591-1-git-send-email-daniel.lezcano@linaro.org>
Move this specific flag to the header file.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
drivers/idle/intel_idle.c | 8 --------
include/linux/cpuidle.h | 16 +++++++++++++---
2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index fe95d54..3f0eb07 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -101,14 +101,6 @@ static int intel_idle_cpu_init(int cpu);
static struct cpuidle_state *cpuidle_state_table;
/*
- * Set this flag for states where the HW flushes the TLB for us
- * and so we don't need cross-calls to keep it consistent.
- * If this flag is set, SW flushes the TLB, so even if the
- * HW doesn't do the flushing, this flag is safe to use.
- */
-#define CPUIDLE_FLAG_TLB_FLUSHED 0x10000
-
-/*
* States are indexed by the cstate number,
* which is also the index into the MWAIT hint array.
* Thus C0 is a dummy.
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 730e12e..be150c9 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -67,10 +67,20 @@ struct cpuidle_state {
* the cpuidle core the specified state can use the *
* enter_dead function. *
* *
+ * CPUIDLE_FLAG_TLB_FLUSHED : Set this flag for states where the HW flushes the *
+ * TLB for us and so we don't need cross-calls to *
+ * keep it consistent. If this flag is set, SW *
+ * flushes the TLB, so even if the HW doesn't do the *
+ * flushing, this flag is safe to use. *
+ * *
*******************************************************************************/
-#define CPUIDLE_FLAG_TIME_VALID (0x01)
-#define CPUIDLE_FLAG_COUPLED (0x02)
-#define CPUIDLE_FLAG_DEAD_VALID (0x04)
+#define CPUIDLE_FLAG_TIME_VALID (0x01)
+#define CPUIDLE_FLAG_COUPLED (0x02)
+#define CPUIDLE_FLAG_DEAD_VALID (0x04)
+#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
+
+
+
/**
* cpuidle_get_statedata - retrieves private driver state data
--
1.7.5.4
^ permalink raw reply related
* [PATCH 3/4] cpuidle: move enter_dead to the driver structure
From: Daniel Lezcano @ 2012-07-05 13:23 UTC (permalink / raw)
To: rjw, lenb; +Cc: linux-acpi, linux-pm, linaro-dev
In-Reply-To: <1341494608-16591-1-git-send-email-daniel.lezcano@linaro.org>
The 'enter_dead' function is only used for processor_idle.c
and the same function is used several times. We fall into the
same abuse with the multiple callbacks for the same function.
This patch fixes that by moving the 'enter_dead' function to the
driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
to handle the callback conditional invokation.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
drivers/acpi/processor_idle.c | 7 +++----
drivers/cpuidle/cpuidle.c | 4 ++--
include/linux/cpuidle.h | 26 ++++++++++++++++++++------
3 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 6d9ec3e..33310fb 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1024,6 +1024,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
struct cpuidle_driver acpi_idle_driver = {
.name = "acpi_idle",
.owner = THIS_MODULE,
+ .enter_dead = acpi_idle_play_dead,
};
/**
@@ -1132,16 +1133,14 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
case ACPI_STATE_C1:
if (cx->entry_method == ACPI_CSTATE_FFH)
state->flags |= CPUIDLE_FLAG_TIME_VALID;
-
+ state->flags |= CPUIDLE_FLAG_DEAD_VALID;
state->enter = acpi_idle_enter_c1;
- state->enter_dead = acpi_idle_play_dead;
drv->safe_state_index = count;
break;
case ACPI_STATE_C2:
- state->flags |= CPUIDLE_FLAG_TIME_VALID;
+ state->flags |= CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_DEAD_VALID;
state->enter = acpi_idle_enter_simple;
- state->enter_dead = acpi_idle_play_dead;
drv->safe_state_index = count;
break;
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index c064144..7dcfa45 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -81,14 +81,14 @@ int cpuidle_play_dead(void)
for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
struct cpuidle_state *s = &drv->states[i];
- if (s->power_usage < power_usage && s->enter_dead) {
+ if (s->power_usage < power_usage && (s->flags & CPUIDLE_FLAG_DEAD_VALID)) {
power_usage = s->power_usage;
dead_state = i;
}
}
if (dead_state != -1)
- return drv->states[dead_state].enter_dead(dev, dead_state);
+ return drv->enter_dead(dev, dead_state);
return -ENODEV;
}
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 4d816c7..730e12e 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -52,14 +52,25 @@ struct cpuidle_state {
int (*enter) (struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index);
-
- int (*enter_dead) (struct cpuidle_device *dev, int index);
};
-/* Idle State Flags */
-#define CPUIDLE_FLAG_TIME_VALID (0x01) /* is residency time measurable? */
-
-#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
+/********************************************************************************
+ * Idle State Flags: *
+ * CPUIDLE_FLAG_TIME_VALID : the drivers could measure the state residency *
+ * time and store it in the 'target_residency'field. *
+ * This flag will tell the cpuidle core to use this *
+ * value to compute an accurate prediction. *
+ * *
+ * CPUIDLE_FLAG_COUPLED : state applies to multiple cpus *
+ * *
+ * CPUIDLE_FLAG_DEAD_VALID : the driver may want to deal with dead cpus, tell *
+ * the cpuidle core the specified state can use the *
+ * enter_dead function. *
+ * *
+ *******************************************************************************/
+#define CPUIDLE_FLAG_TIME_VALID (0x01)
+#define CPUIDLE_FLAG_COUPLED (0x02)
+#define CPUIDLE_FLAG_DEAD_VALID (0x04)
/**
* cpuidle_get_statedata - retrieves private driver state data
@@ -132,6 +143,9 @@ struct cpuidle_driver {
int state_count;
int safe_state_index;
int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int);
+#ifdef CONFIG_ACPI
+ int (*enter_dead)(struct cpuidle_device *, int);
+#endif
};
#ifdef CONFIG_CPU_IDLE
--
1.7.5.4
^ permalink raw reply related
* [PATCH 2/4] cpuidle: define the enter function in the driver structure
From: Daniel Lezcano @ 2012-07-05 13:23 UTC (permalink / raw)
To: rjw, lenb; +Cc: linux-acpi, linux-pm, linaro-dev
In-Reply-To: <1341494608-16591-1-git-send-email-daniel.lezcano@linaro.org>
We have the state index passed as parameter to the 'enter' function.
Most of the drivers assign their 'enter' functions several times in
the cpuidle_state structure, as we have the index, we can delegate
to the driver to handle their own callback array.
That will have the benefit of removing multiple lines of code in the
different drivers.
In order to smoothly modify the driver, the 'enter' function are in
the driver structure and in the cpuidle state structure. That will
let the time to modify the different drivers one by one.
So the 'cpuidle_enter' function checks if the 'enter' callback is
assigned in the driver structure and use it, otherwise it invokes
the 'enter' assigned to the cpuidle_state.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
drivers/cpuidle/cpuidle.c | 4 +++-
include/linux/cpuidle.h | 1 +
2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 0132706..c064144 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -46,7 +46,9 @@ static inline int cpuidle_enter(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int index)
{
struct cpuidle_state *target_state = &drv->states[index];
- return target_state->enter(dev, drv, index);
+
+ return drv->enter ? drv->enter(dev, drv, index) :
+ target_state->enter(dev, drv, index);
}
static inline int cpuidle_enter_tk(struct cpuidle_device *dev,
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index a6b3f2e..4d816c7 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -131,6 +131,7 @@ struct cpuidle_driver {
struct cpuidle_state states[CPUIDLE_STATE_MAX];
int state_count;
int safe_state_index;
+ int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int);
};
#ifdef CONFIG_CPU_IDLE
--
1.7.5.4
^ permalink raw reply related
* [PATCH 1/4] acpi: intel_idle : break dependency between modules
From: Daniel Lezcano @ 2012-07-05 13:23 UTC (permalink / raw)
To: rjw, lenb; +Cc: linux-acpi, linux-pm, linaro-dev
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>
Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
---
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 13103aeb..7048b97 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -429,18 +429,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 8684a0d..a6b3f2e 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -207,14 +207,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
^ permalink raw reply related
* Re: [PATCH -v5 0/6] notifier error injection
From: Akinobu Mita @ 2012-07-04 10:07 UTC (permalink / raw)
To: Andrew Morton
Cc: Michael Ellerman, Greg KH, linux-kernel, linux-mm, Paul Mackerras,
Américo Wang, linux-pm, linuxppc-dev
In-Reply-To: <20120703134120.dc89c7ae.akpm@linux-foundation.org>
2012/7/4 Andrew Morton <akpm@linux-foundation.org>:
> On Sat, 30 Jun 2012 14:59:24 +0900
> Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
>> This provides kernel modules that can be used to test the error handling
>> of notifier call chain failures by injecting artifical errors to the
>> following notifier chain callbacks.
>
> No updates to Documentation/fault-injection/?
Thanks for the remainder.
I'll prepare to add a document to it.
^ permalink raw reply
* Re: [PATCH -v5 0/6] notifier error injection
From: Andrew Morton @ 2012-07-03 20:41 UTC (permalink / raw)
To: Akinobu Mita
Cc: Michael Ellerman, Greg KH, linux-kernel, linux-mm, Paul Mackerras,
Américo Wang, linux-pm, linuxppc-dev
In-Reply-To: <1341035970-20490-1-git-send-email-akinobu.mita@gmail.com>
On Sat, 30 Jun 2012 14:59:24 +0900
Akinobu Mita <akinobu.mita@gmail.com> wrote:
> This provides kernel modules that can be used to test the error handling
> of notifier call chain failures by injecting artifical errors to the
> following notifier chain callbacks.
No updates to Documentation/fault-injection/?
^ permalink raw reply
* Re: linux-next : cpuidle - could you add my tree please
From: Rafael J. Wysocki @ 2012-07-03 19:33 UTC (permalink / raw)
To: Linus Walleij
Cc: Kevin Hilman, Len.Brown, Rob Lee, Lists Linaro-dev,
Stephen Rothwell, Peter De Schrijver, Linux Kernel Mailing List,
Amit Kucheria, linux-acpi, linux-next, Colin Cross, linux-pm,
Linus Torvalds, Andrew Morton
In-Reply-To: <CACRpkdY-vmA57uXWCzG7b7JXkZi0n-HCdCK0C4wPFgv51Q_KFA@mail.gmail.com>
On Tuesday, July 03, 2012, Linus Walleij wrote:
> On Tue, Jul 3, 2012 at 12:14 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>
> > If that makes sense to add myself to the MAINTAINER file as a
> > co-maintainer (understand: send to me also the patches, so I can take
> > care of them if Len does not respond), I am ok with that.
>
> What about a patch adding both you and Len as MAINTAINERs,
> right now there is noone just some informal consensus and noone
> really knows who to send patches to. Let's formalize it.
Send them to me for now. We'll settle the issue when Len is back.
Thanks,
Rafael
^ permalink raw reply
* Re: linux-next : cpuidle - could you add my tree please
From: Linus Walleij @ 2012-07-03 19:20 UTC (permalink / raw)
To: Daniel Lezcano, Len.Brown
Cc: Kevin Hilman, Stephen Rothwell, Rob Lee, Lists Linaro-dev,
Peter De Schrijver, Linux Kernel Mailing List, Amit Kucheria,
linux-acpi, linux-next, Colin Cross, linux-pm, Linus Torvalds,
Andrew Morton
In-Reply-To: <4FF21D48.50301@linaro.org>
On Tue, Jul 3, 2012 at 12:14 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> If that makes sense to add myself to the MAINTAINER file as a
> co-maintainer (understand: send to me also the patches, so I can take
> care of them if Len does not respond), I am ok with that.
What about a patch adding both you and Len as MAINTAINERs,
right now there is noone just some informal consensus and noone
really knows who to send patches to. Let's formalize it.
Yours,
Linus Walleij
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox