Re: [flashrom] [PATCH] Fix handling of write protection at register space address +2.

[Stefan Tauner]

On Wed, 10 Dec 2014 21:24:27 +0400
Roman Lebedev  wrote:
> Invalid locking change 0xf8 requested at 0xb7323002! Please report a bug at
> [e-mail]

Thanks for testing Roman!

Carl-Daniel, do you have any idea why we do the "new_bits & ~REG2_MASK"
in changelock_regspace2_block() in jedec.c at all?
It is essentially your code, c.f.
http://patchwork.coreboot.org/patch/3412/

I propose to use "(old ^ new) & ~REG2_MASK" instead, i.e. check if the
caller requests a change of bits not related to the write protection. I
guess that was your original intention anyway, is that correct?

[PATCH] Fix handling of write protection at register space address +2.

[Stefan Tauner]

Previously we added the offset of the virtual register in several
functions cumulatively, which produced segfaults. This patch renames
a few parameters hence the rather big diff.

Thanks to Roman Lebedev for reporting this issue and testing the patch.

Signed-off-by: Stefan Tauner 
---

Roman, if you got some time could you please retest this refined version
of the patch (it is against r1857)? It should behave pretty much the same
on your setup apart from not showing the message you saw with the last
version.

chipdrivers.h |  1 -
 jedec.c       | 65 +++++++++++++++++++++++++++--------------------------------
 2 files changed, 30 insertions(+), 36 deletions(-)

diff --git a/chipdrivers.h b/chipdrivers.h
index 8529c74..cac94f3 100644
--- a/chipdrivers.h
+++ b/chipdrivers.h
@@ -150,7 +150,6 @@ int unlock_regspace2_uniform_32k(struct flashctx *flash);
 int unlock_regspace2_uniform_64k(struct flashctx *flash);
 int unlock_regspace2_block_eraser_0(struct flashctx *flash);
 int unlock_regspace2_block_eraser_1(struct flashctx *flash);
-int unlock_regspace2_block(const struct flashctx *flash, chipaddr off);
 int printlock_regspace2_uniform_64k(struct flashctx *flash);
 int printlock_regspace2_block_eraser_0(struct flashctx *flash);
 int printlock_regspace2_block_eraser_1(struct flashctx *flash);
