[U-Boot] [PATCH] OneNAND partial read/write support

[Scott Wood <scottwood@freescale.com>]

Kyungmin Park wrote:
> On Wed, Oct 21, 2009 at 7:48 AM, Scott Wood <scottwood@freescale.com> wrote:
>> On Mon, Oct 12, 2009 at 04:27:10PM +0900, Kyungmin Park wrote:
>>> Now OneNAND handles block operation only.
>>> With this patch OneNAND handles all read/write size.
>>>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>> diff --git a/common/cmd_onenand.c b/common/cmd_onenand.c
>>> index 9090940..2b8f01b 100644
>>> --- a/common/cmd_onenand.c
>>> +++ b/common/cmd_onenand.c
>>> @@ -36,7 +36,7 @@ static inline int str2long(char *p, ulong *num)
>>>       return (*p != '\0' && *endptr == '\0') ? 1 : 0;
>>>  }
>>>
>>> -static int arg_off_size(int argc, char *argv[], ulong *off, size_t *size)
>>> +static int arg_off_size(int argc, char *argv[], ulong *off, ssize_t *size)
>>>  {
>>>       if (argc >= 1) {
>>>               if (!(str2long(argv[0], off))) {
>> Are you expecting negative sizes?
> 
> I don't know why these are implemented. it's existing one.

I was asking about the change you were making...  I guess it's because 
of the *off + *size check, though unsigned overflow should work as well.

>>>       struct onenand_chip *this = mtd->priv;
>>> -     int blocks = (int) len >> this->erase_shift;
>>>       int blocksize = (1 << this->erase_shift);
>>>       loff_t ofs = from;
>>>       struct mtd_oob_ops ops = {
>>>               .retlen         = 0,
>>>       };
>>> +     ssize_t thislen;
>>>       int ret;
>>>
>>> -     if (oob)
>>> -             ops.ooblen = blocksize;
>>> -     else
>>> -             ops.len = blocksize;
>>> +     while (len > 0) {
>>> +             thislen = min_t(ssize_t, len, blocksize);
>>> +             thislen = ALIGN(thislen, mtd->writesize);
>>>
>>> -     while (blocks) {
>>>               ret = mtd->block_isbad(mtd, ofs);
>>>               if (ret) {
>>>                       printk("Bad blocks %d at 0x%x\n",
>>>                              (u32)(ofs >> this->erase_shift), (u32)ofs);
>>> -                     ofs += blocksize;
>>> +                     ofs += thislen;
>>>                       continue;
>>>               }
>>>
>>> -             if (oob)
>>> +             if (oob) {
>>>                       ops.oobbuf = buf;
>>> -             else
>>> +                     ops.ooblen = thislen;
>>> +             } else {
>>>                       ops.datbuf = buf;
>>> +                     ops.len = thislen;
>> thislen can be greater than len, in which case you'll be overflowing buf.
> 
> No, len is unsigned int, but thislen is int. and can't overflow since.
> it's size down with blocksize at min macro.

I don't see what signedness has to do with it.  You cap thislen at the 
lesser of len or blocksize, but then you round thislen *up* to the page 
size.  Thus thislen can be greater than len.

>> If you want to allow partial page reads, you need to allocate a temporary
>> buffer at some point.  If not (I don't see a huge need), the ALIGN()
>> should be an error check instead.
> 
> 
>> Does this code handle being given an offset that is not at a block (or
>> page) boundary?  It doesn't look like it (it will try to read across
>> block boundaries).
> 
> Basically it reads/writes data based on block. but last or first
> partial read/writes support.

Partial as in page-aligned but block-unaligned, or partial as in can 
start/stop on any byte?

Either way, it doesn't seem to handle the partial first block properly. 
  It will see that len is greater than the blocksize and not pay any 
attention to the alignment of ofs.

>>> @@ -265,9 +276,10 @@ static int onenand_block_test(u32 start, u32 size)
>>>                       goto next;
>>>               }
>>>
>>> -             if (memcmp(buf, verify_buf, blocksize))
>>> +             if (memcmp(buf, verify_buf, blocksize)) {
>>>                       printk("\nRead/Write test failed at 0x%x\n", (u32)ofs);
>>> -
>>> +                     break;
>>> +             }
>>> @@ -322,6 +334,7 @@ static int onenand_dump(struct mtd_info *mtd, ulong off, int only_oob)
>>>               p += 16;
>>>       }
>>>       puts("OOB:\n");
>>> +     p = oobbuf;
>>>       i = mtd->oobsize >> 3;
>>>       while (i--) {
>>>               printf("\t%02x %02x %02x %02x %02x %02x %02x %02x\n",
>>> @@ -339,7 +352,7 @@ int do_onenand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>>>       struct onenand_chip *this;
>>>       int blocksize;
>>>       ulong addr, ofs;
>>> -     size_t len, retlen = 0;
>>> +     ssize_t len, retlen = 0;
>>>       int ret = 0;
>>>       char *cmd, *s;
>>>
>>> @@ -385,7 +398,8 @@ int do_onenand(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
>>>                       int erase;
>>>
>>>                       erase = strcmp(cmd, "erase") == 0; /* 1 = erase, 0 = test */
>>> -                     printf("\nOneNAND %s: ", erase ? "erase" : "test");
>>> +                     printf("\nOneNAND %s %s: ", erase ? "erase" : "test",
>>> +                             force ? "force" : "");
>> These seem to be unrelated changes.
> 
> Right, but it's for exact display for user.

Ideally, you should put minor unrelated changes in a separate patch from 
more siginficant changes.  At least note them in the changelog.

-Scott