Re: [PATCH 2/6] nand_wait : warn if the nand is busy on exit

[Artem Bityutskiy <dedekind1@gmail.com>]

On Tue, 2011-06-28 at 17:03 +0200, Matthieu CASTET wrote:
> Artem Bityutskiy a écrit :
> > On Sun, 2011-06-26 at 18:26 +0200, Matthieu CASTET wrote:
> >> This patch allow to detect buggy driver/hardware with
> >> bad RnB (dev_ready) management.
> >> This check cost nothing and could help to detect bugs.
> >>
> >> Signed-off-by: Matthieu CASTET <matthieu.castet@parrot.com>
> >> ---
> >>  drivers/mtd/nand/nand_base.c |    2 ++
> >>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index a3c7fd3..095dfea 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -885,6 +885,8 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> >>  	led_trigger_event(nand_led_trigger, LED_OFF);
> >>  
> >>  	status = (int)chip->read_byte(mtd);
> >> +	/* This can happen if in case of timeout or buggy dev_ready */
> >> +	WARN_ON(!(status & NAND_STATUS_READY));
> >>  	return status;
> > 
> > This seem to completely miss the chip->dev_ready != NULL case, e.g.,
> > piece of code above is like this
> > 
> >                 while (time_before(jiffies, timeo)) {
> >                         if (chip->dev_ready) {
> >                                 if (chip->dev_ready(mtd))
> >                                         break;
> >                         } else {
> >                                 if (chip->read_byte(mtd) & NAND_STATUS_READY)
> >                                         break;
> >                         }
> >                         cond_resched();
> >                 }
> > 
> Sorry, I don't understand what you mean.
> 
> We don't care what's done in the loop (chip->dev_ready != NULL or, not). We only
> check when the loop exit, that READY bit is set in the status.

Well, the logic is suspicious.

1. For NAND with chip->dev_ready != NULL, why NAND_STATUS_READY should
be set? We do not check for this in the loop.

2. For NAND with chip->dev_ready != NULL, if NAND_STATUS_READY has to be
set at the end, why wouldn't we drop this chip_ready part completely? We
could just loop while NAND_STATUS_READY is not set.

Isn't this strange?

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)