Skip to contentSkip to conversation

mmc: omap_hsmmc: Fix sleep too long in ISR context.

PATCH In review majianpeng 3 participants 4 messages Aug 7, 2013 – Aug 22, 2013

Subsystems (2)
MMC, SD and SDIO OMAP hs MMC support
lore
Share & tools
lore

majianpeng

author
#0 lore raw

PATCH v2 1/1 mmc: omap_hsmmc: Fix sleep too long in ISR context.

V2:
clean up code.
V1:

	www.mail-archive.com/linux-omap@vger.../msg93239.html‎

We found a problem when we removed a working sd card that the irqaction of omap_hsmmc can sleep to 3.6s. This cause our watchdog to work. In func omap_hsmmc_reset_controller_fsm, it should watch a 0->1 transition.But avoiding endless waiting, it used loops_per_jiffy as the timer. The code is:

Quoted 3 lines

while ((!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))

               && (i++ < limit))
                       cpu_relax();

But generanly loops_per_jiffy as a timer,it should like:

Quoted 2 lines

while(i++ < limit)
cpu_relax();

I found for the long time case, the while-opeation stoped because 'i == limit'. Because added some code, so the duration became too longer than MMC_TIMEOU_US(20ms).

The software can't monitor the transition of hardware for thi case.

Becasue those codes in ISR context, it can't use timer_before/after. I divived the time into 1ms and used udelay(1) to instead. It will cause do additional udelay(1).But from my test,it looks good.

Reported-by Yuzheng Ma <[email protected]>
Tested-by Yuzheng Ma <[email protected]>
Reviewed-by Hein Tibosch <[email protected]>
Signed-off-by Jianpeng Ma <[email protected]>
---
 drivers/mmc/host/omap_hsmmc.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)
drivers/mmc/host/omap_hsmmc.c +11 -14
index 1865321..bbda5ed 100644
@@ -973,9 +973,8 @@ static inline void omap_hsmmc_dbg_report_irq(struct omap_hsmmc_host *host,
 static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 						   unsigned long bit)
 {
-	unsigned long i = 0;
-	unsigned long limit = (loops_per_jiffy *
-				msecs_to_jiffies(MMC_TIMEOUT_MS));
+	/*change to 1ms,so we can use udelay(1)*/
+	unsigned long limit = MMC_TIMEOUT_MS * 1000;
 
 	OMAP_HSMMC_WRITE(host->base, SYSCTL,
 			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);

@@ -984,17 +983,15 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	 * OMAP4 ES2 and greater has an updated reset logic.
 	 * Monitor a 0->1 transition first
 	 */
-	if (mmc_slot(host).features & HSMMC_HAS_UPDATED_RESET) {
-		while ((!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
-					&& (i++ < limit))
-			cpu_relax();
-	}
-	i = 0;
-
-	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) &&
-		(i++ < limit))
-		cpu_relax();
-
+	if (mmc_slot(host).features & HSMMC_HAS_UPDATED_RESET)
+		while (!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
+			&& limit--)
+			udelay(1);
+
+	limit = MMC_TIMEOUT_MS * 1000;
+	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) && limit--)
+		udelay(1);
+
 	if (OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
 		dev_err(mmc_dev(host->mmc),
 			"Timeout waiting on controller reset in %s\n",
Signature · 1 line
1.8.1.2
#1 lore raw

Hi,

[ added some people from TI ]

Quoted 10 lines · majianpeng

V2:
clean up code.
V1:

	www.mail-archive.com/linux-omap@vger.../msg93239.html‎

We found a problem when we removed a working sd card that the irqaction of omap_hsmmc can sleep to 3.6s. This cause our watchdog to work. In func omap_hsmmc_reset_controller_fsm, it should watch a 0->1 transition.But avoiding endless waiting, it used loops_per_jiffy as the timer.

Tried on a OMAP4460:
This can easily be replicated: just withdraw an SD-card and the kernel will get blocked during more than 3 seconds.
Calling OMAP_HSMMC_READ() in this loop makes it last so long.

The function waits for a 0=>1, followed by a 1=>0 transition. The value of 1 always comes, but in most cases the code is just too slow to detect it. The first loop will only stop when (i == limit)

Quoted 31 lines

The code is:

Quoted 3 lines

while ((!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))

               && (i++ < limit))
                       cpu_relax();

But generanly loops_per_jiffy as a timer,it should like:

Quoted 2 lines

while(i++ < limit)
cpu_relax();

I found for the long time case, the while-opeation stoped because 'i == limit'. Because added some code, so the duration became too longer than MMC_TIMEOU_US(20ms).

The software can't monitor the transition of hardware for thi case.

Becasue those codes in ISR context, it can't use timer_before/after. I divived the time into 1ms and used udelay(1) to instead. It will cause do additional udelay(1).But from my test,it looks good.

Reported-by Yuzheng Ma <[email protected]>
Tested-by Yuzheng Ma <[email protected]>
Reviewed-by Hein Tibosch <[email protected]>
Signed-off-by Jianpeng Ma <[email protected]>
---
 drivers/mmc/host/omap_hsmmc.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)
