On Wednesday, April 30, 2014 12:21:20 AM Kieran Clancy wrote:
> Address a regression caused by commit ad332c8a4533:
> (ACPI / EC: Clear stale EC events on Samsung systems)
>
> After the earlier patch, there was found to be a race condition on some
> earlier Samsung systems (N150/N210/N220). The function acpi_ec_clear was
> sometimes discarding a new EC event before its GPE was triggered by the
> system. In the case of these systems, this meant that the "lid open"
> event was not registered on resume if that was the cause of the wake,
> leading to problems when attempting to close the lid to suspend again.
>
> After testing on a number of Samsung systems, both those affected by the
> previous EC bug and those affected by the race condition, it seemed that
> the best course of action was to process rather than discard the events.
> On Samsung systems which accumulate stale EC events, there does not seem
> to be any adverse side-effects of running the associated _Q methods.
>
> This patch adds an argument to the static function acpi_ec_sync_query so
> that it may be used within the acpi_ec_clear loop in place of
> acpi_ec_query_unlocked which was used previously.
>
> With thanks to Stefan Biereigel for reporting the issue, and for all the
> people who helped test the new patch on affected systems.
>
> References: https://lkml.kernel.org/r/532fe3b2.9060...@biereigel-wb.de
> References: https://bugzilla.kernel.org/show_bug.cgi?id=44161#c173
> Reported-by: Stefan Biereigel <ste...@biereigel.de>
> Signed-off-by: Kieran Clancy <clancy.kie...@gmail.com>
> Tested-by: Stefan Biereigel <ste...@biereigel.de>
> Tested-by: Dennis Jansen <dennis.jan...@web.de>
> Tested-by: Nicolas Porcel <nicolasporce...@gmail.com>
> Tested-by: Maurizio D'Addona <mauritiusd...@gmail.com>
> Tested-by: Juan Manuel Cabo <juanmanuel.c...@gmail.com>
> Tested-by: Giannis Koutsou <giannis.kout...@gmail.com>
> Tested-by: Kieran Clancy <clancy.kie...@gmail.com>
> Cc: Lan Tianyu <tianyu....@intel.com>
Queued up for 3.15, thanks!
> ---
>
> To maintainers: Assuming this patch is accepted, please mark this for
> inclusion in all -stable trees. It should be noted that the previous
> patch (ad332c8a4533) was excluded from a number of stable trees after
> the regression was found, but should now be included again along with
> this patch. I am not sure of the correct way to annotate this above.
I only can tag it for 3.14.y, because the mainline regression is in 3.14.
You'll need to ask the -stable maintainers in question to pick up both
patches after this one reaches the Linus' tree.
> drivers/acpi/ec.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index d7d32c2..ad11ba4 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -206,13 +206,13 @@ unlock:
> spin_unlock_irqrestore(&ec->lock, flags);
> }
>
> -static int acpi_ec_sync_query(struct acpi_ec *ec);
> +static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
>
> static int ec_check_sci_sync(struct acpi_ec *ec, u8 state)
> {
> if (state & ACPI_EC_FLAG_SCI) {
> if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
> - return acpi_ec_sync_query(ec);
> + return acpi_ec_sync_query(ec, NULL);
> }
> return 0;
> }
> @@ -443,10 +443,8 @@ acpi_handle ec_get_handle(void)
>
> EXPORT_SYMBOL(ec_get_handle);
>
> -static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
> -
> /*
> - * Clears stale _Q events that might have accumulated in the EC.
> + * Process _Q events that might have accumulated in the EC.
> * Run with locked ec mutex.
> */
> static void acpi_ec_clear(struct acpi_ec *ec)
> @@ -455,7 +453,7 @@ static void acpi_ec_clear(struct acpi_ec *ec)
> u8 value = 0;
>
> for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
> - status = acpi_ec_query_unlocked(ec, &value);
> + status = acpi_ec_sync_query(ec, &value);
> if (status || !value)
> break;
> }
> @@ -582,13 +580,18 @@ static void acpi_ec_run(void *cxt)
> kfree(handler);
> }
>
> -static int acpi_ec_sync_query(struct acpi_ec *ec)
> +static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
> {
> u8 value = 0;
> int status;
> struct acpi_ec_query_handler *handler, *copy;
> - if ((status = acpi_ec_query_unlocked(ec, &value)))
> +
> + status = acpi_ec_query_unlocked(ec, &value);
> + if (data)
> + *data = value;
> + if (status)
> return status;
> +
> list_for_each_entry(handler, &ec->list, node) {
> if (value == handler->query_bit) {
> /* have custom handler for this bit */
> @@ -612,7 +615,7 @@ static void acpi_ec_gpe_query(void *ec_cxt)
> if (!ec)
> return;
> mutex_lock(&ec->mutex);
> - acpi_ec_sync_query(ec);
> + acpi_ec_sync_query(ec, NULL);
> mutex_unlock(&ec->mutex);
> }
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
