Re: [FIX PATCH] ACPI/AC: Add sleep quirk for Thinkpad e530

[Lan Tianyu <tianyu.lan@intel.com>]

On 2013年05月08日 07:54, Rafael J. Wysocki wrote:
> On Monday, May 06, 2013 08:27:33 PM Lan Tianyu wrote:
>> Thinkpad e530 bios notify ac device first and then sleep
>> a specific time before doing actual operations in the
>> EC event handler(_Qxx). This will cause the AC state
>> reported by ACPI event.
>>
>> Method (_Q27, 0, NotSerialized)
>> {
>>       Notify (AC, 0x80)
>>       Sleep (0x03E8)
>>       Store (Zero, PWRS)
>>       PNOT ()
>> }
>>
>> This patch is to add a 1s sleep in the ac driver's
>> notify handler before acpi_ac_get_state() to make
>> sure get right state.
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=45221
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  drivers/acpi/ac.c |   32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
>> index 6d5bf64..0292cbb 100644
>> --- a/drivers/acpi/ac.c
>> +++ b/drivers/acpi/ac.c
>> @@ -28,6 +28,8 @@
>>  #include <linux/slab.h>
>>  #include <linux/init.h>
>>  #include <linux/types.h>
>> +#include <linux/dmi.h>
>> +#include <linux/delay.h>
>>  #ifdef CONFIG_ACPI_PROCFS_POWER
>>  #include <linux/proc_fs.h>
>>  #include <linux/seq_file.h>
>> @@ -74,6 +76,8 @@ static int acpi_ac_resume(struct device *dev);
>>  #endif
>>  static SIMPLE_DEV_PM_OPS(acpi_ac_pm, NULL, acpi_ac_resume);
>>  
>> +static int ac_flag_sleep_for_get_state;
> 
> Hmm.  Why don't you replace ac_flag_sleep_for_get_state with the time
> to sleep in acpi_ac_notify(), something like ac_sleep_before_get_state_ms
> and then do something like this ->
> 
>> +
>>  static struct acpi_driver acpi_ac_driver = {
>>  	.name = "ac",
>>  	.class = ACPI_AC_CLASS,
>> @@ -252,6 +256,15 @@ static void acpi_ac_notify(struct acpi_device *device, u32 event)
>>  	case ACPI_AC_NOTIFY_STATUS:
>>  	case ACPI_NOTIFY_BUS_CHECK:
>>  	case ACPI_NOTIFY_DEVICE_CHECK:
>> +		/* Some buggy bios notify ac device first and then sleep
>> +		 * a specific time before doing actual operations in the
>> +		 * EC event handler(_Qxx). This will cause the AC state
>> +		 * reported by ACPI event wrong. So add a 1s sleep here
>> +		 * to ensure get correct state.
>> +		 */
>> +		if (ac_flag_sleep_for_get_state)
>> +			msleep(1000);
> 
> -> if (ac_sleep_before_get_state_ms > 0)
> 			msleep(ac_sleep_before_get_state_ms);
> 
> and *then* set ac_sleep_before_get_state_ms to 1000 in the Thinkpad e530
> specific quirk?
> 
> If we find another machine having this problem in the future, but needing
> a different sleep time, it will be easier to add it to the blacklist in that
> case.
Yes, that will be more flexible. Thanks for your advice. I will update soon.
> 
> Thanks,
> Rafael
> 
> 
>> +
>>  		acpi_ac_get_state(ac);
>>  		acpi_bus_generate_proc_event(device, event, (u32) ac->state);
>>  		acpi_bus_generate_netlink_event(device->pnp.device_class,
>> @@ -264,6 +277,24 @@ static void acpi_ac_notify(struct acpi_device *device, u32 event)
>>  	return;
>>  }
>>  
>> +static int ac_sleep_for_get_state(const struct dmi_system_id *d)
>> +{
>> +	ac_flag_sleep_for_get_state = 1;
>> +	return 0;
>> +}
>> +
>> +static struct dmi_system_id __initdata ac_dmi_table[] = {
>> +	{
>> +	.callback = ac_sleep_for_get_state,
>> +	.ident = "thinkpad e530",
>> +	.matches = {
>> +		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> +		DMI_MATCH(DMI_PRODUCT_NAME, "32597CG"),
>> +		},
>> +	},
>> +	{},
>> +};
>> +
>>  static int acpi_ac_add(struct acpi_device *device)
>>  {
>>  	int result = 0;
>> @@ -312,6 +343,7 @@ static int acpi_ac_add(struct acpi_device *device)
>>  		kfree(ac);
>>  	}
>>  
>> +	dmi_check_system(ac_dmi_table);
>>  	return result;
>>  }
>>  
>>


-- 
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html