Re: [PATCH] i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR

[Andy Lutomirski <luto@amacapital.net>]

On Mon, May 2, 2016 at 8:50 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, May 02, 2016 at 08:29:42AM -0700, Andy Lutomirski wrote:
>> On Mon, May 2, 2016 at 3:12 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Fri, Apr 29, 2016 at 06:13:52PM -0700, Andy Lutomirski wrote:
>> >> 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.
>
>> > Of course this all breaks if the ASL code expects the state to survive
>> > between transactions.
>> >
>> >> 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