Re: [PATCH v3 3/4] mtd: Stop directly calling master ->_xxx() hooks from mtdpart code

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

+Ladislav

On Fri, 15 Dec 2017 13:39:53 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> 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.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> Changes in v3:
> - unconditionally assign part wrappers as suggested by Brian
> 
> Changes in v2:
> - new patch needed to fix a NULL pointer dereference BUG
> ---
>  drivers/mtd/mtdpart.c | 141 +++++++++++++++++++-------------------------------
>  1 file changed, 53 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index be088bccd593..e83c9d870b11 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -74,8 +74,7 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	int res;
>  
>  	stats = part->parent->ecc_stats;
> -	res = part->parent->_read(part->parent, from + part->offset, len,
> -				  retlen, buf);
> +	res = mtd_read(part->parent, from + part->offset, len, retlen, buf);

This change introduced a regression (reported by Ladislav) because
mtd_read() does not return the number of bitflips and instead returns 0
if we are below ->bitflips_threshold and -EUCLEAN if we're equal
or above. That's a problem in 2 situations:
1/ when ->bitflips_threshold is customized for a specific partition,
   the custom value is ignored in favor of the one set on the master MTD
   device
2/ when PARTITIONED_MASTER is not defined, ->bitflip_threshold is not
   initialized (->bitflip_threshold = 0), thus triggering -EUCLEAN
   every time we read a page

I fear this patch might introduce other subtle regression so I plan to
drop it entirely.

Regards,

Boris

>  	if (unlikely(mtd_is_eccerr(res)))
>  		mtd->ecc_stats.failed +=
>  			part->parent->ecc_stats.failed - stats.failed;