Subject: Re: [PATCH 04/36] cpuidle,intel_idle: Fix CPUIDLE_FLAG_IRQ_ENABLE

From: Jacob Pan <[email-masked]>
Date: Thu, 16 Jun 2022 14:26:56 -0700

, Arnd Bergmann <[email-masked]>, [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], [email-masked], palmer@dab
 belt.com, [email-masked], [email-masked], [email-masked], [email-masked]
Errors-To: linuxppc-dev-bounces+linuxppc-dev=[email-masked]
Sender: "Linuxppc-dev" <linuxppc-dev-bounces+linuxppc-dev=[email-masked]>

Hi Peter,

On Mon, 13 Jun 2022 10:44:22 +0200, Peter Zijlstra <[email-masked]>
wrote:

> On Thu, Jun 09, 2022 at 04:49:21PM -0700, Jacob Pan wrote:
> > Hi Peter,
> > 
> > On Wed, 08 Jun 2022 16:27:27 +0200, Peter Zijlstra
> > <[email-masked]> wrote:
> >   
> > > Commit c227233ad64c ("intel_idle: enable interrupts before C1 on
> > > Xeons") wrecked intel_idle in two ways:
> > > 
> > >  - must not have tracing in idle functions
> > >  - must return with IRQs disabled
> > > 
> > > Additionally, it added a branch for no good reason.
> > > 
> > > Fixes: c227233ad64c ("intel_idle: enable interrupts before C1 on
> > > Xeons") Signed-off-by: Peter Zijlstra (Intel) <[email-masked]>
> > > ---
> > >  drivers/idle/intel_idle.c |   48
> > > +++++++++++++++++++++++++++++++++++----------- 1 file changed, 37
> > > insertions(+), 11 deletions(-)
> > > 
> > > --- a/drivers/idle/intel_idle.c
> > > +++ b/drivers/idle/intel_idle.c
> > > @@ -129,21 +137,37 @@ static unsigned int mwait_substates __in
> > >   *
> > >   * Must be called under local_irq_disable().
> > >   */  
> > nit: this comment is no long true, right?  
> 
> It still is, all the idle routines are called with interrupts disabled,
> but must also exit with interrupts disabled.
> 
> If the idle method requires interrupts to be enabled, it must be sure to
> disable them again before returning. Given all the RCU/tracing concerns
> it must use raw_local_irq_*() for this though.
Makes sense, it is just little confusing when the immediate caller does
raw_local_irq_enable() which does not cancel out local_irq_disable().

Thanks,

Jacob