[PATCH] Input: i8042 - Fix the selftest retry logic

[PATCH] Input: i8042 - Fix the selftest retry logic

[AceLan Kao]

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

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(&param, I8042_CMD_CTL_TEST)) {
-			pr_err("i8042 controller selftest timeout\n");
-			return -ENODEV;
-		}
+		if (i8042_command(&param, 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
 	/*
-- 
2.17.1

Re: [PATCH] Input: i8042 - Fix the selftest retry logic

[Dmitry Torokhov]

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

Re: [PATCH] Input: i8042 - Fix the selftest retry logic

[AceLan Kao]

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

Re: [PATCH] Input: i8042 - Fix the selftest retry logic

[You-Sheng Yang]

[-- Attachment #1.1: Type: text/plain, Size: 561 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2] Input: i8042 - fix the selftest retry logic

[You-Sheng Yang]

From: You-Sheng Yang <vicamo.yang@canonical.com>

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: You-Sheng Yang <vicamo.yang@canonical.com>
---
 drivers/input/serio/i8042.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 20ff2bed3917..e8f2004071d4 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -937,25 +937,28 @@ static int i8042_controller_selftest(void)
 {
 	unsigned char param;
 	int i = 0;
+	int ret;
 
 	/*
 	 * We try this 5 times; on some really fragile systems this does not
 	 * take the first time...
 	 */
-	do {
-
-		if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
-			pr_err("i8042 controller selftest timeout\n");
-			return -ENODEV;
-		}
+	while (i++ < 5) {
 
-		if (param == I8042_RET_CTL_TEST)
+		ret = i8042_command(&param, I8042_CMD_CTL_TEST);
+		if (ret)
+			pr_err("i8042 controller selftest timeout (%d/5)\n", i);
+		else if (param == I8042_RET_CTL_TEST)
 			return 0;
+		else
+			dbg("i8042 controller selftest: %#x != %#x\n",
+			    param, I8042_RET_CTL_TEST);
 
-		dbg("i8042 controller selftest: %#x != %#x\n",
-		    param, I8042_RET_CTL_TEST);
 		msleep(50);
-	} while (i++ < 5);
+	}
+
+	if (ret)
+		return -ENODEV;
 
 #ifdef CONFIG_X86
 	/*
-- 
2.25.0

Re: [PATCH v2] Input: i8042 - fix the selftest retry logic

[AceLan Kao]

This v2 fix my issue, too.
Please consider to merge this patch.
Thanks.

You-Sheng Yang <vicamo@gmail.com> 於 2020年3月10日 週二 上午11:37寫道：
>
> From: You-Sheng Yang <vicamo.yang@canonical.com>
>
> 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: You-Sheng Yang <vicamo.yang@canonical.com>
> ---
>  drivers/input/serio/i8042.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index 20ff2bed3917..e8f2004071d4 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -937,25 +937,28 @@ static int i8042_controller_selftest(void)
>  {
>         unsigned char param;
>         int i = 0;
> +       int ret;
>
>         /*
>          * We try this 5 times; on some really fragile systems this does not
>          * take the first time...
>          */
> -       do {
> -
> -               if (i8042_command(&param, I8042_CMD_CTL_TEST)) {
> -                       pr_err("i8042 controller selftest timeout\n");
> -                       return -ENODEV;
> -               }
> +       while (i++ < 5) {
>
> -               if (param == I8042_RET_CTL_TEST)
> +               ret = i8042_command(&param, I8042_CMD_CTL_TEST);
> +               if (ret)
> +                       pr_err("i8042 controller selftest timeout (%d/5)\n", i);
> +               else if (param == I8042_RET_CTL_TEST)
>                         return 0;
> +               else
> +                       dbg("i8042 controller selftest: %#x != %#x\n",
> +                           param, I8042_RET_CTL_TEST);
>
> -               dbg("i8042 controller selftest: %#x != %#x\n",
> -                   param, I8042_RET_CTL_TEST);
>                 msleep(50);
> -       } while (i++ < 5);
> +       }
> +
> +       if (ret)
> +               return -ENODEV;
>
>  #ifdef CONFIG_X86
>         /*
> --
> 2.25.0
>