drivers/mmc/host/omap_hsmmc.c +0 -0
index 1865321..bbda5ed 100644
@@ -973,9 +973,8 @@ static inline void omap_hsmmc_dbg_report_irq(struct omap_hsmmc_host *host,
 static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_dbg_report_irq(struct omap_hsmmc_host *host, · line 980

Quoted 10 lines
drivers/mmc/host/omap_hsmmc.c +2 -3
 						   unsigned long bit)
 {
-	unsigned long i = 0;
-	unsigned long limit = (loops_per_jiffy *
-				msecs_to_jiffies(MMC_TIMEOUT_MS));
+	/*change to 1ms,so we can use udelay(1)*/
+	unsigned long limit = MMC_TIMEOUT_MS * 1000;
 
 	OMAP_HSMMC_WRITE(host->base, SYSCTL,
 			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);

Checked here: the SRC-bit indeed becomes high.
After the test 'features & HSMMC_HAS_UPDATED_RESET', the bit has become low again already.
Changing to order of statements worked for me, but for Jianpeng Ma this didn't work (timings are 'on the edge').

This reset function is also called while mounting a card and then 'SRC' will be high long enough (more than 100 loops). It is after withdrawing the card that the reset is performed extremely fast.

Quoted 9 lines
drivers/mmc/host/omap_hsmmc.c +0 -5
@@ -984,17 +983,15 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	 * OMAP4 ES2 and greater has an updated reset logic.
 	 * Monitor a 0->1 transition first
 	 */
-	if (mmc_slot(host).features & HSMMC_HAS_UPDATED_RESET) {
-		while ((!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
-					&& (i++ < limit))
-			cpu_relax();
-	}

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 989

Quoted 10 lines
drivers/mmc/host/omap_hsmmc.c +4 -6
-	i = 0;
-
-	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) &&
-		(i++ < limit))
-		cpu_relax();
-
+	if (mmc_slot(host).features & HSMMC_HAS_UPDATED_RESET)
+		while (!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
+			&& limit--)
+			udelay(1);

In this way, the loop will only last about 'MMC_TIMEOUT_MS' ms and my WDT doesn't get triggered :-)

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 997

Quoted 7 lines
drivers/mmc/host/omap_hsmmc.c +4 -0
+	limit = MMC_TIMEOUT_MS * 1000;
+	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) && limit--)
+		udelay(1);
+
 	if (OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
 		dev_err(mmc_dev(host->mmc),
 			"Timeout waiting on controller reset in %s\n",

Anybody an opinion on this?

Thanks, Hein

#2 lore raw
Quoted 64 lines · Hein Tibosch

Hi,

[ added some people from TI ]

Quoted 10 lines · majianpeng

V2:
clean up code.
V1:

	www.mail-archive.com/linux-omap@vger.../msg93239.html‎

We found a problem when we removed a working sd card that the irqaction of omap_hsmmc can sleep to 3.6s. This cause our watchdog to work. In func omap_hsmmc_reset_controller_fsm, it should watch a 0->1 transition.But avoiding endless waiting, it used loops_per_jiffy as the timer.

Tried on a OMAP4460:
This can easily be replicated: just withdraw an SD-card and the kernel will get blocked during more than 3 seconds.
Calling OMAP_HSMMC_READ() in this loop makes it last so long.

The function waits for a 0=>1, followed by a 1=>0 transition. The value of 1 always comes, but in most cases the code is just too slow to detect it. The first loop will only stop when (i == limit)

Quoted 39 lines

The code is:

Quoted 3 lines

while ((!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))

                && (i++ < limit))
                        cpu_relax();

But generanly loops_per_jiffy as a timer,it should like:

Quoted 2 lines

while(i++ < limit)
cpu_relax();

I found for the long time case, the while-opeation stoped because 'i == limit'. Because added some code, so the duration became too longer than MMC_TIMEOU_US(20ms).

The software can't monitor the transition of hardware for thi case.

Becasue those codes in ISR context, it can't use timer_before/after. I divived the time into 1ms and used udelay(1) to instead. It will cause do additional udelay(1).But from my test,it looks good.

Reported-by Yuzheng Ma <[email protected]>
Tested-by Yuzheng Ma <[email protected]>
Reviewed-by Hein Tibosch <[email protected]>
Signed-off-by Jianpeng Ma <[email protected]>
---
  drivers/mmc/host/omap_hsmmc.c | 25 +++++++++++--------------
  1 file changed, 11 insertions(+), 14 deletions(-)
drivers/mmc/host/omap_hsmmc.c +2 -3
index 1865321..bbda5ed 100644
@@ -973,9 +973,8 @@ static inline void omap_hsmmc_dbg_report_irq(struct omap_hsmmc_host *host,
  static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
  						   unsigned long bit)
  {
-	unsigned long i = 0;
-	unsigned long limit = (loops_per_jiffy *
-				msecs_to_jiffies(MMC_TIMEOUT_MS));
+	/*change to 1ms,so we can use udelay(1)*/
+	unsigned long limit = MMC_TIMEOUT_MS * 1000;
Quoted 8 lines

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_dbg_report_irq(struct omap_hsmmc_host *host, · line 980

Quoted 2 lines
drivers/mmc/host/omap_hsmmc.c +0 -0
  	OMAP_HSMMC_WRITE(host->base, SYSCTL,
  			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);

Checked here: the SRC-bit indeed becomes high.
After the test 'features & HSMMC_HAS_UPDATED_RESET', the bit has become low again already.
Changing to order of statements worked for me, but for Jianpeng Ma this didn't work (timings are 'on the edge').

I think 1->0 transition is missed sometimes and waiting until timeout, Jianpeng Ma, which SoC are you testing ?

Quoted 38 lines

This reset function is also called while mounting a card and then 'SRC' will be high long enough (more than 100 loops). It is after withdrawing the card that the reset is performed extremely fast.

Quoted 9 lines
drivers/mmc/host/omap_hsmmc.c +0 -5
@@ -984,17 +983,15 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
  	 * OMAP4 ES2 and greater has an updated reset logic.
  	 * Monitor a 0->1 transition first
  	 */
-	if (mmc_slot(host).features & HSMMC_HAS_UPDATED_RESET) {
-		while ((!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
-					&& (i++ < limit))
-			cpu_relax();
-	}

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 989

Quoted 10 lines
drivers/mmc/host/omap_hsmmc.c +4 -6
-	i = 0;
-
-	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) &&
-		(i++ < limit))
-		cpu_relax();
-
+	if (mmc_slot(host).features & HSMMC_HAS_UPDATED_RESET)
+		while (!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
+			&& limit--)
+			udelay(1);

In this way, the loop will only last about 'MMC_TIMEOUT_MS' ms and my WDT doesn't get triggered :-)

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 997

Quoted 7 lines
drivers/mmc/host/omap_hsmmc.c +4 -0
+	limit = MMC_TIMEOUT_MS * 1000;
+	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) && limit--)
+		udelay(1);
+
  	if (OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
  		dev_err(mmc_dev(host->mmc),
  			"Timeout waiting on controller reset in %s\n",

Anybody an opinion on this?

Quoted 2 lines

Thanks, Hein

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#3 lore raw
Quoted 74 lines · Hein Tibosch
Quoted 64 lines

Hi,

[ added some people from TI ]

Quoted 10 lines · majianpeng

V2:
clean up code.
V1:

	www.mail-archive.com/linux-omap@vger.../msg93239.html‎

We found a problem when we removed a working sd card that the irqaction of omap_hsmmc can sleep to 3.6s. This cause our watchdog to work. In func omap_hsmmc_reset_controller_fsm, it should watch a 0->1 transition.But avoiding endless waiting, it used loops_per_jiffy as the timer.

Tried on a OMAP4460:
This can easily be replicated: just withdraw an SD-card and the kernel will get blocked during more than 3 seconds.
Calling OMAP_HSMMC_READ() in this loop makes it last so long.

The function waits for a 0=>1, followed by a 1=>0 transition. The value of 1 always comes, but in most cases the code is just too slow to detect it. The first loop will only stop when (i == limit)

Quoted 39 lines

The code is:

Quoted 3 lines

while ((!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))

                && (i++ < limit))
                        cpu_relax();

But generanly loops_per_jiffy as a timer,it should like:

Quoted 2 lines

while(i++ < limit)
cpu_relax();

I found for the long time case, the while-opeation stoped because 'i == limit'. Because added some code, so the duration became too longer than MMC_TIMEOU_US(20ms).

The software can't monitor the transition of hardware for thi case.

Becasue those codes in ISR context, it can't use timer_before/after. I divived the time into 1ms and used udelay(1) to instead. It will cause do additional udelay(1).But from my test,it looks good.

Reported-by Yuzheng Ma <[email protected]>
Tested-by Yuzheng Ma <[email protected]>
Reviewed-by Hein Tibosch <[email protected]>
Signed-off-by Jianpeng Ma <[email protected]>
---
  drivers/mmc/host/omap_hsmmc.c | 25 +++++++++++--------------
  1 file changed, 11 insertions(+), 14 deletions(-)
drivers/mmc/host/omap_hsmmc.c +2 -3
index 1865321..bbda5ed 100644
@@ -973,9 +973,8 @@ static inline void omap_hsmmc_dbg_report_irq(struct omap_hsmmc_host *host,
  static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
  						   unsigned long bit)
  {
-	unsigned long i = 0;
-	unsigned long limit = (loops_per_jiffy *
-				msecs_to_jiffies(MMC_TIMEOUT_MS));
+	/*change to 1ms,so we can use udelay(1)*/
+	unsigned long limit = MMC_TIMEOUT_MS * 1000;
Quoted 8 lines

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_dbg_report_irq(struct omap_hsmmc_host *host, · line 980

Quoted 2 lines
drivers/mmc/host/omap_hsmmc.c +0 -0
  	OMAP_HSMMC_WRITE(host->base, SYSCTL,
  			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);

Checked here: the SRC-bit indeed becomes high.
After the test 'features & HSMMC_HAS_UPDATED_RESET', the bit has become low again already.
Changing to order of statements worked for me, but for Jianpeng Ma this didn't work (timings are 'on the edge').

Quoted 3 lines

I think 1->0 transition is missed sometimes and waiting until timeout, Jianpeng Ma, which SoC are you testing ?

Hi,

	my sos is DM8107.

Thanks!
Jianpeng Ma!