Re: ACPI-CA update patch for review

[YONETANI Tomokazu <qhwt+dfly@xxxxxxxxxx>]

On Thu, Nov 30, 2006 at 02:57:31PM +0100, Simon 'corecode' Schubert wrote:
> YONETANI Tomokazu wrote:
> >>:- hand-rolled locking code in AcpiOs{Acquire,Release}Lock() functions:
> >>:  ACPI-CA code has been rewritten in 20060623 to make these functions
                                          ^^^^^^^^
                                          actually this was 20060608

> >>:  to be used as spinlock functions, and called from many places where
> >>:  they weren't before.  Since our implementation of these locking 
> >>functions
> >>:  used lockmgr lock, which cannot be called from cpu_idle(), this led to
> >>:  a panic(mainly when my laptop wakes up from sleep state).  After 
> >>struggling
> >>:  with other locking primitives, I ended up with critical section and
> >>:  special-cased the idlethread.  I believed this shouldn't make the 
> >>situation
> >>:  worse, as ACPI functions called from cpu_idle_hook code in acpi_cpu did
> >>:  not using locking before.  But I'm open to a better implementation,
> >>:  especially how to deal with locking when called from idlethread.
> >>    You could probably use tokens here, but a critical section ought to
> >>    work as well since ACPI functions are only called from cpu #0.
> >Does that apply to ioctl or sysctl support code, too?  Can you shed
> >some light on me to find how it's guaranteed?
> 
> First, why don't you use spinlocks then (note: I didn't read the code)?

Because it led to a panic in lwkt_switch() when the laptop woke up
from sleep state:
  lwkt_switch: still holding 1 exclusive spinlocks

But that was months ago when I tried to replace lockmgr() calls
in AcpiOs{Acquire,Release}Lock() functions with spinlocks, so I need
to revisit that idea before drawing a conclusion.
 
> To just use a critical section, you'd have to migrate the calling thread to 
> cpu0, or have a acpi thread running on cpu0, and send a synchronous message 
> to this thread, processing your request.
> 
> How are the guarantees the ACPI code gives?  Will it ever call a blocking 
> function (kmalloc M_WAITOK, etc) while holding the lock?  Does it expect 
> the lock to persist?  If yes, that's ugly and you'd have to use lockmgr 
> locks and an own thread.

I couldn't find an explicit requirement for the locking functions
except for the description in changes.txt(look for "20060608:")
  http://developer.intel.com/technology/iapc/acpi/downloads/changes.txt
As far as I read the code, the locks won't persist in most places,
except for the line 1753 in acpi_SetSleepState() (in unpatched code).
 
> Why are the ACPI functions called from the idle thread, anyways?

acpi_cpu subsystem installs acpi_cpu_idle() as cpu_idle_hook() function.
acpi_cpu_idle() calls some of low-level ACPI functions named AcpiHw*().
These functions are needed to be called to transition to C2 or deeper state.
And either AcpiHwRegister{Read,Write}() or their caller calls
AcpiOsAcquireLock() to lock AcpiGbl_HardwareLock.  That was why lockmgr
lock can't be used any longer.  I'm not sure if acpi_cpu_idle() can be
moved to its own thread, but maybe worth a try, as it can solve these
nasty things.

Thanks for suggestions.