Re: [PATCH 2/2] mtd: nand: Remove BUG() abuse in nand_scan_tail

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

On Sat, 2 Apr 2016 15:55:24 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Fri,  1 Apr 2016 18:29:24 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> 
> > There's no reason to BUG() when parameters are being
> > validated. Drivers can get things wrong, and it's much nicer
> > to just throw a noisy warn and fail gracefully, than calling
> > BUG() and throwing the whole system down the drain.
> 
> I'm fine with this change as long as all callers are checking
> nand_scan_tail() return value.

Actually, the s3c2410 driver is not checking nand_scan_tail() return
value. Could you send a v2 addressing that?

> 
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > ---
> >  drivers/mtd/nand/nand_base.c | 52 ++++++++++++++++++++++++++++----------------
> >  1 file changed, 33 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index befa04ef4a04..0fe644ebe264 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -4119,10 +4119,12 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  	struct nand_chip *chip = mtd_to_nand(mtd);
> >  	struct nand_ecc_ctrl *ecc = &chip->ecc;
> >  	struct nand_buffers *nbuf;
> > +	int ret;
> >  
> >  	/* New bad blocks should be marked in OOB, flash-based BBT, or both */
> > -	BUG_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
> > -			!(chip->bbt_options & NAND_BBT_USE_FLASH));
> > +	if (WARN_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
> > +		   !(chip->bbt_options & NAND_BBT_USE_FLASH)))
> > +		return -EINVAL;
> >  
> >  	if (!(chip->options & NAND_OWN_BUFFERS)) {
> >  		nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
> > @@ -4160,9 +4162,10 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  			ecc->layout = &nand_oob_128;
> >  			break;
> >  		default:
> > -			pr_warn("No oob scheme defined for oobsize %d\n",
> > -				   mtd->oobsize);
> > -			BUG();
> > +			WARN(1, "No oob scheme defined for oobsize %d\n",
> > +				mtd->oobsize);
> > +			ret = -EINVAL;
> > +			goto err_free;
> >  		}
> >  	}
> >  
> > @@ -4178,8 +4181,9 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  	case NAND_ECC_HW_OOB_FIRST:
> >  		/* Similar to NAND_ECC_HW, but a separate read_page handle */
> >  		if (!ecc->calculate || !ecc->correct || !ecc->hwctl) {
> > -			pr_warn("No ECC functions supplied; hardware ECC not possible\n");
> > -			BUG();
> > +			WARN(1, "No ECC functions supplied; hardware ECC not possible\n");
> > +			ret = -EINVAL;
> > +			goto err_free;
> >  		}
> >  		if (!ecc->read_page)
> >  			ecc->read_page = nand_read_page_hwecc_oob_first;
> > @@ -4209,8 +4213,9 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  		     ecc->read_page == nand_read_page_hwecc ||
> >  		     !ecc->write_page ||
> >  		     ecc->write_page == nand_write_page_hwecc)) {
> > -			pr_warn("No ECC functions supplied; hardware ECC not possible\n");
> > -			BUG();
> > +			WARN(1, "No ECC functions supplied; hardware ECC not possible\n");
> > +			ret = -EINVAL;
> > +			goto err_free;
> >  		}
> >  		/* Use standard syndrome read/write page function? */
> >  		if (!ecc->read_page)
> > @@ -4228,8 +4233,9 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  
> >  		if (mtd->writesize >= ecc->size) {
> >  			if (!ecc->strength) {
> > -				pr_warn("Driver must set ecc.strength when using hardware ECC\n");
> > -				BUG();
> > +				WARN(1, "Driver must set ecc.strength when using hardware ECC\n");
> > +				ret = -EINVAL;
> > +				goto err_free;
> >  			}
> >  			break;
> >  		}
> > @@ -4255,8 +4261,9 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  
> >  	case NAND_ECC_SOFT_BCH:
> >  		if (!mtd_nand_has_bch()) {
> > -			pr_warn("CONFIG_MTD_NAND_ECC_BCH not enabled\n");
> > -			BUG();
> > +			WARN(1, "CONFIG_MTD_NAND_ECC_BCH not enabled\n");
> > +			ret = -EINVAL;
> > +			goto err_free;
> >  		}
> >  		ecc->calculate = nand_bch_calculate_ecc;
> >  		ecc->correct = nand_bch_correct_data;
> > @@ -4281,8 +4288,9 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  		ecc->bytes = 0;
> >  		ecc->priv = nand_bch_init(mtd);
> >  		if (!ecc->priv) {
> > -			pr_warn("BCH ECC initialization failed!\n");
> > -			BUG();
> > +			WARN(1, "BCH ECC initialization failed!\n");
> > +			ret = -EINVAL;
> > +			goto err_free;
> >  		}
> >  		break;
> >  
> > @@ -4300,8 +4308,9 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  		break;
> >  
> >  	default:
> > -		pr_warn("Invalid NAND_ECC_MODE %d\n", ecc->mode);
> > -		BUG();
> > +		WARN(1, "Invalid NAND_ECC_MODE %d\n", ecc->mode);
> > +		ret = -EINVAL;
> > +		goto err_free;
> >  	}
> >  
> >  	/* For many systems, the standard OOB write also works for raw */
> > @@ -4331,8 +4340,9 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  	 */
> >  	ecc->steps = mtd->writesize / ecc->size;
> >  	if (ecc->steps * ecc->size != mtd->writesize) {
> > -		pr_warn("Invalid ECC parameters\n");
> > -		BUG();
> > +		WARN(1, "Invalid ECC parameters\n");
> > +		ret = -EINVAL;
> > +		goto err_free;
> >  	}
> >  	ecc->total = ecc->steps * ecc->bytes;
> >  
> > @@ -4410,6 +4420,10 @@ int nand_scan_tail(struct mtd_info *mtd)
> >  
> >  	/* Build bad block table */
> >  	return chip->scan_bbt(mtd);
> > +err_free:
> > +	if (!(chip->options & NAND_OWN_BUFFERS))
> > +		kfree(chip->buffers);
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL(nand_scan_tail);
> >  
> 
> 
> 



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