flash: stop caching protection state

There are a million reasons why cached protection state might
be stale: power cycling of target, reset, code executing on
the target, etc.

The "flash protect_check" command is now gone. This is *always*
executed when running a "flash info".

As a bonus for more a more robust approach, lots of code could
be deleted.

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>

doc/openocd.texi

@@ -4129,9 +4129,8 @@ The @var{num} parameter is a value shown by @command{flash banks}.
 @deffn Command {flash info} num
 Print info about flash bank @var{num}
 The @var{num} parameter is a value shown by @command{flash banks}.
-The information includes per-sector protect status, which may be
+This command will first query the hardware, it does not print cached
-incorrect (outdated) unless you first issue a
+and possibly stale information.
-@command{flash protect_check num} command.
 @end deffn
 
 @anchor{flash protect}
@@ -4144,14 +4143,6 @@ specifies "to the end of the flash bank".
 The @var{num} parameter is a value shown by @command{flash banks}.
 @end deffn
 
-@deffn Command {flash protect_check} num
-Check protection state of sectors in flash bank @var{num}.
-The @var{num} parameter is a value shown by @command{flash banks}.
-@comment @option{flash erase_sector} using the same syntax.
-This updates the protection information displayed by @option{flash info}.
-(Code execution may have invalidated any state records kept by OpenOCD.)
-@end deffn
-
 @anchor{Flash Driver List}
 @section Flash Driver List
 As noted above, the @command{flash bank} command requires a driver name,

src/flash/nor/cfi.c

