| Message ID | 20230510192731.300786-1-jefferymiller@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: synaptics-rmi4 - retry reading SMBus version on resume | expand |
On Wed, May 10, 2023 at 07:27:22PM +0000, Jeffery Miller wrote: > On resume there can be a period of time after the > preceding serio_resume -> psmouse_deactivate call > where calls to rmi_smb_get_version fail with > -ENXIO. > > The call path in rmi_smb_resume is rmi_smb_resume -> rmi_smb_reset -> > rmi_smb_enable_smbus_mode -> rmi_smb_get_version where > this failure would occur. > > Adding a retry loop ensures that after rmi_smb_reset returns > the following rmi_driver_resume calls in rmi_smbus_resume can > succeed. > > This behavior was seen on a Lenovo T440p machine that required > a delay of approximately 7-12ms. > The retry limit of 5 is chosen to be larger than > this observed delay. > > With this patch the trimmed resume logs look similar to: > ``` > psmouse serio1: PM: calling serio_resume+0x0/0x8c @ 5399, parent: i8042 > [5399] libps2:__ps2_command:316: psmouse serio1: f5 [] - 0/00000000 [] > psmouse serio1: PM: serio_resume+0x0/0x8c returned 0 after 3259 usecs > ... > rmi4_smbus 0-002c: PM: calling rmi_smb_resume ... @ 5454, parent: i2c-0 > ... > [5454] i2c_i801:i801_check_post:414: i801_smbus 0000:00:1f.3: No response > smbus_result: i2c-0 a=02c f=0000 c=fd BYTE_DATA rd res=-6 > rmi4_smbus 0-002c: failed to get SMBus version number! > rmi4_smbus 0-002c: sleeping to retry getting the SMBus version number > ... > rmi4_smbus 0-002c: PM: rmi_smb_resume ... returned 0 after 21351 usecs > ``` > > Signed-off-by: Jeffery Miller <jefferymiller@google.com> > --- > > Early boot dmesg include: > ``` > rmi4_smbus 0-002c: registering SMbus-connected sensor > rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: Synaptics, product: TM2722-001, fw id: 0 > ``` > > The resume order looks correct. The `psmouse serio1` resume returns > before the rmi_smb_resume is called showing the patch from > https://lore.kernel.org/all/89456fcd-a113-4c82-4b10-a9bcaefac68f@google.com/ > is applied and working for that ordering. > > I attempted to try to rule out some interaction between the concurrent > input resume calls for other i8042 devices. > Adding a 7ms delay after psmouse_deactivate which is called in the > preceding psmouse serio1 serio_resume function also allows > this version call to succeed. I am not really fond of adding random repeats in the code base. Andrew, do you know if the Synaptics device needs certain delay when switching to SMbus mode? Thanks.
On Wed, May 10, 2023 at 3:06 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > I am not really fond of adding random repeats in the code base. Andrew, > do you know if the Synaptics device needs certain delay when switching > to SMbus mode? > That's reasonable. It's true this is a sleep to workaround rmi_smbus' expectation that it is able to successfully read from the i801 i2c smbus addr 0x2c on resume when it cannot. I do not know why the i801 i2c addr 0x2c is not responding on resume. Infrequently it won't respond on boot during the initial psmouse_smbus_create_companion but that is less noticeable since it will stay in ps/2 mode and just lack features. I do know adding a sleep in-between the psmouse deactivate call and rmi_smbus's attempts to read from the 0x2c addr allow it to succeed on this machine. I do not know the details as to why however. I can apply workarounds until the underlying issue can be identified and properly resolved. I can apply patches and provide any sort of debugging or extra information that might be useful to anyone familiar with these devices. The smbus controller in this particular machine is: 00:1f.3 SMBus [0c05]: Intel Corporation 8 Series/C220 Series Chipset Family SMBus Controller [8086:8c22] (rev 04) Perhaps there's some way to detect when the addr is available later and have it trigger a new probe similar to how psmouse_smbus_notifier is triggered when the i801 bus becomes available? Thank You, Jeff
> From: Jeffery Miller <jefferymiller@google.com> > Sent: Friday, May 12, 2023 12:37 > To: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Andrew Duggan <andrew@duggan.us>; Jonathan Denose > <jdenose@chromium.org>; jdenose@google.com; Lyude Paul > <lyude@redhat.com>; loic.poulain@linaro.org; > benjamin.tissoires@redhat.com; Andrew Duggan > <aduggan@synaptics.com>; Jonathan Cameron > <Jonathan.Cameron@huawei.com>; Maximilian Luz > <luzmaximilian@gmail.com>; Miguel Ojeda <ojeda@kernel.org>; Uwe Kleine- > König <u.kleine-koenig@pengutronix.de>; linux-input@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH] Input: synaptics-rmi4 - retry reading SMBus version on > resume > > CAUTION: Email originated externally, do not click links or open attachments > unless you recognize the sender and know the content is safe. > > > On Wed, May 10, 2023 at 3:06 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > > > I am not really fond of adding random repeats in the code base. > > Andrew, do you know if the Synaptics device needs certain delay when > > switching to SMbus mode? > > > That's reasonable. It's true this is a sleep to workaround rmi_smbus' > expectation that > it is able to successfully read from the i801 i2c smbus addr 0x2c on resume > when it cannot. > > I do not know why the i801 i2c addr 0x2c is not responding on resume. > Infrequently it won't respond on boot during the initial > psmouse_smbus_create_companion but that is less noticeable since it will > stay in ps/2 mode and just lack features. > > I do know adding a sleep in-between the psmouse deactivate call and > rmi_smbus's attempts to read from the 0x2c addr allow it to succeed on this > machine. I do not know the details as to why however. Our Windows driver does a 30 ms delay between sending the disable command and trying to communicate over SMBus. I don’t know specifically why this delay is needed, but this is how we handle this case. > > I can apply workarounds until the underlying issue can be identified and > properly resolved. > > I can apply patches and provide any sort of debugging or extra information > that might be useful to anyone familiar with these devices. > > The smbus controller in this particular machine is: > 00:1f.3 SMBus [0c05]: Intel Corporation 8 Series/C220 Series Chipset Family > SMBus Controller [8086:8c22] (rev 04) > > Perhaps there's some way to detect when the addr is available later and have > it trigger a new probe similar to how psmouse_smbus_notifier is triggered > when the i801 bus becomes available? > > Thank You, > Jeff Andrew
diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c index 4bf0e1df6a4a..386e80ae141b 100644 --- a/drivers/input/rmi4/rmi_smbus.c +++ b/drivers/input/rmi4/rmi_smbus.c @@ -43,15 +43,26 @@ static int rmi_smb_get_version(struct rmi_smb_xport *rmi_smb) { struct i2c_client *client = rmi_smb->client; int retval; + int tries = 0; /* 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) { - dev_err(&client->dev, "failed to get SMBus version number!\n"); - return retval; - } + do { + if (tries > 0) { + dev_warn(&client->dev, "sleeping to retry getting the SMBus version number\n"); + fsleep(5000); + } + retval = i2c_smbus_read_byte_data(client, + SMB_PROTOCOL_VERSION_ADDRESS); + if (retval >= 0) + return retval + 1; - return retval + 1; + dev_err(&client->dev, "failed to get SMBus version number!\n"); + /* On resume the read of the version can + * momentarily return -ENXIO. + * Retry to allow additional time for it to succeed. + */ + } while (retval == -ENXIO && tries++ < 5); + return retval; } /* SMB block write - wrapper over ic2_smb_write_block */
On resume there can be a period of time after the preceding serio_resume -> psmouse_deactivate call where calls to rmi_smb_get_version fail with -ENXIO. The call path in rmi_smb_resume is rmi_smb_resume -> rmi_smb_reset -> rmi_smb_enable_smbus_mode -> rmi_smb_get_version where this failure would occur. Adding a retry loop ensures that after rmi_smb_reset returns the following rmi_driver_resume calls in rmi_smbus_resume can succeed. This behavior was seen on a Lenovo T440p machine that required a delay of approximately 7-12ms. The retry limit of 5 is chosen to be larger than this observed delay. With this patch the trimmed resume logs look similar to: ``` psmouse serio1: PM: calling serio_resume+0x0/0x8c @ 5399, parent: i8042 [5399] libps2:__ps2_command:316: psmouse serio1: f5 [] - 0/00000000 [] psmouse serio1: PM: serio_resume+0x0/0x8c returned 0 after 3259 usecs ... rmi4_smbus 0-002c: PM: calling rmi_smb_resume ... @ 5454, parent: i2c-0 ... [5454] i2c_i801:i801_check_post:414: i801_smbus 0000:00:1f.3: No response smbus_result: i2c-0 a=02c f=0000 c=fd BYTE_DATA rd res=-6 rmi4_smbus 0-002c: failed to get SMBus version number! rmi4_smbus 0-002c: sleeping to retry getting the SMBus version number ... rmi4_smbus 0-002c: PM: rmi_smb_resume ... returned 0 after 21351 usecs ``` Signed-off-by: Jeffery Miller <jefferymiller@google.com> --- Early boot dmesg include: ``` rmi4_smbus 0-002c: registering SMbus-connected sensor rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: Synaptics, product: TM2722-001, fw id: 0 ``` The resume order looks correct. The `psmouse serio1` resume returns before the rmi_smb_resume is called showing the patch from https://lore.kernel.org/all/89456fcd-a113-4c82-4b10-a9bcaefac68f@google.com/ is applied and working for that ordering. I attempted to try to rule out some interaction between the concurrent input resume calls for other i8042 devices. Adding a 7ms delay after psmouse_deactivate which is called in the preceding psmouse serio1 serio_resume function also allows this version call to succeed. If the rmi_smb_probe device_disable_async_suspend patch is applied it will also avoid this issue. However the time between the psmouse_deactivate call for serio_resume and rmi_smb_resume was over 60ms on my test machine. This would naturally be long enough to avoid this particular delay. drivers/input/rmi4/rmi_smbus.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)