[PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle

[PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

On Dell Latitude 7275 the 5-button array is not exposed in the
ACPI tables, but still notifies are sent to the Intel HID device
object (device ID INT33D5) in response to power button actions while
suspended to idle.  However, they are currently ignored as the
intel-hid driver is not prepared to take care of them.

As a result, power button wakeup from suspend-to-idle doesn't work
on this platform, but suspend-to-idle is the only reliable suspend
variant on it (the S3 implementation in the platform firmware turns
out to be broken), so it would be good to handle it properly.

For this reason, add an upfront check against the power button press
event (0xCE) to notify_handler() in the wakeup mode which allows it
to catch the power button wakeup notification on the affected
platform (even though priv->array is NULL on it) and should not
change the behavior on platforms with priv->array present (because
priv->array contains the event in question in those cases).

Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/platform/x86/intel-hid.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/platform/x86/intel-hid.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/intel-hid.c
+++ linux-pm/drivers/platform/x86/intel-hid.c
@@ -203,15 +203,26 @@ static void notify_handler(acpi_handle h
 	acpi_status status;
 
 	if (priv->wakeup_mode) {
+		/*
+		 * Needed for wakeup from suspend-to-idle to work on some
+		 * platforms that don't expose the 5-button array, but still
+		 * send notifies with the power button event code to this
+		 * device object on power button actions while suspended.
+		 */
+		if (event == 0xce)
+			goto wakeup;
+
 		/* Wake up on 5-button array events only. */
 		if (event == 0xc0 || !priv->array)
 			return;
 
-		if (sparse_keymap_entry_from_scancode(priv->array, event))
-			pm_wakeup_hard_event(&device->dev);
-		else
+		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
 			dev_info(&device->dev, "unknown event 0x%x\n", event);
+			return;
+		}
 
+wakeup:
+		pm_wakeup_hard_event(&device->dev);
 		return;
 	}
 

Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle

[Rafael J. Wysocki]

On Friday, July 28, 2017 02:06:36 AM Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> On Dell Latitude 7275 the 5-button array is not exposed in the
> ACPI tables, but still notifies are sent to the Intel HID device
> object (device ID INT33D5) in response to power button actions while
> suspended to idle.  However, they are currently ignored as the
> intel-hid driver is not prepared to take care of them.
> 
> As a result, power button wakeup from suspend-to-idle doesn't work
> on this platform, but suspend-to-idle is the only reliable suspend
> variant on it (the S3 implementation in the platform firmware turns
> out to be broken), so it would be good to handle it properly.
> 
> For this reason, add an upfront check against the power button press
> event (0xCE) to notify_handler() in the wakeup mode which allows it
> to catch the power button wakeup notification on the affected
> platform (even though priv->array is NULL on it) and should not
> change the behavior on platforms with priv->array present (because
> priv->array contains the event in question in those cases).
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
> Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Please note that this change is requisite for

https://patchwork.kernel.org/patch/9873159/

so are there any objections or concerns?

> ---
>  drivers/platform/x86/intel-hid.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/platform/x86/intel-hid.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/intel-hid.c
> +++ linux-pm/drivers/platform/x86/intel-hid.c
> @@ -203,15 +203,26 @@ static void notify_handler(acpi_handle h
>  	acpi_status status;
>  
>  	if (priv->wakeup_mode) {
> +		/*
> +		 * Needed for wakeup from suspend-to-idle to work on some
> +		 * platforms that don't expose the 5-button array, but still
> +		 * send notifies with the power button event code to this
> +		 * device object on power button actions while suspended.
> +		 */
> +		if (event == 0xce)
> +			goto wakeup;
> +
>  		/* Wake up on 5-button array events only. */
>  		if (event == 0xc0 || !priv->array)
>  			return;
>  
> -		if (sparse_keymap_entry_from_scancode(priv->array, event))
> -			pm_wakeup_hard_event(&device->dev);
> -		else
> +		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
>  			dev_info(&device->dev, "unknown event 0x%x\n", event);
> +			return;
> +		}
>  
> +wakeup:
> +		pm_wakeup_hard_event(&device->dev);
>  		return;
>  	}
>  
> 

Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle

[Andy Shevchenko]

On Tue, Aug 1, 2017 at 12:46 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, July 28, 2017 02:06:36 AM Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> On Dell Latitude 7275 the 5-button array is not exposed in the
>> ACPI tables, but still notifies are sent to the Intel HID device
>> object (device ID INT33D5) in response to power button actions while
>> suspended to idle.  However, they are currently ignored as the
>> intel-hid driver is not prepared to take care of them.
>>
>> As a result, power button wakeup from suspend-to-idle doesn't work
>> on this platform, but suspend-to-idle is the only reliable suspend
>> variant on it (the S3 implementation in the platform firmware turns
>> out to be broken), so it would be good to handle it properly.
>>
>> For this reason, add an upfront check against the power button press
>> event (0xCE) to notify_handler() in the wakeup mode which allows it
>> to catch the power button wakeup notification on the affected
>> platform (even though priv->array is NULL on it) and should not
>> change the behavior on platforms with priv->array present (because
>> priv->array contains the event in question in those cases).
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
>> Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Please note that this change is requisite for
>
> https://patchwork.kernel.org/patch/9873159/
>
> so are there any objections or concerns?

Not from my side,

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
>> ---
>>  drivers/platform/x86/intel-hid.c |   17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> Index: linux-pm/drivers/platform/x86/intel-hid.c
>> ===================================================================
>> --- linux-pm.orig/drivers/platform/x86/intel-hid.c
>> +++ linux-pm/drivers/platform/x86/intel-hid.c
>> @@ -203,15 +203,26 @@ static void notify_handler(acpi_handle h
>>       acpi_status status;
>>
>>       if (priv->wakeup_mode) {
>> +             /*
>> +              * Needed for wakeup from suspend-to-idle to work on some
>> +              * platforms that don't expose the 5-button array, but still
>> +              * send notifies with the power button event code to this
>> +              * device object on power button actions while suspended.
>> +              */
>> +             if (event == 0xce)
>> +                     goto wakeup;
>> +
>>               /* Wake up on 5-button array events only. */
>>               if (event == 0xc0 || !priv->array)
>>                       return;
>>
>> -             if (sparse_keymap_entry_from_scancode(priv->array, event))
>> -                     pm_wakeup_hard_event(&device->dev);
>> -             else
>> +             if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
>>                       dev_info(&device->dev, "unknown event 0x%x\n", event);
>> +                     return;
>> +             }
>>
>> +wakeup:
>> +             pm_wakeup_hard_event(&device->dev);
>>               return;
>>       }
>>
>>
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle

[Rafael J. Wysocki]

On Tue, Aug 1, 2017 at 1:21 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 1, 2017 at 12:46 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Friday, July 28, 2017 02:06:36 AM Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> On Dell Latitude 7275 the 5-button array is not exposed in the
>>> ACPI tables, but still notifies are sent to the Intel HID device
>>> object (device ID INT33D5) in response to power button actions while
>>> suspended to idle.  However, they are currently ignored as the
>>> intel-hid driver is not prepared to take care of them.
>>>
>>> As a result, power button wakeup from suspend-to-idle doesn't work
>>> on this platform, but suspend-to-idle is the only reliable suspend
>>> variant on it (the S3 implementation in the platform firmware turns
>>> out to be broken), so it would be good to handle it properly.
>>>
>>> For this reason, add an upfront check against the power button press
>>> event (0xCE) to notify_handler() in the wakeup mode which allows it
>>> to catch the power button wakeup notification on the affected
>>> platform (even though priv->array is NULL on it) and should not
>>> change the behavior on platforms with priv->array present (because
>>> priv->array contains the event in question in those cases).
>>>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196115
>>> Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Please note that this change is requisite for
>>
>> https://patchwork.kernel.org/patch/9873159/
>>
>> so are there any objections or concerns?
>
> Not from my side,
>
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

OK, thanks!

I'm going to route it through the PM tree then if that's not a problem.

Thanks,
Rafael

Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle

[Andy Shevchenko]

On Tue, Aug 1, 2017 at 2:56 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Aug 1, 2017 at 1:21 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Aug 1, 2017 at 12:46 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Friday, July 28, 2017 02:06:36 AM Rafael J. Wysocki wrote:

>>> Please note that this change is requisite for
>>>
>>> https://patchwork.kernel.org/patch/9873159/
>>>
>>> so are there any objections or concerns?
>>
>> Not from my side,
>>
>> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> OK, thanks!
>
> I'm going to route it through the PM tree then if that's not a problem.

Mario, are you okay with this change?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Tuesday, August 1, 2017 7:37 AM
> To: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; Platform Driver <platform-driver-
> x86@vger.kernel.org>; Darren Hart <dvhart@infradead.org>; LKML <linux-
> kernel@vger.kernel.org>; Linux ACPI <linux-acpi@vger.kernel.org>; Andy
> Shevchenko <andriy.shevchenko@linux.intel.com>; Jérôme de Bretagne
> <jerome.debretagne@gmail.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Alex Hung <alex.hung@canonical.com>
> Subject: Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from
> suspend-to-idle
> 
> On Tue, Aug 1, 2017 at 2:56 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Aug 1, 2017 at 1:21 AM, Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Tue, Aug 1, 2017 at 12:46 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>> On Friday, July 28, 2017 02:06:36 AM Rafael J. Wysocki wrote:
> 
> >>> Please note that this change is requisite for
> >>>
> >>> https://patchwork.kernel.org/patch/9873159/
> >>>
> >>> so are there any objections or concerns?
> >>
> >> Not from my side,
> >>
> >> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> > OK, thanks!
> >
> > I'm going to route it through the PM tree then if that's not a problem.
> 
> Mario, are you okay with this change?
> 

Thanks for checking.  I spent a little time this morning trying to walk through
the ASL as attached to the Bugzilla entry and I think this is the correct approach.

Acked-By: Mario Limonciello <mario.limonciello@dell.com>

Jérôme,
I have one question though.  These events should be happening as a pair.
Press: 0xCE, Release: 0xCF.
What happens with the event on power button release?
Is that showing a message in the log during wakeup from S2I?

Something like "unknown event 0xCF"?
If so, it would be good to also catch and ignore that too.

Thanks,

RE: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Tuesday, August 1, 2017 7:37 AM
> To: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; Platform Driver <platform-driver-
> x86@vger.kernel.org>; Darren Hart <dvhart@infradead.org>; LKML <linux-
> kernel@vger.kernel.org>; Linux ACPI <linux-acpi@vger.kernel.org>; Andy
> Shevchenko <andriy.shevchenko@linux.intel.com>; Jérôme de Bretagne
> <jerome.debretagne@gmail.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Alex Hung <alex.hung@canonical.com>
> Subject: Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from
> suspend-to-idle
> 
> On Tue, Aug 1, 2017 at 2:56 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Aug 1, 2017 at 1:21 AM, Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Tue, Aug 1, 2017 at 12:46 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>> On Friday, July 28, 2017 02:06:36 AM Rafael J. Wysocki wrote:
> 
> >>> Please note that this change is requisite for
> >>>
> >>> https://patchwork.kernel.org/patch/9873159/
> >>>
> >>> so are there any objections or concerns?
> >>
> >> Not from my side,
> >>
> >> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> > OK, thanks!
> >
> > I'm going to route it through the PM tree then if that's not a problem.
> 
> Mario, are you okay with this change?
> 

Thanks for checking.  I spent a little time this morning trying to walk through
the ASL as attached to the Bugzilla entry and I think this is the correct approach.

Acked-By: Mario Limonciello <mario.limonciello@dell.com>

Jérôme,
I have one question though.  These events should be happening as a pair.
Press: 0xCE, Release: 0xCF.
What happens with the event on power button release?
Is that showing a message in the log during wakeup from S2I?

Something like "unknown event 0xCF"?
If so, it would be good to also catch and ignore that too.

Thanks,

Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle

[Jérôme de Bretagne]

2017-08-01 19:04 GMT+02:00  <Mario.Limonciello@dell.com>:
>> -----Original Message-----
>> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
>> Sent: Tuesday, August 1, 2017 7:37 AM
>> To: Rafael J. Wysocki <rafael@kernel.org>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; Platform Driver <platform-driver-
>> x86@vger.kernel.org>; Darren Hart <dvhart@infradead.org>; LKML <linux-
>> kernel@vger.kernel.org>; Linux ACPI <linux-acpi@vger.kernel.org>; Andy
>> Shevchenko <andriy.shevchenko@linux.intel.com>; Jérôme de Bretagne
>> <jerome.debretagne@gmail.com>; Limonciello, Mario
>> <Mario_Limonciello@Dell.com>; Alex Hung <alex.hung@canonical.com>
>> Subject: Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from
>> suspend-to-idle
>>
>> On Tue, Aug 1, 2017 at 2:56 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Tue, Aug 1, 2017 at 1:21 AM, Andy Shevchenko
>> > <andy.shevchenko@gmail.com> wrote:
>> >> On Tue, Aug 1, 2017 at 12:46 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >>> On Friday, July 28, 2017 02:06:36 AM Rafael J. Wysocki wrote:
>>
>> >>> Please note that this change is requisite for
>> >>>
>> >>> https://patchwork.kernel.org/patch/9873159/
>> >>>
>> >>> so are there any objections or concerns?
>> >>
>> >> Not from my side,
>> >>
>> >> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> >
>> > OK, thanks!
>> >
>> > I'm going to route it through the PM tree then if that's not a problem.
>>
>> Mario, are you okay with this change?
>>
>
> Thanks for checking.  I spent a little time this morning trying to walk through
> the ASL as attached to the Bugzilla entry and I think this is the correct approach.
>
> Acked-By: Mario Limonciello <mario.limonciello@dell.com>
>
> Jérôme,
> I have one question though.  These events should be happening as a pair.
> Press: 0xCE, Release: 0xCF.
> What happens with the event on power button release?
> Is that showing a message in the log during wakeup from S2I?
>
> Something like "unknown event 0xCF"?

Mario, I confirm that I can see such release events in the logs:

   intel-hid INT33D5:00: unknown event 0xcf

during wakeup from suspend-to-idle.

> If so, it would be good to also catch and ignore that too.
>
> Thanks,