On Mon, May 2, 2016 at 5:50 PM, Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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 :-/ 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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
