Re: [PATCH 0/1] Failure to hibernate on Dell Latitude 7430

Re: [PATCH 0/1] Failure to hibernate on Dell Latitude 7430

[Linux regression tracking (Thorsten Leemhuis)]

On 18.03.24 20:11, David McFarland wrote:
> I have a Dell Latitude 7430, and recently whenever I hibernate with
> `systemctl hibernate`, the machine would immediately wake.

Thanks for the report and the proposed patch.

> I bisected it to:
> 
> 0c4cae1bc00d PM: hibernate: Avoid missing wakeup events during hibernation

I CCed the author of that change, which should at least be aware of this
any maybe help out.

Also CCed the people that take care of the code your patch modifies,
they should be the best to judge what should be done here.

FWIW, the start of the thread and the proposed patch can be found here:
https://lore.kernel.org/all/20240318191153.6978-1-corngood@gmail.com/

Ciao, Thorsten

> However, the underlying problem seems to be that during hibernation, my
> system gets a 0xcf (power button release) event, and the above change
> causes it to abort hibernation correctly.
> 
> I also noticed that holding the power button down (when it's configured
> to suspend) causes the system to suspend and then wake upon release, if
> it's held long enough.
> 
> I'm attaching a patch which fixes the problem for me, by skipping the
> wake on any of the release events.  These events are all marked
> KEY_IGNORE, so think this is a reasonable thing to do.
> 
> I'm a little worried about the consequences of doing this
> unconditionally in intel-hid.  Perhaps it should be a quirk?
> 
> David McFarland (1):
>   platform/x86/intel/hid: Don't wake on 5-button releases
> 
>  drivers/platform/x86/intel/hid.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

P.S.: To be sure the issue doesn't fall through the cracks unnoticed,
I'm adding it to regzbot, the Linux kernel regression tracking bot:

#regzbot ^introduced 0c4cae1bc00d
#regzbot title PM: hibernate: & platform/x86/intel/hid: hibernate on
Dell Latitude 7430 fails
#regzbot ignore-activity

--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [PATCH 1/1] platform/x86/intel/hid: Don't wake on 5-button releases

[Linux regression tracking (Thorsten Leemhuis)]

[CCing Chris, who authored the culprit; also CCing the platform folks
and a few lists]

On 18.03.24 20:11, David McFarland wrote:
> If, for example, the power button is configured to suspend, holding it
> and releasing it after the machine has suspended, will wake the machine.

David, from here is looks like this is stalled for ten days now. Or was
there some progress and I just missed it?

> Also on some machines, power button release events are sent during
> hibernation, even if the button wasn't used to hibernate the machine.
> This causes hibernation to be aborted.

From the cover letter[1] is sounds a lot like a "Fixes: 0c4cae1bc00d31
("PM: hibernate: Avoid missing wakeup events during hibernation")" would
be appropriate here.

Regarding the patch itself: hopefully this mail will get things moving.

[1]
https://lore.kernel.org/linux-pm/20240318191153.6978-1-corngood@gmail.com/

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

