[rtc-linux] [PATCH v3 1/5] rtc: OMAP: Add system pm_power_off to rtc driver

[AnilKumar, Chimata]

On Wed, Nov 28, 2012 at 16:42:26, Russell King - ARM Linux wrote:
> On Tue, Nov 27, 2012 at 03:42:39PM -0800, Andrew Morton wrote:
> > > +	/* Do not allow to execute any other task */
> > > +	spin_lock_irqsave(&lock, flags);
> > > +	while (1);
> > 
> > I suspect this doesn't do what you want it to do.
> > 
> > Firstly, please provide adequate code comments here so that code
> > readers do not also need to be mind readers.
> > 
> > If you want to stop this CPU dead in its tracks (why?) then
> > 
> > 	local_irq_disable();
> > 	while (1)
> > 		;		/* Note correct code layout */
> > 
> > will do it.  But it means that the NMI watchdog (if present) will come
> > along and whack the machine in the head a few seconds later.  And this
> > does nothing to stop other CPUs.
> > 
> > But not being a mind reader, I'm really at a loss to suggest what
> > should be done here.  

Here intention is to disable all interrupts between rtc power_off request
till system actually went to power off mode, max 2 seconds based on when
the AM335x RTC alarm2 expires. The idea was that since the system is
going to shutdown, it is better to not process any more interrupts.

> It's hooking into the pm_power_off hook, which is called from kernel/sys.c
> via arch code.  We will have already stopped all other CPUs at this point.
> 
> Why there's that while (1) there I don't know; when pm_power_off is not
> hooked, we don't do anything like that - and what will happen in that
> case is we'll return all the way back to sys_reboot(), which will call
> do_exit(0) on us.

When testing with v3.7-rc7, I saw the following BUG() triggered when I did
not use a while(1). Logs below.

Before calling do_exit(0), there is a reboot_mutex lock that is taken in
reboot syscall. do_exit() function is , checking for any locks held by the
processor and if yes then it is printing a BUG_ON from print_held_locks_bug() function along with the locks held information.
Even though the BUG triggers, the system is going to power off because
The hardware has been triggered.

==== (log)
[root at arago /]# poweroff
The system is going down NOW!
Sent SIGTERM to all processes
Sent SIGKILL to all processes
Requesting system poweroff
[   32.125900] Disabling non-boot CPUs ...
[   32.130155] Power down.
[   32.132741] System will go to power_off state in approx. 2 secs
[   32.139994]
[   32.141575] =====================================
[   32.146564] [ BUG: lock held at task exit time! ]
[   32.151522] 3.7.0-rc7-00015-g3f8bbe0 #32 Not tainted
[   32.156721] -------------------------------------
[   32.161676] init/622 is exiting with locks still held!
[   32.167085] 1 lock held by init/622:
[   32.170831]  #0:  (reboot_mutex){+.+...}, at: [<c0056fb8>] sys_reboot+0xf4/0x1e8
[   32.178679]
[   32.178679] stack backtrace:
[   32.183305] [<c001af24>] (unwind_backtrace+0x0/0xf0) from [<c0046978>] (do_exit+0x488/0x7e8)
[   32.192183] [<c0046978>] (do_exit+0x488/0x7e8) from [<c0056fc4>] (sys_reboot+0x100/0x1e8)
[   32.200797] [<c0056fc4>] (sys_reboot+0x100/0x1e8) from [<c0013320>] (ret_fast_syscall+0x0/0x3c)
=====

It is not clear to me where reboot_mutex should have been unlocked before
debug_check_no_locks_held() is called in do_exit()?

Note: In latest linux-next tip I am *not* seeing this error. I have not
bisected yet to see what fixed this. Since with latest linux-next I am
not seeing an issue, I will remove spinlock and the while(1). Hope that
will make the patch more acceptable.

> I don't see a problem with that, and I don't see why we need to spin
> (without any power saving too) waiting for some event.  If we've called
> sys_reboot with LINUX_REBOOT_CMD_POWER_OFF, we'd better have already
> killed most of userspace off by that time anyway.
> 

I did a search of various power-off implementations in *arch/arm*.

Those that use a while(1):
=========================
arch/arm/mach-at91/board-gsia18s.c
arch/arm/mach-at91/setup.c doesn't
arch/arm/mach-cns3xxx/cns3420vb.c
arch/arm/mach-iop32x/glantank.c
arch/arm/mach-iop32x/iq31244.c
arch/arm/mach-iop32x/n2100.c local_irq_disable() and then a while(1)
arch/arm/mach-kirkwood/board-lsxl.c

arch/arm/mach-highbank/highbank.c does while(1) cpu_do_idle()

Those that don't have a while(1):
================================
arch/arm/mach-imx/mach-mx31moboard.c
arch/arm/mach-iop32x/em7210.c

Doesn't have a while(1) but setting a gpio so presumably kills the system immediately:
===========
arch/arm/mach-ixp4xx/dsmg600-setup.c  
arch/arm/mach-ixp4xx/nas100d-setup.c
arch/arm/mach-ixp4xx/nslu2-setup.c
arch/arm/mach-kirkwood/board-dnskw.c
arch/arm/mach-kirkwood/board-ib62x0.c
arch/arm/mach-kirkwood/board-dnskw.c
arch/arm/mach-kirkwood/board-ib62x0.c
arch/arm/mach-kirkwood/d2net_v2-setup.c
arch/arm/mach-kirkwood/netspace_v2-setup.c
arch/arm/mach-kirkwood/netxbig_v2-setup.c
arch/arm/mach-kirkwood/t5325-setup.c
arch/arm/mach-orion5x/dns323-setup.c

arch/arm/mach-vexpress/v2m.c take a spinlock, not sure when it will shut down

arch/arm/mach-kirkwood/board-ts219.c sends a character to PIC and potentially returns...
arch/arm/mach-kirkwood/ts219-setup.c

So it seems there is a plethora of variations of how this is implemented
(probably driven also by hardware differences). Since what I mentioned
above works for me (in linux-next), I will go ahead and submit at v4.
Let me know if there are objections.

Thanks
AnilKumar