From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from down.free-electrons.com ([37.187.137.238] helo=mail.free-electrons.com) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1anUJH-0001tn-Hk for linux-mtd@lists.infradead.org; Tue, 05 Apr 2016 16:58:36 +0000 Date: Tue, 5 Apr 2016 18:58:08 +0200 From: Boris Brezillon To: Ezequiel Garcia Cc: , Brian Norris , Richard Weinberger , David Woodhouse Subject: Re: [PATCH 2/2] mtd: nand: Remove BUG() abuse in nand_scan_tail Message-ID: <20160405185808.4693768d@bbrezillon> In-Reply-To: <1459546164-6269-3-git-send-email-ezequiel@vanguardiasur.com.ar> References: <1459546164-6269-1-git-send-email-ezequiel@vanguardiasur.com.ar> <1459546164-6269-3-git-send-email-ezequiel@vanguardiasur.com.ar> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Fri, 1 Apr 2016 18:29:24 -0300 Ezequiel Garcia 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. > > Signed-off-by: Ezequiel Garcia Applied. Thanks, Boris > --- > 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