| Message ID | 1381876762-10892-3-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Doug. Nice catch. On 15/10/13 23:39, Doug Anderson wrote: > We're running into cases where our enabling of the SDIO interrupt in > dw_mmc doesn't actually take effect. Specifically, adding patch like > this: > > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1076,6 +1076,9 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) > > mci_writel(host, INTMASK, > (int_mask | SDMMC_INT_SDIO(slot->id))); > + int_mask = mci_readl(host, INTMASK); > + if (!(int_mask & SDMMC_INT_SDIO(slot->id))) > + dev_err(&mmc->class_dev, "failed to enable sdio irq\n"); > } else { > > ...actually triggers the error message. That's because the > dw_mci_enable_sdio_irq() unsafely does a read-modify-write of the > INTMASK register. > > We can't just use the standard host->lock since that lock is not irq > safe and mmc_signal_sdio_irq() (called from interrupt context) calls > dw_mci_enable_sdio_irq(). Add a new irq-safe lock to protect INTMASK. > > An alternate solution to this is to punt mmc_signal_sdio_irq() to the > tasklet and then protect INTMASK modifications by the standard host > lock. This seemed like a bit more of a high-latency change. A probably lighter-weight alternative to that alternative is to just make the existing lock irq safe. Has this been considered? I'm not entirely convinced it's worth having a separate lock rather than changing the existing one, but the patch still appears to be correct, so: Reviewed-by: James Hogan <james.hogan@imgtec.com> Cheers James -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
James, On Wed, Oct 16, 2013 at 2:49 AM, James Hogan <james.hogan@imgtec.com> wrote: >> We can't just use the standard host->lock since that lock is not irq >> safe and mmc_signal_sdio_irq() (called from interrupt context) calls >> dw_mci_enable_sdio_irq(). Add a new irq-safe lock to protect INTMASK. >> >> An alternate solution to this is to punt mmc_signal_sdio_irq() to the >> tasklet and then protect INTMASK modifications by the standard host >> lock. This seemed like a bit more of a high-latency change. > > A probably lighter-weight alternative to that alternative is to just > make the existing lock irq safe. Has this been considered? > > I'm not entirely convinced it's worth having a separate lock rather than > changing the existing one, but the patch still appears to be correct, so: > Reviewed-by: James Hogan <james.hogan@imgtec.com> I did look at that alternate solution and rejected it, but am happy to send that up if people think it's better. Here's why I rejected it: 1. It looks like someone has gone through quite a bit of work to _not_ grab the existing lock in the IRQ handler. The IRQ handler always pushes all real work over to the tasklet. I can only assume that they did this for some good reason and I'd hate to switch it without knowing for sure why. 2. We might have performance problems if we blindly changed the existing code to an IRQ-safe spinlock. We hold the existing "host->lock" spinlock for the entire duration of dw_mci_tasklet_func(). That's a pretty intense chunk of code, full of nested loops (some with 500ms timeouts!), mdelay(20) calls to handle some errors, etc. I say "might" because I think that the expected case is that code runs pretty quickly. I ran some brief tests on one system and saw a worst case time of 170us and an common case time of ~10us. Are we OK with having interrupts disabled for that long? Are we OK with having the dw_mci interrupt handler potentially spin on a spinlock for that long? I don't think it would be terrible to look at reworking the way that dw_mmc handles interrupts, but that seems pretty major. BTW: is the cost of an extra lock actually that high? ...or are you talking the cost in terms of code complexity? -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Doug, On 16 October 2013 17:43, Doug Anderson <dianders@chromium.org> wrote: > On Wed, Oct 16, 2013 at 2:49 AM, James Hogan <james.hogan@imgtec.com> wrote: >>> We can't just use the standard host->lock since that lock is not irq >>> safe and mmc_signal_sdio_irq() (called from interrupt context) calls >>> dw_mci_enable_sdio_irq(). Add a new irq-safe lock to protect INTMASK. >>> >>> An alternate solution to this is to punt mmc_signal_sdio_irq() to the >>> tasklet and then protect INTMASK modifications by the standard host >>> lock. This seemed like a bit more of a high-latency change. >> >> A probably lighter-weight alternative to that alternative is to just >> make the existing lock irq safe. Has this been considered? >> >> I'm not entirely convinced it's worth having a separate lock rather than >> changing the existing one, but the patch still appears to be correct, so: >> Reviewed-by: James Hogan <james.hogan@imgtec.com> > > I did look at that alternate solution and rejected it, but am happy to > send that up if people think it's better. Here's why I rejected it: > > 1. It looks like someone has gone through quite a bit of work to _not_ > grab the existing lock in the IRQ handler. The IRQ handler always > pushes all real work over to the tasklet. I can only assume that they > did this for some good reason and I'd hate to switch it without > knowing for sure why. > > 2. We might have performance problems if we blindly changed the > existing code to an IRQ-safe spinlock. We hold the existing > "host->lock" spinlock for the entire duration of > dw_mci_tasklet_func(). That's a pretty intense chunk of code, full of > nested loops (some with 500ms timeouts!), mdelay(20) calls to handle > some errors, etc. I say "might" because I think that the expected > case is that code runs pretty quickly. I ran some brief tests on one > system and saw a worst case time of 170us and an common case time of > ~10us. Are we OK with having interrupts disabled for that long? Are > we OK with having the dw_mci interrupt handler potentially spin on a > spinlock for that long? > Fair enough, I'm convinced now. That code did look pretty heavy when I looked at it too. > > I don't think it would be terrible to look at reworking the way that > dw_mmc handles interrupts, but that seems pretty major. Yeh, I suppose at least the potentially large delays are all for exceptional cases, so it's not a critical problem. > BTW: is the cost of an extra lock actually that high? I don't know tbh. In this case the spinlock usage doesn't appear to actually overlap. > ...or are you talking the cost in terms of code complexity? In this case it'd only be a space and code complexity thing I think. I suppose in some cases the benefit of finer-grained locking is probably pretty marginal, but there's a good case for it here. It might be worth renaming the lock to irq_lock or something like that so it's clear it doesn't have to protect only for INTMASK in the future - up to you. Cheers James -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/17/2013 05:23 AM, James Hogan wrote: > Hi Doug, > > On 16 October 2013 17:43, Doug Anderson <dianders@chromium.org> wrote: >> On Wed, Oct 16, 2013 at 2:49 AM, James Hogan <james.hogan@imgtec.com> wrote: >>>> We can't just use the standard host->lock since that lock is not irq >>>> safe and mmc_signal_sdio_irq() (called from interrupt context) calls >>>> dw_mci_enable_sdio_irq(). Add a new irq-safe lock to protect INTMASK. >>>> >>>> An alternate solution to this is to punt mmc_signal_sdio_irq() to the >>>> tasklet and then protect INTMASK modifications by the standard host >>>> lock. This seemed like a bit more of a high-latency change. >>> >>> A probably lighter-weight alternative to that alternative is to just >>> make the existing lock irq safe. Has this been considered? >>> >>> I'm not entirely convinced it's worth having a separate lock rather than >>> changing the existing one, but the patch still appears to be correct, so: >>> Reviewed-by: James Hogan <james.hogan@imgtec.com> >> >> I did look at that alternate solution and rejected it, but am happy to >> send that up if people think it's better. Here's why I rejected it: >> >> 1. It looks like someone has gone through quite a bit of work to _not_ >> grab the existing lock in the IRQ handler. The IRQ handler always >> pushes all real work over to the tasklet. I can only assume that they >> did this for some good reason and I'd hate to switch it without >> knowing for sure why. >> >> 2. We might have performance problems if we blindly changed the >> existing code to an IRQ-safe spinlock. We hold the existing >> "host->lock" spinlock for the entire duration of >> dw_mci_tasklet_func(). That's a pretty intense chunk of code, full of >> nested loops (some with 500ms timeouts!), mdelay(20) calls to handle >> some errors, etc. I say "might" because I think that the expected >> case is that code runs pretty quickly. I ran some brief tests on one >> system and saw a worst case time of 170us and an common case time of >> ~10us. Are we OK with having interrupts disabled for that long? Are >> we OK with having the dw_mci interrupt handler potentially spin on a >> spinlock for that long? >> > > Fair enough, I'm convinced now. That code did look pretty heavy when I > looked at it too. > >> >> I don't think it would be terrible to look at reworking the way that >> dw_mmc handles interrupts, but that seems pretty major. Yes, Reworking is pretty major. but if we need to rework, i think we can rework it in future. > > Yeh, I suppose at least the potentially large delays are all for > exceptional cases, so it's not a critical problem. > >> BTW: is the cost of an extra lock actually that high? > > I don't know tbh. In this case the spinlock usage doesn't appear to > actually overlap. > >> ...or are you talking the cost in terms of code complexity? > > In this case it'd only be a space and code complexity thing I think. I > suppose in some cases the benefit of finer-grained locking is probably > pretty marginal, but there's a good case for it here. It might be > worth renaming the lock to irq_lock or something like that so it's > clear it doesn't have to protect only for INTMASK in the future - up > to you. It seems good that use the irq_lock than intmask_lock. (It's just naming) > > Cheers > James > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jaehoon / James On Fri, Oct 18, 2013 at 2:51 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >> In this case it'd only be a space and code complexity thing I think. I >> suppose in some cases the benefit of finer-grained locking is probably >> pretty marginal, but there's a good case for it here. It might be >> worth renaming the lock to irq_lock or something like that so it's >> clear it doesn't have to protect only for INTMASK in the future - up >> to you. > It seems good that use the irq_lock than intmask_lock. (It's just naming) Done in v2. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 1b75816..b810654 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -657,6 +657,7 @@ disable: static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data) { + unsigned long irqflags; int sg_len; u32 temp; @@ -693,9 +694,11 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data) mci_writel(host, CTRL, temp); /* Disable RX/TX IRQs, let DMA handle it */ + spin_lock_irqsave(&host->intmask_lock, irqflags); temp = mci_readl(host, INTMASK); temp &= ~(SDMMC_INT_RXDR | SDMMC_INT_TXDR); mci_writel(host, INTMASK, temp); + spin_unlock_irqrestore(&host->intmask_lock, irqflags); host->dma_ops->start(host, sg_len); @@ -704,6 +707,7 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data) static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data) { + unsigned long irqflags; u32 temp; data->error = -EINPROGRESS; @@ -732,9 +736,12 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data) host->part_buf_count = 0; mci_writel(host, RINTSTS, SDMMC_INT_TXDR | SDMMC_INT_RXDR); + + spin_lock_irqsave(&host->intmask_lock, irqflags); temp = mci_readl(host, INTMASK); temp |= SDMMC_INT_TXDR | SDMMC_INT_RXDR; mci_writel(host, INTMASK, temp); + spin_unlock_irqrestore(&host->intmask_lock, irqflags); temp = mci_readl(host, CTRL); temp &= ~SDMMC_CTRL_DMA_ENABLE; @@ -1089,8 +1096,11 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) { struct dw_mci_slot *slot = mmc_priv(mmc); struct dw_mci *host = slot->host; + unsigned long irqflags; u32 int_mask; + spin_lock_irqsave(&host->intmask_lock, irqflags); + /* Enable/disable Slot Specific SDIO interrupt */ int_mask = mci_readl(host, INTMASK); if (enb) @@ -1098,6 +1108,8 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) else int_mask &= ~SDMMC_INT_SDIO(slot->id); mci_writel(host, INTMASK, int_mask); + + spin_unlock_irqrestore(&host->intmask_lock, irqflags); } static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode) @@ -2500,6 +2512,7 @@ int dw_mci_probe(struct dw_mci *host) host->quirks = host->pdata->quirks; spin_lock_init(&host->lock); + spin_lock_init(&host->intmask_lock); INIT_LIST_HEAD(&host->queue); /* diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h index 6ce7d2c..002ab56 100644 --- a/include/linux/mmc/dw_mmc.h +++ b/include/linux/mmc/dw_mmc.h @@ -102,6 +102,11 @@ struct mmc_data; * @cur_slot, @mrq and @state. These must always be updated * at the same time while holding @lock. * + * @intmask_lock is an irq-safe spinlock protecting the INTMASK register + * to allow the interrupt handler to modify it directly. Held for only long + * enough to read-modify-write INTMASK and no other locks are grabbed when + * holding this one. + * * The @mrq field of struct dw_mci_slot is also protected by @lock, * and must always be written at the same time as the slot is added to * @queue. @@ -121,6 +126,7 @@ struct mmc_data; */ struct dw_mci { spinlock_t lock; + spinlock_t intmask_lock; void __iomem *regs; struct scatterlist *sg;
We're running into cases where our enabling of the SDIO interrupt in dw_mmc doesn't actually take effect. Specifically, adding patch like this: +++ b/drivers/mmc/host/dw_mmc.c @@ -1076,6 +1076,9 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb) mci_writel(host, INTMASK, (int_mask | SDMMC_INT_SDIO(slot->id))); + int_mask = mci_readl(host, INTMASK); + if (!(int_mask & SDMMC_INT_SDIO(slot->id))) + dev_err(&mmc->class_dev, "failed to enable sdio irq\n"); } else { ...actually triggers the error message. That's because the dw_mci_enable_sdio_irq() unsafely does a read-modify-write of the INTMASK register. We can't just use the standard host->lock since that lock is not irq safe and mmc_signal_sdio_irq() (called from interrupt context) calls dw_mci_enable_sdio_irq(). Add a new irq-safe lock to protect INTMASK. An alternate solution to this is to punt mmc_signal_sdio_irq() to the tasklet and then protect INTMASK modifications by the standard host lock. This seemed like a bit more of a high-latency change. Reported-by: Bing Zhao <bzhao@marvell.com> Signed-off-by: Doug Anderson <dianders@chromium.org> --- drivers/mmc/host/dw_mmc.c | 13 +++++++++++++ include/linux/mmc/dw_mmc.h | 6 ++++++ 2 files changed, 19 insertions(+)