firmware: ui: Unify delays, remove DISK/KEY split in recovery mode

The reason for having a separate REC_DISK_DELAY and REC_KEY_DELAY has
long been lost in time... in fact, with our current coreboot firmware
stack, polling for keys will always also poll for disks, so we're
already polling for disks in the inner loop anyway. Removing this
distinction will resolve some weirdness in certain error cases.

An unintended side effect is that the usual recovery mode console spam
goes from annoying at 4 times a second to unbearable at 50 times a
second. Let's just remove it instead and get the console output more in
line with what our developer and BROKEN screens show (i.e. nothing,
unless there's any change or user input).

BRANCH=None
BUG=chromium:1009850
TEST=Booted Kevin

Change-Id: Ie1754646e7d17a661c9adebf43483df1785e6127
Signed-off-by: Julius Werner <jwerner@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1856831
Reviewed-by: Joel Kitching <kitching@chromium.org>
(cherry picked from commit 5caaa393b4345ea09e2bf4e336d2c30b80376eab)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1902830
Reviewed-by: Hung-Te Lin <hungte@chromium.org>
Commit-Queue: Hung-Te Lin <hungte@chromium.org>
Tested-by: Hung-Te Lin <hungte@chromium.org>
Auto-Submit: Hung-Te Lin <hungte@chromium.org>

5 files changed, 77 insertions, 123 deletions

diff --git a/firmware/lib/include/vboot_ui_common.h b/firmware/lib/include/vboot_ui_common.h
index 17eb8151..f4ebe530 100644
--- a/firmware/lib/include/vboot_ui_common.h
+++ b/firmware/lib/include/vboot_ui_common.h
@@ -8,6 +8,8 @@
 #ifndef VBOOT_REFERENCE_VBOOT_UI_COMMON_H_
 #define VBOOT_REFERENCE_VBOOT_UI_COMMON_H_
 
+#define KEY_DELAY_MS	20	/* Delay between key scans in UI loops */
+
 enum vb2_beep_type {
 	VB_BEEP_FAILED,		/* Permitted but the operation failed */
 	VB_BEEP_NOT_ALLOWED,	/* Operation disabled by user setting */
diff --git a/firmware/lib/vboot_api_kernel.c b/firmware/lib/vboot_api_kernel.c
index 659cb07c..95507665 100644
--- a/firmware/lib/vboot_api_kernel.c
+++ b/firmware/lib/vboot_api_kernel.c
@@ -62,9 +62,6 @@ vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t get_info_flags)
 	uint32_t disk_count = 0;
 	uint32_t i;
 
-	VB2_DEBUG("VbTryLoadKernel() start, get_info_flags=0x%x\n",
-		  (unsigned)get_info_flags);
-
 	lkp.fwmp = &fwmp;
 	lkp.disk_handle = NULL;
 
@@ -73,8 +70,6 @@ vb2_error_t VbTryLoadKernel(struct vb2_context *ctx, uint32_t get_info_flags)
 					   get_info_flags))
 		disk_count = 0;
 
-	VB2_DEBUG("VbTryLoadKernel() found %d disks\n", (int)disk_count);
-
 	/* Loop over disks */
 	for (i = 0; i < disk_count; i++) {
 		VB2_DEBUG("trying disk %d\n", (int)i);
diff --git a/firmware/lib/vboot_ui.c b/firmware/lib/vboot_ui.c
index cae29ec8..fd61503f 100644
--- a/firmware/lib/vboot_ui.c
+++ b/firmware/lib/vboot_ui.c
@@ -95,8 +95,6 @@ static vb2_error_t VbTryUsb(struct vb2_context *ctx)
 	return retval;
 }
 
-#define CONFIRM_KEY_DELAY 20  /* Check confirm screen keys every 20ms */
-
 int VbUserConfirms(struct vb2_context *ctx, uint32_t confirm_flags)
 {
 	struct vb2_shared_data *sd = vb2_get_sd(ctx);
@@ -161,15 +159,12 @@ int VbUserConfirms(struct vb2_context *ctx, uint32_t confirm_flags)
 			}
 			VbCheckDisplayKey(ctx, key, NULL);
 		}
