Message-Id: <65C23A49-5A55-4CF4-9AFD-2DA504DAABF5@duggan.us>
Date:   Mon, 17 Apr 2023 13:14:42 -0700
From:   Andrew Duggan <andrew@...gan.us>
To:     Jonathan Denose <jdenose@...omium.org>
Cc:     Lyude Paul <lyude@...hat.com>,
        Andrew Duggan <aduggan@...aptics.com>,
        "amandhoot12@...il.com" <amandhoot12@...il.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        "jdenose@...gle.com" <jdenose@...gle.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "markpearson@...ovo.com" <markpearson@...ovo.com>,
        "wsa+renesas@...g-engineering.com" <wsa+renesas@...g-engineering.com>,
        benjamin.tissoires@...hat.com
Subject: Re: [PATCH v2] Input: synaptics - disable intertouch for Lenovo L440

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@...omium.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@...omium.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@...omium.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@...gle.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

Powered by blists - more mailing lists