Re: [PATCH v2 1/2] mtd: Stop directly calling master ->_xxx() hooks from mtdpart code

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Le Sun, 25 Jun 2017 18:01:12 +0200,
Boris Brezillon <boris.brezillon@free-electrons.com> a écrit :

> The MTD layer provides several wrappers around mtd->_xxx() hooks. Call
> these wrappers instead of directly dereferencing the associated ->_xxx()
> pointer.
> 
> This change has been motivated by another rework letting the core
> handle the case where ->_read/write_oob() are implemented but not
> ->_read/write(). In this case, we want mtd_read/write() to fall back to
> ->_read/write_oob() when ->_read/write() are NULL. The problem is,  
> mtdpart is directly calling the ->_xxx() instead of using the wrappers,
> thus leading to a NULL pointer exception.
> 
> Even though we only need to do the change for part_read/write(), going
> through those wrappers for all kind of part -> master operation
> propagation is a good thing, because other wrappers might become
> smarter over time, and the duplicated check overhead (parameters will
> be checked at the partition and master level instead of only at the
> partition level) should be negligible.

Hm, this one is conflicting with Rafal's work on part parsers, but
before I send a v3 rebased on l2-mtd/master I'd like to have feedback
on the changes done here.

> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Changes since v1:
> - new patch needed to fix a NULL pointer dereference BUG
> ---
>  drivers/mtd/mtdpart.c | 71 ++++++++++++++++++++++-----------------------------
>  1 file changed, 31 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index ea5e5307f667..11486ec6cab0 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -68,8 +68,7 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	int res;
>  
>  	stats = part->master->ecc_stats;
> -	res = part->master->_read(part->master, from + part->offset, len,
> -				  retlen, buf);
> +	res = mtd_read(part->master, from + part->offset, len, retlen, buf);
>  	if (unlikely(mtd_is_eccerr(res)))
>  		mtd->ecc_stats.failed +=
>  			part->master->ecc_stats.failed - stats.failed;
> @@ -84,15 +83,15 @@ static int part_point(struct mtd_info *mtd, loff_t from, size_t len,
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
>  
> -	return part->master->_point(part->master, from + part->offset, len,
> -				    retlen, virt, phys);
> +	return mtd_point(part->master, from + part->offset, len, retlen, virt,
> +			 phys);
>  }
>  
>  static int part_unpoint(struct mtd_info *mtd, loff_t from, size_t len)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
>  
> -	return part->master->_unpoint(part->master, from + part->offset, len);
> +	return mtd_unpoint(part->master, from + part->offset, len);
>  }
>  
>  static unsigned long part_get_unmapped_area(struct mtd_info *mtd,
> @@ -103,8 +102,7 @@ static unsigned long part_get_unmapped_area(struct mtd_info *mtd,
>  	struct mtd_part *part = mtd_to_part(mtd);
>  
>  	offset += part->offset;
> -	return part->master->_get_unmapped_area(part->master, len, offset,
> -						flags);
> +	return mtd_get_unmapped_area(part->master, len, offset, flags);
>  }
>  
>  static int part_read_oob(struct mtd_info *mtd, loff_t from,
> @@ -132,7 +130,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
>  			return -EINVAL;
>  	}
>  
> -	res = part->master->_read_oob(part->master, from + part->offset, ops);
> +	res = mtd_read_oob(part->master, from + part->offset, ops);
>  	if (unlikely(res)) {
>  		if (mtd_is_bitflip(res))
>  			mtd->ecc_stats.corrected++;
> @@ -146,48 +144,43 @@ static int part_read_user_prot_reg(struct mtd_info *mtd, loff_t from,
>  		size_t len, size_t *retlen, u_char *buf)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_read_user_prot_reg(part->master, from, len,
> -						 retlen, buf);
> +	return mtd_read_user_prot_reg(part->master, from, len, retlen, buf);
>  }
>  
>  static int part_get_user_prot_info(struct mtd_info *mtd, size_t len,
>  				   size_t *retlen, struct otp_info *buf)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_get_user_prot_info(part->master, len, retlen,
> -						 buf);
> +	return mtd_get_user_prot_info(part->master, len, retlen, buf);
>  }
>  
>  static int part_read_fact_prot_reg(struct mtd_info *mtd, loff_t from,
>  		size_t len, size_t *retlen, u_char *buf)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_read_fact_prot_reg(part->master, from, len,
> -						 retlen, buf);
> +	return mtd_read_fact_prot_reg(part->master, from, len, retlen, buf);
>  }
>  
>  static int part_get_fact_prot_info(struct mtd_info *mtd, size_t len,
>  				   size_t *retlen, struct otp_info *buf)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_get_fact_prot_info(part->master, len, retlen,
> -						 buf);
> +	return mtd_get_fact_prot_info(part->master, len, retlen, buf);
>  }
>  
>  static int part_write(struct mtd_info *mtd, loff_t to, size_t len,
>  		size_t *retlen, const u_char *buf)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_write(part->master, to + part->offset, len,
> -				    retlen, buf);
> +	return mtd_write(part->master, to + part->offset, len, retlen, buf);
>  }
>  
>  static int part_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
>  		size_t *retlen, const u_char *buf)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_panic_write(part->master, to + part->offset, len,
> -					  retlen, buf);
> +	return mtd_panic_write(part->master, to + part->offset, len, retlen,
> +			       buf);
>  }
>  
>  static int part_write_oob(struct mtd_info *mtd, loff_t to,
> @@ -199,30 +192,29 @@ static int part_write_oob(struct mtd_info *mtd, loff_t to,
>  		return -EINVAL;
>  	if (ops->datbuf && to + ops->len > mtd->size)
>  		return -EINVAL;
> -	return part->master->_write_oob(part->master, to + part->offset, ops);
> +	return mtd_write_oob(part->master, to + part->offset, ops);
>  }
>  
>  static int part_write_user_prot_reg(struct mtd_info *mtd, loff_t from,
>  		size_t len, size_t *retlen, u_char *buf)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_write_user_prot_reg(part->master, from, len,
> -						  retlen, buf);
> +	return mtd_write_user_prot_reg(part->master, from, len, retlen, buf);
>  }
>  
>  static int part_lock_user_prot_reg(struct mtd_info *mtd, loff_t from,
>  		size_t len)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_lock_user_prot_reg(part->master, from, len);
> +	return mtd_lock_user_prot_reg(part->master, from, len);
>  }
>  
>  static int part_writev(struct mtd_info *mtd, const struct kvec *vecs,
>  		unsigned long count, loff_t to, size_t *retlen)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_writev(part->master, vecs, count,
> -				     to + part->offset, retlen);
> +	return mtd_writev(part->master, vecs, count, to + part->offset,
> +			  retlen);
>  }
>  
>  static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
> @@ -231,7 +223,7 @@ static int part_erase(struct mtd_info *mtd, struct erase_info *instr)
>  	int ret;
>  
>  	instr->addr += part->offset;
> -	ret = part->master->_erase(part->master, instr);
> +	ret = mtd_erase(part->master, instr);
>  	if (ret) {
>  		if (instr->fail_addr != MTD_FAIL_ADDR_UNKNOWN)
>  			instr->fail_addr -= part->offset;
> @@ -257,51 +249,51 @@ EXPORT_SYMBOL_GPL(mtd_erase_callback);
>  static int part_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_lock(part->master, ofs + part->offset, len);
> +	return mtd_lock(part->master, ofs + part->offset, len);
>  }
>  
>  static int part_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_unlock(part->master, ofs + part->offset, len);
> +	return mtd_unlock(part->master, ofs + part->offset, len);
>  }
>  
>  static int part_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_is_locked(part->master, ofs + part->offset, len);
> +	return mtd_is_locked(part->master, ofs + part->offset, len);
>  }
>  
>  static void part_sync(struct mtd_info *mtd)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	part->master->_sync(part->master);
> +	mtd_sync(part->master);
>  }
>  
>  static int part_suspend(struct mtd_info *mtd)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_suspend(part->master);
> +	return mtd_suspend(part->master);
>  }
>  
>  static void part_resume(struct mtd_info *mtd)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	part->master->_resume(part->master);
> +	mtd_resume(part->master);
>  }
>  
>  static int part_block_isreserved(struct mtd_info *mtd, loff_t ofs)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
>  	ofs += part->offset;
> -	return part->master->_block_isreserved(part->master, ofs);
> +	return mtd_block_isreserved(part->master, ofs);
>  }
>  
>  static int part_block_isbad(struct mtd_info *mtd, loff_t ofs)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
>  	ofs += part->offset;
> -	return part->master->_block_isbad(part->master, ofs);
> +	return mtd_block_isbad(part->master, ofs);
>  }
>  
>  static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
> @@ -310,7 +302,7 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  	int res;
>  
>  	ofs += part->offset;
> -	res = part->master->_block_markbad(part->master, ofs);
> +	res = mtd_block_markbad(part->master, ofs);
>  	if (!res)
>  		mtd->ecc_stats.badblocks++;
>  	return res;
> @@ -319,13 +311,13 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  static int part_get_device(struct mtd_info *mtd)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	return part->master->_get_device(part->master);
> +	return __get_mtd_device(part->master);
>  }
>  
>  static void part_put_device(struct mtd_info *mtd)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
> -	part->master->_put_device(part->master);
> +	__put_mtd_device(part->master);
>  }
>  
>  static int part_ooblayout_ecc(struct mtd_info *mtd, int section,
> @@ -353,8 +345,7 @@ static int part_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
>  {
>  	struct mtd_part *part = mtd_to_part(mtd);
>  
> -	return part->master->_max_bad_blocks(part->master,
> -					     ofs + part->offset, len);
> +	return mtd_max_bad_blocks(part->master, ofs + part->offset, len);
>  }
>  
>  static inline void free_partition(struct mtd_part *p)