Re: [flashrom] [PATCH] Fix sfdp_add_uniform_eraser and its usage.

[Carl-Daniel Hailfinger]

Please use "Fix SFDP probing" as commit message headline and commit the
whole batch of SFDP bugfixes in one commit once the patch is fixed.

Am 20.02.2012 22:29 schrieb Stefan Tauner:
> sfdp_add_uniform_eraser checks for existing erasers. Due to a bug it
> looked for eraser slots that have no erase functions set instead of
> those that have one set.
>
> Postpone adding an erase function for the special 4k block erase
> opcode until we know the flash chip size.
> Check for zero chip size in sfdp_add_uniform_eraser.
>
> Fix the output of the parameter table contents.
>
> This patch fixes the index used to retrieve the eraser types, which
> was off one double word.

For me it looks like this part of the patch introduced a new bug.
> Refine some messages and add a few further debugging prints.
>
> has been tested with the new emulator to be able to write successfully
> and also handle some error cases as expected.
> Signed-off-by: Stefan Tauner
>
> sfdp.c |   38 ++++++++++++++++++++++++++------------
>  1 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/sfdp.c b/sfdp.c
> index 75dfb5f..e1897c7 100644
> --- a/sfdp.c
> +++ b/sfdp.c
> @@ -213,22 +223,25 @@ static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)
>    	goto done;
>    }
> dw = 8;
> /* 8. double word */
> dw = 7;
> for (j = 0; j < 4; j++) {
> /* 8 double words from the start + 2 words for every eraser */
> /* 8 double words from the start + 2 bytes for every eraser */
> tmp8 = buf[(4 * dw) + (2 * j)];

8 double words from the start is 32 bytes from the start. Last time I
checked, 7*4 was not 32.

Regards,
Carl-Daniel

[PATCH] Fix sfdp_add_uniform_eraser and its usage.

[Stefan Tauner]

On Tue, 21 Feb 2012 14:51:38 +0100
Carl-Daniel Hailfinger  wrote:
> Please use "Fix SFDP probing" as commit message headline and commit the
> whole batch of SFDP bugfixes in one commit once the patch is fixed.
>
> Am 20.02.2012 22:29 schrieb Stefan Tauner:
> > sfdp_add_uniform_eraser checks for existing erasers. Due to a bug it
> > looked for eraser slots that have no erase functions set instead of
> > those that have one set.
> >
> > Postpone adding an erase function for the special 4k block erase
> > opcode until we know the flash chip size.
> > Check for zero chip size in sfdp_add_uniform_eraser.
> >
> > Fix the output of the parameter table contents.
> >
> > This patch fixes the index used to retrieve the eraser types, which
> > was off one double word.
>
> For me it looks like this part of the patch introduced a new bug.
> > dw = 8;
> > /* 8. double word */
> > dw = 7;
> > for (j = 0; j < 4; j++) {
> > /* 8 double words from the start + 2 words for every eraser */
> > /* 8 double words from the start + 2 bytes for every eraser */
> > tmp8 = buf[(4 * dw) + (2 * j)];
>
> 8 double words from the start is 32 bytes from the start. Last time I
> checked, 7*4 was not 32.

Xth dw starts at (x - 1) * 4 because they start from offset 0 with the
*first* dw.
the second dw starts at (2 - 1) * 4 = 4,
3rd at (3 - 1) * 4 = 8,
…

that mistake was exactly what haunted me back then apparently. :)

the standard talks about (1-based) Xth double words, so i kept that
nomenclature, but used a 0-based offset (dw). we could leave the
existing solution, change the nomenclature (bad idea imho) or use
1-based offsets and subtract 1 before using them... no ideal solution
available imo.

[PATCH] Fix sfdp_add_uniform_eraser and its usage.

[Carl-Daniel Hailfinger]

Am 21.02.2012 18:04 schrieb Stefan Tauner:
> On Tue, 21 Feb 2012 14:51:38 +0100
> Carl-Daniel Hailfinger  wrote:
> > Please use "Fix SFDP probing" as commit message headline and commit the
> > whole batch of SFDP bugfixes in one commit once the patch is fixed.
> >
> > Am 20.02.2012 22:29 schrieb Stefan Tauner:
> > > sfdp_add_uniform_eraser checks for existing erasers. Due to a bug it
> > > looked for eraser slots that have no erase functions set instead of
> > > those that have one set.
> > >
> > > Postpone adding an erase function for the special 4k block erase
> > > opcode until we know the flash chip size.
> > > Check for zero chip size in sfdp_add_uniform_eraser.
> > >
> > > Fix the output of the parameter table contents.
> > >
> > > This patch fixes the index used to retrieve the eraser types, which
> > > was off one double word.
> >
> > For me it looks like this part of the patch introduced a new bug.
> > > dw = 8;
> > > /* 8. double word */
> > > dw = 7;
> > > for (j = 0; j < 4; j++) {
> > > /* 8 double words from the start + 2 words for every eraser */
> > > /* 8 double words from the start + 2 bytes for every eraser */
> > > tmp8 = buf[(4 * dw) + (2 * j)];
> >
> > 8 double words from the start is 32 bytes from the start. Last time I
> > checked, 7*4 was not 32.
>
> Xth dw starts at (x - 1) * 4 because they start from offset 0 with the
> *first* dw.
> the second dw starts at (2 - 1) * 4 = 4,
> 3rd at (3 - 1) * 4 = 8,
> …
>
> that mistake was exactly what haunted me back then apparently. :)
>
> the standard talks about (1-based) Xth double words, so i kept that
> nomenclature, but used a 0-based offset (dw). we could leave the
> existing solution, change the nomenclature (bad idea imho) or use
> 1-based offsets and subtract 1 before using them... no ideal solution
> available imo.

There is a huge difference between "8 double words from the start" and
"8th double word". One of those comments is wrong, and if your
explanation is correct, the "8 double words from the start" is wrong.

Regards,
Carl-Daniel

[PATCH] Fix SFDP probing.

[Stefan Tauner]

sfdp_add_uniform_eraser checks for existing erasers. Due to a bug it
looked for eraser slots that have no erase functions set instead of
those that have one set.

Postpone adding an erase function for the special 4k block erase
opcode until we know the flash chip size and add an additional check
to sfdp_add_uniform_eraser.

Fix the output of the parameter table contents.

This patch fixes the index used to retrieve the eraser types, which
was off one double word.

Refine some messages and add a few further debugging prints.

Signed-off-by: Stefan Tauner 
---
 sfdp.c |   63 ++++++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/sfdp.c b/sfdp.c
index 75dfb5f..b2fc75f 100644
--- a/sfdp.c
+++ b/sfdp.c
@@ -78,8 +78,10 @@ static int sfdp_add_uniform_eraser(struct flashctx *flash, uint8_t opcode, uint3
    uint32_t total_size = flash->total_size * 1024;
    erasefunc_t *erasefn = spi_get_erasefn_from_opcode(opcode);

-	if (erasefn == NULL || block_size == 0 || total_size % block_size != 0) {
-		msg_cdbg("%s: invalid input\n", __func__);
+	if (erasefn == NULL || total_size == 0 || block_size == 0 ||
+	    total_size % block_size != 0) {
+		msg_cdbg("%s: invalid input, please report to "
+			 "[e-mail]\n", __func__);
    	return 1;
    }

@@ -89,11 +91,12 @@ static int sfdp_add_uniform_eraser(struct flashctx *flash, uint8_t opcode, uint3
    	if (eraser->eraseblocks[0].size == block_size &&
    	    eraser->block_erase == erasefn) {
    		msg_cdbg2("  Tried to add a duplicate block eraser: "
-				  "%d x %d B with opcode 0x%02x\n",
+				  "%d x %d B with opcode 0x%02x.\n",
    			  total_size/block_size, block_size, opcode);
    		return 1;
    	}
-		if (eraser->eraseblocks[0].size != 0 || !eraser->block_erase) {
+		if (eraser->eraseblocks[0].size != 0 ||
+		    eraser->block_erase != NULL) {
    		msg_cspew("  Block Eraser %d is already occupied.\n",
    			  i);
    		continue;
@@ -115,11 +118,12 @@ static int sfdp_add_uniform_eraser(struct flashctx *flash, uint8_t opcode, uint3

static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)
 {
+	uint8_t opcode_4k_erase = 0xFF;
    uint32_t tmp32;
    uint8_t tmp8;
    uint32_t total_size; /* in bytes */
    uint32_t block_size;
-	int dw, j;
+	int j;

msg_cdbg("Parsing JEDEC flash parameter table... ");
    if (len != 9 * 4 && len != 4 * 4) {
@@ -129,11 +133,10 @@ static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)
    msg_cdbg2("\n");
    
    /* 1. double word */
-	dw = 0;
-	tmp32 = buf[(4 * dw) + 0];
-	tmp32 |= ((unsigned int)buf[(4 * dw) + 1]) << 8;
-	tmp32 |= ((unsigned int)buf[(4 * dw) + 2]) << 16;
-	tmp32 |= ((unsigned int)buf[(4 * dw) + 3]) << 24;
+	tmp32 =  ((unsigned int)buf[(4 * 0) + 0]);
+	tmp32 |= ((unsigned int)buf[(4 * 0) + 1]) << 8;
+	tmp32 |= ((unsigned int)buf[(4 * 0) + 2]) << 16;
+	tmp32 |= ((unsigned int)buf[(4 * 0) + 3]) << 24;

tmp8 = (tmp32 >> 17) & 0x3;
    switch (tmp8) {
@@ -181,15 +184,17 @@ static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)
    }

if ((tmp32 & 0x3) == 0x1) {
-		sfdp_add_uniform_eraser(flash, (tmp32 >> 8) & 0xFF, 4 * 1024);
-	}
+		opcode_4k_erase = (tmp32 >> 8) & 0xFF;
+		msg_cspew("  4kB erase opcode is 0x%02x.\n", opcode_4k_erase);
+		/* add the eraser later, because we don't know total_size yet */
+	} else
+		msg_cspew("  4kB erase opcode is not defined.\n");

/* 2. double word */
-	dw = 1;
-	tmp32 = buf[(4 * dw) + 0];
-	tmp32 |= ((unsigned int)buf[(4 * dw) + 1]) << 8;
-	tmp32 |= ((unsigned int)buf[(4 * dw) + 2]) << 16;
-	tmp32 |= ((unsigned int)buf[(4 * dw) + 3]) << 24;
+	tmp32 =  ((unsigned int)buf[(4 * 1) + 0]);
+	tmp32 |= ((unsigned int)buf[(4 * 1) + 1]) << 8;
+	tmp32 |= ((unsigned int)buf[(4 * 1) + 2]) << 16;
+	tmp32 |= ((unsigned int)buf[(4 * 1) + 3]) << 24;

if (tmp32 & (1 << 31)) {
    	msg_cdbg("Flash chip size >= 4 Gb/512 MB not supported.\n");
@@ -204,6 +209,9 @@ static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)
    	return 1;
    }

+	if (opcode_4k_erase != 0xFF)
+		sfdp_add_uniform_eraser(flash, opcode_4k_erase, 4 * 1024);
+
    /* FIXME: double words 3-7 contain unused fast read information */

if (len == 4 * 4) {
@@ -213,22 +221,26 @@ static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)
    	goto done;
    }

-	dw = 8;
+	/* 8. double word */
    for (j = 0; j < 4; j++) {
-		/* 8 double words from the start + 2 words for every eraser */
-		tmp8 = buf[(4 * dw) + (2 * j)];
+		/* 7 double words from the start + 2 bytes for every eraser */
+		tmp8 = buf[(4 * 7) + (j * 2)];
+		msg_cspew("   Erase Sector Type %d Size: 0x%02x\n", j + 1,
+			  tmp8);
    	if (tmp8 == 0) {
-			msg_cdbg2("  Block eraser %d is unused.\n", j);
+			msg_cspew("  Erase Sector Type %d is unused.\n", j);
    		continue;
    	}
    	if (tmp8 >= 31) {
-			msg_cdbg2("  Block size of eraser %d (2^%d) is too big "
-				  "for flashrom.\n", j, tmp8);
+			msg_cdbg2("  Block size of erase Sector Type %d (2^%d) "
+				 "is too big for flashrom.\n", j, tmp8);
    		continue;
    	}
    	block_size = 1 << (tmp8); /* block_size = 2 ^ field */

-		tmp8 = buf[(4 * dw) + (2 * j) + 1];
+		tmp8 = buf[(4 * 7) + (j * 2) + 1];
+		msg_cspew("   Erase Sector Type %d Opcode: 0x%02x\n", j + 1,
+			  tmp8);
    	sfdp_add_uniform_eraser(flash, tmp8, block_size);
    }

@@ -332,7 +344,7 @@ int probe_spi_sfdp(struct flashctx *flash)
    		if ((tmp32 % 8) == 0) {
    			msg_cspew("    0x%04x: ", tmp32);
    		}
-			msg_cspew(" %02x", buf[tmp32]);
+			msg_cspew(" %02x", tbuf[tmp32]);
    		if ((tmp32 % 8) == 7) {
    			msg_cspew("\n");
    			continue;
@@ -342,6 +354,7 @@ int probe_spi_sfdp(struct flashctx *flash)
    			continue;
    		}
    	}
+		msg_cspew("\n");

if (i == 0) { /* Mandatory JEDEC SFDP parameter table */
    		if (hdrs[i].id != 0)