Re: [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop

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@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@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com Cc: Scott Wood scottwood@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. Moreover, I believe the get_timer() method is the one agreed upon for implementing delays.

See the wait_for_irq() function in this file.

I'd like to convert this one to wait_for_bit() once that hits mainline.