Re: [PATCH v2] ACPI / EC: Clear stale EC events on Samsung systems

[Rafael J. Wysocki]

On Saturday, March 01, 2014 12:42:28 AM Kieran Clancy wrote:
> A number of Samsung notebooks (530Uxx/535Uxx/540Uxx/550Pxx/900Xxx/etc)
> continue to log events during sleep (lid open/close, AC plug/unplug,
> battery level change), which accumulate in the EC until a buffer fills.
> After the buffer is full (tests suggest it holds 8 events), GPEs stop
> being triggered for new events. This state persists on wake or even on
> power cycle, and prevents new events from being registered until the EC
> is manually polled.
> 
> This is the root cause of a number of bugs, including AC not being
> detected properly, lid close not triggering suspend, and low ambient
> light not triggering the keyboard backlight. The bug also seemed to be
> responsible for performance issues on at least one user's machine.
> 
> Juan Manuel Cabo found the cause of bug and the workaround of polling
> the EC manually on wake.
> 
> The loop which clears the stale events is based on an earlier patch by
> Lan Tianyu (see referenced attachment).
> 
> This patch:
>  - Adds a function acpi_ec_clear() which polls the EC for stale _Q
>    events at most ACPI_EC_CLEAR_MAX (currently 100) times. A warning is
>    logged if this limit is reached.
>  - Adds a flag EC_FLAGS_CLEAR_ON_RESUME which is set to 1 if the DMI
>    system vendor is Samsung. This check could be replaced by several
>    more specific DMI vendor/product pairs, but it's likely that the bug
>    affects more Samsung products than just the five series mentioned
>    above. Further, it should not be harmful to run acpi_ec_clear() on
>    systems without the bug; it will return immediately after finding no
>    data waiting.
>  - Runs acpi_ec_clear() on initialisation (boot), from acpi_ec_add()
>  - Runs acpi_ec_clear() on wake, from acpi_ec_unblock_transactions()
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=44161
> References: https://bugzilla.kernel.org/show_bug.cgi?id=45461
> References: https://bugzilla.kernel.org/show_bug.cgi?id=57271
> References: https://bugzilla.kernel.org/attachment.cgi?id=126801
> Signed-off-by: Kieran Clancy <clancy.kieran@xxxxxxxxx>
> Suggested-by: Juan Manuel Cabo <juanmanuel.cabo@xxxxxxxxx>
> Reviewed-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> Reviewed-by: Dennis Jansen <dennis.jansen@xxxxxx>
> Tested-by: Kieran Clancy <clancy.kieran@xxxxxxxxx>
> Tested-by: Juan Manuel Cabo <juanmanuel.cabo@xxxxxxxxx>
> Tested-by: Dennis Jansen <dennis.jansen@xxxxxx>
> Tested-by: Maurizio D'Addona <mauritiusdadd@xxxxxxxxx>
> Tested-by: San Zamoyski <san@xxxxxxxxxx>

Queued up for 3.15, thanks!

