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

PATCH Superseded majianpeng 2 participants 3 messages Aug 1, 2013

Subsystems (2)

MMC, SD and SDIO OMAP hs MMC support

lore

Share & tools

lore

majianpeng

author

2013-08-01 02:18 UTC #0 lore raw

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

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.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 the loops_per_jiffy is:

Quoted 2 lines

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

It add some codes so the time became long.
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]>

Signed-off-by Jianpeng Ma <[email protected]>

---
 drivers/mmc/host/omap_hsmmc.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

drivers/mmc/host/omap_hsmmc.c +13 -7

index 1865321..96daca1 100644
@@ -977,6 +977,8 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	unsigned long limit = (loops_per_jiffy *
 				msecs_to_jiffies(MMC_TIMEOUT_MS));
 
+	/*Divided time into us for unit 1,we can use udelay(1)*/
+	i = limit / (MMC_TIMEOUT_MS * 1000);
 	OMAP_HSMMC_WRITE(host->base, SYSCTL,
 			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);

@@ -985,15 +987,19 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	 * 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();
+		while (i--) {
+			if ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
+				break;
+			udelay(1);
+		}
 	}
-	i = 0;
 
-	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) &&
-		(i++ < limit))
-		cpu_relax();
+	i = limit / (MMC_TIMEOUT_MS * 1000);
+	while (i--) {
+		if (!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
+			break;
+		udealy(1);
+	}
 
 	if (OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
 		dev_err(mmc_dev(host->mmc),

Signature · 5 lines

1.8.1.2


Thanks!
Jianpeng Ma

Hein Tibosch

2013-08-01 08:22 UTC #1 lore raw

Hi Jianpeng Ma,

Quoted 28 lines · majianpeng

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.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 the loops_per_jiffy is:
Quoted 2 lines
while(i++ < limit)
	cpu_relax();
It add some codes so the time became long.
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]>
Signed-off-by Jianpeng Ma <[email protected]>
---
 drivers/mmc/host/omap_hsmmc.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
drivers/mmc/host/omap_hsmmc.c +0 -0
index 1865321..96daca1 100644

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

Quoted 5 lines

drivers/mmc/host/omap_hsmmc.c +2 -0

@@ -977,6 +977,8 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	unsigned long limit = (loops_per_jiffy *
 				msecs_to_jiffies(MMC_TIMEOUT_MS));
 
+	/*Divided time into us for unit 1,we can use udelay(1)*/
+	i = limit / (MMC_TIMEOUT_MS * 1000);

'limit' is a number of loops, which you now divide by 20,000?

To get uS, you could just change:

- unsigned long limit = (loops_per_jiffy *
- msecs_to_jiffies(MMC_TIMEOUT_MS));
+ unsigned long limit = 1000 * MMC_TIMEOUT_MS;

and make this amount of loops using udelay().

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

Quoted 10 lines

drivers/mmc/host/omap_hsmmc.c +0 -3

 	OMAP_HSMMC_WRITE(host->base, SYSCTL,
 			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);

@@ -985,15 +987,19 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	 * 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 still don't see why any of these loops could last 3.6 seconds? Yes the __raw_readl() will add some time, but so much?
I'd like to see which value you get for 'limit' on your machine Would PM play a role? Or cpu-freq, and 'loops_per_jiffy' isn't updated on time?

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

Quoted 4 lines

drivers/mmc/host/omap_hsmmc.c +4 -0

+		while (i--) {
+			if ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
+				break;
+			udelay(1);

In earlier threads, the use of udelay was disliked because it's a waste of cpu cycles. The desired bit in SYSCTL will change, while udelay() is still making many useless loops.

Quoted 6 lines

drivers/mmc/host/omap_hsmmc.c +1 -3

+		}
 	}
-	i = 0;
 
-	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) &&
-		(i++ < limit))

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

Quoted 10 lines

drivers/mmc/host/omap_hsmmc.c +6 -1

