On Sun, 2010-01-31 at 00:29 +0100, Rafael J. Wysocki wrote: > Hi, > > While Maxim is testing if the patch below helps with > http://bugzilla.kernel.org/show_bug.cgi?id=14668 > I think it's necessary anyway. > > The problem is that currently there's nothing to prevent us from suspending in > the middle of an EC transaction in progress, at least as far as I can see. > As a result, we can suspend with the ACPI global lock held or something like > this, which leads to problems especially for hibernation (if the resume kernel > passes control to the image kernel in the middle of an EC transaction, things > aren't nice). For this reason I think we should wait until there are no EC > transactions in progress before we suspend and we should prevent any new > EC transactions from starting after that point. The patch below does that. > > However, it does that in the EC's suspend callback, which may be too early, > because there still is _PTS to run, so it might be necessary to do that later. > On the other hand, the mechanics behind the ACPI global lock, which is > acquired in acpi_ec_transaction(), requires that interrupts work, because > otherwise there may be a problem if the global lock is not actually acquired > after ACPI_ACQUIRE_GLOBAL_LOCK(), so the last place in which to > wait for EC transactions to complete seems to be the platform suspend > .prepare() callback. Unfortunately, it's not implemented at the moment for > ACPI and it doesn't have a hibernate counterpart and that's why I'd rather use > the patch below, unless it's known to break things for someone. So, if you > can, please test it and tell me if you have any problems with it. > > Of course, comments are welcome as well. > > Thanks, > Rafael > > --- > drivers/acpi/ec.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > Index: linux-2.6/drivers/acpi/ec.c > =================================================================== > --- linux-2.6.orig/drivers/acpi/ec.c > +++ linux-2.6/drivers/acpi/ec.c > @@ -76,8 +76,9 @@ enum ec_command { > enum { > EC_FLAGS_QUERY_PENDING, /* Query is pending */ > EC_FLAGS_GPE_STORM, /* GPE storm detected */ > - EC_FLAGS_HANDLERS_INSTALLED /* Handlers for GPE and > + EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and > * OpReg are installed */ > + EC_FLAGS_SUSPENDED, /* Driver is suspended */ > }; > > /* If we find an EC via the ECDT, we need to keep a ptr to its context */ > @@ -291,6 +292,10 @@ static int acpi_ec_transaction(struct ac > if (t->rdata) > memset(t->rdata, 0, t->rlen); > mutex_lock(&ec->lock); > + if (test_bit(EC_FLAGS_SUSPENDED, &ec->flags)) { > + status = -EBUSY; > + goto unlock; > + } > if (ec->global_lock) { > status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk); > if (ACPI_FAILURE(status)) { > @@ -1059,16 +1064,26 @@ error: > static int acpi_ec_suspend(struct acpi_device *device, pm_message_t state) > { > struct acpi_ec *ec = acpi_driver_data(device); > + > + mutex_lock(&ec->lock); > + /* Prevent transactions from happening while suspended */ > + set_bit(EC_FLAGS_SUSPENDED, &ec->flags); > /* Stop using GPE */ > acpi_disable_gpe(NULL, ec->gpe); > + mutex_unlock(&ec->lock); > return 0; > } > > static int acpi_ec_resume(struct acpi_device *device) > { > struct acpi_ec *ec = acpi_driver_data(device); > + > + mutex_lock(&ec->lock); > /* Enable use of GPE back */ > acpi_enable_gpe(NULL, ec->gpe); > + /* Allow transactions to happen again */ > + set_bit(EC_FLAGS_SUSPENDED, &ec->flags); ^^^^^^^^^^^^ Thats why it doesn't work here.... Will retest now. > + mutex_unlock(&ec->lock); > return 0; > } > > -- > 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 -- 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
