| Message ID | 20230414092353.v2.1.Ieb687047a5b75c7b7ee5dd258207ef5ca9a3b728@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Input: synaptics - disable intertouch for Lenovo L440 | expand |
Hi Lyude and Jonathan, I was just about to reply and suggest that we look into this issue a little more since the touchpad in the L440 would benefit from the additional data from the intertouch interface. Especially, since it has a large area and several buttons. PS/2 only reports position data for two fingers so three finger gestures is another example. Generally, these types of suspend / resume issues are the result of the touchpad resetting and the firmware expecting commands from the PS/2 interface. On resume, the PS/2 driver should send a command over the PS/2 interface to switch the touchpad firmware back into intertouch (SMBus) mode. The logs you provided look like that's what is happening here. The SMBus driver is sending commands, but the touchpad firmware won't respond until it is switch back into intertouch mode. It has been a while since I have worked on these touchpads, but from what I remember I think there is code in the psmouse-smbus driver to handle these situations. I added Benjamin Tissoires to CC since I think he worked on that handling. I thought suspend / resume was tested on with these "top button" touchpads when support for them was added. I don't know if the L440 specifically included in the testing. I'm curious if this is a regression or not. Regarding the patch, I do have one comment below: > On Apr 17, 2023, at 11:52, Jonathan Denose <jdenose@chromium.org> wrote: > > CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe. > > > Sorry, I thought I sent this as plain text but I think maybe not. > Trying once more, the message was: > > I think that disabling synaptics_intertouch would resolve the issue > mentioned in the commit, but cause a regression in the functionality > that intertouch is supposed to bring, like three-finger gestures. I'm > attaching some of the logs that I captured when the touchpad was > failing on resume. I think the main culprit is > i2c_smbus_read_byte_data where the driver is unable to get the SMBus > version number. I get the following lines in dmesg: > > [ 2869.745860] rmi4_smbus 0-002c: failed to get SMBus version number! > [ 2869.746060] rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed > to read current IRQ mask. > [ 2869.746260] rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6. > [ 2869.746262] rmi4_f01 rmi4-00.fn01: Resume failed with code -6. > [ 2869.746264] rmi4_physical rmi4-00: Failed to suspend functions: -6 > [ 2869.746265] rmi4_smbus 0-002c: Failed to resume device: -6 > [ 2869.746268] rmi4_smbus 0-002c: rmi_smb_resume+0x0/0x6b [rmi_smbus] > returned 0 after 549 usecs > [ 2869.746446] rmi4_physical rmi4-00: Failed to read irqs, code=-6 > > Any ideas on what might be causing this, only on resume from deep sleep? > > > On Mon, Apr 17, 2023 at 1:47 PM Jonathan Denose <jdenose@chromium.org> wrote: >> >> I think that disabling synaptics_intertouch would resolve the issue mentioned in the commit, but cause a regression in the functionality that intertouch is supposed to bring, like three-finger gestures. I'm attaching some of the logs that I captured when the touchpad was failing on resume. I think the main culprit is i2c_smbus_read_byte_data where the driver is unable to get the SMBus version number. I get the following lines in dmesg: >> >> [ 2869.745860] rmi4_smbus 0-002c: failed to get SMBus version number! >> [ 2869.746060] rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed to read current IRQ mask. >> [ 2869.746260] rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6. >> [ 2869.746262] rmi4_f01 rmi4-00.fn01: Resume failed with code -6. >> [ 2869.746264] rmi4_physical rmi4-00: Failed to suspend functions: -6 >> [ 2869.746265] rmi4_smbus 0-002c: Failed to resume device: -6 >> [ 2869.746268] rmi4_smbus 0-002c: rmi_smb_resume+0x0/0x6b [rmi_smbus] returned 0 after 549 usecs >> [ 2869.746446] rmi4_physical rmi4-00: Failed to read irqs, code=-6 >> >> Any ideas on what might be causing this, only on resume from deep sleep? >> >> >> On Fri, Apr 14, 2023 at 11:41 AM Jonathan Denose <jdenose@chromium.org> wrote: >>> >>> When intertouch is enabled for the L440 a (deep)sleep/resume >>> cycle causes the touchpad driver to hang which causes the >>> touchpad to become unresponsive. Disable intertouch resolves >>> this issue and the touchpad is fine after resume from sleep. >>> >>> Additionally, when the PNP id for the L440 is only removed >>> from the topbuttonpad_pnp_ids list, a message is logged to >>> enable psmouse.synaptics_intertouch, which would cause the >>> sleep/resume issue again. By removing the PNP id from >>> topbutton_pnp_ids and then adding it to the >>> forcepad_pnp_ids array, intertouch is disabled and the >>> message is not logged. >>> >>> Signed-off-by: Jonathan Denose <jdenose@google.com> >>> --- >>> >>> Changes in v2: >>> - remove debug statement >>> >>> drivers/input/mouse/synaptics.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c >>> index fa021af8506e4..b7218b7652c20 100644 >>> --- a/drivers/input/mouse/synaptics.c >>> +++ b/drivers/input/mouse/synaptics.c >>> @@ -150,7 +150,6 @@ static const char * const topbuttonpad_pnp_ids[] = { >>> "LEN2001", /* Edge E431 */ >>> "LEN2002", /* Edge E531 */ >>> "LEN2003", >>> - "LEN2004", /* L440 */ >>> "LEN2005", >>> "LEN2006", /* Edge E440/E540 */ >>> "LEN2007", >>> @@ -198,6 +197,7 @@ static const char * const smbus_pnp_ids[] = { >>> static const char * const forcepad_pnp_ids[] = { >>> "SYN300D", >>> "SYN3014", >>> + "LEN2004", /* L440 */ While this does seem to elliminate the message, the touchpad in the L440 is not a forcepad. Adding the L440 PnP ID here implies that it is one of these special forcepads which reports "force" data for contacts and that is not the case here. >>> NULL >>> }; >>> >>> — >>> 2.39.2 >>> Andrew
Hi Andrew, Thanks for your reply. As an update, I was able to try the patch here: https://lore.kernel.org/all/YgHTYrODoo2ou49J@google.com/ and it resolves the suspend/resume issue on this device. I'm not sure what the status of the linked patch is, to me it doesn't look like it was merged anywhere. On Mon, Apr 17, 2023 at 3:14 PM Andrew Duggan <andrew@duggan.us> wrote: > > Hi Lyude and Jonathan, > > I was just about to reply and suggest that we look into this issue a little more since the touchpad in the L440 would benefit from the additional data from the intertouch interface. Especially, since it has a large area and several buttons. PS/2 only reports position data for two fingers so three finger gestures is another example. > > Generally, these types of suspend / resume issues are the result of the touchpad resetting and the firmware expecting commands from the PS/2 interface. On resume, the PS/2 driver should send a command over the PS/2 interface to switch the touchpad firmware back into intertouch (SMBus) mode. The logs you provided look like that's what is happening here. The SMBus driver is sending commands, but the touchpad firmware won't respond until it is switch back into intertouch mode. It has been a while since I have worked on these touchpads, but from what I remember I think there is code in the psmouse-smbus driver to handle these situations. I added Benjamin Tissoires to CC since I think he worked on that handling. I thought suspend / resume was tested on with these "top button" touchpads when support for them was added. I don't know if the L440 specifically included in the testing. I'm curious if this is a regression or not. > > Regarding the patch, I do have one comment below: > > > On Apr 17, 2023, at 11:52, Jonathan Denose <jdenose@chromium.org> wrote: > > > > CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > > > Sorry, I thought I sent this as plain text but I think maybe not. > > Trying once more, the message was: > > > > I think that disabling synaptics_intertouch would resolve the issue > > mentioned in the commit, but cause a regression in the functionality > > that intertouch is supposed to bring, like three-finger gestures. I'm > > attaching some of the logs that I captured when the touchpad was > > failing on resume. I think the main culprit is > > i2c_smbus_read_byte_data where the driver is unable to get the SMBus > > version number. I get the following lines in dmesg: > > > > [ 2869.745860] rmi4_smbus 0-002c: failed to get SMBus version number! > > [ 2869.746060] rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed > > to read current IRQ mask. > > [ 2869.746260] rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6. > > [ 2869.746262] rmi4_f01 rmi4-00.fn01: Resume failed with code -6. > > [ 2869.746264] rmi4_physical rmi4-00: Failed to suspend functions: -6 > > [ 2869.746265] rmi4_smbus 0-002c: Failed to resume device: -6 > > [ 2869.746268] rmi4_smbus 0-002c: rmi_smb_resume+0x0/0x6b [rmi_smbus] > > returned 0 after 549 usecs > > [ 2869.746446] rmi4_physical rmi4-00: Failed to read irqs, code=-6 > > > > Any ideas on what might be causing this, only on resume from deep sleep? > > > > > > On Mon, Apr 17, 2023 at 1:47 PM Jonathan Denose <jdenose@chromium.org> wrote: > >> > >> I think that disabling synaptics_intertouch would resolve the issue mentioned in the commit, but cause a regression in the functionality that intertouch is supposed to bring, like three-finger gestures. I'm attaching some of the logs that I captured when the touchpad was failing on resume. I think the main culprit is i2c_smbus_read_byte_data where the driver is unable to get the SMBus version number. I get the following lines in dmesg: > >> > >> [ 2869.745860] rmi4_smbus 0-002c: failed to get SMBus version number! > >> [ 2869.746060] rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed to read current IRQ mask. > >> [ 2869.746260] rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6. > >> [ 2869.746262] rmi4_f01 rmi4-00.fn01: Resume failed with code -6. > >> [ 2869.746264] rmi4_physical rmi4-00: Failed to suspend functions: -6 > >> [ 2869.746265] rmi4_smbus 0-002c: Failed to resume device: -6 > >> [ 2869.746268] rmi4_smbus 0-002c: rmi_smb_resume+0x0/0x6b [rmi_smbus] returned 0 after 549 usecs > >> [ 2869.746446] rmi4_physical rmi4-00: Failed to read irqs, code=-6 > >> > >> Any ideas on what might be causing this, only on resume from deep sleep? > >> > >> > >> On Fri, Apr 14, 2023 at 11:41 AM Jonathan Denose <jdenose@chromium.org> wrote: > >>> > >>> When intertouch is enabled for the L440 a (deep)sleep/resume > >>> cycle causes the touchpad driver to hang which causes the > >>> touchpad to become unresponsive. Disable intertouch resolves > >>> this issue and the touchpad is fine after resume from sleep. > >>> > >>> Additionally, when the PNP id for the L440 is only removed > >>> from the topbuttonpad_pnp_ids list, a message is logged to > >>> enable psmouse.synaptics_intertouch, which would cause the > >>> sleep/resume issue again. By removing the PNP id from > >>> topbutton_pnp_ids and then adding it to the > >>> forcepad_pnp_ids array, intertouch is disabled and the > >>> message is not logged. > >>> > >>> Signed-off-by: Jonathan Denose <jdenose@google.com> > >>> --- > >>> > >>> Changes in v2: > >>> - remove debug statement > >>> > >>> drivers/input/mouse/synaptics.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c > >>> index fa021af8506e4..b7218b7652c20 100644 > >>> --- a/drivers/input/mouse/synaptics.c > >>> +++ b/drivers/input/mouse/synaptics.c > >>> @@ -150,7 +150,6 @@ static const char * const topbuttonpad_pnp_ids[] = { > >>> "LEN2001", /* Edge E431 */ > >>> "LEN2002", /* Edge E531 */ > >>> "LEN2003", > >>> - "LEN2004", /* L440 */ > >>> "LEN2005", > >>> "LEN2006", /* Edge E440/E540 */ > >>> "LEN2007", > >>> @@ -198,6 +197,7 @@ static const char * const smbus_pnp_ids[] = { > >>> static const char * const forcepad_pnp_ids[] = { > >>> "SYN300D", > >>> "SYN3014", > >>> + "LEN2004", /* L440 */ > > While this does seem to elliminate the message, the touchpad in the L440 is not a forcepad. Adding the L440 PnP ID here implies that it is one of these special forcepads which reports "force" data for contacts and that is not the case here. > > >>> NULL > >>> }; > >>> > >>> — > >>> 2.39.2 > >>> > > > Andrew >
On Mon, Apr 24, 2023 at 02:11:28PM -0500, Jonathan Denose wrote: > Hi Andrew, > > Thanks for your reply. As an update, I was able to try the patch here: > https://lore.kernel.org/all/YgHTYrODoo2ou49J@google.com/ and it > resolves the suspend/resume issue on this device. I'm not sure what > the status of the linked patch is, to me it doesn't look like it was > merged anywhere. This patch shoudl have been superseded by: commit 7b1f781f2d2460693f43d5f764198df558e3494b Author: Dmitry Torokhov <dmitry.torokhov@gmail.com> Date: Tue Feb 15 13:32:26 2022 -0800 Input: psmouse - set up dependency between PS/2 and SMBus companions When we switch from emulated PS/2 to native (RMI4 or Elan) protocols, we create SMBus companion devices that are attached to I2C/SMBus controllers. However, when suspending and resuming, we also need to make sure that we take into account the PS/2 device they are associated with, so that PS/2 device is suspended after the companion and resumed before it, otherwise companions will not work properly. Before I2C devices were marked for asynchronous suspend/resume, this ordering happened naturally, but now we need to enforce it by establishing device links, with PS/2 devices being suppliers and SMBus companions being consumers. Fixes: 172d931910e1 ("i2c: enable async suspend/resume on i2c client devices") Reported-and-tested-by: Hugh Dickins <hughd@google.com> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> Link: https://lore.kernel.org/r/89456fcd-a113-4c82-4b10-a9bcaefac68f@google.com Link: https://lore.kernel.org/r/YgwQN8ynO88CPMju@google.com Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Which should have ensured that PS/2 device is resumed first, before trying to resume RMI interface. I wonder why it is not working for you. Can you enable logging and see if the order of resume is correct or not. If it is still wrong we need to figure out why setting the link between the devices does not have the desired effect. Thanks.
` On Mon, May 1, 2023 at 10:12 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Link: https://lore.kernel.org/r/89456fcd-a113-4c82-4b10-a9bcaefac68f@google.com > > Which should have ensured that PS/2 device is resumed first, before > trying to resume RMI interface. I wonder why it is not working for you. > > Can you enable logging and see if the order of resume is correct or not. > If it is still wrong we need to figure out why setting the link between > the devices does not have the desired effect. I have a Lenovo T440p with a similar issue that fails on resume in rmi_smb_resume's call to rmi_smb_get_version. I've patched rmi_smb_get_version adding a sleep and retry loop in an attempt to see if it could avoid some race condition. I've attached a dmesg log from a 6.3.1 kernel, pm debug messages on and with my retry patch. In this log it succeeds due to the retry loop. I believe I'm seeing the suspend order of rmi_smbus then PS/2 first and PS/2 then rmi_smbus on resume. dmesg during suspend: [ 96.357933] rmi4_smbus 0-002c: PM: calling rmi_smb_suspend+0x0/0x44 [rmi_smbus] @ 2375, parent: i2c-0 ... [ 96.358521] psmouse serio1: PM: calling serio_suspend+0x0/0x1c @ 4517, parent: i8042 dmesg during resume with debugging showing the retry loop around getting the smbus version number: [ 97.048216] psmouse serio1: PM: calling serio_resume+0x0/0x8c @ 4517, parent: i8042 [ 97.051545] psmouse serio1: PM: serio_resume+0x0/0x8c returned 0 after 3325 usecs ... [ 97.051557] rmi4_smbus 0-002c: PM: calling rmi_smb_resume+0x0/0x67 [rmi_smbus] @ 4577, parent: i2c-0 ... [ 97.051725] [4577] i2c_i801:i801_check_post:414: i801_smbus 0000:00:1f.3: No response [ 97.051730] rmi4_smbus 0-002c: failed to get SMBus version number! [ 97.051732] rmi4_smbus 0-002c: sleeping to try again [ 97.052780] [4577] i2c_i801:i801_check_post:414: i801_smbus 0000:00:1f.3: No response [ 97.052785] rmi4_smbus 0-002c: failed to get SMBus version number! [ 97.052786] rmi4_smbus 0-002c: sleeping to try again [ 97.053531] snd_hda_intel 0000:00:03.0: PM: pci_pm_resume+0x0/0xdf returned 0 after 2275 usecs [ 97.053790] [4577] i2c_i801:i801_check_post:414: i801_smbus 0000:00:1f.3: No response [ 97.053795] rmi4_smbus 0-002c: failed to get SMBus version number! [ 97.053796] rmi4_smbus 0-002c: sleeping to try again [ 97.054783] [4577] i2c_i801:i801_check_post:414: i801_smbus 0000:00:1f.3: No response [ 97.054788] rmi4_smbus 0-002c: failed to get SMBus version number! [ 97.054789] rmi4_smbus 0-002c: sleeping to try again [ 97.055779] [4577] i2c_i801:i801_check_post:414: i801_smbus 0000:00:1f.3: No response [ 97.055784] rmi4_smbus 0-002c: failed to get SMBus version number! [ 97.055785] rmi4_smbus 0-002c: sleeping to try again [ 97.057117] rmi4_smbus 0-002c: JAM: time to return 6ms [ 97.057893] rmi4_smbus 0-002c: wrote 4 bytes at 0x86: 0 (4c 00 01 00) ... [ 97.067335] rmi4_smbus 0-002c: wrote 1 bytes at 0x07: 0 (80) [ 97.067340] rmi4_f11 rmi4-00.fn11: Resuming... [ 97.067342] rmi4_smbus 0-002c: PM: rmi_smb_resume+0x0/0x67 [rmi_smbus] returned 0 after 15783 usecs WIth this short timeout loop it seems to take between 6ms to 12ms before the calls to `i2c_smbus_read_byte_data` stop returning -ENXIO from "No response" log lines from i2c_i801. I have seen this on various versions such as 5.15, 5.10 (with the https://lore.kernel.org/r/89456fcd-a113-4c82-4b10-a9bcaefac68f@google.com patch applied), 6.1 and reproduced here on the 6.3.1 kernel. I don't know what the delay is waiting on and if there is some other way to deal with it. In these logs it appears that there are a few calls occuring at the same time as rmi_smb_resume. None of them return just before the retry loop ends and I'm not sure if they are related. I'm not sure if there is more information during this time that would be useful to look at while it's resuming? If I apply the patch to `device_disable_async_suspend(&client->dev);` in rmi_smb_probe it doesn't seem to need the delay. I could post dmesg output with the disable in rmi_smb_probe applied if the timing is possibly interesting to see the ordering and timing in that case. If there is anything else you would like me to try, please let me know. Is it possible there's just some sort of delay occurring here for the i801 bus to come up that can be handled in a different way? Thanks, Jeff
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c index fa021af8506e4..b7218b7652c20 100644 --- a/drivers/input/mouse/synaptics.c +++ b/drivers/input/mouse/synaptics.c @@ -150,7 +150,6 @@ static const char * const topbuttonpad_pnp_ids[] = { "LEN2001", /* Edge E431 */ "LEN2002", /* Edge E531 */ "LEN2003", - "LEN2004", /* L440 */ "LEN2005", "LEN2006", /* Edge E440/E540 */ "LEN2007", @@ -198,6 +197,7 @@ static const char * const smbus_pnp_ids[] = { static const char * const forcepad_pnp_ids[] = { "SYN300D", "SYN3014", + "LEN2004", /* L440 */ NULL };
When intertouch is enabled for the L440 a (deep)sleep/resume cycle causes the touchpad driver to hang which causes the touchpad to become unresponsive. Disable intertouch resolves this issue and the touchpad is fine after resume from sleep. Additionally, when the PNP id for the L440 is only removed from the topbuttonpad_pnp_ids list, a message is logged to enable psmouse.synaptics_intertouch, which would cause the sleep/resume issue again. By removing the PNP id from topbutton_pnp_ids and then adding it to the forcepad_pnp_ids array, intertouch is disabled and the message is not logged. Signed-off-by: Jonathan Denose <jdenose@google.com> --- Changes in v2: - remove debug statement drivers/input/mouse/synaptics.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)