Re: [PATCH] powerpc: powernv: winkle: Restore LPCR with LPCR_PECE1 cleared

[Michael Ellerman <mpe@ellerman.id.au>]

On Wed, 2015-01-14 at 16:43 +0530, Shreyas B. Prabhu wrote:
> LPCR_PECE1 bit controls whether decrementer interrupts are allowed to
> cause exit from power-saving mode. While waking up from winkle, restoring
> LPCR with LPCR_PECE1 set (i.e Decrementer interrupts allowed) can cause
> issue in the following scenario:
> 
> - All the threads in a core are offlined. The core enters deep winkle.
> - Spurious interrupt wakes up a thread in the core. Here LPCR is restored
>   with LPCR_PECE1 bit set.
> - Since it was a spurious interrupt on a offline thread, the thread clears
>   the interrupt and goes back to winkle.
> - Here before the thread executes winkle and puts the core into deep winkle,
>   if a decrementer interrupt occurs on any of the sibling threads in the core
>   that thread wakes up. 
> - Since in offline loop we are flushing interrupt only in case of external
>   interrupt, the decrementer interrupt does not get flushed. So at this stage
>   the thread is stuck in this is loop of waking up at 0x100 due to decrementer
>   interrupt, not flushing the interrupt as only external interrupts get flushed,
>   entering winkle, waking up at 0x100 again.
> 
> Fix this by programming PORE to restore LPCR with LPCR_PECE1 bit 
> cleared when waking up from winkle.

That sounds good.

But this makes the third place where we clear LPCR_PECE1, in addition to:

    static int fastsleep_loop(struct cpuidle_device *dev,
    				struct cpuidle_driver *drv,
    				int index)
    {
    	...
    
    	new_lpcr = old_lpcr;
    	/* Do not exit powersave upon decrementer as we've setup the timer
    	 * offload.
    	 */
    	new_lpcr &= ~LPCR_PECE1;
    
    	mtspr(SPRN_LPCR, new_lpcr);

And:

    static void pnv_smp_cpu_kill_self(void)
    {
    	...
    
    	/* We don't want to take decrementer interrupts while we are offline,
    	 * so clear LPCR:PECE1. We keep PECE2 enabled.
    	 */
    	mtspr(SPRN_LPCR, mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1);


So perhaps we can capture that logic in one place somehow?

cheers