@@ -852,6 +852,17 @@ static int cfi_intel_protect(struct flash_bank *bank, int set, int first, int la
 	 */
 	if ((!set) && (!(pri_ext->feature_support & 0x20)))
 	{
+		/* FIX!!! this code path is broken!!!
+		 *
+		 * The correct approach is:
+		 *
+		 * 1. read out current protection status
+		 *
+		 * 2. override read out protection status w/unprotected.
+		 *
+		 * 3. re-protect what should be protected.
+		 *
+		 */
 		for (i = 0; i < bank->num_sectors; i++)
 		{
 			if (bank->sectors[i].is_protected == 1)

src/flash/nor/core.c

@@ -54,74 +54,27 @@ int flash_driver_erase(struct flash_bank *bank, int first, int last)
 int flash_driver_protect(struct flash_bank *bank, int set, int first, int last)
 {
 	int retval;
-	bool updated = false;
-
-	/* NOTE: "first == last" means (un?)protect just that sector.
-	 code including Lower level ddrivers may rely on this "first <= last"
-	 * invariant.
-	*/
 
 	/* callers may not supply illegal parameters ... */
 	if (first < 0 || first > last || last >= bank->num_sectors)
+	{
+		LOG_ERROR("illegal sector range");
 		return ERROR_FAIL;
+	}
 
 	/* force "set" to 0/1 */
 	set = !!set;
 
-	/*
+	/* DANGER!
-	 * Filter out what trivial nonsense we can, so drivers don't have to.
 	 *
-	 * Don't tell drivers to change to the current state...  it's needless,
+	 * We must not use any cached information about protection state!!!!
-	 * and reducing the amount of work to be done (potentially to nothing)
-	 * speeds at least some things up.
-	 */
-scan:
-	for (int i = first; i <= last; i++) {
-		struct flash_sector *sector = bank->sectors + i;
-
-		/* Only filter requests to protect the already-protected, or
-		 * to unprotect the already-unprotected.  Changing from the
-		 * unknown state (-1) to a known one is unwise but allowed;
-		 * protection status is best checked first.
-		 */
-		if (sector->is_protected != set)
-			continue;
-
-		/* Shrink this range of sectors from the start; don't overrun
-		 * the end.  Also shrink from the end; don't overun the start.
-		 *
-		 * REVISIT we could handle discontiguous regions by issuing
-		 * more than one driver request.  How much would that matter?
-		 */
-		if (i == first && i != last) {
-			updated = true;
-			first++;
-		} else if (i == last && i != first) {
-			updated = true;
-			last--;
-		}
-	}
-
-	/* updating the range affects the tests in the scan loop above; so
-	 * re-scan, to make sure we didn't miss anything.
-	 */
-	if (updated) {
-		updated = false;
-		goto scan;
-	}
-
-	/* Single sector, already protected?  Nothing to do!
-	 * We may have trimmed our parameters into this degenerate case.
 	 *
-	 * FIXME repeating the "is_protected==set" test is a giveaway that
+	 * There are a million things that could change the protect state:
-	 * this fast-exit belongs earlier, in the trim-it-down loop; mve.
+	 *
-	 * */
+	 * the target could have reset, power cycled, been hot plugged,
-	if (first == last && bank->sectors[first].is_protected == set)
+	 * the application could have run, etc.
-		return ERROR_OK;
+	 *
-
+	 * Drivers only receive valid sector range.
-
-	/* Note that we don't pass illegal parameters to drivers; any
-	 * trimming just turns one valid range into another one.
 	 */
 	retval = bank->driver->protect(bank, set, first, last);
 	if (retval != ERROR_OK)
@@ -754,34 +707,3 @@ int flash_write(struct target *target, struct image *image,
 {
 	return flash_write_unlock(target, image, written, erase, false);
 }
-
-/**
- * Invalidates cached flash state which a target can change as it runs.
- *
- * @param target The target being resumed
- *
- * OpenOCD caches some flash state for brief periods.  For example, a sector
- * that is protected must be unprotected before OpenOCD tries to write it,
- * Also, a sector that's not erased must be erased before it's written.
- *
- * As a rule, OpenOCD and target firmware can both modify the flash, so when
- * a target starts running, OpenOCD needs to invalidate its cached state.
- */
-void nor_resume(struct target *target)
-{
-	struct flash_bank *bank;
-
-	for (bank = flash_banks; bank; bank = bank->next) {
-		int i;
-
-		if (bank->target != target)
-			continue;
-
-		for (i = 0; i < bank->num_sectors; i++) {
-			struct flash_sector *sector = bank->sectors + i;
-
-			sector->is_erased = -1;
-			sector->is_protected = -1;
-		}
-	}
-}

src/flash/nor/core.h

@@ -53,6 +53,10 @@ struct flash_sector
 	 * Indication of protection status: 0 = unprotected/unlocked,
 	 * 1 = protected/locked, other = unknown.  Set by
 	 * @c flash_driver_s::protect_check.
+	 *
+	 * This information must be considered stale immediately.
+	 * A million things could make it stale: power cycle,
+	 * reset of target, code running on target, etc.
 	 */
 	int is_protected;
 };
@@ -124,9 +128,6 @@ int flash_unlock_address_range(struct target *target, uint32_t addr,
 int flash_write(struct target *target,
 		struct image *image, uint32_t *written, int erase);
 
-/* invalidate cached state (targets may modify their own flash) */
-void nor_resume(struct target *target);
-
 /**
  * Forces targets to re-examine their erase/protection state.
  * This routine must be called when the system may modify the status.

src/flash/nor/tcl.c

@@ -70,6 +70,11 @@ COMMAND_HANDLER(handle_flash_info_command)
 		if ((retval = p->driver->auto_probe(p)) != ERROR_OK)
 			return retval;
 
+		/* We must query the hardware to avoid printing stale information! */
+		retval = p->driver->protect_check(p);
+		if (retval != ERROR_OK)
+			return retval;
+
 		command_print(CMD_CTX,
 			      "#%" PRIi32 " : %s at 0x%8.8" PRIx32 ", size 0x%8.8" PRIx32 ", buswidth %i, chipwidth %i",
 			      i,
@@ -266,32 +271,6 @@ COMMAND_HANDLER(handle_flash_erase_address_command)
 	return retval;
 }
 
-COMMAND_HANDLER(handle_flash_protect_check_command)
-{
-	if (CMD_ARGC != 1)
-		return ERROR_COMMAND_SYNTAX_ERROR;
-
-	struct flash_bank *p;
-	int retval = CALL_COMMAND_HANDLER(flash_command_get_bank, 0, &p);
-	if (ERROR_OK != retval)
-		return retval;
-
-	if ((retval = p->driver->protect_check(p)) == ERROR_OK)
-	{
-		command_print(CMD_CTX, "successfully checked protect state");
-	}
-	else if (retval == ERROR_FLASH_OPERATION_FAILED)
-	{
-		command_print(CMD_CTX, "checking protection state failed (possibly unsupported) by flash #%s at 0x%8.8" PRIx32, CMD_ARGV[0], p->base);
-	}
-	else
-	{
-		command_print(CMD_CTX, "unknown error when checking protection state of flash bank '#%s' at 0x%8.8" PRIx32, CMD_ARGV[0], p->base);
-	}
-
-	return ERROR_OK;
-}
-
 static int flash_check_sector_parameters(struct command_context *cmd_ctx,
 		uint32_t first, uint32_t last, uint32_t num_sectors)
 {
@@ -706,14 +685,6 @@ static const struct command_registration flash_exec_command_handlers[] = {
 		.help = "Check erase state of all blocks in a "
 			"flash bank.",
 	},
-	{
-		.name = "protect_check",
-		.handler = handle_flash_protect_check_command,
-		.mode = COMMAND_EXEC,
-		.usage = "bank_id",
-		.help = "Check protection state of all blocks in a "
-			"flash bank.",
-	},
 	{
 		.name = "erase_sector",
 		.handler = handle_flash_erase_command,

src/target/target.c

@@ -474,19 +474,6 @@ int target_resume(struct target *target, int current, uint32_t address, int hand
 	if ((retval = target->type->resume(target, current, address, handle_breakpoints, debug_execution)) != ERROR_OK)
 		return retval;
 
-	/* Invalidate any cached protect/erase/... flash status, since
-	 * almost all targets will now be able modify the flash by
-	 * themselves.  We want flash drivers and infrastructure to
-	 * be able to rely on (non-invalidated) cached state.
-	 *
-	 * For now we require that algorithms provided by OpenOCD are
-	 * used only by code which properly maintains that  cached state.
-	 * state
-	 *
-	 * REVISIT do the same for NAND ; maybe other flash flavors too...
-	 */
-		if (!target->running_alg)
-		nor_resume(target);
 	return retval;
 }