Re: [PATCH v4 3/3] mtd: Remove duplicate checks on mtd_oob_ops parameter

[Miquel RAYNAL <miquel.raynal@free-electrons.com>]

Hello Boris,

On Mon,  8 Jan 2018 22:15:42 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Some of the check done in custom ->_read/write_oob() implementation
> are already done by the core (in mtd_check_oob_ops()).

Not sure this is relevant here as your series introduces changes for
the SPI NAND framework, but there are other places where these checks
are, IMHO, also redundant and could be removed. The "past end" string
when grepped in the MTD folder core returns a few more hits.

In the NAND core:
  - nand_do_read_oob()
  - nand_read_oob()
  - nand_do_write_oob()
  - nand_write_oob()
Maybe also in onenand_base.c, but I am less confident for this one:
  - onenand_bbt_read_oob()

What do you think?

Thanks,
Miquèl

> 
> Suggested-by: Peter Pan <peterpansjtu@gmail.com>
> [Remove redundant checks done in mtdpart.c]
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Changes in v4:
> - integrated into the SPI NAND preparation patchset
> 
> Changes in v3:
> - Rebased on mtd/next after "mtd: Stop directly calling master
> ->_xxx() hooks from mtdpart code" had been dropped
> 
> Changes in v2:
> - Merge "mtd: Remove unneeded checks in part_{read,write}_oob()" into
>   this commit
> ---
>  drivers/mtd/devices/docg3.c        |  5 -----
>  drivers/mtd/mtdpart.c              | 23 -----------------------
>  drivers/mtd/nand/nand_base.c       | 31
> ------------------------------- drivers/mtd/onenand/onenand_base.c |
> 18 ------------------ 4 files changed, 77 deletions(-)
> 
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index 5fb5e93d1547..a85af236b44d 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -904,9 +904,6 @@ static int doc_read_oob(struct mtd_info *mtd,
> loff_t from, if (ooblen % DOC_LAYOUT_OOB_SIZE)
>  		return -EINVAL;
>  
> -	if (from + len > mtd->size)
> -		return -EINVAL;
> -
>  	ops->oobretlen = 0;
>  	ops->retlen = 0;
>  	ret = 0;
> @@ -1441,8 +1438,6 @@ static int doc_write_oob(struct mtd_info *mtd,
> loff_t ofs, if (len && ooblen &&
>  	    (len / DOC_LAYOUT_PAGE_SIZE) != (ooblen / oobdelta))
>  		return -EINVAL;
> -	if (ofs + len > mtd->size)
> -		return -EINVAL;
>  
>  	ops->oobretlen = 0;
>  	ops->retlen = 0;
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 07ef168ded98..131bb1d52871 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -108,25 +108,6 @@ static int part_read_oob(struct mtd_info *mtd,
> loff_t from, struct mtd_ecc_stats stats;
>  	int res;
>  
> -	if (from >= mtd->size)
> -		return -EINVAL;
> -	if (ops->datbuf && from + ops->len > mtd->size)
> -		return -EINVAL;
> -
> -	/*
> -	 * If OOB is also requested, make sure that we do not read
> past the end
> -	 * of this partition.
> -	 */
> -	if (ops->oobbuf) {
> -		size_t len, pages;
> -
> -		len = mtd_oobavail(mtd, ops);
> -		pages = mtd_div_by_ws(mtd->size, mtd);
> -		pages -= mtd_div_by_ws(from, mtd);
> -		if (ops->ooboffs + ops->ooblen > pages * len)
> -			return -EINVAL;
> -	}
> -
>  	res = part->parent->_read_oob(part->parent, from +
> part->offset, ops); if (unlikely(mtd_is_eccerr(res)))
>  		mtd->ecc_stats.failed +=
> @@ -190,10 +171,6 @@ static int part_write_oob(struct mtd_info *mtd,
> loff_t to, {
>  	struct mtd_part *part = mtd_to_part(mtd);
>  
> -	if (to >= mtd->size)
> -		return -EINVAL;
> -	if (ops->datbuf && to + ops->len > mtd->size)
> -		return -EINVAL;
>  	return part->parent->_write_oob(part->parent, to +
> part->offset, ops); }
>  
> diff --git a/drivers/mtd/nand/nand_base.c
> b/drivers/mtd/nand/nand_base.c index 889ceadbf607..61eeac233683 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2187,21 +2187,6 @@ static int nand_do_read_oob(struct mtd_info
> *mtd, loff_t from, 
>  	len = mtd_oobavail(mtd, ops);
>  
> -	if (unlikely(ops->ooboffs >= len)) {
> -		pr_debug("%s: attempt to start read outside oob\n",
> -				__func__);
> -		return -EINVAL;
> -	}
> -
> -	/* Do not allow reads past end of device */
> -	if (unlikely(from >= mtd->size ||
> -		     ops->ooboffs + readlen > ((mtd->size >>
> chip->page_shift) -
> -					(from >> chip->page_shift))
> * len)) {
> -		pr_debug("%s: attempt to read beyond end of
> device\n",
> -				__func__);
> -		return -EINVAL;
> -	}
> -
>  	chipnr = (int)(from >> chip->chip_shift);
>  	chip->select_chip(mtd, chipnr);
>  
> @@ -2820,22 +2805,6 @@ static int nand_do_write_oob(struct mtd_info
> *mtd, loff_t to, return -EINVAL;
>  	}
>  
> -	if (unlikely(ops->ooboffs >= len)) {
> -		pr_debug("%s: attempt to start write outside oob\n",
> -				__func__);
> -		return -EINVAL;
> -	}
> -
> -	/* Do not allow write past end of device */
> -	if (unlikely(to >= mtd->size ||
> -		     ops->ooboffs + ops->ooblen >
> -			((mtd->size >> chip->page_shift) -
> -			 (to >> chip->page_shift)) * len)) {
> -		pr_debug("%s: attempt to write beyond end of
> device\n",
> -				__func__);
> -		return -EINVAL;
> -	}
> -
>  	chipnr = (int)(to >> chip->chip_shift);
>  
>  	/*
> diff --git a/drivers/mtd/onenand/onenand_base.c
> b/drivers/mtd/onenand/onenand_base.c index 050ba8a87543..979f4031f23c
> 100644 --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -1383,15 +1383,6 @@ static int onenand_read_oob_nolock(struct
> mtd_info *mtd, loff_t from, return -EINVAL;
>  	}
>  
> -	/* Do not allow reads past end of device */
> -	if (unlikely(from >= mtd->size ||
> -		     column + len > ((mtd->size >> this->page_shift)
> -
> -				     (from >> this->page_shift)) *
> oobsize)) {
> -		printk(KERN_ERR "%s: Attempted to read beyond end of
> device\n",
> -			__func__);
> -		return -EINVAL;
> -	}
> -
>  	stats = mtd->ecc_stats;
>  
>  	readcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_READ :
> ONENAND_CMD_READOOB; @@ -2024,15 +2015,6 @@ static int
> onenand_write_oob_nolock(struct mtd_info *mtd, loff_t to, return
> -EINVAL; }
>  
> -	/* Do not allow reads past end of device */
> -	if (unlikely(to >= mtd->size ||
> -		     column + len > ((mtd->size >> this->page_shift)
> -
> -				     (to >> this->page_shift)) *
> oobsize)) {
> -		printk(KERN_ERR "%s: Attempted to write past end of
> device\n",
> -		       __func__);
> -		return -EINVAL;
> -	}
> -
>  	oobbuf = this->oob_buf;
>  
>  	oobcmd = ONENAND_IS_4KB_PAGE(this) ? ONENAND_CMD_PROG :
> ONENAND_CMD_PROGOOB;



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com