diff mbox series

flash_handler: skip writing to 0xff filled pages on nand

Message ID 20171124123203.29892-1-torben.hohn@linutronix.de
State Changes Requested
Headers show
Series flash_handler: skip writing to 0xff filled pages on nand | expand

Commit Message

Torben Hohn Nov. 24, 2017, 12:32 p.m. UTC 
This is necessary when flashing an ubi image. A block filled with 0ff
does not have an ECC made of 0xff. So this already flips some bits, when
written. However, ubi treats a page full of 0xff as empty, which is
not true here. And the result is that data written to such an empty
page yields ECC Errors.
---
 handlers/flash_handler.c | 45 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

Comments

Stefano Babic Nov. 24, 2017, 1:51 p.m. UTC | #1 
Hi Torben,

On 24/11/2017 13:32, Torben Hohn wrote:
> This is necessary when flashing an ubi image. A block filled with 0ff
> does not have an ECC made of 0xff. So this already flips some bits, when
> written. However, ubi treats a page full of 0xff as empty, which is
> not true here. And the result is that data written to such an empty
> page yields ECC Errors.

I understand you write UBI images, but in SWUpdate there is a UBI Volume
handler that allows to update (and resize) single UBI volumes. Flash
handler is still thought for RAW flash, without UBI.

Anyway...

> ---
>  handlers/flash_handler.c | 45 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c
> index 081c2a4..10ea0f7 100644
> --- a/handlers/flash_handler.c
> +++ b/handlers/flash_handler.c
> @@ -49,6 +49,32 @@
>  
>  void flash_handler(void);
>  
> +/* Check whether buffer is filled with character 'pattern' */
> +static inline int buffer_check_pattern(unsigned char *buffer, size_t size,
> +                                       unsigned char pattern)
> +{
> +        /* Invalid input */
> +        if (!buffer || (size == 0))
> +                return 0;
> +
> +        /* No match on first byte */
> +        if (*buffer != pattern)
> +                return 0;
> +
> +        /* First byte matched and buffer is 1 byte long, OK. */
> +        if (size == 1)
> +                return 1;
> +
> +        /*
> +         * Check buffer longer than 1 byte. We already know that buffer[0]
> +         * matches the pattern, so the test below only checks whether the
> +         * buffer[0...size-2] == buffer[1...size-1] , which is a test for
> +         * whether the buffer is filled with constant value.
> +         */
> +        return !memcmp(buffer, buffer + 1, size - 1);
> +}

I appreciate if you add in the commit message the sources where you took
the code. Changes are in relative new versions of mtd-utils, and it
would be nice if you add a reference which sources and which commits you
have taken as example. This lets track over projects why some changes
were done (the whole story is in mtd-utils and MTD ML).

As last commit, I see Marek's:

commit f5b47ef5112326b06fd875982379769c55a71703
Author: Marek Vasut <marex@denx.de>
Date:   Fri Dec 9 02:47:41 2016 +0100

    nandwrite: Factor out buffer checking code


but it is also based at least on a previous:

commit 15c21334b201dc54870cfd3e9697307c95f7e4dc
Author: Kees Trommel <ctrommel@linvm302.aimsys.nl>
Date:   Tue Dec 6 09:21:19 2016 +0100

    nandwrite: add skip-all-ff-pages-option


> +
> +
>  /*
>   * Writing to the NAND must take into account ECC errors
>   * and BAD sectors.
> @@ -204,14 +230,17 @@ static int flash_write_nand(int mtdnum, struct img_type *img)
>  
>  		}
>  
> -		/* Write out data */
> -		ret = mtd_write(flash->libmtd, mtd, fd, mtdoffset / mtd->eb_size,
> -				mtdoffset % mtd->eb_size,
> -				writebuf,
> -				mtd->min_io_size,
> -				NULL,
> -				0,
> -				MTD_OPS_PLACE_OOB);
> +		ret =0;
> +		if (!buffer_check_pattern(writebuf, mtd->min_io_size, 0xff)) {
> +			/* Write out data */
> +			ret = mtd_write(flash->libmtd, mtd, fd, mtdoffset / mtd->eb_size,
> +					mtdoffset % mtd->eb_size,
> +					writebuf,
> +					mtd->min_io_size,
> +					NULL,
> +					0,
> +					MTD_OPS_PLACE_OOB);
> +		}

