| Message ID | 1373987208-24120-1-git-send-email-b.brezillon@overkiz.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Boris BREZILLON, On Tue, 16 Jul 2013 17:06:48 +0200, Boris BREZILLON wrote: > buf = kmalloc(ATMCI_REGS_SIZE, GFP_KERNEL); > if (!buf) > @@ -389,9 +391,13 @@ static int atmci_regs_show(struct seq_file *s, void *v) > * consistent. > */ > spin_lock_bh(&host->lock); > - clk_enable(host->mck); > + ret = clk_prepare_enable(host->mck); I am not very familiar with the spin_lock_bh() variant, but are you sure we are allowed to sleep within a spin_lock_bh()-protected critical section? Remember that clk_prepare_enable() calls both ->prepare() and ->enable() for the clock, and ->prepare() is allowed to sleep, while ->enable() is guaranteed not to sleep. Therefore, clk_prepare() is usually called at probe time, while clk_enable() is called whenever enabling/disabling the clock is really needed. So not all clk_enable() can transparently be converted into a clk_prepare_enable(). Best regards, Thomas
Hello Thomas, On 16/07/2013 17:13, Thomas Petazzoni wrote: > Dear Boris BREZILLON, > > On Tue, 16 Jul 2013 17:06:48 +0200, Boris BREZILLON wrote: > >> buf = kmalloc(ATMCI_REGS_SIZE, GFP_KERNEL); >> if (!buf) >> @@ -389,9 +391,13 @@ static int atmci_regs_show(struct seq_file *s, void *v) >> * consistent. >> */ >> spin_lock_bh(&host->lock); >> - clk_enable(host->mck); >> + ret = clk_prepare_enable(host->mck); > I am not very familiar with the spin_lock_bh() variant, but are you > sure we are allowed to sleep within a spin_lock_bh()-protected critical > section? > > Remember that clk_prepare_enable() calls both ->prepare() and > ->enable() for the clock, and ->prepare() is allowed to sleep, while > ->enable() is guaranteed not to sleep. > > Therefore, clk_prepare() is usually called at probe time, while > clk_enable() is called whenever enabling/disabling the clock is really > needed. So not all clk_enable() can transparently be converted into a > clk_prepare_enable(). You're absolutely right. We should not call clk_prepare/unprepare inside a critical section, as the prepare/unprepare callback may sleep. In this particular case it won't hurt as the mci clk is a peripheral clk which does not implement the prepare callback (and as a result won't sleep). Anyway, I will fix it. What is the best approach to do so ? 1) call clk_prepare/unprepare in the probe/remove functions and call clk_enable/disable in resume/suspend functions 2) get clk_prepare_enable/disable_unprepare outside of the critical sections (I don't think there is any need for the mci host lock to be held when enabling the clk, clk framework already handle concurrent accesses to clks) I will check other patches of this series to see if they introduce similar issues. > Best regards, > > Thomas
On Tue, Jul 16, 2013 at 05:06:48PM +0200, Boris BREZILLON wrote: > @@ -389,9 +391,13 @@ static int atmci_regs_show(struct seq_file *s, void *v) > * consistent. > */ > spin_lock_bh(&host->lock); > - clk_enable(host->mck); > + ret = clk_prepare_enable(host->mck); > + if (ret) { > + spin_unlock_bh(&host->lock); > + goto out; > + } NAK. This is buggy. clk_prepare() can sleep. Calling clk_prepare() even via clk_prepare_enable() is a blatent bug. > memcpy_fromio(buf, host->regs, ATMCI_REGS_SIZE); > - clk_disable(host->mck); > + clk_disable_unprepare(host->mck); > spin_unlock_bh(&host->lock); Now, given that the CLK API counts enables/disables, having the spin lock around the clk API calls is utterly pointless. This should be: ret = clk_prepare_enable(host->mck); if (ret) goto out; spin_lock_bh(&host->lock); memcpy_fromio(buf, host->regs, ATMCI_REGS_SIZE); spin_unlock_bh(&host->lock); clk_disable_unprepare(host->mclk); or, if you really need to have the clock enabled/disabled within the spinlock (very very very unlikely): ret = clk_prepare(host->mck); if (ret) goto out; spin_lock_bh(&host->lock); ret = clk_enable(host->mck); if (ret == 0) { memcpy_fromio(buf, host->regs, ATMCI_REGS_SIZE); clk_disable(host->mck); } spin_unlock_bh(&host->lock); clk_unprepare(host->mclk); if (ret) goto out; > @@ -1279,7 +1286,7 @@ static void atmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > > spin_lock_bh(&host->lock); > if (!host->mode_reg) { > - clk_enable(host->mck); > + clk_prepare_enable(host->mck); Again, buggy - calling clk_prepare beneath a spinlock is illegal.
diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c index bdb84da..4636af6 100644 --- a/drivers/mmc/host/atmel-mci.c +++ b/drivers/mmc/host/atmel-mci.c @@ -378,6 +378,8 @@ static int atmci_regs_show(struct seq_file *s, void *v) { struct atmel_mci *host = s->private; u32 *buf; + int ret = 0; + buf = kmalloc(ATMCI_REGS_SIZE, GFP_KERNEL); if (!buf) @@ -389,9 +391,13 @@ static int atmci_regs_show(struct seq_file *s, void *v) * consistent. */ spin_lock_bh(&host->lock); - clk_enable(host->mck); + ret = clk_prepare_enable(host->mck); + if (ret) { + spin_unlock_bh(&host->lock); + goto out; + } memcpy_fromio(buf, host->regs, ATMCI_REGS_SIZE); - clk_disable(host->mck); + clk_disable_unprepare(host->mck); spin_unlock_bh(&host->lock); seq_printf(s, "MR:\t0x%08x%s%s ", @@ -442,9 +448,10 @@ static int atmci_regs_show(struct seq_file *s, void *v) val & ATMCI_CFG_LSYNC ? " LSYNC" : ""); } +out: kfree(buf); - return 0; + return ret; } static int atmci_regs_open(struct inode *inode, struct file *file) @@ -1279,7 +1286,7 @@ static void atmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) spin_lock_bh(&host->lock); if (!host->mode_reg) { - clk_enable(host->mck); + clk_prepare_enable(host->mck); atmci_writel(host, ATMCI_CR, ATMCI_CR_SWRST); atmci_writel(host, ATMCI_CR, ATMCI_CR_MCIEN); if (host->caps.has_cfg_reg) @@ -1359,7 +1366,7 @@ static void atmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) atmci_writel(host, ATMCI_CR, ATMCI_CR_MCIDIS); if (host->mode_reg) { atmci_readl(host, ATMCI_MR); - clk_disable(host->mck); + clk_disable_unprepare(host->mck); } host->mode_reg = 0; } @@ -2376,10 +2383,12 @@ static int __init atmci_probe(struct platform_device *pdev) if (!host->regs) goto err_ioremap; - clk_enable(host->mck); + ret = clk_prepare_enable(host->mck); + if (ret) + goto err_request_irq; atmci_writel(host, ATMCI_CR, ATMCI_CR_SWRST); host->bus_hz = clk_get_rate(host->mck); - clk_disable(host->mck); + clk_disable_unprepare(host->mck); host->mapbase = regs->start; @@ -2482,11 +2491,11 @@ static int __exit atmci_remove(struct platform_device *pdev) atmci_cleanup_slot(host->slot[i], i); } - clk_enable(host->mck); + clk_prepare_enable(host->mck); atmci_writel(host, ATMCI_IDR, ~0UL); atmci_writel(host, ATMCI_CR, ATMCI_CR_MCIDIS); atmci_readl(host, ATMCI_SR); - clk_disable(host->mck); + clk_disable_unprepare(host->mck); if (host->dma.chan) dma_release_channel(host->dma.chan);