Re: [PATCH 08/13] mtd: rawnand: gpmi: return generated errors in gpmi_ecc_read_oob()

[Boris Brezillon <boris.brezillon@bootlin.com>]

Hi Sam,

On Thu, 26 Apr 2018 17:41:29 +0200
Sam Lefebvre <sam.lefebvre@essensium.com> wrote:

> Errors can be generated by nand_read_page_op(). It's important
> to check the error flags and pass them upwards.
> 
> Signed-off-by: Sam Lefebvre <sam.lefebvre@essensium.com>
> 
> ---
> 
> v2: also bail out before calling ->read_byte() (Boris)
> 
> When handling errors we need to bail out before calling
> ->read_byte().  
> ---
>  drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index 7ba00b8ab288..60bc1bac7741 100644
> --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> @@ -1334,13 +1334,18 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>  				int page)
>  {
>  	struct gpmi_nand_data *this = nand_get_controller_data(chip);
> +	int ret = 0;

No need to initialize ret to 0 here.

>  
>  	dev_dbg(this->dev, "page number is %d\n", page);
>  	/* clear the OOB buffer */
>  	memset(chip->oob_poi, ~0, mtd->oobsize);
>  
>  	/* Read out the conventional OOB. */
> -	nand_read_page_op(chip, page, mtd->writesize, NULL, 0);
> +	ret = nand_read_page_op(chip, page, mtd->writesize, NULL, 0);
> +

Please drop this empty line.

> +	if (ret)
> +		return ret;
> +
>  	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>  
>  	/*
> @@ -1350,11 +1355,15 @@ static int gpmi_ecc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>  	 */
>  	if (GPMI_IS_MX23(this)) {
>  		/* Read the block mark into the first byte of the OOB buffer. */
> -		nand_read_page_op(chip, page, 0, NULL, 0);
> +		ret = nand_read_page_op(chip, page, 0, NULL, 0);
> +

Here as well.

> +		if (ret)
> +			return ret;
> +
>  		chip->oob_poi[0] = chip->read_byte(mtd);
>  	}
>  
> -	return 0;
> +	return ret;

You can keep return 0 here.

>  }
>  
>  static int

Regards,

Boris