Change in ...coreboot[master]: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33185

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

libpayload/i8042/keyboard: Fix return value check for keyboard_cmd

CB:32951 ("libpayload: Reset PS/2 keyboard") added a call to reset
keyboard and check the return value of keyboard_cmd() to compare
against I8042_KBCMD_ACK. However, keyboard_cmd() already checks for
ACK and returns 1 or 0 based on whether ACK is received.

This change fixes the check introduced by CB:32951 to compare against
0 just like the other checks for keyboard_cmd().

BUG=b:134366527
TEST=Verified that logs do not contain "ERROR: Keyboard reset failed"
anymore.

Change-Id: Idcadaae12e0a44e404a1d98c6deb633d97058203
Signed-off-by: Furquan Shaikh 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33185/1

diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index 12255fb..1bc321d 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -319,8 +319,8 @@

/* Reset keyboard and self test (keyboard side) */
    ret = keyboard_cmd(I8042_KBCMD_RESET);
-	if (ret != I8042_KBCMD_ACK) {
-		printf("ERROR: Keyboard reset failed ACK: 0x%x\n", ret);
+	if (!ret) {
+		printf("ERROR: Keyboard reset failed!\n", ret);
    	return;
    }

[Furquan Shaikh (Code Review)]

Hello Philip Chen, Paul Menzel, Duncan Laurie, Shelley Chen, build bot (Jenkins), Matt Delco, Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33185

to look at the new patch set (#2).

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

libpayload/i8042/keyboard: Fix return value check for keyboard_cmd

CB:32951 ("libpayload: Reset PS/2 keyboard") added a call to reset
keyboard and check the return value of keyboard_cmd() to compare
against I8042_KBCMD_ACK. However, keyboard_cmd() already checks for
ACK and returns 1 or 0 based on whether ACK is received.

This change fixes the check introduced by CB:32951 to compare against
0 just like the other checks for keyboard_cmd().

BUG=b:134366527
TEST=Verified that logs do not contain "ERROR: Keyboard reset failed"
anymore.

Change-Id: Idcadaae12e0a44e404a1d98c6deb633d97058203
Signed-off-by: Furquan Shaikh 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33185/2

[Matt Delco (Code Review)]

Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

LGTM, thanks.  Nothing else in the function logs on error but I like having at least some msg there that helps make it clear when the keyboard isn't working.

https://review.coreboot.org/#/c/33185/2/payloads/libpayload/drivers/i8042/ke... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/#/c/33185/2/payloads/libpayload/drivers/i8042/ke... 
PS2, Line 175: 0xfa
nit: using I8042_KBCMD_ACK here would help avoid the newly added #define from now being unused.

[Furquan Shaikh (Code Review)]

Hello Philip Chen, Paul Menzel, Duncan Laurie, Shelley Chen, Matt Delco, build bot (Jenkins), Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33185

to look at the new patch set (#3).

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

libpayload/i8042/keyboard: Fix return value check for keyboard_cmd

CB:32951 ("libpayload: Reset PS/2 keyboard") added a call to reset
keyboard and check the return value of keyboard_cmd() to compare
against I8042_KBCMD_ACK. However, keyboard_cmd() already checks for
ACK and returns 1 or 0 based on whether ACK is received.

This change fixes the check introduced by CB:32951 to compare against
0 just like the other checks for keyboard_cmd(). Additionally, it adds
error messages for all failed commands in keyboard_init() to make the
prints consistent in case of failure.

BUG=b:134366527
TEST=Verified that logs do not contain "ERROR: Keyboard reset failed"
anymore.

Change-Id: Idcadaae12e0a44e404a1d98c6deb633d97058203
Signed-off-by: Furquan Shaikh 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 15 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/33185/3

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

Patch Set 3:

(1 comment)
> Patch Set 2: Code-Review+1
>
> (1 comment)
>
> LGTM, thanks.  Nothing else in the function logs on error but I like having at least some msg there that helps make it clear when the keyboard isn't working.

Yeah, the msg helped identify the error. I have added similar messages to other steps to make the failure reporting consistent.

https://review.coreboot.org/#/c/33185/2/payloads/libpayload/drivers/i8042/ke... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/#/c/33185/2/payloads/libpayload/drivers/i8042/ke... 
PS2, Line 175: 0xfa
> nit: using I8042_KBCMD_ACK here would help avoid the newly added #define from now being unused.

Done

[Frank Wu (Code Review)]

Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

Patch Set 3: Code-Review+1

[Frank Wu (Code Review)]

Frank Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

Patch Set 3:

verified pass at my side

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

Patch Set 3: Code-Review+2

Thank you very much for the quick fix. Using the new macro, and printing messages is nice to have, and is what I had locally done too, after people reported the issues.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/33185/3/payloads/libpayload/drivers/i8042/ke... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/#/c/33185/3/payloads/libpayload/drivers/i8042/ke... 
PS3, Line 322: 	if (!ret) {
Should it also be possible to ignore the init failure?

!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/33185/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33185/3//COMMIT_MSG@21 
PS3, Line 21: anymore.
Could you please clarify, if only the log message was the problem, or if the keyboard also did not work. In QEMU, there was just the log message, but the keyboard worked in QEMU and on the ASRock E350M1 – in contrast to before the commit, where it did not work.

On the coreboot mailing list, Martin reported [1], that with the commit, nothing worked for him anymore.

[1]: https://mail.coreboot.org/hyperkitty/list/[e-mail]/thread/QQTZ3...

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

libpayload/i8042/keyboard: Fix return value check for keyboard_cmd

CB:32951 ("libpayload: Reset PS/2 keyboard") added a call to reset
keyboard and check the return value of keyboard_cmd() to compare
against I8042_KBCMD_ACK. However, keyboard_cmd() already checks for
ACK and returns 1 or 0 based on whether ACK is received.

This change fixes the check introduced by CB:32951 to compare against
0 just like the other checks for keyboard_cmd(). Additionally, it adds
error messages for all failed commands in keyboard_init() to make the
prints consistent in case of failure.

BUG=b:134366527
TEST=Verified that logs do not contain "ERROR: Keyboard reset failed"
anymore.

Change-Id: Idcadaae12e0a44e404a1d98c6deb633d97058203
Signed-off-by: Furquan Shaikh 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33185
Tested-by: build bot (Jenkins) 
Reviewed-by: Frank Wu 
Reviewed-by: Paul Menzel 
---
M payloads/libpayload/drivers/i8042/keyboard.c
1 file changed, 15 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, approved
  Frank Wu: Looks good to me, but someone else must approve

diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c
index 12255fb..240385c 100644
--- a/payloads/libpayload/drivers/i8042/keyboard.c
+++ b/payloads/libpayload/drivers/i8042/keyboard.c
@@ -172,7 +172,7 @@
 {
    i8042_write_data(cmd);

-	return i8042_wait_read_ps2() == 0xfa;
+	return i8042_wait_read_ps2() == I8042_KBCMD_ACK;
 }

int keyboard_havechar(void)
@@ -319,32 +319,40 @@

/* Reset keyboard and self test (keyboard side) */
    ret = keyboard_cmd(I8042_KBCMD_RESET);
-	if (ret != I8042_KBCMD_ACK) {
-		printf("ERROR: Keyboard reset failed ACK: 0x%x\n", ret);
+	if (!ret) {
+		printf("ERROR: Keyboard reset failed!\n");
    	return;
    }

/* Set scancode set 1 */
    ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE);
-	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE))
+	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
+		printf("ERROR: Keyboard set scancode failed!\n");
    	return;
+	}

ret = keyboard_cmd(I8042_SCANCODE_SET_1);
-	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE))
+	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
+		printf("ERROR: Keyboard scancode set#1 failed!\n");
    	return;
