Re: [kvm-ppc-devel] [PATCH] Add idle power save for ppc 4xx

[Arnd Bergmann <arnd@arndb.de>]

On Monday 31 March 2008, Jerone Young wrote:
> > 
> > > +{
> > > +   unsigned long msr_save;
> > > +
> > > +   /* set wait state MSR */
> > > +   local_irq_enable();
> > > +   msr_save = mfmsr();
> > > +   mtmsr(msr_save|MSR_WE);
> > 
> > Why don't you |MSR_WE|MSR_EE at the same time?
> 
> You technically can do this. But the question is do all 4xx cpus use
> MSR_EE to enable interrupts? I can assume they do (from what I know),
> but figured it would be safer to make the local_irq_enable() call.
> I can change it to just set the MSR_EE bit though, since all 4xx cpus
> (as far as I know) use it.

For correctness reasons, you actually have to set both EE and WE
simultaneously. Otherwise, an interrupt can come in between the two
mtmsr(), mark some thread as runnable and then go to sleep here
without ever checking need_resched() until the next interrupt,
which may take an indefinite time.

> > 
> > > +   local_irq_disable();
> > > +}
> > 
> > None of the other power_save() implementations need this. In fact many
> > of them don't even seem to return; they just loop around mtmsr.
> 
> Sure it can be removed. Though with the comment in the mach_dep
> structure about power_save. It specifically says that interrupts are off
> when it is called. So I was following it here mainly. But I can remove
> the disabling of interrupts, since mtmsr is the only used under
> power_save.

With the current code, it shouldn't matter, because cpu_idle enables
the interrupts right after calling ppc_md->power_save(), but it would
be reasonable to disable the interrupts here anyway, if only to make
the function return with the same state that it was entered.

IMHO, the function should be

void ppc4xx_idle()
{
        unsigned long msr_save;

        msr_save = mfmsr();
	/* enter idle mode */
        mtmsr(msr_save|MSR_WE|MSR_EE);
	/* disable interrupts again */
        mtmsr(msr_save);
}

	Arnd <><

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel