Skip to contentSkip to conversation

OMAP HSMMC: fix a racing case between kmmcd and omap_hsmmc_suspend

PATCH In review Ethan Du 4 participants 5 messages Jun 21, 2010 – Aug 27, 2010

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

Ethan Du

author
#0 lore raw

PATCH 1/1 OMAP HSMMC: fix a racing case between kmmcd and omap_hsmmc_suspend

If suspend called when kmmcd is doing host->ops->disable,
as kmmcd already increased host->en_dis_recurs to 1, the mmc_host_enable in suspend function will return directly without increase the nesting_cnt, which will cause the followed register access carried out to the disabled host.

mmc_suspend_host will enable host itself. No need to enable host before it. Also works on kmmcd will get flushed in mmc_suspend_host, enable host after it will be safe. So make the mmc_host_enable after it.

Signed-off-by Ethan <[email protected]>
---
 drivers/mmc/host/omap_hsmmc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/host/omap_hsmmc.c +1 -1
index b032828..fc74263 100644
@@ -2291,8 +2291,8 @@ static int omap_hsmmc_suspend(struct device *dev)
                        }
                }
                cancel_work_sync(&host->mmc_carddetect_work);
-               mmc_host_enable(host->mmc);
                ret = mmc_suspend_host(host->mmc);
+               mmc_host_enable(host->mmc);
                if (ret == 0) {
                        omap_hsmmc_disable_irq(host);
                        OMAP_HSMMC_WRITE(host->base, HCTL,

--
1.5.6.5

Acked-by on v1 1/1
#1 lore raw
Quoted 10 lines · Ethan Du

If suspend called when kmmcd is doing host->ops->disable,
as kmmcd already increased host->en_dis_recurs to 1, the mmc_host_enable in suspend function will return directly without increase the nesting_cnt, which will cause the followed register access carried out to the disabled host.

mmc_suspend_host will enable host itself. No need to enable host before it. Also works on kmmcd will get flushed in mmc_suspend_host, enable host after it will be safe. So make the mmc_host_enable after it.

Signed-off-by Ethan <[email protected]>

Looks good to me

Acked-by Adrian Hunter <[email protected]>
Quoted 25 lines
---
 drivers/mmc/host/omap_hsmmc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/host/omap_hsmmc.c +1 -2
index b032828..fc74263 100644
@@ -2291,8 +2291,8 @@ static int omap_hsmmc_suspend(struct device *dev)
                        }
                }
                cancel_work_sync(&host->mmc_carddetect_work);
-               mmc_host_enable(host->mmc);
                ret = mmc_suspend_host(host->mmc);
+               mmc_host_enable(host->mmc);
                if (ret == 0) {
                        omap_hsmmc_disable_irq(host);
                        OMAP_HSMMC_WRITE(host->base, HCTL,
--

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

#2 lore raw
Quoted 10 lines · Hunter Adrian (Nokia-D/Helsinki)
Quoted 9 lines · Ethan Du

If suspend called when kmmcd is doing host->ops->disable,
as kmmcd already increased host->en_dis_recurs to 1, the mmc_host_enable in suspend function will return directly without increase the nesting_cnt, which will cause the followed register access carried out to the disabled host.

mmc_suspend_host will enable host itself. No need to enable host before it. Also works on kmmcd will get flushed in mmc_suspend_host, enable host after it will be safe. So make the mmc_host_enable after it.

Quoted 6 lines
Quoted 1 line
Signed-off-by Ethan <[email protected]>

Looks good to me

Acked-by Adrian Hunter <[email protected]>

Ping?

Ethan asked me what is happening to this.

Is is OK Madhu?

Andrew can you take it?

Quoted 31 lines
Quoted 19 lines
---
 drivers/mmc/host/omap_hsmmc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/host/omap_hsmmc.c +1 -2
index b032828..fc74263 100644
@@ -2291,8 +2291,8 @@ static int omap_hsmmc_suspend(struct device *dev)
                        }
                }
                cancel_work_sync(&host->mmc_carddetect_work);
-               mmc_host_enable(host->mmc);
                ret = mmc_suspend_host(host->mmc);
