karosium / smbusb

USB SMBus Interface
GNU Lesser General Public License v2.1
142 stars 42 forks source link

Eeprom verification bypassed? #15

Closed 0r10nV closed 4 years ago

0r10nV commented 4 years ago

It looks like Eeprom verifying after writing to DF is bypassed. Here in the code effective comparison length is Zero.

54 #define EEPROM_BLOCKSZ 0x20
...
58 #define EEPROM_RESERVED_BYTES 32

...
...
...

423 if (memcmp(block,block2,EEPROM_BLOCKSZ-EEPROM_RESERVED_BYTES) == 0) {
424 fprintf(stderr,".");
425 } else {
426 printf("Block verify fail. Block #%d\n",i);
427 exit(0);
428 }

Following correction to code should enable it.


...
416 for (i=0;i<EEPROM_BLOCK_COUNT - (EEPROM_RESERVED_BYTES/EEPROM_BLOCKSZ);i++) { ... }
...

423 if (memcmp(block,block2,EEPROM_BLOCKSZ) == 0) {
...
```C

If it's Ok will revert with pull request.
karosium commented 4 years ago

Thanks, it's definitely a bug and the fix is probably how I originally wanted to implement it, but...

According to the code on page 3 here: http://www.ti.com/lit/an/slua379e/slua379e.pdf there are only 56 rows(/blocks) of dataflash in the bq20zXX/8030 family, which might mean that the issue is actually

#define EEPROM_BLOCK_COUNT 64

leading to 256bytes of garbage at the end of the images. This would explain why verification would've been failing for me and evidently I ended up disabling it by mistake while trying to figure out why.

I'll try to look into it a little bit more over the weekend and confirm.

0r10nV commented 4 years ago

Yes, the actual image sizes accompanying TI's bqEVSW products (..TI/bqEVSW/Plugins/Device defaults/*.dfi) have mostly 1793 bytes in size, but we should take into account that slua379e covers TI native devices which mostly not use DF space beyond 0x700. But when it come to another Vendors who opted to develop their own firmwares (like Sony, Sanyo or Dell) the actual used DF space could differ. For example Sanyo-based bq8030 Gas Gauges store Manufacture Date at 0x7B0 and Serial Number at 0x7B2 i.e. almost at the end of user-allowable offsets.

Like for me,

#define EEPROM_BLOCK_COUNT 64

looks correct because it represent the real size of Full DF of bq8030 family.

To fix verification issue we could correct the number of reserved bytes. According to sluu225 (BootROM API v3.0) it's 32 bytes, but the real size is probably 64 (this value is used in BootROM routine DF_CheckSum() which returns integrity check value for 0x47C0-0x4000=0x7C0 (1984) bytes.

karosium commented 4 years ago

Ah, makes sense! Thanks for the clarification, I wasn't sure if I added the reserved bytes based on documentation or assumption. In that case the proposed fix looks good to me, feel free to send a PR.