flashcp

[Tobias Simon]

Hello everybody,

I am new in this list.
Today I found a bug in the flashcp program of the mtd-utils:

In Line 260 of flashcp.c, the following operation is performed:

erase.length = filestat.st_size & ~(mtd.erasesize - 1);

I think that is not correct, because if mtd.erasesize is not a power of 2,
using a bitmask is wrong. ARM dataflash, for instance has a erasesize of
1056. My suggestion is the following code replacement, which uses modular
arithmetics:

if (filestat.st_size <= mtd.erasesize)
{
erase.length = mtd.erasesize;
}
else
{
erase.length = filestat.st_size + mtd.erasesize - (filestat.st_size %
mtd.erasesize);
}

What's your opinion?
--
Mit freundlichem Gru? / Kind regards

Tobias Simon
- Entwickler -

Simon + Puschmann Software-Systeme GbR
Tobias Simon, Andre Puschmann

Bergrat-Voigt-Stra?e 13
98693 Ilmenau
Tel.: [phone]
E-Mail: tobias.simon at sp-ss.de
Internet: http://www.sp-ss.de

------------------------------------------------------------------------
Important Note: This e-mail and any attachments are confidential, may contain
trade secrets and may well also be legally privileged or otherwise protected
from disclosure. If you have received it in error, you are on notice of its
status. Please notify us immediately by reply e-mail and then delete this
e-mail and any attachment from your system. If you are not the intended
recipient please understand that you must not copy this e-mail or any
attachments or disclose the contents to any other person. Thank you.

[Jörn Engel]

Post by Tobias Simon
I am new in this list.
erase.length = filestat.st_size & ~(mtd.erasesize - 1);
I think that is not correct, because if mtd.erasesize is not a power of 2,
using a bitmask is wrong. ARM dataflash, for instance has a erasesize of
1056. My suggestion is the following code replacement, which uses modular
if (filestat.st_size <= mtd.erasesize)
{
erase.length = mtd.erasesize;
}
else
{
erase.length = filestat.st_size + mtd.erasesize - (filestat.st_size %
mtd.erasesize);
}
What's your opinion?

Many-fold.

1. You're off-by-one when (filestat.st_size % mtd.erasesize) yields
zero.

2. In principle the "patch" looks fine. Except that it should be a
patch and probably follow kernel coding style or something similar.

3. I keep wondering whether the oddball erasesize of dataflash makes
sense. Why did they pick 1056, the exact size of two 512 Byte nand
pages with the standard 16 Bytes of OOB each? Could they need ECC as
well and should the "spare" 32 bytes be used for that instead?

J?rn

[Tobias Simon]

Hello J?rn,

Post by Jörn Engel
1. You're off-by-one when (filestat.st_size % mtd.erasesize) yields
zero.

Oops, you're right!

Post by Jörn Engel
2. In principle the "patch" looks fine. Except that it should be a
patch and probably follow kernel coding style or something similar.

--- mtd-utils-1.0.0/flashcp.c 2006-04-30 22:59:15.000000000 +0200
+++ mtd-utils-1.0.0_new/flashcp.c 2007-07-19 22:57:25.000000000 +0200
@@ -258,7 +258,14 @@
#warning "Check for smaller erase regions"

