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 :-/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
