Improve SST25 status register routines: - Using a 4-bit index into an array with 8 elements leads to out-of-bounds accesses. That bug was introduced by a self-acked patch from someone. Use proper bit masking to fix this. - Factor out common SST25 status register printing. - Use the common SST25 status register printing for SST25VF080B.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-SST25VF080B/spi.c =================================================================== --- flashrom-SST25VF080B/spi.c (Revision 464) +++ flashrom-SST25VF080B/spi.c (Arbeitskopie) @@ -275,6 +275,15 @@ spi_prettyprint_status_register_common(status); }
+void spi_prettyprint_status_register_sst25(uint8_t status) +{ + printf_debug("Chip status register: Block Protect Write Disable " + "(BPL) is %sset\n", (status & (1 << 7)) ? "" : "not "); + printf_debug("Chip status register: Auto Address Increment Programming " + "(AAI) is %sset\n", (status & (1 << 6)) ? "" : "not "); + spi_prettyprint_status_register_common(status); +} + /* Prettyprint the status register. Works for * SST 25VF016 */ @@ -289,11 +298,7 @@ "100000H-1FFFFFH", "all", "all" }; - printf_debug("Chip status register: Block Protect Write Disable " - "(BPL) is %sset\n", (status & (1 << 7)) ? "" : "not "); - printf_debug("Chip status register: Auto Address Increment Programming " - "(AAI) is %sset\n", (status & (1 << 6)) ? "" : "not "); - spi_prettyprint_status_register_common(status); + spi_prettyprint_status_register_sst25(status); printf_debug("Resulting block protection : %s\n", bpt[(status & 0x1c) >> 2]); } @@ -307,13 +312,9 @@ "0x40000-0x7ffff", "all blocks", "all blocks", "all blocks", "all blocks" }; - printf_debug("Chip status register: Block Protect Write Disable " - "(BPL) is %sset\n", (status & (1 << 7)) ? "" : "not "); - printf_debug("Chip status register: Auto Address Increment Programming " - "(AAI) is %sset\n", (status & (1 << 6)) ? "" : "not "); - spi_prettyprint_status_register_common(status); + spi_prettyprint_status_register_sst25(status); printf_debug("Resulting block protection : %s\n", - bpt[(status & 0x3c) >> 2]); + bpt[(status & 0x1c) >> 2]); }
void spi_prettyprint_status_register(struct flashchip *flash) @@ -341,6 +342,9 @@ case 0x258d: spi_prettyprint_status_register_sst25vf040b(status); break; + case 0x258e: + spi_prettyprint_status_register_sst25(status); + break; } break; }
Carl-Daniel Hailfinger wrote:
Improve SST25 status register routines:
- Using a 4-bit index into an array with 8 elements leads to
out-of-bounds accesses. That bug was introduced by a self-acked patch from someone. Use proper bit masking to fix this.
I think it's pointless to write "it was introduced by self-ack" at all if you do not also write who it was. Either go all the way and actually blame someone because you think it's a big deal or don't bother because it's just about _one bit_. It takes more effort to read the english text than to go with the patch.
My point; Some bugs warrant discussion, others maybe not as much.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Peter Stuge peter@stuge.se
On 06.05.2009 15:52, Peter Stuge wrote:
Carl-Daniel Hailfinger wrote:
Improve SST25 status register routines:
- Using a 4-bit index into an array with 8 elements leads to
out-of-bounds accesses. That bug was introduced by a self-acked patch from someone. Use proper bit masking to fix this.
I think it's pointless to write "it was introduced by self-ack" at all if you do not also write who it was. Either go all the way and actually blame someone because you think it's a big deal or don't bother because it's just about _one bit_.
It's one bit which can cause segfaults. I know who did it (svn blame helps), but I will NOT mention names in public. That would be bad style.
It takes more effort to read the english text than to go with the patch.
My point; Some bugs warrant discussion, others maybe not as much.
Anyway, I removed the part about the self-ack from the changelog.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Peter Stuge peter@stuge.se
Thanks, r468.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
I think it's pointless to write "it was introduced by self-ack" at all if you do not also write who it was. Either go all the way and actually blame someone because you think it's a big deal or don't bother because it's just about _one bit_.
It's one bit which can cause segfaults. I know who did it (svn blame helps), but I will NOT mention names in public. That would be bad style.
I don't think it's bad style at all! There is zero drama. This is just a piece of software we're working on together. I make bugs too.
Either the bug is important enough to tease someone a little, or it's not. The information is easily available from svn blame as you mentioned so it's not like you would be letting out a secret.
My point; Some bugs warrant discussion, others maybe not as much.
Anyway, I removed the part about the self-ack from the changelog.
Ok, but I think it would be fine for you to mention who caused it as well, if you felt the need to.
And you could include the original rev. Or not.
Whatever is easiest and feels good. :)
Thanks, r468.
Goodie!
//Peter
-
Carl-Daniel Hailfinger -
Peter Stuge