| Message ID | 20200305064423.16196-1-acelan.kao@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: i8042 - Fix the selftest retry logic | expand |
Hi AceLan, On Thu, Mar 05, 2020 at 02:44:23PM +0800, AceLan Kao wrote: > It returns -NODEV at the first selftest timeout, so the retry logic > doesn't work. Move the return outside of the while loop to make it real > retry 5 times before returns -ENODEV. The retry logic here was for the controller not returning the expected selftest value, not the controller refusing to communicate at all. Could you pease tell me what device requires this change? Thanks.
Hi Dmitry, We have a Dell desktop with ps2 addon card, after S3, the ps2 keyboard lost function and got below errors Jan 15 07:10:08 Rh-MT kernel: [ 346.575353] i8042: i8042 controller selftest timeout Jan 15 07:10:08 Rh-MT kernel: [ 346.575358] PM: Device i8042 failed to resume: error -19 Adding this patch, I found the selftest passes at the second retry and the keyboard continue working fine. Best regards, AceLan Kao. Dmitry Torokhov <dmitry.torokhov@gmail.com> 於 2020年3月6日 週五 下午12:16寫道： > > Hi AceLan, > > On Thu, Mar 05, 2020 at 02:44:23PM +0800, AceLan Kao wrote: > > It returns -NODEV at the first selftest timeout, so the retry logic > > doesn't work. Move the return outside of the while loop to make it real > > retry 5 times before returns -ENODEV. > > The retry logic here was for the controller not returning the expected > selftest value, not the controller refusing to communicate at all. > > Could you pease tell me what device requires this change? > > Thanks. > > -- > Dmitry
On 2020-03-05 14:44, AceLan Kao wrote: > @@ -955,7 +954,9 @@ static int i8042_controller_selftest(void) > dbg("i8042 controller selftest: %#x != %#x\n", > param, I8042_RET_CTL_TEST); > msleep(50); > - } while (i++ < 5); > + } while (++i < 5); > + if (i == 5) > + return -ENODEV; I would like to propose a V2 for this. The original logic allows continuation to device probe when selftest returns a different value than expected, and this is no longer available with this patch. > #ifdef CONFIG_X86 > /* > You-Sheng Yang
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c index 20ff2bed3917..3f6433a5c8e6 100644 --- a/drivers/input/serio/i8042.c +++ b/drivers/input/serio/i8042.c @@ -944,10 +944,9 @@ static int i8042_controller_selftest(void) */ do { - if (i8042_command(¶m, I8042_CMD_CTL_TEST)) { - pr_err("i8042 controller selftest timeout\n"); - return -ENODEV; - } + if (i8042_command(¶m, I8042_CMD_CTL_TEST)) + pr_err("i8042 controller selftest timeout (%d/5)\n", + i+1); if (param == I8042_RET_CTL_TEST) return 0; @@ -955,7 +954,9 @@ static int i8042_controller_selftest(void) dbg("i8042 controller selftest: %#x != %#x\n", param, I8042_RET_CTL_TEST); msleep(50); - } while (i++ < 5); + } while (++i < 5); + if (i == 5) + return -ENODEV; #ifdef CONFIG_X86 /*
It returns -NODEV at the first selftest timeout, so the retry logic doesn't work. Move the return outside of the while loop to make it real retry 5 times before returns -ENODEV. BTW, the origin loop will retry 6 times, also fix this. Signed-off-by: AceLan Kao <acelan.kao@canonical.com> --- drivers/input/serio/i8042.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)