From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Mon, 21 Dec 2015 11:45:50 +0100 Subject: [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop In-Reply-To: References: <1450580348-6243-1-git-send-email-marex@denx.de> <201512211116.41880.marex@denx.de> Message-ID: <201512211145.50736.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: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 ? > >> 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.