| Message ID | 1377287112-12018-1-git-send-email-ccross@android.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 08/23/2013 01:45 PM, Colin Cross wrote: > Calling cpuidle_enter_state is expected to return with interrupts > enabled, but interrupts must be disabled before starting the > ready loop synchronization stage. Call local_irq_disable after > each call to cpuidle_enter_state for the safe state. Tested-by: Stephen Warren <swarren@nvidia.com> Note: I tested the current Tegra cpuidle code that's in next-20130819. IIRC, Joseph reported the issue when trying to enable some additional feature in Tegra30 cpuidle. I didn't actually try to enable whatever that was; I just briefly tested for regressions in the existing code configuration. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 23, 2013 at 4:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 08/23/2013 01:45 PM, Colin Cross wrote: >> Calling cpuidle_enter_state is expected to return with interrupts >> enabled, but interrupts must be disabled before starting the >> ready loop synchronization stage. Call local_irq_disable after >> each call to cpuidle_enter_state for the safe state. > > Tested-by: Stephen Warren <swarren@nvidia.com> > > Note: I tested the current Tegra cpuidle code that's in next-20130819. > IIRC, Joseph reported the issue when trying to enable some additional > feature in Tegra30 cpuidle. I didn't actually try to enable whatever > that was; I just briefly tested for regressions in the existing code > configuration. Is that for the series or just this patch? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/23/2013 06:22 PM, Colin Cross wrote: > On Fri, Aug 23, 2013 at 4:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 08/23/2013 01:45 PM, Colin Cross wrote: >>> Calling cpuidle_enter_state is expected to return with interrupts >>> enabled, but interrupts must be disabled before starting the >>> ready loop synchronization stage. Call local_irq_disable after >>> each call to cpuidle_enter_state for the safe state. >> >> Tested-by: Stephen Warren <swarren@nvidia.com> >> >> Note: I tested the current Tegra cpuidle code that's in next-20130819. >> IIRC, Joseph reported the issue when trying to enable some additional >> feature in Tegra30 cpuidle. I didn't actually try to enable whatever >> that was; I just briefly tested for regressions in the existing code >> configuration. > > Is that for the series or just this patch? The series. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, August 23, 2013 12:45:10 PM Colin Cross wrote: > Calling cpuidle_enter_state is expected to return with interrupts > enabled, but interrupts must be disabled before starting the > ready loop synchronization stage. Call local_irq_disable after > each call to cpuidle_enter_state for the safe state. > > CC: stable@vger.kernel.org > Signed-off-by: Colin Cross <ccross@android.com> I've queued up all thress for 3.12, but I wonder what stable versions they should be included into? All of them or just a subset? Rafael > --- > drivers/cpuidle/coupled.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c > index 2a297f8..db92bcb 100644 > --- a/drivers/cpuidle/coupled.c > +++ b/drivers/cpuidle/coupled.c > @@ -460,6 +460,7 @@ int cpuidle_enter_state_coupled(struct cpuidle_device *dev, > } > entered_state = cpuidle_enter_state(dev, drv, > dev->safe_state_index); > + local_irq_disable(); > } > > /* Read barrier ensures online_count is read after prevent is cleared */ > @@ -485,6 +486,7 @@ retry: > > entered_state = cpuidle_enter_state(dev, drv, > dev->safe_state_index); > + local_irq_disable(); > } > > if (cpuidle_coupled_clear_pokes(dev->cpu)) { >
On Wed, Aug 28, 2013 at 2:28 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Friday, August 23, 2013 12:45:10 PM Colin Cross wrote: >> Calling cpuidle_enter_state is expected to return with interrupts >> enabled, but interrupts must be disabled before starting the >> ready loop synchronization stage. Call local_irq_disable after >> each call to cpuidle_enter_state for the safe state. >> >> CC: stable@vger.kernel.org >> Signed-off-by: Colin Cross <ccross@android.com> > > I've queued up all thress for 3.12, but I wonder what stable versions they > should be included into? All of them or just a subset? The patches apply cleanly back to v3.6. Joseph Lo had 2 minor comments on patch 2 (changing Tegra3 to Tegra20 in the commit message, replacing cpuidle_coupled_poke_pending with cpuidle_coupled_poked in the comment above cpuidle_coupled_poked), do you want to fix those up locally or should I resend the series? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, August 28, 2013 03:00:58 PM Colin Cross wrote: > On Wed, Aug 28, 2013 at 2:28 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Friday, August 23, 2013 12:45:10 PM Colin Cross wrote: > >> Calling cpuidle_enter_state is expected to return with interrupts > >> enabled, but interrupts must be disabled before starting the > >> ready loop synchronization stage. Call local_irq_disable after > >> each call to cpuidle_enter_state for the safe state. > >> > >> CC: stable@vger.kernel.org > >> Signed-off-by: Colin Cross <ccross@android.com> > > > > I've queued up all thress for 3.12, but I wonder what stable versions they > > should be included into? All of them or just a subset? > > The patches apply cleanly back to v3.6. > > Joseph Lo had 2 minor comments on patch 2 (changing Tegra3 to Tegra20 > in the commit message, replacing cpuidle_coupled_poke_pending with > cpuidle_coupled_poked in the comment above cpuidle_coupled_poked), do > you want to fix those up locally or should I resend the series? I'd prefer it to be resent, then, but just the patch(es) that changed. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 24 August 2013 01:15 AM, Colin Cross wrote: > Calling cpuidle_enter_state is expected to return with interrupts > enabled, but interrupts must be disabled before starting the > ready loop synchronization stage. Call local_irq_disable after > each call to cpuidle_enter_state for the safe state. > > CC: stable@vger.kernel.org > Signed-off-by: Colin Cross <ccross@android.com> > --- > drivers/cpuidle/coupled.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c > index 2a297f8..db92bcb 100644 > --- a/drivers/cpuidle/coupled.c > +++ b/drivers/cpuidle/coupled.c > @@ -460,6 +460,7 @@ int cpuidle_enter_state_coupled(struct cpuidle_device *dev, > } > entered_state = cpuidle_enter_state(dev, drv, > dev->safe_state_index); > + local_irq_disable(); Colin, There is still some window where irq remains enabled after exiting safe state. It may introduce some corner case. Instead of this can we pass a parameter to cpuidle_enter_state indicating that irq has to be enabled or not after exit from idle state, which would be false when entering safe state from coupled idle. > } > > /* Read barrier ensures online_count is read after prevent is cleared */ > @@ -485,6 +486,7 @@ retry: > > entered_state = cpuidle_enter_state(dev, drv, > dev->safe_state_index); > + local_irq_disable(); > } > > if (cpuidle_coupled_clear_pokes(dev->cpu)) { -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 28, 2013 at 11:50 PM, Prashant Gaikwad <pgaikwad@nvidia.com> wrote: > On Saturday 24 August 2013 01:15 AM, Colin Cross wrote: >> >> Calling cpuidle_enter_state is expected to return with interrupts >> enabled, but interrupts must be disabled before starting the >> ready loop synchronization stage. Call local_irq_disable after >> each call to cpuidle_enter_state for the safe state. >> >> CC: stable@vger.kernel.org >> Signed-off-by: Colin Cross <ccross@android.com> >> --- >> drivers/cpuidle/coupled.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c >> index 2a297f8..db92bcb 100644 >> --- a/drivers/cpuidle/coupled.c >> +++ b/drivers/cpuidle/coupled.c >> @@ -460,6 +460,7 @@ int cpuidle_enter_state_coupled(struct cpuidle_device >> *dev, >> } >> entered_state = cpuidle_enter_state(dev, drv, >> dev->safe_state_index); >> + local_irq_disable(); > > > Colin, > > There is still some window where irq remains enabled after exiting safe > state. It may introduce some corner case. > Instead of this can we pass a parameter to cpuidle_enter_state indicating > that irq has to be enabled or not after exit from idle state, which would be > false when entering safe state from coupled idle. It's fine for irqs to be enabled when exiting the safe state, in fact on further inspection this patch isn't strictly necessary at all - we always enable interrupts inside cpuidle_coupled_clear_pokes soon after cpuidle_enter_state returns, and then disable them again. It's probably better to disable interrupts right after cpuidle_enter_state, it makes sure interrupts are consistently disabled when calling cpuidle_coupled_set_waiting, cpuidle_coupled_cpus_waiting and cpuidle_coupled_clear_pokes, although that doesn't matter in the current implementation. Rafael, feel free to drop the stable annotation from this patch. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, August 29, 2013 12:12:17 AM Colin Cross wrote: > On Wed, Aug 28, 2013 at 11:50 PM, Prashant Gaikwad <pgaikwad@nvidia.com> wrote: > > On Saturday 24 August 2013 01:15 AM, Colin Cross wrote: > >> > >> Calling cpuidle_enter_state is expected to return with interrupts > >> enabled, but interrupts must be disabled before starting the > >> ready loop synchronization stage. Call local_irq_disable after > >> each call to cpuidle_enter_state for the safe state. > >> > >> CC: stable@vger.kernel.org > >> Signed-off-by: Colin Cross <ccross@android.com> > >> --- > >> drivers/cpuidle/coupled.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c > >> index 2a297f8..db92bcb 100644 > >> --- a/drivers/cpuidle/coupled.c > >> +++ b/drivers/cpuidle/coupled.c > >> @@ -460,6 +460,7 @@ int cpuidle_enter_state_coupled(struct cpuidle_device > >> *dev, > >> } > >> entered_state = cpuidle_enter_state(dev, drv, > >> dev->safe_state_index); > >> + local_irq_disable(); > > > > > > Colin, > > > > There is still some window where irq remains enabled after exiting safe > > state. It may introduce some corner case. > > Instead of this can we pass a parameter to cpuidle_enter_state indicating > > that irq has to be enabled or not after exit from idle state, which would be > > false when entering safe state from coupled idle. > > It's fine for irqs to be enabled when exiting the safe state, in fact > on further inspection this patch isn't strictly necessary at all - we > always enable interrupts inside cpuidle_coupled_clear_pokes soon after > cpuidle_enter_state returns, and then disable them again. It's > probably better to disable interrupts right after cpuidle_enter_state, > it makes sure interrupts are consistently disabled when calling > cpuidle_coupled_set_waiting, cpuidle_coupled_cpus_waiting and > cpuidle_coupled_clear_pokes, although that doesn't matter in the > current implementation. > > Rafael, feel free to drop the stable annotation from this patch. I will, thanks!
diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c index 2a297f8..db92bcb 100644 --- a/drivers/cpuidle/coupled.c +++ b/drivers/cpuidle/coupled.c @@ -460,6 +460,7 @@ int cpuidle_enter_state_coupled(struct cpuidle_device *dev, } entered_state = cpuidle_enter_state(dev, drv, dev->safe_state_index); + local_irq_disable(); } /* Read barrier ensures online_count is read after prevent is cleared */ @@ -485,6 +486,7 @@ retry: entered_state = cpuidle_enter_state(dev, drv, dev->safe_state_index); + local_irq_disable(); } if (cpuidle_coupled_clear_pokes(dev->cpu)) {
Calling cpuidle_enter_state is expected to return with interrupts enabled, but interrupts must be disabled before starting the ready loop synchronization stage. Call local_irq_disable after each call to cpuidle_enter_state for the safe state. CC: stable@vger.kernel.org Signed-off-by: Colin Cross <ccross@android.com> --- drivers/cpuidle/coupled.c | 2 ++ 1 file changed, 2 insertions(+)