On Mon, May 2, 2016 at 8:50 AM, Mika Westerberg
[off-list ref] wrote:

On Mon, May 02, 2016 at 08:29:42AM -0700, Andy Lutomirski wrote:

quoted

On Mon, May 2, 2016 at 3:12 AM, Mika Westerberg
[off-list ref] wrote:

quoted

On Fri, Apr 29, 2016 at 06:13:52PM -0700, Andy Lutomirski wrote:

quoted

A question, though: there's nothing that keeps i801_access from being
called in between consecutive ACPI accesses.  Could this confuse the
ASL code?  Would it be helpful if i801_access were to save away the
old register state and restore it when it's done in the event that an
opregion access has been seen so that the ASL-configured state doesn't
get stomped on?

Looking at those ASL methods of Lenovo Yoga 900 for example they seem to
initialize the hardware, do the transaction and cleanup in one go.
That's also what the i2c-i801.c driver is doing as far as I can say. So
in that sense they should not mess with each other.

Is your locking actually sufficient to get that right?  You're taking
acpi_lock, which is private to the driver, so you're only holding it
during actual opregion access AFAICT.  That means that, if one thread
is in the ACPI interpreter in one of these blocks and another thread
is in the driver, they could still interleave their accesses.  Am I
missing something?

No, you are right.

quoted

quoted

Of course this all breaks if the ASL code expects the state to survive
between transactions.

quoted

Also, what happens if i801_access happens while the i801 master is
busy with an ASL-initiated transaction?  Will it wait for the
transaction to finish?

Yes, it should since ->acpi_lock is taken by i801_acpi_io_handler().

But i801_acpi_io_handler has no concept of a transaction AFAICT.

Indeed it only handles access one-by-one not by transaction. So if the
ASL code is in middle of a transaction and i2c-i801.c starts one as well
it will mess the registers.

Back to drawing board :-/

:-/

I can try to test using the ACPI debugger maybe, but my laptop with
this problem doesn't have any calls to the offending ASL at all, so
just booting it won't exercise any of the interesting cases at all.

-- 
Andy Lutomirski
AMA Capital Management, LLC