| Message ID | 20240805141608.170844-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | platform/x86: ideapad-laptop: Stop calling i8042_command() | expand |
On Mon, Aug 05, 2024 at 04:16:08PM +0200, Hans de Goede wrote: > Commit 07a4a4fc83dd ("ideapad: add Lenovo IdeaPad Z570 support (part 2)") > added an i8042_command(..., I8042_CMD_AUX_[EN|DIS]ABLE) call to > the ideapad-laptop driver to suppress the touchpad events at the PS/2 > AUX controller level. > > Commit c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux > port on/off on select models") limited this to only do this by default > on the IdeaPad Z570 to replace a growing list of models on which > the i8042_command() call was disabled by quirks because it was causing > issues. > > A recent report shows that this is causing issues even on the Z570 for > which it was originally added because it can happen on resume before > the i8042 controller's own resume() method has run: > > [ 50.241235] ideapad_acpi VPC2004:00: PM: calling acpi_subsys_resume+0x0/0x5d @ 4492, parent: PNP0C09:00 > [ 50.242055] snd_hda_intel 0000:00:0e.0: PM: pci_pm_resume+0x0/0xed returned 0 after 13511 usecs > [ 50.242120] snd_hda_codec_realtek hdaudioC0D0: PM: calling hda_codec_pm_resume+0x0/0x19 [snd_hda_codec] @ 4518, parent: 0000:00:0e.0 > [ 50.247406] i8042: [49434] a8 -> i8042 (command) > [ 50.247468] ideapad_acpi VPC2004:00: PM: acpi_subsys_resume+0x0/0x5d returned 0 after 6220 usecs > ... > [ 50.247883] i8042 kbd 00:01: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 > [ 50.247894] i8042 kbd 00:01: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs > [ 50.247906] i8042 aux 00:02: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 > [ 50.247916] i8042 aux 00:02: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs > ... > [ 50.248301] i8042 i8042: PM: calling platform_pm_resume+0x0/0x41 @ 4492, parent: platform > [ 50.248377] i8042: [49434] 55 <- i8042 (flush, kbd) > [ 50.248407] i8042: [49435] aa -> i8042 (command) > [ 50.248601] i8042: [49435] 00 <- i8042 (return) > [ 50.248604] i8042: [49435] i8042 controller selftest: 0x0 != 0x55 > > Dmitry (input subsys maintainer) pointed out that just sending > KEY_TOUCHPAD_OFF/KEY_TOUCHPAD_ON which the ideapad-laptop driver > already does should be sufficient and that it then is up to userspace > to filter out touchpad events after having received a KEY_TOUCHPAD_OFF. > > Given all the problems the i8042_command() call has been causing just > removing it indeed seems best, so this removes it completely. Note that > this only impacts the Ideapad Z570 since it was already disabled by > default on all other models. Right, I think stopping using it here is the best. I also had a draft of a patch to allow establishing a link between i8042 driver and users of i8042_command() so that they do not disturb resuming of the keyboard controller. I need to finish it. > > Fixes: c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux port on/off on select models") > Reported-by: Jonathan Denose <jdenose@chromium.org> > Closes: https://lore.kernel.org/linux-input/20231102075243.1.Idb37ff8043a29f607beab6440c32b9ae52525825@changeid/ > Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Maxim Mikityanskiy <maxtram95@gmail.com> > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> FWIW: Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Thanks.
On Mon, 05 Aug 2024 at 16:16:08 +0200, Hans de Goede wrote: > Commit 07a4a4fc83dd ("ideapad: add Lenovo IdeaPad Z570 support (part 2)") > added an i8042_command(..., I8042_CMD_AUX_[EN|DIS]ABLE) call to > the ideapad-laptop driver to suppress the touchpad events at the PS/2 > AUX controller level. > > Commit c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux > port on/off on select models") limited this to only do this by default > on the IdeaPad Z570 to replace a growing list of models on which > the i8042_command() call was disabled by quirks because it was causing > issues. > > A recent report shows that this is causing issues even on the Z570 for > which it was originally added because it can happen on resume before > the i8042 controller's own resume() method has run: > > [ 50.241235] ideapad_acpi VPC2004:00: PM: calling acpi_subsys_resume+0x0/0x5d @ 4492, parent: PNP0C09:00 > [ 50.242055] snd_hda_intel 0000:00:0e.0: PM: pci_pm_resume+0x0/0xed returned 0 after 13511 usecs > [ 50.242120] snd_hda_codec_realtek hdaudioC0D0: PM: calling hda_codec_pm_resume+0x0/0x19 [snd_hda_codec] @ 4518, parent: 0000:00:0e.0 > [ 50.247406] i8042: [49434] a8 -> i8042 (command) > [ 50.247468] ideapad_acpi VPC2004:00: PM: acpi_subsys_resume+0x0/0x5d returned 0 after 6220 usecs > ... > [ 50.247883] i8042 kbd 00:01: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 > [ 50.247894] i8042 kbd 00:01: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs > [ 50.247906] i8042 aux 00:02: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 > [ 50.247916] i8042 aux 00:02: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs > ... > [ 50.248301] i8042 i8042: PM: calling platform_pm_resume+0x0/0x41 @ 4492, parent: platform > [ 50.248377] i8042: [49434] 55 <- i8042 (flush, kbd) > [ 50.248407] i8042: [49435] aa -> i8042 (command) > [ 50.248601] i8042: [49435] 00 <- i8042 (return) > [ 50.248604] i8042: [49435] i8042 controller selftest: 0x0 != 0x55 What exactly is the issue? Is it just a few errors in dmesg, or does 8042 stop responding completely? I've seen something similar when I enabled the touchpad while moving the cursor, but it was just a matter of a few lines in dmesg and a protocol resync, both touchpad and keyboard worked after that. > Dmitry (input subsys maintainer) pointed out that just sending > KEY_TOUCHPAD_OFF/KEY_TOUCHPAD_ON which the ideapad-laptop driver > already does should be sufficient and that it then is up to userspace > to filter out touchpad events after having received a KEY_TOUCHPAD_OFF. I believe it's not the case (at least it wasn't back then). The whole point of my patch in the first place was to make touchpad toggle work properly on Z570. Userspace (GNOME) supports two variants of hardware: 1. Laptops that disable touchpad themselves and send out KEY_TOUCHPAD_ON/OFF to report the status. Upon receiving these keycodes, GNOME just shows the status pop-up and relies on firmware to disable the touchpad. 2. Laptops that just send KEY_TOUCHPAD_TOGGLE whenever the key is pressed. GNOME maintains its own touchpad state and disables it in software (as well as showing the pop-up). That means, userspace is not filtering out events upon receiving KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 is weird. It maintains the touchpad state in firmware to light up the status LED, but the firmware doesn't do the actual touchpad disablement. That is, if we use TOGGLE, the LED will get out of sync. If we use ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. > Given all the problems the i8042_command() call has been causing just > removing it indeed seems best, so this removes it completely. Note that > this only impacts the Ideapad Z570 since it was already disabled by > default on all other models. While I agree that i8042_command() is not a perfect solution, I don't like the idea of breaking the touchpad toggle, even if "only one" laptop model is affected. Can we suppress input events from the touchpad in some other way, purely in software? I.e. don't call i8042_command(), don't disrupt the PS/2 protocol, but instead let ideapad-laptop tell psmouse to stop generating input events for a while? > Fixes: c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux port on/off on select models") > Reported-by: Jonathan Denose <jdenose@chromium.org> > Closes: https://lore.kernel.org/linux-input/20231102075243.1.Idb37ff8043a29f607beab6440c32b9ae52525825@changeid/ > Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Maxim Mikityanskiy <maxtram95@gmail.com> > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/ideapad-laptop.c | 37 --------------------------- > 1 file changed, 37 deletions(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 1ace711f7442..255fb56ec9ee 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -18,7 +18,6 @@ > #include <linux/device.h> > #include <linux/dmi.h> > #include <linux/fb.h> > -#include <linux/i8042.h> > #include <linux/init.h> > #include <linux/input.h> > #include <linux/input/sparse-keymap.h> > @@ -144,7 +143,6 @@ struct ideapad_private { > bool hw_rfkill_switch : 1; > bool kbd_bl : 1; > bool touchpad_ctrl_via_ec : 1; > - bool ctrl_ps2_aux_port : 1; > bool usb_charging : 1; > } features; > struct { > @@ -182,12 +180,6 @@ MODULE_PARM_DESC(set_fn_lock_led, > "Enable driver based updates of the fn-lock LED on fn-lock changes. " > "If you need this please report this to: platform-driver-x86@vger.kernel.org"); > > -static bool ctrl_ps2_aux_port; > -module_param(ctrl_ps2_aux_port, bool, 0444); > -MODULE_PARM_DESC(ctrl_ps2_aux_port, > - "Enable driver based PS/2 aux port en-/dis-abling on touchpad on/off toggle. " > - "If you need this please report this to: platform-driver-x86@vger.kernel.org"); > - > static bool touchpad_ctrl_via_ec; > module_param(touchpad_ctrl_via_ec, bool, 0444); > MODULE_PARM_DESC(touchpad_ctrl_via_ec, > @@ -1560,7 +1552,6 @@ static void ideapad_fn_lock_led_exit(struct ideapad_private *priv) > static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_events) > { > unsigned long value; > - unsigned char param; > int ret; > > /* Without reading from EC touchpad LED doesn't switch state */ > @@ -1568,15 +1559,6 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > if (ret) > return; > > - /* > - * Some IdeaPads don't really turn off touchpad - they only > - * switch the LED state. We (de)activate KBC AUX port to turn > - * touchpad off and on. We send KEY_TOUCHPAD_OFF and > - * KEY_TOUCHPAD_ON to not to get out of sync with LED > - */ > - if (priv->features.ctrl_ps2_aux_port) > - i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > - > /* > * On older models the EC controls the touchpad and toggles it on/off > * itself, in this case we report KEY_TOUCHPAD_ON/_OFF. Some models do > @@ -1699,23 +1681,6 @@ static const struct dmi_system_id hw_rfkill_list[] = { > {} > }; > > -/* > - * On some models the EC toggles the touchpad muted LED on touchpad toggle > - * hotkey presses, but the EC does not actually disable the touchpad itself. > - * On these models the driver needs to explicitly enable/disable the i8042 > - * (PS/2) aux port. > - */ > -static const struct dmi_system_id ctrl_ps2_aux_port_list[] = { > - { > - /* Lenovo Ideapad Z570 */ > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > - DMI_MATCH(DMI_PRODUCT_VERSION, "Ideapad Z570"), > - }, > - }, > - {} > -}; > - > static void ideapad_check_features(struct ideapad_private *priv) > { > acpi_handle handle = priv->adev->handle; > @@ -1725,8 +1690,6 @@ static void ideapad_check_features(struct ideapad_private *priv) > set_fn_lock_led || dmi_check_system(set_fn_lock_led_list); > priv->features.hw_rfkill_switch = > hw_rfkill_switch || dmi_check_system(hw_rfkill_list); > - priv->features.ctrl_ps2_aux_port = > - ctrl_ps2_aux_port || dmi_check_system(ctrl_ps2_aux_port_list); > priv->features.touchpad_ctrl_via_ec = touchpad_ctrl_via_ec; > > if (!read_ec_data(handle, VPCCMD_R_FAN, &val)) > -- > 2.45.2 >
Hi Maxim, On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: > On Mon, 05 Aug 2024 at 16:16:08 +0200, Hans de Goede wrote: >> Commit 07a4a4fc83dd ("ideapad: add Lenovo IdeaPad Z570 support (part 2)") >> added an i8042_command(..., I8042_CMD_AUX_[EN|DIS]ABLE) call to >> the ideapad-laptop driver to suppress the touchpad events at the PS/2 >> AUX controller level. >> >> Commit c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux >> port on/off on select models") limited this to only do this by default >> on the IdeaPad Z570 to replace a growing list of models on which >> the i8042_command() call was disabled by quirks because it was causing >> issues. >> >> A recent report shows that this is causing issues even on the Z570 for >> which it was originally added because it can happen on resume before >> the i8042 controller's own resume() method has run: >> >> [ 50.241235] ideapad_acpi VPC2004:00: PM: calling acpi_subsys_resume+0x0/0x5d @ 4492, parent: PNP0C09:00 >> [ 50.242055] snd_hda_intel 0000:00:0e.0: PM: pci_pm_resume+0x0/0xed returned 0 after 13511 usecs >> [ 50.242120] snd_hda_codec_realtek hdaudioC0D0: PM: calling hda_codec_pm_resume+0x0/0x19 [snd_hda_codec] @ 4518, parent: 0000:00:0e.0 >> [ 50.247406] i8042: [49434] a8 -> i8042 (command) >> [ 50.247468] ideapad_acpi VPC2004:00: PM: acpi_subsys_resume+0x0/0x5d returned 0 after 6220 usecs >> ... >> [ 50.247883] i8042 kbd 00:01: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 >> [ 50.247894] i8042 kbd 00:01: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs >> [ 50.247906] i8042 aux 00:02: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 >> [ 50.247916] i8042 aux 00:02: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs >> ... >> [ 50.248301] i8042 i8042: PM: calling platform_pm_resume+0x0/0x41 @ 4492, parent: platform >> [ 50.248377] i8042: [49434] 55 <- i8042 (flush, kbd) >> [ 50.248407] i8042: [49435] aa -> i8042 (command) >> [ 50.248601] i8042: [49435] 00 <- i8042 (return) >> [ 50.248604] i8042: [49435] i8042 controller selftest: 0x0 != 0x55 > > What exactly is the issue? Is it just a few errors in dmesg, or does > 8042 stop responding completely? When this issue happens at resume the touchpad stops sending events completely because the i8042 driver's resume() method fails and exits early. > > I've seen something similar when I enabled the touchpad while moving the > cursor, but it was just a matter of a few lines in dmesg and a protocol > resync, both touchpad and keyboard worked after that. Right, the problem is that in this case the i8042's resume() method is failing, which I believe causes the Elan ps/2 driver to not get re-attached to the aux port on resume. > >> Dmitry (input subsys maintainer) pointed out that just sending >> KEY_TOUCHPAD_OFF/KEY_TOUCHPAD_ON which the ideapad-laptop driver >> already does should be sufficient and that it then is up to userspace >> to filter out touchpad events after having received a KEY_TOUCHPAD_OFF. > > I believe it's not the case (at least it wasn't back then). The whole > point of my patch in the first place was to make touchpad toggle work > properly on Z570. > > Userspace (GNOME) supports two variants of hardware: > > 1. Laptops that disable touchpad themselves and send out > KEY_TOUCHPAD_ON/OFF to report the status. Upon receiving these keycodes, > GNOME just shows the status pop-up and relies on firmware to disable the > touchpad. > > 2. Laptops that just send KEY_TOUCHPAD_TOGGLE whenever the key is > pressed. GNOME maintains its own touchpad state and disables it in > software (as well as showing the pop-up). You're right I had forgotten about this. There is really no reason why GNOME cannot also suppress events after a TOUCHPAD_OFF event, but atm it indeed does not do this. We could fix this by patching: plugins/media-keys/gsd-media-keys-manager.c of gnome-settings-daemon to also update the TOUCHPAD_ENABLED_KEY setting when receiving KEY_TOUCHPAD_ON/OFF. Something which I think we should do to, but that will not help solve this bug since we cannot rely on users having a fixed g-s-d. So: self-NACK for this patch. (which is a bummer because I really liked being able to just remove this) > That means, userspace is not filtering out events upon receiving > KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send > KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 > is weird. It maintains the touchpad state in firmware to light up the > status LED, but the firmware doesn't do the actual touchpad disablement. > > That is, if we use TOGGLE, the LED will get out of sync. If we use > ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. Ack. So how about this instead: diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index 1ace711f7442..b7fa06f793cb 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ * touchpad off and on. We send KEY_TOUCHPAD_OFF and * KEY_TOUCHPAD_ON to not to get out of sync with LED */ - if (priv->features.ctrl_ps2_aux_port) + if (send_events && priv->features.ctrl_ps2_aux_port) i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); /* Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume correctly reflects the state before suspend/resume in both touchpad on / off states ? Jonathan, as the reporter of the original suspend/resume issue, can you check if a kernel with this patch + ideapad-laptop re-enabled no longer has the suspend/resume issue you were seeing ? Regards, Hans > >> Given all the problems the i8042_command() call has been causing just >> removing it indeed seems best, so this removes it completely. Note that >> this only impacts the Ideapad Z570 since it was already disabled by >> default on all other models. > > While I agree that i8042_command() is not a perfect solution, I don't > like the idea of breaking the touchpad toggle, even if "only one" laptop > model is affected. Can we suppress input events from the touchpad in > some other way, purely in software? I.e. don't call i8042_command(), > don't disrupt the PS/2 protocol, but instead let ideapad-laptop tell > psmouse to stop generating input events for a while? > >> Fixes: c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux port on/off on select models") >> Reported-by: Jonathan Denose <jdenose@chromium.org> >> Closes: https://lore.kernel.org/linux-input/20231102075243.1.Idb37ff8043a29f607beab6440c32b9ae52525825@changeid/ >> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Cc: Maxim Mikityanskiy <maxtram95@gmail.com> >> Cc: stable@vger.kernel.org >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/platform/x86/ideapad-laptop.c | 37 --------------------------- >> 1 file changed, 37 deletions(-) >> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c >> index 1ace711f7442..255fb56ec9ee 100644 >> --- a/drivers/platform/x86/ideapad-laptop.c >> +++ b/drivers/platform/x86/ideapad-laptop.c >> @@ -18,7 +18,6 @@ >> #include <linux/device.h> >> #include <linux/dmi.h> >> #include <linux/fb.h> >> -#include <linux/i8042.h> >> #include <linux/init.h> >> #include <linux/input.h> >> #include <linux/input/sparse-keymap.h> >> @@ -144,7 +143,6 @@ struct ideapad_private { >> bool hw_rfkill_switch : 1; >> bool kbd_bl : 1; >> bool touchpad_ctrl_via_ec : 1; >> - bool ctrl_ps2_aux_port : 1; >> bool usb_charging : 1; >> } features; >> struct { >> @@ -182,12 +180,6 @@ MODULE_PARM_DESC(set_fn_lock_led, >> "Enable driver based updates of the fn-lock LED on fn-lock changes. " >> "If you need this please report this to: platform-driver-x86@vger.kernel.org"); >> >> -static bool ctrl_ps2_aux_port; >> -module_param(ctrl_ps2_aux_port, bool, 0444); >> -MODULE_PARM_DESC(ctrl_ps2_aux_port, >> - "Enable driver based PS/2 aux port en-/dis-abling on touchpad on/off toggle. " >> - "If you need this please report this to: platform-driver-x86@vger.kernel.org"); >> - >> static bool touchpad_ctrl_via_ec; >> module_param(touchpad_ctrl_via_ec, bool, 0444); >> MODULE_PARM_DESC(touchpad_ctrl_via_ec, >> @@ -1560,7 +1552,6 @@ static void ideapad_fn_lock_led_exit(struct ideapad_private *priv) >> static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_events) >> { >> unsigned long value; >> - unsigned char param; >> int ret; >> >> /* Without reading from EC touchpad LED doesn't switch state */ >> @@ -1568,15 +1559,6 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ >> if (ret) >> return; >> >> - /* >> - * Some IdeaPads don't really turn off touchpad - they only >> - * switch the LED state. We (de)activate KBC AUX port to turn >> - * touchpad off and on. We send KEY_TOUCHPAD_OFF and >> - * KEY_TOUCHPAD_ON to not to get out of sync with LED >> - */ >> - if (priv->features.ctrl_ps2_aux_port) >> - i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); >> - >> /* >> * On older models the EC controls the touchpad and toggles it on/off >> * itself, in this case we report KEY_TOUCHPAD_ON/_OFF. Some models do >> @@ -1699,23 +1681,6 @@ static const struct dmi_system_id hw_rfkill_list[] = { >> {} >> }; >> >> -/* >> - * On some models the EC toggles the touchpad muted LED on touchpad toggle >> - * hotkey presses, but the EC does not actually disable the touchpad itself. >> - * On these models the driver needs to explicitly enable/disable the i8042 >> - * (PS/2) aux port. >> - */ >> -static const struct dmi_system_id ctrl_ps2_aux_port_list[] = { >> - { >> - /* Lenovo Ideapad Z570 */ >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> - DMI_MATCH(DMI_PRODUCT_VERSION, "Ideapad Z570"), >> - }, >> - }, >> - {} >> -}; >> - >> static void ideapad_check_features(struct ideapad_private *priv) >> { >> acpi_handle handle = priv->adev->handle; >> @@ -1725,8 +1690,6 @@ static void ideapad_check_features(struct ideapad_private *priv) >> set_fn_lock_led || dmi_check_system(set_fn_lock_led_list); >> priv->features.hw_rfkill_switch = >> hw_rfkill_switch || dmi_check_system(hw_rfkill_list); >> - priv->features.ctrl_ps2_aux_port = >> - ctrl_ps2_aux_port || dmi_check_system(ctrl_ps2_aux_port_list); >> priv->features.touchpad_ctrl_via_ec = touchpad_ctrl_via_ec; >> >> if (!read_ec_data(handle, VPCCMD_R_FAN, &val)) >> -- >> 2.45.2 >> >
On Mon, Aug 05, 2024 at 05:45:19PM +0200, Hans de Goede wrote: > Hi Maxim, > > On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: > > On Mon, 05 Aug 2024 at 16:16:08 +0200, Hans de Goede wrote: > >> Commit 07a4a4fc83dd ("ideapad: add Lenovo IdeaPad Z570 support (part 2)") > >> added an i8042_command(..., I8042_CMD_AUX_[EN|DIS]ABLE) call to > >> the ideapad-laptop driver to suppress the touchpad events at the PS/2 > >> AUX controller level. > >> > >> Commit c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux > >> port on/off on select models") limited this to only do this by default > >> on the IdeaPad Z570 to replace a growing list of models on which > >> the i8042_command() call was disabled by quirks because it was causing > >> issues. > >> > >> A recent report shows that this is causing issues even on the Z570 for > >> which it was originally added because it can happen on resume before > >> the i8042 controller's own resume() method has run: > >> > >> [ 50.241235] ideapad_acpi VPC2004:00: PM: calling acpi_subsys_resume+0x0/0x5d @ 4492, parent: PNP0C09:00 > >> [ 50.242055] snd_hda_intel 0000:00:0e.0: PM: pci_pm_resume+0x0/0xed returned 0 after 13511 usecs > >> [ 50.242120] snd_hda_codec_realtek hdaudioC0D0: PM: calling hda_codec_pm_resume+0x0/0x19 [snd_hda_codec] @ 4518, parent: 0000:00:0e.0 > >> [ 50.247406] i8042: [49434] a8 -> i8042 (command) > >> [ 50.247468] ideapad_acpi VPC2004:00: PM: acpi_subsys_resume+0x0/0x5d returned 0 after 6220 usecs > >> ... > >> [ 50.247883] i8042 kbd 00:01: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 > >> [ 50.247894] i8042 kbd 00:01: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs > >> [ 50.247906] i8042 aux 00:02: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 > >> [ 50.247916] i8042 aux 00:02: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs > >> ... > >> [ 50.248301] i8042 i8042: PM: calling platform_pm_resume+0x0/0x41 @ 4492, parent: platform > >> [ 50.248377] i8042: [49434] 55 <- i8042 (flush, kbd) > >> [ 50.248407] i8042: [49435] aa -> i8042 (command) > >> [ 50.248601] i8042: [49435] 00 <- i8042 (return) > >> [ 50.248604] i8042: [49435] i8042 controller selftest: 0x0 != 0x55 > > > > What exactly is the issue? Is it just a few errors in dmesg, or does > > 8042 stop responding completely? > > When this issue happens at resume the touchpad stops sending events > completely because the i8042 driver's resume() method fails and exits > early. We actually retry up to 5 times so we usually get the right response from the controller. Additionally on x86 we do not abort initialization/resume even if controller selftest still fails after all the retries. > > > > > I've seen something similar when I enabled the touchpad while moving the > > cursor, but it was just a matter of a few lines in dmesg and a protocol > > resync, both touchpad and keyboard worked after that. > > Right, the problem is that in this case the i8042's resume() method > is failing, which I believe causes the Elan ps/2 driver to not get > re-attached to the aux port on resume. There's a69ce592cbe0 ("Input: elantech - fix touchpad state on resume for Lenovo N24") that sends disable/enable pair as part of Elan touchpad resume handling which unwedges the touchpad. > > > > > >> Dmitry (input subsys maintainer) pointed out that just sending > >> KEY_TOUCHPAD_OFF/KEY_TOUCHPAD_ON which the ideapad-laptop driver > >> already does should be sufficient and that it then is up to userspace > >> to filter out touchpad events after having received a KEY_TOUCHPAD_OFF. > > > > I believe it's not the case (at least it wasn't back then). The whole > > point of my patch in the first place was to make touchpad toggle work > > properly on Z570. > > > > Userspace (GNOME) supports two variants of hardware: > > > > 1. Laptops that disable touchpad themselves and send out > > KEY_TOUCHPAD_ON/OFF to report the status. Upon receiving these keycodes, > > GNOME just shows the status pop-up and relies on firmware to disable the > > touchpad. > > > > 2. Laptops that just send KEY_TOUCHPAD_TOGGLE whenever the key is > > pressed. GNOME maintains its own touchpad state and disables it in > > software (as well as showing the pop-up). > > You're right I had forgotten about this. There is really no reason > why GNOME cannot also suppress events after a TOUCHPAD_OFF event, > but atm it indeed does not do this. We could fix this by patching: > plugins/media-keys/gsd-media-keys-manager.c of gnome-settings-daemon > to also update the TOUCHPAD_ENABLED_KEY setting when receiving > KEY_TOUCHPAD_ON/OFF. Something which I think we should do to, > but that will not help solve this bug since we cannot rely > on users having a fixed g-s-d. > > So: self-NACK for this patch. (which is a bummer because I really > liked being able to just remove this) > > > That means, userspace is not filtering out events upon receiving > > KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send > > KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 > > is weird. It maintains the touchpad state in firmware to light up the > > status LED, but the firmware doesn't do the actual touchpad disablement. > > > > That is, if we use TOGGLE, the LED will get out of sync. If we use > > ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. Still, poking the touchpad directly at a random time is not something that we should be doing. The command may come in the middle of touchpad initialization or in the middle of resuming, or at another inopportune moment - as you mentioned yourself toggling while using the touchpad results in a spew in dmesg. We have "inhibit/uninhibit" sysfs controls that allow suppressing input events form a device, they should be used instead. > > Ack. > > So how about this instead: > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 1ace711f7442..b7fa06f793cb 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > * touchpad off and on. We send KEY_TOUCHPAD_OFF and > * KEY_TOUCHPAD_ON to not to get out of sync with LED > */ > - if (priv->features.ctrl_ps2_aux_port) > + if (send_events && priv->features.ctrl_ps2_aux_port) > i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > > /* > > Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume > correctly reflects the state before suspend/resume in both touchpad on / off states ? > > Jonathan, as the reporter of the original suspend/resume issue, can you check if > a kernel with this patch + ideapad-laptop re-enabled no longer has the suspend/resume > issue you were seeing ? > > Regards, > > Hans > Thanks.
Hi, On 8/5/24 7:00 PM, Dmitry Torokhov wrote: > On Mon, Aug 05, 2024 at 05:45:19PM +0200, Hans de Goede wrote: >> Hi Maxim, >> >> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: >>> On Mon, 05 Aug 2024 at 16:16:08 +0200, Hans de Goede wrote: >>>> Commit 07a4a4fc83dd ("ideapad: add Lenovo IdeaPad Z570 support (part 2)") >>>> added an i8042_command(..., I8042_CMD_AUX_[EN|DIS]ABLE) call to >>>> the ideapad-laptop driver to suppress the touchpad events at the PS/2 >>>> AUX controller level. >>>> >>>> Commit c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux >>>> port on/off on select models") limited this to only do this by default >>>> on the IdeaPad Z570 to replace a growing list of models on which >>>> the i8042_command() call was disabled by quirks because it was causing >>>> issues. >>>> >>>> A recent report shows that this is causing issues even on the Z570 for >>>> which it was originally added because it can happen on resume before >>>> the i8042 controller's own resume() method has run: >>>> >>>> [ 50.241235] ideapad_acpi VPC2004:00: PM: calling acpi_subsys_resume+0x0/0x5d @ 4492, parent: PNP0C09:00 >>>> [ 50.242055] snd_hda_intel 0000:00:0e.0: PM: pci_pm_resume+0x0/0xed returned 0 after 13511 usecs >>>> [ 50.242120] snd_hda_codec_realtek hdaudioC0D0: PM: calling hda_codec_pm_resume+0x0/0x19 [snd_hda_codec] @ 4518, parent: 0000:00:0e.0 >>>> [ 50.247406] i8042: [49434] a8 -> i8042 (command) >>>> [ 50.247468] ideapad_acpi VPC2004:00: PM: acpi_subsys_resume+0x0/0x5d returned 0 after 6220 usecs >>>> ... >>>> [ 50.247883] i8042 kbd 00:01: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 >>>> [ 50.247894] i8042 kbd 00:01: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs >>>> [ 50.247906] i8042 aux 00:02: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 >>>> [ 50.247916] i8042 aux 00:02: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs >>>> ... >>>> [ 50.248301] i8042 i8042: PM: calling platform_pm_resume+0x0/0x41 @ 4492, parent: platform >>>> [ 50.248377] i8042: [49434] 55 <- i8042 (flush, kbd) >>>> [ 50.248407] i8042: [49435] aa -> i8042 (command) >>>> [ 50.248601] i8042: [49435] 00 <- i8042 (return) >>>> [ 50.248604] i8042: [49435] i8042 controller selftest: 0x0 != 0x55 >>> >>> What exactly is the issue? Is it just a few errors in dmesg, or does >>> 8042 stop responding completely? >> >> When this issue happens at resume the touchpad stops sending events >> completely because the i8042 driver's resume() method fails and exits >> early. > > We actually retry up to 5 times so we usually get the right response > from the controller. Additionally on x86 we do not abort > initialization/resume even if controller selftest still fails after all > the retries. > >> >>> >>> I've seen something similar when I enabled the touchpad while moving the >>> cursor, but it was just a matter of a few lines in dmesg and a protocol >>> resync, both touchpad and keyboard worked after that. >> >> Right, the problem is that in this case the i8042's resume() method >> is failing, which I believe causes the Elan ps/2 driver to not get >> re-attached to the aux port on resume. > > There's a69ce592cbe0 ("Input: elantech - fix touchpad state on resume > for Lenovo N24") that sends disable/enable pair as part of Elan touchpad > resume handling which unwedges the touchpad. > >> >> >>> >>>> Dmitry (input subsys maintainer) pointed out that just sending >>>> KEY_TOUCHPAD_OFF/KEY_TOUCHPAD_ON which the ideapad-laptop driver >>>> already does should be sufficient and that it then is up to userspace >>>> to filter out touchpad events after having received a KEY_TOUCHPAD_OFF. >>> >>> I believe it's not the case (at least it wasn't back then). The whole >>> point of my patch in the first place was to make touchpad toggle work >>> properly on Z570. >>> >>> Userspace (GNOME) supports two variants of hardware: >>> >>> 1. Laptops that disable touchpad themselves and send out >>> KEY_TOUCHPAD_ON/OFF to report the status. Upon receiving these keycodes, >>> GNOME just shows the status pop-up and relies on firmware to disable the >>> touchpad. >>> >>> 2. Laptops that just send KEY_TOUCHPAD_TOGGLE whenever the key is >>> pressed. GNOME maintains its own touchpad state and disables it in >>> software (as well as showing the pop-up). >> >> You're right I had forgotten about this. There is really no reason >> why GNOME cannot also suppress events after a TOUCHPAD_OFF event, >> but atm it indeed does not do this. We could fix this by patching: >> plugins/media-keys/gsd-media-keys-manager.c of gnome-settings-daemon >> to also update the TOUCHPAD_ENABLED_KEY setting when receiving >> KEY_TOUCHPAD_ON/OFF. Something which I think we should do to, >> but that will not help solve this bug since we cannot rely >> on users having a fixed g-s-d. >> >> So: self-NACK for this patch. (which is a bummer because I really >> liked being able to just remove this) >> >>> That means, userspace is not filtering out events upon receiving >>> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send >>> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 >>> is weird. It maintains the touchpad state in firmware to light up the >>> status LED, but the firmware doesn't do the actual touchpad disablement. >>> >>> That is, if we use TOGGLE, the LED will get out of sync. If we use >>> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. > > Still, poking the touchpad directly at a random time is not something > that we should be doing. The command may come in the middle of touchpad > initialization or in the middle of resuming, or at another inopportune > moment - as you mentioned yourself toggling while using the touchpad > results in a spew in dmesg. > > We have "inhibit/uninhibit" sysfs controls that allow suppressing input > events form a device, they should be used instead. Using those indeed would be better, I guess this requires 2 things: 1. Some helper to find the struct input_dev for the input_dev related to the ps/2 aux port 2. In kernel API / functions to do inhibit/uninhibit (maybe these already exist?) Anyways I have to focus on camera stuff for the rest of this week, so lets continue this discussion next week. Regards, Hans
On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote: > On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: > > That means, userspace is not filtering out events upon receiving > > KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send > > KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 > > is weird. It maintains the touchpad state in firmware to light up the > > status LED, but the firmware doesn't do the actual touchpad disablement. > > > > That is, if we use TOGGLE, the LED will get out of sync. If we use > > ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. > > Ack. > > So how about this instead: > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 1ace711f7442..b7fa06f793cb 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > * touchpad off and on. We send KEY_TOUCHPAD_OFF and > * KEY_TOUCHPAD_ON to not to get out of sync with LED > */ > - if (priv->features.ctrl_ps2_aux_port) > + if (send_events && priv->features.ctrl_ps2_aux_port) > i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > > /* > > Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume > correctly reflects the state before suspend/resume in both touchpad on / off states ? *Maxim Just a heads-up, my Z570 now belongs to a family member, we'll test what you asked, but right now there is a btrfs corruption on that laptop that we need to fix first, it interferes with kernel compilation =/ Sorry for the delay.
Hi Maxim, On 8/12/24 4:37 PM, Maxim Mikityanskiy wrote: > On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote: >> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: >>> That means, userspace is not filtering out events upon receiving >>> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send >>> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 >>> is weird. It maintains the touchpad state in firmware to light up the >>> status LED, but the firmware doesn't do the actual touchpad disablement. >>> >>> That is, if we use TOGGLE, the LED will get out of sync. If we use >>> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. >> >> Ack. >> >> So how about this instead: >> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c >> index 1ace711f7442..b7fa06f793cb 100644 >> --- a/drivers/platform/x86/ideapad-laptop.c >> +++ b/drivers/platform/x86/ideapad-laptop.c >> @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ >> * touchpad off and on. We send KEY_TOUCHPAD_OFF and >> * KEY_TOUCHPAD_ON to not to get out of sync with LED >> */ >> - if (priv->features.ctrl_ps2_aux_port) >> + if (send_events && priv->features.ctrl_ps2_aux_port) >> i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); >> >> /* >> >> Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume >> correctly reflects the state before suspend/resume in both touchpad on / off states ? > > *Maxim Oops, sorry. > Just a heads-up, my Z570 now belongs to a family member, we'll test what > you asked, but right now there is a btrfs corruption on that laptop that > we need to fix first, it interferes with kernel compilation =/ Note as discussed in another part of the thread the original bug report actually was not on a Z570, so the whole usage of i8042_command() on suspend/resume was a bit of a red herring. And the suspend/resume issue has been fixed in another way in the mean time. So there really is no need to test this change anymore. At the moment there are no planned changes to ideapad-laptop related to this. Good luck with the BTRFS problem. Regards, Hans
On Mon, Aug 12, 2024 at 04:41:50PM +0200, Hans de Goede wrote: > Hi Maxim, > > On 8/12/24 4:37 PM, Maxim Mikityanskiy wrote: > > On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote: > >> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: > >>> That means, userspace is not filtering out events upon receiving > >>> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send > >>> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 > >>> is weird. It maintains the touchpad state in firmware to light up the > >>> status LED, but the firmware doesn't do the actual touchpad disablement. > >>> > >>> That is, if we use TOGGLE, the LED will get out of sync. If we use > >>> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. > >> > >> Ack. > >> > >> So how about this instead: > >> > >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > >> index 1ace711f7442..b7fa06f793cb 100644 > >> --- a/drivers/platform/x86/ideapad-laptop.c > >> +++ b/drivers/platform/x86/ideapad-laptop.c > >> @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > >> * touchpad off and on. We send KEY_TOUCHPAD_OFF and > >> * KEY_TOUCHPAD_ON to not to get out of sync with LED > >> */ > >> - if (priv->features.ctrl_ps2_aux_port) > >> + if (send_events && priv->features.ctrl_ps2_aux_port) > >> i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > >> > >> /* > >> > >> Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume > >> correctly reflects the state before suspend/resume in both touchpad on / off states ? > > > > *Maxim > > Oops, sorry. > > > Just a heads-up, my Z570 now belongs to a family member, we'll test what > > you asked, but right now there is a btrfs corruption on that laptop that > > we need to fix first, it interferes with kernel compilation =/ > > Note as discussed in another part of the thread the original bug report > actually was not on a Z570, so the whole usage of i8042_command() on > suspend/resume was a bit of a red herring. And the suspend/resume issue > has been fixed in another way in the mean time. > > So there really is no need to test this change anymore. At the moment > there are no planned changes to ideapad-laptop related to this. I think we still need to stop ideapad-laptop poking into 8042, especially ahead of time. If we do not want to wait for userspace to handle this properly, I wonder if we could not create an input_handler that would attach to the touchpad device and filter out all events coming from the touchpad if touchpad is supposed to be off. Thanks.
Hi Dmitry, On 8/12/24 7:24 PM, Dmitry Torokhov wrote: > On Mon, Aug 12, 2024 at 04:41:50PM +0200, Hans de Goede wrote: >> Hi Maxim, >> >> On 8/12/24 4:37 PM, Maxim Mikityanskiy wrote: >>> On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote: >>>> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: >>>>> That means, userspace is not filtering out events upon receiving >>>>> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send >>>>> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 >>>>> is weird. It maintains the touchpad state in firmware to light up the >>>>> status LED, but the firmware doesn't do the actual touchpad disablement. >>>>> >>>>> That is, if we use TOGGLE, the LED will get out of sync. If we use >>>>> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. >>>> >>>> Ack. >>>> >>>> So how about this instead: >>>> >>>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c >>>> index 1ace711f7442..b7fa06f793cb 100644 >>>> --- a/drivers/platform/x86/ideapad-laptop.c >>>> +++ b/drivers/platform/x86/ideapad-laptop.c >>>> @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ >>>> * touchpad off and on. We send KEY_TOUCHPAD_OFF and >>>> * KEY_TOUCHPAD_ON to not to get out of sync with LED >>>> */ >>>> - if (priv->features.ctrl_ps2_aux_port) >>>> + if (send_events && priv->features.ctrl_ps2_aux_port) >>>> i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); >>>> >>>> /* >>>> >>>> Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume >>>> correctly reflects the state before suspend/resume in both touchpad on / off states ? >>> >>> *Maxim >> >> Oops, sorry. >> >>> Just a heads-up, my Z570 now belongs to a family member, we'll test what >>> you asked, but right now there is a btrfs corruption on that laptop that >>> we need to fix first, it interferes with kernel compilation =/ >> >> Note as discussed in another part of the thread the original bug report >> actually was not on a Z570, so the whole usage of i8042_command() on >> suspend/resume was a bit of a red herring. And the suspend/resume issue >> has been fixed in another way in the mean time. >> >> So there really is no need to test this change anymore. At the moment >> there are no planned changes to ideapad-laptop related to this. > > I think we still need to stop ideapad-laptop poking into 8042, > especially ahead of time. I agree. I think your suggestion of using the new(ish) [un]inhibit support in the input subsystem for this instead of poking at the i8042 is a good idea. As I mentioned when you first suggested this, I guess this requires 2 things: 1. Some helper to find the struct input_dev for the input_dev related to the ps/2 aux port 2. In kernel API / functions to do inhibit/uninhibit (maybe these already exist?) > If we do not want to wait for userspace to > handle this properly, I wonder if we could not create an > input_handler that would attach to the touchpad device and filter out > all events coming from the touchpad if touchpad is supposed to be off. I think using the inhibit stuff would be better no? The biggest problems with trying to fix this are: 1. Finding time to work on this 2. Finding someone willing to test the patches Finding the time is going to be an issue for me since the i8042_command() calls are only still done on a single model laptop (using a DMI quirk) inside ideapad-laptop now, so this is pretty low priority IMHO. Which in practice means that I will simply never get around to this, sorry... Regards, Hans
On Mon, Aug 12, 2024 at 08:18:24PM +0200, Hans de Goede wrote: > Hi Dmitry, > > On 8/12/24 7:24 PM, Dmitry Torokhov wrote: > > On Mon, Aug 12, 2024 at 04:41:50PM +0200, Hans de Goede wrote: > >> Hi Maxim, > >> > >> On 8/12/24 4:37 PM, Maxim Mikityanskiy wrote: > >>> On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote: > >>>> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: > >>>>> That means, userspace is not filtering out events upon receiving > >>>>> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send > >>>>> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 > >>>>> is weird. It maintains the touchpad state in firmware to light up the > >>>>> status LED, but the firmware doesn't do the actual touchpad disablement. > >>>>> > >>>>> That is, if we use TOGGLE, the LED will get out of sync. If we use > >>>>> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. > >>>> > >>>> Ack. > >>>> > >>>> So how about this instead: > >>>> > >>>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > >>>> index 1ace711f7442..b7fa06f793cb 100644 > >>>> --- a/drivers/platform/x86/ideapad-laptop.c > >>>> +++ b/drivers/platform/x86/ideapad-laptop.c > >>>> @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > >>>> * touchpad off and on. We send KEY_TOUCHPAD_OFF and > >>>> * KEY_TOUCHPAD_ON to not to get out of sync with LED > >>>> */ > >>>> - if (priv->features.ctrl_ps2_aux_port) > >>>> + if (send_events && priv->features.ctrl_ps2_aux_port) > >>>> i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > >>>> > >>>> /* > >>>> > >>>> Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume > >>>> correctly reflects the state before suspend/resume in both touchpad on / off states ? > >>> > >>> *Maxim > >> > >> Oops, sorry. > >> > >>> Just a heads-up, my Z570 now belongs to a family member, we'll test what > >>> you asked, but right now there is a btrfs corruption on that laptop that > >>> we need to fix first, it interferes with kernel compilation =/ > >> > >> Note as discussed in another part of the thread the original bug report > >> actually was not on a Z570, so the whole usage of i8042_command() on > >> suspend/resume was a bit of a red herring. And the suspend/resume issue > >> has been fixed in another way in the mean time. > >> > >> So there really is no need to test this change anymore. At the moment > >> there are no planned changes to ideapad-laptop related to this. > > > > I think we still need to stop ideapad-laptop poking into 8042, > > especially ahead of time. > > I agree. I think your suggestion of using the new(ish) [un]inhibit > support in the input subsystem for this instead of poking at the i8042 > is a good idea. > > As I mentioned when you first suggested this, I guess this requires 2 things: > > 1. Some helper to find the struct input_dev for the input_dev related > to the ps/2 aux port > 2. In kernel API / functions to do inhibit/uninhibit > (maybe these already exist?) > > > If we do not want to wait for userspace to > > handle this properly, I wonder if we could not create an > > input_handler that would attach to the touchpad device and filter out > > all events coming from the touchpad if touchpad is supposed to be off. > > I think using the inhibit stuff would be better no? The issue with inhibit/uninhibit is that they are only exposed to userpsace via sysfs. And as you mentioned we need to locate the input device corresponding to the touchpad. With input handler we are essentially getting both - psmouse does not do anything special in inhibit so it is just the input core dropping events, the same as with the filter handler, and we can use hanlder's match table to limit it to the touchpad and input core will find the device for us. > > The biggest problems with trying to fix this are: > > 1. Finding time to work on this > 2. Finding someone willing to test the patches > > Finding the time is going to be an issue for me since the i8042_command() > calls are only still done on a single model laptop (using a DMI quirk) > inside ideapad-laptop now, so this is pretty low priority IMHO. Which > in practice means that I will simply never get around to this, sorry... Yeah, I can see that ;) Maybe I will find a couple of hours to waste... Thanks.
On Mon, Aug 12, 2024 at 12:26:47PM -0700, Dmitry Torokhov wrote: > On Mon, Aug 12, 2024 at 08:18:24PM +0200, Hans de Goede wrote: > > Hi Dmitry, > > > > On 8/12/24 7:24 PM, Dmitry Torokhov wrote: > > > On Mon, Aug 12, 2024 at 04:41:50PM +0200, Hans de Goede wrote: > > >> Hi Maxim, > > >> > > >> On 8/12/24 4:37 PM, Maxim Mikityanskiy wrote: > > >>> On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote: > > >>>> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: > > >>>>> That means, userspace is not filtering out events upon receiving > > >>>>> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send > > >>>>> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 > > >>>>> is weird. It maintains the touchpad state in firmware to light up the > > >>>>> status LED, but the firmware doesn't do the actual touchpad disablement. > > >>>>> > > >>>>> That is, if we use TOGGLE, the LED will get out of sync. If we use > > >>>>> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. > > >>>> > > >>>> Ack. > > >>>> > > >>>> So how about this instead: > > >>>> > > >>>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > > >>>> index 1ace711f7442..b7fa06f793cb 100644 > > >>>> --- a/drivers/platform/x86/ideapad-laptop.c > > >>>> +++ b/drivers/platform/x86/ideapad-laptop.c > > >>>> @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > > >>>> * touchpad off and on. We send KEY_TOUCHPAD_OFF and > > >>>> * KEY_TOUCHPAD_ON to not to get out of sync with LED > > >>>> */ > > >>>> - if (priv->features.ctrl_ps2_aux_port) > > >>>> + if (send_events && priv->features.ctrl_ps2_aux_port) > > >>>> i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > > >>>> > > >>>> /* > > >>>> > > >>>> Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume > > >>>> correctly reflects the state before suspend/resume in both touchpad on / off states ? > > >>> > > >>> *Maxim > > >> > > >> Oops, sorry. > > >> > > >>> Just a heads-up, my Z570 now belongs to a family member, we'll test what > > >>> you asked, but right now there is a btrfs corruption on that laptop that > > >>> we need to fix first, it interferes with kernel compilation =/ > > >> > > >> Note as discussed in another part of the thread the original bug report > > >> actually was not on a Z570, so the whole usage of i8042_command() on > > >> suspend/resume was a bit of a red herring. And the suspend/resume issue > > >> has been fixed in another way in the mean time. > > >> > > >> So there really is no need to test this change anymore. At the moment > > >> there are no planned changes to ideapad-laptop related to this. > > > > > > I think we still need to stop ideapad-laptop poking into 8042, > > > especially ahead of time. > > > > I agree. I think your suggestion of using the new(ish) [un]inhibit > > support in the input subsystem for this instead of poking at the i8042 > > is a good idea. > > > > As I mentioned when you first suggested this, I guess this requires 2 things: > > > > 1. Some helper to find the struct input_dev for the input_dev related > > to the ps/2 aux port > > 2. In kernel API / functions to do inhibit/uninhibit > > (maybe these already exist?) > > > > > If we do not want to wait for userspace to > > > handle this properly, I wonder if we could not create an > > > input_handler that would attach to the touchpad device and filter out > > > all events coming from the touchpad if touchpad is supposed to be off. > > > > I think using the inhibit stuff would be better no? > > The issue with inhibit/uninhibit is that they are only exposed to > userpsace via sysfs. And as you mentioned we need to locate the input > device corresponding to the touchpad. > > With input handler we are essentially getting both - psmouse does not do > anything special in inhibit so it is just the input core dropping > events, the same as with the filter handler, and we can use hanlder's > match table to limit it to the touchpad and input core will find the > device for us. > > > > > The biggest problems with trying to fix this are: > > > > 1. Finding time to work on this > > 2. Finding someone willing to test the patches > > > > Finding the time is going to be an issue for me since the i8042_command() > > calls are only still done on a single model laptop (using a DMI quirk) > > inside ideapad-laptop now, so this is pretty low priority IMHO. Which > > in practice means that I will simply never get around to this, sorry... > > Yeah, I can see that ;) Maybe I will find a couple of hours to waste... Maybe something like below can work? platform/x86: ideapad-laptop: do not poke keyboard controller From: Dmitry Torokhov <dmitry.torokhov@gmail.com> On Ideapad Z570 the driver tries to disable and reenable data coming from the touchpad by poking directly into 8042 keyboard controller. This may coincide with the controller resuming and leads to spews in dmesg and potentially other instabilities. Instead of using i8042_command() to control the touchpad state create a input handler that serves as a filter and drop events coming from the touchpad when it is supposed to be off. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/platform/x86/ideapad-laptop.c | 171 ++++++++++++++++++++++++++++++++- 1 file changed, 168 insertions(+), 3 deletions(-) diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index fcf13d88fd6e..2f40feefd5e3 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -17,7 +17,6 @@ #include <linux/device.h> #include <linux/dmi.h> #include <linux/fb.h> -#include <linux/i8042.h> #include <linux/init.h> #include <linux/input.h> #include <linux/input/sparse-keymap.h> @@ -157,6 +156,13 @@ struct ideapad_private { struct led_classdev led; unsigned int last_brightness; } fn_lock; + struct { + bool initialized; + bool active; + struct input_handler handler; + struct input_dev *tp_dev; + spinlock_t lock; + } tp_switch; }; static bool no_bt_rfkill; @@ -1236,6 +1242,158 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv) } } +struct ideapad_tpswitch_handle { + struct input_handle handle; + struct ideapad_private *priv; +}; + +#define to_tpswitch_handle(h) \ + container_of(h, struct ideapad_tpswitch_handle, handle); + +static int ideapad_tpswitch_connect(struct input_handler *handler, + struct input_dev *dev, + const struct input_device_id *id) +{ + struct ideapad_private *priv = + container_of(handler, struct ideapad_private, tp_switch.handler); + struct ideapad_tpswitch_handle *h; + int error; + + h = kzalloc(sizeof(*h), GFP_KERNEL); + if (!h) + return -ENOMEM; + + h->priv = priv; + h->handle.dev = dev; + h->handle.handler = handler; + h->handle.name = "ideapad-tpswitch"; + + error = input_register_handle(&h->handle); + if (error) + goto err_free_handle; + + /* + * FIXME: ideally we do not want to open the input device here + * if there are no other users. We need a notion of "observer" + * handlers in the input core. + */ + error = input_open_device(&h->handle); + if (error) + goto err_unregister_handle; + + scoped_guard(spinlock_irq, &priv->tp_switch.lock) + priv->tp_switch.tp_dev = dev; + + return 0; + + err_unregister_handle: + input_unregister_handle(&h->handle); +err_free_handle: + kfree(h); + return error; +} + +static void ideapad_tpswitch_disconnect(struct input_handle *handle) +{ + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); + struct ideapad_private *priv = h->priv; + + scoped_guard(spinlock_irq, &priv->tp_switch.lock) + priv->tp_switch.tp_dev = NULL; + + input_close_device(handle); + input_unregister_handle(handle); + kfree(h); +} + +static bool ideapad_tpswitch_filter(struct input_handle *handle, + unsigned int type, unsigned int code, + int value) +{ + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); + struct ideapad_private *priv = h->priv; + + if (!priv->tp_switch.active) + return false; + + /* Allow passing button release events, drop everything else */ + return !(type == EV_KEY && value == 0) && + !(type == EV_SYN && code == SYN_REPORT); + +} + +static const struct input_device_id ideapad_tpswitch_ids[] = { + { + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | + INPUT_DEVICE_ID_MATCH_KEYBIT | + INPUT_DEVICE_ID_MATCH_ABSBIT, + .bustype = BUS_I8042, + .vendor = 0x0002, + .evbit = { BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS) }, + .keybit = { [BIT_WORD(BTN_TOOL_FINGER)] = + BIT_MASK(BTN_TOOL_FINGER) }, + .absbit = { BIT_MASK(ABS_X) | BIT_MASK(ABS_Y) | + BIT_MASK(ABS_PRESSURE) | + BIT_MASK(ABS_TOOL_WIDTH) }, + }, + { } +}; + +static int ideapad_tpswitch_init(struct ideapad_private *priv) +{ + int error; + + if (!priv->features.ctrl_ps2_aux_port) + return 0; + + spin_lock_init(&priv->tp_switch.lock); + + priv->tp_switch.handler.name = "ideapad-tpswitch"; + priv->tp_switch.handler.id_table = ideapad_tpswitch_ids; + priv->tp_switch.handler.filter = ideapad_tpswitch_filter; + priv->tp_switch.handler.connect = ideapad_tpswitch_connect; + priv->tp_switch.handler.disconnect = ideapad_tpswitch_disconnect; + + error = input_register_handler(&priv->tp_switch.handler); + if (error) { + dev_err(&priv->platform_device->dev, + "failed to register touchpad switch handler: %d", + error); + return error; + } + + priv->tp_switch.initialized = true; + return 0; +} + +static void ideapad_tpswitch_exit(struct ideapad_private *priv) +{ + if (priv->tp_switch.initialized) { + input_unregister_handler(&priv->tp_switch.handler); + priv->tp_switch.initialized = false; + } +} + +static void ideapad_tpswitch_toggle(struct ideapad_private *priv, bool on) +{ + guard(spinlock_irq)(&priv->tp_switch.lock); + + priv->tp_switch.active = on; + if (on) { + struct input_dev *tp_dev = priv->tp_switch.tp_dev; + if (tp_dev) { + input_report_key(tp_dev, BTN_TOUCH, 0); + input_report_key(tp_dev, BTN_TOOL_FINGER, 0); + input_report_key(tp_dev, BTN_TOOL_DOUBLETAP, 0); + input_report_key(tp_dev, BTN_TOOL_TRIPLETAP, 0); + input_report_key(tp_dev, BTN_LEFT, 0); + input_report_key(tp_dev, BTN_RIGHT, 0); + input_report_key(tp_dev, BTN_MIDDLE, 0); + input_sync(tp_dev); + } + } +} + /* * backlight */ @@ -1567,7 +1725,6 @@ static void ideapad_fn_lock_led_exit(struct ideapad_private *priv) static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_events) { unsigned long value; - unsigned char param; int ret; /* Without reading from EC touchpad LED doesn't switch state */ @@ -1582,7 +1739,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ * KEY_TOUCHPAD_ON to not to get out of sync with LED */ if (priv->features.ctrl_ps2_aux_port) - i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); + ideapad_tpswitch_toggle(priv, value); /* * On older models the EC controls the touchpad and toggles it on/off @@ -1927,6 +2084,10 @@ static int ideapad_acpi_add(struct platform_device *pdev) if (err) goto input_failed; + err = ideapad_tpswitch_init(priv); + if (err) + goto tpswitch_failed; + err = ideapad_kbd_bl_init(priv); if (err) { if (err != -ENODEV) @@ -2001,6 +2162,9 @@ static int ideapad_acpi_add(struct platform_device *pdev) ideapad_fn_lock_led_exit(priv); ideapad_kbd_bl_exit(priv); + ideapad_tpswitch_exit(priv); + +tpswitch_failed: ideapad_input_exit(priv); input_failed: @@ -2029,6 +2193,7 @@ static void ideapad_acpi_remove(struct platform_device *pdev) ideapad_fn_lock_led_exit(priv); ideapad_kbd_bl_exit(priv); + ideapad_tpswitch_exit(priv); ideapad_input_exit(priv); ideapad_debugfs_exit(priv); ideapad_sysfs_exit(priv);
On Sun, 18 Aug 2024 at 13:30:37 -0700, Dmitry Torokhov wrote: > On Mon, Aug 12, 2024 at 12:26:47PM -0700, Dmitry Torokhov wrote: > > On Mon, Aug 12, 2024 at 08:18:24PM +0200, Hans de Goede wrote: > > > Hi Dmitry, > > > > > > On 8/12/24 7:24 PM, Dmitry Torokhov wrote: > > > > On Mon, Aug 12, 2024 at 04:41:50PM +0200, Hans de Goede wrote: > > > >> Hi Maxim, > > > >> > > > >> On 8/12/24 4:37 PM, Maxim Mikityanskiy wrote: > > > >>> On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote: > > > >>>> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: > > > >>>>> That means, userspace is not filtering out events upon receiving > > > >>>>> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send > > > >>>>> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 > > > >>>>> is weird. It maintains the touchpad state in firmware to light up the > > > >>>>> status LED, but the firmware doesn't do the actual touchpad disablement. > > > >>>>> > > > >>>>> That is, if we use TOGGLE, the LED will get out of sync. If we use > > > >>>>> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. > > > >>>> > > > >>>> Ack. > > > >>>> > > > >>>> So how about this instead: > > > >>>> > > > >>>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > > > >>>> index 1ace711f7442..b7fa06f793cb 100644 > > > >>>> --- a/drivers/platform/x86/ideapad-laptop.c > > > >>>> +++ b/drivers/platform/x86/ideapad-laptop.c > > > >>>> @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > > > >>>> * touchpad off and on. We send KEY_TOUCHPAD_OFF and > > > >>>> * KEY_TOUCHPAD_ON to not to get out of sync with LED > > > >>>> */ > > > >>>> - if (priv->features.ctrl_ps2_aux_port) > > > >>>> + if (send_events && priv->features.ctrl_ps2_aux_port) > > > >>>> i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > > > >>>> > > > >>>> /* > > > >>>> > > > >>>> Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume > > > >>>> correctly reflects the state before suspend/resume in both touchpad on / off states ? > > > >>> > > > >>> *Maxim > > > >> > > > >> Oops, sorry. > > > >> > > > >>> Just a heads-up, my Z570 now belongs to a family member, we'll test what > > > >>> you asked, but right now there is a btrfs corruption on that laptop that > > > >>> we need to fix first, it interferes with kernel compilation =/ > > > >> > > > >> Note as discussed in another part of the thread the original bug report > > > >> actually was not on a Z570, so the whole usage of i8042_command() on > > > >> suspend/resume was a bit of a red herring. And the suspend/resume issue > > > >> has been fixed in another way in the mean time. > > > >> > > > >> So there really is no need to test this change anymore. At the moment > > > >> there are no planned changes to ideapad-laptop related to this. > > > > > > > > I think we still need to stop ideapad-laptop poking into 8042, > > > > especially ahead of time. > > > > > > I agree. I think your suggestion of using the new(ish) [un]inhibit > > > support in the input subsystem for this instead of poking at the i8042 > > > is a good idea. > > > > > > As I mentioned when you first suggested this, I guess this requires 2 things: > > > > > > 1. Some helper to find the struct input_dev for the input_dev related > > > to the ps/2 aux port > > > 2. In kernel API / functions to do inhibit/uninhibit > > > (maybe these already exist?) > > > > > > > If we do not want to wait for userspace to > > > > handle this properly, I wonder if we could not create an > > > > input_handler that would attach to the touchpad device and filter out > > > > all events coming from the touchpad if touchpad is supposed to be off. > > > > > > I think using the inhibit stuff would be better no? > > > > The issue with inhibit/uninhibit is that they are only exposed to > > userpsace via sysfs. And as you mentioned we need to locate the input > > device corresponding to the touchpad. > > > > With input handler we are essentially getting both - psmouse does not do > > anything special in inhibit so it is just the input core dropping > > events, the same as with the filter handler, and we can use hanlder's > > match table to limit it to the touchpad and input core will find the > > device for us. > > > > > > > > The biggest problems with trying to fix this are: > > > > > > 1. Finding time to work on this > > > 2. Finding someone willing to test the patches > > > > > > Finding the time is going to be an issue for me since the i8042_command() > > > calls are only still done on a single model laptop (using a DMI quirk) > > > inside ideapad-laptop now, so this is pretty low priority IMHO. Which > > > in practice means that I will simply never get around to this, sorry... > > > > Yeah, I can see that ;) Maybe I will find a couple of hours to waste... > > Maybe something like below can work? Great patch, thank you, I'll test it and report the results. See some minor comments below. > > > platform/x86: ideapad-laptop: do not poke keyboard controller > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > On Ideapad Z570 the driver tries to disable and reenable data coming > from the touchpad by poking directly into 8042 keyboard controller. > This may coincide with the controller resuming and leads to spews in > dmesg and potentially other instabilities. > > Instead of using i8042_command() to control the touchpad state create a > input handler that serves as a filter and drop events coming from the > touchpad when it is supposed to be off. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/platform/x86/ideapad-laptop.c | 171 ++++++++++++++++++++++++++++++++- > 1 file changed, 168 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index fcf13d88fd6e..2f40feefd5e3 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -17,7 +17,6 @@ > #include <linux/device.h> > #include <linux/dmi.h> > #include <linux/fb.h> > -#include <linux/i8042.h> > #include <linux/init.h> > #include <linux/input.h> > #include <linux/input/sparse-keymap.h> > @@ -157,6 +156,13 @@ struct ideapad_private { > struct led_classdev led; > unsigned int last_brightness; > } fn_lock; > + struct { > + bool initialized; > + bool active; > + struct input_handler handler; > + struct input_dev *tp_dev; > + spinlock_t lock; > + } tp_switch; > }; > > static bool no_bt_rfkill; > @@ -1236,6 +1242,158 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv) > } > } > > +struct ideapad_tpswitch_handle { > + struct input_handle handle; > + struct ideapad_private *priv; > +}; > + > +#define to_tpswitch_handle(h) \ > + container_of(h, struct ideapad_tpswitch_handle, handle); > + > +static int ideapad_tpswitch_connect(struct input_handler *handler, > + struct input_dev *dev, > + const struct input_device_id *id) > +{ > + struct ideapad_private *priv = > + container_of(handler, struct ideapad_private, tp_switch.handler); > + struct ideapad_tpswitch_handle *h; > + int error; > + > + h = kzalloc(sizeof(*h), GFP_KERNEL); > + if (!h) > + return -ENOMEM; > + > + h->priv = priv; > + h->handle.dev = dev; > + h->handle.handler = handler; > + h->handle.name = "ideapad-tpswitch"; > + > + error = input_register_handle(&h->handle); > + if (error) > + goto err_free_handle; > + > + /* > + * FIXME: ideally we do not want to open the input device here > + * if there are no other users. We need a notion of "observer" > + * handlers in the input core. > + */ > + error = input_open_device(&h->handle); > + if (error) > + goto err_unregister_handle; > + > + scoped_guard(spinlock_irq, &priv->tp_switch.lock) > + priv->tp_switch.tp_dev = dev; > + > + return 0; > + > + err_unregister_handle: > + input_unregister_handle(&h->handle); > +err_free_handle: > + kfree(h); > + return error; > +} > + > +static void ideapad_tpswitch_disconnect(struct input_handle *handle) > +{ > + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); > + struct ideapad_private *priv = h->priv; > + > + scoped_guard(spinlock_irq, &priv->tp_switch.lock) Nice syntax, I didn't know about it before. > + priv->tp_switch.tp_dev = NULL; > + > + input_close_device(handle); > + input_unregister_handle(handle); > + kfree(h); > +} > + > +static bool ideapad_tpswitch_filter(struct input_handle *handle, > + unsigned int type, unsigned int code, > + int value) > +{ > + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); > + struct ideapad_private *priv = h->priv; > + > + if (!priv->tp_switch.active) This check seems inverted. ideapad_tpswitch_toggle assigns true when the touchpad is enabled. > + return false; > + > + /* Allow passing button release events, drop everything else */ > + return !(type == EV_KEY && value == 0) && > + !(type == EV_SYN && code == SYN_REPORT); > + > +} > + > +static const struct input_device_id ideapad_tpswitch_ids[] = { > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > + INPUT_DEVICE_ID_MATCH_KEYBIT | > + INPUT_DEVICE_ID_MATCH_ABSBIT, > + .bustype = BUS_I8042, > + .vendor = 0x0002, > + .evbit = { BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS) }, > + .keybit = { [BIT_WORD(BTN_TOOL_FINGER)] = > + BIT_MASK(BTN_TOOL_FINGER) }, > + .absbit = { BIT_MASK(ABS_X) | BIT_MASK(ABS_Y) | > + BIT_MASK(ABS_PRESSURE) | > + BIT_MASK(ABS_TOOL_WIDTH) }, > + }, > + { } > +}; > + > +static int ideapad_tpswitch_init(struct ideapad_private *priv) > +{ > + int error; > + > + if (!priv->features.ctrl_ps2_aux_port) Nit: the comment above ctrl_ps2_aux_port and the MODULE_PARAM_DESC should be altered, because it no longer disables PS/2 AUX, but just filters the events on software level. Not sure whether we want to keep the old name for the module parameter. I think it's better to keep it, because it essentially serves the same purpose, but the implementation is better. > + return 0; > + > + spin_lock_init(&priv->tp_switch.lock); > + > + priv->tp_switch.handler.name = "ideapad-tpswitch"; > + priv->tp_switch.handler.id_table = ideapad_tpswitch_ids; > + priv->tp_switch.handler.filter = ideapad_tpswitch_filter; > + priv->tp_switch.handler.connect = ideapad_tpswitch_connect; > + priv->tp_switch.handler.disconnect = ideapad_tpswitch_disconnect; > + > + error = input_register_handler(&priv->tp_switch.handler); > + if (error) { > + dev_err(&priv->platform_device->dev, > + "failed to register touchpad switch handler: %d", > + error); > + return error; > + } > + > + priv->tp_switch.initialized = true; > + return 0; > +} > + > +static void ideapad_tpswitch_exit(struct ideapad_private *priv) > +{ > + if (priv->tp_switch.initialized) { > + input_unregister_handler(&priv->tp_switch.handler); > + priv->tp_switch.initialized = false; > + } > +} > + > +static void ideapad_tpswitch_toggle(struct ideapad_private *priv, bool on) > +{ > + guard(spinlock_irq)(&priv->tp_switch.lock); > + > + priv->tp_switch.active = on; > + if (on) { > + struct input_dev *tp_dev = priv->tp_switch.tp_dev; > + if (tp_dev) { > + input_report_key(tp_dev, BTN_TOUCH, 0); > + input_report_key(tp_dev, BTN_TOOL_FINGER, 0); > + input_report_key(tp_dev, BTN_TOOL_DOUBLETAP, 0); > + input_report_key(tp_dev, BTN_TOOL_TRIPLETAP, 0); > + input_report_key(tp_dev, BTN_LEFT, 0); > + input_report_key(tp_dev, BTN_RIGHT, 0); > + input_report_key(tp_dev, BTN_MIDDLE, 0); > + input_sync(tp_dev); > + } > + } > +} > + > /* > * backlight > */ > @@ -1567,7 +1725,6 @@ static void ideapad_fn_lock_led_exit(struct ideapad_private *priv) > static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_events) > { > unsigned long value; > - unsigned char param; > int ret; > > /* Without reading from EC touchpad LED doesn't switch state */ > @@ -1582,7 +1739,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > * KEY_TOUCHPAD_ON to not to get out of sync with LED > */ > if (priv->features.ctrl_ps2_aux_port) > - i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > + ideapad_tpswitch_toggle(priv, value); > > /* > * On older models the EC controls the touchpad and toggles it on/off > @@ -1927,6 +2084,10 @@ static int ideapad_acpi_add(struct platform_device *pdev) > if (err) > goto input_failed; > > + err = ideapad_tpswitch_init(priv); > + if (err) > + goto tpswitch_failed; > + > err = ideapad_kbd_bl_init(priv); > if (err) { > if (err != -ENODEV) > @@ -2001,6 +2162,9 @@ static int ideapad_acpi_add(struct platform_device *pdev) > > ideapad_fn_lock_led_exit(priv); > ideapad_kbd_bl_exit(priv); > + ideapad_tpswitch_exit(priv); > + > +tpswitch_failed: > ideapad_input_exit(priv); > > input_failed: > @@ -2029,6 +2193,7 @@ static void ideapad_acpi_remove(struct platform_device *pdev) > > ideapad_fn_lock_led_exit(priv); > ideapad_kbd_bl_exit(priv); > + ideapad_tpswitch_exit(priv); > ideapad_input_exit(priv); > ideapad_debugfs_exit(priv); > ideapad_sysfs_exit(priv);
On Tue, 20 Aug 2024 at 13:46:53 +0300, Maxim Mikityanskiy wrote: > On Sun, 18 Aug 2024 at 13:30:37 -0700, Dmitry Torokhov wrote: > > On Mon, Aug 12, 2024 at 12:26:47PM -0700, Dmitry Torokhov wrote: > > > On Mon, Aug 12, 2024 at 08:18:24PM +0200, Hans de Goede wrote: > > > > Hi Dmitry, > > > > > > > > On 8/12/24 7:24 PM, Dmitry Torokhov wrote: > > > > > On Mon, Aug 12, 2024 at 04:41:50PM +0200, Hans de Goede wrote: > > > > >> Hi Maxim, > > > > >> > > > > >> On 8/12/24 4:37 PM, Maxim Mikityanskiy wrote: > > > > >>> On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote: > > > > >>>> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote: > > > > >>>>> That means, userspace is not filtering out events upon receiving > > > > >>>>> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send > > > > >>>>> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570 > > > > >>>>> is weird. It maintains the touchpad state in firmware to light up the > > > > >>>>> status LED, but the firmware doesn't do the actual touchpad disablement. > > > > >>>>> > > > > >>>>> That is, if we use TOGGLE, the LED will get out of sync. If we use > > > > >>>>> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel. > > > > >>>> > > > > >>>> Ack. > > > > >>>> > > > > >>>> So how about this instead: > > > > >>>> > > > > >>>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > > > > >>>> index 1ace711f7442..b7fa06f793cb 100644 > > > > >>>> --- a/drivers/platform/x86/ideapad-laptop.c > > > > >>>> +++ b/drivers/platform/x86/ideapad-laptop.c > > > > >>>> @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > > > > >>>> * touchpad off and on. We send KEY_TOUCHPAD_OFF and > > > > >>>> * KEY_TOUCHPAD_ON to not to get out of sync with LED > > > > >>>> */ > > > > >>>> - if (priv->features.ctrl_ps2_aux_port) > > > > >>>> + if (send_events && priv->features.ctrl_ps2_aux_port) > > > > >>>> i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > > > > >>>> > > > > >>>> /* > > > > >>>> > > > > >>>> Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume > > > > >>>> correctly reflects the state before suspend/resume in both touchpad on / off states ? > > > > >>> > > > > >>> *Maxim > > > > >> > > > > >> Oops, sorry. > > > > >> > > > > >>> Just a heads-up, my Z570 now belongs to a family member, we'll test what > > > > >>> you asked, but right now there is a btrfs corruption on that laptop that > > > > >>> we need to fix first, it interferes with kernel compilation =/ > > > > >> > > > > >> Note as discussed in another part of the thread the original bug report > > > > >> actually was not on a Z570, so the whole usage of i8042_command() on > > > > >> suspend/resume was a bit of a red herring. And the suspend/resume issue > > > > >> has been fixed in another way in the mean time. > > > > >> > > > > >> So there really is no need to test this change anymore. At the moment > > > > >> there are no planned changes to ideapad-laptop related to this. > > > > > > > > > > I think we still need to stop ideapad-laptop poking into 8042, > > > > > especially ahead of time. > > > > > > > > I agree. I think your suggestion of using the new(ish) [un]inhibit > > > > support in the input subsystem for this instead of poking at the i8042 > > > > is a good idea. > > > > > > > > As I mentioned when you first suggested this, I guess this requires 2 things: > > > > > > > > 1. Some helper to find the struct input_dev for the input_dev related > > > > to the ps/2 aux port > > > > 2. In kernel API / functions to do inhibit/uninhibit > > > > (maybe these already exist?) > > > > > > > > > If we do not want to wait for userspace to > > > > > handle this properly, I wonder if we could not create an > > > > > input_handler that would attach to the touchpad device and filter out > > > > > all events coming from the touchpad if touchpad is supposed to be off. > > > > > > > > I think using the inhibit stuff would be better no? > > > > > > The issue with inhibit/uninhibit is that they are only exposed to > > > userpsace via sysfs. And as you mentioned we need to locate the input > > > device corresponding to the touchpad. > > > > > > With input handler we are essentially getting both - psmouse does not do > > > anything special in inhibit so it is just the input core dropping > > > events, the same as with the filter handler, and we can use hanlder's > > > match table to limit it to the touchpad and input core will find the > > > device for us. > > > > > > > > > > > The biggest problems with trying to fix this are: > > > > > > > > 1. Finding time to work on this > > > > 2. Finding someone willing to test the patches > > > > > > > > Finding the time is going to be an issue for me since the i8042_command() > > > > calls are only still done on a single model laptop (using a DMI quirk) > > > > inside ideapad-laptop now, so this is pretty low priority IMHO. Which > > > > in practice means that I will simply never get around to this, sorry... > > > > > > Yeah, I can see that ;) Maybe I will find a couple of hours to waste... > > > > Maybe something like below can work? > > Great patch, thank you, I'll test it and report the results. See some > minor comments below. > > > > > > > platform/x86: ideapad-laptop: do not poke keyboard controller > > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > On Ideapad Z570 the driver tries to disable and reenable data coming > > from the touchpad by poking directly into 8042 keyboard controller. > > This may coincide with the controller resuming and leads to spews in > > dmesg and potentially other instabilities. > > > > Instead of using i8042_command() to control the touchpad state create a > > input handler that serves as a filter and drop events coming from the > > touchpad when it is supposed to be off. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/platform/x86/ideapad-laptop.c | 171 ++++++++++++++++++++++++++++++++- > > 1 file changed, 168 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > > index fcf13d88fd6e..2f40feefd5e3 100644 > > --- a/drivers/platform/x86/ideapad-laptop.c > > +++ b/drivers/platform/x86/ideapad-laptop.c > > @@ -17,7 +17,6 @@ > > #include <linux/device.h> > > #include <linux/dmi.h> > > #include <linux/fb.h> > > -#include <linux/i8042.h> > > #include <linux/init.h> > > #include <linux/input.h> > > #include <linux/input/sparse-keymap.h> > > @@ -157,6 +156,13 @@ struct ideapad_private { > > struct led_classdev led; > > unsigned int last_brightness; > > } fn_lock; > > + struct { > > + bool initialized; > > + bool active; > > + struct input_handler handler; > > + struct input_dev *tp_dev; > > + spinlock_t lock; > > + } tp_switch; > > }; > > > > static bool no_bt_rfkill; > > @@ -1236,6 +1242,158 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv) > > } > > } > > > > +struct ideapad_tpswitch_handle { > > + struct input_handle handle; > > + struct ideapad_private *priv; > > +}; > > + > > +#define to_tpswitch_handle(h) \ > > + container_of(h, struct ideapad_tpswitch_handle, handle); > > + > > +static int ideapad_tpswitch_connect(struct input_handler *handler, > > + struct input_dev *dev, > > + const struct input_device_id *id) > > +{ > > + struct ideapad_private *priv = > > + container_of(handler, struct ideapad_private, tp_switch.handler); > > + struct ideapad_tpswitch_handle *h; > > + int error; > > + > > + h = kzalloc(sizeof(*h), GFP_KERNEL); > > + if (!h) > > + return -ENOMEM; > > + > > + h->priv = priv; > > + h->handle.dev = dev; > > + h->handle.handler = handler; > > + h->handle.name = "ideapad-tpswitch"; > > + > > + error = input_register_handle(&h->handle); > > + if (error) > > + goto err_free_handle; > > + > > + /* > > + * FIXME: ideally we do not want to open the input device here > > + * if there are no other users. We need a notion of "observer" > > + * handlers in the input core. > > + */ > > + error = input_open_device(&h->handle); > > + if (error) > > + goto err_unregister_handle; > > + > > + scoped_guard(spinlock_irq, &priv->tp_switch.lock) > > + priv->tp_switch.tp_dev = dev; > > + > > + return 0; > > + > > + err_unregister_handle: > > + input_unregister_handle(&h->handle); > > +err_free_handle: > > + kfree(h); > > + return error; > > +} > > + > > +static void ideapad_tpswitch_disconnect(struct input_handle *handle) > > +{ > > + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); > > + struct ideapad_private *priv = h->priv; > > + > > + scoped_guard(spinlock_irq, &priv->tp_switch.lock) > > Nice syntax, I didn't know about it before. > > > + priv->tp_switch.tp_dev = NULL; > > + > > + input_close_device(handle); > > + input_unregister_handle(handle); > > + kfree(h); > > +} > > + > > +static bool ideapad_tpswitch_filter(struct input_handle *handle, > > + unsigned int type, unsigned int code, > > + int value) > > +{ > > + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); > > + struct ideapad_private *priv = h->priv; > > + > > + if (!priv->tp_switch.active) > > This check seems inverted. ideapad_tpswitch_toggle assigns true when the > touchpad is enabled. I tested the patch on Z570 (with this check inverted), and it seems to work great. Also tested what happens on resume from suspend: the laptop reenables the touchpad (the LED turns off on suspend and blinks briefly on resume), and the driver handles it properly. > > > + return false; > > + > > + /* Allow passing button release events, drop everything else */ > > + return !(type == EV_KEY && value == 0) && > > + !(type == EV_SYN && code == SYN_REPORT); > > + > > +} > > + > > +static const struct input_device_id ideapad_tpswitch_ids[] = { > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > + INPUT_DEVICE_ID_MATCH_KEYBIT | > > + INPUT_DEVICE_ID_MATCH_ABSBIT, > > + .bustype = BUS_I8042, > > + .vendor = 0x0002, > > + .evbit = { BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS) }, > > + .keybit = { [BIT_WORD(BTN_TOOL_FINGER)] = > > + BIT_MASK(BTN_TOOL_FINGER) }, > > + .absbit = { BIT_MASK(ABS_X) | BIT_MASK(ABS_Y) | > > + BIT_MASK(ABS_PRESSURE) | > > + BIT_MASK(ABS_TOOL_WIDTH) }, > > + }, > > + { } > > +}; > > + > > +static int ideapad_tpswitch_init(struct ideapad_private *priv) > > +{ > > + int error; > > + > > + if (!priv->features.ctrl_ps2_aux_port) > > Nit: the comment above ctrl_ps2_aux_port and the MODULE_PARAM_DESC > should be altered, because it no longer disables PS/2 AUX, but just > filters the events on software level. > > Not sure whether we want to keep the old name for the module parameter. > I think it's better to keep it, because it essentially serves the same > purpose, but the implementation is better. > > > + return 0; > > + > > + spin_lock_init(&priv->tp_switch.lock); > > + > > + priv->tp_switch.handler.name = "ideapad-tpswitch"; > > + priv->tp_switch.handler.id_table = ideapad_tpswitch_ids; > > + priv->tp_switch.handler.filter = ideapad_tpswitch_filter; > > + priv->tp_switch.handler.connect = ideapad_tpswitch_connect; > > + priv->tp_switch.handler.disconnect = ideapad_tpswitch_disconnect; > > + > > + error = input_register_handler(&priv->tp_switch.handler); > > + if (error) { > > + dev_err(&priv->platform_device->dev, > > + "failed to register touchpad switch handler: %d", > > + error); > > + return error; > > + } > > + > > + priv->tp_switch.initialized = true; > > + return 0; > > +} > > + > > +static void ideapad_tpswitch_exit(struct ideapad_private *priv) > > +{ > > + if (priv->tp_switch.initialized) { > > + input_unregister_handler(&priv->tp_switch.handler); > > + priv->tp_switch.initialized = false; > > + } > > +} > > + > > +static void ideapad_tpswitch_toggle(struct ideapad_private *priv, bool on) > > +{ > > + guard(spinlock_irq)(&priv->tp_switch.lock); > > + > > + priv->tp_switch.active = on; > > + if (on) { > > + struct input_dev *tp_dev = priv->tp_switch.tp_dev; > > + if (tp_dev) { > > + input_report_key(tp_dev, BTN_TOUCH, 0); > > + input_report_key(tp_dev, BTN_TOOL_FINGER, 0); > > + input_report_key(tp_dev, BTN_TOOL_DOUBLETAP, 0); > > + input_report_key(tp_dev, BTN_TOOL_TRIPLETAP, 0); > > + input_report_key(tp_dev, BTN_LEFT, 0); > > + input_report_key(tp_dev, BTN_RIGHT, 0); > > + input_report_key(tp_dev, BTN_MIDDLE, 0); > > + input_sync(tp_dev); > > + } > > + } > > +} > > + > > /* > > * backlight > > */ > > @@ -1567,7 +1725,6 @@ static void ideapad_fn_lock_led_exit(struct ideapad_private *priv) > > static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_events) > > { > > unsigned long value; > > - unsigned char param; > > int ret; > > > > /* Without reading from EC touchpad LED doesn't switch state */ > > @@ -1582,7 +1739,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ > > * KEY_TOUCHPAD_ON to not to get out of sync with LED > > */ > > if (priv->features.ctrl_ps2_aux_port) > > - i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > > + ideapad_tpswitch_toggle(priv, value); > > > > /* > > * On older models the EC controls the touchpad and toggles it on/off > > @@ -1927,6 +2084,10 @@ static int ideapad_acpi_add(struct platform_device *pdev) > > if (err) > > goto input_failed; > > > > + err = ideapad_tpswitch_init(priv); > > + if (err) > > + goto tpswitch_failed; > > + > > err = ideapad_kbd_bl_init(priv); > > if (err) { > > if (err != -ENODEV) > > @@ -2001,6 +2162,9 @@ static int ideapad_acpi_add(struct platform_device *pdev) > > > > ideapad_fn_lock_led_exit(priv); > > ideapad_kbd_bl_exit(priv); > > + ideapad_tpswitch_exit(priv); > > + > > +tpswitch_failed: > > ideapad_input_exit(priv); > > > > input_failed: > > @@ -2029,6 +2193,7 @@ static void ideapad_acpi_remove(struct platform_device *pdev) > > > > ideapad_fn_lock_led_exit(priv); > > ideapad_kbd_bl_exit(priv); > > + ideapad_tpswitch_exit(priv); > > ideapad_input_exit(priv); > > ideapad_debugfs_exit(priv); > > ideapad_sysfs_exit(priv);
On Wed, Aug 21, 2024 at 12:40:34AM +0300, Maxim Mikityanskiy wrote: > On Tue, 20 Aug 2024 at 13:46:53 +0300, Maxim Mikityanskiy wrote: > > On Sun, 18 Aug 2024 at 13:30:37 -0700, Dmitry Torokhov wrote: > > > > > > Maybe something like below can work? > > > > Great patch, thank you, I'll test it and report the results. See some > > minor comments below. > > > > > > > > > > > platform/x86: ideapad-laptop: do not poke keyboard controller > > > > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > > > On Ideapad Z570 the driver tries to disable and reenable data coming > > > from the touchpad by poking directly into 8042 keyboard controller. > > > This may coincide with the controller resuming and leads to spews in > > > dmesg and potentially other instabilities. > > > > > > Instead of using i8042_command() to control the touchpad state create a > > > input handler that serves as a filter and drop events coming from the > > > touchpad when it is supposed to be off. > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > --- > > > drivers/platform/x86/ideapad-laptop.c | 171 ++++++++++++++++++++++++++++++++- > > > 1 file changed, 168 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > > > index fcf13d88fd6e..2f40feefd5e3 100644 > > > --- a/drivers/platform/x86/ideapad-laptop.c > > > +++ b/drivers/platform/x86/ideapad-laptop.c > > > @@ -17,7 +17,6 @@ > > > #include <linux/device.h> > > > #include <linux/dmi.h> > > > #include <linux/fb.h> > > > -#include <linux/i8042.h> > > > #include <linux/init.h> > > > #include <linux/input.h> > > > #include <linux/input/sparse-keymap.h> > > > @@ -157,6 +156,13 @@ struct ideapad_private { > > > struct led_classdev led; > > > unsigned int last_brightness; > > > } fn_lock; > > > + struct { > > > + bool initialized; > > > + bool active; > > > + struct input_handler handler; > > > + struct input_dev *tp_dev; > > > + spinlock_t lock; > > > + } tp_switch; > > > }; > > > > > > static bool no_bt_rfkill; > > > @@ -1236,6 +1242,158 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv) > > > } > > > } > > > > > > +struct ideapad_tpswitch_handle { > > > + struct input_handle handle; > > > + struct ideapad_private *priv; > > > +}; > > > + > > > +#define to_tpswitch_handle(h) \ > > > + container_of(h, struct ideapad_tpswitch_handle, handle); > > > + > > > +static int ideapad_tpswitch_connect(struct input_handler *handler, > > > + struct input_dev *dev, > > > + const struct input_device_id *id) > > > +{ > > > + struct ideapad_private *priv = > > > + container_of(handler, struct ideapad_private, tp_switch.handler); > > > + struct ideapad_tpswitch_handle *h; > > > + int error; > > > + > > > + h = kzalloc(sizeof(*h), GFP_KERNEL); > > > + if (!h) > > > + return -ENOMEM; > > > + > > > + h->priv = priv; > > > + h->handle.dev = dev; > > > + h->handle.handler = handler; > > > + h->handle.name = "ideapad-tpswitch"; > > > + > > > + error = input_register_handle(&h->handle); > > > + if (error) > > > + goto err_free_handle; > > > + > > > + /* > > > + * FIXME: ideally we do not want to open the input device here > > > + * if there are no other users. We need a notion of "observer" > > > + * handlers in the input core. > > > + */ > > > + error = input_open_device(&h->handle); > > > + if (error) > > > + goto err_unregister_handle; > > > + > > > + scoped_guard(spinlock_irq, &priv->tp_switch.lock) > > > + priv->tp_switch.tp_dev = dev; > > > + > > > + return 0; > > > + > > > + err_unregister_handle: > > > + input_unregister_handle(&h->handle); > > > +err_free_handle: > > > + kfree(h); > > > + return error; > > > +} > > > + > > > +static void ideapad_tpswitch_disconnect(struct input_handle *handle) > > > +{ > > > + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); > > > + struct ideapad_private *priv = h->priv; > > > + > > > + scoped_guard(spinlock_irq, &priv->tp_switch.lock) > > > > Nice syntax, I didn't know about it before. > > > > > + priv->tp_switch.tp_dev = NULL; > > > + > > > + input_close_device(handle); > > > + input_unregister_handle(handle); > > > + kfree(h); > > > +} > > > + > > > +static bool ideapad_tpswitch_filter(struct input_handle *handle, > > > + unsigned int type, unsigned int code, > > > + int value) > > > +{ > > > + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); > > > + struct ideapad_private *priv = h->priv; > > > + > > > + if (!priv->tp_switch.active) > > > > This check seems inverted. ideapad_tpswitch_toggle assigns true when the > > touchpad is enabled. > > I tested the patch on Z570 (with this check inverted), and it seems to > work great. > > Also tested what happens on resume from suspend: the laptop reenables > the touchpad (the LED turns off on suspend and blinks briefly on > resume), and the driver handles it properly. Great, thank you! Give me a couple of days and I think I will implement observer/passive handler support and we can figure out how to merge this...
On Tue, Aug 20, 2024 at 10:28:24PM -0700, Dmitry Torokhov wrote: > On Wed, Aug 21, 2024 at 12:40:34AM +0300, Maxim Mikityanskiy wrote: > > On Tue, 20 Aug 2024 at 13:46:53 +0300, Maxim Mikityanskiy wrote: > > > On Sun, 18 Aug 2024 at 13:30:37 -0700, Dmitry Torokhov wrote: > > > > > > > > Maybe something like below can work? > > > > > > Great patch, thank you, I'll test it and report the results. See some > > > minor comments below. > > > > > > > > > > > > > > > platform/x86: ideapad-laptop: do not poke keyboard controller > > > > > > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > > > > > On Ideapad Z570 the driver tries to disable and reenable data coming > > > > from the touchpad by poking directly into 8042 keyboard controller. > > > > This may coincide with the controller resuming and leads to spews in > > > > dmesg and potentially other instabilities. > > > > > > > > Instead of using i8042_command() to control the touchpad state create a > > > > input handler that serves as a filter and drop events coming from the > > > > touchpad when it is supposed to be off. > > > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > --- > > > > drivers/platform/x86/ideapad-laptop.c | 171 ++++++++++++++++++++++++++++++++- > > > > 1 file changed, 168 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > > > > index fcf13d88fd6e..2f40feefd5e3 100644 > > > > --- a/drivers/platform/x86/ideapad-laptop.c > > > > +++ b/drivers/platform/x86/ideapad-laptop.c > > > > @@ -17,7 +17,6 @@ > > > > #include <linux/device.h> > > > > #include <linux/dmi.h> > > > > #include <linux/fb.h> > > > > -#include <linux/i8042.h> > > > > #include <linux/init.h> > > > > #include <linux/input.h> > > > > #include <linux/input/sparse-keymap.h> > > > > @@ -157,6 +156,13 @@ struct ideapad_private { > > > > struct led_classdev led; > > > > unsigned int last_brightness; > > > > } fn_lock; > > > > + struct { > > > > + bool initialized; > > > > + bool active; > > > > + struct input_handler handler; > > > > + struct input_dev *tp_dev; > > > > + spinlock_t lock; > > > > + } tp_switch; > > > > }; > > > > > > > > static bool no_bt_rfkill; > > > > @@ -1236,6 +1242,158 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv) > > > > } > > > > } > > > > > > > > +struct ideapad_tpswitch_handle { > > > > + struct input_handle handle; > > > > + struct ideapad_private *priv; > > > > +}; > > > > + > > > > +#define to_tpswitch_handle(h) \ > > > > + container_of(h, struct ideapad_tpswitch_handle, handle); > > > > + > > > > +static int ideapad_tpswitch_connect(struct input_handler *handler, > > > > + struct input_dev *dev, > > > > + const struct input_device_id *id) > > > > +{ > > > > + struct ideapad_private *priv = > > > > + container_of(handler, struct ideapad_private, tp_switch.handler); > > > > + struct ideapad_tpswitch_handle *h; > > > > + int error; > > > > + > > > > + h = kzalloc(sizeof(*h), GFP_KERNEL); > > > > + if (!h) > > > > + return -ENOMEM; > > > > + > > > > + h->priv = priv; > > > > + h->handle.dev = dev; > > > > + h->handle.handler = handler; > > > > + h->handle.name = "ideapad-tpswitch"; > > > > + > > > > + error = input_register_handle(&h->handle); > > > > + if (error) > > > > + goto err_free_handle; > > > > + > > > > + /* > > > > + * FIXME: ideally we do not want to open the input device here > > > > + * if there are no other users. We need a notion of "observer" > > > > + * handlers in the input core. > > > > + */ > > > > + error = input_open_device(&h->handle); > > > > + if (error) > > > > + goto err_unregister_handle; > > > > + > > > > + scoped_guard(spinlock_irq, &priv->tp_switch.lock) > > > > + priv->tp_switch.tp_dev = dev; > > > > + > > > > + return 0; > > > > + > > > > + err_unregister_handle: > > > > + input_unregister_handle(&h->handle); > > > > +err_free_handle: > > > > + kfree(h); > > > > + return error; > > > > +} > > > > + > > > > +static void ideapad_tpswitch_disconnect(struct input_handle *handle) > > > > +{ > > > > + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); > > > > + struct ideapad_private *priv = h->priv; > > > > + > > > > + scoped_guard(spinlock_irq, &priv->tp_switch.lock) > > > > > > Nice syntax, I didn't know about it before. > > > > > > > + priv->tp_switch.tp_dev = NULL; > > > > + > > > > + input_close_device(handle); > > > > + input_unregister_handle(handle); > > > > + kfree(h); > > > > +} > > > > + > > > > +static bool ideapad_tpswitch_filter(struct input_handle *handle, > > > > + unsigned int type, unsigned int code, > > > > + int value) > > > > +{ > > > > + struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle); > > > > + struct ideapad_private *priv = h->priv; > > > > + > > > > + if (!priv->tp_switch.active) > > > > > > This check seems inverted. ideapad_tpswitch_toggle assigns true when the > > > touchpad is enabled. > > > > I tested the patch on Z570 (with this check inverted), and it seems to > > work great. > > > > Also tested what happens on resume from suspend: the laptop reenables > > the touchpad (the LED turns off on suspend and blinks briefly on > > resume), and the driver handles it properly. > > Great, thank you! Give me a couple of days and I think I will implement > observer/passive handler support and we can figure out how to merge > this... OK, so if you could try the patch below that would be great. Don't forget to set ".passive_observer = 1" in the ideapad handler. Thanks!
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index 1ace711f7442..255fb56ec9ee 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -18,7 +18,6 @@ #include <linux/device.h> #include <linux/dmi.h> #include <linux/fb.h> -#include <linux/i8042.h> #include <linux/init.h> #include <linux/input.h> #include <linux/input/sparse-keymap.h> @@ -144,7 +143,6 @@ struct ideapad_private { bool hw_rfkill_switch : 1; bool kbd_bl : 1; bool touchpad_ctrl_via_ec : 1; - bool ctrl_ps2_aux_port : 1; bool usb_charging : 1; } features; struct { @@ -182,12 +180,6 @@ MODULE_PARM_DESC(set_fn_lock_led, "Enable driver based updates of the fn-lock LED on fn-lock changes. " "If you need this please report this to: platform-driver-x86@vger.kernel.org"); -static bool ctrl_ps2_aux_port; -module_param(ctrl_ps2_aux_port, bool, 0444); -MODULE_PARM_DESC(ctrl_ps2_aux_port, - "Enable driver based PS/2 aux port en-/dis-abling on touchpad on/off toggle. " - "If you need this please report this to: platform-driver-x86@vger.kernel.org"); - static bool touchpad_ctrl_via_ec; module_param(touchpad_ctrl_via_ec, bool, 0444); MODULE_PARM_DESC(touchpad_ctrl_via_ec, @@ -1560,7 +1552,6 @@ static void ideapad_fn_lock_led_exit(struct ideapad_private *priv) static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_events) { unsigned long value; - unsigned char param; int ret; /* Without reading from EC touchpad LED doesn't switch state */ @@ -1568,15 +1559,6 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_ if (ret) return; - /* - * Some IdeaPads don't really turn off touchpad - they only - * switch the LED state. We (de)activate KBC AUX port to turn - * touchpad off and on. We send KEY_TOUCHPAD_OFF and - * KEY_TOUCHPAD_ON to not to get out of sync with LED - */ - if (priv->features.ctrl_ps2_aux_port) - i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); - /* * On older models the EC controls the touchpad and toggles it on/off * itself, in this case we report KEY_TOUCHPAD_ON/_OFF. Some models do @@ -1699,23 +1681,6 @@ static const struct dmi_system_id hw_rfkill_list[] = { {} }; -/* - * On some models the EC toggles the touchpad muted LED on touchpad toggle - * hotkey presses, but the EC does not actually disable the touchpad itself. - * On these models the driver needs to explicitly enable/disable the i8042 - * (PS/2) aux port. - */ -static const struct dmi_system_id ctrl_ps2_aux_port_list[] = { - { - /* Lenovo Ideapad Z570 */ - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), - DMI_MATCH(DMI_PRODUCT_VERSION, "Ideapad Z570"), - }, - }, - {} -}; - static void ideapad_check_features(struct ideapad_private *priv) { acpi_handle handle = priv->adev->handle; @@ -1725,8 +1690,6 @@ static void ideapad_check_features(struct ideapad_private *priv) set_fn_lock_led || dmi_check_system(set_fn_lock_led_list); priv->features.hw_rfkill_switch = hw_rfkill_switch || dmi_check_system(hw_rfkill_list); - priv->features.ctrl_ps2_aux_port = - ctrl_ps2_aux_port || dmi_check_system(ctrl_ps2_aux_port_list); priv->features.touchpad_ctrl_via_ec = touchpad_ctrl_via_ec; if (!read_ec_data(handle, VPCCMD_R_FAN, &val))
Commit 07a4a4fc83dd ("ideapad: add Lenovo IdeaPad Z570 support (part 2)") added an i8042_command(..., I8042_CMD_AUX_[EN|DIS]ABLE) call to the ideapad-laptop driver to suppress the touchpad events at the PS/2 AUX controller level. Commit c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux port on/off on select models") limited this to only do this by default on the IdeaPad Z570 to replace a growing list of models on which the i8042_command() call was disabled by quirks because it was causing issues. A recent report shows that this is causing issues even on the Z570 for which it was originally added because it can happen on resume before the i8042 controller's own resume() method has run: [ 50.241235] ideapad_acpi VPC2004:00: PM: calling acpi_subsys_resume+0x0/0x5d @ 4492, parent: PNP0C09:00 [ 50.242055] snd_hda_intel 0000:00:0e.0: PM: pci_pm_resume+0x0/0xed returned 0 after 13511 usecs [ 50.242120] snd_hda_codec_realtek hdaudioC0D0: PM: calling hda_codec_pm_resume+0x0/0x19 [snd_hda_codec] @ 4518, parent: 0000:00:0e.0 [ 50.247406] i8042: [49434] a8 -> i8042 (command) [ 50.247468] ideapad_acpi VPC2004:00: PM: acpi_subsys_resume+0x0/0x5d returned 0 after 6220 usecs ... [ 50.247883] i8042 kbd 00:01: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 [ 50.247894] i8042 kbd 00:01: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs [ 50.247906] i8042 aux 00:02: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0 [ 50.247916] i8042 aux 00:02: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs ... [ 50.248301] i8042 i8042: PM: calling platform_pm_resume+0x0/0x41 @ 4492, parent: platform [ 50.248377] i8042: [49434] 55 <- i8042 (flush, kbd) [ 50.248407] i8042: [49435] aa -> i8042 (command) [ 50.248601] i8042: [49435] 00 <- i8042 (return) [ 50.248604] i8042: [49435] i8042 controller selftest: 0x0 != 0x55 Dmitry (input subsys maintainer) pointed out that just sending KEY_TOUCHPAD_OFF/KEY_TOUCHPAD_ON which the ideapad-laptop driver already does should be sufficient and that it then is up to userspace to filter out touchpad events after having received a KEY_TOUCHPAD_OFF. Given all the problems the i8042_command() call has been causing just removing it indeed seems best, so this removes it completely. Note that this only impacts the Ideapad Z570 since it was already disabled by default on all other models. Fixes: c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux port on/off on select models") Reported-by: Jonathan Denose <jdenose@chromium.org> Closes: https://lore.kernel.org/linux-input/20231102075243.1.Idb37ff8043a29f607beab6440c32b9ae52525825@changeid/ Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Maxim Mikityanskiy <maxtram95@gmail.com> Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/ideapad-laptop.c | 37 --------------------------- 1 file changed, 37 deletions(-)