[PATCH 0/2] nand: Remove BUG abuse

[PATCH 0/2] nand: Remove BUG abuse

[Ezequiel Garcia]

Hi,

While using nandsim to debug an issue with an UBI image,
I hit a BUG() when passing some crazy ID values to nandsim.

I've always felt that nand_base.c is sort of abusing the 
BUG() macro, so decided to fix it.

There are other BUG() uses, but they aren't in the nand_scan
path, so I'm not touching them.

Patches apply cleanly on v4.6-rc1.

Ezequiel Garcia (2):
  mtd: nand: Drop mtd.owner requirement in nand_scan
  mtd: nand: Remove BUG() abuse in nand_scan_tail

 drivers/mtd/nand/nand_base.c | 62 ++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 28 deletions(-)

-- 
2.7.0

[PATCH 1/2] mtd: nand: Drop mtd.owner requirement in nand_scan

[Ezequiel Garcia]

Since commit 807f16d4db95 ("mtd: core: set some defaults
when dev.parent is set"), it's now legal for drivers
to call nand_scan and nand_scan_ident without setting
mtd.owner.

Drop the check and while at it remove the BUG() abuse.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/mtd/nand/nand_base.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index c3733a10a6e7..befa04ef4a04 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4013,7 +4013,6 @@ static int nand_dt_init(struct nand_chip *chip)
  * This is the first phase of the normal nand_scan() function. It reads the
  * flash ID and sets up MTD fields accordingly.
  *
- * The mtd->owner field must be set to the module of the caller.
  */
 int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 		    struct nand_flash_dev *table)
@@ -4433,19 +4432,12 @@ EXPORT_SYMBOL(nand_scan_tail);
  *
  * This fills out all the uninitialized function pointers with the defaults.
  * The flash ID is read and the mtd/chip structures are filled with the