+               mmc_host_enable(host->mmc);
                if (ret == 0) {
                        omap_hsmmc_disable_irq(host);
                        OMAP_HSMMC_WRITE(host->base, HCTL,
--
Quoted 6 lines

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

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

Acked-by
#3 lore raw
Quoted 33 lines

-----Original Message-----
From: Adrian Hunter [mailto:[email protected]]
Sent: Tuesday, July 06, 2010 1:11 PM
To: Madhusudhan Chikkature
Cc: Hunter Adrian (Nokia-D/Helsinki); Ethan Du; linux-mmc; Andrew Morton
Subject: Re: [PATCH] OMAP HSMMC: fix a racing case between kmmcd and omap_hsmmc_suspend

Quoted 3 lines · Hunter Adrian (Nokia-D/Helsinki)
Quoted 2 lines · Ethan Du

If suspend called when kmmcd is doing host->ops->disable, as kmmcd already increased host->en_dis_recurs to 1, the

mmc_host_enable

Quoted 1 line
Quoted 1 line

in suspend function will return directly without increase the

nesting_cnt,

Quoted 1 line
Quoted 1 line

which will cause the followed register access carried out to the

disabled host.

Quoted 2 lines
Quoted 2 lines

mmc_suspend_host will enable host itself. No need to enable host before

it.

Quoted 1 line
Quoted 1 line

Also works on kmmcd will get flushed in mmc_suspend_host, enable host

after

Quoted 8 lines
Quoted 3 lines

it will be safe. So make the mmc_host_enable after it.

Signed-off-by Ethan <[email protected]>

Looks good to me

Acked-by Adrian Hunter <[email protected]>
Quoted 8 lines

Ping?

Ethan asked me what is happening to this.

Is is OK Madhu?

Andrew can you take it?

Sorry, I missed to look at this patch. It looks good.

Acked-by Madhusudhan Chikkature <[email protected]>
Quoted 32 lines
Quoted 5 lines

Review

Quoted 5 lines
---
 drivers/mmc/host/omap_hsmmc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
(quoted diff) +0 -0

b/drivers/mmc/host/omap_hsmmc.c

Quoted 26 lines
Quoted 14 lines

index b032828..fc74263 100644

drivers/mmc/host/omap_hsmmc.c +1 -2
@@ -2291,8 +2291,8 @@ static int omap_hsmmc_suspend(struct device *dev)
                        }
                }
                cancel_work_sync(&host->mmc_carddetect_work);
-               mmc_host_enable(host->mmc);
                ret = mmc_suspend_host(host->mmc);
+               mmc_host_enable(host->mmc);
                if (ret == 0) {
                        omap_hsmmc_disable_irq(host);
                        OMAP_HSMMC_WRITE(host->base, HCTL,
--
Quoted 6 lines

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

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

#4 lore raw

PATCH RESEND 1/1 OMAP HSMMC: fix a racing case between kmmcd and omap_hsmmc_suspend

In reply to Ethan Du

Hi,

This doesn't seem to have been merged -- Andrew, could you take it?

From: Ethan Du <[email protected]>
Date: Mon, 21 Jun 2010 19:20:41 +0800

If suspend called when kmmcd is doing host->ops->disable,
as kmmcd already increased host->en_dis_recurs to 1, the mmc_host_enable in suspend function will return directly without increase the nesting_cnt, which will cause the followed register access carried out to the disabled host.

mmc_suspend_host will enable host itself. No need to enable host before it. Also works on kmmcd will get flushed in mmc_suspend_host, enable host after it will be safe. So make the mmc_host_enable after it.

Signed-off-by Ethan <[email protected]>
Acked-by Adrian Hunter <[email protected]>
Acked-by Madhusudhan Chikkature <[email protected]>
[cjb: rebase against current Linus]
---
 drivers/mmc/host/omap_hsmmc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/mmc/host/omap_hsmmc.c +1 -1
index ba623d6..1f7d01a 100644
@@ -2323,8 +2323,8 @@ static int omap_hsmmc_suspend(struct device *dev)
 			}
 		}
 		cancel_work_sync(&host->mmc_carddetect_work);
-		mmc_host_enable(host->mmc);
 		ret = mmc_suspend_host(host->mmc);
+		mmc_host_enable(host->mmc);
 		if (ret == 0) {
 			omap_hsmmc_disable_irq(host);
 			OMAP_HSMMC_WRITE(host->base, HCTL,

--
1.7.0.1

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child