erase.start = 0;
- erase.length = filestat.st_size & ~(mtd.erasesize - 1);
+
+ if (filestat.st_size % mtd.erasesize) {
+ erase.length = filestat.st_size + mtd.erasesize - (filestat.st_size %
mtd.erasesize);
+ }
+ else {
+ erase.length = filestat.st_size;
+ }
+
if (filestat.st_size % mtd.erasesize) erase.length += mtd.erasesize;
if (flags & FLAG_VERBOSE)
{

Post by Jörn Engel
3. I keep wondering whether the oddball erasesize of dataflash makes
sense. Why did they pick 1056, the exact size of two 512 Byte nand
pages with the standard 16 Bytes of OOB each? Could they need ECC as
well and should the "spare" 32 bytes be used for that instead?

The standard page/erase-size of the AT45DB642x Dataflash we use
(http://www.atmel.com/dyn/resources/prod_documents/doc3542.pdf) is 1056.
Although it is configurable to 1024 by software, the atmel_mtd.c driver
(linux-2.6.21) uses 1056 as the erase size. The driver itself is full of odd
erase-sizes:
/*
* Detect and initialize DataFlash device:
*
* Device Density ID code #Pages PageSize Offset
* AT45DB011B 1Mbit (128K) xx0011xx (0x0c) 512 264 9
* AT45DB021B 2Mbit (256K) xx0101xx (0x14) 1025 264 9
* AT45DB041B 4Mbit (512K) xx0111xx (0x1c) 2048 264 9
* AT45DB081B 8Mbit (1M) xx1001xx (0x24) 4096 264 9
* AT45DB0161B 16Mbit (2M) xx1011xx (0x2c) 4096 528 10
* AT45DB0321B 32Mbit (4M) xx1101xx (0x34) 8192 528 10
* AT45DB0642 64Mbit (8M) xx111xxx (0x3c) 8192 1056 11
* AT45DB1282 128Mbit (16M) xx0100xx (0x10) 16384 1056 11
*/
--
Mit freundlichem Gru? / Kind regards

Tobias Simon
- Entwickler -

Simon + Puschmann Software-Systeme GbR
Tobias Simon, Andre Puschmann

Bergrat-Voigt-Stra?e 13
98693 Ilmenau
Tel.: [phone]
E-Mail: tobias.simon at sp-ss.de
Internet: http://www.sp-ss.de

------------------------------------------------------------------------
Important Note: This e-mail and any attachments are confidential, may contain
trade secrets and may well also be legally privileged or otherwise protected
from disclosure. If you have received it in error, you are on notice of its
status. Please notify us immediately by reply e-mail and then delete this
e-mail and any attachment from your system. If you are not the intended
recipient please understand that you must not copy this e-mail or any
attachments or disclose the contents to any other person. Thank you.

[Jörn Engel]

Please keep me on Cc:. It is generally considered to be polite and
significantly reduces the chances of me missing a reply.

Post by Tobias Simon

Post by Jörn Engel
1. You're off-by-one when (filestat.st_size % mtd.erasesize) yields
zero.

Oops, you're right!

Post by Jörn Engel
2. In principle the "patch" looks fine. Except that it should be a
patch and probably follow kernel coding style or something similar.

--- mtd-utils-1.0.0/flashcp.c 2006-04-30 22:59:15.000000000 +0200
+++ mtd-utils-1.0.0_new/flashcp.c 2007-07-19 22:57:25.000000000 +0200
@@ -258,7 +258,14 @@
#warning "Check for smaller erase regions"
erase.start = 0;
- erase.length = filestat.st_size & ~(mtd.erasesize - 1);
+
+ if (filestat.st_size % mtd.erasesize) {
+ erase.length = filestat.st_size + mtd.erasesize - (filestat.st_size %
mtd.erasesize);
+ }
+ else {
+ erase.length = filestat.st_size;
+ }
+

Still off-by-one and needlessly complicated.

erase.length = (filestat.st_size / mtd.erasesize) * mtd.erasesize

This always rounds down, just like the current implementation. Whether
rounding down is correct I don't know. Maybe Josh does.

Post by Tobias Simon
The standard page/erase-size of the AT45DB642x Dataflash we use
(http://www.atmel.com/dyn/resources/prod_documents/doc3542.pdf) is 1056.
Although it is configurable to 1024 by software, the atmel_mtd.c driver
(linux-2.6.21) uses 1056 as the erase size. The driver itself is full of odd
/*
*
* Device Density ID code #Pages PageSize Offset
* AT45DB011B 1Mbit (128K) xx0011xx (0x0c) 512 264 9
* AT45DB021B 2Mbit (256K) xx0101xx (0x14) 1025 264 9
* AT45DB041B 4Mbit (512K) xx0111xx (0x1c) 2048 264 9
* AT45DB081B 8Mbit (1M) xx1001xx (0x24) 4096 264 9
* AT45DB0161B 16Mbit (2M) xx1011xx (0x2c) 4096 528 10
* AT45DB0321B 32Mbit (4M) xx1101xx (0x34) 8192 528 10
* AT45DB0642 64Mbit (8M) xx111xxx (0x3c) 8192 1056 11
* AT45DB1282 128Mbit (16M) xx0100xx (0x10) 16384 1056 11
*/

Not too odd. Just the usual 8/256 bytes extra for OOB usage.

Post by Tobias Simon
------------------------------------------------------------------------
Important Note: This e-mail and any attachments are confidential, may contain
trade secrets and may well also be legally privileged or otherwise protected
from disclosure. If you have received it in error, you are on notice of its
status. Please notify us immediately by reply e-mail and then delete this
e-mail and any attachment from your system. If you are not the intended
recipient please understand that you must not copy this e-mail or any
attachments or disclose the contents to any other person. Thank you.

Such a disclaimer will make most people simply ignore you. If I hadn't
already answered most of your mail before seeing this I would have as
well.

J?rn

[Tobias Simon]

Hello J?rn,

and thanks for your advices!

Post by Jörn Engel
Still off-by-one and needlessly complicated.
erase.length = (filestat.st_size / mtd.erasesize) * mtd.erasesize
This always rounds down, just like the current implementation. Whether
rounding down is correct I don't know. Maybe Josh does.

I thought that rounding up would be a better choice, because the length is
passed to the erase callback of the device driver.
Won't "flashcp" forget to erase the last block if we round down?

I will test both versions next week, because I have no hardware here.


Best Regards,
Tobias

[Tobias Simon]

Hello J?rn,

I just saw the following line, which is part of the original source:
"if (filestat.st_size % mtd.erasesize) erase.length += mtd.erasesize;"

So either this line could be deleted (when using my last patch):


--- flashcp_old.c 2007-07-20 01:15:28.000000000 +0200
+++ flashcp_a.c 2007-07-20 01:16:48.000000000 +0200
@@ -257,9 +257,15 @@
#warning "Check for smaller erase regions"

erase.start = 0;
- erase.length = filestat.st_size & ~(mtd.erasesize - 1);
- if (filestat.st_size % mtd.erasesize) erase.length += mtd.erasesize;
- if (flags & FLAG_VERBOSE)
+
+ if (filestat.st_size % mtd.erasesize) {
+ erase.length = filestat.st_size + mtd.erasesize - (filestat.st_size %
mtd.erasesize);
+ }
+ else {
+ erase.length = filestat.st_size;
+ }
+
+ if (flags & FLAG_VERBOSE)
{
/* if the user wants verbose output, erase 1 block at a time
and show him/her what's going on */
int blocks = erase.length / mtd.erasesize;



... or left there in conjunction with your "rounding down" code:


--- flashcp_old.c 2007-07-20 01:15:28.000000000 +0200
+++ flashcp_b.c 2007-07-20 01:14:02.000000000 +0200
@@ -257,9 +257,12 @@
#warning "Check for smaller erase regions"

erase.start = 0;
- erase.length = filestat.st_size & ~(mtd.erasesize - 1);
- if (filestat.st_size % mtd.erasesize) erase.length += mtd.erasesize;
- if (flags & FLAG_VERBOSE)
+
+ erase.length = (filestat.st_size / mtd.erasesize) * mtd.erasesize;
+ if (filestat.st_size % mtd.erasesize)
+ erase.length += mtd.erasesize;
+
+ if (flags & FLAG_VERBOSE)
{
/* if the user wants verbose output, erase 1 block at a time
and show him/her what's going on */
int blocks = erase.length / mtd.erasesize;


I prefer the second patch, because it's easier to understand.


Best Regards,
Tobias

[Jörn Engel]

Post by Tobias Simon
"if (filestat.st_size % mtd.erasesize) erase.length += mtd.erasesize;"
--- flashcp_old.c 2007-07-20 01:15:28.000000000 +0200
+++ flashcp_b.c 2007-07-20 01:14:02.000000000 +0200
@@ -257,9 +257,12 @@
#warning "Check for smaller erase regions"
erase.start = 0;
- erase.length = filestat.st_size & ~(mtd.erasesize - 1);
- if (filestat.st_size % mtd.erasesize) erase.length += mtd.erasesize;
- if (flags & FLAG_VERBOSE)
+
+ erase.length = (filestat.st_size / mtd.erasesize) * mtd.erasesize;
+ if (filestat.st_size % mtd.erasesize)
+ erase.length += mtd.erasesize;
+
+ if (flags & FLAG_VERBOSE)
{
/* if the user wants verbose output, erase 1 block at a time
and show him/her what's going on */
int blocks = erase.length / mtd.erasesize;

Maybe something like this:
#define ODD_ALIGN(n, align) ((((n) + (align) - 1) / (align)) * (align))
...
erase.length = ODD_ALIGN(filestat.st_size, mtd.erasesize);

But I should really disengage now. I don't even know what flashcp is
supposed to do.

J?rn