* [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop
@ 2015-12-20 2:59 Marek Vasut
2015-12-21 8:40 ` Masahiro Yamada
0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2015-12-20 2:59 UTC (permalink / raw)
To: u-boot
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;
+ }
+ }
}
for (i = 0; i < denali->max_banks; i++)
--
2.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop
2015-12-20 2:59 [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop Marek Vasut
@ 2015-12-21 8:40 ` Masahiro Yamada
2015-12-21 10:16 ` Marek Vasut
0 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2015-12-21 8:40 UTC (permalink / raw)
To: u-boot
Hi Marek,
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?
See the wait_for_irq() function in this file.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop
2015-12-21 8:40 ` Masahiro Yamada
@ 2015-12-21 10:16 ` Marek Vasut
2015-12-21 10:32 ` Masahiro Yamada
0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2015-12-21 10:16 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop
2015-12-21 10:16 ` Marek Vasut
@ 2015-12-21 10:32 ` Masahiro Yamada
2015-12-21 10:45 ` Marek Vasut
0 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2015-12-21 10:32 UTC (permalink / raw)
To: u-boot
Hi Marek,
2015-12-21 19:16 GMT+09:00 Marek Vasut <marex@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@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.
OK. Why do you want to wait precisely 1 sec.
Rationale for "1 sec", please?
> 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.
>> 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.
This function just waits long enough,
1 sec, 2 sec, or whatever.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop
2015-12-21 10:32 ` Masahiro Yamada
@ 2015-12-21 10:45 ` Marek Vasut
2015-12-21 10:56 ` Masahiro Yamada
0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2015-12-21 10:45 UTC (permalink / raw)
To: u-boot
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@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@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.
>
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop
2015-12-21 10:45 ` Marek Vasut
@ 2015-12-21 10:56 ` Masahiro Yamada
2015-12-21 14:47 ` Marek Vasut
0 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2015-12-21 10:56 UTC (permalink / raw)
To: u-boot
Hi Marek,
2015-12-21 19:45 GMT+09:00 Marek Vasut <marex@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@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@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.
>>
>> 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.
>> >> 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.
So, I am OK with your way
(and also with your next patch for the wait_for_bit() conversion, if you like).
Go ahead!
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop
2015-12-21 10:56 ` Masahiro Yamada
@ 2015-12-21 14:47 ` Marek Vasut
0 siblings, 0 replies; 7+ messages in thread
From: Marek Vasut @ 2015-12-21 14:47 UTC (permalink / raw)
To: u-boot
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@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@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@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.
> >>
> >> 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 ;-)
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-12-21 14:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-20 2:59 [U-Boot] [PATCH] mtd: nand: denali: Nuke unbounded loop Marek Vasut
2015-12-21 8:40 ` Masahiro Yamada
2015-12-21 10:16 ` Marek Vasut
2015-12-21 10:32 ` Masahiro Yamada
2015-12-21 10:45 ` Marek Vasut
2015-12-21 10:56 ` Masahiro Yamada
2015-12-21 14:47 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.