Ok, fine, so code is aligned with mtd-utils, as far as I can see. Please
resend with changes and I will merge it.

>  		if (ret) {
>  			long long i;
>  			if (errno != EIO) {
> 

Best regards,
Stefano Babic
Torben Hohn Nov. 24, 2017, 2:35 p.m. UTC | #2 
On Fri, Nov 24, 2017 at 02:51:33PM +0100, Stefano Babic wrote:
> Hi Torben,
> 
> On 24/11/2017 13:32, Torben Hohn wrote:
> > This is necessary when flashing an ubi image. A block filled with 0ff
> > does not have an ECC made of 0xff. So this already flips some bits, when
> > written. However, ubi treats a page full of 0xff as empty, which is
> > not true here. And the result is that data written to such an empty
> > page yields ECC Errors.
> 
> I understand you write UBI images, but in SWUpdate there is a UBI Volume
> handler that allows to update (and resize) single UBI volumes. Flash
> handler is still thought for RAW flash, without UBI.

This is a complete ubi image generated via ubinize and containing more
than one ubi volume. 
The right way would be an ubiformat handler. This is just a quick
solution for my current Problem.

> 
> Anyway...
> 
> > ---
> >  handlers/flash_handler.c | 45 +++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 37 insertions(+), 8 deletions(-)
> > 
> > diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c
> > index 081c2a4..10ea0f7 100644
> > --- a/handlers/flash_handler.c
> > +++ b/handlers/flash_handler.c
> > @@ -49,6 +49,32 @@
> >  
> >  void flash_handler(void);
> >  
> > +/* Check whether buffer is filled with character 'pattern' */
> > +static inline int buffer_check_pattern(unsigned char *buffer, size_t size,
> > +                                       unsigned char pattern)
> > +{
> > +        /* Invalid input */
> > +        if (!buffer || (size == 0))
> > +                return 0;
> > +
> > +        /* No match on first byte */
> > +        if (*buffer != pattern)
> > +                return 0;
> > +
> > +        /* First byte matched and buffer is 1 byte long, OK. */
> > +        if (size == 1)
> > +                return 1;
> > +
> > +        /*
> > +         * Check buffer longer than 1 byte. We already know that buffer[0]
> > +         * matches the pattern, so the test below only checks whether the
> > +         * buffer[0...size-2] == buffer[1...size-1] , which is a test for
> > +         * whether the buffer is filled with constant value.
> > +         */
> > +        return !memcmp(buffer, buffer + 1, size - 1);
> > +}
> 
> I appreciate if you add in the commit message the sources where you took
> the code. Changes are in relative new versions of mtd-utils, and it
> would be nice if you add a reference which sources and which commits you
> have taken as example. This lets track over projects why some changes
> were done (the whole story is in mtd-utils and MTD ML).
> 
> As last commit, I see Marek's:
> 
> commit f5b47ef5112326b06fd875982379769c55a71703
> Author: Marek Vasut <marex@denx.de>
> Date:   Fri Dec 9 02:47:41 2016 +0100
> 
>     nandwrite: Factor out buffer checking code
> 
> 
> but it is also based at least on a previous:
> 
> commit 15c21334b201dc54870cfd3e9697307c95f7e4dc
> Author: Kees Trommel <ctrommel@linvm302.aimsys.nl>
> Date:   Tue Dec 6 09:21:19 2016 +0100
> 
>     nandwrite: add skip-all-ff-pages-option

Ok, will do.

> 
> 
> > +
> > +
> >  /*
> >   * Writing to the NAND must take into account ECC errors
> >   * and BAD sectors.
> > @@ -204,14 +230,17 @@ static int flash_write_nand(int mtdnum, struct img_type *img)
> >  
> >  		}
> >  
> > -		/* Write out data */
> > -		ret = mtd_write(flash->libmtd, mtd, fd, mtdoffset / mtd->eb_size,
> > -				mtdoffset % mtd->eb_size,
> > -				writebuf,
> > -				mtd->min_io_size,
> > -				NULL,
> > -				0,
> > -				MTD_OPS_PLACE_OOB);
> > +		ret =0;
> > +		if (!buffer_check_pattern(writebuf, mtd->min_io_size, 0xff)) {
> > +			/* Write out data */
> > +			ret = mtd_write(flash->libmtd, mtd, fd, mtdoffset / mtd->eb_size,
> > +					mtdoffset % mtd->eb_size,
> > +					writebuf,
> > +					mtd->min_io_size,
> > +					NULL,
> > +					0,
> > +					MTD_OPS_PLACE_OOB);
> > +		}
> 
> Ok, fine, so code is aligned with mtd-utils, as far as I can see. Please
> resend with changes and I will merge it.

Yes. The changes are taken from mtd-utils nandwrite.
The only change i made, is that this patch does skipffs unconditionally.
Is that ok ? or do you want an option.

> 
> >  		if (ret) {
> >  			long long i;
> >  			if (errno != EIO) {
> > 
> 
> Best regards,
> Stefano Babic
> 
> -- 
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================
Stefano Babic Nov. 24, 2017, 3:40 p.m. UTC | #3 
Hi Torben,

On 24/11/2017 15:35, Torben Hohn wrote:
> On Fri, Nov 24, 2017 at 02:51:33PM +0100, Stefano Babic wrote:
>> Hi Torben,
>>
>> On 24/11/2017 13:32, Torben Hohn wrote:
>>> This is necessary when flashing an ubi image. A block filled with 0ff
>>> does not have an ECC made of 0xff. So this already flips some bits, when
>>> written. However, ubi treats a page full of 0xff as empty, which is
>>> not true here. And the result is that data written to such an empty
>>> page yields ECC Errors.
>>
>> I understand you write UBI images, but in SWUpdate there is a UBI Volume
>> handler that allows to update (and resize) single UBI volumes. Flash
>> handler is still thought for RAW flash, without UBI.
> 
> This is a complete ubi image generated via ubinize and containing more
> than one ubi volume. 

Yes, I understood this - the use case in SWUpdate is without ubinize,
and volumes are created with the "ubipartition".

> The right way would be an ubiformat handler.

IMHO a ubiformat can be added to the current ubipartition handler - it
is not yet done because I had the use case where no volumes must be
created in field.

If "ubipartition" format and creates the volumes, the single volumes can
be stored as separate artifact, each of them installed by the "ubivol"
handler.

> This is just a quick
> solution for my current Problem.

ok, it does not hurt, it can be in.

> 
>>
>> Anyway...
>>
>>> ---
>>>  handlers/flash_handler.c | 45 +++++++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 37 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c
>>> index 081c2a4..10ea0f7 100644
>>> --- a/handlers/flash_handler.c
>>> +++ b/handlers/flash_handler.c
>>> @@ -49,6 +49,32 @@
>>>  
>>>  void flash_handler(void);
>>>  
>>> +/* Check whether buffer is filled with character 'pattern' */
>>> +static inline int buffer_check_pattern(unsigned char *buffer, size_t size,
>>> +                                       unsigned char pattern)
>>> +{
>>> +        /* Invalid input */
>>> +        if (!buffer || (size == 0))
>>> +                return 0;
>>> +
>>> +        /* No match on first byte */
>>> +        if (*buffer != pattern)
>>> +                return 0;
>>> +
>>> +        /* First byte matched and buffer is 1 byte long, OK. */
>>> +        if (size == 1)
>>> +                return 1;
>>> +
>>> +        /*
>>> +         * Check buffer longer than 1 byte. We already know that buffer[0]
>>> +         * matches the pattern, so the test below only checks whether the
>>> +         * buffer[0...size-2] == buffer[1...size-1] , which is a test for
>>> +         * whether the buffer is filled with constant value.
>>> +         */
>>> +        return !memcmp(buffer, buffer + 1, size - 1);
>>> +}
>>
>> I appreciate if you add in the commit message the sources where you took
>> the code. Changes are in relative new versions of mtd-utils, and it
>> would be nice if you add a reference which sources and which commits you
>> have taken as example. This lets track over projects why some changes
>> were done (the whole story is in mtd-utils and MTD ML).
>>
>> As last commit, I see Marek's:
>>
>> commit f5b47ef5112326b06fd875982379769c55a71703
>> Author: Marek Vasut <marex@denx.de>
>> Date:   Fri Dec 9 02:47:41 2016 +0100
>>
>>     nandwrite: Factor out buffer checking code
>>
>>
>> but it is also based at least on a previous:
>>
>> commit 15c21334b201dc54870cfd3e9697307c95f7e4dc
>> Author: Kees Trommel <ctrommel@linvm302.aimsys.nl>
>> Date:   Tue Dec 6 09:21:19 2016 +0100
>>
>>     nandwrite: add skip-all-ff-pages-option
> 
> Ok, will do.

Thanks.

> 
>>
>>
>>> +
>>> +
>>>  /*
>>>   * Writing to the NAND must take into account ECC errors
>>>   * and BAD sectors.
>>> @@ -204,14 +230,17 @@ static int flash_write_nand(int mtdnum, struct img_type *img)
>>>  
>>>  		}
>>>  
>>> -		/* Write out data */
>>> -		ret = mtd_write(flash->libmtd, mtd, fd, mtdoffset / mtd->eb_size,
>>> -				mtdoffset % mtd->eb_size,
>>> -				writebuf,
>>> -				mtd->min_io_size,
>>> -				NULL,
>>> -				0,
>>> -				MTD_OPS_PLACE_OOB);
>>> +		ret =0;
>>> +		if (!buffer_check_pattern(writebuf, mtd->min_io_size, 0xff)) {
>>> +			/* Write out data */
>>> +			ret = mtd_write(flash->libmtd, mtd, fd, mtdoffset / mtd->eb_size,
>>> +					mtdoffset % mtd->eb_size,
>>> +					writebuf,
>>> +					mtd->min_io_size,
>>> +					NULL,
>>> +					0,
>>> +					MTD_OPS_PLACE_OOB);
>>> +		}
>>
>> Ok, fine, so code is aligned with mtd-utils, as far as I can see. Please
>> resend with changes and I will merge it.
> 
> Yes. The changes are taken from mtd-utils nandwrite.

Right, I saw it.

> The only change i made, is that this patch does skipffs unconditionally.
> Is that ok ? or do you want an option.

No, it is ok - we do not introduce dead case if they are not needed. It
could be added later if it will be necessary.

Best regards,
Stefano Babic
diff mbox series

Patch

diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c
index 081c2a4..10ea0f7 100644
--- a/handlers/flash_handler.c
+++ b/handlers/flash_handler.c
@@ -49,6 +49,32 @@ 
 
 void flash_handler(void);
 
+/* Check whether buffer is filled with character 'pattern' */
+static inline int buffer_check_pattern(unsigned char *buffer, size_t size,
+                                       unsigned char pattern)
+{
+        /* Invalid input */
+        if (!buffer || (size == 0))
+                return 0;
+
+        /* No match on first byte */
+        if (*buffer != pattern)
+                return 0;
+
+        /* First byte matched and buffer is 1 byte long, OK. */
+        if (size == 1)
+                return 1;
+
+        /*
+         * Check buffer longer than 1 byte. We already know that buffer[0]
+         * matches the pattern, so the test below only checks whether the
+         * buffer[0...size-2] == buffer[1...size-1] , which is a test for
+         * whether the buffer is filled with constant value.
+         */
+        return !memcmp(buffer, buffer + 1, size - 1);
+}
+
+
 /*
  * Writing to the NAND must take into account ECC errors
  * and BAD sectors.
@@ -204,14 +230,17 @@  static int flash_write_nand(int mtdnum, struct img_type *img)
 
 		}
 
-		/* Write out data */
-		ret = mtd_write(flash->libmtd, mtd, fd, mtdoffset / mtd->eb_size,
-				mtdoffset % mtd->eb_size,
-				writebuf,
-				mtd->min_io_size,
-				NULL,
-				0,
-				MTD_OPS_PLACE_OOB);
+		ret =0;
+		if (!buffer_check_pattern(writebuf, mtd->min_io_size, 0xff)) {
+			/* Write out data */
+			ret = mtd_write(flash->libmtd, mtd, fd, mtdoffset / mtd->eb_size,
+					mtdoffset % mtd->eb_size,
+					writebuf,
+					mtd->min_io_size,
+					NULL,
+					0,
+					MTD_OPS_PLACE_OOB);
+		}
 		if (ret) {
 			long long i;
 			if (errno != EIO) {