diff --git a/jedec.c b/jedec.c
index 1345b89..6de6745 100644
--- a/jedec.c
+++ b/jedec.c
@@ -589,11 +589,10 @@ static int regspace2_walk_unlockblocks(const struct flashctx *flash, const struc
 #define REG2_LOCKDOWN (1 << 1)
 #define REG2_MASK (REG2_RWLOCK | REG2_LOCKDOWN)

-static int printlock_regspace2_block(const struct flashctx *flash, chipaddr offset)
+static int printlock_regspace2_block(const struct flashctx *flash, chipaddr lockreg)
 {
-	chipaddr wrprotect = flash->virtual_registers + offset + 2;
-	uint8_t state = chip_readb(flash, wrprotect);
-	msg_cdbg("Lock status of block at 0x%0*" PRIxPTR " is ", PRIxPTR_WIDTH, offset);
+	uint8_t state = chip_readb(flash, lockreg);
+	msg_cdbg("Lock status of block at 0x%0*" PRIxPTR " is ", PRIxPTR_WIDTH, lockreg);
    switch (state & REG2_MASK) {
    case 0:
    	msg_cdbg("Full Access.\n");
@@ -656,47 +655,44 @@ int printlock_regspace2_block_eraser_1(struct flashctx *flash)
    return regspace2_walk_unlockblocks(flash, unlockblocks, &printlock_regspace2_block);
 }

-static int changelock_regspace2_block(const struct flashctx *flash, chipaddr offset, uint8_t new_bits)
+static int changelock_regspace2_block(const struct flashctx *flash, chipaddr lockreg, uint8_t old, uint8_t new)
 {
-	chipaddr wrprotect = flash->virtual_registers + offset + 2;
-	uint8_t old;
-
-	if (new_bits & ~REG2_MASK) {
-		msg_cerr("Invalid locking change 0x%02x requested at 0x%0*" PRIxPTR "! "
+	if (((old ^ new) & REG2_MASK) != 0) {
+		msg_cerr("Invalid locking change from 0x%02x to 0x%02x requested at 0x%0*" PRIxPTR "!\n"
    		 "Please report a bug at [e-mail]\n",
-			 new_bits, PRIxPTR_WIDTH, offset);
+			 old, new, PRIxPTR_WIDTH, lockreg);
    	return -1;
    }
-	old = chip_readb(flash, wrprotect);
    /* Early exist if no change (of read/write/lockdown) was requested. */
-	if (((old ^ new_bits) & REG2_MASK) == 0) {
-		msg_cdbg2("Locking status at 0x%0*" PRIxPTR " not changed\n", PRIxPTR_WIDTH, offset);
+	if (((old ^ new) & REG2_MASK) == 0) {
+		msg_cdbg2("Locking status at 0x%0*" PRIxPTR " not changed\n", PRIxPTR_WIDTH, lockreg);
    	return 0;
    }
    /* Normally lockdowns can not be cleared. Try nevertheless if requested. */
-	if ((old & REG2_LOCKDOWN) && !(new_bits & REG2_LOCKDOWN)) {
-		chip_writeb(flash, old & ~REG2_LOCKDOWN, wrprotect);
-		if (chip_readb(flash, wrprotect) != (old & ~REG2_LOCKDOWN)) {
-			msg_cerr("Lockdown can't be removed at 0x%0*" PRIxPTR "!\n", PRIxPTR_WIDTH, offset);
+	if ((old & REG2_LOCKDOWN) && !(new & REG2_LOCKDOWN)) {
+		chip_writeb(flash, old & ~REG2_LOCKDOWN, lockreg);
+		if (chip_readb(flash, lockreg) != (old & ~REG2_LOCKDOWN)) {
+			msg_cerr("Lockdown can't be removed at 0x%0*" PRIxPTR "!\n", PRIxPTR_WIDTH, lockreg);
    		return -1;
    	}
    }
    /* Change read or write lock? */
-	if ((old ^ new_bits) & REG2_RWLOCK) {
+	if ((old ^ new) & REG2_RWLOCK) {
    	/* Do not lockdown yet. */
-		msg_cdbg("Changing locking status at 0x%0*" PRIxPTR " to 0x%02x\n", PRIxPTR_WIDTH, offset, new_bits & REG2_RWLOCK);
-		chip_writeb(flash, new_bits & REG2_RWLOCK, wrprotect);
-		if (chip_readb(flash, wrprotect) != (new_bits & REG2_RWLOCK)) {
-			msg_cerr("Locking status change FAILED at 0x%0*" PRIxPTR "!\n", PRIxPTR_WIDTH, offset);
+		msg_cdbg("Changing locking status at 0x%0*" PRIxPTR " to 0x%02x\n",
+			 PRIxPTR_WIDTH, lockreg, new & REG2_RWLOCK);
+		chip_writeb(flash, new & REG2_RWLOCK, lockreg);
+		if (chip_readb(flash, lockreg) != (new & REG2_RWLOCK)) {
+			msg_cerr("Locking status change FAILED at 0x%0*" PRIxPTR "!\n", PRIxPTR_WIDTH, lockreg);
    		return -1;
    	}
    }
    /* Enable lockdown if requested. */
-	if (!(old & REG2_LOCKDOWN) && (new_bits & REG2_LOCKDOWN)) {
-		msg_cdbg("Enabling lockdown at 0x%0*" PRIxPTR "\n", PRIxPTR_WIDTH, offset);
-		chip_writeb(flash, new_bits, wrprotect);
-		if (chip_readb(flash, wrprotect) != new_bits) {
-			msg_cerr("Enabling lockdown FAILED at 0x%0*" PRIxPTR "!\n", PRIxPTR_WIDTH, offset);
+	if (!(old & REG2_LOCKDOWN) && (new & REG2_LOCKDOWN)) {
+		msg_cdbg("Enabling lockdown at 0x%0*" PRIxPTR "\n", PRIxPTR_WIDTH, lockreg);
+		chip_writeb(flash, new, lockreg);
+		if (chip_readb(flash, lockreg) != new) {
+			msg_cerr("Enabling lockdown FAILED at 0x%0*" PRIxPTR "!\n", PRIxPTR_WIDTH, lockreg);
    		return -1;
    	}
    }
@@ -704,19 +700,18 @@ static int changelock_regspace2_block(const struct flashctx *flash, chipaddr off
    return 0;
 }

-int unlock_regspace2_block(const struct flashctx *flash, chipaddr off)
+static int unlock_regspace2_block_generic(const struct flashctx *flash, chipaddr lockreg)
 {
-	chipaddr wrprotect = flash->virtual_registers + off + 2;
-	uint8_t old = chip_readb(flash, wrprotect);
+	uint8_t old = chip_readb(flash, lockreg);
    /* We don't care for the lockdown bit as long as the RW locks are 0 after we're done */
-	return changelock_regspace2_block(flash, off, old & ~REG2_RWLOCK);
+	return changelock_regspace2_block(flash, lockreg, old, old & ~REG2_RWLOCK);
 }

static int unlock_regspace2_uniform(struct flashctx *flash, unsigned long block_size)
 {
    const unsigned int elems = flash->chip->total_size * 1024 / block_size;
    struct unlockblock blocks[2] = {{.size = block_size, .count = elems}};
-	return regspace2_walk_unlockblocks(flash, blocks, &unlock_regspace2_block);
+	return regspace2_walk_unlockblocks(flash, blocks, &unlock_regspace2_block_generic);
 }

int unlock_regspace2_uniform_64k(struct flashctx *flash)
@@ -734,7 +729,7 @@ int unlock_regspace2_block_eraser_0(struct flashctx *flash)
    // FIXME: this depends on the eraseblocks not to be filled up completely (i.e. to be null-terminated).
    const struct unlockblock *unlockblocks =
    	(const struct unlockblock *)flash->chip->block_erasers[0].eraseblocks;
-	return regspace2_walk_unlockblocks(flash, unlockblocks, &unlock_regspace2_block);
+	return regspace2_walk_unlockblocks(flash, unlockblocks, &unlock_regspace2_block_generic);
 }

int unlock_regspace2_block_eraser_1(struct flashctx *flash)
@@ -742,6 +737,6 @@ int unlock_regspace2_block_eraser_1(struct flashctx *flash)
    // FIXME: this depends on the eraseblocks not to be filled up completely (i.e. to be null-terminated).
    const struct unlockblock *unlockblocks =
    	(const struct unlockblock *)flash->chip->block_erasers[1].eraseblocks;
-	return regspace2_walk_unlockblocks(flash, unlockblocks, &unlock_regspace2_block);
+	return regspace2_walk_unlockblocks(flash, unlockblocks, &unlock_regspace2_block_generic);
 }