-		VbExSleepMs(CONFIRM_KEY_DELAY);
+		VbExSleepMs(KEY_DELAY_MS);
 	} while (!shutdown_requested);
 
 	return -1;
 }
 
-/* Delay in developer ui */
-#define DEV_KEY_DELAY        20       /* Check keys every 20ms */
-
 /*
  * User interface for selecting alternative firmware
  *
@@ -217,7 +212,7 @@ static vb2_error_t vb2_altfw_ui(struct vb2_context *ctx)
 			VbCheckDisplayKey(ctx, key, NULL);
 			break;
 		}
-		VbExSleepMs(DEV_KEY_DELAY);
+		VbExSleepMs(KEY_DELAY_MS);
 	} while (active);
 
 	/* Back to developer screen */
@@ -306,7 +301,7 @@ static vb2_error_t vb2_enter_vendor_data_ui(struct vb2_context *ctx,
 			VbCheckDisplayKey(ctx, key, &data);
 			break;
 		}
-		VbExSleepMs(DEV_KEY_DELAY);
+		VbExSleepMs(KEY_DELAY_MS);
 	} while (1);
 
 	return VB2_SUCCESS;
@@ -373,7 +368,7 @@ static vb2_error_t vb2_vendor_data_ui(struct vb2_context *ctx)
 			VbCheckDisplayKey(ctx, key, &data);
 			break;
 		}
-		VbExSleepMs(DEV_KEY_DELAY);
+		VbExSleepMs(KEY_DELAY_MS);
 	} while (1);
 
 	return VB2_SUCCESS;
@@ -463,7 +458,7 @@ static vb2_error_t vb2_diagnostics_ui(struct vb2_context *ctx)
 			break;
 		}
 		if (active) {
-			VbExSleepMs(DEV_KEY_DELAY);
+			VbExSleepMs(KEY_DELAY_MS);
 		}
 	} while (active);
 
@@ -750,7 +745,7 @@ static vb2_error_t vb2_developer_ui(struct vb2_context *ctx)
 			break;
 		}
 
-		VbExSleepMs(DEV_KEY_DELAY);
+		VbExSleepMs(KEY_DELAY_MS);
 	} while(vb2_audio_looping());
 
  fallout:
@@ -788,18 +783,12 @@ vb2_error_t VbBootDiagnostic(struct vb2_context *ctx)
 	return retval;
 }
 
-/* Delay in recovery mode */
-#define REC_DISK_DELAY       1000     /* Check disks every 1s */
-#define REC_KEY_DELAY        20       /* Check keys every 20ms */
-#define REC_MEDIA_INIT_DELAY 500      /* Check removable media every 500ms */
-
 static vb2_error_t recovery_ui(struct vb2_context *ctx)
 {
 	struct vb2_shared_data *sd = vb2_get_sd(ctx);
 	VbSharedDataHeader *shared = sd->vbsd;
 	uint32_t retval;
 	uint32_t key;
-	int i;
 	const char release_button_msg[] =
 		"Release the recovery button and try again\n";
 	const char recovery_pressed_msg[] =
@@ -837,14 +826,13 @@ static vb2_error_t recovery_ui(struct vb2_context *ctx)
 				  vb2_check_diagnostic_key(ctx, key)) !=
 				  VB2_SUCCESS)
 				return retval;
-			VbExSleepMs(REC_KEY_DELAY);
+			VbExSleepMs(KEY_DELAY_MS);
 		}
 	}
 
 	/* Loop and wait for a recovery image */
 	VB2_DEBUG("VbBootRecovery() waiting for a recovery image\n");
 	while (1) {
-		VB2_DEBUG("VbBootRecovery() attempting to load kernel2\n");
 		retval = VbTryLoadKernel(ctx, VB_DISK_FLAG_REMOVABLE);
 
 		if (VB2_SUCCESS == retval)
@@ -855,80 +843,63 @@ static vb2_error_t recovery_ui(struct vb2_context *ctx)
 				VB_SCREEN_RECOVERY_NO_GOOD,
 				0, NULL);
 
