From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Mon, 21 Dec 2015 15:47:28 +0100 Subject: [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop In-Reply-To: References: <1450580348-6243-1-git-send-email-marex@denx.de> <201512211145.50736.marex@denx.de> Message-ID: <201512211547.28205.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 : > > 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 : > >> > 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 : > >> >> > 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 > >> >> > Cc: Masahiro Yamada > >> >> > Cc: Scott Wood > >> >> > --- > >> >> > > >> >> > 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 ;-)