Bing Zhao at Marvell discovered a race in the way that dw_mmc was
doing a read-modify-write of the INTMASK register. This 2-patch
series attempts to fix the problem using a simple spinlock. In order
to do so cleanly, we include a patch to tidy up the way that we
disable low power mode when using SDIO interrupts.
This patch series was not tested on ToT Linux other than basic
compiling and booting, since we don't have the whole Marvell SDIO
stack up and running in mainline yet. This series is based on
mmc-next (e76b855 mmc: sdhci-esdhc-imx: set actual_clock in clock
setting) merged atop mainlinx Linux.
Doug Anderson (2):
mmc: dw_mmc: Cleanup disable of low power mode w/ SDIO interrupts
mmc: dw_mmc: Protect read-modify-write of INTMASK with a lock
drivers/mmc/host/dw_mmc.c | 79 +++++++++++++++++++++++++++-------------------
drivers/mmc/host/dw_mmc.h | 1 +
include/linux/mmc/dw_mmc.h | 6 ++++
3 files changed, 54 insertions(+), 32 deletions(-)
--
1.8.4
In the patch (9623b5b mmc: dw_mmc: Disable low power mode if SDIO
interrupts are used) I added code that disabled the low power mode of
dw_mmc when SDIO interrupts are used. That code worked but always
felt a little hacky because we ended up disabling low power as a side
effect of the first enable_sdio_irq() call. That wouldn't be so bad
except that disabling low power involves a complicated process of
writing to the CMD/CMDARG registers and that extra process makes it
difficult to cleanly the read-modify-write race in
dw_mci_enable_sdio_irq() (see future patch in the series).
Change the code to take advantage of the init_card() callback of the
mmc core to know when an SDIO card has been inserted. If we've got a
SDIO card and we're configured to use SDIO Interrupts then turn off
"low power mode" right away.
Signed-off-by: Doug Anderson <[email protected]>
---
drivers/mmc/host/dw_mmc.c | 68 ++++++++++++++++++++++++-----------------------
drivers/mmc/host/dw_mmc.h | 1 +
2 files changed, 36 insertions(+), 33 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 0a6a512..1b75816 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -27,6 +27,7 @@
#include <linux/stat.h>
#include <linux/delay.h>
#include <linux/irq.h>
+#include <linux/mmc/card.h>
#include <linux/mmc/host.h>
#include <linux/mmc/mmc.h>
#include <linux/mmc/sdio.h>
@@ -822,7 +823,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
/* enable clock; only low power if no SDIO */
clk_en_a = SDMMC_CLKEN_ENABLE << slot->id;
- if (!(mci_readl(host, INTMASK) & SDMMC_INT_SDIO(slot->id)))
+ if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags))
clk_en_a |= SDMMC_CLKEN_LOW_PWR << slot->id;
mci_writel(host, CLKENA, clk_en_a);
@@ -1050,27 +1051,37 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
return present;
}
-/*
- * Disable lower power mode.
- *
- * Low power mode will stop the card clock when idle. According to the
- * description of the CLKENA register we should disable low power mode
- * for SDIO cards if we need SDIO interrupts to work.
- *
- * This function is fast if low power mode is already disabled.
- */
-static void dw_mci_disable_low_power(struct dw_mci_slot *slot)
+static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
{
+ struct dw_mci_slot *slot = mmc_priv(mmc);
struct dw_mci *host = slot->host;
- u32 clk_en_a;
- const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
- clk_en_a = mci_readl(host, CLKENA);
+ /*
+ * Low power mode will stop the card clock when idle. According to the
+ * description of the CLKENA register we should disable low power mode
+ * for SDIO cards if we need SDIO interrupts to work.
+ */
+ if (mmc->caps | MMC_CAP_SDIO_IRQ) {
+ const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
+ u32 clk_en_a_old;
+ u32 clk_en_a;
- if (clk_en_a & clken_low_pwr) {
- mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
- mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
- SDMMC_CMD_PRV_DAT_WAIT, 0);
+ clk_en_a_old = mci_readl(host, CLKENA);
+
+ if (card->type == MMC_TYPE_SDIO ||
+ card->type == MMC_TYPE_SD_COMBO) {
+ set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+ clk_en_a = clk_en_a_old & ~clken_low_pwr;
+ } else {
+ clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
+ clk_en_a = clk_en_a_old | clken_low_pwr;
+ }
+
+ if (clk_en_a != clk_en_a_old) {
+ mci_writel(host, CLKENA, clk_en_a);
+ mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
+ SDMMC_CMD_PRV_DAT_WAIT, 0);
+ }
}
}
@@ -1082,21 +1093,11 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
/* Enable/disable Slot Specific SDIO interrupt */
int_mask = mci_readl(host, INTMASK);
- if (enb) {
- /*
- * Turn off low power mode if it was enabled. This is a bit of
- * a heavy operation and we disable / enable IRQs a lot, so
- * we'll leave low power mode disabled and it will get
- * re-enabled again in dw_mci_setup_bus().
- */
- dw_mci_disable_low_power(slot);
-
- mci_writel(host, INTMASK,
- (int_mask | SDMMC_INT_SDIO(slot->id)));
- } else {
- mci_writel(host, INTMASK,
- (int_mask & ~SDMMC_INT_SDIO(slot->id)));
- }
+ if (enb)
+ int_mask |= SDMMC_INT_SDIO(slot->id);
+ else
+ int_mask &= ~SDMMC_INT_SDIO(slot->id);
+ mci_writel(host, INTMASK, int_mask);
}
static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
@@ -1140,6 +1141,7 @@ static const struct mmc_host_ops dw_mci_ops = {
.get_cd = dw_mci_get_cd,
.enable_sdio_irq = dw_mci_enable_sdio_irq,
.execute_tuning = dw_mci_execute_tuning,
+ .init_card = dw_mci_init_card,
};
static void dw_mci_request_end(struct dw_mci *host, struct mmc_request *mrq)
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 6bf24ab..26d4657 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -227,6 +227,7 @@ struct dw_mci_slot {
unsigned long flags;
#define DW_MMC_CARD_PRESENT 0
#define DW_MMC_CARD_NEED_INIT 1
+#define DW_MMC_CARD_NO_LOW_PWR 2
int id;
int last_detect_state;
};
--
1.8.4
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 <[email protected]>
Signed-off-by: Doug Anderson <[email protected]>
---
drivers/mmc/host/dw_mmc.c | 13 +++++++++++++
include/linux/mmc/dw_mmc.h | 6 ++++++
2 files changed, 19 insertions(+)
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;
--
1.8.4
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 <[email protected]>
Cheers
James
James,
On Wed, Oct 16, 2013 at 2:49 AM, James Hogan <[email protected]> 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 <[email protected]>
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
Hi Doug,
On 16 October 2013 17:43, Doug Anderson <[email protected]> wrote:
> On Wed, Oct 16, 2013 at 2:49 AM, James Hogan <[email protected]> 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 <[email protected]>
>
> 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
Hi Doug,
On 10/16/2013 07:39 AM, Doug Anderson wrote:
> In the patch (9623b5b mmc: dw_mmc: Disable low power mode if SDIO
> interrupts are used) I added code that disabled the low power mode of
> dw_mmc when SDIO interrupts are used. That code worked but always
> felt a little hacky because we ended up disabling low power as a side
> effect of the first enable_sdio_irq() call. That wouldn't be so bad
> except that disabling low power involves a complicated process of
> writing to the CMD/CMDARG registers and that extra process makes it
> difficult to cleanly the read-modify-write race in
> dw_mci_enable_sdio_irq() (see future patch in the series).
>
> Change the code to take advantage of the init_card() callback of the
> mmc core to know when an SDIO card has been inserted. If we've got a
> SDIO card and we're configured to use SDIO Interrupts then turn off
> "low power mode" right away.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> drivers/mmc/host/dw_mmc.c | 68 ++++++++++++++++++++++++-----------------------
> drivers/mmc/host/dw_mmc.h | 1 +
> 2 files changed, 36 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 0a6a512..1b75816 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -27,6 +27,7 @@
> #include <linux/stat.h>
> #include <linux/delay.h>
> #include <linux/irq.h>
> +#include <linux/mmc/card.h>
> #include <linux/mmc/host.h>
> #include <linux/mmc/mmc.h>
> #include <linux/mmc/sdio.h>
> @@ -822,7 +823,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>
> /* enable clock; only low power if no SDIO */
> clk_en_a = SDMMC_CLKEN_ENABLE << slot->id;
> - if (!(mci_readl(host, INTMASK) & SDMMC_INT_SDIO(slot->id)))
> + if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags))
> clk_en_a |= SDMMC_CLKEN_LOW_PWR << slot->id;
> mci_writel(host, CLKENA, clk_en_a);
>
> @@ -1050,27 +1051,37 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> return present;
> }
>
> -/*
> - * Disable lower power mode.
> - *
> - * Low power mode will stop the card clock when idle. According to the
> - * description of the CLKENA register we should disable low power mode
> - * for SDIO cards if we need SDIO interrupts to work.
> - *
> - * This function is fast if low power mode is already disabled.
> - */
> -static void dw_mci_disable_low_power(struct dw_mci_slot *slot)
> +static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
> {
> + struct dw_mci_slot *slot = mmc_priv(mmc);
> struct dw_mci *host = slot->host;
> - u32 clk_en_a;
> - const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>
> - clk_en_a = mci_readl(host, CLKENA);
> + /*
> + * Low power mode will stop the card clock when idle. According to the
> + * description of the CLKENA register we should disable low power mode
> + * for SDIO cards if we need SDIO interrupts to work.
> + */
> + if (mmc->caps | MMC_CAP_SDIO_IRQ) {
mmc->caps & MMC_CAP_SDIO_IRQ?
> + const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
> + u32 clk_en_a_old;
> + u32 clk_en_a;
>
> - if (clk_en_a & clken_low_pwr) {
> - mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
> - mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> - SDMMC_CMD_PRV_DAT_WAIT, 0);
> + clk_en_a_old = mci_readl(host, CLKENA);
> +
> + if (card->type == MMC_TYPE_SDIO ||
> + card->type == MMC_TYPE_SD_COMBO) {
> + set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> + clk_en_a = clk_en_a_old & ~clken_low_pwr;
> + } else {
> + clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> + clk_en_a = clk_en_a_old | clken_low_pwr;
When this condition is entered? card->type is always MMC_TYPE_SDIO or MMC_TYPE_SD_COMBO, isn't?
Best Regards,
Jaehoon Chung
> + }
> +
> + if (clk_en_a != clk_en_a_old) {
> + mci_writel(host, CLKENA, clk_en_a);
> + mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> + SDMMC_CMD_PRV_DAT_WAIT, 0);
> + }
> }
> }
>
> @@ -1082,21 +1093,11 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>
> /* Enable/disable Slot Specific SDIO interrupt */
> int_mask = mci_readl(host, INTMASK);
> - if (enb) {
> - /*
> - * Turn off low power mode if it was enabled. This is a bit of
> - * a heavy operation and we disable / enable IRQs a lot, so
> - * we'll leave low power mode disabled and it will get
> - * re-enabled again in dw_mci_setup_bus().
> - */
> - dw_mci_disable_low_power(slot);
> -
> - mci_writel(host, INTMASK,
> - (int_mask | SDMMC_INT_SDIO(slot->id)));
> - } else {
> - mci_writel(host, INTMASK,
> - (int_mask & ~SDMMC_INT_SDIO(slot->id)));
> - }
> + if (enb)
> + int_mask |= SDMMC_INT_SDIO(slot->id);
> + else
> + int_mask &= ~SDMMC_INT_SDIO(slot->id);
> + mci_writel(host, INTMASK, int_mask);
> }
>
> static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> @@ -1140,6 +1141,7 @@ static const struct mmc_host_ops dw_mci_ops = {
> .get_cd = dw_mci_get_cd,
> .enable_sdio_irq = dw_mci_enable_sdio_irq,
> .execute_tuning = dw_mci_execute_tuning,
> + .init_card = dw_mci_init_card,
> };
>
> static void dw_mci_request_end(struct dw_mci *host, struct mmc_request *mrq)
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 6bf24ab..26d4657 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -227,6 +227,7 @@ struct dw_mci_slot {
> unsigned long flags;
> #define DW_MMC_CARD_PRESENT 0
> #define DW_MMC_CARD_NEED_INIT 1
> +#define DW_MMC_CARD_NO_LOW_PWR 2
> int id;
> int last_detect_state;
> };
>
On 10/17/2013 05:23 AM, James Hogan wrote:
> Hi Doug,
>
> On 16 October 2013 17:43, Doug Anderson <[email protected]> wrote:
>> On Wed, Oct 16, 2013 at 2:49 AM, James Hogan <[email protected]> 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 <[email protected]>
>>
>> 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
>
Jaehoon,
On Fri, Oct 18, 2013 at 2:42 AM, Jaehoon Chung <[email protected]> wrote:
>> - clk_en_a = mci_readl(host, CLKENA);
>> + /*
>> + * Low power mode will stop the card clock when idle. According to the
>> + * description of the CLKENA register we should disable low power mode
>> + * for SDIO cards if we need SDIO interrupts to work.
>> + */
>> + if (mmc->caps | MMC_CAP_SDIO_IRQ) {
> mmc->caps & MMC_CAP_SDIO_IRQ?
Wow, that was an embarrassing one. Thanks.
>> + const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
>> + u32 clk_en_a_old;
>> + u32 clk_en_a;
>>
>> - if (clk_en_a & clken_low_pwr) {
>> - mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
>> - mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
>> - SDMMC_CMD_PRV_DAT_WAIT, 0);
>> + clk_en_a_old = mci_readl(host, CLKENA);
>> +
>> + if (card->type == MMC_TYPE_SDIO ||
>> + card->type == MMC_TYPE_SD_COMBO) {
>> + set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>> + clk_en_a = clk_en_a_old & ~clken_low_pwr;
>> + } else {
>> + clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
>> + clk_en_a = clk_en_a_old | clken_low_pwr;
> When this condition is entered? card->type is always MMC_TYPE_SDIO or MMC_TYPE_SD_COMBO, isn't?
Ugh, that's not intuitive. This callback is only called for SDIO
cards and not MMC/SD cards? That means if you plug in an SDIO card
and then eject it and plug in an SD card you won't get to low power.
Hrm.
I dug around a bit and couldn't find a better way to do this and then
I realized that the other user of the init_card() callback has the
same bug, so for the next version of the series I'm proposing a fix
for mmc core to add this for all types. If you have a better
suggestion, I'm all ears.
-Doug
Jaehoon / James
On Fri, Oct 18, 2013 at 2:51 AM, Jaehoon Chung <[email protected]> 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.
Hi Doug,
This change looks good.
There's comment below.
On Sat, October 19, 2013, Doug Anderson wrote:
> Jaehoon,
>
> On Fri, Oct 18, 2013 at 2:42 AM, Jaehoon Chung <[email protected]> wrote:
> >> - clk_en_a = mci_readl(host, CLKENA);
> >> + /*
> >> + * Low power mode will stop the card clock when idle. According to the
> >> + * description of the CLKENA register we should disable low power mode
> >> + * for SDIO cards if we need SDIO interrupts to work.
> >> + */
> >> + if (mmc->caps | MMC_CAP_SDIO_IRQ) {
> > mmc->caps & MMC_CAP_SDIO_IRQ?
>
> Wow, that was an embarrassing one. Thanks.
>
> >> + const u32 clken_low_pwr = SDMMC_CLKEN_LOW_PWR << slot->id;
> >> + u32 clk_en_a_old;
> >> + u32 clk_en_a;
> >>
> >> - if (clk_en_a & clken_low_pwr) {
> >> - mci_writel(host, CLKENA, clk_en_a & ~clken_low_pwr);
> >> - mci_send_cmd(slot, SDMMC_CMD_UPD_CLK |
> >> - SDMMC_CMD_PRV_DAT_WAIT, 0);
> >> + clk_en_a_old = mci_readl(host, CLKENA);
> >> +
> >> + if (card->type == MMC_TYPE_SDIO ||
> >> + card->type == MMC_TYPE_SD_COMBO) {
&& card->quirks & MMC_QUIRK_BROKEN_CLK_GATING
How about considering MMC_QUIRK_BROKEN_CLK_GATING?
Some sdio device can work with gating clock.
For this, mmc_fixup_device() should be called prior to init_card() in core(sdio.c).
I guess you found that.
Thanks,
Seungwon Jeon
> >> + set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> >> + clk_en_a = clk_en_a_old & ~clken_low_pwr;
> >> + } else {
> >> + clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
> >> + clk_en_a = clk_en_a_old | clken_low_pwr;
> > When this condition is entered? card->type is always MMC_TYPE_SDIO or MMC_TYPE_SD_COMBO, isn't?
>
> Ugh, that's not intuitive. This callback is only called for SDIO
> cards and not MMC/SD cards? That means if you plug in an SDIO card
> and then eject it and plug in an SD card you won't get to low power.
> Hrm.
>
> I dug around a bit and couldn't find a better way to do this and then
> I realized that the other user of the init_card() callback has the
> same bug, so for the next version of the series I'm proposing a fix
> for mmc core to add this for all types. If you have a better
> suggestion, I'm all ears.
>
> -Doug
> --
> 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
Seungwon,
On Wed, Oct 23, 2013 at 12:25 PM, Seungwon Jeon <[email protected]> wrote:
>> >> + if (card->type == MMC_TYPE_SDIO ||
>> >> + card->type == MMC_TYPE_SD_COMBO) {
> && card->quirks & MMC_QUIRK_BROKEN_CLK_GATING
> How about considering MMC_QUIRK_BROKEN_CLK_GATING?
> Some sdio device can work with gating clock.
> For this, mmc_fixup_device() should be called prior to init_card() in core(sdio.c).
> I guess you found that.
By SDIO devices, are you referring to actual SDIO cards or some
implementations of dw_mmc?
As far as I understand in the CLKENA description in the generic
documentation from Synopsys it say that for SDIO cards you must not
stop the clock if interrupts must be detected. To me, that means that
all dw_mmc implementations require this change if they support SDIO
interrupts (hence checking for MMC_CAP_SDIO_IRQ).
I guess I did make the assumption in this change that all (reasonable)
SDIO cards would be using SDIO interrupts if they are available. If
we could find out ahead of time if a given SDIO driver was planning to
use interrupts we could do better. The old solution did better in
this way and we could probably make it work (and still fix the
read-modify-write race) if we thought this was really important. The
code gets a little more twisted to try to avoid holding the IRQ-safe
spinlock while updating the clock, but I could attempt it...
NOTE: We've recently found that there are still occasions when we lose
SDIO interrupts with dw_mmc and our Marvell WiFi driver, especially
when those interrupts happen back-to-back. That problem appears
unrelated to this one (it's what Bing was investigating when he found
the original race). Anyone that has any thoughts, please let me
know...
-Doug
On Thu, October 24, 2013, Doug Anderson wrote:
> Seungwon,
>
> On Wed, Oct 23, 2013 at 12:25 PM, Seungwon Jeon <[email protected]> wrote:
> >> >> + if (card->type == MMC_TYPE_SDIO ||
> >> >> + card->type == MMC_TYPE_SD_COMBO) {
> > && card->quirks & MMC_QUIRK_BROKEN_CLK_GATING
> > How about considering MMC_QUIRK_BROKEN_CLK_GATING?
> > Some sdio device can work with gating clock.
> > For this, mmc_fixup_device() should be called prior to init_card() in core(sdio.c).
> > I guess you found that.
>
> By SDIO devices, are you referring to actual SDIO cards or some
> implementations of dw_mmc?
>
> As far as I understand in the CLKENA description in the generic
> documentation from Synopsys it say that for SDIO cards you must not
> stop the clock if interrupts must be detected. To me, that means that
> all dw_mmc implementations require this change if they support SDIO
> interrupts (hence checking for MMC_CAP_SDIO_IRQ).
CLKENA description in manual means that host controller can't detect the SDIO interrupt signal
or wifi device can't generate the interrupt without clock?
Host controller based on Synopsys supports asynchronous interrupts. It seems to depend on wifi Device.
If host can do and wifi device can also work with clock gating, we may enable low-power mode.
For MMC_QUIRK_BROKEN_CLK_GATING, I referred the code in 'mmc/core/quirks.c'
In addition, although host can support MMC_CAP_SDIO_IRQ, some wifi drivers use
OOB(Out-of-band) interrupt. That means host can apply clock gating to reduce
power consumption.
Thanks,
Seungwon Jeon
>
> I guess I did make the assumption in this change that all (reasonable)
> SDIO cards would be using SDIO interrupts if they are available. If
> we could find out ahead of time if a given SDIO driver was planning to
> use interrupts we could do better. The old solution did better in
> this way and we could probably make it work (and still fix the
> read-modify-write race) if we thought this was really important. The
> code gets a little more twisted to try to avoid holding the IRQ-safe
> spinlock while updating the clock, but I could attempt it...
>
>
> NOTE: We've recently found that there are still occasions when we lose
> SDIO interrupts with dw_mmc and our Marvell WiFi driver, especially
> when those interrupts happen back-to-back. That problem appears
> unrelated to this one (it's what Bing was investigating when he found
> the original race). Anyone that has any thoughts, please let me
> know...
>
> -Doug
> --
> 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
Seungwon,
On Fri, Oct 25, 2013 at 2:29 AM, Seungwon Jeon <[email protected]> wrote:
>> By SDIO devices, are you referring to actual SDIO cards or some
>> implementations of dw_mmc?
>>
>> As far as I understand in the CLKENA description in the generic
>> documentation from Synopsys it say that for SDIO cards you must not
>> stop the clock if interrupts must be detected. To me, that means that
>> all dw_mmc implementations require this change if they support SDIO
>> interrupts (hence checking for MMC_CAP_SDIO_IRQ).
>
> CLKENA description in manual means that host controller can't detect the SDIO interrupt signal
> or wifi device can't generate the interrupt without clock?
My reading of the "if interrupts must be detected" in the manual
implies that that interrupts simply can't be detected by the
controller.
> Host controller based on Synopsys supports asynchronous interrupts. It seems to depend on wifi Device.
> If host can do and wifi device can also work with clock gating, we may enable low-power mode.
Ah, interesting! I wish I had known about this earlier and we could
have actually used it in our designs. Please correct me if I'm wrong
but...
It looks like asynchronous interrupts is when you use a separate INT#
line for your SDIO interrupts and is available only for eSDIO
(embedded SDIO?), right. ...so that means it more a property of the
board rather than the card itself. In other words: to use
asynchronous interrupts you need to be on a SoC that supports the INT#
line, you need to have it wired up to an eSDIO module, and you need
the eSDIO card to support it.
Assuming that I understand all of the above I'd suggest (in a future
patch) that we add a property like 'dedicated-sdio-irq' to device
trees. If we see this property AND we don't see
MMC_QUIRK_BROKEN_CLK_GATING then we know we don't need to disable
clock gating.
Does that sound right? If so I'd still love to land my patch and we
could add the extra logic in a separate patch.
> For MMC_QUIRK_BROKEN_CLK_GATING, I referred the code in 'mmc/core/quirks.c'
> In addition, although host can support MMC_CAP_SDIO_IRQ, some wifi drivers use
> OOB(Out-of-band) interrupt. That means host can apply clock gating to reduce
> power consumption.
I think OOB interrupt is the same as asynchronous interrupt, but if
I'm wrong please correct me.
-Doug