[U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop
Marek Vasut
marex at denx.de
Mon Dec 21 15:47:28 CET 2015
On Monday, December 21, 2015 at 11:56:30 AM, Masahiro Yamada wrote:
> Hi Marek,
Hi!
> 2015-12-21 19:45 GMT+09:00 Marek Vasut <marex at denx.de>:
> > On Monday, December 21, 2015 at 11:32:09 AM, Masahiro Yamada wrote:
> >> Hi Marek,
> >>
> >> 2015-12-21 19:16 GMT+09:00 Marek Vasut <marex at denx.de>:
> >> > On Monday, December 21, 2015 at 09:40:16 AM, Masahiro Yamada wrote:
> >> >> Hi Marek,
> >> >
> >> > Hi,
> >> >
> >> >> 2015-12-20 11:59 GMT+09:00 Marek Vasut <marex at denx.de>:
> >> >> > This particular unbounded loop in the Denali NAND reset routine can
> >> >> > lead to a system hang in case neither of the Timeout and Completed
> >> >> > bits get set.
> >> >> >
> >> >> > Refactor the code a bit so it's readable and implement timer so the
> >> >> > loop is bounded instead. This way the complete hang can be
> >> >> > prevented even if the NAND fails to reset.
> >> >> >
> >> >> > Signed-off-by: Marek Vasut <marex at denx.de>
> >> >> > Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
> >> >> > Cc: Scott Wood <scottwood at freescale.com>
> >> >> > ---
> >> >> >
> >> >> > drivers/mtd/nand/denali.c | 26 ++++++++++++++++++++------
> >> >> > 1 file changed, 20 insertions(+), 6 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> >> >> > index 192be7d..8a8cca9 100644
> >> >> > --- a/drivers/mtd/nand/denali.c
> >> >> > +++ b/drivers/mtd/nand/denali.c
> >> >> > @@ -199,6 +199,7 @@ static void reset_bank(struct denali_nand_info
> >> >> > *denali)
> >> >> >
> >> >> > static uint32_t denali_nand_reset(struct denali_nand_info *denali)
> >> >> > {
> >> >> >
> >> >> > int i;
> >> >> >
> >> >> > + u32 start, reg;
> >> >> >
> >> >> > for (i = 0; i < denali->max_banks; i++)
> >> >> >
> >> >> > writel(INTR_STATUS__RST_COMP |
> >> >> > INTR_STATUS__TIME_OUT,
> >> >> >
> >> >> > @@ -206,12 +207,25 @@ static uint32_t denali_nand_reset(struct
> >> >> > denali_nand_info *denali)
> >> >> >
> >> >> > for (i = 0; i < denali->max_banks; i++) {
> >> >> >
> >> >> > writel(1 << i, denali->flash_reg + DEVICE_RESET);
> >> >> >
> >> >> > - while (!(readl(denali->flash_reg + INTR_STATUS(i))
> >> >> > & - (INTR_STATUS__RST_COMP |
> >> >> > INTR_STATUS__TIME_OUT))) - if
> >> >> > (readl(denali->flash_reg + INTR_STATUS(i)) & -
> >> >> >
> >> >> > INTR_STATUS__TIME_OUT)
> >> >> >
> >> >> > - debug("NAND Reset operation timed
> >> >> > out on bank" - " %d\n", i); +
> >> >> > + start = get_timer(0);
> >> >> > + while (1) {
> >> >> > + reg = readl(denali->flash_reg +
> >> >> > INTR_STATUS(i)); + if (reg &
> >> >> > INTR_STATUS__TIME_OUT) {
> >> >> > + debug("NAND Reset operation timed
> >> >> > out on bank %d\n", + i);
> >> >> > + break;
> >> >> > + }
> >> >> > +
> >> >> > + /* Reset completed and did not time out,
> >> >> > all good. */ + if (reg &
> >> >> > INTR_STATUS__RST_COMP) + break;
> >> >> > +
> >> >> > + if (get_timer(start) > (CONFIG_SYS_HZ /
> >> >> > 1000)) { + debug("%s: Reset timed
> >> >> > out!\n", __func__); + break;
> >> >> > + }
> >> >> > + }
> >> >>
> >> >> I feel it is too much here
> >> >> about using get_timer() & CONFIG_SYS_HZ.
> >> >>
> >> >> How about iterating udelay(1) up to reasonable times?
> >> >
> >> > The get_timer() provides more precise delay , in this case, it's 1 sec
> >> > . Using just udelay() doesn't provide such precise control over the
> >> > delay.
> >>
> >> OK. Why do you want to wait precisely 1 sec.
> >> Rationale for "1 sec", please?
> >
> > There isn't any, 1 second sounds about right for a timeout in my mind.
> >
> >> > Moreover, I believe the get_timer() method is the one agreed upon for
> >> > implementing delays.
> >>
> >> You do not have to use CONFIG_SYS_HZ.
> >>
> >> As commented in lib/timer.c,
> >> get_timer() returns time in milliseconds.
> >
> > Ah, so you'd prefer just 1000 in there instead ?
>
> Yes.
Roger, makes sense, will do.
> >> >> See the wait_for_irq() function in this file.
> >> >
> >> > I'd like to convert this one to wait_for_bit() once that hits
> >> > mainline.
> >>
> >> No justice for the conversion.
> >
> > It'd better to have one timeout function than multiple implementation of
> > the same thing in multiple drivers, that's all.
> >
> >> This function just waits long enough,
> >> 1 sec, 2 sec, or whatever.
>
> I noticed one thing I had missed.
>
> While the CPU is stuck in udelay() function,
> it cannot check the register flag that might have been already set.
> This possibly wastes a small piece of time slice.
Well yeah, but that's such a small timeslice that it's not very important.
> So, I am OK with your way
> (and also with your next patch for the wait_for_bit() conversion, if you
> like).
>
> Go ahead!
THanks ;-)
More information about the U-Boot
mailing list