| Message ID | 20240318191153.6978-2-corngood@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Failure to hibernate on Dell Latitude 7430 | expand |
[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); >
"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.
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
-----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-----
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); >
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);
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. Signed-off-by: David McFarland <corngood@gmail.com> --- drivers/platform/x86/intel/hid.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)