> ---
> 
> Changes in PATCH v2:
>  - Changed some of the 'Signed-off-by' lines to better reflect contributions,
>    as suggested by Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>.
>  - Directly reference prior work by Lan Tianyu.
>  - Increase ACPI_EC_CLEAR_MAX to 100, after discussion with
>    Li Guang <lig.fnst@xxxxxxxxxxxxxx> and Juan Manuel Cabo.
>  - Made source comments more explicit, thanks to suggestions by Li Guang.
>  - Marked warning for hitting ACPI_EC_CLEAR_MAX as unlikely(), as suggested by
>    Dennis Jansen.
> 
>  drivers/acpi/ec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 959d41a..d7d32c2 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -67,6 +67,8 @@ enum ec_command {
>  #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
>  #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
>  #define ACPI_EC_MSI_UDELAY	550	/* Wait 550us for MSI EC */
> +#define ACPI_EC_CLEAR_MAX	100	/* Maximum number of events to query
> +					 * when trying to clear the EC */
>  
>  enum {
>  	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
> @@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
>  static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
>  static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
>  static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */
> +static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */
>  
>  /* --------------------------------------------------------------------------
>                               Transaction Management
> @@ -440,6 +443,29 @@ 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.
> + * Run with locked ec mutex.
> + */
> +static void acpi_ec_clear(struct acpi_ec *ec)
> +{
> +	int i, status;
> +	u8 value = 0;
> +
> +	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
> +		status = acpi_ec_query_unlocked(ec, &value);
> +		if (status || !value)
> +			break;
> +	}
> +
> +	if (unlikely(i == ACPI_EC_CLEAR_MAX))
> +		pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
> +	else
> +		pr_info("%d stale EC events cleared\n", i);
> +}
> +
>  void acpi_ec_block_transactions(void)
>  {
>  	struct acpi_ec *ec = first_ec;
> @@ -463,6 +489,10 @@ void acpi_ec_unblock_transactions(void)
>  	mutex_lock(&ec->mutex);
>  	/* Allow transactions to be carried out again */
>  	clear_bit(EC_FLAGS_BLOCKED, &ec->flags);
> +
> +	if (EC_FLAGS_CLEAR_ON_RESUME)
> +		acpi_ec_clear(ec);
> +
>  	mutex_unlock(&ec->mutex);
>  }
>  
> @@ -821,6 +851,13 @@ static int acpi_ec_add(struct acpi_device *device)
>  
>  	/* EC is fully operational, allow queries */
>  	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
> +
> +	/* Clear stale _Q events if hardware might require that */
> +	if (EC_FLAGS_CLEAR_ON_RESUME) {
> +		mutex_lock(&ec->mutex);
> +		acpi_ec_clear(ec);
> +		mutex_unlock(&ec->mutex);
> +	}
>  	return ret;
>  }
>  
> @@ -922,6 +959,30 @@ static int ec_enlarge_storm_threshold(const struct dmi_system_id *id)
>  	return 0;
>  }
>  
> +/*
> + * On some hardware it is necessary to clear events accumulated by the EC during
> + * sleep. These ECs stop reporting GPEs until they are manually polled, if too
> + * many events are accumulated. (e.g. Samsung Series 5/9 notebooks)
> + *
> + * https://bugzilla.kernel.org/show_bug.cgi?id=44161
> + *
> + * Ideally, the EC should also be instructed NOT to accumulate events during
> + * sleep (which Windows seems to do somehow), but the interface to control this
> + * behaviour is not known at this time.
> + *
> + * Models known to be affected are Samsung 530Uxx/535Uxx/540Uxx/550Pxx/900Xxx,
> + * however it is very likely that other Samsung models are affected.
> + *
> + * On systems which don't accumulate _Q events during sleep, this extra check
> + * should be harmless.
> + */
> +static int ec_clear_on_resume(const struct dmi_system_id *id)
> +{
> +	pr_debug("Detected system needing EC poll on resume.\n");
> +	EC_FLAGS_CLEAR_ON_RESUME = 1;
> +	return 0;
> +}
> +
>  static struct dmi_system_id ec_dmi_table[] __initdata = {
>  	{
>  	ec_skip_dsdt_scan, "Compal JFL92", {
> @@ -965,6 +1026,9 @@ static struct dmi_system_id ec_dmi_table[] __initdata = {
>  	ec_validate_ecdt, "ASUS hardware", {
>  	DMI_MATCH(DMI_SYS_VENDOR, "ASUSTek Computer Inc."),
>  	DMI_MATCH(DMI_PRODUCT_NAME, "L4R"),}, NULL},
> +	{
> +	ec_clear_on_resume, "Samsung hardware", {
> +	DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG ELECTRONICS CO., LTD.")}, NULL},
>  	{},
>  };
>  
> 

-- 
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/