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

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

[Jonathan Denose]

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

 drivers/input/mouse/synaptics.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index fa021af8506e4..77a4f58128e84 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
 };
 
@@ -1769,6 +1769,8 @@ static int synaptics_create_intertouch(struct psmouse *psmouse,
 		.flags = I2C_CLIENT_HOST_NOTIFY,
 	};
 
+	psmouse_dbg(psmouse, "topbuttonpad is: %s\n", topbuttonpad ? "true" : "false");
+
 	return psmouse_smbus_init(psmouse, &intertouch_board,
 				  &pdata, sizeof(pdata), true,
 				  leave_breadcrumbs);
-- 
2.40.0.577.gac1e443424-goog

Re: [PATCH] Input: synaptics - disable intertouch for Lenovo L440

[Lyude Paul]

This patch looks fine to me, I'm a bit curious though whether the folks at
Synaptics have any idea of some other workaround we might be able to do rather
than disabling intertouch? Added Andrew Duggan to CC to ask.

If we don't get any response from them after a while, feel free to consider
this:

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Wed, 2023-04-12 at 17:54 -0500, Jonathan Denose 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>
> ---
> 
>  drivers/input/mouse/synaptics.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index fa021af8506e4..77a4f58128e84 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
>  };
>  
> @@ -1769,6 +1769,8 @@ static int synaptics_create_intertouch(struct psmouse *psmouse,
>  		.flags = I2C_CLIENT_HOST_NOTIFY,
>  	};
>  
> +	psmouse_dbg(psmouse, "topbuttonpad is: %s\n", topbuttonpad ? "true" : "false");
> +
>  	return psmouse_smbus_init(psmouse, &intertouch_board,
>  				  &pdata, sizeof(pdata), true,
>  				  leave_breadcrumbs);

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

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

[Jonathan Denose]

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 */
 	NULL
 };
 
-- 
2.39.2

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

[Andrew Duggan]

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

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

[Jonathan Denose]

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
>

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

[Dmitry Torokhov]

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.

-- 
Dmitry

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

[Jeffery Miller]

[-- Attachment #1: Type: text/plain, Size: 4499 bytes --]

`
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

[-- Attachment #2: dmesg-6-3-1-retry.out.xz --]
[-- Type: application/x-xz, Size: 23836 bytes --]

[-- Attachment #3: 0001-Input-synaptics-rmi4-retry-reading-SMBus-version-on-.patch --]
[-- Type: text/x-patch, Size: 2960 bytes --]

From fd4e87a0d1d8efa0f3a924a5e5cdd47f10b336dc Mon Sep 17 00:00:00 2001
From: Jeffery Miller <jefferymiller@google.com>
Date: Thu, 20 Apr 2023 11:35:59 -0500
Subject: [PATCH] Input: synaptics-rmi4 - retry reading SMBus version on resume

When resuming from suspend there is a delay where
getting the version data from the smbus returns
-ENXIO. This causes the rmi4 resume to fail and
the touchpad to stop working after resume.

In lieu of finding what this really needs to wait
on just retry a few times so it works.

The limit of 100ms is arbitrary.
My tests have shown this to take an extra 7-12ms
on resume to succeed on the Lenovo t440p machine.

Logs on resume now look something like:
```
[  108.622951] rmi4_smbus 0-002c: calling rmi_smb_resume+0x0/0x63 [rmi_smbus] @ 3975, parent: i2c-0
...
[  108.623132] rmi4_smbus 0-002c: failed to get SMBus version number!
[  108.623134] rmi4_smbus 0-002c: sleeping to try again
[  108.626003] rmi4_smbus 0-002c: failed to get SMBus version number!
[  108.626005] rmi4_smbus 0-002c: sleeping to try again
[  108.629025] rmi4_smbus 0-002c: failed to get SMBus version number!
[  108.629028] rmi4_smbus 0-002c: sleeping to try again
[  108.632017] rmi4_smbus 0-002c: failed to get SMBus version number!
[  108.632019] rmi4_smbus 0-002c: sleeping to try again
[  108.636172] rmi4_smbus 0-002c: wrote 4 bytes at 0x86: 0 (4c 00 01 00)
```
Signed-off-by: Jeffery Miller <jefferymiller@google.com>
---
 drivers/input/rmi4/rmi_smbus.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index 4bf0e1df6a4a..1fccad5811cf 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -9,6 +9,7 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/jiffies.h>
 #include <linux/kconfig.h>
 #include <linux/lockdep.h>
 #include <linux/module.h>
@@ -44,14 +45,27 @@ static int rmi_smb_get_version(struct rmi_smb_xport *rmi_smb)
 	struct i2c_client *client = rmi_smb->client;
 	int retval;
 
+	unsigned long deadline = jiffies + msecs_to_jiffies(100);
+	unsigned long before = jiffies;
+
 	/* Check if for SMBus new version device by reading version byte. */
-	retval = i2c_smbus_read_byte_data(client, SMB_PROTOCOL_VERSION_ADDRESS);
-	if (retval < 0) {
+	while (time_before(jiffies, deadline)) {
+
+		retval = i2c_smbus_read_byte_data(client,
+				SMB_PROTOCOL_VERSION_ADDRESS);
+		if (retval >= 0) {
+			retval++;
+			break;
+		}
+
 		dev_err(&client->dev, "failed to get SMBus version number!\n");
-		return retval;
+		if (retval != -ENXIO)
+			break;
+		dev_warn(&client->dev, "sleeping to try again\n");
+		fsleep(500);
 	}
-
-	return retval + 1;
+	dev_warn(&client->dev, "JAM: time to return %ums", jiffies_to_msecs(jiffies - before));
+	return retval;
 }
 
 /* SMB block write - wrapper over ic2_smb_write_block */
-- 
2.40.1.495.gc816e09b53d-goog