On Monday, March 02, 2015 02:13:05 PM Rafael J. Wysocki wrote:
> On Monday, March 02, 2015 10:08:23 AM Lorenzo Pieralisi wrote:
> > On Sat, Feb 28, 2015 at 11:58:21PM +0000, Rafael J. Wysocki wrote:
> > > On Saturday, February 28, 2015 11:54:23 AM Lorenzo Pieralisi wrote:
>
> [cut]
>
> > > Index: linux-pm/drivers/cpuidle/cpuidle.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> > > +++ linux-pm/drivers/cpuidle/cpuidle.c
> > > @@ -230,15 +230,39 @@ int cpuidle_select(struct cpuidle_driver
> > > * @dev: the cpuidle device
> > > * @index: the index in the idle state table
> > > *
> > > - * Returns the index in the idle state, < 0 in case of error.
> > > - * The error code depends on the backend driver
> > > + * Returns the index in the idle state, < 0 in case of error. -EBUSY is
> > > + * returned to indicate that the target state was temporarily
> > > inaccessible.
> > > + * The other error codes depend on the backend driver.
> > > */
> > > int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> > > int index)
> > > {
> > > - if (cpuidle_state_is_coupled(dev, drv, index))
> > > - return cpuidle_enter_state_coupled(dev, drv, index);
> > > - return cpuidle_enter_state(dev, drv, index);
> > > + unsigned int broadcast;
> > > + int ret;
> > > +
> > > + broadcast = drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP;
> > > +
> > > + /*
> > > + * Tell the time framework to switch to a broadcast timer
> > > + * because our local timer will be shutdown. If a local timer
> > > + * is used from another cpu as a broadcast timer, this call may
> > > + * fail if it is not available
> > > + */
> > > + if (broadcast) {
> > > + ret = clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
> > > + &dev->cpu);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + ret = cpuidle_state_is_coupled(dev, drv, index) ?
> > > + cpuidle_enter_state_coupled(dev, drv, index) :
> > > + cpuidle_enter_state(dev, drv, index);
> > > +
> > > + if (broadcast)
> > > + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> > > +
> > > + return ret;
> >
> > You have to check this return value in cpuidle_enter_freeze() too
> > otherwise we return to the idle thread on -EBUSY without
> > even executing arch_cpu_idle() and with IRQ disabled, code hits
> > the WARN_ON_ONCE line 180.
>
> Right.
>
> > There are multiple ways of fixing the issue, either you check the
> > cpuidle_enter_freeze() return value (you add one) to cpuidle_idle_call()
> > to make code consistent with the cpuidle_idle_call "normal" idle
> > behaviour or you add the return value check in cpuidle_enter_freeze(),
> > I am fine both ways.
>
> Well, in both cases we'd end up with a function enabling interrupts on exit
> in some cases and not doing that in some other ones. Not nice.
>
> Below is an alternative to that (on top of the previous patches). Can you
> test it please?
Actually, this one is still slightly incorrect, because we only should call
cpuidle_reflect() if we've called cpuidle_select() before. Also it's better
to pass cpuidle_driver and cpuidle_device to all functions called by
cpuidle_idle_call().
Two patches will follow. [1/2] is a cleanup re-arranging the code in
cpuidle_idle_call() to move the fallback path to the end of the function.
[2/2] is a replacement for the patch sent previously.
Please test.
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
