Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Philip Rakity]

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.
-----------------------------------------------------------------------------------

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Philip Rakity]

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.
-----------------------------------------------------------------------------------

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Chris Ball]

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/>

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Chris Ball]

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/>

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Andrew Bresticker]

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/>

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Chris Ball]

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/>

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Chris Ball]

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/>

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Andrew Bresticker]

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/

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[John Tobias]

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/

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Andrew Bresticker]

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
>

Re: [PATCH] mmc: sdhci: fix possible scheduling while atomic

[Philip Rakity]

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

[PATCH] mmc: sdhci: fix possible scheduling while atomic

[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