flashcp

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.: 0173/2752144
E-Mail: tobias.simon@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.

Re: flashcp

[Jörn Engel]

On Thu, 19 July 2007 20:46:46 +0200, Tobias Simon wrote:
> 
> 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?

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

-- 
Fools ignore complexity.  Pragmatists suffer it.
Some can avoid it.  Geniuses remove it.
-- Perlis's Programming Proverb #58, SIGPLAN Notices, Sept.  1982

Re: flashcp

[Tobias Simon]

Hello Jörn,

> 1. You're off-by-one when (filestat.st_size % mtd.erasesize) yields
> zero.
Oops, you're right!

> 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)
         {

> 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.: 0173/2752144
E-Mail: tobias.simon@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.

Re: flashcp

[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.

On Thu, 19 July 2007 23:08:21 +0200, Tobias Simon wrote:
> 
> > 1. You're off-by-one when (filestat.st_size % mtd.erasesize) yields
> > zero.
> Oops, you're right!
> 
> > 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.

> 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
>  */

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

> ------------------------------------------------------------------------
> 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

-- 
Happiness isn't having what you want, it's wanting what you have.
-- unknown

Re: flashcp

[Tobias Simon]

Hello Jörn,

and thanks for your advices!

> 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

Re: flashcp

[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

Re: flashcp

[Jörn Engel]

On Fri, 20 July 2007 01:23:46 +0200, Tobias Simon wrote:
> 
> I just saw the following line, which is part of the original source:
> "if (filestat.st_size % mtd.erasesize) 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;

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

-- 
Data expands to fill the space available for storage.
-- Parkinson's Law