+		key = VbExKeyboardRead();
 		/*
-		 * Scan keyboard more frequently than media, since x86
-		 * platforms don't like to scan USB too rapidly.
+		 * We might want to enter dev-mode from the Insert
+		 * screen if all of the following are true:
+		 *   - user pressed Ctrl-D
+		 *   - we can honor the virtual dev switch
+		 *   - not already in dev mode
+		 *   - user forced recovery mode
 		 */
-		for (i = 0; i < REC_DISK_DELAY; i += REC_KEY_DELAY) {
-			key = VbExKeyboardRead();
-			/*
-			 * We might want to enter dev-mode from the Insert
-			 * screen if all of the following are true:
-			 *   - user pressed Ctrl-D
-			 *   - we can honor the virtual dev switch
-			 *   - not already in dev mode
-			 *   - user forced recovery mode
-			 */
-			if (key == VB_KEY_CTRL('D') &&
-			    !(shared->flags & VBSD_BOOT_DEV_SWITCH_ON) &&
-			    (shared->flags & VBSD_BOOT_REC_SWITCH_ON)) {
-				if (!(shared->flags &
-				      VBSD_BOOT_REC_SWITCH_VIRTUAL) &&
-				    VbExGetSwitches(
-				    VB_SWITCH_FLAG_PHYS_PRESENCE_PRESSED)) {
-					/*
-					 * Is the presence button stuck?  In
-					 * any case we don't like this.  Beep
-					 * and ignore.
-					 */
-					vb2_error_notify(release_button_msg,
-							 recovery_pressed_msg,
-							 VB_BEEP_NOT_ALLOWED);
-					continue;
-				}
-
-				/* Ask the user to confirm entering dev-mode */
-				VbDisplayScreen(ctx,
-						VB_SCREEN_RECOVERY_TO_DEV,
-						0, NULL);
-				/* SPACE means no... */
-				uint32_t vbc_flags =
-					VB_CONFIRM_SPACE_MEANS_NO |
-					VB_CONFIRM_MUST_TRUST_KEYBOARD;
-				switch (VbUserConfirms(ctx, vbc_flags)) {
-				case 1:
-					VB2_DEBUG("Enabling dev-mode...\n");
-					if (VB2_SUCCESS != SetVirtualDevMode(1))
-						return VBERROR_TPM_SET_BOOT_MODE_STATE;
-					VB2_DEBUG("Reboot so it will take "
-						  "effect\n");
-					if (VbExGetSwitches
-					    (VB_SWITCH_FLAG_ALLOW_USB_BOOT))
-						VbAllowUsbBoot(ctx);
-					return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
-				case -1:
-					VB2_DEBUG("Shutdown requested\n");
-					return VBERROR_SHUTDOWN_REQUESTED;
-				default: /* zero, actually */
-					VB2_DEBUG("Not enabling dev-mode\n");
-					/*
-					 * Jump out of the outer loop to
-					 * refresh the display quickly.
-					 */
-					i = 4;
-					break;
-				}
-			} else if ((retval =
-				    vb2_check_diagnostic_key(ctx, key)) !=
-				    VB2_SUCCESS) {
-				return retval;
-			} else {
-				VbCheckDisplayKey(ctx, key, NULL);
+		if (key == VB_KEY_CTRL('D') &&
+		    !(shared->flags & VBSD_BOOT_DEV_SWITCH_ON) &&
+		    (shared->flags & VBSD_BOOT_REC_SWITCH_ON)) {
+			if (!(shared->flags & VBSD_BOOT_REC_SWITCH_VIRTUAL) &&
+			    VbExGetSwitches(
+					VB_SWITCH_FLAG_PHYS_PRESENCE_PRESSED)) {
+				/*
+				 * Is the presence button stuck?  In any case
+				 * we don't like this.  Beep and ignore.
+				 */
+				vb2_error_notify(release_button_msg,
+						 recovery_pressed_msg,
+						 VB_BEEP_NOT_ALLOWED);
+				continue;
 			}
-			if (VbWantShutdown(ctx, key))
+
+			/* Ask the user to confirm entering dev-mode */
+			VbDisplayScreen(ctx, VB_SCREEN_RECOVERY_TO_DEV,
+					0, NULL);
+			/* SPACE means no... */
+			uint32_t vbc_flags = VB_CONFIRM_SPACE_MEANS_NO |
+					     VB_CONFIRM_MUST_TRUST_KEYBOARD;
+			switch (VbUserConfirms(ctx, vbc_flags)) {
+			case 1:
+				VB2_DEBUG("Enabling dev-mode...\n");
+				if (VB2_SUCCESS != SetVirtualDevMode(1))
+					return VBERROR_TPM_SET_BOOT_MODE_STATE;
+				VB2_DEBUG("Reboot so it will take effect\n");
+				if (VbExGetSwitches
+				    (VB_SWITCH_FLAG_ALLOW_USB_BOOT))
+					VbAllowUsbBoot(ctx);
+				return VBERROR_EC_REBOOT_TO_RO_REQUIRED;
+			case -1:
+				VB2_DEBUG("Shutdown requested\n");
 				return VBERROR_SHUTDOWN_REQUESTED;
-			VbExSleepMs(REC_KEY_DELAY);
+			default: /* zero, actually */
+				VB2_DEBUG("Not enabling dev-mode\n");
+				break;
+			}
+		} else if ((retval = vb2_check_diagnostic_key(ctx, key)) !=
+			   VB2_SUCCESS) {
+			return retval;
+		} else {
+			VbCheckDisplayKey(ctx, key, NULL);
 		}
+		if (VbWantShutdown(ctx, key))
+			return VBERROR_SHUTDOWN_REQUESTED;
+		VbExSleepMs(KEY_DELAY_MS);
 	}
 
 	return VB2_SUCCESS;
diff --git a/firmware/lib/vboot_ui_menu.c b/firmware/lib/vboot_ui_menu.c
index d1808b90..b932ce9e 100644
--- a/firmware/lib/vboot_ui_menu.c
+++ b/firmware/lib/vboot_ui_menu.c
@@ -500,9 +500,6 @@ static vb2_error_t vb2_handle_menu_input(struct vb2_context *ctx,
 	return VBERROR_KEEP_LOOPING;
 }
 
-/* Delay in developer menu */
-#define DEV_KEY_DELAY        20       /* Check keys every 20ms */
-
 /* Master table of all menus. Menus with size == 0 count as menuless screens. */
 static struct vb2_menu menus[VB_MENU_COUNT] = {
 	[VB_MENU_DEV_WARNING] = {
@@ -815,7 +812,7 @@ static vb2_error_t vb2_developer_menu(struct vb2_context *ctx)
 		if (key != 0)
 			vb2_audio_start(ctx);
 
-		VbExSleepMs(DEV_KEY_DELAY);
+		VbExSleepMs(KEY_DELAY_MS);
 
 		/* If dev mode was disabled, loop forever (never timeout) */
 	} while (disable_dev_boot ? 1 : vb2_audio_looping());
@@ -867,11 +864,6 @@ static vb2_error_t broken_ui(struct vb2_context *ctx)
 	}
 }
 
