* [PATCH 2/3] mmc: protect against clockgate races @ 2010-11-16 16:56 Linus Walleij 2010-11-16 17:27 ` David Vrabel 2010-11-18 3:31 ` Chris Ball 0 siblings, 2 replies; 4+ messages in thread From: Linus Walleij @ 2010-11-16 16:56 UTC (permalink / raw) To: linux-mmc, Chris Ball; +Cc: Linus Walleij, Ohad Ben-Cohen This adds a mutex to protect from races between gate and ungate calls colliding. Cc: Ohad Ben-Cohen <ohad@wizery.com> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- This should fix the race observed by Ohad earlier. --- drivers/mmc/core/host.c | 11 ++++++++--- include/linux/mmc/host.h | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index f74b386..92e3370 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -68,9 +68,9 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host) unsigned long flags; if (!freq) { - pr_err("%s: frequency set to 0 in disable function, " - "this means the clock is already disabled.\n", - mmc_hostname(host)); + pr_debug("%s: frequency set to 0 in disable function, " + "this means the clock is already disabled.\n", + mmc_hostname(host)); return; } /* @@ -94,6 +94,7 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host) spin_unlock_irqrestore(&host->clk_lock, flags); return; } + mutex_lock(&host->clk_gate_mutex); spin_lock_irqsave(&host->clk_lock, flags); if (!host->clk_requests) { spin_unlock_irqrestore(&host->clk_lock, flags); @@ -103,6 +104,7 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host) pr_debug("%s: gated MCI clock\n", mmc_hostname(host)); } spin_unlock_irqrestore(&host->clk_lock, flags); + mutex_unlock(&host->clk_gate_mutex); } /* @@ -128,6 +130,7 @@ void mmc_host_clk_ungate(struct mmc_host *host) { unsigned long flags; + mutex_lock(&host->clk_gate_mutex); spin_lock_irqsave(&host->clk_lock, flags); if (host->clk_gated) { spin_unlock_irqrestore(&host->clk_lock, flags); @@ -137,6 +140,7 @@ void mmc_host_clk_ungate(struct mmc_host *host) } host->clk_requests++; spin_unlock_irqrestore(&host->clk_lock, flags); + mutex_unlock(&host->clk_gate_mutex); } /** @@ -214,6 +218,7 @@ static inline void mmc_host_clk_init(struct mmc_host *host) host->clk_gated = false; INIT_WORK(&host->clk_gate_work, mmc_host_clk_gate_work); spin_lock_init(&host->clk_lock); + mutex_init(&host->clk_gate_mutex); } /** diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 955bc10..53496bb 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -178,6 +178,7 @@ struct mmc_host { struct work_struct clk_gate_work; /* delayed clock gate */ unsigned int clk_old; /* old clock value cache */ spinlock_t clk_lock; /* lock for clk fields */ + struct mutex clk_gate_mutex; /* mutex for clock gating */ #endif /* host specific block data */ -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] mmc: protect against clockgate races 2010-11-16 16:56 [PATCH 2/3] mmc: protect against clockgate races Linus Walleij @ 2010-11-16 17:27 ` David Vrabel 2010-11-17 15:46 ` Linus Walleij 2010-11-18 3:31 ` Chris Ball 1 sibling, 1 reply; 4+ messages in thread From: David Vrabel @ 2010-11-16 17:27 UTC (permalink / raw) To: Linus Walleij; +Cc: linux-mmc, Chris Ball, Ohad Ben-Cohen Linus Walleij wrote: > This adds a mutex to protect from races between gate and ungate > calls colliding. Can you rework this to not require both host->clk_gate_mutex and host->clk_lock? Is host->clk_lock necessary any more? David > Cc: Ohad Ben-Cohen <ohad@wizery.com> > Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> > --- > This should fix the race observed by Ohad earlier. > --- > drivers/mmc/core/host.c | 11 ++++++++--- > include/linux/mmc/host.h | 1 + > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index f74b386..92e3370 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -68,9 +68,9 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host) > unsigned long flags; > > if (!freq) { > - pr_err("%s: frequency set to 0 in disable function, " > - "this means the clock is already disabled.\n", > - mmc_hostname(host)); > + pr_debug("%s: frequency set to 0 in disable function, " > + "this means the clock is already disabled.\n", > + mmc_hostname(host)); > return; > } > /* > @@ -94,6 +94,7 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host) > spin_unlock_irqrestore(&host->clk_lock, flags); > return; > } > + mutex_lock(&host->clk_gate_mutex); > spin_lock_irqsave(&host->clk_lock, flags); > if (!host->clk_requests) { > spin_unlock_irqrestore(&host->clk_lock, flags); > @@ -103,6 +104,7 @@ static void mmc_host_clk_gate_delayed(struct mmc_host *host) > pr_debug("%s: gated MCI clock\n", mmc_hostname(host)); > } > spin_unlock_irqrestore(&host->clk_lock, flags); > + mutex_unlock(&host->clk_gate_mutex); > } > > /* > @@ -128,6 +130,7 @@ void mmc_host_clk_ungate(struct mmc_host *host) > { > unsigned long flags; > > + mutex_lock(&host->clk_gate_mutex); > spin_lock_irqsave(&host->clk_lock, flags); > if (host->clk_gated) { > spin_unlock_irqrestore(&host->clk_lock, flags); > @@ -137,6 +140,7 @@ void mmc_host_clk_ungate(struct mmc_host *host) > } > host->clk_requests++; > spin_unlock_irqrestore(&host->clk_lock, flags); > + mutex_unlock(&host->clk_gate_mutex); > } > > /** > @@ -214,6 +218,7 @@ static inline void mmc_host_clk_init(struct mmc_host *host) > host->clk_gated = false; > INIT_WORK(&host->clk_gate_work, mmc_host_clk_gate_work); > spin_lock_init(&host->clk_lock); > + mutex_init(&host->clk_gate_mutex); > } > > /** > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 955bc10..53496bb 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -178,6 +178,7 @@ struct mmc_host { > struct work_struct clk_gate_work; /* delayed clock gate */ > unsigned int clk_old; /* old clock value cache */ > spinlock_t clk_lock; /* lock for clk fields */ > + struct mutex clk_gate_mutex; /* mutex for clock gating */ > #endif > > /* host specific block data */ -- David Vrabel, Senior Software Engineer, Drivers CSR, Churchill House, Cambridge Business Park, Tel: +44 (0)1223 692562 Cowley Road, Cambridge, CB4 0WZ http://www.csr.com/ Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] mmc: protect against clockgate races 2010-11-16 17:27 ` David Vrabel @ 2010-11-17 15:46 ` Linus Walleij 0 siblings, 0 replies; 4+ messages in thread From: Linus Walleij @ 2010-11-17 15:46 UTC (permalink / raw) To: David Vrabel; +Cc: linux-mmc, Chris Ball, Ohad Ben-Cohen 2010/11/16 David Vrabel <david.vrabel@csr.com>: > Linus Walleij wrote: >> This adds a mutex to protect from races between gate and ungate >> calls colliding. > > Can you rework this to not require both host->clk_gate_mutex and > host->clk_lock? Is host->clk_lock necessary any more? No I can't. I really wanted to, and the bug Ohad spotted was the result of trying to do exactly that. But it doesn't work. The reason is that mmc_host_clk_gate() is called from mmc_request_done(), which may be called from interrupt context, thus it cannot take a mutex. Trying to use only the spinlock gives the inverse problem: the gating code calls out to mmc_set_ios() or mmc_set_clock() which must be called in process context because they can be real slow, and surely shouldn't disable interrupts like taking a spinlock does. So what the patch does is handle slowpath and fastpath alike and for this we need a slowpath synchronization primitive (the mutex) and a fastpath synchronization primitive (the spinlock). Yours, Linus Walleij ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] mmc: protect against clockgate races 2010-11-16 16:56 [PATCH 2/3] mmc: protect against clockgate races Linus Walleij 2010-11-16 17:27 ` David Vrabel @ 2010-11-18 3:31 ` Chris Ball 1 sibling, 0 replies; 4+ messages in thread From: Chris Ball @ 2010-11-18 3:31 UTC (permalink / raw) To: Linus Walleij; +Cc: linux-mmc, Ohad Ben-Cohen Hi Linus, On Tue, Nov 16, 2010 at 05:56:52PM +0100, Linus Walleij wrote: > This adds a mutex to protect from races between gate and ungate > calls colliding. > > Cc: Ohad Ben-Cohen <ohad@wizery.com> > Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> Thanks, merged into mmc-next for .38. -- Chris Ball <cjb@laptop.org> <http://printf.net/> One Laptop Per Child ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-11-18 3:31 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-11-16 16:56 [PATCH 2/3] mmc: protect against clockgate races Linus Walleij 2010-11-16 17:27 ` David Vrabel 2010-11-17 15:46 ` Linus Walleij 2010-11-18 3:31 ` Chris Ball
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox