* [PATCH] mmc: sdhci: fix possible scheduling while atomic
@ 2014-01-17 19:57 Andrew Bresticker
2014-01-17 22:58 ` Philip Rakity
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Bresticker @ 2014-01-17 19:57 UTC (permalink / raw)
To: Chris Ball; +Cc: linux-mmc, linux-kernel, Andrew Bresticker
sdhci_execute_tuning() takes host->lock without disabling interrupts.
Use spin_lock_irq{save,restore} instead so that we avoid taking an
interrupt and scheduling while holding host->lock.
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
---
drivers/mmc/host/sdhci.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ec3eb30..84c80e7 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1857,12 +1857,13 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
unsigned long timeout;
int err = 0;
bool requires_tuning_nonuhs = false;
+ unsigned long flags;
host = mmc_priv(mmc);
sdhci_runtime_pm_get(host);
disable_irq(host->irq);
- spin_lock(&host->lock);
+ spin_lock_irqsave(&host->lock, flags);
ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
@@ -1882,14 +1883,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
requires_tuning_nonuhs)
ctrl |= SDHCI_CTRL_EXEC_TUNING;
else {
- spin_unlock(&host->lock);
+ spin_unlock_irqrestore(&host->lock, flags);
enable_irq(host->irq);
sdhci_runtime_pm_put(host);
return 0;
}
if (host->ops->platform_execute_tuning) {
- spin_unlock(&host->lock);
+ spin_unlock_irqrestore(&host->lock, flags);
enable_irq(host->irq);
err = host->ops->platform_execute_tuning(host, opcode);
sdhci_runtime_pm_put(host);
@@ -1963,7 +1964,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
host->cmd = NULL;
host->mrq = NULL;
- spin_unlock(&host->lock);
+ spin_unlock_irqrestore(&host->lock, flags);
enable_irq(host->irq);
/* Wait for Buffer Read Ready interrupt */
@@ -1971,7 +1972,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
(host->tuning_done == 1),
msecs_to_jiffies(50));
disable_irq(host->irq);
- spin_lock(&host->lock);
+ spin_lock_irqsave(&host->lock, flags);
if (!host->tuning_done) {
pr_info(DRIVER_NAME ": Timeout waiting for "
@@ -2046,7 +2047,7 @@ out:
err = 0;
sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
- spin_unlock(&host->lock);
+ spin_unlock_irqrestore(&host->lock, flags);
enable_irq(host->irq);
sdhci_runtime_pm_put(host);
--
1.8.5.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic
2014-01-17 19:57 [PATCH] mmc: sdhci: fix possible scheduling while atomic Andrew Bresticker
@ 2014-01-17 22:58 ` Philip Rakity
2014-01-17 23:10 ` Andrew Bresticker
2014-01-17 23:11 ` John Tobias
0 siblings, 2 replies; 10+ messages in thread
From: Philip Rakity @ 2014-01-17 22:58 UTC (permalink / raw)
To: Andrew Bresticker
Cc: Chris Ball, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
On Jan 17, 2014, at 7:57 PM, Andrew Bresticker <abrestic@chromium.org> wrote:
> sdhci_execute_tuning() takes host->lock without disabling interrupts.
> Use spin_lock_irq{save,restore} instead so that we avoid taking an
> interrupt and scheduling while holding host->lock.
>
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> ---
> drivers/mmc/host/sdhci.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ec3eb30..84c80e7 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1857,12 +1857,13 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> unsigned long timeout;
> int err = 0;
> bool requires_tuning_nonuhs = false;
> + unsigned long flags;
>
> host = mmc_priv(mmc);
>
> sdhci_runtime_pm_get(host);
> disable_irq(host->irq);
> - spin_lock(&host->lock);
> + spin_lock_irqsave(&host->lock, flags);
The disable_irq() call stops the controller from doing interrupts.
Please explain what problem you are seeing
>
> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>
> @@ -1882,14 +1883,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> requires_tuning_nonuhs)
> ctrl |= SDHCI_CTRL_EXEC_TUNING;
> else {
> - spin_unlock(&host->lock);
> + spin_unlock_irqrestore(&host->lock, flags);
> enable_irq(host->irq);
> sdhci_runtime_pm_put(host);
> return 0;
> }
>
> if (host->ops->platform_execute_tuning) {
> - spin_unlock(&host->lock);
> + spin_unlock_irqrestore(&host->lock, flags);
> enable_irq(host->irq);
> err = host->ops->platform_execute_tuning(host, opcode);
> sdhci_runtime_pm_put(host);
> @@ -1963,7 +1964,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> host->cmd = NULL;
> host->mrq = NULL;
>
> - spin_unlock(&host->lock);
> + spin_unlock_irqrestore(&host->lock, flags);
> enable_irq(host->irq);
>
> /* Wait for Buffer Read Ready interrupt */
> @@ -1971,7 +1972,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> (host->tuning_done == 1),
> msecs_to_jiffies(50));
> disable_irq(host->irq);
> - spin_lock(&host->lock);
> + spin_lock_irqsave(&host->lock, flags);
>
> if (!host->tuning_done) {
> pr_info(DRIVER_NAME ": Timeout waiting for "
> @@ -2046,7 +2047,7 @@ out:
> err = 0;
>
> sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
> - spin_unlock(&host->lock);
> + spin_unlock_irqrestore(&host->lock, flags);
> enable_irq(host->irq);
> sdhci_runtime_pm_put(host);
>
> --
> 1.8.5.2
>
> --
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic
2014-01-17 22:58 ` Philip Rakity
@ 2014-01-17 23:10 ` Andrew Bresticker
2014-01-17 23:11 ` John Tobias
1 sibling, 0 replies; 10+ messages in thread
From: Andrew Bresticker @ 2014-01-17 23:10 UTC (permalink / raw)
To: Philip Rakity
Cc: Chris Ball, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
On Fri, Jan 17, 2014 at 2:58 PM, Philip Rakity <prakity@nvidia.com> wrote:
>
> On Jan 17, 2014, at 7:57 PM, Andrew Bresticker <abrestic@chromium.org> wrote:
>
>> sdhci_execute_tuning() takes host->lock without disabling interrupts.
>> Use spin_lock_irq{save,restore} instead so that we avoid taking an
>> interrupt and scheduling while holding host->lock.
>>
>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>> ---
>> drivers/mmc/host/sdhci.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ec3eb30..84c80e7 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1857,12 +1857,13 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> unsigned long timeout;
>> int err = 0;
>> bool requires_tuning_nonuhs = false;
>> + unsigned long flags;
>>
>> host = mmc_priv(mmc);
>>
>> sdhci_runtime_pm_get(host);
>> disable_irq(host->irq);
>> - spin_lock(&host->lock);
>> + spin_lock_irqsave(&host->lock, flags);
>
>
> The disable_irq() call stops the controller from doing interrupts.
Right, but it does not disable other IRQ sources that could cause us
to schedule.
> Please explain what problem you are seeing
The issue we were seeing was that a card-detect interrupt was
triggered (*not* the controller interrupt), causing the card-detect
irq thread to recurse on host->lock:
[ 60.962218] BUG: spinlock cpu recursion on CPU#0, irq/362-700b040/89
[ 60.975253] lock: 0xee210c80, .magic: dead4ead, .owner:
kworker/u8:1/33, .owner_cpu: 0
[ 60.991638] CPU: 0 PID: 89 Comm: irq/362-700b040 Not tainted 3.10.18 #2
[ 61.005199] [<800153cc>] (unwind_backtrace+0x0/0x118) from
[<800124e4>] (show_stack+0x20/0x24)
[ 61.022824] [<800124e4>] (show_stack+0x20/0x24) from [<8053d584>]
(dump_stack+0x20/0x28)
[ 61.039389] [<8053d584>] (dump_stack+0x20/0x28) from [<8021d508>]
(spin_dump+0x80/0x94)
[ 61.055773] [<8021d508>] (spin_dump+0x80/0x94) from [<8021d548>]
(spin_bug+0x2c/0x30)
[ 61.071803] [<8021d548>] (spin_bug+0x2c/0x30) from [<8021d61c>]
(do_raw_spin_lock+0x70/0x15c)
[ 61.089250] [<8021d61c>] (do_raw_spin_lock+0x70/0x15c) from
[<8054098c>] (_raw_spin_lock_irqsave+0x20/0x28)
[ 61.109175] [<8054098c>] (_raw_spin_lock_irqsave+0x20/0x28) from
[<803e2af0>] (sdhci_card_event+0x28/0xfc)
[ 61.128922] [<803e2af0>] (sdhci_card_event+0x28/0xfc) from
[<803dc880>] (mmc_gpio_cd_irqt+0x30/0x4c)
[ 61.147609] [<803dc880>] (mmc_gpio_cd_irqt+0x30/0x4c) from
[<80091858>] (irq_thread+0xf0/0x224)
[ 61.165412] [<80091858>] (irq_thread+0xf0/0x224) from [<80050db4>]
(kthread+0xc8/0xd8)
[ 61.181623] [<80050db4>] (kthread+0xc8/0xd8) from [<8000e4d8>]
(ret_from_fork+0x14/0x20)
Thanks,
Andrew
>
>>
>> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>
>> @@ -1882,14 +1883,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> requires_tuning_nonuhs)
>> ctrl |= SDHCI_CTRL_EXEC_TUNING;
>> else {
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>> sdhci_runtime_pm_put(host);
>> return 0;
>> }
>>
>> if (host->ops->platform_execute_tuning) {
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>> err = host->ops->platform_execute_tuning(host, opcode);
>> sdhci_runtime_pm_put(host);
>> @@ -1963,7 +1964,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> host->cmd = NULL;
>> host->mrq = NULL;
>>
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>>
>> /* Wait for Buffer Read Ready interrupt */
>> @@ -1971,7 +1972,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> (host->tuning_done == 1),
>> msecs_to_jiffies(50));
>> disable_irq(host->irq);
>> - spin_lock(&host->lock);
>> + spin_lock_irqsave(&host->lock, flags);
>>
>> if (!host->tuning_done) {
>> pr_info(DRIVER_NAME ": Timeout waiting for "
>> @@ -2046,7 +2047,7 @@ out:
>> err = 0;
>>
>> sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>> sdhci_runtime_pm_put(host);
>>
>> --
>> 1.8.5.2
>>
>> --
>> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic
2014-01-17 22:58 ` Philip Rakity
2014-01-17 23:10 ` Andrew Bresticker
@ 2014-01-17 23:11 ` John Tobias
2014-01-17 23:16 ` Andrew Bresticker
1 sibling, 1 reply; 10+ messages in thread
From: John Tobias @ 2014-01-17 23:11 UTC (permalink / raw)
To: Philip Rakity
Cc: Andrew Bresticker, Chris Ball, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
There's an existing patch for that...
http://www.spinics.net/lists/arm-kernel/msg296596.html
On Fri, Jan 17, 2014 at 2:58 PM, Philip Rakity <prakity@nvidia.com> wrote:
>
> On Jan 17, 2014, at 7:57 PM, Andrew Bresticker <abrestic@chromium.org> wrote:
>
>> sdhci_execute_tuning() takes host->lock without disabling interrupts.
>> Use spin_lock_irq{save,restore} instead so that we avoid taking an
>> interrupt and scheduling while holding host->lock.
>>
>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>> ---
>> drivers/mmc/host/sdhci.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ec3eb30..84c80e7 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1857,12 +1857,13 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> unsigned long timeout;
>> int err = 0;
>> bool requires_tuning_nonuhs = false;
>> + unsigned long flags;
>>
>> host = mmc_priv(mmc);
>>
>> sdhci_runtime_pm_get(host);
>> disable_irq(host->irq);
>> - spin_lock(&host->lock);
>> + spin_lock_irqsave(&host->lock, flags);
>
>
> The disable_irq() call stops the controller from doing interrupts.
> Please explain what problem you are seeing
>
>>
>> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>
>> @@ -1882,14 +1883,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> requires_tuning_nonuhs)
>> ctrl |= SDHCI_CTRL_EXEC_TUNING;
>> else {
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>> sdhci_runtime_pm_put(host);
>> return 0;
>> }
>>
>> if (host->ops->platform_execute_tuning) {
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>> err = host->ops->platform_execute_tuning(host, opcode);
>> sdhci_runtime_pm_put(host);
>> @@ -1963,7 +1964,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> host->cmd = NULL;
>> host->mrq = NULL;
>>
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>>
>> /* Wait for Buffer Read Ready interrupt */
>> @@ -1971,7 +1972,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> (host->tuning_done == 1),
>> msecs_to_jiffies(50));
>> disable_irq(host->irq);
>> - spin_lock(&host->lock);
>> + spin_lock_irqsave(&host->lock, flags);
>>
>> if (!host->tuning_done) {
>> pr_info(DRIVER_NAME ": Timeout waiting for "
>> @@ -2046,7 +2047,7 @@ out:
>> err = 0;
>>
>> sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>> sdhci_runtime_pm_put(host);
>>
>> --
>> 1.8.5.2
>>
>> --
>> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic
2014-01-17 23:11 ` John Tobias
@ 2014-01-17 23:16 ` Andrew Bresticker
2014-01-17 23:40 ` Chris Ball
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Bresticker @ 2014-01-17 23:16 UTC (permalink / raw)
To: John Tobias
Cc: Philip Rakity, Chris Ball, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org
On Fri, Jan 17, 2014 at 3:11 PM, John Tobias <john.tobias.ph@gmail.com> wrote:
> There's an existing patch for that...
> http://www.spinics.net/lists/arm-kernel/msg296596.html
Ah, I see. Looks like it has yet to be picked up...
>
> On Fri, Jan 17, 2014 at 2:58 PM, Philip Rakity <prakity@nvidia.com> wrote:
>>
>> On Jan 17, 2014, at 7:57 PM, Andrew Bresticker <abrestic@chromium.org> wrote:
>>
>>> sdhci_execute_tuning() takes host->lock without disabling interrupts.
>>> Use spin_lock_irq{save,restore} instead so that we avoid taking an
>>> interrupt and scheduling while holding host->lock.
>>>
>>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>>> ---
>>> drivers/mmc/host/sdhci.c | 13 +++++++------
>>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index ec3eb30..84c80e7 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1857,12 +1857,13 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>> unsigned long timeout;
>>> int err = 0;
>>> bool requires_tuning_nonuhs = false;
>>> + unsigned long flags;
>>>
>>> host = mmc_priv(mmc);
>>>
>>> sdhci_runtime_pm_get(host);
>>> disable_irq(host->irq);
>>> - spin_lock(&host->lock);
>>> + spin_lock_irqsave(&host->lock, flags);
>>
>>
>> The disable_irq() call stops the controller from doing interrupts.
>> Please explain what problem you are seeing
>>
>>>
>>> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>
>>> @@ -1882,14 +1883,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>> requires_tuning_nonuhs)
>>> ctrl |= SDHCI_CTRL_EXEC_TUNING;
>>> else {
>>> - spin_unlock(&host->lock);
>>> + spin_unlock_irqrestore(&host->lock, flags);
>>> enable_irq(host->irq);
>>> sdhci_runtime_pm_put(host);
>>> return 0;
>>> }
>>>
>>> if (host->ops->platform_execute_tuning) {
>>> - spin_unlock(&host->lock);
>>> + spin_unlock_irqrestore(&host->lock, flags);
>>> enable_irq(host->irq);
>>> err = host->ops->platform_execute_tuning(host, opcode);
>>> sdhci_runtime_pm_put(host);
>>> @@ -1963,7 +1964,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>> host->cmd = NULL;
>>> host->mrq = NULL;
>>>
>>> - spin_unlock(&host->lock);
>>> + spin_unlock_irqrestore(&host->lock, flags);
>>> enable_irq(host->irq);
>>>
>>> /* Wait for Buffer Read Ready interrupt */
>>> @@ -1971,7 +1972,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>> (host->tuning_done == 1),
>>> msecs_to_jiffies(50));
>>> disable_irq(host->irq);
>>> - spin_lock(&host->lock);
>>> + spin_lock_irqsave(&host->lock, flags);
>>>
>>> if (!host->tuning_done) {
>>> pr_info(DRIVER_NAME ": Timeout waiting for "
>>> @@ -2046,7 +2047,7 @@ out:
>>> err = 0;
>>>
>>> sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
>>> - spin_unlock(&host->lock);
>>> + spin_unlock_irqrestore(&host->lock, flags);
>>> enable_irq(host->irq);
>>> sdhci_runtime_pm_put(host);
>>>
>>> --
>>> 1.8.5.2
>>>
>>> --
>>> 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
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic
2014-01-17 23:16 ` Andrew Bresticker
@ 2014-01-17 23:40 ` Chris Ball
2014-01-18 3:21 ` Andrew Bresticker
0 siblings, 1 reply; 10+ messages in thread
From: Chris Ball @ 2014-01-17 23:40 UTC (permalink / raw)
To: Andrew Bresticker
Cc: John Tobias, Philip Rakity, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org, Dong Aisheng
Hi, adding Aisheng,
On Fri, Jan 17 2014, Andrew Bresticker wrote:
> On Fri, Jan 17, 2014 at 3:11 PM, John Tobias <john.tobias.ph@gmail.com> wrote:
>> There's an existing patch for that...
>> http://www.spinics.net/lists/arm-kernel/msg296596.html
>
> Ah, I see. Looks like it has yet to be picked up...
The patches aren't quite identical -- Andrew's leaves the
disable_irq() call in and Aisheng's removes it. Which should I take?
Thanks,
- Chris.
--
Chris Ball <chris@printf.net> <http://printf.net/>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic
2014-01-17 23:40 ` Chris Ball
@ 2014-01-18 3:21 ` Andrew Bresticker
2014-01-18 3:40 ` Chris Ball
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Bresticker @ 2014-01-18 3:21 UTC (permalink / raw)
To: Chris Ball
Cc: John Tobias, Philip Rakity, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org, Dong Aisheng
On Fri, Jan 17, 2014 at 3:40 PM, Chris Ball <chris@printf.net> wrote:
> Hi, adding Aisheng,
>
> On Fri, Jan 17 2014, Andrew Bresticker wrote:
>> On Fri, Jan 17, 2014 at 3:11 PM, John Tobias <john.tobias.ph@gmail.com> wrote:
>>> There's an existing patch for that...
>>> http://www.spinics.net/lists/arm-kernel/msg296596.html
>>
>> Ah, I see. Looks like it has yet to be picked up...
>
> The patches aren't quite identical -- Andrew's leaves the
> disable_irq() call in and Aisheng's removes it. Which should I take?
Since the disable_irq() is now redundant, I suppose Aisheng's is more correct.
Thanks,
Andrew
>
> Thanks,
>
> - Chris.
> --
> Chris Ball <chris@printf.net> <http://printf.net/>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic
2014-01-18 3:21 ` Andrew Bresticker
@ 2014-01-18 3:40 ` Chris Ball
0 siblings, 0 replies; 10+ messages in thread
From: Chris Ball @ 2014-01-18 3:40 UTC (permalink / raw)
To: Andrew Bresticker
Cc: John Tobias, Philip Rakity, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org, Dong Aisheng
Hi,
On Sat, Jan 18 2014, Andrew Bresticker wrote:
>>>> There's an existing patch for that...
>>>> http://www.spinics.net/lists/arm-kernel/msg296596.html
>>>
>>> Ah, I see. Looks like it has yet to be picked up...
>>
>> The patches aren't quite identical -- Andrew's leaves the
>> disable_irq() call in and Aisheng's removes it. Which should I take?
>
> Since the disable_irq() is now redundant, I suppose Aisheng's is more correct
Thanks, pushed Aisheng's version to mmc-next for 3.14.
- Chris.
--
Chris Ball <chris@printf.net> <http://printf.net/>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic
@ 2014-01-20 9:51 Philip Rakity
2014-01-20 15:03 ` Philip Rakity
0 siblings, 1 reply; 10+ messages in thread
From: Philip Rakity @ 2014-01-20 9:51 UTC (permalink / raw)
To: Chris Ball; +Cc: linux-mmc@vger.kernel.org
Chris,
The suggested fix can lock out interrupts for up to 150ms. There needs to be another way.
So, I would strongly recommend we find another solution.
Philip
On Jan 17, 2014, at 7:57 PM, Andrew Bresticker <abrestic@chromium.org> wrote:
> sdhci_execute_tuning() takes host->lock without disabling interrupts.
> Use spin_lock_irq{save,restore} instead so that we avoid taking an
> interrupt and scheduling while holding host->lock.
>
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> ---
> drivers/mmc/host/sdhci.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ec3eb30..84c80e7 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1857,12 +1857,13 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> unsigned long timeout;
> int err = 0;
> bool requires_tuning_nonuhs = false;
> + unsigned long flags;
>
> host = mmc_priv(mmc);
>
> sdhci_runtime_pm_get(host);
> disable_irq(host->irq);
> - spin_lock(&host->lock);
> + spin_lock_irqsave(&host->lock, flags);
The disable_irq() call stops the controller from doing interrupts.
Please explain what problem you are seeing
>
> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>
> @@ -1882,14 +1883,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> requires_tuning_nonuhs)
> ctrl |= SDHCI_CTRL_EXEC_TUNING;
> else {
> - spin_unlock(&host->lock);
> + spin_unlock_irqrestore(&host->lock, flags);
> enable_irq(host->irq);
> sdhci_runtime_pm_put(host);
> return 0;
> }
>
> if (host->ops->platform_execute_tuning) {
> - spin_unlock(&host->lock);
> + spin_unlock_irqrestore(&host->lock, flags);
> enable_irq(host->irq);
> err = host->ops->platform_execute_tuning(host, opcode);
> sdhci_runtime_pm_put(host);
> @@ -1963,7 +1964,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> host->cmd = NULL;
> host->mrq = NULL;
>
> - spin_unlock(&host->lock);
> + spin_unlock_irqrestore(&host->lock, flags);
> enable_irq(host->irq);
>
> /* Wait for Buffer Read Ready interrupt */
> @@ -1971,7 +1972,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
> (host->tuning_done == 1),
> msecs_to_jiffies(50));
> disable_irq(host->irq);
> - spin_lock(&host->lock);
> + spin_lock_irqsave(&host->lock, flags);
>
> if (!host->tuning_done) {
> pr_info(DRIVER_NAME ": Timeout waiting for "
> @@ -2046,7 +2047,7 @@ out:
> err = 0;
>
> sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
> - spin_unlock(&host->lock);
> + spin_unlock_irqrestore(&host->lock, flags);
> enable_irq(host->irq);
> sdhci_runtime_pm_put(host);
>
> --
> 1.8.5.2
>
> --
> 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
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic
2014-01-20 9:51 Philip Rakity
@ 2014-01-20 15:03 ` Philip Rakity
0 siblings, 0 replies; 10+ messages in thread
From: Philip Rakity @ 2014-01-20 15:03 UTC (permalink / raw)
To: Chris Ball; +Cc: linux-mmc@vger.kernel.org
I have no way to test this problem but I was thinking along the lines of
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1862,6 +1862,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32
sdhci_runtime_pm_get(host);
disable_irq(host->irq);
+ sdhci_disable_card_detection(host);
spin_lock(&host->lock);
ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
@@ -1883,6 +1884,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32
ctrl |= SDHCI_CTRL_EXEC_TUNING;
else {
spin_unlock(&host->lock);
+ sdhci_enable_card_detection(host);
enable_irq(host->irq);
sdhci_runtime_pm_put(host);
return 0;
@@ -1890,6 +1892,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32
if (host->ops->platform_execute_tuning) {
spin_unlock(&host->lock);
+ sdhci_enable_card_detection(host);
enable_irq(host->irq);
err = host->ops->platform_execute_tuning(host, opcode);
sdhci_runtime_pm_put(host);
@@ -2047,6 +2050,7 @@ out:
sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
spin_unlock(&host->lock);
+ sdhci_enable_card_detection(host);
enable_irq(host->irq);
sdhci_runtime_pm_put(host);
regards,
Philip
On Jan 20, 2014, at 9:51 AM, Philip Rakity <prakity@nvidia.com> wrote:
>
> Chris,
>
> The suggested fix can lock out interrupts for up to 150ms. There needs to be another way.
> So, I would strongly recommend we find another solution.
>
> Philip
>
> On Jan 17, 2014, at 7:57 PM, Andrew Bresticker <abrestic@chromium.org> wrote:
>
>> sdhci_execute_tuning() takes host->lock without disabling interrupts.
>> Use spin_lock_irq{save,restore} instead so that we avoid taking an
>> interrupt and scheduling while holding host->lock.
>>
>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>> ---
>> drivers/mmc/host/sdhci.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ec3eb30..84c80e7 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1857,12 +1857,13 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> unsigned long timeout;
>> int err = 0;
>> bool requires_tuning_nonuhs = false;
>> + unsigned long flags;
>>
>> host = mmc_priv(mmc);
>>
>> sdhci_runtime_pm_get(host);
>> disable_irq(host->irq);
>> - spin_lock(&host->lock);
>> + spin_lock_irqsave(&host->lock, flags);
>
>
> The disable_irq() call stops the controller from doing interrupts.
> Please explain what problem you are seeing
>
>>
>> ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>
>> @@ -1882,14 +1883,14 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> requires_tuning_nonuhs)
>> ctrl |= SDHCI_CTRL_EXEC_TUNING;
>> else {
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>> sdhci_runtime_pm_put(host);
>> return 0;
>> }
>>
>> if (host->ops->platform_execute_tuning) {
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>> err = host->ops->platform_execute_tuning(host, opcode);
>> sdhci_runtime_pm_put(host);
>> @@ -1963,7 +1964,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> host->cmd = NULL;
>> host->mrq = NULL;
>>
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>>
>> /* Wait for Buffer Read Ready interrupt */
>> @@ -1971,7 +1972,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> (host->tuning_done == 1),
>> msecs_to_jiffies(50));
>> disable_irq(host->irq);
>> - spin_lock(&host->lock);
>> + spin_lock_irqsave(&host->lock, flags);
>>
>> if (!host->tuning_done) {
>> pr_info(DRIVER_NAME ": Timeout waiting for "
>> @@ -2046,7 +2047,7 @@ out:
>> err = 0;
>>
>> sdhci_clear_set_irqs(host, SDHCI_INT_DATA_AVAIL, ier);
>> - spin_unlock(&host->lock);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> enable_irq(host->irq);
>> sdhci_runtime_pm_put(host);
>>
>> --
>> 1.8.5.2
>>
>> --
>> 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
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-01-20 15:03 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-17 19:57 [PATCH] mmc: sdhci: fix possible scheduling while atomic Andrew Bresticker
2014-01-17 22:58 ` Philip Rakity
2014-01-17 23:10 ` Andrew Bresticker
2014-01-17 23:11 ` John Tobias
2014-01-17 23:16 ` Andrew Bresticker
2014-01-17 23:40 ` Chris Ball
2014-01-18 3:21 ` Andrew Bresticker
2014-01-18 3:40 ` Chris Ball
-- strict thread matches above, loose matches on Subject: below --
2014-01-20 9:51 Philip Rakity
2014-01-20 15:03 ` Philip Rakity
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).