On Mon, May 2, 2016 at 5:50 PM, 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 :-/

It doesn't look like trying to make it safe for both the driver and
AML to use the same range of addresses is a realistic goal.  At least
the benefit will not outweigh the resulting complexity if the opregion
is actually never used by AML in practice.

So, why don't we have a flag that will make i801_access() return an
error on every invocation when set (the callers of it should be able
to cope with that or they are broken IMO) and why don't we set it when
AML accesses the opregion first time?

Yes, that will prevent the driver from actually working *when* AML
turns out to use the opregion, but IMO that's a bit more friendly than
preventing the driver from loading at all.