Chenging 2 bits in MSR in ppc6xx_idle() with 1 command?

Chenging 2 bits in MSR in ppc6xx_idle() with 1 command?

[Guennadi Liakhovetski]

Hi

Here's a code fragment from ppc6xx_idle(), which should send the CPU into 
a powersaving mode (DOZE or NAP) and re-enable interrupts after a 
local_irq_disable():

	mfmsr	r7
	ori	r7,r7,MSR_EE
	oris	r7,r7,MSR_POW@h
1:	sync
	mtmsr	r7
	isync
	b	1b

Whereas MPC8245's user manual says, that when setting the MSR_POW bit in 
the MSR one may not set any other bit in it with the same instruction. 
Does this mean that the above does not actually work on those (and 
similar) CPUs or does it work because of the loop?

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: Chenging 2 bits in MSR in ppc6xx_idle() with 1 command?

[Benjamin Herrenschmidt]

On Mon, 2006-12-25 at 21:07 +0100, Guennadi Liakhovetski wrote:
> Hi
> 
> Here's a code fragment from ppc6xx_idle(), which should send the CPU into 
> a powersaving mode (DOZE or NAP) and re-enable interrupts after a 
> local_irq_disable():
> 
> 	mfmsr	r7
> 	ori	r7,r7,MSR_EE
> 	oris	r7,r7,MSR_POW@h
> 1:	sync
> 	mtmsr	r7
> 	isync
> 	b	1b
> 
> Whereas MPC8245's user manual says, that when setting the MSR_POW bit in 
> the MSR one may not set any other bit in it with the same instruction. 
> Does this mean that the above does not actually work on those (and 
> similar) CPUs or does it work because of the loop?

That doc bit looks a bit strange. The kernel pretty much relies on
setting MSR:EE and MSR:POW atomicaly.

Ben.

Re: Chenging 2 bits in MSR in ppc6xx_idle() with 1 command?

[Guennadi Liakhovetski]

On Wed, 27 Dec 2006, Benjamin Herrenschmidt wrote:

> On Mon, 2006-12-25 at 21:07 +0100, Guennadi Liakhovetski wrote:
> > Hi
> > 
> > Here's a code fragment from ppc6xx_idle(), which should send the CPU into 
> > a powersaving mode (DOZE or NAP) and re-enable interrupts after a 
> > local_irq_disable():
> > 
> > 	mfmsr	r7
> > 	ori	r7,r7,MSR_EE
> > 	oris	r7,r7,MSR_POW@h
> > 1:	sync
> > 	mtmsr	r7
> > 	isync
> > 	b	1b
> > 
> > Whereas MPC8245's user manual says, that when setting the MSR_POW bit in 
> > the MSR one may not set any other bit in it with the same instruction. 
> > Does this mean that the above does not actually work on those (and 
> > similar) CPUs or does it work because of the loop?
> 
> That doc bit looks a bit strange. The kernel pretty much relies on
> setting MSR:EE and MSR:POW atomicaly.

So, would be good to verify? As MSR is implementation-specific and, for 
example, in 603e manual it doesn't have this limitation, it should be 
verified on one of platforms where the manual explicitely mentions that. 
If you post a test-patch to check it I could test it. Otherwise I could 
try to cook something up myself.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: Chenging 2 bits in MSR in ppc6xx_idle() with 1 command?

[Guennadi Liakhovetski]

On Wed, 27 Dec 2006, Benjamin Herrenschmidt wrote:

> On Mon, 2006-12-25 at 21:07 +0100, Guennadi Liakhovetski wrote:
> > Hi
> > 
> > Here's a code fragment from ppc6xx_idle(), which should send the CPU into 
> > a powersaving mode (DOZE or NAP) and re-enable interrupts after a 
> > local_irq_disable():
> > 
> > 	mfmsr	r7
> > 	ori	r7,r7,MSR_EE
> > 	oris	r7,r7,MSR_POW@h
> > 1:	sync
> > 	mtmsr	r7
> > 	isync
> > 	b	1b
> > 
> > Whereas MPC8245's user manual says, that when setting the MSR_POW bit in 
> > the MSR one may not set any other bit in it with the same instruction. 
> > Does this mean that the above does not actually work on those (and 
> > similar) CPUs or does it work because of the loop?
> 
> That doc bit looks a bit strange. The kernel pretty much relies on
> setting MSR:EE and MSR:POW atomicaly.

Hm, wouldn't it just work? In ppc6xx_idle() the _TLF_NAPPING bit is set. 
If as a result of mtmsr only the EE bit is set and we get an interrupt, we 
end up in transfer_to_handler(), check the flag, it is set, so we branch 
to power_save_6xx_restore(). There we clear NAP/DOZE and just jump to 
transfer_to_handler_cont(). Why did you say we'd miss the check for 
need_resched (on IRC)? How is this case difference from if we really did 
go to NAP / DOZE?

Are there other places in the kernel where we rely on setting MSR:POW and 
some other bit atomically?

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: Chenging 2 bits in MSR in ppc6xx_idle() with 1 command?

[Benjamin Herrenschmidt]

> Hm, wouldn't it just work? In ppc6xx_idle() the _TLF_NAPPING bit is set. 
> If as a result of mtmsr only the EE bit is set and we get an interrupt, we 
> end up in transfer_to_handler(), check the flag, it is set, so we branch 
> to power_save_6xx_restore(). There we clear NAP/DOZE and just jump to 
> transfer_to_handler_cont(). Why did you say we'd miss the check for 
> need_resched (on IRC)? How is this case difference from if we really did 
> go to NAP / DOZE?
> 
> Are there other places in the kernel where we rely on setting MSR:POW and 
> some other bit atomically?

Indeed, the napping "recovery" code might save us here... funny, as it
wasn't implemented to handle that case at all...

Ben.

Re: Chenging 2 bits in MSR in ppc6xx_idle() with 1 command?

[Segher Boessenkool]

>>> Whereas MPC8245's user manual says, that when setting the MSR_POW 
>>> bit in
>>> the MSR one may not set any other bit in it with the same 
>>> instruction.
>>> Does this mean that the above does not actually work on those (and
>>> similar) CPUs or does it work because of the loop?
>>
>> That doc bit looks a bit strange. The kernel pretty much relies on
>> setting MSR:EE and MSR:POW atomicaly.
>
> Hm, wouldn't it just work? In ppc6xx_idle() the _TLF_NAPPING bit is 
> set.
> If as a result of mtmsr only the EE bit is set and we get an interrupt,

But what if only the POW flag gets set, and EE doesn't?
That's one CPU that won't wake up anymore :-/

It sounds like changing the sequence to first set EE, sync,
isync, and only then set POW as well will work safely on all
hardware?


Segher