From 737c9b6258c6e68714ae264ff36126eb5d382d6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=98yvind=20Harboe?= Date: Wed, 5 May 2010 15:08:34 +0200 Subject: [PATCH] flash: stop caching protection state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- doc/openocd.texi | 13 +----- src/flash/nor/cfi.c | 11 +++++ src/flash/nor/core.c | 100 +++++-------------------------------------- src/flash/nor/core.h | 7 +-- src/flash/nor/tcl.c | 39 +++-------------- src/target/target.c | 13 ------ 6 files changed, 33 insertions(+), 150 deletions(-) diff --git a/doc/openocd.texi b/doc/openocd.texi index d311c8eee..a4c4de292 100644 --- a/doc/openocd.texi +++ b/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 -incorrect (outdated) unless you first issue a -@command{flash protect_check num} command. +This command will first query the hardware, it does not print cached +and possibly stale information. @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, diff --git a/src/flash/nor/cfi.c b/src/flash/nor/cfi.c index 92b553b54..4ef41b9ad 100644 --- a/src/flash/nor/cfi.c +++ b/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) diff --git a/src/flash/nor/core.c b/src/flash/nor/core.c index e07ca1f64..936f07ca6 100644 --- a/src/flash/nor/core.c +++ b/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; - /* - * Filter out what trivial nonsense we can, so drivers don't have to. + /* DANGER! * - * Don't tell drivers to change to the current state... it's needless, - * 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. + * We must not use any cached information about protection state!!!! * - * FIXME repeating the "is_protected==set" test is a giveaway that - * this fast-exit belongs earlier, in the trim-it-down loop; mve. - * */ - if (first == last && bank->sectors[first].is_protected == set) - return ERROR_OK; - - - /* Note that we don't pass illegal parameters to drivers; any - * trimming just turns one valid range into another one. + * There are a million things that could change the protect state: + * + * the target could have reset, power cycled, been hot plugged, + * the application could have run, etc. + * + * Drivers only receive valid sector range. */ 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; - } - } -} diff --git a/src/flash/nor/core.h b/src/flash/nor/core.h index b15267744..1dfd721be 100644 --- a/src/flash/nor/core.h +++ b/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. diff --git a/src/flash/nor/tcl.c b/src/flash/nor/tcl.c index 947fd046b..17c6e9103 100644 --- a/src/flash/nor/tcl.c +++ b/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, diff --git a/src/target/target.c b/src/target/target.c index d17bb7445..37e515a64 100644 --- a/src/target/target.c +++ b/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; }