+	}

/*
     * Set default parameters.
     * Fix for broken QEMU ps/2 make scancodes.
     */
    ret = keyboard_cmd(0xf6);
-	if (!ret)
+	if (!ret) {
+		printf("ERROR: Keyboard set default params failed!\n");
    	return;
+	}

/* Enable scanning */
    ret = keyboard_cmd(I8042_KBCMD_EN);
-	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE))
+	if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) {
+		printf("ERROR: Keyboard enable scanning failed!\n");
    	return;
+	}

console_add_input_driver(&cons);
 }

[Matt Delco (Code Review)]

Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/33185/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33185/3//COMMIT_MSG@21 
PS3, Line 21: anymore.
> Could you please clarify, if only the log message was the problem, or if the keyboard also did not w […]

The keyboard was non-functional on the platform I was using. I only noticed the log after tracing back to the code addition and then looked for the msg in the log to get confirmation I was hitting the newly added path.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33185 )

Change subject: libpayload/i8042/keyboard: Fix return value check for keyboard_cmd
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/33185/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33185/3//COMMIT_MSG@21 
PS3, Line 21: anymore.
> The keyboard was non-functional on the platform I was using. […]

We were observing that the keyboard did not work with this change.

https://review.coreboot.org/#/c/33185/3/payloads/libpayload/drivers/i8042/ke... 
File payloads/libpayload/drivers/i8042/keyboard.c:

https://review.coreboot.org/#/c/33185/3/payloads/libpayload/drivers/i8042/ke... 
PS3, Line 322: 	if (!ret) {
> Should it also be possible to ignore the init failure? […]

I initially thought of doing that, but looking at the places where this was added, it seemed to be specific to scanning i.e. setting of scancode or enable scanning. I can add that in a follow-up commit if it seems to be the right thing to do here. +Duncan.