A question about nand_read()

A question about nand_read()

[Patrick Turley]

My tree is up-to-date as I'm writing this message.

I'm looking at nand_read() in nand_base.c, starting at line 1175, quoted  
here for your convenience:


static int nand_read(struct mtd_info *mtd, loff_t from, size_t len,
                      size_t *retlen, uint8_t *buf)
{
         struct nand_chip *chip = mtd->priv;
         int ret;

         /* Do not allow reads past end of device */
         if ((from + len) > mtd->size)
                 return -EINVAL;
         if (!len)
                 return 0;

         nand_get_device(chip, mtd, FL_READING);

         chip->ops.len = len;
         chip->ops.datbuf = buf;
         chip->ops.oobbuf = NULL;

         ret = nand_do_read_ops(mtd, from, &chip->ops);

         *retlen = chip->ops.retlen;

         nand_release_device(mtd);

         return ret;
}


In particular, I'm looking at the preparation to call nand_do_read_ops(),  
where the chip->ops structure is filled in.

Here's a small excerpt from nand_do_read_ops() that concerns me:


     /* Now read the page into the buffer */
     if (unlikely(ops->mode == MTD_OOB_RAW))
             ret = chip->ecc.read_page_raw(mtd, chip, bufpoi);
     else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob)
             ret = chip->ecc.read_subpage(mtd, chip, col, bytes, bufpoi);
     else
             ret = chip->ecc.read_page(mtd, chip, bufpoi);
     if (ret < 0)
             break;


Note that nand_do_read_ops() will look at the mode field of the ops  
structure, but this field hasn't been set by nand_read().

On the face of it, this looks like a bug. Is there something I don't know  
that makes this not a bug? For example, is the value in the mode field  
intended to persist across operations?

Re: A question about nand_read()

[Patrick Turley]

No love? Is my question too n00bish?

On Tue, 18 Nov 2008 18:43:55 -0600, Patrick Turley  
<patrick.turley@freescale.com> wrote:

> My tree is up-to-date as I'm writing this message.
>
> I'm looking at nand_read() in nand_base.c, starting at line 1175, quoted
> here for your convenience:
>
>
> static int nand_read(struct mtd_info *mtd, loff_t from, size_t len,
>                       size_t *retlen, uint8_t *buf)
> {
>          struct nand_chip *chip = mtd->priv;
>          int ret;
>
>          /* Do not allow reads past end of device */
>          if ((from + len) > mtd->size)
>                  return -EINVAL;
>          if (!len)
>                  return 0;
>
>          nand_get_device(chip, mtd, FL_READING);
>
>          chip->ops.len = len;
>          chip->ops.datbuf = buf;
>          chip->ops.oobbuf = NULL;
>
>          ret = nand_do_read_ops(mtd, from, &chip->ops);
>
>          *retlen = chip->ops.retlen;
>
>          nand_release_device(mtd);
>
>          return ret;
> }
>
>
> In particular, I'm looking at the preparation to call nand_do_read_ops(),
> where the chip->ops structure is filled in.
>
> Here's a small excerpt from nand_do_read_ops() that concerns me:
>
>
>      /* Now read the page into the buffer */
>      if (unlikely(ops->mode == MTD_OOB_RAW))
>              ret = chip->ecc.read_page_raw(mtd, chip, bufpoi);
>      else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob)
>              ret = chip->ecc.read_subpage(mtd, chip, col, bytes, bufpoi);
>      else
>              ret = chip->ecc.read_page(mtd, chip, bufpoi);
>      if (ret < 0)
>              break;
>
>
> Note that nand_do_read_ops() will look at the mode field of the ops
> structure, but this field hasn't been set by nand_read().
>
> On the face of it, this looks like a bug. Is there something I don't know
> that makes this not a bug? For example, is the value in the mode field
> intended to persist across operations?
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: A question about nand_read()

[Adrian Hunter]

Patrick Turley wrote:
> No love? Is my question too n00bish?

People are busy.

You should test this yourself, and if you can make it go wrong
submit a patch.

I glanced at the code and it looks OK because struct nand_chip
is always initialised to zero and chip->ops.mode is never assigned.