> Signed-off-by: David McFarland <corngood@gmail.com>
>> ---
>  drivers/platform/x86/intel/hid.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/hid.c b/drivers/platform/x86/intel/hid.c
> index 7457ca2b27a6..707de9895965 100644
> --- a/drivers/platform/x86/intel/hid.c
> +++ b/drivers/platform/x86/intel/hid.c
> @@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  	struct platform_device *device = context;
>  	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
>  	unsigned long long ev_index;
> +	struct key_entry *ke;
>  	int err;
>  
>  	/*
> @@ -545,11 +546,16 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  		if (event == 0xc0 || !priv->array)
>  			return;
>  
> -		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
> +		ke = sparse_keymap_entry_from_scancode(priv->array, event);
> +
> +		if (!ke) {
>  			dev_info(&device->dev, "unknown event 0x%x\n", event);
>  			return;
>  		}
>  
> +		if (ke->type == KE_IGNORE)
> +			return;
> +
>  wakeup:
>  		pm_wakeup_hard_event(&device->dev);
>  

Re: [PATCH 1/1] platform/x86/intel/hid: Don't wake on 5-button releases

[David McFarland]

"Linux regression tracking (Thorsten Leemhuis)"
<regressions@leemhuis.info> writes:

> David, from here is looks like this is stalled for ten days now. Or was
> there some progress and I just missed it?

No, I've not seen any emails since your last.

> From the cover letter[1] is sounds a lot like a "Fixes: 0c4cae1bc00d31
> ("PM: hibernate: Avoid missing wakeup events during hibernation")" would
> be appropriate here.

The specific behaviour I encountered (failure to hibernate) started with
that commit, but I think it just exposed the underlying behaviour (wake
on button release), which probably dates to when the driver was
introduced.

I believe it would have been possible to reproduce the other behaviour I
mentioned (long hold of button to suspend causes the machine to wake on
release), even before 0c4cae1bc00d31.

That's why I left it off, but I'm happy to revise.

Re: [PATCH 1/1] platform/x86/intel/hid: Don't wake on 5-button releases

[Linux regression tracking (Thorsten Leemhuis)]

On 29.03.24 19:06, David McFarland wrote:
> "Linux regression tracking (Thorsten Leemhuis)"
> <regressions@leemhuis.info> writes:
> 
>> David, from here is looks like this is stalled for ten days now. Or was
>> there some progress and I just missed it?
> No, I've not seen any emails since your last.

Thx for confirming.

>> From the cover letter[1] is sounds a lot like a "Fixes: 0c4cae1bc00d31
>> ("PM: hibernate: Avoid missing wakeup events during hibernation")" would
>> be appropriate here.
> 
> The specific behaviour I encountered (failure to hibernate) started with
> that commit, but I think it just exposed the underlying behaviour (wake
> on button release), which probably dates to when the driver was
> introduced.

Well, it depends on the maintainer in question (so you might better want
to ignore this advice!), but I'd say: mention that in the patch
description and add Fixes: tag, to ensure people pick it up when the
change that exposed the problem is backported.

This is hinted at in submitting-patches: "This tag also assists the
stable kernel team in determining which stable kernel versions should
receive your fix.". Maybe that text should mention scenario.

Ciao, Thorsten

Re: [PATCH 1/1] platform/x86/intel/hid: Don't wake on 5-button releases

[Enrik Berkhan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Fri, 2024-03-29 at 14:51 +0100, Linux regression tracking (Thorsten
Leemhuis) wrote:
> [CCing Chris, who authored the culprit; also CCing the platform folks
> and a few lists]

(Intentionally replying to the "wrong" e-mail to get the enlarged
recipient list for free ...)

> 

> Regarding the patch itself: hopefully this mail will get things
> moving.
> 
> [1]
> https://lore.kernel.org/linux-pm/20240318191153.6978-1-corngood@gmail.com/
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker'
> hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
> #regzbot poke
> 
> > Signed-off-by: David McFarland <corngood@gmail.com>

Tested-by: Enrik Berkhan <Enrik.Berkhan@inka.de>

> > > ---
> >  drivers/platform/x86/intel/hid.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/intel/hid.c
> > b/drivers/platform/x86/intel/hid.c
> > index 7457ca2b27a6..707de9895965 100644
> > --- a/drivers/platform/x86/intel/hid.c
> > +++ b/drivers/platform/x86/intel/hid.c
> > @@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle,
> > u32 event, void *context)
> >  	struct platform_device *device = context;
> >  	struct intel_hid_priv *priv = dev_get_drvdata(&device-
> > >dev);
> >  	unsigned long long ev_index;
> > +	struct key_entry *ke;
> >  	int err;
> >  
> >  	/*
> > @@ -545,11 +546,16 @@ static void notify_handler(acpi_handle
> > handle, u32 event, void *context)
> >  		if (event == 0xc0 || !priv->array)
> >  			return;
> >  
> > -		if (!sparse_keymap_entry_from_scancode(priv-
> > >array, event)) {
> > +		ke = sparse_keymap_entry_from_scancode(priv-
> > >array, event);
> > +
> > +		if (!ke) {
> >  			dev_info(&device->dev, "unknown event
> > 0x%x\n", event);
> >  			return;
> >  		}
> >  
> > +		if (ke->type == KE_IGNORE)
> > +			return;
> > +
> >  wakeup:
> >  		pm_wakeup_hard_event(&device->dev);
> >  

-----BEGIN PGP SIGNATURE-----

iQGzBAEBCAAdFiEEbLyQBMI0qAWiF6l31AjqSkbLPKQFAmYK1IQACgkQ1AjqSkbL
PKSUXwv/YWiU334Osus+niffw9gN94vCQid6OWuTIbX2zMNIunHmpHouMtfo6dpb
7Qsyaz0sxaUEclRPLf/bSAV8zWromM/4Xoa/DoBq+GPOLle5wTJAlislhZVfDZFX
FZ0EfUg+MvThqlJD8yhGNJWGVnSn92hDid7yy5Y/BSWTPCNhRxEsqy+d0pN82EnD
0mqWnpfG28uqK6dtOLnvbiOuWQhgKYerKFYHXw8FLxZV0eKb3+RqBFf6ZdsAjUEC
+d7G9Oq4pc9jxZNvOMop+z3Eskd/bD8Wi2aDtR1qcRENMaK0ucevTuOjGL8Gh80c
SbgKu2eU/qVlbnVrcaPgpB0CXXza7SYUynw5XWB8jBWOj6w2nxpnQPm5a0Z5aNYw
/kLuabhSommEmUIJtoOA2vgO8D0ACfF2n95cNXaJRiOhuag3SRoair50TOzmf09Z
4uco5iaWeIQwEpVFGdy+qh0V18kYclnNMZHC9bhO6heBDLB9Wy/iXDxko9tMXm/v
XnfAKKw9
=udJV
-----END PGP SIGNATURE-----

Re: [PATCH 1/1] platform/x86/intel/hid: Don't wake on 5-button releases

[Hans de Goede]

Hi David,

Thank you for your patch.

I became aware of this patch because the discussion on the regressions
list. Next time for patches to files under drivers/platform/x86/
at a minimum please Cc: platform-driver-x86@vger.kernel.org and
preferably also send the patch directly to me and Ilpo as shown
by get_maintainer.pl:

[hans@shalem linux]$ scripts/get_maintainer.pl -f drivers/platform/x86/intel/vbtn.c
AceLan Kao <acelan.kao@canonical.com> (maintainer:INTEL VIRTUAL BUTTON DRIVER)
Hans de Goede <hdegoede@redhat.com> (maintainer:X86 PLATFORM DRIVERS)
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> (maintainer:X86 PLATFORM DRIVERS)
platform-driver-x86@vger.kernel.org (open list:INTEL VIRTUAL BUTTON DRIVER)
linux-kernel@vger.kernel.org (open list)

Please make sure you are sending this to the right people for v2.

On 3/18/24 8:11 PM, David McFarland wrote:
> If, for example, the power button is configured to suspend, holding it
> and releasing it after the machine has suspended, will wake the machine.
> 
> Also on some machines, power button release events are sent during
> hibernation, even if the button wasn't used to hibernate the machine.
> This causes hibernation to be aborted.
> 

As discussed by Thorsten this needs a fixes tag, to help with backporting
it to relevant stable kernels. Also for v2 please don't forget to add
Enrik's Tested-by from elsewhere in thread:

Tested-by: Enrik Berkhan <Enrik.Berkhan@inka.de>



> Signed-off-by: David McFarland <corngood@gmail.com>
> ---
>  drivers/platform/x86/intel/hid.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/hid.c b/drivers/platform/x86/intel/hid.c
> index 7457ca2b27a6..707de9895965 100644
> --- a/drivers/platform/x86/intel/hid.c
> +++ b/drivers/platform/x86/intel/hid.c
> @@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  	struct platform_device *device = context;
>  	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
>  	unsigned long long ev_index;
> +	struct key_entry *ke;
>  	int err;
>  
>  	/*
> @@ -545,11 +546,16 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  		if (event == 0xc0 || !priv->array)
>  			return;
>  
> -		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
> +		ke = sparse_keymap_entry_from_scancode(priv->array, event);
> +

I would prefer for there to be no empty line between the "ke =" assignment
and the "if (!ke)".

Otherwise the patch looks good to me, so for v3
you can add:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

After fixing this + adding the Fixes and Tested-by tags.

Regards,

Hans




> +		if (!ke) {
>  			dev_info(&device->dev, "unknown event 0x%x\n", event);
>  			return;
>  		}
>  
> +		if (ke->type == KE_IGNORE)
> +			return;
> +
>  wakeup:
>  		pm_wakeup_hard_event(&device->dev);
>  

[PATCH v2] platform/x86/intel/hid: Don't wake on 5-button releases

[David McFarland]

If, for example, the power button is configured to suspend, holding it
and releasing it after the machine has suspended, will wake the machine.

Also on some machines, power button release events are sent during
hibernation, even if the button wasn't used to hibernate the machine.
This causes hibernation to be aborted.

Fixes: 0c4cae1bc00d ("PM: hibernate: Avoid missing wakeup events during hibernation")
Signed-off-by: David McFarland <corngood@gmail.com>
Tested-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v2: Added tags and fixed whitespace, as requested by Hans.

 drivers/platform/x86/intel/hid.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/hid.c b/drivers/platform/x86/intel/hid.c
index 7457ca2b27a6..9ffbdc988fe5 100644
--- a/drivers/platform/x86/intel/hid.c
+++ b/drivers/platform/x86/intel/hid.c
@@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	struct platform_device *device = context;
 	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
 	unsigned long long ev_index;
+	struct key_entry *ke;
 	int err;
 
 	/*
@@ -545,11 +546,15 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		if (event == 0xc0 || !priv->array)
 			return;
 
-		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
+		ke = sparse_keymap_entry_from_scancode(priv->array, event);
+		if (!ke) {
 			dev_info(&device->dev, "unknown event 0x%x\n", event);
 			return;
 		}
 
+		if (ke->type == KE_IGNORE)
+			return;
+
 wakeup:
 		pm_wakeup_hard_event(&device->dev);
 
-- 
2.42.0

Re: [PATCH v2] platform/x86/intel/hid: Don't wake on 5-button releases

[Hans de Goede]

Hi,

On 4/4/24 1:41 PM, David McFarland wrote:
> If, for example, the power button is configured to suspend, holding it
> and releasing it after the machine has suspended, will wake the machine.
> 
> Also on some machines, power button release events are sent during
> hibernation, even if the button wasn't used to hibernate the machine.
> This causes hibernation to be aborted.
> 
> Fixes: 0c4cae1bc00d ("PM: hibernate: Avoid missing wakeup events during hibernation")
> Signed-off-by: David McFarland <corngood@gmail.com>
> Tested-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thank you, this version looks good to me.

Ilpo, can you pick this up as a bug-fix for the 6.9 cycle please?

Regards,

Hans



> ---
> v2: Added tags and fixed whitespace, as requested by Hans.
> 
>  drivers/platform/x86/intel/hid.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/hid.c b/drivers/platform/x86/intel/hid.c
> index 7457ca2b27a6..9ffbdc988fe5 100644
> --- a/drivers/platform/x86/intel/hid.c
> +++ b/drivers/platform/x86/intel/hid.c
> @@ -504,6 +504,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  	struct platform_device *device = context;
>  	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
>  	unsigned long long ev_index;
> +	struct key_entry *ke;
>  	int err;
>  
>  	/*
> @@ -545,11 +546,15 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  		if (event == 0xc0 || !priv->array)
>  			return;
>  
> -		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
> +		ke = sparse_keymap_entry_from_scancode(priv->array, event);
> +		if (!ke) {
>  			dev_info(&device->dev, "unknown event 0x%x\n", event);
>  			return;
>  		}
>  
> +		if (ke->type == KE_IGNORE)
> +			return;
> +
>  wakeup:
>  		pm_wakeup_hard_event(&device->dev);
>  

Re: [PATCH v2] platform/x86/intel/hid: Don't wake on 5-button releases

[Ilpo Järvinen]

On Thu, 04 Apr 2024 08:41:45 -0300, David McFarland wrote:

> If, for example, the power button is configured to suspend, holding it
> and releasing it after the machine has suspended, will wake the machine.
> 
> Also on some machines, power button release events are sent during
> hibernation, even if the button wasn't used to hibernate the machine.
> This causes hibernation to be aborted.
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86/intel/hid: Don't wake on 5-button releases
      commit: 5864e479ca4344f3a5df8074524da24c960f440b

--
 i.