[MTD] [NAND] remove len/ooblen confusion.

As was discussed between Ricard Wanderlöf, David Woodhouse, Artem
Bityutskiy and me, the current API for reading/writing OOB is confusing.

The thing that introduces confusion is the need to specify ops.len
together with ops.ooblen for reads/writes that concern only OOB not data
area. So, ops.len is overloaded: when ops.datbuf != NULL it serves to
specify the length of the data read, and when ops.datbuf == NULL, it
serves to specify the full OOB read length.

The patch inlined below is the slightly updated version of the previous
patch serving the same purpose, but with the new Artem's comments taken
into account.

Artem, BTW, thanks a lot for your valuable input!

Signed-off-by: Vitaly Wool <vwool@ru.mvista.com>
Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/drivers/mtd/inftlcore.c b/drivers/mtd/inftlcore.c
index a1b2de605000bf88d2c712a8f8a6e3db0c1a6dcd..d2f54c037e0f8622e4c8d8e4bcbdefce8f103bad 100644 (file)
--- a/drivers/mtd/inftlcore.c
+++ b/drivers/mtd/inftlcore.c
@@ -163,10 +163,9 @@ int inftl_read_oob(struct mtd_info *mtd, loff_t offs, size_t len,
        ops.ooblen = len;
        ops.oobbuf = buf;
        ops.datbuf = NULL;
-       ops.len = len;
 
        res = mtd->read_oob(mtd, offs & ~(mtd->writesize - 1), &ops);
-       *retlen = ops.retlen;
+       *retlen = ops.oobretlen;
        return res;
 }
 