-		cpu_relax();
+	i = limit / (MMC_TIMEOUT_MS * 1000);
+	while (i--) {
+		if (!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
+			break;
+		udealy(1);
+	}
 
 	if (OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
 		dev_err(mmc_dev(host->mmc),

Hein

majianpeng

author

2013-08-01 08:40 UTC #2 lore raw

Quoted 42 lines

Hi Jianpeng Ma,
Quoted 28 lines · majianpeng
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.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 the loops_per_jiffy is:
Quoted 2 lines
while(i++ < limit)
	cpu_relax();
It add some codes so the time became long.
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]>
Signed-off-by Jianpeng Ma <[email protected]>
---
 drivers/mmc/host/omap_hsmmc.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
drivers/mmc/host/omap_hsmmc.c +0 -0
index 1865321..96daca1 100644
drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 981
Quoted 5 lines
drivers/mmc/host/omap_hsmmc.c +2 -0
@@ -977,6 +977,8 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	unsigned long limit = (loops_per_jiffy *
 				msecs_to_jiffies(MMC_TIMEOUT_MS));
 
+	/*Divided time into us for unit 1,we can use udelay(1)*/
+	i = limit / (MMC_TIMEOUT_MS * 1000);
'limit' is a number of loops, which you now divide by 20,000?
To get uS, you could just change:

Review

Quoted 3 lines

(quoted diff) +1 -2

-	unsigned long limit = (loops_per_jiffy *
-				msecs_to_jiffies(MMC_TIMEOUT_MS));
+	unsigned long limit = 1000 * MMC_TIMEOUT_MS;

Yes, you are right.

Quoted 5 lines

and make this amount of loops using udelay().
Quoted 3 lines
drivers/mmc/host/omap_hsmmc.c +0 -0
 	OMAP_HSMMC_WRITE(host->base, SYSCTL,
 			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);
 

Quoted 13 lines

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 990
Quoted 7 lines
drivers/mmc/host/omap_hsmmc.c +0 -3
@@ -985,15 +987,19 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	 * 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 still don't see why any of these loops could last 3.6 seconds? Yes the __raw_readl() will add some time, but so much?
I'd like to see which value you get for 'limit' on your machine Would PM play a role? Or cpu-freq, and 'loops_per_jiffy' isn't updated on time?

From my test, i found it don't monitor a 0->1 transtion.That is the last result is 'i = limit'. The later while opertion stop also because 'i = limit'.
The basic reason is the hardware.
It write bit and monitor 1-->0---1>---0.But we start monitor we only got 0 and the value don't change. Maybe the transition is 1-->0.So the monitor can't work but still waste cpu.

Quoted 9 lines

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 993
Quoted 5 lines
drivers/mmc/host/omap_hsmmc.c +4 -0
+		while (i--) {
+			if ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
+				break;
+			udelay(1);
In earlier threads, the use of udelay was disliked because it's a waste of cpu cycles. The desired bit in SYSCTL will change, while udelay() is still making many useless loops.

Yes, but it at most wast 1us.

Jianpeng Ma

Quoted 15 lines

drivers/mmc/host/omap_hsmmc.c +7 -4

+		}
 	}
-	i = 0;
 
-	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) &&
-		(i++ < limit))
-		cpu_relax();
+	i = limit / (MMC_TIMEOUT_MS * 1000);
+	while (i--) {
+		if (!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
+			break;
+		udealy(1);
+	}

Quoted 3 lines

drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 1005
Quoted 2 lines
drivers/mmc/host/omap_hsmmc.c +0 -0
 	if (OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
 		dev_err(mmc_dev(host->mmc),
Hein

Thanks!
Jianpeng Ma

Quoted 90 lines

Hi Jianpeng Ma,
Quoted 28 lines · majianpeng
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.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 the loops_per_jiffy is:
Quoted 2 lines
while(i++ < limit)
	cpu_relax();
It add some codes so the time became long.
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]>
Signed-off-by Jianpeng Ma <[email protected]>
---
 drivers/mmc/host/omap_hsmmc.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
drivers/mmc/host/omap_hsmmc.c +0 -0
index 1865321..96daca1 100644
drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 981
Quoted 5 lines
drivers/mmc/host/omap_hsmmc.c +2 -0
@@ -977,6 +977,8 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	unsigned long limit = (loops_per_jiffy *
 				msecs_to_jiffies(MMC_TIMEOUT_MS));
 
+	/*Divided time into us for unit 1,we can use udelay(1)*/
+	i = limit / (MMC_TIMEOUT_MS * 1000);
'limit' is a number of loops, which you now divide by 20,000?
To get uS, you could just change:
(quoted diff) +1 -2
-	unsigned long limit = (loops_per_jiffy *
-				msecs_to_jiffies(MMC_TIMEOUT_MS));
+	unsigned long limit = 1000 * MMC_TIMEOUT_MS;
and make this amount of loops using udelay().
drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 990
Quoted 10 lines
drivers/mmc/host/omap_hsmmc.c +0 -3
 	OMAP_HSMMC_WRITE(host->base, SYSCTL,
 			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);
 
@@ -985,15 +987,19 @@ static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,
 	 * 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 still don't see why any of these loops could last 3.6 seconds? Yes the __raw_readl() will add some time, but so much?
I'd like to see which value you get for 'limit' on your machine Would PM play a role? Or cpu-freq, and 'loops_per_jiffy' isn't updated on time?
drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 993
Quoted 4 lines
drivers/mmc/host/omap_hsmmc.c +4 -0
+		while (i--) {
+			if ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
+				break;
+			udelay(1);
In earlier threads, the use of udelay was disliked because it's a waste of cpu cycles. The desired bit in SYSCTL will change, while udelay() is still making many useless loops.
Quoted 6 lines
drivers/mmc/host/omap_hsmmc.c +1 -3
+		}
 	}
-	i = 0;
 
-	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) &&
-		(i++ < limit))
drivers/mmc/host/omap_hsmmc.c static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host, · line 1005
Quoted 10 lines
drivers/mmc/host/omap_hsmmc.c +6 -1
-		cpu_relax();
+	i = limit / (MMC_TIMEOUT_MS * 1000);
+	while (i--) {
+		if (!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
+			break;
+		udealy(1);
+	}
 
 	if (OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
 		dev_err(mmc_dev(host->mmc),
Hein