Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700418 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-08-01 00:00 +0200
    Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-08-01 01:30 +0200
      Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle "Rafael J. Wysocki" <rafael@kernel.org> - 2017-08-01 14:00 +0200
        Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-08-01 14:40 +0200
          RE: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from  suspend-to-idle <Mario.Limonciello@dell.com> - 2017-08-01 19:10 +0200
            Re: [PATCH] platform/x86: intel-hid: Wake up Dell Latitude 7275 from suspend-to-idle Jérôme de Bretagne          <jerome.debretagne@gmail.com> - 2017-08-02 11:10 +0200

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

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;
>  	}
>  
> 

[toc] | [next] | [standalone]

#1700499

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

[toc] | [prev] | [next] | [standalone]

#1700937

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

[toc] | [prev] | [next] | [standalone]

#1700984

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

[toc] | [prev] | [next] | [standalone]

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

> -----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,

[toc] | [prev] | [next] | [standalone]

#1701934

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,

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web