diff mbox series

[v2] Input: synaptics - disable intertouch for Lenovo L440

Message ID 20230414092353.v2.1.Ieb687047a5b75c7b7ee5dd258207ef5ca9a3b728@changeid
State New
Headers show
Series [v2] Input: synaptics - disable intertouch for Lenovo L440 | expand

Commit Message

Jonathan Denose April 14, 2023, 4:41 p.m. UTC
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(-)

Comments

Andrew Duggan April 17, 2023, 8:14 p.m. UTC | #1
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
Jonathan Denose April 24, 2023, 7:11 p.m. UTC | #2
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
>
Dmitry Torokhov May 2, 2023, 3:11 a.m. UTC | #3
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.
Jeffery Miller May 4, 2023, 3:32 a.m. UTC | #4
`
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 mbox series

Patch

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