> On Tue, 18 Nov 2008 18:43:55 -0600, Patrick Turley  
> <patrick.turley@freescale.com> wrote:
> 
>> My tree is up-to-date as I'm writing this message.
>>
>> I'm looking at nand_read() in nand_base.c, starting at line 1175, quoted
>> here for your convenience:
>>
>>
>> static int nand_read(struct mtd_info *mtd, loff_t from, size_t len,
>>                       size_t *retlen, uint8_t *buf)
>> {
>>          struct nand_chip *chip = mtd->priv;
>>          int ret;
>>
>>          /* Do not allow reads past end of device */
>>          if ((from + len) > mtd->size)
>>                  return -EINVAL;
>>          if (!len)
>>                  return 0;
>>
>>          nand_get_device(chip, mtd, FL_READING);
>>
>>          chip->ops.len = len;
>>          chip->ops.datbuf = buf;
>>          chip->ops.oobbuf = NULL;
>>
>>          ret = nand_do_read_ops(mtd, from, &chip->ops);
>>
>>          *retlen = chip->ops.retlen;
>>
>>          nand_release_device(mtd);
>>
>>          return ret;
>> }
>>
>>
>> In particular, I'm looking at the preparation to call nand_do_read_ops(),
>> where the chip->ops structure is filled in.
>>
>> Here's a small excerpt from nand_do_read_ops() that concerns me:
>>
>>
>>      /* Now read the page into the buffer */
>>      if (unlikely(ops->mode == MTD_OOB_RAW))
>>              ret = chip->ecc.read_page_raw(mtd, chip, bufpoi);
>>      else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob)
>>              ret = chip->ecc.read_subpage(mtd, chip, col, bytes, bufpoi);
>>      else
>>              ret = chip->ecc.read_page(mtd, chip, bufpoi);
>>      if (ret < 0)
>>              break;
>>
>>
>> Note that nand_do_read_ops() will look at the mode field of the ops
>> structure, but this field hasn't been set by nand_read().
>>
>> On the face of it, this looks like a bug. Is there something I don't know
>> that makes this not a bug? For example, is the value in the mode field
>> intended to persist across operations?
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

Re: A question about nand_read()

[Patrick Turley]

Fair enough. I'll check it out.


On Mon, 24 Nov 2008 03:53:05 -0600, Adrian Hunter  
<ext-adrian.hunter@nokia.com> wrote:

> Patrick Turley wrote:
>> No love? Is my question too n00bish?
>
> People are busy.
>
> You should test this yourself, and if you can make it go wrong
> submit a patch.
>
> I glanced at the code and it looks OK because struct nand_chip
> is always initialised to zero and chip->ops.mode is never assigned.
>
>> On Tue, 18 Nov 2008 18:43:55 -0600, Patrick Turley   
>> <patrick.turley@freescale.com> wrote:
>>
>>> My tree is up-to-date as I'm writing this message.
>>>
>>> I'm looking at nand_read() in nand_base.c, starting at line 1175,  
>>> quoted
>>> here for your convenience:
>>>
>>>
>>> static int nand_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>                       size_t *retlen, uint8_t *buf)
>>> {
>>>          struct nand_chip *chip = mtd->priv;
>>>          int ret;
>>>
>>>          /* Do not allow reads past end of device */
>>>          if ((from + len) > mtd->size)
>>>                  return -EINVAL;
>>>          if (!len)
>>>                  return 0;
>>>
>>>          nand_get_device(chip, mtd, FL_READING);
>>>
>>>          chip->ops.len = len;
>>>          chip->ops.datbuf = buf;
>>>          chip->ops.oobbuf = NULL;
>>>
>>>          ret = nand_do_read_ops(mtd, from, &chip->ops);
>>>
>>>          *retlen = chip->ops.retlen;
>>>
>>>          nand_release_device(mtd);
>>>
>>>          return ret;
>>> }
>>>
>>>
>>> In particular, I'm looking at the preparation to call  
>>> nand_do_read_ops(),
>>> where the chip->ops structure is filled in.
>>>
>>> Here's a small excerpt from nand_do_read_ops() that concerns me:
>>>
>>>
>>>      /* Now read the page into the buffer */
>>>      if (unlikely(ops->mode == MTD_OOB_RAW))
>>>              ret = chip->ecc.read_page_raw(mtd, chip, bufpoi);
>>>      else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob)
>>>              ret = chip->ecc.read_subpage(mtd, chip, col, bytes,  
>>> bufpoi);
>>>      else
>>>              ret = chip->ecc.read_page(mtd, chip, bufpoi);
>>>      if (ret < 0)
>>>              break;
>>>
>>>
>>> Note that nand_do_read_ops() will look at the mode field of the ops
>>> structure, but this field hasn't been set by nand_read().
>>>
>>> On the face of it, this looks like a bug. Is there something I don't  
>>> know
>>> that makes this not a bug? For example, is the value in the mode field
>>> intended to persist across operations?
>>>
>>> ______________________________________________________
>>> Linux MTD discussion mailing list
>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>    ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>