-/* Delay in recovery mode */
-#define REC_DISK_DELAY       1000     /* Check disks every 1s */
-#define REC_KEY_DELAY        20       /* Check keys every 20ms */
-#define REC_MEDIA_INIT_DELAY 500      /* Check removable media every 500ms */
-
 /**
  * Main function that handles recovery menu functionality
  *
@@ -883,13 +875,11 @@ static vb2_error_t recovery_ui(struct vb2_context *ctx)
 	uint32_t key;
 	uint32_t key_flags;
 	vb2_error_t ret;
-	int i;
 
 	/* Loop and wait for a recovery image */
 	VB2_DEBUG("waiting for a recovery image\n");
 	usb_nogood = -1;
 	while (1) {
-		VB2_DEBUG("attempting to load kernel2\n");
 		ret = VbTryLoadKernel(ctx, VB_DISK_FLAG_REMOVABLE);
 
 		if (VB2_SUCCESS == ret)
@@ -901,25 +891,18 @@ static vb2_error_t recovery_ui(struct vb2_context *ctx)
 			enter_recovery_base_screen(ctx);
 		}
 
-		/*
-		 * Scan keyboard more frequently than media, since x86
-		 * platforms don't like to scan USB too rapidly.
-		 */
-		for (i = 0; i < REC_DISK_DELAY; i += REC_KEY_DELAY) {
-			key = VbExKeyboardReadWithFlags(&key_flags);
-			if (key == VB_BUTTON_VOL_UP_DOWN_COMBO_PRESS) {
-				if (key_flags & VB_KEY_FLAG_TRUSTED_KEYBOARD)
-					enter_to_dev_menu(ctx);
-				else
-					VB2_DEBUG("ERROR: untrusted combo?!\n");
-			} else {
-				ret = vb2_handle_menu_input(ctx, key,
-							    key_flags);
-				if (ret != VBERROR_KEEP_LOOPING)
-					return ret;
-			}
-			VbExSleepMs(REC_KEY_DELAY);
+		key = VbExKeyboardReadWithFlags(&key_flags);
+		if (key == VB_BUTTON_VOL_UP_DOWN_COMBO_PRESS) {
+			if (key_flags & VB_KEY_FLAG_TRUSTED_KEYBOARD)
+				enter_to_dev_menu(ctx);
+			else
+				VB2_DEBUG("ERROR: untrusted combo?!\n");
+		} else {
+			ret = vb2_handle_menu_input(ctx, key, key_flags);
+			if (ret != VBERROR_KEEP_LOOPING)
+				return ret;
 		}
+		VbExSleepMs(KEY_DELAY_MS);
 	}
 }
 