@@ -184,10 +183,9 @@ int inftl_write_oob(struct mtd_info *mtd, loff_t offs, size_t len,
        ops.ooblen = len;
        ops.oobbuf = buf;
        ops.datbuf = NULL;
-       ops.len = len;
 
        res = mtd->write_oob(mtd, offs & ~(mtd->writesize - 1), &ops);
-       *retlen = ops.retlen;
+       *retlen = ops.oobretlen;
        return res;
 }
 

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 866c8e0d57e4c331c49b0781a146e0f9f128de34..07618f51d969e7e4d4555330ff532ca04cfe7adf 100644 (file)
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -499,13 +499,12 @@ static int mtd_ioctl(struct inode *inode, struct file *file,
                if (ret)
                        return ret;
 
-               ops.len = buf.length;
                ops.ooblen = buf.length;
                ops.ooboffs = buf.start & (mtd->oobsize - 1);
                ops.datbuf = NULL;
                ops.mode = MTD_OOB_PLACE;
 
-               if (ops.ooboffs && ops.len > (mtd->oobsize - ops.ooboffs))
+               if (ops.ooboffs && ops.ooblen > (mtd->oobsize - ops.ooboffs))
                        return -EINVAL;
 
                ops.oobbuf = kmalloc(buf.length, GFP_KERNEL);
@@ -520,7 +519,7 @@ static int mtd_ioctl(struct inode *inode, struct file *file,
                buf.start &= ~(mtd->oobsize - 1);
                ret = mtd->write_oob(mtd, buf.start, &ops);
 
-               if (copy_to_user(argp + sizeof(uint32_t), &ops.retlen,
+               if (copy_to_user(argp + sizeof(uint32_t), &ops.oobretlen,
                                 sizeof(uint32_t)))
                        ret = -EFAULT;
 
@@ -548,7 +547,6 @@ static int mtd_ioctl(struct inode *inode, struct file *file,
                if (ret)
                        return ret;
 
-               ops.len = buf.length;
                ops.ooblen = buf.length;
                ops.ooboffs = buf.start & (mtd->oobsize - 1);
                ops.datbuf = NULL;
@@ -564,10 +562,10 @@ static int mtd_ioctl(struct inode *inode, struct file *file,
                buf.start &= ~(mtd->oobsize - 1);
                ret = mtd->read_oob(mtd, buf.start, &ops);
 
-               if (put_user(ops.retlen, (uint32_t __user *)argp))
+               if (put_user(ops.oobretlen, (uint32_t __user *)argp))
                        ret = -EFAULT;
-               else if (ops.retlen && copy_to_user(buf.ptr, ops.oobbuf,
-                                                   ops.retlen))
+               else if (ops.oobretlen && copy_to_user(buf.ptr, ops.oobbuf,
+                                                   ops.oobretlen))
                        ret = -EFAULT;
 
                kfree(ops.oobbuf);

diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index 1fea631b58520aef095c356730709722f6b38248..cf927a8803e711e008624809e55e6301d913aa0a 100644 (file)
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -247,7 +247,7 @@ concat_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
        struct mtd_oob_ops devops = *ops;
        int i, err, ret = 0;
 
-       ops->retlen = 0;
+       ops->retlen = ops->oobretlen = 0;
 
        for (i = 0; i < concat->num_subdev; i++) {
                struct mtd_info *subdev = concat->subdev[i];
@@ -263,6 +263,7 @@ concat_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
 
                err = subdev->read_oob(subdev, from, &devops);
                ops->retlen += devops.retlen;
+               ops->oobretlen += devops.oobretlen;
 
                /* Save information about bitflips! */
                if (unlikely(err)) {
@@ -278,14 +279,18 @@ concat_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
                                return err;
                }
 
-               devops.len = ops->len - ops->retlen;
-               if (!devops.len)
-                       return ret;
-
-               if (devops.datbuf)
+               if (devops.datbuf) {
+                       devops.len = ops->len - ops->retlen;
+                       if (!devops.len)
+                               return ret;
                        devops.datbuf += devops.retlen;
-               if (devops.oobbuf)
-                       devops.oobbuf += devops.ooblen;
+               }
+               if (devops.oobbuf) {
+                       devops.ooblen = ops->ooblen - ops->oobretlen;
+                       if (!devops.ooblen)
+                               return ret;
+                       devops.oobbuf += ops->oobretlen;
+               }
 
                from = 0;
        }
@@ -321,14 +326,18 @@ concat_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops)
                if (err)
                        return err;
 
-               devops.len = ops->len - ops->retlen;
-               if (!devops.len)
-                       return 0;
-
-               if (devops.datbuf)
+               if (devops.datbuf) {
+                       devops.len = ops->len - ops->retlen;
+                       if (!devops.len)
+                               return 0;
                        devops.datbuf += devops.retlen;
-               if (devops.oobbuf)
-                       devops.oobbuf += devops.ooblen;
+               }
+               if (devops.oobbuf) {
+                       devops.ooblen = ops->ooblen - ops->oobretlen;
+                       if (!devops.ooblen)
+                               return 0;
+                       devops.oobbuf += devops.oobretlen;
+               }
                to = 0;
        }
        return -EINVAL;

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 06a930372b7a5eb174d03872763612ecc0753adb..a20f75fd8d61a1ba0faf4bcecfca81112175d646 100644 (file)
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -94,7 +94,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 
        if (from >= mtd->size)
                return -EINVAL;
-       if (from + ops->len > mtd->size)
+       if (ops->datbuf && from + ops->len > mtd->size)
                return -EINVAL;
        res = part->master->read_oob(part->master, from + part->offset, ops);
 
@@ -161,7 +161,7 @@ static int part_write_oob(struct mtd_info *mtd, loff_t to,
 
        if (to >= mtd->size)
                return -EINVAL;
-       if (to + ops->len > mtd->size)
+       if (ops->datbuf && to + ops->len > mtd->size)
                return -EINVAL;
        return part->master->write_oob(part->master, to + part->offset, ops);
 }

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 8df36e2c9a5397d3330775885332b3a435b18cb1..5dcb2e066ce71f3a5b8bb2f2ad37f9e7422f2620 100644 (file)
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -897,12 +897,11 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
  * @chip:      nand chip structure
  * @oob:       oob destination address
  * @ops:       oob ops structure
+ * @len:       size of oob to transfer
  */
 static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob,
-                                 struct mtd_oob_ops *ops)
+                                 struct mtd_oob_ops *ops, size_t len)
 {
-       size_t len = ops->ooblen;
-
        switch(ops->mode) {
 
        case MTD_OOB_PLACE:
@@ -960,6 +959,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
        int sndcmd = 1;
        int ret = 0;
        uint32_t readlen = ops->len;
+       uint32_t oobreadlen = ops->ooblen;
        uint8_t *bufpoi, *oob, *buf;
 
        stats = mtd->ecc_stats;
@@ -1006,10 +1006,17 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 
                        if (unlikely(oob)) {
                                /* Raw mode does data:oob:data:oob */
-                               if (ops->mode != MTD_OOB_RAW)
-                                       oob = nand_transfer_oob(chip, oob, ops);
-                               else
-                                       buf = nand_transfer_oob(chip, buf, ops);
+                               if (ops->mode != MTD_OOB_RAW) {
+                                       int toread = min(oobreadlen,
+                                               chip->ecc.layout->oobavail);
+                                       if (toread) {
+                                               oob = nand_transfer_oob(chip,
+                                                       oob, ops, toread);
+                                               oobreadlen -= toread;
+                                       }
+                               } else
+                                       buf = nand_transfer_oob(chip,
+                                               buf, ops, mtd->oobsize);
                        }
 
                        if (!(chip->options & NAND_NO_READRDY)) {
@@ -1056,6 +1063,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
        }
 
        ops->retlen = ops->len - (size_t) readlen;
+       if (oob)
+               ops->oobretlen = ops->ooblen - oobreadlen;
 
        if (ret)
                return ret;
@@ -1256,12 +1265,18 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
        int page, realpage, chipnr, sndcmd = 1;
        struct nand_chip *chip = mtd->priv;
        int blkcheck = (1 << (chip->phys_erase_shift - chip->page_shift)) - 1;
-       int readlen = ops->len;
+       int readlen = ops->ooblen;
+       int len;
        uint8_t *buf = ops->oobbuf;
 
        DEBUG(MTD_DEBUG_LEVEL3, "nand_read_oob: from = 0x%08Lx, len = %i\n",
              (unsigned long long)from, readlen);
 
+       if (ops->mode == MTD_OOB_RAW)
+               len = mtd->oobsize;
+       else
+               len = chip->ecc.layout->oobavail;
+
        chipnr = (int)(from >> chip->chip_shift);
        chip->select_chip(mtd, chipnr);
 
@@ -1271,7 +1286,9 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 
        while(1) {
                sndcmd = chip->ecc.read_oob(mtd, chip, page, sndcmd);
-               buf = nand_transfer_oob(chip, buf, ops);
+
+               len = min(len, readlen);
+               buf = nand_transfer_oob(chip, buf, ops, len);
 
                if (!(chip->options & NAND_NO_READRDY)) {
                        /*
@@ -1286,7 +1303,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
                                nand_wait_ready(mtd);
                }
 
-               readlen -= ops->ooblen;
+               readlen -= len;
                if (!readlen)
                        break;
 
@@ -1308,7 +1325,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
                        sndcmd = 1;
        }
 
-       ops->retlen = ops->len;
+       ops->oobretlen = ops->ooblen;
        return 0;
 }
 
@@ -1329,7 +1346,7 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
        ops->retlen = 0;
 
        /* Do not allow reads past end of device */
-       if ((from + ops->len) > mtd->size) {
+       if (ops->datbuf && (from + ops->len) > mtd->size) {
                DEBUG(MTD_DEBUG_LEVEL0, "nand_read_oob: "
                      "Attempt read beyond end of device\n");
                return -EINVAL;
@@ -1654,6 +1671,8 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
        }
 
        ops->retlen = ops->len - writelen;
+       if (unlikely(oob))
+               ops->oobretlen = ops->ooblen;
        return ret;
 }
 
@@ -1709,10 +1728,10 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
        struct nand_chip *chip = mtd->priv;
 
        DEBUG(MTD_DEBUG_LEVEL3, "nand_write_oob: to = 0x%08x, len = %i\n",
-             (unsigned int)to, (int)ops->len);
+             (unsigned int)to, (int)ops->ooblen);
 
        /* Do not allow write past end of page */
-       if ((ops->ooboffs + ops->len) > mtd->oobsize) {
+       if ((ops->ooboffs + ops->ooblen) > mtd->oobsize) {
                DEBUG(MTD_DEBUG_LEVEL0, "nand_write_oob: "
                      "Attempt to write past end of page\n");
                return -EINVAL;
@@ -1748,7 +1767,7 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
        if (status)
                return status;
 
-       ops->retlen = ops->len;
+       ops->oobretlen = ops->ooblen;
 
        return 0;
 }
@@ -1768,7 +1787,7 @@ static int nand_write_oob(struct mtd_info *mtd, loff_t to,
        ops->retlen = 0;
 
        /* Do not allow writes past end of device */
-       if ((to + ops->len) > mtd->size) {
+       if (ops->datbuf && (to + ops->len) > mtd->size) {
                DEBUG(MTD_DEBUG_LEVEL0, "nand_read_oob: "
                      "Attempt read beyond end of device\n");
                return -EINVAL;

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 9402653eb09b4982ac7d924beda88cb203442741..4e74fe9af29ef02cb0a0f2a0134651abbc3757b2 100644 (file)
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -333,7 +333,6 @@ static int scan_block_fast(struct mtd_info *mtd, struct nand_bbt_descr *bd,
        struct mtd_oob_ops ops;
        int j, ret;
 
-       ops.len = mtd->oobsize;
        ops.ooblen = mtd->oobsize;
        ops.oobbuf = buf;
        ops.ooboffs = 0;
@@ -676,10 +675,10 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
                                       "bad block table\n");
                        }
                        /* Read oob data */
-                       ops.len = (len >> this->page_shift) * mtd->oobsize;
+                       ops.ooblen = (len >> this->page_shift) * mtd->oobsize;
                        ops.oobbuf = &buf[len];
                        res = mtd->read_oob(mtd, to + mtd->writesize, &ops);
-                       if (res < 0 || ops.retlen != ops.len)
+                       if (res < 0 || ops.oobretlen != ops.ooblen)
                                goto outerr;
 
                        /* Calc the byte offset in the buffer */

diff --git a/drivers/mtd/nftlcore.c b/drivers/mtd/nftlcore.c
index d974adab3492c7ac1bd4a3988fd7f9d7f7acdb17..f4d38546068ff6f3fc2bf9bbcc00f70ce686caa3 100644 (file)
--- a/drivers/mtd/nftlcore.c
+++ b/drivers/mtd/nftlcore.c
@@ -147,10 +147,9 @@ int nftl_read_oob(struct mtd_info *mtd, loff_t offs, size_t len,
        ops.ooblen = len;
        ops.oobbuf = buf;
        ops.datbuf = NULL;
-       ops.len = len;
 
        res = mtd->read_oob(mtd, offs & ~(mtd->writesize - 1), &ops);
-       *retlen = ops.retlen;
+       *retlen = ops.oobretlen;
        return res;
 }
 
@@ -168,10 +167,9 @@ int nftl_write_oob(struct mtd_info *mtd, loff_t offs, size_t len,
        ops.ooblen = len;
        ops.oobbuf = buf;
        ops.datbuf = NULL;
-       ops.len = len;
 
        res = mtd->write_oob(mtd, offs & ~(mtd->writesize - 1), &ops);
-       *retlen = ops.retlen;
+       *retlen = ops.oobretlen;
        return res;
 }
 

diff --git a/drivers/mtd/ssfdc.c b/drivers/mtd/ssfdc.c
index 79d3bb659bfe713a6685e2fc34a4c01d48827ac8..e834cc16c9f997c8feefab76cad2a97d3b0ad867 100644 (file)
--- a/drivers/mtd/ssfdc.c
+++ b/drivers/mtd/ssfdc.c
@@ -172,13 +172,12 @@ static int read_raw_oob(struct mtd_info *mtd, loff_t offs, uint8_t *buf)
 
        ops.mode = MTD_OOB_RAW;
        ops.ooboffs = 0;
-       ops.ooblen = mtd->oobsize;
-       ops.len = OOB_SIZE;
+       ops.ooblen = OOB_SIZE;
        ops.oobbuf = buf;
        ops.datbuf = NULL;
 
        ret = mtd->read_oob(mtd, offs, &ops);
-       if (ret < 0 || ops.retlen != OOB_SIZE)
+       if (ret < 0 || ops.oobretlen != OOB_SIZE)
                return -1;
 
        return 0;

diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
index b9b700730dfeeac71b612f1e4365afec29a19e2c..dcb18e9a646edc4ae80b2f79dd9118be8b0a9013 100644 (file)
--- a/fs/jffs2/wbuf.c
+++ b/fs/jffs2/wbuf.c
@@ -968,8 +968,7 @@ int jffs2_check_oob_empty(struct jffs2_sb_info *c,
        int oobsize = c->mtd->oobsize;
        struct mtd_oob_ops ops;
 
-       ops.len = NR_OOB_SCAN_PAGES * oobsize;
-       ops.ooblen = oobsize;
+       ops.ooblen = NR_OOB_SCAN_PAGES * oobsize;
        ops.oobbuf = c->oobbuf;
        ops.ooboffs = 0;
        ops.datbuf = NULL;
@@ -982,10 +981,10 @@ int jffs2_check_oob_empty(struct jffs2_sb_info *c,
                return ret;
        }
 
-       if (ops.retlen < ops.len) {
+       if (ops.oobretlen < ops.ooblen) {
                D1(printk(KERN_WARNING "jffs2_check_oob_empty(): Read OOB "
                          "returned short read (%zd bytes not %d) for block "
-                         "at %08x\n", ops.retlen, ops.len, jeb->offset));
+                         "at %08x\n", ops.oobretlen, ops.ooblen, jeb->offset));
                return -EIO;
        }
 
@@ -1004,7 +1003,7 @@ int jffs2_check_oob_empty(struct jffs2_sb_info *c,
        }
 
        /* we know, we are aligned :) */
-       for (page = oobsize; page < ops.len; page += sizeof(long)) {
+       for (page = oobsize; page < ops.ooblen; page += sizeof(long)) {
                long dat = *(long *)(&ops.oobbuf[page]);
                if(dat != -1)
                        return 1;
@@ -1032,7 +1031,6 @@ int jffs2_check_nand_cleanmarker (struct jffs2_sb_info *c,
                return 2;
        }
 
-       ops.len = oobsize;
        ops.ooblen = oobsize;
        ops.oobbuf = c->oobbuf;
        ops.ooboffs = 0;
@@ -1047,10 +1045,10 @@ int jffs2_check_nand_cleanmarker (struct jffs2_sb_info *c,
                return ret;
        }
 
-       if (ops.retlen < ops.len) {
+       if (ops.oobretlen < ops.ooblen) {
                D1 (printk (KERN_WARNING "jffs2_check_nand_cleanmarker(): "
                            "Read OOB return short read (%zd bytes not %d) "
-                           "for block at %08x\n", ops.retlen, ops.len,
+                           "for block at %08x\n", ops.oobretlen, ops.ooblen,
                            jeb->offset));
                return -EIO;
        }
@@ -1089,8 +1087,7 @@ int jffs2_write_nand_cleanmarker(struct jffs2_sb_info *c,
        n.nodetype = cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER);
        n.totlen = cpu_to_je32(8);
 
-       ops.len = c->fsdata_len;
-       ops.ooblen = c->fsdata_len;;
+       ops.ooblen = c->fsdata_len;
        ops.oobbuf = (uint8_t *)&n;
        ops.ooboffs = c->fsdata_pos;
        ops.datbuf = NULL;
@@ -1104,10 +1101,10 @@ int jffs2_write_nand_cleanmarker(struct jffs2_sb_info *c,
                          jeb->offset, ret));
                return ret;
        }
-       if (ops.retlen != ops.len) {
+       if (ops.oobretlen != ops.ooblen) {
                D1(printk(KERN_WARNING "jffs2_write_nand_cleanmarker(): "
                          "Short write for block at %08x: %zd not %d\n",
-                         jeb->offset, ops.retlen, ops.len));
+                         jeb->offset, ops.oobretlen, ops.ooblen));
                return -EIO;
        }
        return 0;

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 94a443d45258485cf79e71f76c7d01bc2c39b2eb..4fc391ec9d01342ba68d76cf96a526faa69e83fd 100644 (file)
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -75,15 +75,12 @@ typedef enum {
  * struct mtd_oob_ops - oob operation operands
  * @mode:      operation mode
  *
- * @len:       number of bytes to write/read. When a data buffer is given
- *             (datbuf != NULL) this is the number of data bytes. When
- *             no data buffer is available this is the number of oob bytes.
+ * @len:       number of data bytes to write/read
  *
- * @retlen:    number of bytes written/read. When a data buffer is given
- *             (datbuf != NULL) this is the number of data bytes. When
- *             no data buffer is available this is the number of oob bytes.
+ * @retlen:    number of data bytes written/read
  *
- * @ooblen:    number of oob bytes per page
+ * @ooblen:    number of oob bytes to write/read
+ * @oobretlen: number of oob bytes written/read
  * @ooboffs:   offset of oob data in the oob area (only relevant when
  *             mode = MTD_OOB_PLACE)
  * @datbuf:    data buffer - if NULL only oob data are read/written
@@ -94,6 +91,7 @@ struct mtd_oob_ops {
        size_t          len;
        size_t          retlen;
        size_t          ooblen;
+       size_t          oobretlen;
        uint32_t        ooboffs;
        uint8_t         *datbuf;
        uint8_t         *oobbuf;