[U-Boot] [PATCH] mtd: nand: fix the written length when nand_write_skip_bad failed

[htbegin]

Hi, Scott

Thanks for your review.

On Tue, Mar 5, 2013 at 9:58 AM, Scott Wood <scottwood at freescale.com> wrote:
> On 03/02/2013 03:01:10 AM, Tao Hou wrote:
>>
>> When the data has been partially written into the NAND Flash,
>> returning the written length instead of 0. The written length
>> may be useful when the upper level decides to continue the writing
>> after skipping the block causing the write failure.
>
>
> We already do that in some code paths.
>
>
>> Signed-off-by: Tao Hou <hotforest at gmail.com>
>> Cc: Scott Wood <scottwood at freescale.com>
>> Cc: Ben Gardiner <bengardiner at nanometrics.ca>
>> Cc: Lei Wen <leiwen at marvell.com>
>> ---
>>  drivers/mtd/nand/nand_util.c |   22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>
>
> Could you rebase this on top of this patch:
> http://patchwork.ozlabs.org/patch/224842/
Do you mean a V2 patch ?

> BTW, are you actually using WITH_YAFFS_OOB?  I think there are some other
> things wrong with it at the moment, as mentioned here:
> http://lists.denx.de/pipermail/u-boot/2013-March/148378.html
No, I don't use it.

>
>
>> diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
>> index de1d13e..f57d723 100644
>> --- a/drivers/mtd/nand/nand_util.c
>> +++ b/drivers/mtd/nand/nand_util.c
>> @@ -496,8 +496,10 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t
>> offset, size_t *length,
>>
>>  #ifdef CONFIG_CMD_NAND_YAFFS
>>         if (flags & WITH_YAFFS_OOB) {
>> -               if (flags & ~WITH_YAFFS_OOB)
>> +               if (flags & ~WITH_YAFFS_OOB) {
>> +                       *length = 0;
>>                         return -EINVAL;
>> +               }
>>
>>                 int pages;
>>                 pages = nand->erasesize / nand->writesize;
>> @@ -505,6 +507,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t
>> offset, size_t *length,
>>                 if (*length % (nand->writesize + nand->oobsize)) {
>>                         printf("Attempt to write incomplete page"
>>                                 " in yaffs mode\n");
>> +                       *length = 0;
>>                         return -EINVAL;
>>                 }
>>         } else
>> @@ -542,7 +545,6 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t
>> offset, size_t *length,
>>                 if (rval == 0)
>>                         return 0;
>>
>> -               *length = 0;
>>                 printf("NAND write to offset %llx failed %d\n",
>>                         offset, rval);
>>                 return rval;
>
>
> OK so far...
>
>
>> @@ -550,7 +552,7 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t
>> offset, size_t *length,
>>
>>         while (left_to_write > 0) {
>>                 size_t block_offset = offset & (nand->erasesize - 1);
>> -               size_t write_size, truncated_write_size;
>> +               size_t write_size, truncated_write_size, written_size;
>>
>>                 WATCHDOG_RESET();
>>
>> @@ -586,8 +588,10 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t
>> offset, size_t *length,
>>                                 ops.oobbuf = ops.datbuf + pagesize;
>>
>>                                 rval = nand->write_oob(nand, offset,
>> &ops);
>> -                               if (rval != 0)
>> +                               if (rval != 0) {
>> +                                       written_size = pagesize_oob *
>> page;
>>                                         break;
>> +                               }
>>
>>                                 offset += pagesize;
>>                                 p_buffer += pagesize_oob;
>> @@ -605,14 +609,18 @@ int nand_write_skip_bad(nand_info_t *nand, loff_t
>> offset, size_t *length,
>>
>>                         rval = nand_write(nand, offset,
>> &truncated_write_size,
>>                                         p_buffer);
>> -                       offset += write_size;
>> -                       p_buffer += write_size;
>> +                       if (rval == 0) {
>> +                               offset += write_size;
>> +                               p_buffer += write_size;
>> +                       } else {
>> +                               written_size = truncated_write_size;
>> +                       }
>>                 }
>>
>>                 if (rval != 0) {
>>                         printf("NAND write to offset %llx failed %d\n",
>>                                 offset, rval);
>> -                       *length -= left_to_write;
>> +                       *length -= left_to_write - written_size;
>>                         return rval;
>>                 }
>
>
> ...but I don't see why this part is needed (or correct).  Why doesn't
> "*length -= left_to_write" already get you what you want?
>
> -Scott
I just use "*length -= left_to_write - written_size" to tell the upper
level that what
had been actually happened. For the current block, "written_size" has
been written to the NAND Flash yet, so left_to_write should be
subtracted by "written_size".

Cheers,
Hou.