diff --git a/tests/vboot_detach_menu_tests.c b/tests/vboot_detach_menu_tests.c
index e6e1f4c2..ade3abd9 100644
--- a/tests/vboot_detach_menu_tests.c
+++ b/tests/vboot_detach_menu_tests.c
@@ -1808,7 +1808,10 @@ static void VbBootRecTest(void)
 	mock_keypress[2] = VB_BUTTON_VOL_DOWN_SHORT_PRESS; // language
 	mock_keypress[3] = VB_BUTTON_POWER_SHORT_PRESS;
 	vbtlk_retval[0] = VBERROR_NO_DISK_FOUND - VB_DISK_FLAG_REMOVABLE;
-	vbtlk_retval[1] = VB2_ERROR_MOCK - VB_DISK_FLAG_REMOVABLE;
+	vbtlk_retval[1] = VBERROR_NO_DISK_FOUND - VB_DISK_FLAG_REMOVABLE;
+	vbtlk_retval[2] = VBERROR_NO_DISK_FOUND - VB_DISK_FLAG_REMOVABLE;
+	vbtlk_retval[3] = VBERROR_NO_DISK_FOUND - VB_DISK_FLAG_REMOVABLE;
+	vbtlk_retval[4] = VB2_ERROR_MOCK - VB_DISK_FLAG_REMOVABLE;
 	TEST_EQ(VbBootRecoveryMenu(&ctx), VBERROR_SHUTDOWN_REQUESTED,
 		"Drop back to NOGOOD from LANGUAGE when inserting invalid USB");
 	TEST_EQ(shutdown_request_calls_left, 0, "  timed out");