- * appropriate values. The mtd->owner field must be set to the module of the
- * caller.
+ * appropriate values.
  */
 int nand_scan(struct mtd_info *mtd, int maxchips)
 {
 	int ret;
 
-	/* Many callers got this wrong, so check for it for a while... */
-	if (!mtd->owner && caller_is_module()) {
-		pr_crit("%s called with NULL mtd->owner!\n", __func__);
-		BUG();
-	}
-
 	ret = nand_scan_ident(mtd, maxchips, NULL);
 	if (!ret)
 		ret = nand_scan_tail(mtd);
-- 
2.7.0

Re: [PATCH 1/2] mtd: nand: Drop mtd.owner requirement in nand_scan

[Richard Weinberger]

Am 01.04.2016 um 23:29 schrieb Ezequiel Garcia:
> Since commit 807f16d4db95 ("mtd: core: set some defaults
> when dev.parent is set"), it's now legal for drivers
> to call nand_scan and nand_scan_ident without setting
> mtd.owner.
> 
> Drop the check and while at it remove the BUG() abuse.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/mtd/nand/nand_base.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index c3733a10a6e7..befa04ef4a04 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4013,7 +4013,6 @@ static int nand_dt_init(struct nand_chip *chip)
>   * This is the first phase of the normal nand_scan() function. It reads the
>   * flash ID and sets up MTD fields accordingly.
>   *
> - * The mtd->owner field must be set to the module of the caller.
>   */
>  int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  		    struct nand_flash_dev *table)
> @@ -4433,19 +4432,12 @@ EXPORT_SYMBOL(nand_scan_tail);
>   *
>   * This fills out all the uninitialized function pointers with the defaults.
>   * The flash ID is read and the mtd/chip structures are filled with the
> - * appropriate values. The mtd->owner field must be set to the module of the
> - * caller.
> + * appropriate values.
>   */
>  int nand_scan(struct mtd_info *mtd, int maxchips)
>  {
>  	int ret;
>  
> -	/* Many callers got this wrong, so check for it for a while... */
> -	if (!mtd->owner && caller_is_module()) {
> -		pr_crit("%s called with NULL mtd->owner!\n", __func__);
> -		BUG();
> -	}

Look okay to me.
Do we have an in-kernel user which currently hits this condition?

Thanks,
//richard

Re: [PATCH 1/2] mtd: nand: Drop mtd.owner requirement in nand_scan

[Ezequiel Garcia]

On 1 April 2016 at 18:57, Richard Weinberger <richard@nod.at> wrote:
> Am 01.04.2016 um 23:29 schrieb Ezequiel Garcia:
>> Since commit 807f16d4db95 ("mtd: core: set some defaults
>> when dev.parent is set"), it's now legal for drivers
>> to call nand_scan and nand_scan_ident without setting
>> mtd.owner.
>>
>> Drop the check and while at it remove the BUG() abuse.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> ---
>>  drivers/mtd/nand/nand_base.c | 10 +---------
>>  1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index c3733a10a6e7..befa04ef4a04 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -4013,7 +4013,6 @@ static int nand_dt_init(struct nand_chip *chip)
>>   * This is the first phase of the normal nand_scan() function. It reads the
>>   * flash ID and sets up MTD fields accordingly.
>>   *
>> - * The mtd->owner field must be set to the module of the caller.
>>   */
>>  int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>>                   struct nand_flash_dev *table)
>> @@ -4433,19 +4432,12 @@ EXPORT_SYMBOL(nand_scan_tail);
>>   *
>>   * This fills out all the uninitialized function pointers with the defaults.
>>   * The flash ID is read and the mtd/chip structures are filled with the
>> - * appropriate values. The mtd->owner field must be set to the module of the
>> - * caller.
>> + * appropriate values.
>>   */
>>  int nand_scan(struct mtd_info *mtd, int maxchips)
>>  {
>>       int ret;
>>
>> -     /* Many callers got this wrong, so check for it for a while... */
>> -     if (!mtd->owner && caller_is_module()) {
>> -             pr_crit("%s called with NULL mtd->owner!\n", __func__);
>> -             BUG();
>> -     }
>
> Look okay to me.
> Do we have an in-kernel user which currently hits this condition?
>

I think that (after the recent mtd.owner swept) all the drivers that
use nand_scan, will hit this condition if built as modules.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 1/2] mtd: nand: Drop mtd.owner requirement in nand_scan

[Brian Norris]

On Fri, Apr 01, 2016 at 06:29:23PM -0300, Ezequiel Garcia wrote:
> Since commit 807f16d4db95 ("mtd: core: set some defaults
> when dev.parent is set"), it's now legal for drivers
> to call nand_scan and nand_scan_ident without setting
> mtd.owner.
> 
> Drop the check and while at it remove the BUG() abuse.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/mtd/nand/nand_base.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index c3733a10a6e7..befa04ef4a04 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4013,7 +4013,6 @@ static int nand_dt_init(struct nand_chip *chip)
>   * This is the first phase of the normal nand_scan() function. It reads the
>   * flash ID and sets up MTD fields accordingly.
>   *
> - * The mtd->owner field must be set to the module of the caller.
>   */
>  int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  		    struct nand_flash_dev *table)
> @@ -4433,19 +4432,12 @@ EXPORT_SYMBOL(nand_scan_tail);
>   *
>   * This fills out all the uninitialized function pointers with the defaults.
>   * The flash ID is read and the mtd/chip structures are filled with the
> - * appropriate values. The mtd->owner field must be set to the module of the
> - * caller.
> + * appropriate values.
>   */
>  int nand_scan(struct mtd_info *mtd, int maxchips)
>  {
>  	int ret;
>  
> -	/* Many callers got this wrong, so check for it for a while... */
> -	if (!mtd->owner && caller_is_module()) {
> -		pr_crit("%s called with NULL mtd->owner!\n", __func__);
> -		BUG();
> -	}

Ooh, yikes! Forgot this was there. I guess no one noticed, because fewer
drivers are using plain nand_scan() these days (instead of splitting up
nand_scan_ident() and nand_scan_tail()), and also, many NAND users don't
run their drivers as modules.

Anyway, this is probably worth -stable, right? (i.e., "Fixes:
807f16d4db95 ..." and "Cc: <stable@vger.kernel.org>")

I can take this directly, or Boris, if you feel like there will be other
for-v4.5 NAND material, you can queue this up instead.

Brian

> -
>  	ret = nand_scan_ident(mtd, maxchips, NULL);
>  	if (!ret)
>  		ret = nand_scan_tail(mtd);
> -- 
> 2.7.0
> 

Re: [PATCH 1/2] mtd: nand: Drop mtd.owner requirement in nand_scan

[Boris Brezillon]

On Fri, 1 Apr 2016 15:26:49 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Fri, Apr 01, 2016 at 06:29:23PM -0300, Ezequiel Garcia wrote:
> > Since commit 807f16d4db95 ("mtd: core: set some defaults
> > when dev.parent is set"), it's now legal for drivers
> > to call nand_scan and nand_scan_ident without setting
> > mtd.owner.
> > 
> > Drop the check and while at it remove the BUG() abuse.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> > ---
> >  drivers/mtd/nand/nand_base.c | 10 +---------
> >  1 file changed, 1 insertion(+), 9 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index c3733a10a6e7..befa04ef4a04 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -4013,7 +4013,6 @@ static int nand_dt_init(struct nand_chip *chip)
> >   * This is the first phase of the normal nand_scan() function. It reads the
> >   * flash ID and sets up MTD fields accordingly.
> >   *
> > - * The mtd->owner field must be set to the module of the caller.
> >   */
> >  int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> >  		    struct nand_flash_dev *table)
> > @@ -4433,19 +4432,12 @@ EXPORT_SYMBOL(nand_scan_tail);
> >   *
> >   * This fills out all the uninitialized function pointers with the defaults.
> >   * The flash ID is read and the mtd/chip structures are filled with the
> > - * appropriate values. The mtd->owner field must be set to the module of the
> > - * caller.
> > + * appropriate values.
> >   */
> >  int nand_scan(struct mtd_info *mtd, int maxchips)
> >  {
> >  	int ret;
> >  
> > -	/* Many callers got this wrong, so check for it for a while... */
> > -	if (!mtd->owner && caller_is_module()) {
> > -		pr_crit("%s called with NULL mtd->owner!\n", __func__);
> > -		BUG();
> > -	}
> 
> Ooh, yikes! Forgot this was there. I guess no one noticed, because fewer
> drivers are using plain nand_scan() these days (instead of splitting up
> nand_scan_ident() and nand_scan_tail()), and also, many NAND users don't
> run their drivers as modules.
> 
> Anyway, this is probably worth -stable, right? (i.e., "Fixes:
> 807f16d4db95 ..." and "Cc: <stable@vger.kernel.org>")
> 
> I can take this directly, or Boris, if you feel like there will be other
> for-v4.5 NAND material, you can queue this up instead.

No, take it directly.

On a more general note, not sure creating a nand/fixes branch and
sending you PRs after each -rc (if fixes are available of course) is
really efficient. Maybe I'm wrong, but I don't expect to see more than a
couple of fixes per release, and it's probably better if you keep
taking them directly (with my acks).

What do you think?


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

Re: [PATCH 1/2] mtd: nand: Drop mtd.owner requirement in nand_scan

[Brian Norris]

On Sat, Apr 02, 2016 at 03:52:16PM +0200, Boris Brezillon wrote:
> On Fri, 1 Apr 2016 15:26:49 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
> > On Fri, Apr 01, 2016 at 06:29:23PM -0300, Ezequiel Garcia wrote:
> > > Since commit 807f16d4db95 ("mtd: core: set some defaults
> > > when dev.parent is set"), it's now legal for drivers
> > > to call nand_scan and nand_scan_ident without setting
> > > mtd.owner.
> > > 
> > > Drop the check and while at it remove the BUG() abuse.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
[...]

> > 
> > Ooh, yikes! Forgot this was there. I guess no one noticed, because fewer
> > drivers are using plain nand_scan() these days (instead of splitting up
> > nand_scan_ident() and nand_scan_tail()), and also, many NAND users don't
> > run their drivers as modules.
> > 
> > Anyway, this is probably worth -stable, right? (i.e., "Fixes:
> > 807f16d4db95 ..." and "Cc: <stable@vger.kernel.org>")
> > 
> > I can take this directly, or Boris, if you feel like there will be other
> > for-v4.5 NAND material, you can queue this up instead.
> 
> No, take it directly.

Applied patch 1 to linux-mtd.git. Thanks!

> On a more general note, not sure creating a nand/fixes branch and
> sending you PRs after each -rc (if fixes are available of course) is
> really efficient. Maybe I'm wrong, but I don't expect to see more than a
> couple of fixes per release, and it's probably better if you keep
> taking them directly (with my acks).
> 
> What do you think?

That's probably fine. I'll still keep my eyes open for fixes like this,
but ping me if you think I've missed anything.

Brian

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

[Ezequiel Garcia]

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 <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);
 
-- 
2.7.0

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

[Richard Weinberger]

Am 01.04.2016 um 23:29 schrieb Ezequiel Garcia:
> 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 <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/mtd/nand/nand_base.c | 52 ++++++++++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 19 deletions(-)

This makes sense.

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

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

[Boris Brezillon]

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.

> 
> 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

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

[Boris Brezillon]

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

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

[Ezequiel Garcia]

On 4 April 2016 at 12:20, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> 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?
>

Hmm, I don't see how that relates to this patch.
As far as I can see, it's two completely independent issues.

Or am I missing something here?
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

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

[Richard Weinberger]

Am 04.04.2016 um 17:20 schrieb Boris Brezillon:
> 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?

And maybe add __must_check to nand_scan_tail() such that we catch issues like
these.

Thanks,
//richard

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

[Ezequiel Garcia]

On 4 April 2016 at 12:30, Richard Weinberger <richard@nod.at> wrote:
> Am 04.04.2016 um 17:20 schrieb Boris Brezillon:
>> 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?
>
> And maybe add __must_check to nand_scan_tail() such that we catch issues like
> these.
>

In fact, why not adding must_check to all the functions that can fail
in the kernel?

That'll help catch even more issues ;-)
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

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

[Boris Brezillon]

On Mon, 4 Apr 2016 12:34:00 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> On 4 April 2016 at 12:30, Richard Weinberger <richard@nod.at> wrote:
> > Am 04.04.2016 um 17:20 schrieb Boris Brezillon:
> >> 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?
> >
> > And maybe add __must_check to nand_scan_tail() such that we catch issues like
> > these.
> >
> 
> In fact, why not adding must_check to all the functions that can fail
> in the kernel?
> 
> That'll help catch even more issues ;-)

I'll take your patch and patch the s3c2410 driver myself. Still think
that keeping the BUG() calls until all callers have been patched to
check nand_scan_tail() return code would be safer, but I don't care
enough to spend more time arguing on this :P.

Thanks,

Boris

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

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

[Brian Norris]

On Mon, Apr 04, 2016 at 05:20:48PM +0200, Boris Brezillon wrote:
> 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?

One could argue that we as a kernel community don't care about those
systems which are currently configured to hit 100%-reproducible BUG()
statements at boot time, and so this wouldn't really be a regression.
Also, there are already error cases in nand_scan_tail() that might
return non-zero, so such drivers are already broken.

Of course, it'd be nice to fix these drivers anyway.

Brian

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

[Boris Brezillon]

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.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

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

Re: [PATCH 0/2] nand: Remove BUG abuse

[Boris Brezillon]

Hi Ezequiel,

On Fri,  1 Apr 2016 18:29:22 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> Hi,
> 
> While using nandsim to debug an issue with an UBI image,
> I hit a BUG() when passing some crazy ID values to nandsim.
> 
> I've always felt that nand_base.c is sort of abusing the 
> BUG() macro, so decided to fix it.
> 
> There are other BUG() uses, but they aren't in the nand_scan
> path, so I'm not touching them.

Now that you've opened the door to this rework, can you also patch other
locations where BUG() is employed? :)

Thanks,

Boris

> 
> Patches apply cleanly on v4.6-rc1.
> 
> Ezequiel Garcia (2):
>   mtd: nand: Drop mtd.owner requirement in nand_scan
>   mtd: nand: Remove BUG() abuse in nand_scan_tail
> 
>  drivers/mtd/nand/nand_base.c | 62 ++++++++++++++++++++++++--------------------
>  1 file changed, 34 insertions(+), 28 deletions(-)
> 



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

Re: [PATCH 0/2] nand: Remove BUG abuse

[Ezequiel Garcia]

On 2 April 2016 at 10:55, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Ezequiel,
>
> On Fri,  1 Apr 2016 18:29:22 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>
>> Hi,
>>
>> While using nandsim to debug an issue with an UBI image,
>> I hit a BUG() when passing some crazy ID values to nandsim.
>>
>> I've always felt that nand_base.c is sort of abusing the
>> BUG() macro, so decided to fix it.
>>
>> There are other BUG() uses, but they aren't in the nand_scan
>> path, so I'm not touching them.
>
> Now that you've opened the door to this rework, can you also patch other
> locations where BUG() is employed? :)
>

Well, it's true that BUG() is abused all over the place, but most of them
should be of the "not reached" type.

So, I fixed these two because they *are* reached and they are in the core.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 0/2] nand: Remove BUG abuse

[Boris Brezillon]

On Sat, 2 Apr 2016 12:37:06 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> On 4 April 2016 at 12:20, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > 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?
> >
> 
> Hmm, I don't see how that relates to this patch.
> As far as I can see, it's two completely independent issues.
> 
> Or am I missing something here?

Well, you're removing BUG() calls and are returning an error instead, so
if existing nand_scan_tail() callers don't check the return status you
may hide an existing bug...

I know it's unlikely to happen, but I'd still prefer to have all
nand_scan_tail() callers to check the return value before removing
those calls to BUG().

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

Re: [PATCH 0/2] nand: Remove BUG abuse

[Ezequiel Garcia]

On 4 April 2016 at 12:33, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Sat, 2 Apr 2016 12:37:06 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>
>> On 4 April 2016 at 12:20, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> > 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?
>> >
>>
>> Hmm, I don't see how that relates to this patch.
>> As far as I can see, it's two completely independent issues.
>>
>> Or am I missing something here?
>
> Well, you're removing BUG() calls and are returning an error instead, so
> if existing nand_scan_tail() callers don't check the return status you
> may hide an existing bug...
>

Yes, I am removing a BUG() - thus replacing an oops (potentially a panic),
with an error return.

However, nand_scan_tail can already fail and return an error
for several other reasons.

> I know it's unlikely to happen, but I'd still prefer to have all
> nand_scan_tail() callers to check the return value before removing
> those calls to BUG().
>

It's not about unlikeliness, but about correlation and bisectability.
IMO, the fact that some drivers don't check the return of nand_scan_tail
is a bug on its own, and has nothing to do with this patch.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar