libpayload/i8042/keyboard: ERROR Keyboard reset failed when selecting Secondary Payload in SeaBIOS
Hi,
Just tested a build using this config: https://github.com/merge/skulls/blob/master/x230/nonfree-defconfig-139b3cef0... with a recent coreboot (my master branch HEAD is at 0da3a8a91b soc/intel/baytrail: set default VBIOS filename and PCI ID)
When selecting "3" or "4" for the secondary payloads like coreinfo in Seabios, I always hit this: payloads/libpayload/drivers/i8042/keyboard.c: printf("ERROR: Keyboard reset failed ACK: 0x%x\n", ret);
I get "ERROR: Keyboard reset failed ACK: 0x1".
and I basically have to shutdown.
what's wrong? I think commit 7ae606f57f0b3d450ae748141b0e2367041b27d3 Paul?
thanks, martin
I think we are also seeing this issue after downstreaming: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
+Matt Delco delco@google.com posted some comments on this CL.
On Tue, Jun 4, 2019 at 1:11 PM Martin Kepplinger martink@posteo.de wrote:
Hi,
Just tested a build using this config:
https://github.com/merge/skulls/blob/master/x230/nonfree-defconfig-139b3cef0... with a recent coreboot (my master branch HEAD is at 0da3a8a91b soc/intel/baytrail: set default VBIOS filename and PCI ID)
When selecting "3" or "4" for the secondary payloads like coreinfo in Seabios, I always hit this: payloads/libpayload/drivers/i8042/keyboard.c: printf("ERROR: Keyboard reset failed ACK: 0x%x\n", ret);
I get "ERROR: Keyboard reset failed ACK: 0x1".
and I basically have to shutdown.
what's wrong? I think commit 7ae606f57f0b3d450ae748141b0e2367041b27d3 Paul?
thanks, martin _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Here is a tiny example patch which should solve this issue by removing "return;" : "Still print this message but don't abort prematurely" ( could also submit via gerrit but only a day later ). I got it at Lenovo G505S but everything is working fine despite this annoying message - at least while using this laptop's internal PS/2 keyboard (haven't tried a USB yet). Initially I thought it's coming from SeaBIOS, but after researching indeed it comes from this "./coreboot/payloads/libpayload/drivers/i8042/keyboard.c" place you've mentioned.
Solve a keyboard initialization problem by not aborting prematurely if its' reset has failed.
Signed-off-by: Mike Banon <mikebdp2 at gmail.com> --- diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 12255fb..b34dbca 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -321,7 +321,6 @@ void keyboard_init(void) ret = keyboard_cmd(I8042_KBCMD_RESET); if (ret != I8042_KBCMD_ACK) { printf("ERROR: Keyboard reset failed ACK: 0x%x\n", ret); - return; }
/* Set scancode set 1 */
On Tue, Jun 4, 2019 at 8:14 AM Joel Kitching via coreboot coreboot@coreboot.org wrote:
I think we are also seeing this issue after downstreaming: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
+Matt Delco posted some comments on this CL.
On Tue, Jun 4, 2019 at 1:11 PM Martin Kepplinger martink@posteo.de wrote:
Hi,
Just tested a build using this config: https://github.com/merge/skulls/blob/master/x230/nonfree-defconfig-139b3cef0... with a recent coreboot (my master branch HEAD is at 0da3a8a91b soc/intel/baytrail: set default VBIOS filename and PCI ID)
When selecting "3" or "4" for the secondary payloads like coreinfo in Seabios, I always hit this: payloads/libpayload/drivers/i8042/keyboard.c: printf("ERROR: Keyboard reset failed ACK: 0x%x\n", ret);
I get "ERROR: Keyboard reset failed ACK: 0x1".
and I basically have to shutdown.
what's wrong? I think commit 7ae606f57f0b3d450ae748141b0e2367041b27d3 Paul?
thanks, martin _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Dear Martin,
On 06/04/19 07:10, Martin Kepplinger wrote:
Just tested a build using this config: https://github.com/merge/skulls/blob/master/x230/nonfree-defconfig-139b3cef0... with a recent coreboot (my master branch HEAD is at 0da3a8a91b soc/intel/baytrail: set default VBIOS filename and PCI ID)
When selecting "3" or "4" for the secondary payloads like coreinfo in Seabios, I always hit this: payloads/libpayload/drivers/i8042/keyboard.c: printf("ERROR: Keyboard reset failed ACK: 0x%x\n", ret);
I get "ERROR: Keyboard reset failed ACK: 0x1".
and I basically have to shutdown.
The message is indeed unintended, but I am pretty sure it’s not the reason for you to have to shut down. Matt Delco pointed out that the the function `keyboard_cmd()` does not return the response code but true or false. I missed that.
what's wrong? I think commit 7ae606f57f0b3d450ae748141b0e2367041b27d3 Paul?
Did you test if everything works, if you revert the commit?
Did the PS/2 keyboard work before when loading libpayload based payloads from SeaBIOS or GRUB (chainloader)?
Kind regards,
Paul
Dear Martin,
On 06/04/19 12:01, Paul Menzel wrote:
On 06/04/19 07:10, Martin Kepplinger wrote:
Just tested a build using this config: https://github.com/merge/skulls/blob/master/x230/nonfree-defconfig-139b3cef0... with a recent coreboot (my master branch HEAD is at 0da3a8a91b soc/intel/baytrail: set default VBIOS filename and PCI ID)
When selecting "3" or "4" for the secondary payloads like coreinfo in Seabios, I always hit this: payloads/libpayload/drivers/i8042/keyboard.c: printf("ERROR: Keyboard reset failed ACK: 0x%x\n", ret);
I get "ERROR: Keyboard reset failed ACK: 0x1".
and I basically have to shutdown.
The message is indeed unintended, but I am pretty sure it’s not the reason for you to have to shut down. Matt Delco pointed out that the the function `keyboard_cmd()` does not return the response code but true or false. I missed that.
Reading the diff again, indeed a `return` was added in case of failure, so the commit could cause the problem you described.
what's wrong? I think commit 7ae606f57f0b3d450ae748141b0e2367041b27d3 Paul?
Did you test if everything works, if you revert the commit?
Did the PS/2 keyboard work before when loading libpayload based payloads from SeaBIOS or GRUB (chainloader)?
I’ll submit a fix to check true/false.
Kind regards,
Paul
Dear coreboot folks,
On 06/04/19 12:15, Paul Menzel wrote:
On 06/04/19 12:01, Paul Menzel wrote:
On 06/04/19 07:10, Martin Kepplinger wrote:
Just tested a build using this config: https://github.com/merge/skulls/blob/master/x230/nonfree-defconfig-139b3cef0... with a recent coreboot (my master branch HEAD is at 0da3a8a91b soc/intel/baytrail: set default VBIOS filename and PCI ID)
When selecting "3" or "4" for the secondary payloads like coreinfo in Seabios, I always hit this: payloads/libpayload/drivers/i8042/keyboard.c: printf("ERROR: Keyboard reset failed ACK: 0x%x\n", ret);
I get "ERROR: Keyboard reset failed ACK: 0x1".
and I basically have to shutdown.
The message is indeed unintended, but I am pretty sure it’s not the reason for you to have to shut down. Matt Delco pointed out that the the function `keyboard_cmd()` does not return the response code but true or false. I missed that.
Reading the diff again, indeed a `return` was added in case of failure, so the commit could cause the problem you described.
what's wrong? I think commit 7ae606f57f0b3d450ae748141b0e2367041b27d3 Paul?
Did you test if everything works, if you revert the commit?
Did the PS/2 keyboard work before when loading libpayload based payloads from SeaBIOS or GRUB (chainloader)?
I’ll submit a fix to check true/false.
Furquan created a fix [1] and it was accepted and is now in the master branch.
Please verify, if it works for you now.
Kind regards,
Paul
Am 04.06.2019 13:59 schrieb Paul Menzel:
Dear coreboot folks,
On 06/04/19 12:15, Paul Menzel wrote:
On 06/04/19 12:01, Paul Menzel wrote:
On 06/04/19 07:10, Martin Kepplinger wrote:
what's wrong? I think commit 7ae606f57f0b3d450ae748141b0e2367041b27d3 Paul?
Did you test if everything works, if you revert the commit?
Did the PS/2 keyboard work before when loading libpayload based payloads from SeaBIOS or GRUB (chainloader)?
I’ll submit a fix to check true/false.
Furquan created a fix [1] and it was accepted and is now in the master branch.
Please verify, if it works for you now.
Kind regards,
Paul
Testing that on the X230, I get:
"ERROR: Keyboard set scancode failed!" and keyboard does not work at all, in said payloads.
Also, reverting https://review.coreboot.org/c/coreboot/+/32951/ ( 7ae606f57f0b3d450ae748141b0e2367041b27d3 ) fixes the problem.
Please fix this. I also think we can not return early when "RESET" fails. Why not just (try to) reset silently, if possible?
thanks,
martin
Martin, could you please check what happens if you apply this patch but then remove a return that follows ""ERROR: Keyboard set scancode failed!" ? Perhaps we'll have to do the same for all these returns : "Still print this message but don't abort prematurely"
Signed-off-by: Mike Banon <mikebdp2 at gmail.com> --- diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 240385ce6d..c81392ab72 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -328,7 +328,6 @@ void keyboard_init(void) ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE); if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) { printf("ERROR: Keyboard set scancode failed!\n"); - return; }
ret = keyboard_cmd(I8042_SCANCODE_SET_1);
On Wed, Jun 5, 2019 at 8:34 AM Martin Kepplinger martink@posteo.de wrote:
Am 04.06.2019 13:59 schrieb Paul Menzel:
Dear coreboot folks,
On 06/04/19 12:15, Paul Menzel wrote:
On 06/04/19 12:01, Paul Menzel wrote:
On 06/04/19 07:10, Martin Kepplinger wrote:
what's wrong? I think commit 7ae606f57f0b3d450ae748141b0e2367041b27d3 Paul?
Did you test if everything works, if you revert the commit?
Did the PS/2 keyboard work before when loading libpayload based payloads from SeaBIOS or GRUB (chainloader)?
I’ll submit a fix to check true/false.
Furquan created a fix [1] and it was accepted and is now in the master branch.
Please verify, if it works for you now.
Kind regards,
Paul
Testing that on the X230, I get:
"ERROR: Keyboard set scancode failed!" and keyboard does not work at all, in said payloads.
Also, reverting https://review.coreboot.org/c/coreboot/+/32951/ ( 7ae606f57f0b3d450ae748141b0e2367041b27d3 ) fixes the problem.
Please fix this. I also think we can not return early when "RESET" fails. Why not just (try to) reset silently, if possible?
thanks,
martin
I can test later but I think I can see what happens. It fixes the issue, but still prints "ERROR: Keyboard set scancode failed!".
If we know that it's no error on at least one platform, we shouldn't print "ERROR" IMO, but as long as the bug gets fixes, I guess I'm fine.
thanks martin
On 05.06.19 07:53, Mike Banon wrote:
Martin, could you please check what happens if you apply this patch but then remove a return that follows ""ERROR: Keyboard set scancode failed!" ? Perhaps we'll have to do the same for all these returns : "Still print this message but don't abort prematurely"
Signed-off-by: Mike Banon <mikebdp2 at gmail.com>
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 240385ce6d..c81392ab72 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -328,7 +328,6 @@ void keyboard_init(void) ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE); if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) { printf("ERROR: Keyboard set scancode failed!\n");
return;}
ret = keyboard_cmd(I8042_SCANCODE_SET_1);
On Wed, Jun 5, 2019 at 8:34 AM Martin Kepplinger martink@posteo.de wrote:
Am 04.06.2019 13:59 schrieb Paul Menzel:
Dear coreboot folks,
On 06/04/19 12:15, Paul Menzel wrote:
On 06/04/19 12:01, Paul Menzel wrote:
On 06/04/19 07:10, Martin Kepplinger wrote:
what's wrong? I think commit 7ae606f57f0b3d450ae748141b0e2367041b27d3 Paul?
Did you test if everything works, if you revert the commit?
Did the PS/2 keyboard work before when loading libpayload based payloads from SeaBIOS or GRUB (chainloader)?
I’ll submit a fix to check true/false.
Furquan created a fix [1] and it was accepted and is now in the master branch.
Please verify, if it works for you now.
Kind regards,
Paul
Testing that on the X230, I get:
"ERROR: Keyboard set scancode failed!" and keyboard does not work at all, in said payloads.
Also, reverting https://review.coreboot.org/c/coreboot/+/32951/ ( 7ae606f57f0b3d450ae748141b0e2367041b27d3 ) fixes the problem.
Please fix this. I also think we can not return early when "RESET" fails. Why not just (try to) reset silently, if possible?
thanks,
martin
Paul, what do you think about removing the return and error message from the RESET cmd? Other thoughts?
thanks martin
Am 05.06.2019 07:58 schrieb Martin Kepplinger:
I can test later but I think I can see what happens. It fixes the issue, but still prints "ERROR: Keyboard set scancode failed!".
If we know that it's no error on at least one platform, we shouldn't print "ERROR" IMO, but as long as the bug gets fixes, I guess I'm fine.
thanks martin
On 05.06.19 07:53, Mike Banon wrote:
Martin, could you please check what happens if you apply this patch but then remove a return that follows ""ERROR: Keyboard set scancode failed!" ? Perhaps we'll have to do the same for all these returns : "Still print this message but don't abort prematurely"
Signed-off-by: Mike Banon <mikebdp2 at gmail.com>
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 240385ce6d..c81392ab72 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -328,7 +328,6 @@ void keyboard_init(void) ret = keyboard_cmd(I8042_KBCMD_SET_SCANCODE); if (!ret && !CONFIG(LP_PC_KEYBOARD_IGNORE_INIT_FAILURE)) { printf("ERROR: Keyboard set scancode failed!\n");
return;}
ret = keyboard_cmd(I8042_SCANCODE_SET_1);
On Wed, Jun 5, 2019 at 8:34 AM Martin Kepplinger martink@posteo.de wrote:
Am 04.06.2019 13:59 schrieb Paul Menzel:
Dear coreboot folks,
On 06/04/19 12:15, Paul Menzel wrote:
On 06/04/19 12:01, Paul Menzel wrote:
On 06/04/19 07:10, Martin Kepplinger wrote:
> what's wrong? I think commit > 7ae606f57f0b3d450ae748141b0e2367041b27d3 > Paul?
Did you test if everything works, if you revert the commit?
Did the PS/2 keyboard work before when loading libpayload based payloads from SeaBIOS or GRUB (chainloader)?
I’ll submit a fix to check true/false.
Furquan created a fix [1] and it was accepted and is now in the master branch.
Please verify, if it works for you now.
Kind regards,
Paul
Testing that on the X230, I get:
"ERROR: Keyboard set scancode failed!" and keyboard does not work at all, in said payloads.
Also, reverting https://review.coreboot.org/c/coreboot/+/32951/ ( 7ae606f57f0b3d450ae748141b0e2367041b27d3 ) fixes the problem.
Please fix this. I also think we can not return early when "RESET" fails. Why not just (try to) reset silently, if possible?
thanks,
martin
Dear Martin,
First, please try to use interleaved style for quoting.
On 05.06.19 13:47, Martin Kepplinger wrote:
Am 05.06.2019 07:58 schrieb Martin Kepplinger:
I can test later but I think I can see what happens. It fixes the issue, but still prints "ERROR: Keyboard set scancode failed!".
If we know that it's no error on at least one platform, we shouldn't print "ERROR" IMO, but as long as the bug gets fixes, I guess I'm fine.
It’s how it was done before, and I think it is useful to have error messages especially in case it aborts. Just to be sure, is the message
ERROR: Keyboard set scancode failed!
also new for you, or did you see it in the past before the change resetting the keyboard, but the keyboard kept working. (Maybe with no reset it was still working because of the initialization by SeaBIOS or GRUB.)
Paul, what do you think about removing the return and error message from the RESET cmd? Other thoughts?
Your error is not about the failing reset command anymore.
You should be ablet to work around it by selecting LP_PC_KEYBOARD_IGNORE_INIT_FAILURE in the libpayload configuration.
Maybe that should be selected by default or we should indeed remove all the returns from the error paths in the hope the keyboard will be functional despite the non-working commands before.
Kind regards,
Paul
On 06.06.19 08:20, Paul Menzel wrote:
Dear Martin,
First, please try to use interleaved style for quoting.
On 05.06.19 13:47, Martin Kepplinger wrote:
Am 05.06.2019 07:58 schrieb Martin Kepplinger:
I can test later but I think I can see what happens. It fixes the issue, but still prints "ERROR: Keyboard set scancode failed!".
If we know that it's no error on at least one platform, we shouldn't print "ERROR" IMO, but as long as the bug gets fixes, I guess I'm fine.
It’s how it was done before, and I think it is useful to have error messages especially in case it aborts. Just to be sure, is the message
ERROR: Keyboard set scancode failed!
also new for you, or did you see it in the past before the change resetting the keyboard, but the keyboard kept working. (Maybe with no reset it was still working because of the initialization by SeaBIOS or GRUB.)
it's new. Before the change that adds the reset, all was fine and no errors printed.
Paul, what do you think about removing the return and error message from the RESET cmd? Other thoughts?
Your error is not about the failing reset command anymore.
You should be ablet to work around it by selecting LP_PC_KEYBOARD_IGNORE_INIT_FAILURE in the libpayload configuration.
how do I select that?
Maybe that should be selected by default or we should indeed remove all the returns from the error paths in the hope the keyboard will be functional despite the non-working commands before.
We must add a fix. If you can select said config by default for libpayload users, that's fine. Otherwise, remove the return I guess...
thanks, martin
Am 05.06.2019 07:58 schrieb Martin Kepplinger:
I can test later but I think I can see what happens. It fixes the issue, but still prints "ERROR: Keyboard set scancode failed!".
There's also a chance that it'd prematurely "return;" slightly later - in this case my tiny patch wouldn't fix the issue and commenting out the another return would be also required
On Thu, Jun 6, 2019 at 9:50 AM Martin Kepplinger martink@posteo.de wrote:
On 06.06.19 08:20, Paul Menzel wrote:
Dear Martin,
First, please try to use interleaved style for quoting.
On 05.06.19 13:47, Martin Kepplinger wrote:
Am 05.06.2019 07:58 schrieb Martin Kepplinger:
I can test later but I think I can see what happens. It fixes the issue, but still prints "ERROR: Keyboard set scancode failed!".
If we know that it's no error on at least one platform, we shouldn't print "ERROR" IMO, but as long as the bug gets fixes, I guess I'm fine.
It’s how it was done before, and I think it is useful to have error messages especially in case it aborts. Just to be sure, is the message
ERROR: Keyboard set scancode failed!also new for you, or did you see it in the past before the change resetting the keyboard, but the keyboard kept working. (Maybe with no reset it was still working because of the initialization by SeaBIOS or GRUB.)
it's new. Before the change that adds the reset, all was fine and no errors printed.
Paul, what do you think about removing the return and error message from the RESET cmd? Other thoughts?
Your error is not about the failing reset command anymore.
You should be ablet to work around it by selecting LP_PC_KEYBOARD_IGNORE_INIT_FAILURE in the libpayload configuration.
how do I select that?
Maybe that should be selected by default or we should indeed remove all the returns from the error paths in the hope the keyboard will be functional despite the non-working commands before.
We must add a fix. If you can select said config by default for libpayload users, that's fine. Otherwise, remove the return I guess...
thanks, martin
Hi,
I've send a revert for review: https://review.coreboot.org/c/coreboot/+/33244
It seems to me that the added reset isn't following the protocol (i.e. ignores another returned byte). I'm not familiar with these controllers and lack the time to look into it right now. So that's the best I can do atm.
In the long run, I think we might want to replicate Linux' i8042 driver behavior in _all_ payloads. Having a common implementation in the payloads and OS would considerably decrease the necessary testing and provide a cleaner cut what has to be done in coreboot and what in the payload.
Nico
-
Joel Kitching -
Martin Kepplinger -
Mike Banon -
Nico Huber -
Paul Menzel